Re: [PATCH/RFC 1/3] Teach mv to move submodules together with their work trees

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 11, 2013, 16:46 UTC
Message-ID: <7vd2u1j9q5.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CALkWK0mWHwTV8dc9F3tyq9HYOnC2S56x_efr1+eRsqJqTFutXA@mail.gmail.com>

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Show 10 quoted lines

> Why does it search for a submodule with a trailing slash in the index?
>  You make it sound like it's doing something unnatural; in reality, it
> does this because it executes lstat() on the filesystem path
> specified, and the stat mode matches S_ISDIR (because it _is_ a
> directory on the filesystem).  It is stored in the index as an entry
> (without a trailing slash) with the mode 160000, gitlink.
>
> What happens if I attempt to git mv oldpath/ newpath/ (with the
> literal slashes, probably because I'm using a stupid shell
> completion)?

I think it should work.

	mkdir a && >a/f && git add a/f && git mv a/ b/

Show 6 quoted lines

>> Fix that by searching for the name without a trailing slash and continue
>> if it is a submodule.
>
> So, the correct solution is not to "search for a name without a
> trailing slash", but rather to handle the gitlink entry in the index
> appropriately.

For moving an entire directory's contents, because the index is flat, you would look for "name/", because you know all of the paths contained in it will share that prefix.

But when dealing with a submodule, you need to see if the path that found to be a directory in the working tree is a gitlink in the index. And the way to do so is to look for it without trailing slash, because there is nothing hanging under it in the index.

So the right implementation of "handle appropriately" is to "search without slash". They are saying the same thing, and the latter is a more specific way to point out in what way the existing code that wanted to delegate moving to "submodule mv" and not having to worry about gitlinks was unprepared for it, and what change is needed to make it "handle appropriately".

So I think there is no need to rephrase here, but your comment makes me wonder if the patch covers the case where you have a submodule at "a/x" and the user does "git mv a/ b/". The src_is_dir thing will notice "a/" is a directory, finds all the paths inside a/ including "a/x" (and we won't see any paths inside the submodule's working tree) to "b/", and update the cache entries and the working tree. Does the adjusting of the path for that moved submodule, which is a theme for [PATCH 3/3], cover that case, too?

Another thing to wonder is if we correctly reject "git mv a/x/f here" in the same example where a/x is a directory. The path is beyond the lower boundary of our working tree and should not be touched.