Re: [PATCH] submodule operations: tighten pathspec errors

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

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.