Re: [PATCH] submodule operations: tighten pathspec errors

[Stefan Beller <sbeller@google.com>]

On Thu, May 26, 2016 at 1:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

Yes.

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

right.

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

such that we can do a

    if (report_path_error(unmatch_ok ? NULL : ps_matched, pathspec, prefix)))
        result = -1;

That looks good and inside of report_path_error we would only need a

    if (!ps_matched)
        return 0;

at the beginning.

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

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.

Thanks,
Stefan

>