Stefan Beller [off-list ref] writes:
In 74703a1e4d (2015-09-02, submodule: rewrite `module_list` shell
function in C), "submodule deinit ." was broken.
The original module_list accepted '.' as a pathspec just fine, as it
was using
git ls-files -z --error-unmatch --stage -- "$@" || { custom filtering}
and git ls-files doesn't make a difference between "." and no arguments
there. When using the parse_pathspec function in C, this is a difference
however, when no path matches.
Is that an accurate description of the issue?
The original (above) errors out if there is a pathspec that does not
match any path in the index. The C rewrite however instead does
this:
if (!S_ISGITLINK(ce->ce_mode) ||
!match_pathspec(pathspec, ce->name, ce_namelen(ce),
0, ps_matched, 1))
continue;
to error out if there is a pathspec that does not match any
submodule path. That is the root cause of the difference in
behaviour.
So if we were to aim for a faithful rewrite, perhaps swapping the
order of the check, i.e.
if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),
0, ps_matched, 1) ||
!S_ISGITLINK(ce->ce_mode))
continue;
Now, arguably, the behaviour of C rewrite makes more sense in that
it would diagnose a pathspec that does not match a submodule as an
error, e.g.
$ git submodule--helper list 'COPYIN*'
error: pathspec 'COPYIN*' did not match any file(s) known to git.
#unmatched
The error message _is_ wrong, but the end result is more helpful to
the user---the user thought there was a submodule that would match
that pathspec, and there isn't, so we suspect there was a typo and
cautiously error out.
"submodule deinit ." may have "worked" in the sense that you would
have at least one path in your tree and avoided this "nothing
matches" most of the time. It would have still failed with the
exactly same error if run in an empty repository, i.e.
$ E=/var/tmp/x/empty && rm -fr "$E" && mkdir -p "$E" && cd "$E"
$ git init
$ rungit v2.6.6 submodule deinit .
error: pathspec '.' did not match any file(s) known to git.
Did you forget to 'git add'?
$ >file && git add file
$ rungit v2.6.6 submodule deinit .
$ echo $?
0
In other words, "Use '.' if you really want to" is a faulty
suggestion. There is no guarantee that it would match anything in
the old world order, and certainly there is no guarantee that it
would match any submodule in the new world order.
When another person who is not Per Cederqvist realizes that the
logic that issues the faulty suggestion is because the command wants
some pathspec, she may try
$ git submodule deinit '*'
and complain that it used to work but it no longer, even with the
band-aid patch under discussion that special cases '.'.
So I dunno. This is not only "deinit", but also the post rewrite
version catches
$ git submodule init -- 'COPYIN*'
as an error, which we probably would want to keep, so I am reluctant
to suggest swapping the order of the check to do pathspec first and
then gitlink-ness (it has performance implications but correctness
is a more important issue), but if we want to keep the backward
compatibility, that would be the best bug-to-bug compatible fix in
the shorter term.
On Tue, Mar 22, 2016 at 11:53 AM, Junio C Hamano [off-list ref] wrote:
Stefan Beller [off-list ref] writes:
quoted
In 74703a1e4d (2015-09-02, submodule: rewrite `module_list` shell
function in C), "submodule deinit ." was broken.
The original module_list accepted '.' as a pathspec just fine, as it
was using
git ls-files -z --error-unmatch --stage -- "$@" || { custom filtering}
and git ls-files doesn't make a difference between "." and no arguments
there. When using the parse_pathspec function in C, this is a difference
however, when no path matches.
Is that an accurate description of the issue?
The original (above) errors out if there is a pathspec that does not
match any path in the index. The C rewrite however instead does
this:
if (!S_ISGITLINK(ce->ce_mode) ||
!match_pathspec(pathspec, ce->name, ce_namelen(ce),
0, ps_matched, 1))
continue;
to error out if there is a pathspec that does not match any
submodule path. That is the root cause of the difference in
behaviour.
So if we were to aim for a faithful rewrite, perhaps swapping the
order of the check, i.e.
if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),
0, ps_matched, 1) ||
!S_ISGITLINK(ce->ce_mode))
continue;
Now, arguably, the behaviour of C rewrite makes more sense in that
it would diagnose a pathspec that does not match a submodule as an
error, e.g.
$ git submodule--helper list 'COPYIN*'
error: pathspec 'COPYIN*' did not match any file(s) known to git.
#unmatched
The error message _is_ wrong, but the end result is more helpful to
the user---the user thought there was a submodule that would match
that pathspec, and there isn't, so we suspect there was a typo and
cautiously error out.
"submodule deinit ." may have "worked" in the sense that you would
have at least one path in your tree and avoided this "nothing
matches" most of the time. It would have still failed with the
exactly same error if run in an empty repository, i.e.
$ E=/var/tmp/x/empty && rm -fr "$E" && mkdir -p "$E" && cd "$E"
$ git init
$ rungit v2.6.6 submodule deinit .
error: pathspec '.' did not match any file(s) known to git.
Did you forget to 'git add'?
$ >file && git add file
$ rungit v2.6.6 submodule deinit .
$ echo $?
0
In other words, "Use '.' if you really want to" is a faulty
suggestion. There is no guarantee that it would match anything in
the old world order, and certainly there is no guarantee that it
would match any submodule in the new world order.
When another person who is not Per Cederqvist realizes that the
logic that issues the faulty suggestion is because the command wants
some pathspec, she may try
$ git submodule deinit '*'
and complain that it used to work but it no longer, even with the
band-aid patch under discussion that special cases '.'.
So I dunno. This is not only "deinit", but also the post rewrite
version catches
$ git submodule init -- 'COPYIN*'
as an error, which we probably would want to keep, so I am reluctant
to suggest swapping the order of the check to do pathspec first and
then gitlink-ness (it has performance implications but correctness
is a more important issue), but if we want to keep the backward
compatibility, that would be the best bug-to-bug compatible fix in
the shorter term.
So I have 2 goals in mind:
* Git suggested to use '.' explicitly, so it should just work --even
for a completely
empty repository (see the test for it)
* Eventually -- not in this patch, but a later patch targeted at
master -- we want to
remove the recommendation to use '.', and allow no arguments or a
different argument
for "all submodules". git add uses '.' for it though, so '.' seems
right and valid.
git add '*' is also valid.
Maybe combine the second idea with a slight refactoring of
parse_pathspec, such that you
can pass a callback function to parse_pathspec which can decided on
each file if it is
a file to inspect. (i.e. for our usecase we'd check for ce_mode to be
GITLINK, another
hypothetical use case would be using parse_pathspec for finding all
files with a certain
property, e.g. finding all files ending in .c or files written in all
capital letters or such)
Then you could do a conditional parse_pathspec over the partial
repository which matched
the additional filtering function.
Maybe we can also special case the "force" argument only here for
Cedars use case.
("git submodule deinit ." complains because there are no further
submodules, but -f solves
the complaint?)