Re: [PATCH] submodule operations: tighten pathspec errors

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 1, 2016, 21:14 UTC
Message-ID: <xmqqy46owr0n.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAGZ79kbdfEJ1iSpOJ=HfHP=EvVxB9Sv+5Zk+goLSOJphh8ZZ+w@mail.gmail.com>

Stefan Beller <sbeller@google.com> writes:

Show 20 quoted lines

> On Thu, May 26, 2016 at 1:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>>> @@ -36,10 +37,9 @@ static int module_list_compute(int argc, const char **argv,
>>>
>>>       for (i = 0; i < active_nr; i++) {
>>>               const struct cache_entry *ce = active_cache[i];
>>> -
>>> -             if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),
>>> -                                 0, ps_matched, 1) ||
>>> -                 !S_ISGITLINK(ce->ce_mode))
>>> +             if (!S_ISGITLINK(ce->ce_mode) ||
>>> +                 !match_pathspec(pathspec, ce->name, ce_namelen(ce),
>>> +                                 0, ps_matched, 1))
>>>                       continue;
>>
>> OK, this is the crucial bit in this patch. pathspec matches are now
>> done only against gitlinks, so any unmatch is "pattern or path
>> given, but there was no such submodule".
>
> right.

That changes the semantics, and its user visible effect may deserve to be in the documentation, no?

It used to be that "git submodule--helper list COPYING" did not complain, but with this change, it would. Which may be a good change, but "git submodule--helper list sub*" where most but not all of glob expansion done by the shell are submodule directories may be a common thing people may do, and it may be irritating to see the unmatch complaints. I dunno.

When we know we are not going to complain (i.e. --unmatch-ok option is given from the command line), I think it is perfectly fine (and it is even preferrable) to swap the order of the check. The mode check done with S_ISGITLINK() is much cheaper and it is likely to yield true much less often, which in turn would allow us to make fewer calls to more expensive match_pathspec().

But when we want to diagnose typo (i.e. --unmatch-ok was not given), we may want to preserve the current order, so that the "sub*" example in a few paragraphs ago would not irritate the users.

Show 7 quoted lines

>> It is tempting to update report_path_error() return "OK" when its
>> first parameter is NULL.
>
> such that we can do a
>
>     if (report_path_error(unmatch_ok ? NULL : ps_matched, pathspec, prefix)))
>         result = -1;

I actually was thinking about setting ps_matched to NULL so that both match_pathspec() and report_path_error() would get NULL. match_pathspec() has to do _more_ work when ps_matched[] aka seen[] is given.

Show 22 quoted lines

>>> @@ -407,7 +410,7 @@ cmd_foreach()
>>>       # command in the subshell (and a recursive call to this function)
>>>       exec 3<&0
>>>
>>> -     git submodule--helper list --prefix "$wt_prefix"|
>>> +     git submodule--helper list ${unmatch:+--unmatch} --prefix "$wt_prefix"|
>>
>> For this to work, somebody must ensure that the variable unmatch is
>> either unset or set to empty unless the user gave --error-unmatch to
>> us.  There is a block of empty assignments hear the beginning of
>> this file for that very purpose, i.e. resetting a stray environment
>> variable that could be in user's environment.
>>
>> The patch itself does not look too bad.  I do not have an opinion on
>> which one should be the default, and I certainly would understand if
>> you want to keep the default loose (i.e. not complaining) with an
>> optional error checking, but whichever default you choose, the
>> option and variable names need to be clarified to avoid confusion.
>
> Ok I'll fix the variable names; I think for consistency with e.g.
> ls-files --error-unmatch
> we would want to be loose by default and strict on that option.

I do not think the "typo-prevention" safety measure should suddenly be turned into opt-in; I'd suggest "--unmatch-ok" instead.