Skip to content

merge-tree --stdin: flush stdout #1862

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

merge-tree --stdin: flush stdout #1862

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3b31797
merge-tree --stdin: flush stdout to avoid deadlock
phillipwood Feb 14, 2025
63b09db
merge-tree: remove redundant code
phillipwood Feb 15, 2025
f95a15a
merge-tree: only use basic merge config
phillipwood Feb 16, 2025
1645b0e
merge-tree: improve docs for --stdin
phillipwood Feb 15, 2025
a017982
merge-tree: fix link formatting in html docs
phillipwood Feb 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions builtin/merge-tree.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "tree.h"
Copy link

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):

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.

Copy link

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, 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!

Patrick

Copy link

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):

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

Copy link

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, 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';

Expand Down Expand Up @@ -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);
Expand Down