Jay Soffian <jaysoffian@xxxxxxxxx> writes:
> 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@xxxxxxxxx>
> ---
> On Fri, Oct 7, 2011 at 1:30 PM, Junio C Hamano <gitster@xxxxxxxxx> 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].
> 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 ";".
> @@ -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.
--
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