merge-tree --stdin: flush stdout

From: Phillip Wood <[email protected]>

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 <[email protected]>

---
 builtin/merge-tree.c | 2 ++
 1 file changed, 2 insertions(+)

builtin/merge-tree.c +2 -1

index 9a6c8b4e4cf..57f4340faba 100644
@@ -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

From: Phillip Wood <[email protected]>

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 <[email protected]>

---
 builtin/merge-tree.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

builtin/merge-tree.c +2 -6

index 57f4340faba..3c73482f2b0 100644
@@ -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

Quoted 6 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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.

Quoted 37 lines

Signed-off-by Phillip Wood <[email protected]>

---
  builtin/merge-tree.c | 7 ++-----
  1 file changed, 2 insertions(+), 5 deletions(-)

builtin/merge-tree.c +2 -6

index 57f4340faba..3c73482f2b0 100644
@@ -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);
--

Quoted 1 line

gitgitgadget

Looks good.

From: Phillip Wood <[email protected]>

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 <[email protected]>

---
 Documentation/git-merge-tree.txt | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Documentation/git-merge-tree.txt +5 -0

index 0b6a8a19b1f..efb16b4f27d 100644
@@ -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

Documentation/git-merge-tree.txt +1 -1

 [[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

Quoted 4 lines · Phillip Wood via GitGitGadget

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?

Quoted 22 lines

Signed-off-by Phillip Wood <[email protected]>

---
  Documentation/git-merge-tree.txt | 8 ++++++--
  1 file changed, 6 insertions(+), 2 deletions(-)

Documentation/git-merge-tree.txt +5 -0

index 0b6a8a19b1f..efb16b4f27d 100644
@@ -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

Documentation/git-merge-tree.txt This is an integer status followed by a NUL character. The integer status is: · line 120

Quoted 5 lines

Documentation/git-merge-tree.txt +0 -2

@@ -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)?

Quoted 13 lines

Documentation/git-merge-tree.txt +1 -1

  [[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

Hi Elijah

Quoted 9 lines · Elijah Newren

Quoted 4 lines · Phillip Wood via GitGitGadget

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

From: Phillip Wood <[email protected]>

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 <[email protected]>

---
 Documentation/git-merge-tree.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Documentation/git-merge-tree.txt +2 -2

index efb16b4f27d..cf0578f9b5e 100644
@@ -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

builtin/merge-tree.c +1 -0

index 9a6c8b4e4cf..57f4340faba 100644
@@ -18,6 +18,7 @@
 #include "tree.h"
 #include "config.h"
 #include "strvec.h"
+#include "write-or-die.h"
 
 static int line_termination = '\n';
 

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!

Quoted 18 lines · Phillip Wood via GitGitGadget

builtin/merge-tree.c +2 -0

index 9a6c8b4e4cf..57f4340faba 100644
@@ -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");
 

builtin/merge-tree.c int cmd_merge_tree(int argc, · line 630

Quoted 2 lines

builtin/merge-tree.c +0 -0

 			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!

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 <[email protected]>

 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

On Tue, Feb 18, 2025 at 8:24 AM Phillip Wood via GitGitGadget

Quoted 15 lines

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.

Quoted 5 lines

Signed-off-by Phillip Wood <[email protected]>

  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.

On Tue, Feb 18, 2025 at 8:24 AM Phillip Wood via GitGitGadget
...

Quoted 3 lines

 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.