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

[Stefan Beller <sbeller@google.com>]

On Tue, Mar 22, 2016 at 2:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>> I do not think that buys us much.  You have already shown how to
>>> implement "filter first and then pathspec match" if a caller
>>> wants to (which turned out to be a regression in this case, but
>>> that is besides the point).
>>
>> And by including this filtering into the pathspec machine we can pass
>> a flag DONT_BREAK_ON_NO_FILTER_RESULTS_WHEN_HAVING_OTHER_MATCHES
>> (name for illustration purpose only ;) which is how I understand this
>> regression?
>
> But you do not even need that if you fix the regression with
> something like this, no?  Do we need to add complexity to pathspec
> machinery only to make it easier to misuse it and then add another
> DONT_BREAK_... band-aid to fix a bug that can come from such a
> misuse?
>
>  builtin/submodule--helper.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index ed764c9..740b57a 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -37,10 +37,11 @@ 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 (!S_ISGITLINK(ce->ce_mode) ||
> -                   !match_pathspec(pathspec, ce->name, ce_namelen(ce),
> +               if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),
>                                     0, ps_matched, 1))
>                         continue;
> +               if (!S_ISGITLINK(ce->ce_mode))
> +                       continue;
>
>                 ALLOC_GROW(list->entries, list->nr + 1, list->alloc);
>                 list->entries[list->nr++] = ce;


This patch is conceptually, what you said in the first message as

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

I was under the impression that we do not want to have this bugfix
(at least long term) and then I tried to come up with an idea, which
is both:
* correct in this case
* and catches the git submodule init -- 'COPYIN*' case as failure