On Fri, Oct 7, 2011 at 1:30 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Jay Soffian <jaysoffian@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> ...
>> @@ -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).
> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html