[PATCH] rerere-train: modernise a bit

[PATCH] rerere-train: modernise a bit

[Junio C Hamano]

The script wants to create a list of merges using "rev-list" and
filters commits that do not have more than one parent, but if we
always pass "--merges" to "rev-list", there is no need to filter.

The command uses "git show --pretty=format:..." on a single commit
while generating progress reports, which means this title line is
left unterminated.  It should have used --pretty=tformat:...
instead, or better yet, use the more modern --format=... to ensure
that the title line is properly terminated.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 contrib/rerere-train.sh | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git c/contrib/rerere-train.sh w/contrib/rerere-train.sh
index 75125d6ae0..499b07e4a6 100755
--- c/contrib/rerere-train.sh
+++ w/contrib/rerere-train.sh
@@ -66,14 +66,9 @@ original_HEAD=$(git rev-parse --verify HEAD) || {
 
 mkdir -p "$GIT_DIR/rr-cache" || exit
 
-git rev-list --parents "$@" |
+git rev-list --parents --merges "$@" |
 while read commit parent1 other_parents
 do
-	if test -z "$other_parents"
-	then
-		# Skip non-merges
-		continue
-	fi
 	git checkout -q "$parent1^0"
 	if git merge $other_parents >/dev/null 2>&1
 	then
@@ -86,7 +81,7 @@ do
 	fi
 	if test -s "$GIT_DIR/MERGE_RR"
 	then
-		git show -s --pretty=format:"Learning from %h %s" "$commit"
+		git show -s --format="Learning from %h %s" "$commit"
 		git rerere
 		git checkout -q $commit -- .
 		git rerere

Re: [PATCH] rerere-train: modernise a bit

[Derrick Stolee]

On 2/16/2022 2:05 AM, Junio C Hamano wrote:
> The script wants to create a list of merges using "rev-list" and
> filters commits that do not have more than one parent, but if we
> always pass "--merges" to "rev-list", there is no need to filter.
> 
> The command uses "git show --pretty=format:..." on a single commit
> while generating progress reports, which means this title line is
> left unterminated.  It should have used --pretty=tformat:...
> instead, or better yet, use the more modern --format=... to ensure
> that the title line is properly terminated.

I'm unfamiliar with the rerere-train.sh script, but the changes
are pretty clearly achieving what you describe here.

Thanks,
-Stolee

Re: [PATCH] rerere-train: modernise a bit

[Johannes Altmanninger]

On Tue, Feb 15, 2022 at 11:05:45PM -0800, Junio C Hamano wrote:
> The script wants to create a list of merges using "rev-list" and
> filters commits that do not have more than one parent, but if we
> always pass "--merges" to "rev-list", there is no need to filter.
> 
> The command uses "git show --pretty=format:..." on a single commit
> while generating progress reports, which means this title line is
> left unterminated.  It should have used --pretty=tformat:...

Yep, tformat is more correct semantically, but it's worth noting that there
is no behavior change here. These commands behave the same

	git show -s --pretty=tformat:"Learning" HEAD
	git show -s --pretty=format:"Learning" HEAD

I guess we automagically add a final newline somewhere, if it's missing.

If there is a final newline ("Learning%n"), then the commands show different
behavior. The subject (%s) can never have a newline, so that's not the
case here.

I'd add something like this (for the lack of knowing where exactly the
implicit newline comes from):

	No harm was done because we implicitly add the trailing newline,
	but it should have used --pretty=tformat:...

> instead, or better yet, use the more modern --format=... to ensure
> that the title line is properly terminated.

> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  contrib/rerere-train.sh | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git c/contrib/rerere-train.sh w/contrib/rerere-train.sh
> index 75125d6ae0..499b07e4a6 100755
> --- c/contrib/rerere-train.sh
> +++ w/contrib/rerere-train.sh
> @@ -66,14 +66,9 @@ original_HEAD=$(git rev-parse --verify HEAD) || {
>  
>  mkdir -p "$GIT_DIR/rr-cache" || exit
>  
> -git rev-list --parents "$@" |
> +git rev-list --parents --merges "$@" |
>  while read commit parent1 other_parents
>  do
> -	if test -z "$other_parents"
> -	then
> -		# Skip non-merges
> -		continue
> -	fi
>  	git checkout -q "$parent1^0"
>  	if git merge $other_parents >/dev/null 2>&1
>  	then
> @@ -86,7 +81,7 @@ do
>  	fi
>  	if test -s "$GIT_DIR/MERGE_RR"
>  	then
> -		git show -s --pretty=format:"Learning from %h %s" "$commit"
> +		git show -s --format="Learning from %h %s" "$commit"
>  		git rerere
>  		git checkout -q $commit -- .
>  		git rerere

Re* [PATCH] rerere-train: modernise a bit

[Junio C Hamano]

Johannes Altmanninger <aclopte@gmail.com> writes:

> Yep, tformat is more correct semantically, but it's worth noting that there
> is no behavior change here. These commands behave the same
>
> 	git show -s --pretty=tformat:"Learning" HEAD
> 	git show -s --pretty=format:"Learning" HEAD

Your observation is not quite right.

The difference between tformat and format does matter in practice,
unless your pager is hiding the difference.

    $ export GIT_PAGER=cat; # disable the pager
    $ git show -s --pretty=format:"%s" HEAD; echo Q
    The eighth batchQ
    $ exit

This episode also exposes another bug in the rerere-train script,
caused by the fact that it lets GIT_PAGER to interfere.

--- >8 ---
Subject: rerere-train: prevent GIT_PAGER from pausing 'git show -s'

The script uses "git show -s --format" to display the title of the
merge commit being studied, without explicitly disabling the pager,
which is not a safe thing to do in a script.

For example, when the pager is set to "less" with "-SF" options (-S
tells the pager not to fold lines but allow horizontal scrolling to
show the overly long lines, -F tells the pager not to wait if the
output in its entirety is shown on a single page), and the title of
the merge commit is longer than the width of the terminal, the pager
will wait until the end-user tells it to quit after showing the
single line.

Explicitly disable the pager for this "git show" invocation to avoid
this.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 contrib/rerere-train.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git c/contrib/rerere-train.sh w/contrib/rerere-train.sh
index 499b07e4a6..2b9df7b6f2 100755
--- c/contrib/rerere-train.sh
+++ w/contrib/rerere-train.sh
@@ -81,7 +81,7 @@ do
 	fi
 	if test -s "$GIT_DIR/MERGE_RR"
 	then
-		git show -s --format="Learning from %h %s" "$commit"
+		git --no-pager show -s --format="Learning from %h %s" "$commit"
 		git rerere
 		git checkout -q $commit -- .
 		git rerere

Re: Re* [PATCH] rerere-train: modernise a bit

[Johannes Altmanninger]

On Sun, Feb 27, 2022 at 11:07:55AM -0800, Junio C Hamano wrote:
> Johannes Altmanninger <aclopte@gmail.com> writes:
> 
> > Yep, tformat is more correct semantically, but it's worth noting that there
> > is no behavior change here. These commands behave the same
> >
> > 	git show -s --pretty=tformat:"Learning" HEAD
> > 	git show -s --pretty=format:"Learning" HEAD
> 
> Your observation is not quite right.
> 
> The difference between tformat and format does matter in practice,
> unless your pager is hiding the difference.

Right, I forgot about the pager.
Both patches LGTM then.
The --no-pager fix would have prevented my confusion, which is an argument
for placing it first.

> 
>     $ export GIT_PAGER=cat; # disable the pager
>     $ git show -s --pretty=format:"%s" HEAD; echo Q
>     The eighth batchQ
>     $ exit
> 
> This episode also exposes another bug in the rerere-train script,
> caused by the fact that it lets GIT_PAGER to interfere.
> 
> --- >8 ---
> Subject: rerere-train: prevent GIT_PAGER from pausing 'git show -s'
> 
> The script uses "git show -s --format" to display the title of the
> merge commit being studied, without explicitly disabling the pager,
> which is not a safe thing to do in a script.
> 
> For example, when the pager is set to "less" with "-SF" options (-S
> tells the pager not to fold lines but allow horizontal scrolling to
> show the overly long lines, -F tells the pager not to wait if the
> output in its entirety is shown on a single page), and the title of
> the merge commit is longer than the width of the terminal, the pager
> will wait until the end-user tells it to quit after showing the
> single line.
> 
> Explicitly disable the pager for this "git show" invocation to avoid
> this.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  contrib/rerere-train.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git c/contrib/rerere-train.sh w/contrib/rerere-train.sh
> index 499b07e4a6..2b9df7b6f2 100755
> --- c/contrib/rerere-train.sh
> +++ w/contrib/rerere-train.sh
> @@ -81,7 +81,7 @@ do
>  	fi
>  	if test -s "$GIT_DIR/MERGE_RR"
>  	then
> -		git show -s --format="Learning from %h %s" "$commit"
> +		git --no-pager show -s --format="Learning from %h %s" "$commit"
>  		git rerere
>  		git checkout -q $commit -- .
>  		git rerere

[PATCH v2] rerere-train: two fixes to the use of "git show -s"

[Junio C Hamano]

The script uses "git show -s" to display the title of the merge
commit being studied, without explicitly disabling the pager, which
is not a safe thing to do in a script.

For example, when the pager is set to "less" with "-SF" options (-S
tells the pager not to fold lines but allow horizontal scrolling to
show the overly long lines, -F tells the pager not to wait if the
output in its entirety is shown on a single page), and the title of
the merge commit is longer than the width of the terminal, the pager
will wait until the end-user tells it to quit after showing the
single line.

Explicitly disable the pager with this "git show" invocation to fix
this.

The command uses the "--pretty=format:..." format, which adds LF in
between each pair of commits it outputs, which means that the label
for the merge being learned from will be followed by the next
message on the same line.  "--pretty=tformat:..." is what we should
instead, which adds LF after each commit, or a more modern way to
spell it, i.e. "--format=...".  This existing breakage becomes
easier to see, now we no longer use the pager.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * Relative to the initial version, the "--no-merges" change has
   been removed because the end user can still give --merges from
   the command line and the filtering of merges done by the script
   is still needed for correctness.

 contrib/rerere-train.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/rerere-train.sh b/contrib/rerere-train.sh
index 75125d6ae0..26b724c8c6 100755
--- a/contrib/rerere-train.sh
+++ b/contrib/rerere-train.sh
@@ -86,7 +86,7 @@ do
 	fi
 	if test -s "$GIT_DIR/MERGE_RR"
 	then
-		git show -s --pretty=format:"Learning from %h %s" "$commit"
+		git --no-pager show -s --format="Learning from %h %s" "$commit"
 		git rerere
 		git checkout -q $commit -- .
 		git rerere
-- 
2.35.1-354-g715d08a9e5

Re: Re* [PATCH] rerere-train: modernise a bit

[Junio C Hamano]

Johannes Altmanninger <aclopte@gmail.com> writes:

> The --no-pager fix would have prevented my confusion, which is an argument
> for placing it first.

I would rather squash them into one.  "--no-pager" alone would give
an apparent regression to people like you whose pager "corrected"
the output from the command, but with two changes squashed together,
we would not have to see any regression.

Thanks.

Re: [PATCH v2] rerere-train: two fixes to the use of "git show -s"

[Johannes Altmanninger]

On Sun, Feb 27, 2022 at 02:09:24PM -0800, Junio C Hamano wrote:
> The script uses "git show -s" to display the title of the merge
> commit being studied, without explicitly disabling the pager, which
> is not a safe thing to do in a script.
> 
> For example, when the pager is set to "less" with "-SF" options (-S
> tells the pager not to fold lines but allow horizontal scrolling to
> show the overly long lines, -F tells the pager not to wait if the
> output in its entirety is shown on a single page), and the title of
> the merge commit is longer than the width of the terminal, the pager
> will wait until the end-user tells it to quit after showing the
> single line.
> 
> Explicitly disable the pager with this "git show" invocation to fix
> this.
> 
> The command uses the "--pretty=format:..." format, which adds LF in
> between each pair of commits it outputs, which means that the label
> for the merge being learned from will be followed by the next
> message on the same line.  "--pretty=tformat:..." is what we should
> instead, which adds LF after each commit, or a more modern way to
> spell it, i.e. "--format=...".  This existing breakage becomes
> easier to see, now we no longer use the pager.

Sounds good (definitely better than two separate commits).

> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> 
>  * Relative to the initial version, the "--no-merges" change has

it was "--merges", not "--no-merges"

>    been removed because the end user can still give --merges from
>    the command line and the filtering of merges done by the script
>    is still needed for correctness.

You probably mean that the user can pass "--no-merges HEAD"
but that would just make the effective command

	git rev-list --merges --no-merges HEAD

which outputs nothing. I don't think `git rev-list --merges "$@"` will
ever output non-merge commits, so the filtering should not be necessary.

> 
>  contrib/rerere-train.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/contrib/rerere-train.sh b/contrib/rerere-train.sh
> index 75125d6ae0..26b724c8c6 100755
> --- a/contrib/rerere-train.sh
> +++ b/contrib/rerere-train.sh
> @@ -86,7 +86,7 @@ do
>  	fi
>  	if test -s "$GIT_DIR/MERGE_RR"
>  	then
> -		git show -s --pretty=format:"Learning from %h %s" "$commit"
> +		git --no-pager show -s --format="Learning from %h %s" "$commit"
>  		git rerere
>  		git checkout -q $commit -- .
>  		git rerere
> -- 
> 2.35.1-354-g715d08a9e5
>