Junio C Hamano <gits...@pobox.com> writes:
> Siddharth Kannan <kannan.siddhart...@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.
