On Thu, May 26, 2016 at 1:00 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Stefan Beller <sbeller@xxxxxxxxxx> 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
>
--
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