patchsubmodule operations: tighten pathspec errors

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 5295b72..91c49ec 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -19,7 +19,8 @@ struct module_list {
>  static int module_list_compute(int argc, const char **argv,
>  			       const char *prefix,
>  			       struct pathspec *pathspec,
> -			       struct module_list *list)
> +			       struct module_list *list,
> +			       int unmatch)

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

> @@ -53,7 +53,9 @@ static int module_list_compute(int argc, const char **argv,
>  			i++;
>  	}
>  
> -	if (ps_matched && report_path_error(ps_matched, pathspec, prefix))
> +	if (!unmatch &&
> +	    ps_matched &&
> +	    report_path_error(ps_matched, pathspec, prefix))
>  		result = -1;

> diff --git a/git-submodule.sh b/git-submodule.sh
> index fb68f1f..f10e10a 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -391,6 +391,9 @@ cmd_foreach()
>  		--recursive)
>  			recursive=1
>  			;;
> +		--error-unmatch)
> +			unmatch=1
> +			;;

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

> Stefan Beller <sbeller@google.com> writes:
>
>> It's a first initial version with no tests (and probably conflicting with
>> some topics in flight), but I was curious how involved this issue actually is,
>> so I took a stab at implementing it.
>
> I take it to mean "This is s/PATCH/RFC/".
>
>> +--error-unmatch::
>> +     If the pathspec included a specification that did not match,
>> +     the usual operation is to error out. This switch suppresses
>> +     error reporting and continues the operation.
>
> The behaviour described is a total opposite of the option with the
> same name "ls-files" has, no?
>
> If there were no default, --error-unmatch would make an unmatching
> pathspec an error and --no-error-unmatch would make it a non-error.
>
> If the default is to error out, there is no need for --error-unmatch
> to exist, but you do want --no-error-unmatch aka --unmatch-ok.
>
> If the default is not to error out, --error-unmatch should make it
> notice and turn it into an error.
>
> I am guessing that you were debating yourself which should be the
> default and the patch ended up in an inconsistent state, the
> description assuming a more strict default, while the option name
> assuming a less strict default.

>
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index 5295b72..91c49ec 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -19,7 +19,8 @@ struct module_list {
>>  static int module_list_compute(int argc, const char **argv,
>>                              const char *prefix,
>>                              struct pathspec *pathspec,
>> -                            struct module_list *list)
>> +                            struct module_list *list,
>> +                            int unmatch)
>
> What is "unmatch"?  "Catch unmatch errors, please?"  "Do not check
> and report unmatch errors?"
>
> My cursory read of a few hunks below tells me that you meant the
> latter, i.e. "unmatch_ok".
>
>> @@ -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".

>
>> @@ -53,7 +53,9 @@ static int module_list_compute(int argc, const char **argv,
>>                       i++;
>>       }
>>
>> -     if (ps_matched && report_path_error(ps_matched, pathspec, prefix))
>> +     if (!unmatch &&
>> +         ps_matched &&
>> +         report_path_error(ps_matched, pathspec, prefix))
>>               result = -1;
>
> If unmatch is not true, then check if ps_matched records "aw, this
> pathspec element did not get used" and complain.  If unmatch is
> true, we do not do that.
>
> Which confirms my earlier "'unmatch' here means 'unmatch_ok'".
>
> It is tempting to update report_path_error() return "OK" when its
> first parameter is NULL.

>
>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index fb68f1f..f10e10a 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -391,6 +391,9 @@ cmd_foreach()
>>               --recursive)
>>                       recursive=1
>>                       ;;
>> +             --error-unmatch)
>> +                     unmatch=1
>> +                     ;;
>
> So "--error-unmatch" does pass "--unmatch" which is "please ignore
> unmatch errors".  That is a bit strange (see above).
>
>> @@ -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.

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

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

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

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

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

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

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