* [PATCH] submodule operations: tighten pathspec errors
@ 2016-05-21 1:21 Stefan Beller
2016-05-26 20:00 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Beller @ 2016-05-21 1:21 UTC (permalink / raw)
Cc: git, Stefan Beller
when the pathspec did not match any path, error out.
Add the `--error-unmatch` switch to use the old behavior.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
This was taken from the Left Over Bits:
git submodule $cmd $pathspec may want to error out when the $pathspec does
not match any submodules. There must be an --unmatch-ok option to override
this safety, though. Cf. $gmane/289535
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 was debating if inside git-submodules.sh we want to pass around a variable
or instead export an environment variable GIT_SUBMODULE_UNMATCH, such that we
do not give it as an argument to the submodule--helper builtin.
(This version is developed on top of the sb/submodule-deinit-all)
Documentation/git-submodule.txt | 15 ++++++++++-----
builtin/submodule--helper.c | 19 +++++++++++--------
git-submodule.sh | 41 ++++++++++++++++++++++++++++++-----------
3 files changed, 51 insertions(+), 24 deletions(-)
diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index ad85183..ceacc02 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -11,16 +11,16 @@ SYNOPSIS
[verse]
'git submodule' [--quiet] add [-b <branch>] [-f|--force] [--name <name>]
[--reference <repository>] [--depth <depth>] [--] <repository> [<path>]
-'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...]
-'git submodule' [--quiet] init [--] [<path>...]
-'git submodule' [--quiet] deinit [-f|--force] (--all|[--] <path>...)
-'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
+'git submodule' [--quiet] status [--error-unmatch] [--cached] [--recursive] [--] [<path>...]
+'git submodule' [--quiet] init [--error-unmatch] [--] [<path>...]
+'git submodule' [--quiet] deinit [--error-unmatch] [-f|--force] (--all|[--] <path>...)
+'git submodule' [--quiet] update [--error-unmatch] [--init] [--remote] [-N|--no-fetch]
[-f|--force] [--rebase|--merge] [--reference <repository>]
[--depth <depth>] [--recursive] [--] [<path>...]
'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) <n>]
[commit] [--] [<path>...]
'git submodule' [--quiet] foreach [--recursive] <command>
-'git submodule' [--quiet] sync [--recursive] [--] [<path>...]
+'git submodule' [--quiet] sync [--error-unmatch] [--recursive] [--] [<path>...]
DESCRIPTION
@@ -260,6 +260,11 @@ OPTIONS
The name of the branch is recorded as `submodule.<name>.branch` in
`.gitmodules` for `update --remote`.
+--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.
+
-f::
--force::
This option is only valid for add, deinit and update commands.
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)
{
int i, result = 0;
char *ps_matched = NULL;
@@ -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;
ALLOC_GROW(list->entries, list->nr + 1, list->alloc);
@@ -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;
free(ps_matched);
@@ -63,11 +65,12 @@ static int module_list_compute(int argc, const char **argv,
static int module_list(int argc, const char **argv, const char *prefix)
{
- int i;
+ int i, unmatch = 0;
struct pathspec pathspec;
struct module_list list = MODULE_LIST_INIT;
struct option module_list_options[] = {
+ OPT_BOOL(0, "unmatch", &unmatch, N_("Do not report error if no matches are found")),
OPT_STRING(0, "prefix", &prefix,
N_("path"),
N_("alternative anchor for relative paths")),
@@ -82,7 +85,7 @@ static int module_list(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, module_list_options,
git_submodule_helper_usage, 0);
- if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0) {
+ if (module_list_compute(argc, argv, prefix, &pathspec, &list, unmatch) < 0) {
printf("#unmatched\n");
return 1;
}
diff --git a/git-submodule.sh b/git-submodule.sh
index fb68f1f..f10e10a 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -6,13 +6,13 @@
dashless=$(basename "$0" | sed -e 's/-/ /')
USAGE="[--quiet] add [-b <branch>] [-f|--force] [--name <name>] [--reference <repository>] [--] <repository> [<path>]
- or: $dashless [--quiet] status [--cached] [--recursive] [--] [<path>...]
- or: $dashless [--quiet] init [--] [<path>...]
- or: $dashless [--quiet] deinit [-f|--force] (--all| [--] <path>...)
- or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--reference <repository>] [--recursive] [--] [<path>...]
+ or: $dashless [--quiet] status [--error-unmatch] [--cached] [--recursive] [--] [<path>...]
+ or: $dashless [--quiet] init [--error-unmatch] [--] [<path>...]
+ or: $dashless [--quiet] deinit [--error-unmatch] [-f|--force] (--all| [--] <path>...)
+ or: $dashless [--quiet] update [--error-unmatch] [--init] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--reference <repository>] [--recursive] [--] [<path>...]
or: $dashless [--quiet] summary [--cached|--files] [--summary-limit <n>] [commit] [--] [<path>...]
or: $dashless [--quiet] foreach [--recursive] <command>
- or: $dashless [--quiet] sync [--recursive] [--] [<path>...]"
+ or: $dashless [--quiet] sync [--error-unmatch] [--recursive] [--] [<path>...]"
OPTIONS_SPEC=
SUBDIRECTORY_OK=Yes
. git-sh-setup
@@ -391,6 +391,9 @@ cmd_foreach()
--recursive)
recursive=1
;;
+ --error-unmatch)
+ unmatch=1
+ ;;
-*)
usage
;;
@@ -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"|
while read mode sha1 stage sm_path
do
die_if_unmatched "$mode"
@@ -453,6 +456,9 @@ cmd_init()
-q|--quiet)
GIT_QUIET=1
;;
+ --error-unmatch)
+ unmatch=1
+ ;;
--)
shift
break
@@ -467,7 +473,7 @@ cmd_init()
shift
done
- git submodule--helper list --prefix "$wt_prefix" "$@" |
+ git submodule--helper list ${unmatch:+--unmatch} --prefix "$wt_prefix" "$@" |
while read mode sha1 stage sm_path
do
die_if_unmatched "$mode"
@@ -534,6 +540,9 @@ cmd_deinit()
--all)
deinit_all=t
;;
+ --error-unmatch)
+ unmatch=1
+ ;;
--)
shift
break
@@ -558,7 +567,7 @@ cmd_deinit()
die "$(eval_gettext "Use '--all' if you really want to deinitialize all submodules")"
fi
- git submodule--helper list --prefix "$wt_prefix" "$@" |
+ git submodule--helper list ${unmatch:+--unmatch} --prefix "$wt_prefix" "$@" |
while read mode sha1 stage sm_path
do
die_if_unmatched "$mode"
@@ -661,6 +670,9 @@ cmd_update()
--recursive)
recursive=1
;;
+ --error-unmatch)
+ unmatch=1
+ ;;
--checkout)
update="checkout"
;;
@@ -692,7 +704,7 @@ cmd_update()
fi
cloned_modules=
- git submodule--helper list --prefix "$wt_prefix" "$@" | {
+ git submodule--helper list ${unmatch:+--unmatch} --prefix "$wt_prefix" "$@" | {
err=
while read mode sha1 stage sm_path
do
@@ -1115,6 +1127,9 @@ cmd_status()
--recursive)
recursive=1
;;
+ --error-unmatch)
+ unmatch=1
+ ;;
--)
shift
break
@@ -1129,7 +1144,7 @@ cmd_status()
shift
done
- git submodule--helper list --prefix "$wt_prefix" "$@" |
+ git submodule--helper list ${unmatch:+--unmatch} --prefix "$wt_prefix" "$@" |
while read mode sha1 stage sm_path
do
die_if_unmatched "$mode"
@@ -1193,6 +1208,10 @@ cmd_sync()
recursive=1
shift
;;
+ --error-unmatch)
+ unmatch=1
+ shift
+ ;;
--)
shift
break
@@ -1206,7 +1225,7 @@ cmd_sync()
esac
done
cd_to_toplevel
- git submodule--helper list --prefix "$wt_prefix" "$@" |
+ git submodule--helper list ${unmatch:+--unmatch} --prefix "$wt_prefix" "$@" |
while read mode sha1 stage sm_path
do
die_if_unmatched "$mode"
--
2.8.0.2.gaa9b48a.dirty
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] submodule operations: tighten pathspec errors
2016-05-21 1:21 [PATCH] submodule operations: tighten pathspec errors Stefan Beller
@ 2016-05-26 20:00 ` Junio C Hamano
2016-06-01 20:55 ` Stefan Beller
0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2016-05-26 20:00 UTC (permalink / raw)
To: Stefan Beller; +Cc: git
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] submodule operations: tighten pathspec errors
2016-05-26 20:00 ` Junio C Hamano
@ 2016-06-01 20:55 ` Stefan Beller
2016-06-01 21:14 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Beller @ 2016-06-01 20:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] submodule operations: tighten pathspec errors
2016-06-01 20:55 ` Stefan Beller
@ 2016-06-01 21:14 ` Junio C Hamano
2016-06-01 21:20 ` Junio C Hamano
2016-06-06 19:31 ` Stefan Beller
0 siblings, 2 replies; 6+ messages in thread
From: Junio C Hamano @ 2016-06-01 21:14 UTC (permalink / raw)
To: Stefan Beller; +Cc: git
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] submodule operations: tighten pathspec errors
2016-06-01 21:14 ` Junio C Hamano
@ 2016-06-01 21:20 ` Junio C Hamano
2016-06-06 19:31 ` Stefan Beller
1 sibling, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2016-06-01 21:20 UTC (permalink / raw)
To: Stefan Beller; +Cc: git
Junio C Hamano <gitster@pobox.com> writes:
>> 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.
Sorry, I should have grepped before hitting "send". I think this is
in line with "git rm --ignore-unmatch". We complain by default, but
the user can choose to squelch.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] submodule operations: tighten pathspec errors
2016-06-01 21:14 ` Junio C Hamano
2016-06-01 21:20 ` Junio C Hamano
@ 2016-06-06 19:31 ` Stefan Beller
1 sibling, 0 replies; 6+ messages in thread
From: Stefan Beller @ 2016-06-06 19:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Wed, Jun 1, 2016 at 2:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.
I fixed all the variable names and was confident with what I had,
but this is an issue indeed as the error message is plain wrong:
git$ git submodule--helper list sub*
error: pathspec 'submodule.c' did not match any file(s) known to git.
error: pathspec 'submodule-config.c' did not match any file(s) known to git.
error: pathspec 'submodule-config.h' did not match any file(s) known to git.
error: pathspec 'submodule-config.o' did not match any file(s) known to git.
error: pathspec 'submodule.h' did not match any file(s) known to git.
error: pathspec 'submodule_multiple_references' did not match any
file(s) known to git.
error: pathspec 'submodule.o' did not match any file(s) known to git.
error: pathspec 'submodulespec.o' did not match any file(s) known to git.
(I realize I have some stray object files laying around)
So I do not think we can do
- 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))
or we need to have custom error reporting, which checks if any file
still matches the pathspec (and ignore non gitlinks)
>
> When we know we are not going to complain (i.e. --unmatch-ok option
I'd rather go with ignore-unmatch as git-rm does use that.
> 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().
As said, in that case we would then need a
pathspec_mark_ps_matched(ce->name, ce_namelen(ce), ps_matched)
>
> 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.
I see.
if (!S_ISGITLINK(ce->ce_mode)) {
pathspec_mark_ps_matched(...);
continue;
} else {
if ( !match_pathspec(pathspec, ce->name, ce_namelen(ce),
0, ps_matched, 1))
continue;
}
...
doesn't look very appealing, so I guess we'd just keep the current behavior
of
- if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),
- 0, ps_matched, 1) ||
- !S_ISGITLINK(ce->ce_mode))
>
>>> 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.
Yes for one call; No from the birds eye view;
the first lines that use the seen[]:
if (seen && seen[i] == MATCHED_EXACTLY)
continue;
so if we have `ps->nr > 0` then it is beneficial to have a
seen array, I think?
>
>>>> @@ -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.
done.
>>>
>>> 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.
--ignore-unmatch has the same meaning and is taken by git-rm already?
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-06-06 19:31 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-21 1:21 [PATCH] submodule operations: tighten pathspec errors Stefan Beller
2016-05-26 20:00 ` Junio C Hamano
2016-06-01 20:55 ` Stefan Beller
2016-06-01 21:14 ` Junio C Hamano
2016-06-01 21:20 ` Junio C Hamano
2016-06-06 19:31 ` Stefan Beller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.