On Sun, Oct 21, 2018 at 7:52 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Jonathan Nieder <jrnieder@xxxxxxxxx> 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. :-/
> 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!
>
> 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@xxxxxxxxxx>
---
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
