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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 7, 2011, 17:30 UTC
Message-ID: <7vk48gwvyd.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1318001347-11347-1-git-send-email-jaysoffian@gmail.com>

Jay Soffian <jaysoffian@gmail.com> writes:

Show 17 quoted lines

> 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?

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.