{"thread":{"id":"59816","subject":"Re: When someone creates a new worktree by copying an existing one","startedAt":"2023-06-01T08:35:17Z","lastAt":"2023-06-02T19:19:19Z","messageCount":3,"participants":["Tao Klerks","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"477860","messageId":"CAPMMpoi9vy1xyvpAHE6qCwgazLSg=u4rR9eX098Gf9VL60XSOg@mail.gmail.com","threadId":"59816","inReplyTo":"CAPMMpohHo2co7n_NjD9XntBs3DVU91Rqx8dmRrSWg=1eof+Rhw@mail.gmail.com","subject":"Re: When someone creates a new worktree by copying an existing one","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2023-06-01T08:32:23Z","receivedAt":"2023-06-01T08:35:17Z","isPatch":false,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Wed, May 31, 2023 at 8:59 PM Tao Klerks <tao@klerks.biz> wrote:\n>\n> I think I can implement something reasonably effective with a pair of\n> hooks (pre-commit and post-checkout look like good candidates), but\n> I'm weirded out that something like this should need to be \"custom\".\n>\n\nFWIW, here's what that code has ended up looking like, in\npost-checkout and pre-commit:\n\n########################################################################\n# WORKTREE GIT FILE PATH TO REPO WORKTREE GITDIR FILE PATH CONSISTENCY #\n########################################################################\n# (this code is duplicated across pre-commit and post-checkout)\n\nWORKTREE_GIT_FILE_PATH=\"$PWD/.git\"\nGITDIR_PREFIX=\"gitdir: \"\n# Only run worktree consistency checks if it's a worktree - if .git is a file\nif [[ -f \"$WORKTREE_GIT_FILE_PATH\" ]]; then\n  WORKTREE_GIT_FILE_CONTENT=$(head -n 1 \"$WORKTREE_GIT_FILE_PATH\")\n  WORKTREE_METADATA_FOLDER_PATH=${WORKTREE_GIT_FILE_CONTENT#\"$GITDIR_PREFIX\"}\n  WORKTREE_GITDIR_FILE_PATH=\"$WORKTREE_METADATA_FOLDER_PATH/gitdir\"\n  # if the gitdir file doesn't exist, git will complain loudly, no\nneed to interfere.\n  if [[ -f \"$WORKTREE_GITDIR_FILE_PATH\" ]]; then\n    WORKTREE_GITDIR_PATH=$(head -n 1 \"$WORKTREE_GITDIR_FILE_PATH\")\n    # if the gitdir value doesn't match this worktree's git file path,\nwe have a problem.\n    if ! [[ \"$WORKTREE_GIT_FILE_PATH\" -ef \"$WORKTREE_GITDIR_PATH\" ]]; then\n      >&2 echo \"\"\n      >&2 echo \"******************************************************************************\"\n      >&2 echo \"* WARNING: It looks like this worktree has moved or\nbeen copied incorrectly! *\"\n      >&2 echo \"******************************************************************************\"\n      >&2 echo \"\"\n      >&2 echo \"The git repo expects this worktree to be at:\n$(realpath \"$(dirname \"$WORKTREE_GITDIR_PATH\")\")\"\n      >&2 echo \"\"\n      >&2 echo \"However, it is at: $(realpath \"$(dirname\n\"$WORKTREE_GIT_FILE_PATH\")\")\"\n      >&2 echo \"\"\n      >&2 echo \"If you copied an existing worktree to create this one,\nthen the two worktrees\"\n      >&2 echo \"will be badly 'linked', with changes in one affecting\nthe other. To resolve this,\"\n      >&2 echo \"please delete one of the two, and run 'git worktree\nrepair' in the remaining one.\"\n      >&2 echo \"You can then properly create a new worktree using 'git\nworktree add'. You\"\n      >&2 echo \"should delete whichever worktree doesn't contain\nuncommitted changes you\"\n      >&2 echo \"want to keep, of course.\"\n      >&2 echo \"\"\n      >&2 echo \"If you simply moved/renamed this worktree, then run\n'git worktree repair' to\"\n      >&2 echo \"make the git repo and worktree 'match up' again, and\nresolve this warning.\"\n      >&2 echo \"\"\n      >&2 echo \"******************************************************************************\"\n      >&2 echo \"\"\n      # in post-checkout this exit code won't change git's behavior,\nbut git still \"echoes\" it to\n      # the calling program, so it's still a useful signal that\nsomething went wrong - which is\n      # warranted under the circumstances.\n      exit 1\n    fi\n  fi\nfi\n"},{"id":"477898","messageId":"CAPig+cQ6tuh1nGG+t6c2O8VL-=Ggy+hWzJHTFWCb7xRH2HZkXg@mail.gmail.com","threadId":"59816","inReplyTo":"CAPMMpohHo2co7n_NjD9XntBs3DVU91Rqx8dmRrSWg=1eof+Rhw@mail.gmail.com","subject":"Re: When someone creates a new worktree by copying an existing one","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-06-01T19:48:35Z","receivedAt":"2023-06-01T19:48:55Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, May 31, 2023 at 3:00 PM Tao Klerks <tao@klerks.biz> wrote:\n> I've recently encountered some users who've got themselves into\n> trouble by copying a worktree into another folder, to create a second\n> worktree, instead of using \"git worktree add\".\n>\n> They assumed this would be fine, just the same as creating a new\n> *repository* by copying an new *repository* is generally fine (except\n> when you introduce worktrees, of course).\n>\n> There users eventually understood that something was wrong, as you\n> might expect, when a checkout in one worktree also \"checked out\" (and\n> produced lots of unexpected changes) in the other worktree - or\n> similar weirdnesses, eg on commit.\n>\n> Ideally, as I user, I would expect git to start shouting at me to \"git\n> worktree repair\" the moment the mutual reference between the \".git\"\n> file of the worktree, and the \"gitdir\" file of the repo's worktree\n> metadata, stopped agreeing.\n>\n> Is there any reason we can't/don't have such a safety check? (would it\n> be expensive??)\n>\n> I think I can implement something reasonably effective with a pair of\n> hooks (pre-commit and post-checkout look like good candidates), but\n> I'm weirded out that something like this should need to be \"custom\".\n>\n> I did a quick search of the archives but didn't find anything\n> relevant. Has this been discussed? Should I try to prepare a patch\n> including that sort of validation... somewhere?\n\nJust some thoughts, not a proper answer...\n\nIf you want to detect this sort of problem early, then patching one of\nthe initialization functions which is called by each Git command which\nrequires a working tree (i.e. those with NEED_WORK_TREE set or which\ncall setup_work_tree() manually) would be one approach. It's not\nimmediately obvious which function should be patched since there is\ndeep and mysterious interaction between the various startup functions,\nbut probably something in setup.c or environment.c.\n\nAn alternative might be to perform the detection only when the \"index\"\nis about to be updated or some other worktree-local administrative\nentry, but that seems like a much less tractable approach.\n\nAside from the corruption you describe above, there may be other types\nof corruption worth detecting early, but any such detection may be\nprohibitively expensive, even if it's just the type you mentioned. (By\n\"prohibitively\", I'm referring to previous work to reduce the startup\ntime of Git commands; people may not want to see those gains reversed,\nespecially for a case such as this which is likely to be rare.)\n\nUnless I'm mistaken, the described corruption can only be detected by\na Git command running within the duplicate directory itself since\nthere will be no entry in .git/worktrees/ corresponding to the\nmanually-created copy. Hence, Git commands run in any other worktree\nwill be unable to detect it, not even the \"git worktree\" command.\n\nThe validation performed by worktree.c:validate_worktree() is already\ncapable of detecting the sort of corruption you describe, if handed a\n`struct worktree` populated to point at the worktree which is a copy\nof the original worktree. However, there is no API presently to create\na `struct worktree` from an arbitrary path, so that is something you'd\nhave to add if you want to take advantage of\nworktree.c:validate_worktree(). Adding such a function does feel\nsomewhat ugly and special-purpose, though.\n\nI was going to mention that suggesting to the user merely to use \"git\nworktree repair\" would not help in this situation, but I see in your\nfollowup email that you instruct the user to first delete one of the\nworktrees before running \"git worktree repair\", which would indeed\nwork. However, you probably want the instructions to be clearer,\nsaying that the user should delete the worktree directory manually\n(say, with \"rm -rf\"), _not_ with \"git worktree remove\".\n\nIt wouldn't help with early-detection, but it might be worthwhile to\nadd a \"git worktree validate\" command which detects and reports\nproblems with worktree administrative data (i.e. call\nworktree.c:validate_worktree() for each worktree). However, the above\ncaveat still applies about inability to detect the problem from other\nworktrees.\n\nUpgrading \"git worktree repair\" to report the problem might be\nworthwhile, but it also is subject to the same caveat about inability\nto detect the problem from other worktrees.\n\nOne might suggest that \"git worktree repair\" should somehow repair the\nsituation itself, perhaps by creating a new entry in .git/worktrees/,\nbut it's not at all clear if that is a good idea. Any such automated\nsolution would need to be very careful not to throw away the user's\nwork (for instance, work which is staged in the \"index\").\n\nIt may be possible to add some option or subcommand to \"git worktree\"\nwhich would allow a user to \"attach\" an existing directory as a\nworktree. For instance:\n\n    % cp -R worktreeA worktreeB\n    % git worktree attach worktreeB\n\nwould create the necessary administrative entries in\n.git/worktrees/worktreeB/ and make worktreeB/.git point at\n.git/worktrees/worktreeB. However, I'm extremely reluctant to suggest\ndoing this since we don't want to encourage users to perform worktree\nmanipulation manually; they should be using \"git worktree\" for all\nsuch tasks. (We also don't want users hijacking a worktree which\nbelongs to some other repository.)\n"},{"id":"477980","messageId":"CAPMMpoikxfpcoN1T7A6FD76MSAvBqjJNsc55-mJc2ubM7gUAYw@mail.gmail.com","threadId":"59816","inReplyTo":"CAPig+cQ6tuh1nGG+t6c2O8VL-=Ggy+hWzJHTFWCb7xRH2HZkXg@mail.gmail.com","subject":"Re: When someone creates a new worktree by copying an existing one","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2023-06-02T19:19:02Z","receivedAt":"2023-06-02T19:19:19Z","isPatch":false,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Thu, Jun 1, 2023 at 9:48 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>\n> On Wed, May 31, 2023 at 3:00 PM Tao Klerks <tao@klerks.biz> wrote:\n> >\n>\n>\n> Aside from the corruption you describe above, there may be other types\n> of corruption worth detecting early, but any such detection may be\n> prohibitively expensive, even if it's just the type you mentioned. (By\n> \"prohibitively\", I'm referring to previous work to reduce the startup\n> time of Git commands; people may not want to see those gains reversed,\n> especially for a case such as this which is likely to be rare.)\n\nWill keep that in mind, thx.\n\n>\n> Unless I'm mistaken, the described corruption can only be detected by\n> a Git command running within the duplicate directory itself since\n> there will be no entry in .git/worktrees/ corresponding to the\n> manually-created copy. Hence, Git commands run in any other worktree\n> will be unable to detect it, not even the \"git worktree\" command.\n>\n\nThat matches my understanding.\n\n> I was going to mention that suggesting to the user merely to use \"git\n> worktree repair\" would not help in this situation, but I see in your\n> followup email that you instruct the user to first delete one of the\n> worktrees before running \"git worktree repair\", which would indeed\n> work. However, you probably want the instructions to be clearer,\n> saying that the user should delete the worktree directory manually\n> (say, with \"rm -rf\"), _not_ with \"git worktree remove\".\n>\n\nMakes sense, thanks for the tip!\n\n> It wouldn't help with early-detection, but it might be worthwhile to\n> add a \"git worktree validate\" command which detects and reports\n> problems with worktree administrative data (i.e. call\n> worktree.c:validate_worktree() for each worktree). However, the above\n> caveat still applies about inability to detect the problem from other\n> worktrees.\n>\n\nYep, that might be interesting, but wouldn't obviously address the\ncase I'm most interested in.\n\nMy objective is to tell the user \"you've done something wrong, and are\nnow in a problem state\" as soon as they start operating in their\nnewly-copied worktree.\n\n> Upgrading \"git worktree repair\" to report the problem might be\n> worthwhile, but it also is subject to the same caveat about inability\n> to detect the problem from other worktrees.\n>\n\nYeah, I would have appreciated a \"git worktree repair -n\" option when\nI was writing the hook scripts, to avoid having to \"bash\" my way\naround the .git and gitdir references.\n\nTo your point, my hook script could do a much better job than it\ncurrently does: instead of saying \"there's *something* wrong\", it\ncould actually check whether the \"wrong\" original worktree gitdir path\nstored in the repo actually corresponds to a real path,. and thereby\ndifferentiate between \"run git worktree repair to fix things\" and\n\"delete this unholy worktree you have created with rm -rf\".\n\n> One might suggest that \"git worktree repair\" should somehow repair the\n> situation itself, perhaps by creating a new entry in .git/worktrees/,\n> but it's not at all clear if that is a good idea. Any such automated\n> solution would need to be very careful not to throw away the user's\n> work (for instance, work which is staged in the \"index\").\n>\n\nYeah, I agree that could end up weird. Imagine:\n* The user creates a copy (B) of the original worktree (A)\n* Initially, the index, HEAD etc is all correct for both worktrees -\nthey are identical\n* The user keeps working in the original worktree, A, staging stuff,\ncommitting stuff, switching branches - git has no way of warning of\nanything wrong. There *is* nothing wrong with the git repo.\n* The user changes directory to the \"bad\" worktree, \"B\".\n* Now git can warn that there is something wrong, and offer \"repair by\ncreating a new worktree from the shared state?\"... but the resulting\nworktree would still be a complete mess, with a large set of\nunexplained unexplainable unstaged changes, some of which might have\noriginally been real unstaged changes at the time of the copy.\n\nAs far as I can tell, the only \"right\" thing is to tell the user to\ndelete the duplicate-and-mistargeted worktree. As long as the warning\nfrom git comes early enough, the user is unlikely to have managed to\ndo any work in that folder before they find out it's unusable.\n\n> It may be possible to add some option or subcommand to \"git worktree\"\n> which would allow a user to \"attach\" an existing directory as a\n> worktree. For instance:\n>\n>     % cp -R worktreeA worktreeB\n>     % git worktree attach worktreeB\n>\n> would create the necessary administrative entries in\n> .git/worktrees/worktreeB/ and make worktreeB/.git point at\n> .git/worktrees/worktreeB. However, I'm extremely reluctant to suggest\n> doing this since we don't want to encourage users to perform worktree\n> manipulation manually; they should be using \"git worktree\" for all\n> such tasks. (We also don't want users hijacking a worktree which\n> belongs to some other repository.)\n\nAgreed - my main concern with even attempting to offer this is that\nthe user who does this by accident is ill-equipped to let git know\nwhat the correct HEAD for that directory is!\n\nThanks again for the feedback.\n\nI'll add this to my list of \"things to see if one day I could help\nwith\", but likely won't get to it.\n"}]}