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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 19, 2025, 16:02 UTC
Message-ID: <xmqqseo9hqh8.fsf@gitster.g>
In-Reply-To: <Z7V48_q3Uu9d0D3Q@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 24 quoted lines

> 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'.