Re: [PATCH 5/6 v5] merge.c: delegate handling of "-" shorthand to revision.c:get_sha1

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 1, 2017, 23:05 UTC
Message-ID: <xmqqinnsegxb.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqqpoi0eho8.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 30 quoted lines

> Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
>
>> The callchain for handling each argument contains the function
>> revision.c:get_sha1 where the shorthand for "-" ~ "@{-1}" has already been
>> implemented in a previous patch; the complete callchain leading to that
>> function is:
>>
>> 1. merge.c:collect_parents
>> 2. commit.c:get_merge_parent : this function calls revision.c:get_sha1
>>
>> This patch also adds a test for checking that the shorthand works properly
>
> This breaks "git merge".
>
>> +test_expect_success 'merge - should work' '
>> +        git checkout testing-2 &&
>> +        git merge - &&
>> +        git rev-parse HEAD HEAD^^ | sort >actual &&
>> +        git rev-parse master testing-1 | sort >expect &&
>> +        test_cmp expect actual
>
> This test is not sufficient to catch a regression I seem to be
> seeing.
>
> 	$ git checkout side
> 	$ git checkout pu
> 	$ git merge -
>
> used to say "Merge branch 'side' into pu".  With this series merged,
> I seem to be getting "Merge commit '-' into pu".

You stopped at get_sha1_1() in your 3817cebabc ("sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}"", 2017-02-25), instead of going down to get_sha1_basic() and teaching it that "-" means the same thing as "@{-1}", which would in turn require you to teach dwim_ref() that "-" is the same thing as "@{-1}". As dwim_ref() does not know about "-" and does not expand it to the refname like it expands "@{-1}", it would break and that is why 3817cebabc punts at a bit higher in the callchain.

The breakage by this patch to "git merge" happens for the same reason. cmd_merge() calls collect_parents() which annotates the commits that are merged with their textual name, which used to be "@{-1}" without this patch but now "-" is passed as-is. This annotation will be given to merge_name(), and the first thing it does is dwim_ref(). The function knows what to do with "@{-1}", but it does not know what to do with "-", and that is why you end up producing "Merge commit '-' into ...".

Dropping this patch from the series would make things consistent with what was done in 3817cebabc and I think that is a sensible place to stop. After the dust settles, We _can_ later dig further by teaching dwim_ref() and friends what "-" means, and when it is done, this patch would become useful.

Thanks.