Bug with "git mv" and submodules, and with "git submodule add something --force"
- From
Stefan Beller <sbeller@google.com>
- Date
- Oct 22, 2018, 21:52 UTC
- Message-ID
- <20181022215236.72238-1-sbeller@google.com>
- In-Reply-To
- <xmqqd0s2y9o9.fsf@gitster-ct.c.googlers.com>
On Sun, Oct 21, 2018 at 7:52 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 14 quoted lines
> > Jonathan Nieder <jrnieder@gmail.com> writes: > > > Stefan Beller wrote: > > > >> Maybe for now we can do with just an update of the documentation/bugs > >> section and say we cannot move files in and out of submodules? > > > > I think we have some existing logic to prevent "git add"-ing a file > > within a submodule to the superproject, for example. > > There is die_path_inside_submodule() that sanity-checks the pathspec > and rejects. But I think that is done primarily to give an error > message and not strictly necesary for correctness.
c08397e3aa (pathspec: remove PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag, 2017-05-11) seems to be relevant here, as that factors out the warning.
> The real safety of "git add" is its call to dir.c::fill_directory(); > it collects untracked paths that match the pathspec so that they can > be added as new paths, but because it won't cross the module > boundary, you won't get such a path in the index to begin with.
It would not cross the boundary and fail silently as it would treat a path into the submodule as a no-op.
> > So "git mv" should learn the same trick. And perhaps the trick needs > > to be moved down a layer (e.g. into the index API). Hints?
Yeah, I think we'd want to teach git-mv about that trick.
Unfortunately git-mv is one of the last remainders of not properly using pathspecs, and die_path_inside_submodule expects a pathspec. :-/
Show 8 quoted lines
> You would want to be able to remove a submodule and replace it with > a directory, but you can probably do it in two steps, i.e. > > git reset --hard > git rm --cached sha1collisiondetection > echo Now a regular dir >sha1collisiondetection/READ.ME > find sha1collisiondetection ! -type d -print0 | > git update-index --add --stdin -z
"Ignoring path sha1collisiondetection/.git"
Nice!
Show 13 quoted lines
> > So from that point of view, forbidding (starting from the same state > of our project) this sequence: > > git reset --hard > echo Now a regular dir >sha1collisiondetection/READ.ME > find sha1collisiondetection ! -type d -print0 | > git update-index --add --remove --stdin -z > > that would nuke the submodule and replace it with a directory within > which there are files would be OK. Making the latter's default > rejection overridable with ADD_CACHE_OK_TO_REPLACE would also be > fine.
I am having trouble of relating these commands to the original git-mv across submodule boundaries.
Moving files from the submodule out to the superproject, seems to fail properly, though having a less-than-optimal error message:
$ git mv sha1collisiondetection/LICENSE.txt . fatal: not under version control, source=sha1collisiondetection/LICENSE.txt, destination=LICENSE.txt
and moving things inside was the original report, below is a proof of concept that would yield
./git mv -v cache.h sha1collisiondetection/ fatal: moving across submodule boundaries not supported, source=cache.h, destination=sha1collisiondetection/cache.h
--8<-- Subject: [WIP/PATCH] builtin/mv.c: disallow moving across submodule boundaries
Signed-off-by: Stefan Beller <sbeller@google.com> --- builtin/mv.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/builtin/mv.c b/builtin/mv.c index 80bb967a63..9ec4b2f0a3 100644 --- a/builtin/mv.c +++ b/builtin/mv.c @@ -172,7 +172,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix) /* Checking */ for (i = 0; i < argc; i++) { const char *src = source[i], *dst = destination[i]; - int length, src_is_dir; + int length, src_is_dir, pos; const char *bad = NULL; if (show_only) @@ -243,6 +243,13 @@ int cmd_mv(int argc, const char **argv, const char *prefix) else string_list_insert(&src_for_dst, dst); + pos = cache_name_pos(dst, strlen(dst)); + if (pos < 0) { + pos = -(pos + 1); + if (!S_ISGITLINK(active_cache[pos]->ce_mode)) + bad = _("moving across submodule boundaries not supported"); + } + if (!bad) continue; if (!ignore_errors)
-- 2.19.0