{"thread":{"id":"57419","subject":"[PATCH] rerere-train: modernise a bit","startedAt":"2022-02-16T07:11:24Z","lastAt":"2022-02-28T05:33:32Z","messageCount":8,"participants":["Junio C Hamano","Derrick Stolee","Johannes Altmanninger"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"448550","messageId":"xmqqsfsjuw8m.fsf@gitster.g","threadId":"57419","inReplyTo":null,"subject":"[PATCH] rerere-train: modernise a bit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-16T07:05:45Z","receivedAt":"2022-02-16T07:11:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The script wants to create a list of merges using \"rev-list\" and\nfilters commits that do not have more than one parent, but if we\nalways pass \"--merges\" to \"rev-list\", there is no need to filter.\n\nThe command uses \"git show --pretty=format:...\" on a single commit\nwhile generating progress reports, which means this title line is\nleft unterminated.  It should have used --pretty=tformat:...\ninstead, or better yet, use the more modern --format=... to ensure\nthat the title line is properly terminated.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n contrib/rerere-train.sh | 9 ++-------\n 1 file changed, 2 insertions(+), 7 deletions(-)\n\ndiff --git c/contrib/rerere-train.sh w/contrib/rerere-train.sh\nindex 75125d6ae0..499b07e4a6 100755\n--- c/contrib/rerere-train.sh\n+++ w/contrib/rerere-train.sh\n@@ -66,14 +66,9 @@ original_HEAD=$(git rev-parse --verify HEAD) || {\n \n mkdir -p \"$GIT_DIR/rr-cache\" || exit\n \n-git rev-list --parents \"$@\" |\n+git rev-list --parents --merges \"$@\" |\n while read commit parent1 other_parents\n do\n-\tif test -z \"$other_parents\"\n-\tthen\n-\t\t# Skip non-merges\n-\t\tcontinue\n-\tfi\n \tgit checkout -q \"$parent1^0\"\n \tif git merge $other_parents >/dev/null 2>&1\n \tthen\n@@ -86,7 +81,7 @@ do\n \tfi\n \tif test -s \"$GIT_DIR/MERGE_RR\"\n \tthen\n-\t\tgit show -s --pretty=format:\"Learning from %h %s\" \"$commit\"\n+\t\tgit show -s --format=\"Learning from %h %s\" \"$commit\"\n \t\tgit rerere\n \t\tgit checkout -q $commit -- .\n \t\tgit rerere\n"},{"id":"448920","messageId":"dfa34726-6133-01e5-c591-22f3ce1f8363@github.com","threadId":"57419","inReplyTo":"xmqqsfsjuw8m.fsf@gitster.g","subject":"Re: [PATCH] rerere-train: modernise a bit","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-02-20T19:52:30Z","receivedAt":"2022-02-20T19:52:35Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/16/2022 2:05 AM, Junio C Hamano wrote:\n> The script wants to create a list of merges using \"rev-list\" and\n> filters commits that do not have more than one parent, but if we\n> always pass \"--merges\" to \"rev-list\", there is no need to filter.\n> \n> The command uses \"git show --pretty=format:...\" on a single commit\n> while generating progress reports, which means this title line is\n> left unterminated.  It should have used --pretty=tformat:...\n> instead, or better yet, use the more modern --format=... to ensure\n> that the title line is properly terminated.\n\nI'm unfamiliar with the rerere-train.sh script, but the changes\nare pretty clearly achieving what you describe here.\n\nThanks,\n-Stolee\n"},{"id":"449702","messageId":"20220227180203.pakrqimsxbjx47tu@gmail.com","threadId":"57419","inReplyTo":"xmqqsfsjuw8m.fsf@gitster.g","subject":"Re: [PATCH] rerere-train: modernise a bit","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2022-02-27T18:02:03Z","receivedAt":"2022-02-27T18:02:22Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"On Tue, Feb 15, 2022 at 11:05:45PM -0800, Junio C Hamano wrote:\n> The script wants to create a list of merges using \"rev-list\" and\n> filters commits that do not have more than one parent, but if we\n> always pass \"--merges\" to \"rev-list\", there is no need to filter.\n> \n> The command uses \"git show --pretty=format:...\" on a single commit\n> while generating progress reports, which means this title line is\n> left unterminated.  It should have used --pretty=tformat:...\n\nYep, tformat is more correct semantically, but it's worth noting that there\nis no behavior change here. These commands behave the same\n\n\tgit show -s --pretty=tformat:\"Learning\" HEAD\n\tgit show -s --pretty=format:\"Learning\" HEAD\n\nI guess we automagically add a final newline somewhere, if it's missing.\n\nIf there is a final newline (\"Learning%n\"), then the commands show different\nbehavior. The subject (%s) can never have a newline, so that's not the\ncase here.\n\nI'd add something like this (for the lack of knowing where exactly the\nimplicit newline comes from):\n\n\tNo harm was done because we implicitly add the trailing newline,\n\tbut it should have used --pretty=tformat:...\n\n> instead, or better yet, use the more modern --format=... to ensure\n> that the title line is properly terminated.\n\n> \n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  contrib/rerere-train.sh | 9 ++-------\n>  1 file changed, 2 insertions(+), 7 deletions(-)\n> \n> diff --git c/contrib/rerere-train.sh w/contrib/rerere-train.sh\n> index 75125d6ae0..499b07e4a6 100755\n> --- c/contrib/rerere-train.sh\n> +++ w/contrib/rerere-train.sh\n> @@ -66,14 +66,9 @@ original_HEAD=$(git rev-parse --verify HEAD) || {\n>  \n>  mkdir -p \"$GIT_DIR/rr-cache\" || exit\n>  \n> -git rev-list --parents \"$@\" |\n> +git rev-list --parents --merges \"$@\" |\n>  while read commit parent1 other_parents\n>  do\n> -\tif test -z \"$other_parents\"\n> -\tthen\n> -\t\t# Skip non-merges\n> -\t\tcontinue\n> -\tfi\n>  \tgit checkout -q \"$parent1^0\"\n>  \tif git merge $other_parents >/dev/null 2>&1\n>  \tthen\n> @@ -86,7 +81,7 @@ do\n>  \tfi\n>  \tif test -s \"$GIT_DIR/MERGE_RR\"\n>  \tthen\n> -\t\tgit show -s --pretty=format:\"Learning from %h %s\" \"$commit\"\n> +\t\tgit show -s --format=\"Learning from %h %s\" \"$commit\"\n>  \t\tgit rerere\n>  \t\tgit checkout -q $commit -- .\n>  \t\tgit rerere\n"},{"id":"449703","messageId":"xmqqy21w3z78.fsf_-_@gitster.g","threadId":"57419","inReplyTo":"20220227180203.pakrqimsxbjx47tu@gmail.com","subject":"Re* [PATCH] rerere-train: modernise a bit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-27T19:07:55Z","receivedAt":"2022-02-27T19:08:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Altmanninger <aclopte@gmail.com> writes:\n\n> Yep, tformat is more correct semantically, but it's worth noting that there\n> is no behavior change here. These commands behave the same\n>\n> \tgit show -s --pretty=tformat:\"Learning\" HEAD\n> \tgit show -s --pretty=format:\"Learning\" HEAD\n\nYour observation is not quite right.\n\nThe difference between tformat and format does matter in practice,\nunless your pager is hiding the difference.\n\n    $ export GIT_PAGER=cat; # disable the pager\n    $ git show -s --pretty=format:\"%s\" HEAD; echo Q\n    The eighth batchQ\n    $ exit\n\nThis episode also exposes another bug in the rerere-train script,\ncaused by the fact that it lets GIT_PAGER to interfere.\n\n--- >8 ---\nSubject: rerere-train: prevent GIT_PAGER from pausing 'git show -s'\n\nThe script uses \"git show -s --format\" to display the title of the\nmerge commit being studied, without explicitly disabling the pager,\nwhich is not a safe thing to do in a script.\n\nFor example, when the pager is set to \"less\" with \"-SF\" options (-S\ntells the pager not to fold lines but allow horizontal scrolling to\nshow the overly long lines, -F tells the pager not to wait if the\noutput in its entirety is shown on a single page), and the title of\nthe merge commit is longer than the width of the terminal, the pager\nwill wait until the end-user tells it to quit after showing the\nsingle line.\n\nExplicitly disable the pager for this \"git show\" invocation to avoid\nthis.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n contrib/rerere-train.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git c/contrib/rerere-train.sh w/contrib/rerere-train.sh\nindex 499b07e4a6..2b9df7b6f2 100755\n--- c/contrib/rerere-train.sh\n+++ w/contrib/rerere-train.sh\n@@ -81,7 +81,7 @@ do\n \tfi\n \tif test -s \"$GIT_DIR/MERGE_RR\"\n \tthen\n-\t\tgit show -s --format=\"Learning from %h %s\" \"$commit\"\n+\t\tgit --no-pager show -s --format=\"Learning from %h %s\" \"$commit\"\n \t\tgit rerere\n \t\tgit checkout -q $commit -- .\n \t\tgit rerere\n"},{"id":"449712","messageId":"20220227202328.7afrpuaujgwsnmcy@gmail.com","threadId":"57419","inReplyTo":"xmqqy21w3z78.fsf_-_@gitster.g","subject":"Re: Re* [PATCH] rerere-train: modernise a bit","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2022-02-27T20:23:28Z","receivedAt":"2022-02-27T20:23:39Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"On Sun, Feb 27, 2022 at 11:07:55AM -0800, Junio C Hamano wrote:\n> Johannes Altmanninger <aclopte@gmail.com> writes:\n> \n> > Yep, tformat is more correct semantically, but it's worth noting that there\n> > is no behavior change here. These commands behave the same\n> >\n> > \tgit show -s --pretty=tformat:\"Learning\" HEAD\n> > \tgit show -s --pretty=format:\"Learning\" HEAD\n> \n> Your observation is not quite right.\n> \n> The difference between tformat and format does matter in practice,\n> unless your pager is hiding the difference.\n\nRight, I forgot about the pager.\nBoth patches LGTM then.\nThe --no-pager fix would have prevented my confusion, which is an argument\nfor placing it first.\n\n> \n>     $ export GIT_PAGER=cat; # disable the pager\n>     $ git show -s --pretty=format:\"%s\" HEAD; echo Q\n>     The eighth batchQ\n>     $ exit\n> \n> This episode also exposes another bug in the rerere-train script,\n> caused by the fact that it lets GIT_PAGER to interfere.\n> \n> --- >8 ---\n> Subject: rerere-train: prevent GIT_PAGER from pausing 'git show -s'\n> \n> The script uses \"git show -s --format\" to display the title of the\n> merge commit being studied, without explicitly disabling the pager,\n> which is not a safe thing to do in a script.\n> \n> For example, when the pager is set to \"less\" with \"-SF\" options (-S\n> tells the pager not to fold lines but allow horizontal scrolling to\n> show the overly long lines, -F tells the pager not to wait if the\n> output in its entirety is shown on a single page), and the title of\n> the merge commit is longer than the width of the terminal, the pager\n> will wait until the end-user tells it to quit after showing the\n> single line.\n> \n> Explicitly disable the pager for this \"git show\" invocation to avoid\n> this.\n> \n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  contrib/rerere-train.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git c/contrib/rerere-train.sh w/contrib/rerere-train.sh\n> index 499b07e4a6..2b9df7b6f2 100755\n> --- c/contrib/rerere-train.sh\n> +++ w/contrib/rerere-train.sh\n> @@ -81,7 +81,7 @@ do\n>  \tfi\n>  \tif test -s \"$GIT_DIR/MERGE_RR\"\n>  \tthen\n> -\t\tgit show -s --format=\"Learning from %h %s\" \"$commit\"\n> +\t\tgit --no-pager show -s --format=\"Learning from %h %s\" \"$commit\"\n>  \t\tgit rerere\n>  \t\tgit checkout -q $commit -- .\n>  \t\tgit rerere\n"},{"id":"449719","messageId":"20220227220924.2144325-1-gitster@pobox.com","threadId":"57419","inReplyTo":"xmqqsfsjuw8m.fsf@gitster.g","subject":"[PATCH v2] rerere-train: two fixes to the use of \"git show -s\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-27T22:09:24Z","receivedAt":"2022-02-27T22:12:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The script uses \"git show -s\" to display the title of the merge\ncommit being studied, without explicitly disabling the pager, which\nis not a safe thing to do in a script.\n\nFor example, when the pager is set to \"less\" with \"-SF\" options (-S\ntells the pager not to fold lines but allow horizontal scrolling to\nshow the overly long lines, -F tells the pager not to wait if the\noutput in its entirety is shown on a single page), and the title of\nthe merge commit is longer than the width of the terminal, the pager\nwill wait until the end-user tells it to quit after showing the\nsingle line.\n\nExplicitly disable the pager with this \"git show\" invocation to fix\nthis.\n\nThe command uses the \"--pretty=format:...\" format, which adds LF in\nbetween each pair of commits it outputs, which means that the label\nfor the merge being learned from will be followed by the next\nmessage on the same line.  \"--pretty=tformat:...\" is what we should\ninstead, which adds LF after each commit, or a more modern way to\nspell it, i.e. \"--format=...\".  This existing breakage becomes\neasier to see, now we no longer use the pager.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * Relative to the initial version, the \"--no-merges\" change has\n   been removed because the end user can still give --merges from\n   the command line and the filtering of merges done by the script\n   is still needed for correctness.\n\n contrib/rerere-train.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/rerere-train.sh b/contrib/rerere-train.sh\nindex 75125d6ae0..26b724c8c6 100755\n--- a/contrib/rerere-train.sh\n+++ b/contrib/rerere-train.sh\n@@ -86,7 +86,7 @@ do\n \tfi\n \tif test -s \"$GIT_DIR/MERGE_RR\"\n \tthen\n-\t\tgit show -s --pretty=format:\"Learning from %h %s\" \"$commit\"\n+\t\tgit --no-pager show -s --format=\"Learning from %h %s\" \"$commit\"\n \t\tgit rerere\n \t\tgit checkout -q $commit -- .\n \t\tgit rerere\n-- \n2.35.1-354-g715d08a9e5\n\n"},{"id":"449720","messageId":"xmqq5yp03qls.fsf@gitster.g","threadId":"57419","inReplyTo":"20220227202328.7afrpuaujgwsnmcy@gmail.com","subject":"Re: Re* [PATCH] rerere-train: modernise a bit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-27T22:13:35Z","receivedAt":"2022-02-27T22:13:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Altmanninger <aclopte@gmail.com> writes:\n\n> The --no-pager fix would have prevented my confusion, which is an argument\n> for placing it first.\n\nI would rather squash them into one.  \"--no-pager\" alone would give\nan apparent regression to people like you whose pager \"corrected\"\nthe output from the command, but with two changes squashed together,\nwe would not have to see any regression.\n\nThanks.\n\n"},{"id":"449722","messageId":"20220228053315.czkke7hfiav4qh3s@gmail.com","threadId":"57419","inReplyTo":"20220227220924.2144325-1-gitster@pobox.com","subject":"Re: [PATCH v2] rerere-train: two fixes to the use of \"git show -s\"","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2022-02-28T05:33:15Z","receivedAt":"2022-02-28T05:33:32Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"On Sun, Feb 27, 2022 at 02:09:24PM -0800, Junio C Hamano wrote:\n> The script uses \"git show -s\" to display the title of the merge\n> commit being studied, without explicitly disabling the pager, which\n> is not a safe thing to do in a script.\n> \n> For example, when the pager is set to \"less\" with \"-SF\" options (-S\n> tells the pager not to fold lines but allow horizontal scrolling to\n> show the overly long lines, -F tells the pager not to wait if the\n> output in its entirety is shown on a single page), and the title of\n> the merge commit is longer than the width of the terminal, the pager\n> will wait until the end-user tells it to quit after showing the\n> single line.\n> \n> Explicitly disable the pager with this \"git show\" invocation to fix\n> this.\n> \n> The command uses the \"--pretty=format:...\" format, which adds LF in\n> between each pair of commits it outputs, which means that the label\n> for the merge being learned from will be followed by the next\n> message on the same line.  \"--pretty=tformat:...\" is what we should\n> instead, which adds LF after each commit, or a more modern way to\n> spell it, i.e. \"--format=...\".  This existing breakage becomes\n> easier to see, now we no longer use the pager.\n\nSounds good (definitely better than two separate commits).\n\n> \n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n> \n>  * Relative to the initial version, the \"--no-merges\" change has\n\nit was \"--merges\", not \"--no-merges\"\n\n>    been removed because the end user can still give --merges from\n>    the command line and the filtering of merges done by the script\n>    is still needed for correctness.\n\nYou probably mean that the user can pass \"--no-merges HEAD\"\nbut that would just make the effective command\n\n\tgit rev-list --merges --no-merges HEAD\n\nwhich outputs nothing. I don't think `git rev-list --merges \"$@\"` will\never output non-merge commits, so the filtering should not be necessary.\n\n> \n>  contrib/rerere-train.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/contrib/rerere-train.sh b/contrib/rerere-train.sh\n> index 75125d6ae0..26b724c8c6 100755\n> --- a/contrib/rerere-train.sh\n> +++ b/contrib/rerere-train.sh\n> @@ -86,7 +86,7 @@ do\n>  \tfi\n>  \tif test -s \"$GIT_DIR/MERGE_RR\"\n>  \tthen\n> -\t\tgit show -s --pretty=format:\"Learning from %h %s\" \"$commit\"\n> +\t\tgit --no-pager show -s --format=\"Learning from %h %s\" \"$commit\"\n>  \t\tgit rerere\n>  \t\tgit checkout -q $commit -- .\n>  \t\tgit rerere\n> -- \n> 2.35.1-354-g715d08a9e5\n> \n"}]}