Re: [PATCH] submodule helper: accept '.' for repositories with no submodules
On Tue, Mar 22, 2016 at 2:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 39 quoted lines
> 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
Show 11 quoted lines
> 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
Stefan Beller <sbeller@google.com>