threads / patch / 57419

patchrerere-train: modernise a bit

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

## tl;dr

8 messages between Feb 16, 2022 and Feb 28, 2022. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Junio C Hamano· Feb 16, 2022, 07:05 UTC · lore

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(-)
Show changes to diff +2 −7
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
Derrick Stolee· Feb 20, 2022, 19:52 UTC · re: Junio C Hamano · lore

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

On 2/16/2022 2:05 AM, Junio C Hamano wrote:
Show 9 quoted lines
> 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

Johannes Altmanninger· Feb 27, 2022, 18:02 UTC · re: Junio C Hamano · lore

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

On Tue, Feb 15, 2022 at 11:05:45PM -0800, Junio C Hamano wrote:
Show 7 quoted lines
> 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.
Show 35 quoted lines
> 
> 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
Junio C Hamano· Feb 27, 2022, 19:07 UTC · re: Johannes Altmanninger · lore

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

Johannes Altmanninger <aclopte@gmail.com> writes:
Show 5 quoted lines
> 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(-)
Show changes to diff +1 −1
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
Johannes Altmanninger· Feb 27, 2022, 20:23 UTC · re: Junio C Hamano · lore

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

On Sun, Feb 27, 2022 at 11:07:55AM -0800, Junio C Hamano wrote:
Show 12 quoted lines
> 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.

Show 45 quoted lines
> 
>     $ 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
Junio C Hamano· Feb 27, 2022, 22:13 UTC · re: Johannes Altmanninger · lore

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

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.
Junio C Hamano· Feb 27, 2022, 22:09 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to contrib/rerere-train.sh +1 −1
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
Johannes Altmanninger· Feb 28, 2022, 05:33 UTC · re: Junio C Hamano · lore

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

On Sun, Feb 27, 2022 at 02:09:24PM -0800, Junio C Hamano wrote:
Show 22 quoted lines
> 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).
Show 5 quoted lines
> 
> 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.

Show 20 quoted lines
> 
>  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
>

← back to recent threads