Re: [PATCH] checkout: swap the order of ambiguity check for :/ syntax

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Aug 26, 2016 at 12:19 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Duy Nguyen <pclouds@xxxxxxxxx> writes:
>
>> On Wed, Aug 24, 2016 at 11:35 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>>> Nguyễn Thái Ngọc Duy  <pclouds@xxxxxxxxx> writes:
>>>
>>>> It's not wonderful, but it's in line with how git-checkout stops caring
>>>> about ambiguity after the first argument can be resolved as a ref
>>>> (there's even a test for it, t2010.6).
>>>
>>> But that is justifiable because checkout can only ever take one
>>> revision.  What follows, if there are any, must be paths, and more
>>> importantly, it would be perfectly reasonable if some of them were
>>> missing in the working tree ("ow, I accidentally removed that file,
>>> I need to resurrect it from the index").  Does the same justification
>>> apply to this change?
>>
>> I think there is a misunderstanding. My "after" is in "after the first
>> argument can be resolved, check if it exists in worktree too, if so
>> it's ambiguous and bail". This is usually how we detect ambiguation.
>> But git-checkout does not do the "check if it exists..." clause.
>
> Hmph.  The "case 4" in the function you touched says
>
>          * case 4: git checkout <something> <paths>
>          *
>          *   The first argument must not be ambiguous.
>          *   - If it's *only* a reference, treat it like case (1).
>          *   - If it's only a path, treat it like case (2).
>          *   - else: fail.
>
> Did we break it recently?

I was driven by testing and did not read the code carefully (but on
the other hand, ambiguation plus dwim is never a pleasant read). Near
the end of this function we have

        if (!has_dash_dash) {/* case (3).(d) -> (1) */
                /*
                 * Do not complain the most common case
                 *      git checkout branch
                 * even if there happen to be a file called 'branch';
                 * it would be extremely annoying.
                 */
                if (argc)
                        verify_non_filename(NULL, arg);

argc == 0 would be case 3d, argc != 0 is case 4 and "touch abc; git
checkout :/abc def" does complain about :/abc being ambiguous. So no
we haven't broken anything yet, but I do with my patch. If I want to
make a special case for :/ to speed it up, I need to be careful about
this (and I probably need a test too because it's not detected).

While at there, verify_non_filename() in this function (and
check_filename() too) should pass the right prefix in argument 1, not
NULL, I think. I'll need to do some digging to see why they are NULL
in the first place though.
-- 
Duy
--
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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]