Re: [PATCH v2] Teach merge the '[-e|--edit]' option
- From
Jay Soffian <jaysoffian@gmail.com>
- Date
- Oct 8, 2011, 18:11 UTC
- Message-ID
- <CAG+J_Dzrk5x0+JRC8EbrAxjZE+hD+-5mp+H=F=M8Su2WosPfmg@mail.gmail.com>
- In-Reply-To
- <7vfwj4tplw.fsf@alter.siamese.dyndns.org>
On Fri, Oct 7, 2011 at 6:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
> 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".
No Junio, you have my argument completely reversed.
I am complaining that git-merge implements commits internally, which gives it unique behavior from git-{commit/cherry-pick/revert} (the latter two of which just run external git-commit). I'm saying merge is fundamentally broken to do it this way. And maybe that's something that should be fixed in 2.0 -- that git-merge should just call out to git-commit, just like cherry-pick/revert do.
In case that's not clear: I think that git-merge should eventually behave identically to "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,
Yes.
> losing some hook support and such.
Yes, I would lose the post-merge hook and such.
>, or would you rather make the split case work similar to the usual merge?
No, I would not do that.
BTW, the same arguments apply to git-am, which uses git-commit-tree, and so implements its own set of hooks.
> 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?
Strongly disagree. I think it would make much more sense for all commits to flow through git-commit, which would ensure consistent behavior. I think we've got a mishmash of hooks which evolved over time.
Show 5 quoted lines
>> 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].
No, I meant that git-commit includes status information about the commit itself as comments in the commit message (git config commit.status), and I didn't implement that. I don't think that makes this patch incomplete however, that could be added by a later patch.
I'll send another iteration with your comments below addressed.
j.
Show 48 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 ";".
>
>> @@ -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.
>
>