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

[Jeff King <peff@peff.net>]

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.

As you noted, the problem is only with giving a pathspec to "git
commit". The bug is in the custom code git-commit uses to set up the
index for a partial commit. It generates a list of entries for the
partial commit via list_path(), and then tries to add them to the index.
But of course "d", being a directory, does not make any sense as an
index entry itself (unless as a submodule).

I'd have thought that we should just be using the same code that "git
add" does here (which obviously works). But all of this comes from
2888605c64 (builtin-commit: fix partial-commit support, 2007-11-18),
which explicitly says "you can't just run add_files_to_cache()", which
is what git-add does under the hood.

I don't know if we could revisit those assumptions and reuse some of the
existing code, or if the custom partial-commit code could be fixed.  I
guess the root of the issue is that in the call to list_paths(), we call
overlay_tree_on_index(). And that's how we end up thinking that "d" is a
useful path to talk about, even though it has already been removed from
the index (we have "d/b.txt" instead).

So I'm not sure if we need a solution here-ish:

diff --git a/builtin/commit.c b/builtin/commit.c
index 65196a2827..25e65e986d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -302,7 +302,9 @@ static void add_remove_files(struct string_list *list)
 			continue;
 
 		if (!lstat(p->string, &st)) {
-			if (add_to_index(&the_index, p->string, &st, 0))
+			if (S_ISDIR(st.st_mode))
+				warning("woah, %s is a dir", p->string);
+			else if (add_to_index(&the_index, p->string, &st, 0))
 				die(_("updating files failed"));
 		} else
 			remove_file_from_index(&the_index, p->string);

but I'm not quite sure what we should be doing instead of that
warning(). Should we updating and including everything in "d/"? What
about if there were a "d/c.txt" that was not a tracked file at all?

It might also be that the bug is earlier in list_paths(), where we
should notice that "d" is gone and now we have entries under "d/".

I dunno. I probably won't dig much further on this myself, but it's
possible Junio (cc'd) might have an idea right away.

-Peff