{"thread":{"id":"60767","subject":"Bugreport","startedAt":"2024-01-19T13:36:13Z","lastAt":"2024-01-25T18:54:35Z","messageCount":6,"participants":["Frank Schwidom","brian m. carlson","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"487069","messageId":"20240119132551.GA31532@debian64","threadId":"60767","inReplyTo":null,"subject":"Bugreport","fromName":"Frank Schwidom","fromEmail":"schwidom@gmx.net","sentAt":"2024-01-19T13:25:51Z","receivedAt":"2024-01-19T13:36:13Z","isPatch":false,"sender":{"key":"schwidom@gmx.net","avatar":null},"body":"\nThis bug exists in possibly all git versions.\n\n$ git init\n$ touch a.txt\n$ ln -s a.txt d\n$ git add .\n$ git commit -m + .\n[master (root-commit) f6b4468] +\n 2 files changed, 1 insertion(+)\n create mode 100644 a.txt\n create mode 120000 d\n$ ls -la\ntotal 12\ndrwxr-xr-x 3 ox ox 4096 Jan 19 14:10 .\ndrwxr-xr-x 4 ox ox 4096 Jan 19 14:04 ..\ndrwxr-xr-x 8 ox ox 4096 Jan 19 14:10 .git\n-rw-r--r-- 1 ox ox    0 Jan 19 14:10 a.txt\nlrwxrwxrwx 1 ox ox    5 Jan 19 14:10 d -> a.txt\n$ rm d\n$ mkdir d\n$ touch d/b.txt\n$ git add .\n$ git commit . -m +\nerror: 'd' does not have a commit checked out\nfatal: updating files failed\n\n\n# I expect that git just replaces the link by the directory. But it makes problems.\n\n# Workaround:\n\n$ rm -rf d\n$ git add .\n$ git commit -m + .\n[master 522e6db] +\n 1 file changed, 1 deletion(-)\n delete mode 120000 d\n$ mkdir d\n$ touch d/b.txt\n$ git add .\n$ git commit -m + .\n[master 8a125ee] +\n 1 file changed, 0 insertions(+), 0 deletions(-)\n create mode 100644 d/b.txt\n\n[System Info]\ngit version:\ngit version 2.43.0\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 6.1.0-8-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.1.25-1 (2023-04-22) x86_64\ncompiler info: gnuc: 13.2\nlibc info: glibc: 2.37\n$SHELL (typically, interactive shell): /bin/bash\n\n\n[Enabled Hooks]\n\nThanks in advance,\nFrank Schwidom\n"},{"id":"487117","messageId":"ZasCZ0YetzmlBFvw@tapette.crustytoothpaste.net","threadId":"60767","inReplyTo":"20240119132551.GA31532@debian64","subject":"Re: Bugreport","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-01-19T23:14:47Z","receivedAt":"2024-01-19T23:14:57Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-01-19 at 13:25:51, Frank Schwidom wrote:\n> \n> This bug exists in possibly all git versions.\n> \n> $ git init\n> $ touch a.txt\n> $ ln -s a.txt d\n> $ git add .\n> $ git commit -m + .\n> [master (root-commit) f6b4468] +\n>  2 files changed, 1 insertion(+)\n>  create mode 100644 a.txt\n>  create mode 120000 d\n> $ ls -la\n> total 12\n> drwxr-xr-x 3 ox ox 4096 Jan 19 14:10 .\n> drwxr-xr-x 4 ox ox 4096 Jan 19 14:04 ..\n> drwxr-xr-x 8 ox ox 4096 Jan 19 14:10 .git\n> -rw-r--r-- 1 ox ox    0 Jan 19 14:10 a.txt\n> lrwxrwxrwx 1 ox ox    5 Jan 19 14:10 d -> a.txt\n> $ rm d\n> $ mkdir d\n> $ touch d/b.txt\n> $ git add .\n> $ git commit . -m +\n> error: 'd' does not have a commit checked out\n> fatal: updating files failed\n\nI can confirm this behaviour[0], and it's definitely wrong that it\nthinks `d` is a submodule.  It does, however, work if you do `git commit\n-m +` (that is, without the .), which makes sense since the relevant\nchange is already staged in the index.\n\nI'm not going to get to a patch or more thorough investigation, but\nperhaps someone else will.\n\n[0] git version 2.43.0.381.gb435a96ce8\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"487119","messageId":"20240120004628.GA117170@coredump.intra.peff.net","threadId":"60767","inReplyTo":"ZasCZ0YetzmlBFvw@tapette.crustytoothpaste.net","subject":"partial commit of file-to-directory change, was Re: Bugreport","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-20T00:46:28Z","receivedAt":"2024-01-20T00:46:37Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 19, 2024 at 11:14:47PM +0000, brian m. carlson wrote:\n\n> > $ git commit . -m +\n> > error: 'd' does not have a commit checked out\n> > fatal: updating files failed\n> \n> I can confirm this behaviour[0], and it's definitely wrong that it\n> thinks `d` is a submodule.  It does, however, work if you do `git commit\n> -m +` (that is, without the .), which makes sense since the relevant\n> change is already staged in the index.\n> \n> I'm not going to get to a patch or more thorough investigation, but\n> perhaps someone else will.\n\nI peeked at this a bit earlier; I didn't come up with a solution, but\nhere are a few more details.\n\nAs you noted, the problem is only with giving a pathspec to \"git\ncommit\". The bug is in the custom code git-commit uses to set up the\nindex for a partial commit. It generates a list of entries for the\npartial commit via list_path(), and then tries to add them to the index.\nBut of course \"d\", being a directory, does not make any sense as an\nindex entry itself (unless as a submodule).\n\nI'd have thought that we should just be using the same code that \"git\nadd\" does here (which obviously works). But all of this comes from\n2888605c64 (builtin-commit: fix partial-commit support, 2007-11-18),\nwhich explicitly says \"you can't just run add_files_to_cache()\", which\nis what git-add does under the hood.\n\nI don't know if we could revisit those assumptions and reuse some of the\nexisting code, or if the custom partial-commit code could be fixed.  I\nguess the root of the issue is that in the call to list_paths(), we call\noverlay_tree_on_index(). And that's how we end up thinking that \"d\" is a\nuseful path to talk about, even though it has already been removed from\nthe index (we have \"d/b.txt\" instead).\n\nSo I'm not sure if we need a solution here-ish:\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 65196a2827..25e65e986d 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -302,7 +302,9 @@ static void add_remove_files(struct string_list *list)\n \t\t\tcontinue;\n \n \t\tif (!lstat(p->string, &st)) {\n-\t\t\tif (add_to_index(&the_index, p->string, &st, 0))\n+\t\t\tif (S_ISDIR(st.st_mode))\n+\t\t\t\twarning(\"woah, %s is a dir\", p->string);\n+\t\t\telse if (add_to_index(&the_index, p->string, &st, 0))\n \t\t\t\tdie(_(\"updating files failed\"));\n \t\t} else\n \t\t\tremove_file_from_index(&the_index, p->string);\n\nbut I'm not quite sure what we should be doing instead of that\nwarning(). Should we updating and including everything in \"d/\"? What\nabout if there were a \"d/c.txt\" that was not a tracked file at all?\n\nIt might also be that the bug is earlier in list_paths(), where we\nshould notice that \"d\" is gone and now we have entries under \"d/\".\n\nI dunno. I probably won't dig much further on this myself, but it's\npossible Junio (cc'd) might have an idea right away.\n\n-Peff\n"},{"id":"487121","messageId":"xmqq4jf8j010.fsf@gitster.g","threadId":"60767","inReplyTo":"20240120004628.GA117170@coredump.intra.peff.net","subject":"Re: partial commit of file-to-directory change, was Re: Bugreport","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-20T00:55:55Z","receivedAt":"2024-01-20T00:56:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I dunno. I probably won't dig much further on this myself, but it's\n> possible Junio (cc'd) might have an idea right away.\n\nSorry, but I do not have any idea \"right away\".  I even needed to\nsee the tree of 2888605c64 and check if we had submodules back then\n(it turns out that we did).\n"},{"id":"487126","messageId":"xmqqmst0hk8k.fsf@gitster.g","threadId":"60767","inReplyTo":"20240120004628.GA117170@coredump.intra.peff.net","subject":"Re: partial commit of file-to-directory change, was Re: Bugreport","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-20T01:22:19Z","receivedAt":"2024-01-20T01:22:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> It might also be that the bug is earlier in list_paths(), where we\n> should notice that \"d\" is gone and now we have entries under \"d/\".\n\nWe had a related discussion on \"commit -i/-o tests\" review thread,\nwhere we noticed that \"git commit -i foobar\", when \"foobar\" does not\nmatch any path in the index, silently ignore that unmatching\npathspec (but \"commit -o foobar\" does error out, saying \"did not\nmatch any file(s) known to git\").  The important design decision we\nmade long time ago when we introduced partial commit (\"commit -o\npathspec\") is that we do not _add_ paths (that match the pathspec)\nthat are not tracked.  We require explicit \"git add\" for them before\nyou run \"git commit\".\n\nSo, I suspect that list_paths() that gives \"d\" as a thing to add is\nbroken.  \"commit -m + .\" at that point is saying \"we should get the\nlatest contents from the working tree for all the paths in the real\nindex, add[*] them to the temporary index freshly read from HEAD,\nand the resulting temporary index should be written out as a tree\"\nto create a commit, and then \"the same set of contents for the paths\nthen should be added to the real index, so that the differences\nbetween the tree we wrote out of the temporary index to update the\nHEAD and the real index would still be the changes already in the\nindex before this partial commit\".\n\n    Side note: here, \"add\" would include removing (if the working\n    tree file for the path is gone) or killing 'd' while adding\n    'd/b.txt'.\n\nSo I would understand if 'd/b.txt' is listed (because in the real\nindex there already is d/b.txt known), but if the directory 'd'\nitself is listed, that does sound like a bug.\n"},{"id":"487376","messageId":"xmqqjznxw8ev.fsf@gitster.g","threadId":"60767","inReplyTo":"20240120004628.GA117170@coredump.intra.peff.net","subject":"Re: partial commit of file-to-directory change, was Re: Bugreport","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-25T18:54:32Z","receivedAt":"2024-01-25T18:54:35Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Jan 19, 2024 at 11:14:47PM +0000, brian m. carlson wrote:\n>\n>> > $ git commit . -m +\n>> > error: 'd' does not have a commit checked out\n>> > fatal: updating files failed\n>> \n>> I can confirm this behaviour[0], and it's definitely wrong that it\n>> thinks `d` is a submodule.  It does, however, work if you do `git commit\n>> -m +` (that is, without the .), which makes sense since the relevant\n>> change is already staged in the index.\n>> \n>> I'm not going to get to a patch or more thorough investigation, but\n>> perhaps someone else will.\n>\n> I peeked at this a bit earlier; I didn't come up with a solution, but\n> here are a few more details.\n\nI didn't either X-<, and the thing is somewhat nasty, as there are two\nstates (HEAD and the index) involved.\n\n * For paths that do not match the pathspec, we want to take the\n   version from the HEAD.  For paths that do match the pathspec, we\n   want to take the version from the working tree.  And after making\n   the partial commit with such changes, we want to make the same\n   change to the index to prepare for the next commit.\n\n * With the way the code is currently structured, we find paths that\n   appear either in the index or in the HEAD to match against the\n   pathspec.  This is so that we can honor an earlier \"git rm\" since\n   we read HEAD.\n\n * Then we check each of these paths that are either in the index or\n   in HEAD that matched the pathspec.  If it is missing from the\n   working tree, we would remove it from both the temporary ndex\n   used for the partial commit, and the real index that we'll\n   continue to use after the partial commit.  If it exists in the\n   working tree, we would take the contents of it to update.\n\n   For the purpose of this operation, a path D that was a blob in\n   the index should be _removed_ when it is a directory in the\n   working tree (i.e. made room to create D/F).  And we should not\n   add D/F when running \"git commit D\" or \"git commit D/F\", because\n   the partial commit does not \"git add\" (it only does \"git add -u\")\n\nIn the example in the discussion, we had D that was a blob and got\nreplaced with a directory.  If we did \"git add -u D\", we would just\nhave removed D from the index, so the partial commit should do the\nsame.  Which means we need to know the type of the thing we expect.\nBut list_paths() only returns a list of path names, and does not\ngive us the type of the thing we expect.  We use the same list\ntwice, once to update the temporary index (which we create by\nreading HEAD) and update the paths listed there from the working\ntree.  And the same list is used again to update the real index\n(which is what the user had before starting the command) and update\nthe paths listed there from the working tree.  The tricky part is\nthat the type of (or even existence of) D may have changed between\nthe HEAD and the index, but we want to perform the same operation to\nthe real index with respect to the paths we touched to create the\npartial commit.\n\nNaively, add_remove_files() looks like an obvious place to see if\nthe path is in the working tree (which we already do) *and* compare\nit with \"the type of the thing we expect\", but the thing is that\nthis function is called twice with different index (once with the\ntemporary index that started from HEAD, i.e. oblivious to what the\nuser did to the index since HEAD; then again with the real index\nwith the changes the user made since HEAD), so we cannot just say\n\"let's see what this path D is in the index\".\n\nI _think_ we would need to update list_paths() so that it records a\nbit more than just pathnames that match the pathspec, namely, what\nshould be done to the path, by doing lstat() on the working tree\npaths.\n\n"}]}