Re: [PATCH] submodule operations: tighten pathspec errors

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

> 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.

>> 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.

>>> @@ -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.