merge-tree --stdin: flush stdout #1862
Open
Conversation
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
|
/preview |
|
There are issues in commit 2046cf6: |
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]>
phillipwood
force-pushed
the
merge-tree-flush
branch
from
c121776 to
8972289
Compare
|
/preview |
|
Error: git fetch https://github.com/gitgitgadget/git -- +refs/notes/gitgitgadget:refs/notes/gitgitgadget +refs/heads/maint:refs/remotes/upstream/maint +refs/heads/seen:refs/remotes/upstream/seen +refs/heads/master:refs/remotes/upstream/master +refs/heads/next:refs/remotes/upstream/next +refs/pull/1862/head:refs/pull/1862/head +refs/pull/1862/merge:refs/pull/1862/merge failed: 128, |
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -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.
| @@ -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.
>
| @@ -40,6 +40,11 @@ After the merge completes, a new toplevel tree object is created. See | |||
| OPTIONS | |||
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]>
>
> 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 <[email protected]>
> ---
> 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
> --
> gitgitgadgetThere was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
Hi Elijah
On 17/02/2025 20:26, Elijah Newren wrote:
> On Sun, Feb 16, 2025 at 8:37 AM Phillip Wood via GitGitGadget
> <[email protected]> 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 <[email protected]>
>> ---
>> 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
> There was a problem hiding this comment.
On the Git mailing list, Elijah Newren wrote (reply to this):
On Tue, Feb 18, 2025 at 2:02 AM Phillip Wood <[email protected]> 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
> > <[email protected]> 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.
| 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.
|
User |
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 <[email protected]>
Commit 9c93ba4 (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 <[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]>
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]>
phillipwood
force-pushed
the
merge-tree-flush
branch
from
8972289 to
a017982
Compare
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Elijah Newren wrote (reply to this): On Tue, Feb 18, 2025 at 8:24 AM Phillip Wood via GitGitGadget
<[email protected]> 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 <[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 the Git mailing list, Phillip Wood wrote (reply to this): 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 <[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.
That's great, thanks very much for reviewing these patches
Best Wishes
Phillip |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Elijah Newren <[email protected]> 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. |
|
This patch series was integrated into seen via git@2481aec. |
| @@ -18,6 +18,7 @@ | |||
| #include "tree.h" | |||
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'.
|
User |
|
This patch series was integrated into seen via git@f0762e0. |
|
This patch series was integrated into next via git@ebc3ae6. |
|
This patch series was integrated into seen via git@bb34ed1. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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.
Cc: Elijah Newren [email protected]
cc: Phillip Wood [email protected]
cc: Patrick Steinhardt [email protected]