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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 7, 2011, 22:15 UTC
Message-ID: <7vfwj4tplw.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1318023997-54810-1-git-send-email-jaysoffian@gmail.com>

Jay Soffian <jaysoffian@gmail.com> writes:

Show 16 quoted lines

> Implemented internally instead of as "git merge --no-commit && git commit"
> so that "merge --edit" is otherwise consistent (hooks, etc) with "merge".
>
> Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
> ---
> On Fri, Oct 7, 2011 at 1:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> 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
>
> I disagree that this is the proper way to do it. --edit is a new option, there's
> no obviously "correct" behavior. You think 'merge --edit' should behave just
> like 'merge', I think 'merge --edit' should behave like 'merge --no-commit &&
> commit'.
>
> The commit performed internally by git-merge is already wildly inconsistent with
> git-commit.

Think and look forward.

You are complaining that the "commit" does not know enough to behave as if it were a part of the merge command workflow if you split a usual merge into two steps "merge --no-commit; commit".

How would you make it better? Would you strip all the things usual "merge" does, so that it would work identically to the split one, losing some hook support and such, or would you rather make the split case work similar to the usual merge?

I'd say between "merge" and "merge --no-commit ; commit", the latter is what needs to be fixed. Viewed that way, why would you even consider making the new option behave similar to the _wrong_ one?

> I didn't bother with the commit status, it's more code than I wanted
> to deal with duplicating/refactoring from commit.c.

What do you mean by "commit status"? If you mean this patch is incomplete, it would have been nicer if it were labeled with [PATCH/RFC].

Show 9 quoted lines

> diff --git a/builtin/merge.c b/builtin/merge.c
> index ee56974371..0dee53b7e4 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -46,6 +46,7 @@ static const char * const builtin_merge_usage[] = {
>  
>  static int show_diffstat = 1, shortlog_len, squash;
>  static int option_commit = 1, allow_fast_forward = 1;
> +static int option_edit = 0;

No need to move this into .data segment when it can be in .bss segment. Drop the unnecessary " = 0" before ";".

Show 29 quoted lines

> @@ -842,30 +845,54 @@ static void add_strategies(const char *string, unsigned attr)
>  
>  }
>  
> -static void write_merge_msg(void)
> +static void write_merge_msg(struct strbuf *msg)
>  {
>  	int fd = open(git_path("MERGE_MSG"), O_WRONLY | O_CREAT, 0666);
>  	if (fd < 0)
>  		die_errno(_("Could not open '%s' for writing"),
>  			  git_path("MERGE_MSG"));
> -	if (write_in_full(fd, merge_msg.buf, merge_msg.len) != merge_msg.len)
> +	if (write_in_full(fd, msg->buf, msg->len) != msg->len)
>  		die_errno(_("Could not write to '%s'"), git_path("MERGE_MSG"));
>  	close(fd);
>  }
>  
> -static void read_merge_msg(void)
> +static void read_merge_msg(struct strbuf *msg)
>  {
> -	strbuf_reset(&merge_msg);
> -	if (strbuf_read_file(&merge_msg, git_path("MERGE_MSG"), 0) < 0)
> +	strbuf_reset(msg);
> +	if (strbuf_read_file(msg, git_path("MERGE_MSG"), 0) < 0)
>  		die_errno(_("Could not read from '%s'"), git_path("MERGE_MSG"));
>  }
>  
> -static void run_prepare_commit_msg(void)
> +static void write_merge_state();

s/()/(void)/;

Thanks.