Re: [PATCH] submodule helper: accept '.' for repositories with no submodules

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 22, 2016, 18:53 UTC
Message-ID: <xmqqy49agx98.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1458669579-32160-1-git-send-email-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 11 quoted lines

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