partial commit of file-to-directory change, was Re: Bugreport
- From
Jeff King <peff@peff.net>
- Date
- Jan 20, 2024, 00:46 UTC
- Message-ID
- <20240120004628.GA117170@coredump.intra.peff.net>
- In-Reply-To
- <ZasCZ0YetzmlBFvw@tapette.crustytoothpaste.net>
On Fri, Jan 19, 2024 at 11:14:47PM +0000, brian m. carlson wrote:
Show 11 quoted lines
> > $ 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