Re: [PATCH 7/8] merge-tree: support saving merge messages to a separate file

From: Fabian Stelzer <fs@gigacodes.de>
Date: Jan 3, 2022, 17:22 UTC
Message-ID: <20220103172255.54psh2e5iqzd37sy@fs>
In-Reply-To: <CABPp-BH4okfDXVC418HwfHVR2_NtbKFBOfyYGZ9mWnABMzSruw@mail.gmail.com>

On 03.01.2022 08:51, Elijah Newren wrote:

>On Mon, Jan 3, 2022 at 4:31 AM Fabian Stelzer <fs@gigacodes.de> wrote:
>>
>> On 31.12.2021 05:04, Elijah Newren via GitGitGadget wrote:
>> >From: Elijah Newren <newren@gmail.com>

[...]

Show 23 quoted lines

>> >
>> > static int real_merge(struct merge_tree_options *o,
>> >@@ -442,8 +443,15 @@ static int real_merge(struct merge_tree_options *o,
>> >        */
>> >
>> >       merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
>> >+
>> >+      if (o->messages_file) {
>> >+              FILE *fp = xfopen(o->messages_file, "w");
>> >+              merge_display_update_messages(&opt, &result, fp);
>> >+              fclose(fp);
>>
>> I don't know enough about how merge-ort works internally, but it looks to me
>> like at this point the merge already happened and we just didn't clean up
>> (finalize) yet. It feels wrong to die() at this point just because we can't
>> open messages_file.
>
>Yes, the merge already happened; there now exists a new toplevel tree
>(that nothing references).  I'm not sure I understand what's wrong
>with die'ing here, though.  I can't tell if you want to defer the
>die-ing until later, or just avoid the die-ing and return some kind of
>success despite failing to complete what the user requested.
>

I think i would prefer the merge operation to abort before actually merging when not being able to write its logfile. Otherwise we possibly do a whole lot of work that`s inaccessible afterwards isn't it? (since we don`t print the hash)

Thanks for your work on this feature. I think this could open a lot of new possibilities.