merge-tree --stdin: flush stdout #1862
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,11 +40,17 @@ 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, | ||
|
There was a problem hiding this comment. On the Git mailing list, Elijah Newren wrote (reply to this): On Sun, Feb 16, 2025 at 8:37 AM Phillip Wood via GitGitGadget
<[email protected]> wrote:
>
> 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(-)
>
> 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. |
||
| 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,OUTPUT>> below for more | ||
| information. | ||
|
|
||
| --name-only:: | ||
| In the Conflicted file info section, instead of writing a list | ||
|
|
@@ -116,8 +122,6 @@ This is an integer status followed by a NUL character. The integer status is: | |
|
|
||
| 0: merge had conflicts | ||
| 1: merge was clean | ||
|
|
||
| [[OIDTLT]] | ||
| OID of toplevel tree | ||
|
|
@@ -235,6 +239,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 | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ | |
| #include "tree.h" | ||
|
There was a problem hiding this comment. On the Git mailing list, Elijah Newren wrote (reply to this): On Sun, Feb 16, 2025 at 8:37 AM Phillip Wood via GitGitGadget
<[email protected]> wrote:
>
> 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.
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 <[email protected]>
> ---
> 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.There was a problem hiding this comment. On the Git mailing list, Patrick Steinhardt wrote (reply to this): 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!
PatrickThere was a problem hiding this comment. On the Git mailing list, Phillip Wood wrote (reply to this): 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
There was a problem hiding this comment. On the Git mailing list, Junio C Hamano wrote (reply to this): Patrick Steinhardt <[email protected]> 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'.
|
||
| #include "config.h" | ||
| #include "strvec.h" | ||
| #include "write-or-die.h" | ||
|
|
||
| static int line_termination = '\n'; | ||
|
|
||
|
|
@@ -575,7 +576,7 @@ int cmd_merge_tree(int argc, | |
| }; | ||
|
|
||
| /* Init merge options */ | ||
| init_basic_merge_options(&o.merge_options, the_repository); | ||
|
|
||
| /* Parse arguments */ | ||
| original_argc = argc - 1; /* ignoring argv[0] */ | ||
|
|
@@ -600,7 +601,6 @@ int cmd_merge_tree(int argc, | |
| line_termination = '\0'; | ||
|
There was a problem hiding this comment. On the Git mailing list, Elijah Newren wrote (reply to this): On Sun, Feb 16, 2025 at 8:37 AM Phillip Wood via GitGitGadget
<[email protected]> wrote:
>
> 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.
> Signed-off-by: Phillip Wood <[email protected]>
> ---
> 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.There was a problem hiding this comment. On the Git mailing list, Phillip Wood wrote (reply to this): Hi Elijah
On 17/02/2025 20:15, Elijah Newren wrote:
> On Sun, Feb 16, 2025 at 8:37 AM Phillip Wood via GitGitGadget
> <[email protected]> wrote:
>>
>> 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.
That's a good suggestion - I'll re-roll
Thanks
Phillip
>> Signed-off-by: Phillip Wood <[email protected]>
>> ---
>> 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.
> |
||
| while (strbuf_getline_lf(&buf, stdin) != EOF) { | ||
| struct strbuf **split; | ||
| const char *input_merge_base = NULL; | ||
|
|
||
| split = strbuf_split(&buf, ' '); | ||
|
|
@@ -617,15 +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]); | ||
| real_merge(&o, input_merge_base, split[2]->buf, split[3]->buf, prefix); | ||
| } else if (!input_merge_base && !split[2]) { | ||
| 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"); | ||
|
|
||
| strbuf_list_free(split); | ||
| } | ||
| strbuf_release(&buf); | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Elijah Newren wrote (reply to this):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Phillip Wood wrote (reply to this):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Elijah Newren wrote (reply to this):