Re: partial commit of file-to-directory change, was Re: Bugreport

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 25, 2024, 18:54 UTC
Message-ID: <xmqqjznxw8ev.fsf@gitster.g>
In-Reply-To: <20240120004628.GA117170@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 16 quoted lines

> On Fri, Jan 19, 2024 at 11:14:47PM +0000, brian m. carlson wrote:
>
>> > $ git commit . -m +
>> > error: 'd' does not have a commit checked out
>> > fatal: updating files failed
>> 
>> I can confirm this behaviour[0], and it's definitely wrong that it
>> thinks `d` is a submodule.  It does, however, work if you do `git commit
>> -m +` (that is, without the .), which makes sense since the relevant
>> change is already staged in the index.
>> 
>> I'm not going to get to a patch or more thorough investigation, but
>> perhaps someone else will.
>
> I peeked at this a bit earlier; I didn't come up with a solution, but
> here are a few more details.

I didn't either X-<, and the thing is somewhat nasty, as there are two states (HEAD and the index) involved.

 * For paths that do not match the pathspec, we want to take the
   version from the HEAD.  For paths that do match the pathspec, we
   want to take the version from the working tree.  And after making
   the partial commit with such changes, we want to make the same
   change to the index to prepare for the next commit.

 * With the way the code is currently structured, we find paths that
   appear either in the index or in the HEAD to match against the
   pathspec.  This is so that we can honor an earlier "git rm" since
   we read HEAD.

 * Then we check each of these paths that are either in the index or
   in HEAD that matched the pathspec.  If it is missing from the
   working tree, we would remove it from both the temporary ndex
   used for the partial commit, and the real index that we'll
   continue to use after the partial commit.  If it exists in the
   working tree, we would take the contents of it to update.

   For the purpose of this operation, a path D that was a blob in
   the index should be _removed_ when it is a directory in the
   working tree (i.e. made room to create D/F).  And we should not
   add D/F when running "git commit D" or "git commit D/F", because
   the partial commit does not "git add" (it only does "git add -u")

In the example in the discussion, we had D that was a blob and got replaced with a directory. If we did "git add -u D", we would just have removed D from the index, so the partial commit should do the same. Which means we need to know the type of the thing we expect. But list_paths() only returns a list of path names, and does not give us the type of the thing we expect. We use the same list twice, once to update the temporary index (which we create by reading HEAD) and update the paths listed there from the working tree. And the same list is used again to update the real index (which is what the user had before starting the command) and update the paths listed there from the working tree. The tricky part is that the type of (or even existence of) D may have changed between the HEAD and the index, but we want to perform the same operation to the real index with respect to the paths we touched to create the partial commit.

Naively, add_remove_files() looks like an obvious place to see if the path is in the working tree (which we already do) *and* compare it with "the type of the thing we expect", but the thing is that this function is called twice with different index (once with the temporary index that started from HEAD, i.e. oblivious to what the user did to the index since HEAD; then again with the real index with the changes the user made since HEAD), so we cannot just say "let's see what this path D is in the index".

I _think_ we would need to update list_paths() so that it records a bit more than just pathnames that match the pathspec, namely, what should be done to the path, by doing lstat() on the working tree paths.