{"thread":{"id":"56412","subject":"[PATCH] rebase, cherry-pick, revert: only run from toplevel","startedAt":"2021-08-31T03:03:57Z","lastAt":"2021-09-01T06:48:37Z","messageCount":12,"participants":["Elijah Newren via GitGitGadget","Elijah Newren","Johannes Sixt","Jeff King","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"434226","messageId":"pull.1083.git.git.1630379030665.gitgitgadget@gmail.com","threadId":"56412","inReplyTo":null,"subject":"[PATCH] rebase, cherry-pick, revert: only run from toplevel","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-31T03:03:50Z","receivedAt":"2021-08-31T03:03:57Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nAllowing rebase, cherry-pick and revert to run from subdirectories\ninevitably leads to eventual user confusion. For example, if they\nare within a directory that was created by one of the patches being\nrebased, then the rebase operation could hit a conflict before the\ndirectory is restored leading the user to be running from a directory\nthat no longer exists. Similarly with cherry-pick and revert, those\noperations could result in the directory being removed.\n\nSimilar to bisect, simply require that these commands be run from the\ntoplevel to avoid such problems.\n\nSigned-off-by: Elijah Newren \n---\n rebase, cherry-pick, revert: only run from toplevel\n \n Allowing rebase, cherry-pick and revert to run from subdirectories\n inevitably leads to eventual user confusion. For example, if they are\n within a directory that was created by one of the patches being rebased,\n then the rebase operation could hit a conflict before the directory is\n restored leading the user to be running from a directory that no longer\n exists. Similarly with cherry-pick and revert, those operations could\n result in the directory being removed.\n \n Similar to bisect, simply require that these commands be run from the\n toplevel to avoid such problems.\n \n Signed-off-by: Elijah Newren newren@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1083%2Fnewren%2Ftoplevel-sequencing-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1083/newren/toplevel-sequencing-v1\nPull-Request: https://github.com/git/git/pull/1083\n\n builtin/rebase.c | 3 +++\n builtin/revert.c | 6 ++++++\n t/t3404-rebase-interactive.sh | 22 ----------------------\n 3 files changed, 9 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex c284a7ace19..7100f0627f6 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1430,6 +1430,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(builtin_rebase_usage,\n \t\t\t\t builtin_rebase_options);\n \n+\tif (prefix)\n+\t\tdie(_(\"You need to run this command from the toplevel of the working tree.\"));\n+\n \toptions.allow_empty_message = 1;\n \tgit_config(rebase_config, &options);\n \t/* options.gpg_sign_opt will be either \"-S\" or NULL */\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 237f2f18d4c..9a150dcbdaf 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -230,6 +230,9 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n \tstruct replay_opts opts = REPLAY_OPTS_INIT;\n \tint res;\n \n+\tif (prefix)\n+\t\tdie(_(\"You need to run this command from the toplevel of the working tree.\"));\n+\n \topts.action = REPLAY_REVERT;\n \tsequencer_init_config(&opts);\n \tres = run_sequencer(argc, argv, &opts);\n@@ -243,6 +246,9 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n \tstruct replay_opts opts = REPLAY_OPTS_INIT;\n \tint res;\n \n+\tif (prefix)\n+\t\tdie(_(\"You need to run this command from the toplevel of the working tree.\"));\n+\n \topts.action = REPLAY_PICK;\n \tsequencer_init_config(&opts);\n \tres = run_sequencer(argc, argv, &opts);\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 66bcbbf9528..dd1afb97fca 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -112,28 +112,6 @@ test_expect_success 'rebase -i with the exec command' '\n \trm -f touch-*\n '\n \n-test_expect_success 'rebase -i with the exec command runs from tree root' '\n-\tgit checkout primary &&\n-\tmkdir subdir && (cd subdir &&\n-\tset_fake_editor &&\n-\tFAKE_LINES=\"1 exec_>touch-subdir\" \\\n-\t\tgit rebase -i HEAD^\n-\t) &&\n-\ttest_path_is_file touch-subdir &&\n-\trm -fr subdir\n-'\n-\n-test_expect_success 'rebase -i with exec allows git commands in subdirs' '\n-\ttest_when_finished \"rm -rf subdir\" &&\n-\ttest_when_finished \"git rebase --abort ||:\" &&\n-\tgit checkout primary &&\n-\tmkdir subdir && (cd subdir &&\n-\tset_fake_editor &&\n-\tFAKE_LINES=\"1 x_cd_subdir_&&_git_rev-parse_--is-inside-work-tree\" \\\n-\t\tgit rebase -i HEAD^\n-\t)\n-'\n-\n test_expect_success 'rebase -i sets work tree properly' '\n \ttest_when_finished \"rm -rf subdir\" &&\n \ttest_when_finished \"test_might_fail git rebase --abort\" &&\n\nbase-commit: 6c40894d2466d4e7fddc047a05116aa9d14712ee\n-- \ngitgitgadget\n"},{"id":"434227","messageId":"CABPp-BEyach6xTxHduUzBLmh9bKG1aFZkBev4dOwWrw55VvNmQ@mail.gmail.com","threadId":"56412","inReplyTo":"pull.1083.git.git.1630379030665.gitgitgadget@gmail.com","subject":"Re: [PATCH] rebase, cherry-pick, revert: only run from toplevel","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-08-31T03:05:29Z","receivedAt":"2021-08-31T03:05:46Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Aug 30, 2021 at 8:03 PM Elijah Newren via GitGitGadget\n wrote:\n>\n> From: Elijah Newren \n>\n> Allowing rebase, cherry-pick and revert to run from subdirectories\n> inevitably leads to eventual user confusion. For example, if they\n> are within a directory that was created by one of the patches being\n> rebased, then the rebase operation could hit a conflict before the\n> directory is restored leading the user to be running from a directory\n> that no longer exists. Similarly with cherry-pick and revert, those\n> operations could result in the directory being removed.\n>\n> Similar to bisect, simply require that these commands be run from the\n> toplevel to avoid such problems.\n\nSee also and the rest of the thread.\n"},{"id":"434233","messageId":"38fef2e3-a78a-c83b-85f8-dae064931703@kdbg.org","threadId":"56412","inReplyTo":"pull.1083.git.git.1630379030665.gitgitgadget@gmail.com","subject":"Re: [PATCH] rebase, cherry-pick, revert: only run from toplevel","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2021-08-31T05:55:50Z","receivedAt":"2021-08-31T05:55:54Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 31.08.21 um 05:03 schrieb Elijah Newren via GitGitGadget:\n> From: Elijah Newren \n> \n> Allowing rebase, cherry-pick and revert to run from subdirectories\n> inevitably leads to eventual user confusion. For example, if they\n> are within a directory that was created by one of the patches being\n> rebased, then the rebase operation could hit a conflict before the\n> directory is restored leading the user to be running from a directory\n> that no longer exists. Similarly with cherry-pick and revert, those\n> operations could result in the directory being removed.\n> \n> Similar to bisect, simply require that these commands be run from the\n> toplevel to avoid such problems.\n\nI am not a friend of this change. I understand the motivation behind it.\nBut most of the time, rebase and cherry-pick are operated on own code,\nwhere directories do not disappear and appear at random, and this new\nenforced condition becomes awkward.\n\nOne of my use-cases is that I operate git-rebase from an untracked build\ndirectory inside the repository. Having to pass -C .. all the time\nstrikes the wrong balance, IMO.\n\nThings are slightly different for git-bisect (at least for me), because\noftentimes it is operated on foreign code, where you may not know which\ndirectories come and go. But even that is a weak argument to force the\ncommand to the top-level of the repository.\n\nPerhaps it is sufficient to force git-pull to the top-level (if it isn't\nalready).\n\n-- Hannes\n"},{"id":"434243","messageId":"YS3Tv7UfNkF+adry@coredump.intra.peff.net","threadId":"56412","inReplyTo":"pull.1083.git.git.1630379030665.gitgitgadget@gmail.com","subject":"Re: [PATCH] rebase, cherry-pick, revert: only run from toplevel","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-31T07:01:19Z","receivedAt":"2021-08-31T07:01:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 31, 2021 at 03:03:50AM +0000, Elijah Newren via GitGitGadget wrote:\n\n> From: Elijah Newren \n> \n> Allowing rebase, cherry-pick and revert to run from subdirectories\n> inevitably leads to eventual user confusion. For example, if they\n> are within a directory that was created by one of the patches being\n> rebased, then the rebase operation could hit a conflict before the\n> directory is restored leading the user to be running from a directory\n> that no longer exists. Similarly with cherry-pick and revert, those\n> operations could result in the directory being removed.\n> \n> Similar to bisect, simply require that these commands be run from the\n> toplevel to avoid such problems.\n\nIMHO this is too draconian. You are occasionally helping people who are\nin a directory which goes away over the course of the operation. But you\nare hurting everyone who _isn't_ in that situation, and who needlessly\nhas to re-issue their command after doing a \"cd\".\n\nI think we'd be much better served to do even a rudimentary analysis of\nwhether the operation will be a problem. E.g., if we taught the checkout\ncode to error out when the cwd is going to disappear, then:\n\n - we'd protect the user from confusion during regular sight-seeing via\n \"git checkout v0.99\" and so forth\n\n - we'd protect the most common cases for git-rebase (your patches\n introduce \"subdir/\", but it is not yet in the parent directory). We\n wouldn't preemptively avoid a rebase where subdir/ disappears and\n then reappears in the middle of the series. We could find such a\n case by iterating over the patches, but IMHO it's not worth the\n computation.\n\n - we could likewise protect git-bisect, making it more reasonable to\n loosen its current restriction\n\n - we might want to teach similar logic to sequencer operations, so\n that applying a patch would likewise error-out. That would protect\n cherry-pick and revert, but also make the \"subdir/ disappears\n mid-patch-series\" case pretty nice: the specific patch that deletes\n it would fail to apply, and then you could \"cd .. && git rebase\n --continue\".\n\n I suspect that the \"oops, we're going to delete cwd\" code would end\n up in unpack-trees anyway, which means that both checkout and all of\n this sequencer operations would use the same code.\n\nNow I have spent zero time looking into actually coding this, so it may\nturn out to be much trickier than I am suggesting. But this seems like a\nmuch more fruitful direction, where we can protect users in cases where\nthey benefit (and give them sensible and actionable error messages),\nwithout bothering people in the majority of cases where their cwd\ndoesn't go away.\n\n-Peff\n"},{"id":"434331","messageId":"CABPp-BFmU+RaAjq4_0-PSfRgH1Jc63nN0fMuDWk2+iDbdz7CCA@mail.gmail.com","threadId":"56412","inReplyTo":"YS3Tv7UfNkF+adry@coredump.intra.peff.net","subject":"Re: [PATCH] rebase, cherry-pick, revert: only run from toplevel","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-08-31T20:14:55Z","receivedAt":"2021-08-31T20:15:09Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Aug 31, 2021 at 12:01 AM Jeff King wrote:\n>\n> On Tue, Aug 31, 2021 at 03:03:50AM +0000, Elijah Newren via GitGitGadget wrote:\n>\n> > From: Elijah Newren \n> >\n> > Allowing rebase, cherry-pick and revert to run from subdirectories\n> > inevitably leads to eventual user confusion. For example, if they\n> > are within a directory that was created by one of the patches being\n> > rebased, then the rebase operation could hit a conflict before the\n> > directory is restored leading the user to be running from a directory\n> > that no longer exists. Similarly with cherry-pick and revert, those\n> > operations could result in the directory being removed.\n> >\n> > Similar to bisect, simply require that these commands be run from the\n> > toplevel to avoid such problems.\n>\n> IMHO this is too draconian. You are occasionally helping people who are\n> in a directory which goes away over the course of the operation. But you\n> are hurting everyone who _isn't_ in that situation, and who needlessly\n> has to re-issue their command after doing a \"cd\".\n>\n> I think we'd be much better served to do even a rudimentary analysis of\n> whether the operation will be a problem. E.g., if we taught the checkout\n> code to error out when the cwd is going to disappear, then:\n>\n> - we'd protect the user from confusion during regular sight-seeing via\n> \"git checkout v0.99\" and so forth\n>\n> - we'd protect the most common cases for git-rebase (your patches\n> introduce \"subdir/\", but it is not yet in the parent directory). We\n> wouldn't preemptively avoid a rebase where subdir/ disappears and\n> then reappears in the middle of the series. We could find such a\n> case by iterating over the patches, but IMHO it's not worth the\n> computation.\n>\n> - we could likewise protect git-bisect, making it more reasonable to\n> loosen its current restriction\n>\n> - we might want to teach similar logic to sequencer operations, so\n> that applying a patch would likewise error-out. That would protect\n> cherry-pick and revert, but also make the \"subdir/ disappears\n> mid-patch-series\" case pretty nice: the specific patch that deletes\n> it would fail to apply, and then you could \"cd .. && git rebase\n> --continue\".\n>\n> I suspect that the \"oops, we're going to delete cwd\" code would end\n> up in unpack-trees anyway, which means that both checkout and all of\n> this sequencer operations would use the same code.\n\nWell, sequencer uses the merge machinery, and merge-ort creates a\ntoplevel merge tree that includes conflict markers in files and\nwhatnot, and then only updates the working tree via checkout(). So,\nyeah, it really should be the same codepath.\n\n> Now I have spent zero time looking into actually coding this, so it may\n> turn out to be much trickier than I am suggesting. But this seems like a\n> much more fruitful direction, where we can protect users in cases where\n> they benefit (and give them sensible and actionable error messages),\n> without bothering people in the majority of cases where their cwd\n> doesn't go away.\n\nOoh, this sounds intriguing to me...but what if we changed that rule\nslightly and just decided to never make the cwd go away? Currently,\nthe checkout code removes directories if they have no tracked or\nuntracked or ignored files left, i.e. if they're empty. What if we\ndecide to only have remove_scheduled_dirs() remove directories that\nare empty AND they are not the current working directory?\n\nGranted, that could leave a directory in the way of files from other\nrevisions, but the same was already true in the analogous situation\nwhere there were leftover untracked files in some directory. Since\nall our code paths already have to handle directories leftover due to\nuntracked files, perhaps avoiding removing the current working\ndirectory would just automatically work for us? (I think it'd handle\nall of checkout, bisect, rebase, cherry-pick, revert, and merge.)\n"},{"id":"434381","messageId":"YS7rl8ynKD0fAerG@nand.local","threadId":"56412","inReplyTo":"CABPp-BFmU+RaAjq4_0-PSfRgH1Jc63nN0fMuDWk2+iDbdz7CCA@mail.gmail.com","subject":"Re: [PATCH] rebase, cherry-pick, revert: only run from toplevel","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-01T02:55:19Z","receivedAt":"2021-09-01T02:55:23Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Aug 31, 2021 at 01:14:55PM -0700, Elijah Newren wrote:\n> > Now I have spent zero time looking into actually coding this, so it may\n> > turn out to be much trickier than I am suggesting. But this seems like a\n> > much more fruitful direction, where we can protect users in cases where\n> > they benefit (and give them sensible and actionable error messages),\n> > without bothering people in the majority of cases where their cwd\n> > doesn't go away.\n>\n> Ooh, this sounds intriguing to me...but what if we changed that rule\n> slightly and just decided to never make the cwd go away? Currently,\n> the checkout code removes directories if they have no tracked or\n> untracked or ignored files left, i.e. if they're empty. What if we\n> decide to only have remove_scheduled_dirs() remove directories that\n> are empty AND they are not the current working directory?\n\nHmm. My first thought after reading this is that it would cause\nsurprising behavior for anybody who had 'git add --all' in their 'rebase\n-x' script. But would it?\n\nI.e., imagine somebody doing an in-place sed in a rebase and then `git\nadd --all`-ing the result at each point in history. If the directory\nthey were in ever went away, then the *next* revision would add that\ndirectory right back.\n\nThat behavior seems somewhat surprising to me, or at least I could\nimagine it being surprising to users.\n\nAnother thought is what should happen when the current directory goes\naway and then comes back as a file? We wouldn't be able to checkout that\nfile, I don't think, so it might be a dead end.\n\nThanks,\nTaylor\n"},{"id":"434384","messageId":"CABPp-BGkQTBBR_D-3EJPY-ONVYY4jjbEE4zA40n_oiz2DCrHgA@mail.gmail.com","threadId":"56412","inReplyTo":"YS7rl8ynKD0fAerG@nand.local","subject":"Re: [PATCH] rebase, cherry-pick, revert: only run from toplevel","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-09-01T04:43:15Z","receivedAt":"2021-09-01T04:43:30Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Aug 31, 2021 at 7:55 PM Taylor Blau wrote:\n>\n> On Tue, Aug 31, 2021 at 01:14:55PM -0700, Elijah Newren wrote:\n> > > Now I have spent zero time looking into actually coding this, so it may\n> > > turn out to be much trickier than I am suggesting. But this seems like a\n> > > much more fruitful direction, where we can protect users in cases where\n> > > they benefit (and give them sensible and actionable error messages),\n> > > without bothering people in the majority of cases where their cwd\n> > > doesn't go away.\n> >\n> > Ooh, this sounds intriguing to me...but what if we changed that rule\n> > slightly and just decided to never make the cwd go away? Currently,\n> > the checkout code removes directories if they have no tracked or\n> > untracked or ignored files left, i.e. if they're empty. What if we\n> > decide to only have remove_scheduled_dirs() remove directories that\n> > are empty AND they are not the current working directory?\n>\n> Hmm. My first thought after reading this is that it would cause\n> surprising behavior for anybody who had 'git add --all' in their 'rebase\n> -x' script. But would it?\n>\n> I.e., imagine somebody doing an in-place sed in a rebase and then `git\n> add --all`-ing the result at each point in history. If the directory\n> they were in ever went away, then the *next* revision would add that\n> directory right back.\n>\n> That behavior seems somewhat surprising to me, or at least I could\n> imagine it being surprising to users.\n\nI'm not following. `git add --all` doesn't add empty directories, so\nI don't see how my proposed change would cause any problems in such a\ncase; nothing would be added back.\n\n> Another thought is what should happen when the current directory goes\n> away and then comes back as a file? We wouldn't be able to checkout that\n> file, I don't think, so it might be a dead end.\n\nI'm not following this either. Peff's original suggestion was to\nerror out only when we knew it could cause problems, in particular\nwhen the working directory would be removed. Here I've shifted the\nway the problem is viewed by just not removing the working directory,\nbut the end result is the same -- it errors out when the removal was\nneeded. Given that erroring out is exactly what we wanted for a case\nlike this, why does that make it a dead end?\n"},{"id":"434387","messageId":"YS8IucBBDX61QdFB@nand.local","threadId":"56412","inReplyTo":"CABPp-BGkQTBBR_D-3EJPY-ONVYY4jjbEE4zA40n_oiz2DCrHgA@mail.gmail.com","subject":"Re: [PATCH] rebase, cherry-pick, revert: only run from toplevel","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-01T04:59:37Z","receivedAt":"2021-09-01T04:59:40Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Aug 31, 2021 at 09:43:15PM -0700, Elijah Newren wrote:\n> On Tue, Aug 31, 2021 at 7:55 PM Taylor Blau wrote:\n> >\n> > On Tue, Aug 31, 2021 at 01:14:55PM -0700, Elijah Newren wrote:\n> > > > Now I have spent zero time looking into actually coding this, so it may\n> > > > turn out to be much trickier than I am suggesting. But this seems like a\n> > > > much more fruitful direction, where we can protect users in cases where\n> > > > they benefit (and give them sensible and actionable error messages),\n> > > > without bothering people in the majority of cases where their cwd\n> > > > doesn't go away.\n> > >\n> > > Ooh, this sounds intriguing to me...but what if we changed that rule\n> > > slightly and just decided to never make the cwd go away? Currently,\n> > > the checkout code removes directories if they have no tracked or\n> > > untracked or ignored files left, i.e. if they're empty. What if we\n> > > decide to only have remove_scheduled_dirs() remove directories that\n> > > are empty AND they are not the current working directory?\n> >\n> > Hmm. My first thought after reading this is that it would cause\n> > surprising behavior for anybody who had 'git add --all' in their 'rebase\n> > -x' script. But would it?\n> >\n> > I.e., imagine somebody doing an in-place sed in a rebase and then `git\n> > add --all`-ing the result at each point in history. If the directory\n> > they were in ever went away, then the *next* revision would add that\n> > directory right back.\n> >\n> > That behavior seems somewhat surprising to me, or at least I could\n> > imagine it being surprising to users.\n>\n> I'm not following. `git add --all` doesn't add empty directories, so\n> I don't see how my proposed change would cause any problems in such a\n> case; nothing would be added back.\n\nAhh, it was I who wasn't following. You were proposing to leave the\ndirectories in place but empty. Agreed that there wouldn't be any\nproblems with that.\n\n> > Another thought is what should happen when the current directory goes\n> > away and then comes back as a file? We wouldn't be able to checkout that\n> > file, I don't think, so it might be a dead end.\n>\n> I'm not following this either. Peff's original suggestion was to\n> error out only when we knew it could cause problems, in particular\n> when the working directory would be removed. Here I've shifted the\n> way the problem is viewed by just not removing the working directory,\n> but the end result is the same -- it errors out when the removal was\n> needed. Given that erroring out is exactly what we wanted for a case\n> like this, why does that make it a dead end?\n\nThe way you shifted the problem makes it possible for us to discover\nthat only right before we're about to fail, right? In other words, if\nyou're doing a rebase then you're potentially leaving a lot of wasted\nwork on the table if you realize halfway through the operation that you\ncouldn't complete it.\n\nEven though I think that's *not* what you want for rebase, it ironically\n*is* what you might want for bisect, since the path we'll take isn't\nknown ahead of time. So even if there are some paths that would result\nin a directory -> file conversion in the cwd, we don't need to fail the\noperation ahead of time if the bisection doesn't actually take that\npath.\n\nOn the other hand, it does still leave a lot of work on the table if the\nbisection does eventually want to change the current working directory\ninto a file, depending on where that change happens.\n\nMaybe those cases are pretty niche in practice. I have to imagine that\nthey are. If my guess is right, then I think your approach makes sense.\n\nThanks,\nTaylor\n"},{"id":"434391","messageId":"xmqqk0k0sw2h.fsf@gitster.g","threadId":"56412","inReplyTo":"CABPp-BFmU+RaAjq4_0-PSfRgH1Jc63nN0fMuDWk2+iDbdz7CCA@mail.gmail.com","subject":"Re: [PATCH] rebase, cherry-pick, revert: only run from toplevel","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-01T05:29:10Z","receivedAt":"2021-09-01T05:29:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> Ooh, this sounds intriguing to me...but what if we changed that rule\n> slightly and just decided to never make the cwd go away? Currently,\n> the checkout code removes directories if they have no tracked or\n> untracked or ignored files left, i.e. if they're empty. What if we\n> decide to only have remove_scheduled_dirs() remove directories that\n> are empty AND they are not the current working directory?\n\nIs that generally doable? What would we do when the directory the\nsubcommand was started from (or one of its parent directories) is\nnot just missing but has to be a file in the revision the subcommand\nis trying to checkout?\n"},{"id":"434393","messageId":"CABPp-BFET9PhgAh+xvMWzTGD+pW3ujOUXO6JOF1rtFMMpDHZvg@mail.gmail.com","threadId":"56412","inReplyTo":"xmqqk0k0sw2h.fsf@gitster.g","subject":"Re: [PATCH] rebase, cherry-pick, revert: only run from toplevel","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-09-01T06:08:35Z","receivedAt":"2021-09-01T06:08:49Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Aug 31, 2021 at 10:29 PM Junio C Hamano wrote:\n>\n> Elijah Newren writes:\n>\n> > Ooh, this sounds intriguing to me...but what if we changed that rule\n> > slightly and just decided to never make the cwd go away? Currently,\n> > the checkout code removes directories if they have no tracked or\n> > untracked or ignored files left, i.e. if they're empty. What if we\n> > decide to only have remove_scheduled_dirs() remove directories that\n> > are empty AND they are not the current working directory?\n>\n> Is that generally doable? What would we do when the directory the\n> subcommand was started from (or one of its parent directories) is\n> not just missing but has to be a file in the revision the subcommand\n> is trying to checkout?\n\nThe same problem (an untracked directory is in the way) already exists\nand has to be handled by all relevant subcommands, right?\n\nIn particular, if the current working directory only has untracked\nfiles in it, then the directory cannot be removed. That will prevent\nus from checking out the revision we want, so we have to throw an\nerror.\n\nSo my idea just piggy backs on that, resulting in the same error also\nbeing shown when the current working directory has 0 untracked files\nwithin it.\n\nSince the whole thread started from, \"maybe we should throw an error\ninstead of continuing if it would result in the current working\ndirectory getting deleted\", I believe this idea does exactly what we\nwere looking for...and nicely tailors the new error cases to precisely\nthe situations we wanted them for -- when the current working\ndirectory would have been removed by the old code.\n"},{"id":"434395","messageId":"YS8eEtwQvF7TaLCb@coredump.intra.peff.net","threadId":"56412","inReplyTo":"CABPp-BFET9PhgAh+xvMWzTGD+pW3ujOUXO6JOF1rtFMMpDHZvg@mail.gmail.com","subject":"Re: [PATCH] rebase, cherry-pick, revert: only run from toplevel","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-01T06:30:42Z","receivedAt":"2021-09-01T06:30:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 31, 2021 at 11:08:35PM -0700, Elijah Newren wrote:\n\n> On Tue, Aug 31, 2021 at 10:29 PM Junio C Hamano wrote:\n> >\n> > Elijah Newren writes:\n> >\n> > > Ooh, this sounds intriguing to me...but what if we changed that rule\n> > > slightly and just decided to never make the cwd go away? Currently,\n> > > the checkout code removes directories if they have no tracked or\n> > > untracked or ignored files left, i.e. if they're empty. What if we\n> > > decide to only have remove_scheduled_dirs() remove directories that\n> > > are empty AND they are not the current working directory?\n> >\n> > Is that generally doable? What would we do when the directory the\n> > subcommand was started from (or one of its parent directories) is\n> > not just missing but has to be a file in the revision the subcommand\n> > is trying to checkout?\n> \n> The same problem (an untracked directory is in the way) already exists\n> and has to be handled by all relevant subcommands, right?\n> \n> In particular, if the current working directory only has untracked\n> files in it, then the directory cannot be removed. That will prevent\n> us from checking out the revision we want, so we have to throw an\n> error.\n> \n> So my idea just piggy backs on that, resulting in the same error also\n> being shown when the current working directory has 0 untracked files\n> within it.\n> \n> Since the whole thread started from, \"maybe we should throw an error\n> instead of continuing if it would result in the current working\n> directory getting deleted\", I believe this idea does exactly what we\n> were looking for...and nicely tailors the new error cases to precisely\n> the situations we wanted them for -- when the current working\n> directory would have been removed by the old code.\n\nYeah, this is definitely what I had in mind for my original suggestion:\nextending the \"do not delete directory if it contains an untracked file\"\nrule to the cwd. I guess I was thinking of it as an error, but it really\n_isn't_ an error in most cases, as long as we don't need to recreate\nanother entry there (and if we do, then as you say, the existing error\npaths should handle it).\n\nIt does leave crufty directories behind in the working tree that Git\nwould not then delete (because it only cleans up directories after it\nremoves their contents, and there are no contents left to remove).\nThat's probably not a big deal, though. If you are moving around history\nwith things like rebase or bisect, you'd likely move back to a working\ntree with the directory anyway. And it's not like you can't end up with\nsuch directories through other means (and \"git clean -d\" takes care of\nthem).\n\nSo I do like the approach.\n\n-Peff\n\nPS I'm assuming this is all taking the cwd of the process based on the\n original \"prefix\" (since Git processes generally move up to the\n top-level of the working tree). But that still leaves the problem for\n _other_ processes. E.g., if I am working in two terminals, one could\n be in \"subdir/\" and I could issue a bisect or rebase command from the\n other at the top-level, which would put one of them into the\n confusing state. You could strengthen the rule to \"is _any_ process\n using this as a cwd\", but it's hard to do so portably. And I know\n I've been annoyed by similar such safety valves before (\"why can't I\n delete this? Argh, some process I don't care about is squatting on\n it\").\n"},{"id":"434396","messageId":"CABPp-BEaCCSTrte+s8tr+3zvrgg6LWCi8Z3dTz7CvkXWyJgKFQ@mail.gmail.com","threadId":"56412","inReplyTo":"YS8IucBBDX61QdFB@nand.local","subject":"Re: [PATCH] rebase, cherry-pick, revert: only run from toplevel","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-09-01T06:48:23Z","receivedAt":"2021-09-01T06:48:37Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Aug 31, 2021 at 9:59 PM Taylor Blau wrote:\n>\n> On Tue, Aug 31, 2021 at 09:43:15PM -0700, Elijah Newren wrote:\n> > On Tue, Aug 31, 2021 at 7:55 PM Taylor Blau wrote:\n> > >\n> > > On Tue, Aug 31, 2021 at 01:14:55PM -0700, Elijah Newren wrote:\n> > > > > Now I have spent zero time looking into actually coding this, so it may\n> > > > > turn out to be much trickier than I am suggesting. But this seems like a\n> > > > > much more fruitful direction, where we can protect users in cases where\n> > > > > they benefit (and give them sensible and actionable error messages),\n> > > > > without bothering people in the majority of cases where their cwd\n> > > > > doesn't go away.\n> > > >\n> > > > Ooh, this sounds intriguing to me...but what if we changed that rule\n> > > > slightly and just decided to never make the cwd go away? Currently,\n> > > > the checkout code removes directories if they have no tracked or\n> > > > untracked or ignored files left, i.e. if they're empty. What if we\n> > > > decide to only have remove_scheduled_dirs() remove directories that\n> > > > are empty AND they are not the current working directory?\n> > >\n> > > Hmm. My first thought after reading this is that it would cause\n> > > surprising behavior for anybody who had 'git add --all' in their 'rebase\n> > > -x' script. But would it?\n> > >\n> > > I.e., imagine somebody doing an in-place sed in a rebase and then `git\n> > > add --all`-ing the result at each point in history. If the directory\n> > > they were in ever went away, then the *next* revision would add that\n> > > directory right back.\n> > >\n> > > That behavior seems somewhat surprising to me, or at least I could\n> > > imagine it being surprising to users.\n> >\n> > I'm not following. `git add --all` doesn't add empty directories, so\n> > I don't see how my proposed change would cause any problems in such a\n> > case; nothing would be added back.\n>\n> Ahh, it was I who wasn't following. You were proposing to leave the\n> directories in place but empty. Agreed that there wouldn't be any\n> problems with that.\n>\n> > > Another thought is what should happen when the current directory goes\n> > > away and then comes back as a file? We wouldn't be able to checkout that\n> > > file, I don't think, so it might be a dead end.\n> >\n> > I'm not following this either. Peff's original suggestion was to\n> > error out only when we knew it could cause problems, in particular\n> > when the working directory would be removed. Here I've shifted the\n> > way the problem is viewed by just not removing the working directory,\n> > but the end result is the same -- it errors out when the removal was\n> > needed. Given that erroring out is exactly what we wanted for a case\n> > like this, why does that make it a dead end?\n>\n> The way you shifted the problem makes it possible for us to discover\n> that only right before we're about to fail, right? In other words, if\n> you're doing a rebase then you're potentially leaving a lot of wasted\n> work on the table if you realize halfway through the operation that you\n> couldn't complete it.\n\nSure, it shifts things towards failing later. However, for\ncorrectness reasons, merge-ort has already fundamentally shifted\neverything working-directory related towards failing later.\n\nmerge-recursive (the old algorithm), via unpack-trees, tried to do\nchecks for \"locally modified files in the way\" and \"untracked\nfiles/directories in the way\" early on in the process. However, such\nchecks were unaware of rename detection logic and thus had both false\npositives and false negatives in terms of throwing errors and saying\nthe merge could not proceed. Those false positives and negatives were\nintrinsic to the design of merge-recursive; they'll never be fixed.\nThe only correct fix is to fundamentally change the design -- do the\nentire merge operation without touching the working tree or index, and\nonly after you have a new resulting merge tree, then do a checkout\noperation to switch from the old tree to the new merge result tree;\nonly at that final checkout step do you fail if there are locally\nmodified files or untracked files/directories in the way. That's what\nmerge-ort does.\n\nmerge-ort is also written to allow a whole rebase to be done\n\"in-memory\" and only update the working tree and index after we have\nthe final end-tree. test-tool fast-rebase already works this way. In\nsuch a case, we'd only notice an untracked directory being in the way\nat the point we hit a conflict or when we got to the end of the\nrebase. So again, erroring out when there is a directory with >= 1\nuntracked files (and at the end of the operation) isn't a new issue at\nall. The only thing that is new is we'd also error out if a directory\nhad 0 untracked files in it.\n\nHowever, I disagree with the \"a lot of wasted work\"; rebase is\nhundreds or thousands of times faster than it used to be as you\nhighlighted in your blog post. ;-) Also, as per my --remerge-diff\nthread, merge-ort can remerge 33.5 merge commits/second in linux.git.\nAnd that's just merges; rebases are faster than merges because (a) the\nintersection (rather than union) of changes on both sides of history\ntends to be much smaller for rebases than merges, due to the fact that\nindividual commits tend to only modify a handful of files (and\nmerge-ort optimizations work very well when the intersection of files\nchanged on both sides is small), and (b) we can cache rename detection\nresults for the upstream side of history in rebases.\n\n> Even though I think that's *not* what you want for rebase, it ironically\n\nI think it's totally what we want for rebase; it's perfectly aligned\nwith the merge-ort correctness changes to move things to\nfail-at-the-end for working-directory-related problems.\n\n> *is* what you might want for bisect, since the path we'll take isn't\n> known ahead of time. So even if there are some paths that would result\n> in a directory -> file conversion in the cwd, we don't need to fail the\n> operation ahead of time if the bisection doesn't actually take that\n> path.\n>\n> On the other hand, it does still leave a lot of work on the table if the\n> bisection does eventually want to change the current working directory\n> into a file, depending on where that change happens.\n>\n> Maybe those cases are pretty niche in practice. I have to imagine that\n> they are. If my guess is right, then I think your approach makes sense.\n\n:-)\n"}]}