[PATCH 0/5] merge-tree --stdin: flush stdout

[PATCH 0/5] merge-tree --stdin: flush stdout

[Phillip Wood via GitGitGadget]

I tried to squash some fixup commits with "git merge-tree --stdin" and found
that my script deadlocked because the output of "git merge-tree" is not
flushed after each merge. The first patch fixes that and the rest are
cleanups I noticed while reading the code and documentation. This series is
based on maint.

Phillip Wood (5):
  merge-tree --stdin: flush stdout to avoid deadlock
  merge-tree: remove redundant code
  merge-tree: only use basic merge config
  merge-tree: improve docs for --stdin
  merge-tree: fix link formatting in html docs

 Documentation/git-merge-tree.txt | 11 ++++++++---
 builtin/merge-tree.c             | 11 +++++------
 2 files changed, 13 insertions(+), 9 deletions(-)


base-commit: f93ff170b93a1782659637824b25923245ac9dd1
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1862%2Fphillipwood%2Fmerge-tree-flush-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1862/phillipwood/merge-tree-flush-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1862
-- 
gitgitgadget

[PATCH 1/5] merge-tree --stdin: flush stdout to avoid deadlock

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

If a process tries to read the output from "git merge-tree --stdin"
before it closes merge-tree's stdin then it deadlocks. This happens
because merge-tree does not flush its output before trying to read
another line of input and means that it is not possible to cherry-pick a
sequence of commits using "git merge-tree --stdin". Fix this by calling
maybe_flush_or_die() before trying to read the next line of
input. Flushing the output after each merge does not seem to affect the
performance, any difference is lost in the noise even after increasing
the number of runs.

$ git rev-list --merges --parents -n100 origin/master |
	sed 's/^[^ ]* //' >/tmp/merges
$ hyperfine -L flush 0,1 --warmup 1 --runs 30 \
	'GIT_FLUSH={flush} ./git merge-tree --stdin </tmp/merges'
Benchmark 1: GIT_FLUSH=0 ./git merge-tree --stdin </tmp/merges
  Time (mean ± σ):     546.6 ms ±  11.7 ms    [User: 503.2 ms, System: 40.9 ms]
  Range (min … max):   535.9 ms … 567.7 ms    30 runs

Benchmark 2: GIT_FLUSH=1 ./git merge-tree --stdin </tmp/merges
  Time (mean ± σ):     546.9 ms ±  12.0 ms    [User: 505.9 ms, System: 38.9 ms]
  Range (min … max):   529.8 ms … 570.0 ms    30 runs

Summary
  'GIT_FLUSH=0 ./git merge-tree --stdin </tmp/merges' ran
    1.00 ± 0.03 times faster than 'GIT_FLUSH=1 ./git merge-tree --stdin </tmp/merges'

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/merge-tree.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 9a6c8b4e4cf..57f4340faba 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -18,6 +18,7 @@
 #include "tree.h"
 #include "config.h"
 #include "strvec.h"
+#include "write-or-die.h"
 
 static int line_termination = '\n';
 
@@ -623,6 +624,7 @@ int cmd_merge_tree(int argc,
 			} else {
 				die(_("malformed input line: '%s'."), buf.buf);
 			}
+			maybe_flush_or_die(stdout, "stdout");
 
 			if (result < 0)
 				die(_("merging cannot continue; got unclean result of %d"), result);
-- 
gitgitgadget

[PATCH 2/5] merge-tree: remove redundant code

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

real_merge() only ever returns "0" or "1" as it dies if the merge status
is less than zero. Therefore the check for "result < 0" is redundant and
the result variable is not needed.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/merge-tree.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 57f4340faba..3c73482f2b0 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -601,7 +601,6 @@ int cmd_merge_tree(int argc,
 		line_termination = '\0';
 		while (strbuf_getline_lf(&buf, stdin) != EOF) {
 			struct strbuf **split;
-			int result;
 			const char *input_merge_base = NULL;
 
 			split = strbuf_split(&buf, ' ');
@@ -618,16 +617,14 @@ int cmd_merge_tree(int argc,
 			if (input_merge_base && split[2] && split[3] && !split[4]) {
 				strbuf_rtrim(split[2]);
 				strbuf_rtrim(split[3]);
-				result = real_merge(&o, input_merge_base, split[2]->buf, split[3]->buf, prefix);
+				real_merge(&o, input_merge_base, split[2]->buf, split[3]->buf, prefix);
 			} else if (!input_merge_base && !split[2]) {
-				result = real_merge(&o, NULL, split[0]->buf, split[1]->buf, prefix);
+				real_merge(&o, NULL, split[0]->buf, split[1]->buf, prefix);
 			} else {
 				die(_("malformed input line: '%s'."), buf.buf);
 			}
 			maybe_flush_or_die(stdout, "stdout");
 
-			if (result < 0)
-				die(_("merging cannot continue; got unclean result of %d"), result);
 			strbuf_list_free(split);
 		}
 		strbuf_release(&buf);
-- 
gitgitgadget

[PATCH 3/5] merge-tree: only use basic merge config

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Commit 9c93ba4d0ae (merge-recursive: honor diff.algorithm, 2024-07-13)
replaced init_merge_options() with init_basic_merge_config() for use in
plumbing commands and init_ui_merge_config() for use in porcelain
commands. As "git merge-tree" is a plumbing command it should call
init_basic_merge_config() rather than init_ui_merge_config(). The merge
ort machinery ignores "diff.algorithm" so the behavior is unchanged by
this commit but it future proofs us against any future changes to
init_ui_merge_config().

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/merge-tree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 3c73482f2b0..3ec7127b3a6 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -576,7 +576,7 @@ int cmd_merge_tree(int argc,
 	};
 
 	/* Init merge options */
-	init_ui_merge_options(&o.merge_options, the_repository);
+	init_basic_merge_options(&o.merge_options, the_repository);
 
 	/* Parse arguments */
 	original_argc = argc - 1; /* ignoring argv[0] */
-- 
gitgitgadget

[PATCH 4/5] merge-tree: improve docs for --stdin

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Add a section for --stdin in the list of options and document that it
implies -z so readers know how to parse the output. Also correct the
merge status documentation for --stdin as if the status is less than
zero "git merge-tree" dies before printing it.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 Documentation/git-merge-tree.txt | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
index 0b6a8a19b1f..efb16b4f27d 100644
--- a/Documentation/git-merge-tree.txt
+++ b/Documentation/git-merge-tree.txt
@@ -40,6 +40,11 @@ After the merge completes, a new toplevel tree object is created.  See
 OPTIONS
 -------
 
+--stdin::
+	Read the commits to merge from the standard input rather than
+	the command-line. See <<INPUT,INPUT FORMAT>> below for more
+	information.  Implies `-z`.
+
 -z::
 	Do not quote filenames in the <Conflicted file info> section,
 	and end each filename with a NUL character rather than
@@ -116,8 +121,6 @@ This is an integer status followed by a NUL character.  The integer status is:
 
      0: merge had conflicts
      1: merge was clean
-     <0: something prevented the merge from running (e.g. access to repository
-	 objects denied by filesystem)
 
 [[OIDTLT]]
 OID of toplevel tree
@@ -235,6 +238,7 @@ with linkgit:git-merge[1]:
   * any messages that would have been printed to stdout (the
     <<IM,Informational messages>>)
 
+[[INPUT]]
 INPUT FORMAT
 ------------
 'git merge-tree --stdin' input format is fully text based. Each line
-- 
gitgitgadget

[PATCH 5/5] merge-tree: fix link formatting in html docs

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

In the html documentation the link to the "OUTPUT" section is surrounded
by square brackets. Fix this by adding explicit link text to the cross
reference.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 Documentation/git-merge-tree.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
index efb16b4f27d..cf0578f9b5e 100644
--- a/Documentation/git-merge-tree.txt
+++ b/Documentation/git-merge-tree.txt
@@ -49,7 +49,8 @@ OPTIONS
 	Do not quote filenames in the <Conflicted file info> section,
 	and end each filename with a NUL character rather than
 	newline.  Also begin the messages section with a NUL character
-	instead of a newline.  See <<OUTPUT>> below for more information.
+	instead of a newline.  See <<OUTPUT,OUTPUT>> below for more
+	information.
 
 --name-only::
 	In the Conflicted file info section, instead of writing a list
-- 
gitgitgadget

Re: [PATCH 1/5] merge-tree --stdin: flush stdout to avoid deadlock

[Elijah Newren]

On Sun, Feb 16, 2025 at 8:37 AM Phillip Wood via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> If a process tries to read the output from "git merge-tree --stdin"
> before it closes merge-tree's stdin then it deadlocks. This happens
> because merge-tree does not flush its output before trying to read
> another line of input and means that it is not possible to cherry-pick a
> sequence of commits using "git merge-tree --stdin". Fix this by calling
> maybe_flush_or_die() before trying to read the next line of
> input.

Makes sense.

> Flushing the output after each merge does not seem to affect the
> performance, any difference is lost in the noise even after increasing
> the number of runs.
>
> $ git rev-list --merges --parents -n100 origin/master |
>         sed 's/^[^ ]* //' >/tmp/merges
> $ hyperfine -L flush 0,1 --warmup 1 --runs 30 \
>         'GIT_FLUSH={flush} ./git merge-tree --stdin </tmp/merges'
> Benchmark 1: GIT_FLUSH=0 ./git merge-tree --stdin </tmp/merges
>   Time (mean ± σ):     546.6 ms ±  11.7 ms    [User: 503.2 ms, System: 40.9 ms]
>   Range (min … max):   535.9 ms … 567.7 ms    30 runs
>
> Benchmark 2: GIT_FLUSH=1 ./git merge-tree --stdin </tmp/merges
>   Time (mean ± σ):     546.9 ms ±  12.0 ms    [User: 505.9 ms, System: 38.9 ms]
>   Range (min … max):   529.8 ms … 570.0 ms    30 runs
>
> Summary
>   'GIT_FLUSH=0 ./git merge-tree --stdin </tmp/merges' ran
>     1.00 ± 0.03 times faster than 'GIT_FLUSH=1 ./git merge-tree --stdin </tmp/merges'

Nice; thanks for checking and providing these stats.

> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/merge-tree.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> index 9a6c8b4e4cf..57f4340faba 100644
> --- a/builtin/merge-tree.c
> +++ b/builtin/merge-tree.c
> @@ -18,6 +18,7 @@
>  #include "tree.h"
>  #include "config.h"
>  #include "strvec.h"
> +#include "write-or-die.h"
>
>  static int line_termination = '\n';
>
> @@ -623,6 +624,7 @@ int cmd_merge_tree(int argc,
>                         } else {
>                                 die(_("malformed input line: '%s'."), buf.buf);
>                         }
> +                       maybe_flush_or_die(stdout, "stdout");
>
>                         if (result < 0)
>                                 die(_("merging cannot continue; got unclean result of %d"), result);
> --
> gitgitgadget

Looks good to me.

Re: [PATCH 2/5] merge-tree: remove redundant code

[Elijah Newren]

On Sun, Feb 16, 2025 at 8:37 AM Phillip Wood via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> real_merge() only ever returns "0" or "1" as it dies if the merge status
> is less than zero. Therefore the check for "result < 0" is redundant and
> the result variable is not needed.

Indeed, the only return statement in real_merge(), occurring on the
last line of the function, is even:
    return !result.clean; /* result.clean < 0 handled above */

However, it might be worth adding to the commit message some comments
about o->use_stdin here.  When o->use_stdin is true, that the program
exit status is 0 for both successful merges and conflicts but the
conflict status for each individual commit is printed as part of the
output.  As such, the return status isn't used in those cases and
real_merge() might as well be a void function.  However, when
o->use_stdin is false, the exit status from real_merge is used, which
is why that callsite (not visibile in this patch since it is
unmodified) still pays attention to real_merge()'s return status.

> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/merge-tree.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> index 57f4340faba..3c73482f2b0 100644
> --- a/builtin/merge-tree.c
> +++ b/builtin/merge-tree.c
> @@ -601,7 +601,6 @@ int cmd_merge_tree(int argc,
>                 line_termination = '\0';
>                 while (strbuf_getline_lf(&buf, stdin) != EOF) {
>                         struct strbuf **split;
> -                       int result;
>                         const char *input_merge_base = NULL;
>
>                         split = strbuf_split(&buf, ' ');
> @@ -618,16 +617,14 @@ int cmd_merge_tree(int argc,
>                         if (input_merge_base && split[2] && split[3] && !split[4]) {
>                                 strbuf_rtrim(split[2]);
>                                 strbuf_rtrim(split[3]);
> -                               result = real_merge(&o, input_merge_base, split[2]->buf, split[3]->buf, prefix);
> +                               real_merge(&o, input_merge_base, split[2]->buf, split[3]->buf, prefix);
>                         } else if (!input_merge_base && !split[2]) {
> -                               result = real_merge(&o, NULL, split[0]->buf, split[1]->buf, prefix);
> +                               real_merge(&o, NULL, split[0]->buf, split[1]->buf, prefix);
>                         } else {
>                                 die(_("malformed input line: '%s'."), buf.buf);
>                         }
>                         maybe_flush_or_die(stdout, "stdout");
>
> -                       if (result < 0)
> -                               die(_("merging cannot continue; got unclean result of %d"), result);
>                         strbuf_list_free(split);
>                 }
>                 strbuf_release(&buf);
> --
> gitgitgadget

Looks good.

Re: [PATCH 4/5] merge-tree: improve docs for --stdin

[Elijah Newren]

On Sun, Feb 16, 2025 at 8:37 AM Phillip Wood via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Add a section for --stdin in the list of options and document that it
> implies -z so readers know how to parse the output.

Makes sense.

> Also correct the
> merge status documentation for --stdin as if the status is less than
> zero "git merge-tree" dies before printing it.

This also makes sense, but...die'ing still has an exit status
associated with it right?

> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  Documentation/git-merge-tree.txt | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
> index 0b6a8a19b1f..efb16b4f27d 100644
> --- a/Documentation/git-merge-tree.txt
> +++ b/Documentation/git-merge-tree.txt
> @@ -40,6 +40,11 @@ After the merge completes, a new toplevel tree object is created.  See
>  OPTIONS
>  -------
>
> +--stdin::
> +       Read the commits to merge from the standard input rather than
> +       the command-line. See <<INPUT,INPUT FORMAT>> below for more
> +       information.  Implies `-z`.
> +
>  -z::
>         Do not quote filenames in the <Conflicted file info> section,
>         and end each filename with a NUL character rather than
> @@ -116,8 +121,6 @@ This is an integer status followed by a NUL character.  The integer status is:
>
>       0: merge had conflicts
>       1: merge was clean
> -     <0: something prevented the merge from running (e.g. access to repository
> -        objects denied by filesystem)

Should this line be kept but replace "<0" with "128" (the exit status of die)?

>
>  [[OIDTLT]]
>  OID of toplevel tree
> @@ -235,6 +238,7 @@ with linkgit:git-merge[1]:
>    * any messages that would have been printed to stdout (the
>      <<IM,Informational messages>>)
>
> +[[INPUT]]
>  INPUT FORMAT
>  ------------
>  'git merge-tree --stdin' input format is fully text based. Each line
> --
> gitgitgadget

Re: [PATCH 5/5] merge-tree: fix link formatting in html docs

[Elijah Newren]

On Sun, Feb 16, 2025 at 8:37 AM Phillip Wood via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> In the html documentation the link to the "OUTPUT" section is surrounded
> by square brackets. Fix this by adding explicit link text to the cross
> reference.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  Documentation/git-merge-tree.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
> index efb16b4f27d..cf0578f9b5e 100644
> --- a/Documentation/git-merge-tree.txt
> +++ b/Documentation/git-merge-tree.txt
> @@ -49,7 +49,8 @@ OPTIONS
>         Do not quote filenames in the <Conflicted file info> section,
>         and end each filename with a NUL character rather than
>         newline.  Also begin the messages section with a NUL character
> -       instead of a newline.  See <<OUTPUT>> below for more information.
> +       instead of a newline.  See <<OUTPUT,OUTPUT>> below for more
> +       information.
>
>  --name-only::
>         In the Conflicted file info section, instead of writing a list
> --
> gitgitgadget

Seems to be the only line in git-merge-tree.txt matching <<.*>> which
also matches <<[,]*>>; i.e. looks like the only case that needed to be
fixed.  Thanks for fixing it.

Re: [PATCH 2/5] merge-tree: remove redundant code

[Phillip Wood]

Hi Elijah

On 17/02/2025 20:15, Elijah Newren wrote:
> On Sun, Feb 16, 2025 at 8:37 AM Phillip Wood via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> real_merge() only ever returns "0" or "1" as it dies if the merge status
>> is less than zero. Therefore the check for "result < 0" is redundant and
>> the result variable is not needed.
> 
> Indeed, the only return statement in real_merge(), occurring on the
> last line of the function, is even:
>      return !result.clean; /* result.clean < 0 handled above */
> 
> However, it might be worth adding to the commit message some comments
> about o->use_stdin here.  When o->use_stdin is true, that the program
> exit status is 0 for both successful merges and conflicts but the
> conflict status for each individual commit is printed as part of the
> output.  As such, the return status isn't used in those cases and
> real_merge() might as well be a void function.  However, when
> o->use_stdin is false, the exit status from real_merge is used, which
> is why that callsite (not visibile in this patch since it is
> unmodified) still pays attention to real_merge()'s return status.

That's a good suggestion - I'll re-roll

Thanks

Phillip

>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>   builtin/merge-tree.c | 7 ++-----
>>   1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
>> index 57f4340faba..3c73482f2b0 100644
>> --- a/builtin/merge-tree.c
>> +++ b/builtin/merge-tree.c
>> @@ -601,7 +601,6 @@ int cmd_merge_tree(int argc,
>>                  line_termination = '\0';
>>                  while (strbuf_getline_lf(&buf, stdin) != EOF) {
>>                          struct strbuf **split;
>> -                       int result;
>>                          const char *input_merge_base = NULL;
>>
>>                          split = strbuf_split(&buf, ' ');
>> @@ -618,16 +617,14 @@ int cmd_merge_tree(int argc,
>>                          if (input_merge_base && split[2] && split[3] && !split[4]) {
>>                                  strbuf_rtrim(split[2]);
>>                                  strbuf_rtrim(split[3]);
>> -                               result = real_merge(&o, input_merge_base, split[2]->buf, split[3]->buf, prefix);
>> +                               real_merge(&o, input_merge_base, split[2]->buf, split[3]->buf, prefix);
>>                          } else if (!input_merge_base && !split[2]) {
>> -                               result = real_merge(&o, NULL, split[0]->buf, split[1]->buf, prefix);
>> +                               real_merge(&o, NULL, split[0]->buf, split[1]->buf, prefix);
>>                          } else {
>>                                  die(_("malformed input line: '%s'."), buf.buf);
>>                          }
>>                          maybe_flush_or_die(stdout, "stdout");
>>
>> -                       if (result < 0)
>> -                               die(_("merging cannot continue; got unclean result of %d"), result);
>>                          strbuf_list_free(split);
>>                  }
>>                  strbuf_release(&buf);
>> --
>> gitgitgadget
> 
> Looks good.
> 

Re: [PATCH 4/5] merge-tree: improve docs for --stdin

[Phillip Wood]

Hi Elijah

On 17/02/2025 20:26, Elijah Newren wrote:
> On Sun, Feb 16, 2025 at 8:37 AM Phillip Wood via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> Also correct the
>> merge status documentation for --stdin as if the status is less than
>> zero "git merge-tree" dies before printing it.
> 
> This also makes sense, but...die'ing still has an exit status
> associated with it right?

It does, but that is documented in a separate section which says that if 
there is an error it exits with a code that isn't 0 or 1. The section 
I've altered is documenting what "git merge-tree --stdin" prints to 
stdout and if result.clean is less than zero then it dies it does not 
print anything to stdout.

Best Wishes

Phillip

>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>   Documentation/git-merge-tree.txt | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
>> index 0b6a8a19b1f..efb16b4f27d 100644
>> --- a/Documentation/git-merge-tree.txt
>> +++ b/Documentation/git-merge-tree.txt
>> @@ -40,6 +40,11 @@ After the merge completes, a new toplevel tree object is created.  See
>>   OPTIONS
>>   -------
>>
>> +--stdin::
>> +       Read the commits to merge from the standard input rather than
>> +       the command-line. See <<INPUT,INPUT FORMAT>> below for more
>> +       information.  Implies `-z`.
>> +
>>   -z::
>>          Do not quote filenames in the <Conflicted file info> section,
>>          and end each filename with a NUL character rather than
>> @@ -116,8 +121,6 @@ This is an integer status followed by a NUL character.  The integer status is:
>>
>>        0: merge had conflicts
>>        1: merge was clean
>> -     <0: something prevented the merge from running (e.g. access to repository
>> -        objects denied by filesystem)
> 
> Should this line be kept but replace "<0" with "128" (the exit status of die)?
> 
>>
>>   [[OIDTLT]]
>>   OID of toplevel tree
>> @@ -235,6 +238,7 @@ with linkgit:git-merge[1]:
>>     * any messages that would have been printed to stdout (the
>>       <<IM,Informational messages>>)
>>
>> +[[INPUT]]
>>   INPUT FORMAT
>>   ------------
>>   'git merge-tree --stdin' input format is fully text based. Each line
>> --
>> gitgitgadget
> 

Re: [PATCH 4/5] merge-tree: improve docs for --stdin

[Elijah Newren]

On Tue, Feb 18, 2025 at 2:02 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Elijah
>
> On 17/02/2025 20:26, Elijah Newren wrote:
> > On Sun, Feb 16, 2025 at 8:37 AM Phillip Wood via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >>
> >> Also correct the
> >> merge status documentation for --stdin as if the status is less than
> >> zero "git merge-tree" dies before printing it.
> >
> > This also makes sense, but...die'ing still has an exit status
> > associated with it right?
>
> It does, but that is documented in a separate section which says that if
> there is an error it exits with a code that isn't 0 or 1. The section
> I've altered is documenting what "git merge-tree --stdin" prints to
> stdout and if result.clean is less than zero then it dies it does not
> print anything to stdout.
>
> Best Wishes
>
> Phillip

Oh, right, so your patch is good then.  Thanks for pointing out what I missed.

[PATCH v2 0/5] merge-tree --stdin: flush stdout

[Phillip Wood via GitGitGadget]

Thanks to Elijah for his comments on V1. I've updated the commit message of
patch 2 as he suggested. The rest of the patches are unchanged.

V1 Cover Letter:

I tried to squash some fixup commits with "git merge-tree --stdin" and found
that my script deadlocked because the output of "git merge-tree" is not
flushed after each merge. The first patch fixes that and the rest are
cleanups I noticed while reading the code and documentation. This series is
based on maint.

Phillip Wood (5):
  merge-tree --stdin: flush stdout to avoid deadlock
  merge-tree: remove redundant code
  merge-tree: only use basic merge config
  merge-tree: improve docs for --stdin
  merge-tree: fix link formatting in html docs

 Documentation/git-merge-tree.txt | 11 ++++++++---
 builtin/merge-tree.c             | 11 +++++------
 2 files changed, 13 insertions(+), 9 deletions(-)


base-commit: f93ff170b93a1782659637824b25923245ac9dd1
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1862%2Fphillipwood%2Fmerge-tree-flush-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1862/phillipwood/merge-tree-flush-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1862

Range-diff vs v1:

 1:  3b317978509 = 1:  3b317978509 merge-tree --stdin: flush stdout to avoid deadlock
 2:  16fec87766f ! 2:  63b09dbe1b7 merge-tree: remove redundant code
     @@ Commit message
      
          real_merge() only ever returns "0" or "1" as it dies if the merge status
          is less than zero. Therefore the check for "result < 0" is redundant and
     -    the result variable is not needed.
     +    the result variable is not needed. The return value of real_merge() is
     +    ignored because exit status of "git merge-tree --stdin" is "0" for both
     +    successful and conflicted merges (the status of each merge is written to
     +    stdout). The return type of real_merge() is not changed as it is used
     +    for the program's exit status when "--stdin" is not given.
      
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
 3:  bf1dc603a15 = 3:  f95a15a4203 merge-tree: only use basic merge config
 4:  4c416850634 = 4:  1645b0e747e merge-tree: improve docs for --stdin
 5:  89722894c87 = 5:  a0179820092 merge-tree: fix link formatting in html docs

-- 
gitgitgadget

[PATCH v2 1/5] merge-tree --stdin: flush stdout to avoid deadlock

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

If a process tries to read the output from "git merge-tree --stdin"
before it closes merge-tree's stdin then it deadlocks. This happens
because merge-tree does not flush its output before trying to read
another line of input and means that it is not possible to cherry-pick a
sequence of commits using "git merge-tree --stdin". Fix this by calling
maybe_flush_or_die() before trying to read the next line of
input. Flushing the output after each merge does not seem to affect the
performance, any difference is lost in the noise even after increasing
the number of runs.

$ git rev-list --merges --parents -n100 origin/master |
	sed 's/^[^ ]* //' >/tmp/merges
$ hyperfine -L flush 0,1 --warmup 1 --runs 30 \
	'GIT_FLUSH={flush} ./git merge-tree --stdin </tmp/merges'
Benchmark 1: GIT_FLUSH=0 ./git merge-tree --stdin </tmp/merges
  Time (mean ± σ):     546.6 ms ±  11.7 ms    [User: 503.2 ms, System: 40.9 ms]
  Range (min … max):   535.9 ms … 567.7 ms    30 runs

Benchmark 2: GIT_FLUSH=1 ./git merge-tree --stdin </tmp/merges
  Time (mean ± σ):     546.9 ms ±  12.0 ms    [User: 505.9 ms, System: 38.9 ms]
  Range (min … max):   529.8 ms … 570.0 ms    30 runs

Summary
  'GIT_FLUSH=0 ./git merge-tree --stdin </tmp/merges' ran
    1.00 ± 0.03 times faster than 'GIT_FLUSH=1 ./git merge-tree --stdin </tmp/merges'

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/merge-tree.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 9a6c8b4e4cf..57f4340faba 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -18,6 +18,7 @@
 #include "tree.h"
 #include "config.h"
 #include "strvec.h"
+#include "write-or-die.h"
 
 static int line_termination = '\n';
 
@@ -623,6 +624,7 @@ int cmd_merge_tree(int argc,
 			} else {
 				die(_("malformed input line: '%s'."), buf.buf);
 			}
+			maybe_flush_or_die(stdout, "stdout");
 
 			if (result < 0)
 				die(_("merging cannot continue; got unclean result of %d"), result);
-- 
gitgitgadget

[PATCH v2 2/5] merge-tree: remove redundant code

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

real_merge() only ever returns "0" or "1" as it dies if the merge status
is less than zero. Therefore the check for "result < 0" is redundant and
the result variable is not needed. The return value of real_merge() is
ignored because exit status of "git merge-tree --stdin" is "0" for both
successful and conflicted merges (the status of each merge is written to
stdout). The return type of real_merge() is not changed as it is used
for the program's exit status when "--stdin" is not given.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/merge-tree.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 57f4340faba..3c73482f2b0 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -601,7 +601,6 @@ int cmd_merge_tree(int argc,
 		line_termination = '\0';
 		while (strbuf_getline_lf(&buf, stdin) != EOF) {
 			struct strbuf **split;
-			int result;
 			const char *input_merge_base = NULL;
 
 			split = strbuf_split(&buf, ' ');
@@ -618,16 +617,14 @@ int cmd_merge_tree(int argc,
 			if (input_merge_base && split[2] && split[3] && !split[4]) {
 				strbuf_rtrim(split[2]);
 				strbuf_rtrim(split[3]);
-				result = real_merge(&o, input_merge_base, split[2]->buf, split[3]->buf, prefix);
+				real_merge(&o, input_merge_base, split[2]->buf, split[3]->buf, prefix);
 			} else if (!input_merge_base && !split[2]) {
-				result = real_merge(&o, NULL, split[0]->buf, split[1]->buf, prefix);
+				real_merge(&o, NULL, split[0]->buf, split[1]->buf, prefix);
 			} else {
 				die(_("malformed input line: '%s'."), buf.buf);
 			}
 			maybe_flush_or_die(stdout, "stdout");
 
-			if (result < 0)
-				die(_("merging cannot continue; got unclean result of %d"), result);
 			strbuf_list_free(split);
 		}
 		strbuf_release(&buf);
-- 
gitgitgadget

[PATCH v2 3/5] merge-tree: only use basic merge config

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Commit 9c93ba4d0ae (merge-recursive: honor diff.algorithm, 2024-07-13)
replaced init_merge_options() with init_basic_merge_config() for use in
plumbing commands and init_ui_merge_config() for use in porcelain
commands. As "git merge-tree" is a plumbing command it should call
init_basic_merge_config() rather than init_ui_merge_config(). The merge
ort machinery ignores "diff.algorithm" so the behavior is unchanged by
this commit but it future proofs us against any future changes to
init_ui_merge_config().

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/merge-tree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 3c73482f2b0..3ec7127b3a6 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -576,7 +576,7 @@ int cmd_merge_tree(int argc,
 	};
 
 	/* Init merge options */
-	init_ui_merge_options(&o.merge_options, the_repository);
+	init_basic_merge_options(&o.merge_options, the_repository);
 
 	/* Parse arguments */
 	original_argc = argc - 1; /* ignoring argv[0] */
-- 
gitgitgadget

[PATCH v2 4/5] merge-tree: improve docs for --stdin

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Add a section for --stdin in the list of options and document that it
implies -z so readers know how to parse the output. Also correct the
merge status documentation for --stdin as if the status is less than
zero "git merge-tree" dies before printing it.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 Documentation/git-merge-tree.txt | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
index 0b6a8a19b1f..efb16b4f27d 100644
--- a/Documentation/git-merge-tree.txt
+++ b/Documentation/git-merge-tree.txt
@@ -40,6 +40,11 @@ After the merge completes, a new toplevel tree object is created.  See
 OPTIONS
 -------
 
+--stdin::
+	Read the commits to merge from the standard input rather than
+	the command-line. See <<INPUT,INPUT FORMAT>> below for more
+	information.  Implies `-z`.
+
 -z::
 	Do not quote filenames in the <Conflicted file info> section,
 	and end each filename with a NUL character rather than
@@ -116,8 +121,6 @@ This is an integer status followed by a NUL character.  The integer status is:
 
      0: merge had conflicts
      1: merge was clean
-     <0: something prevented the merge from running (e.g. access to repository
-	 objects denied by filesystem)
 
 [[OIDTLT]]
 OID of toplevel tree
@@ -235,6 +238,7 @@ with linkgit:git-merge[1]:
   * any messages that would have been printed to stdout (the
     <<IM,Informational messages>>)
 
+[[INPUT]]
 INPUT FORMAT
 ------------
 'git merge-tree --stdin' input format is fully text based. Each line
-- 
gitgitgadget

[PATCH v2 5/5] merge-tree: fix link formatting in html docs

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

In the html documentation the link to the "OUTPUT" section is surrounded
by square brackets. Fix this by adding explicit link text to the cross
reference.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 Documentation/git-merge-tree.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
index efb16b4f27d..cf0578f9b5e 100644
--- a/Documentation/git-merge-tree.txt
+++ b/Documentation/git-merge-tree.txt
@@ -49,7 +49,8 @@ OPTIONS
 	Do not quote filenames in the <Conflicted file info> section,
 	and end each filename with a NUL character rather than
 	newline.  Also begin the messages section with a NUL character
-	instead of a newline.  See <<OUTPUT>> below for more information.
+	instead of a newline.  See <<OUTPUT,OUTPUT>> below for more
+	information.
 
 --name-only::
 	In the Conflicted file info section, instead of writing a list
-- 
gitgitgadget

Re: [PATCH v2 0/5] merge-tree --stdin: flush stdout

[Elijah Newren]

On Tue, Feb 18, 2025 at 8:24 AM Phillip Wood via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> Thanks to Elijah for his comments on V1. I've updated the commit message of
> patch 2 as he suggested. The rest of the patches are unchanged.
>
> V1 Cover Letter:
>
> I tried to squash some fixup commits with "git merge-tree --stdin" and found
> that my script deadlocked because the output of "git merge-tree" is not
> flushed after each merge. The first patch fixes that and the rest are
> cleanups I noticed while reading the code and documentation. This series is
> based on maint.
>
> Phillip Wood (5):
>   merge-tree --stdin: flush stdout to avoid deadlock
>   merge-tree: remove redundant code
>   merge-tree: only use basic merge config
>   merge-tree: improve docs for --stdin
>   merge-tree: fix link formatting in html docs
>
>  Documentation/git-merge-tree.txt | 11 ++++++++---
>  builtin/merge-tree.c             | 11 +++++------
>  2 files changed, 13 insertions(+), 9 deletions(-)
>
>
> base-commit: f93ff170b93a1782659637824b25923245ac9dd1
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1862%2Fphillipwood%2Fmerge-tree-flush-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1862/phillipwood/merge-tree-flush-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/1862
>
> Range-diff vs v1:
>
>  1:  3b317978509 = 1:  3b317978509 merge-tree --stdin: flush stdout to avoid deadlock
>  2:  16fec87766f ! 2:  63b09dbe1b7 merge-tree: remove redundant code
>      @@ Commit message
>
>           real_merge() only ever returns "0" or "1" as it dies if the merge status
>           is less than zero. Therefore the check for "result < 0" is redundant and
>      -    the result variable is not needed.
>      +    the result variable is not needed. The return value of real_merge() is
>      +    ignored because exit status of "git merge-tree --stdin" is "0" for both
>      +    successful and conflicted merges (the status of each merge is written to
>      +    stdout). The return type of real_merge() is not changed as it is used
>      +    for the program's exit status when "--stdin" is not given.
>
>           Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>
>  3:  bf1dc603a15 = 3:  f95a15a4203 merge-tree: only use basic merge config
>  4:  4c416850634 = 4:  1645b0e747e merge-tree: improve docs for --stdin
>  5:  89722894c87 = 5:  a0179820092 merge-tree: fix link formatting in html docs

This round looks good to me; thanks.

Re: [PATCH v2 0/5] merge-tree --stdin: flush stdout

[Phillip Wood]

On 18/02/2025 16:46, Elijah Newren wrote:
> On Tue, Feb 18, 2025 at 8:24 AM Phillip Wood via GitGitGadget
>> Range-diff vs v1:
>>
>>   1:  3b317978509 = 1:  3b317978509 merge-tree --stdin: flush stdout to avoid deadlock
>>   2:  16fec87766f ! 2:  63b09dbe1b7 merge-tree: remove redundant code
>>       @@ Commit message
>>
>>            real_merge() only ever returns "0" or "1" as it dies if the merge status
>>            is less than zero. Therefore the check for "result < 0" is redundant and
>>       -    the result variable is not needed.
>>       +    the result variable is not needed. The return value of real_merge() is
>>       +    ignored because exit status of "git merge-tree --stdin" is "0" for both
>>       +    successful and conflicted merges (the status of each merge is written to
>>       +    stdout). The return type of real_merge() is not changed as it is used
>>       +    for the program's exit status when "--stdin" is not given.
>>
>>            Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>>   3:  bf1dc603a15 = 3:  f95a15a4203 merge-tree: only use basic merge config
>>   4:  4c416850634 = 4:  1645b0e747e merge-tree: improve docs for --stdin
>>   5:  89722894c87 = 5:  a0179820092 merge-tree: fix link formatting in html docs
> 
> This round looks good to me; thanks.

That's great, thanks very much for reviewing these patches

Best Wishes

Phillip

Re: [PATCH v2 0/5] merge-tree --stdin: flush stdout

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> On Tue, Feb 18, 2025 at 8:24 AM Phillip Wood via GitGitGadget
> ...
>>  3:  bf1dc603a15 = 3:  f95a15a4203 merge-tree: only use basic merge config
>>  4:  4c416850634 = 4:  1645b0e747e merge-tree: improve docs for --stdin
>>  5:  89722894c87 = 5:  a0179820092 merge-tree: fix link formatting in html docs
>
> This round looks good to me; thanks.

Thanks, both.  Queued.

Re: [PATCH v2 1/5] merge-tree --stdin: flush stdout to avoid deadlock

[Patrick Steinhardt]

On Tue, Feb 18, 2025 at 04:24:35PM +0000, Phillip Wood via GitGitGadget wrote:
> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> index 9a6c8b4e4cf..57f4340faba 100644
> --- a/builtin/merge-tree.c
> +++ b/builtin/merge-tree.c
> @@ -18,6 +18,7 @@
>  #include "tree.h"
>  #include "config.h"
>  #include "strvec.h"
> +#include "write-or-die.h"
>  
>  static int line_termination = '\n';
>  
> @@ -623,6 +624,7 @@ int cmd_merge_tree(int argc,
>  			} else {
>  				die(_("malformed input line: '%s'."), buf.buf);
>  			}
> +			maybe_flush_or_die(stdout, "stdout");
>  
>  			if (result < 0)
>  				die(_("merging cannot continue; got unclean result of %d"), result);

I was briefly wondering whether we should rather move this into
`real_merge()` itself, which is responsible for writing to stdout. But
the only other callsite doesn't really care as it will exit immediately
anyway. So this is probably fine.

Overall the series looks good to me, thanks!

Patrick

Re: [PATCH v2 1/5] merge-tree --stdin: flush stdout to avoid deadlock

[Phillip Wood]

Hi Patrick

On 19/02/2025 06:23, Patrick Steinhardt wrote:
> 
> I was briefly wondering whether we should rather move this into
> `real_merge()` itself, which is responsible for writing to stdout. But
> the only other callsite doesn't really care as it will exit immediately
> anyway. So this is probably fine.

I did wonder about that when I was writing the patch but decided it only 
mattered when --stdin was given.

> Overall the series looks good to me, thanks!

Thanks

Phillip

Re: [PATCH v2 1/5] merge-tree --stdin: flush stdout to avoid deadlock

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Tue, Feb 18, 2025 at 04:24:35PM +0000, Phillip Wood via GitGitGadget wrote:
>> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
>> index 9a6c8b4e4cf..57f4340faba 100644
>> --- a/builtin/merge-tree.c
>> +++ b/builtin/merge-tree.c
>> @@ -18,6 +18,7 @@
>>  #include "tree.h"
>>  #include "config.h"
>>  #include "strvec.h"
>> +#include "write-or-die.h"
>>  
>>  static int line_termination = '\n';
>>  
>> @@ -623,6 +624,7 @@ int cmd_merge_tree(int argc,
>>  			} else {
>>  				die(_("malformed input line: '%s'."), buf.buf);
>>  			}
>> +			maybe_flush_or_die(stdout, "stdout");
>>  
>>  			if (result < 0)
>>  				die(_("merging cannot continue; got unclean result of %d"), result);
>
> I was briefly wondering whether we should rather move this into
> `real_merge()` itself, which is responsible for writing to stdout.

Indeed.  Its "if we are working stdin mode, emit a line break" near
the end does hint that the original intent was to ensure the output
goes out before we return to read more (even though the code forgets
that the stdio layer buffers), so it doubly makes sense to place the
logic to flush before the function returns, no?

> But
> the only other callsite doesn't really care as it will exit immediately
> anyway. So this is probably fine.

With the current set of callers, yes.  When we write these
sentences, our imagination most likely fails short to come up with a
reason why the next callsite needs to be added to the program, so I
would strongly suggest flushing inside real_merge() IF it were a
public function, but because it is a file-scope static helper, we
hopefully will remember we would need to add a flush to the caller
when we add the fourth caller.  So I do not care too deeply either
way.

> Overall the series looks good to me, thanks!

Thanks, all.  Let's mark it for 'next'.