Re: [PATCHv5] pathspec: give better message for submodule related pathspec error

From: Stefan Beller <sbeller@google.com>
Date: Jan 4, 2017, 23:28 UTC
Message-ID: <CAGZ79kbo3uKD5Un_P+EPdO2mMt6WUVjWPqfYEbitzU0zw0ex3A@mail.gmail.com>
In-Reply-To: <20170104231605.GB68241@google.com>

> I haven't taken a through look at this patch but I think you may want to
> base it off of 'origin/bw/pathspec-cleanup' series as the changes made in this
> patch now conflict with that series.

eh right, I forgot to mention this in the notes, it requires sb/submodule-embed-gitdir as well, so I'll have to figure that out.

>
> Also I still don't really think this solves the problem of telling the
> user what is wrong, which is that the submodule's gitdir is gone.
>

The "git dir gone" is not a big deal IMHO as a deinitialized submodule is perfectly fine (e.g. not initialized). The errors as I tested in Gerrit, a superproject that contains submodules in plugins/* :

    : gerrit/plugins/cookbook-plugin$ git add .
    fatal: Pathspec '.' is in submodule 'plugins/cookbook-plugin'
    : gerrit/plugins/cookbook-plugin$ cd ..
    : gerrit/plugins$ git add cookbook-plugin/a
    fatal: Pathspec 'cookbook-plugin/a' is in submodule
'plugins/cookbook-plugin'
    : gerrit/plugins$ git add cookbook-plugin/.
    : gerrit/plugins$ git add cookbook-plugin/./.
    : gerrit/plugins$

I think that is perfect behavior for now, as it reliably detects (a) the submodule being there and (b) if you are in there, no matter if there is a .git dir or not.

The same error coming up if the submodule is initialized and valid, e.g.

    : gerrit/plugins$ git submodule update --init cookbook-plugin
    : gerrit/plugins$ git add cookbook-plugin/a/.
    fatal: Pathspec 'cookbook-plugin/a/.' is in submodule
'plugins/cookbook-plugin'

So I think this is pretty much exactly what we want for now:
* if PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE is set
   we keep the behavior as is and do the expensive thing
* if the caller wants to use path inside of a submodule no matter
  the git dir of the submodule, then set the CHEAP flag instead
* in case of the assert (that I originally wanted to fix), we fall back to the
  EXPENSIVE thing reporting the error message that we already reported
  in such cases.

TL;DR: I was rather asking about the code being a viable; by now I am convinced this is the correct behavior. ;)