On Fri, Oct 7, 2011 at 6:15 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> 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.
>> 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.
>> 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