Re: [PATCH] Teach merge the '[-e|--edit]' option

From: Jay Soffian <jaysoffian@gmail.com>
Date: Oct 7, 2011, 18:01 UTC
Message-ID: <CAG+J_Dz7-tTdgT=cqoKhK+fAhmESLnp93yHyxOF_NOY5Wx01+w@mail.gmail.com>
In-Reply-To: <7vk48gwvyd.fsf@alter.siamese.dyndns.org>

On Fri, Oct 7, 2011 at 1:30 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 28 quoted lines

> Jay Soffian <jaysoffian@gmail.com> writes:
>
>> Implement "git merge [-e|--edit]" as "git merge --no-commit && git commit"
>> as a convenience for the user.
>>
>> Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
>> ---
>> ...
>> @@ -1447,6 +1457,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>>       }
>>
>>       if (merge_was_ok) {
>> +             if (option_edit) {
>> +                     const char *args[] = {"commit", "-e", NULL};
>> +                     return run_command_v_opt(args, RUN_GIT_CMD);
>> +             }
>>               fprintf(stderr, _("Automatic merge went well; "
>>                       "stopped before committing as requested\n"));
>>               return 0;
>
>
> I wanted to like this approach, thinking this approach might be safer and
> with the least chance of breaking other codepaths, but this feels like an
> ugly hack.
>
> Are we still honoring all the hooks "git merge" honors?  More importantly,
> isn't this make it impossible for future maintainers of this command to
> enhance the command by adding other hooks after the commit is made?

Git is already inconsistent with respect to which hooks are called when. Shouldn't post-merge be called on a merge commit regardless of whether you use --no-commit or not? Well, it isn't, it's only called when merge performs the commit internally. The post-merge hook was probably a mistake -- git calls the post-commit hook passing the context as an argument, so probably merge should just call post-commit "merge". But that ship has sailed.

See also 65969d43d1 (merge: honor prepare-commit-msg hook, 2011-02-14).

Show 9 quoted lines

> If we wanted to do this properly, we should update builtin/merge.c to call
> launch_editor() before it runs commit_tree(), in a way similar to how
> prepare_to_commit() in builtin/commit.c does so when e.g. "commit -m foo -e"
> is run. An editmsg is prepared (you already have it in MERGE_MSG), the
> editor is allowed to update it, and then the original code before such a
> patch will run using the updated contents of MERGE_MSG. That way, the _only_
> change in behaviour when "-e" is used is to let the user update the message
> used in the commit log, and everything else would run exactly the same way
> as if no "-e" was given, including the invocation of hooks.

I find git very difficult to reason about (and inconsistent in its behavior) due to piecemeal hoisting of some functionality into porcelain commands (another example, revert.c building in the recursive merge strategy but not any others).

I actually think a better choice would be to remove commit_tree() from merge and always have it run commit externally. I'm not seriously suggesting that be done, but it would make git more consistent. But I'm not going to send in a patch which makes the situation worse.

j.