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

On Fri, Oct 7, 2011 at 1:30 PM, Junio C Hamano [off-list ref] wrote:

Jay Soffian [off-list ref] writes:

quoted

Implement "git merge [-e|--edit]" as "git merge --no-commit && git commit"
as a convenience for the user.

Signed-off-by: Jay Soffian <redacted>
---
...

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