| Message ID | 1c36c334-9f10-3859-c92f-3d889e226769@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | bisect: fix "reset" when branch is checked out elsewhere | expand |
Rubén Justo <rjusto@gmail.com> writes: > Since 1d0fa89 (checkout: add --ignore-other-wortrees, 2015-01-03) we > have a safety valve in checkout/switch to prevent the same branch from > being checked out simultaneously in multiple worktrees. > > If a branch is bisected in a worktree while also being checked out in > another worktree; when the bisection is finished, checking out the > branch back in the current worktree may fail. True. But we should explain why failing is a bad thing here. After all, "git checkout foo" to check out a branch 'foo' that is being used in a different worktree linked to the same repository fails, and that is a GOOD thing. Is the logic behind your "may fail and that is a bad thing" something like this? When "git bisect reset" goes back to the branch, it used to error out if the same branch is checked out in a different worktree. Since this can happen only after the end-user deliberately checked out the branch twice, erroring out does not contribute to any safety. Having said that... > @@ -245,7 +245,8 @@ static int bisect_reset(const char *commit) > struct child_process cmd = CHILD_PROCESS_INIT; > > cmd.git_cmd = 1; > - strvec_pushl(&cmd.args, "checkout", branch.buf, "--", NULL); > + strvec_pushl(&cmd.args, "checkout", "--ignore-other-worktrees", > + branch.buf, "--", NULL); "git bisect reset" does take an arbitrary commit or branch name, which may not be the original branch the user was on. If the user did not have any branch checked out twice, can they do something like $ git checkout foo $ git bisect start HEAD HEAD~20 ... bisect session finds the first bad commit ... $ git bisect reset bar where 'foo' is checked out only in this worktree? What happens if 'bar' has been checked out in a different worktree linked to the same repository while this bisect was going on? The current code may fail due to the safety "checkout" has, but isn't that exactly what we want? I.e. prevent 'bar' from being checked out twice by mistake? Giving 'bar' on the command line of "bisect reset" is likely because the user wants to start working on that branch, without necessarily knowing that they already have a worktree that checked out the branch elsewhere---in other words, isn't that a lazy folks' shorthand for "git bisect reset && git checkout bar"? If we loosen the safety only when bisect_reset() receives NULL to its commit parameter, i.e. we are going back to the original branch, the damage might be limited to narrower use cases, but I still am not sure if the loosening is worth it. IIUC, the scenario that may be helped would go like this: ... another worktree has 'foo' checked out ... $ git checkout --ignore-other-worktrees foo $ git bisect start HEAD HEAD~20 ... bisect session finds the first bad commit ... $ git bisect reset The last step wants to go back to 'foo', and it may be more convenient if it did not fail to go back to the risky state the user originally created. But doesn't the error message already tell us how to go back after this step refuses to recreate such a risky state? It sugests "git bisect reset <commit>" to switch to a detached HEAD, so presumably, after seeing the above fail and reading the error message, the user could do $ git bisect reset foo^{commit} to finish the bisect session and detach the head at 'foo', and then the "usual" mantra to recreate the risky state that 'foo' is checked out twice can be done, i.e. $ git checkout --ignore-other-worktrees foo So, I am not sure if this is a good idea in general. Or do I misunderstand why you think "checking out may fail" is a bad thing?
On 22-ene-2023 18:01:46, Junio C Hamano wrote: > Rubén Justo <rjusto@gmail.com> writes: > > > Since 1d0fa89 (checkout: add --ignore-other-wortrees, 2015-01-03) we > > have a safety valve in checkout/switch to prevent the same branch from > > being checked out simultaneously in multiple worktrees. > > > > If a branch is bisected in a worktree while also being checked out in > > another worktree; when the bisection is finished, checking out the > > branch back in the current worktree may fail. > > True. But we should explain why failing is a bad thing here. After > all, "git checkout foo" to check out a branch 'foo' that is being > used in a different worktree linked to the same repository fails, > and that is a GOOD thing. Is the logic behind your "may fail and > that is a bad thing" something like this? > > When "git bisect reset" goes back to the branch, it used to error > out if the same branch is checked out in a different worktree. > Since this can happen only after the end-user deliberately checked > out the branch twice, erroring out does not contribute to any > safety. > > Having said that... > > > @@ -245,7 +245,8 @@ static int bisect_reset(const char *commit) > > struct child_process cmd = CHILD_PROCESS_INIT; > > > > cmd.git_cmd = 1; > > - strvec_pushl(&cmd.args, "checkout", branch.buf, "--", NULL); > > + strvec_pushl(&cmd.args, "checkout", "--ignore-other-worktrees", > > + branch.buf, "--", NULL); > > "git bisect reset" does take an arbitrary commit or branch name, > which may not be the original branch the user was on. If the > user did not have any branch checked out twice, can they do > something like > > $ git checkout foo > $ git bisect start HEAD HEAD~20 > ... bisect session finds the first bad commit ... > $ git bisect reset bar > > where 'foo' is checked out only in this worktree? What happens if > 'bar' has been checked out in a different worktree linked to the > same repository while this bisect was going on? The current code > may fail due to the safety "checkout" has, but isn't that exactly > what we want? I.e. prevent 'bar' from being checked out twice by > mistake? Giving 'bar' on the command line of "bisect reset" is > likely because the user wants to start working on that branch, > without necessarily knowing that they already have a worktree that > checked out the branch elsewhere---in other words, isn't that a lazy > folks' shorthand for "git bisect reset && git checkout bar"? > > If we loosen the safety only when bisect_reset() receives NULL to > its commit parameter, i.e. we are going back to the original branch, > the damage might be limited to narrower use cases, but I still am > not sure if the loosening is worth it. > > IIUC, the scenario that may be helped would go like this: > > ... another worktree has 'foo' checked out ... > $ git checkout --ignore-other-worktrees foo > $ git bisect start HEAD HEAD~20 > ... bisect session finds the first bad commit ... > $ git bisect reset > > The last step wants to go back to 'foo', and it may be more > convenient if it did not fail to go back to the risky state the user > originally created. But doesn't the error message already tell us > how to go back after this step refuses to recreate such a risky > state? It sugests "git bisect reset <commit>" to switch to a > detached HEAD, so presumably, after seeing the above fail and > reading the error message, the user could do > > $ git bisect reset foo^{commit} > > to finish the bisect session and detach the head at 'foo', and then > the "usual" mantra to recreate the risky state that 'foo' is checked > out twice can be done, i.e. > > $ git checkout --ignore-other-worktrees foo > > So, I am not sure if this is a good idea in general. > > Or do I misunderstand why you think "checking out may fail" is a bad > thing? Maybe we make it fail for no reason. Thank you for thinking about this with depth. I know you are aware, but for other readers; In another thread, a bug-fix is being reviewed that, if accepted, will affect "bisect". With that fix, the phrase "checking out may fail" will become "checking out will fail". But does it need to fail? As you said, we want to reject normal check outs of a branch when that branch is already checked out in other worktrees. Because of this, sounds reasonable to leave the implicit 'checkout' in 'bisect reset' to fail in those cases, and just add some tests to notice if this changes in the future. But, and this is what makes me think that "checking out will fail" is the wrong choice for "bisect", while bisecting, with the worktree in a detached HEAD state, the branch to which "bisect reset" will check out back (BISECT_START), is still considered checked out in the working tree: $ git checkout -b work $ git bisect start HEAD HEAD~3 ... bisect detaches the current worktree ... $ git worktree add work Preparing worktree (checkout out 'work') fatal: 'work' is already checked out at ... So, checking out back to the implicitly checked out branch sounds like it should not fail. And should be pretty secure (under the risk accepted by the user) because we do not allow to normally check out the branch in another worktree. We even reject the checkout in the current worktree while bisecting, but let's leave that for another time. Note that due to the bug, what we are changing here is the reset on "second worktrees". That means, "bisect reset" on the main worktree has been allowed for some time, we are just making it official. And this is the less disrupting change in the usage. Which does not justify the change but does support it. This is the reasoning I had and what makes me think that "checking out may fail" is an inconvenience for the user, without any gain. Now, if these arguments are reasonable, the next issue is what and how to allow. I chose at least strict, but maybe we can do something more elaborate... just NULL sounds good.
Rubén Justo <rjusto@gmail.com> writes: > But, and this is what makes me think that "checking out will fail" is the wrong > choice for "bisect", while bisecting, with the worktree in a detached HEAD > state, the branch to which "bisect reset" will check out back (BISECT_START), > is still considered checked out in the working tree: > > $ git checkout -b work > $ git bisect start HEAD HEAD~3 > ... bisect detaches the current worktree ... > $ git worktree add work > Preparing worktree (checkout out 'work') > fatal: 'work' is already checked out at ... > > So, checking out back to the implicitly checked out branch sounds like it > should not fail. If that is what you are aiming at, I suspect that the posted patch is doing it in a wrong way. Instead, we should just declare that the branch being bisected does not mean the branch cannot be checked out elsewhere, so that $ git worktree add --detach ../another HEAD^0 $ git checkout -b work $ git bisect start work work~3 ... detaches ... $ git -C ../another checkout work should just work, no? I admit I haven't thought things through, but I tend to be sympathetic to such a declaration. After all, "bisect" is a read-only operation as far as the branch you happened to be on when you started a bisect session is concerned. Jumping around and materializing tree states recorded in various commits leading to the tip of the branch and inspecting them would not change anything on the branch itself. And more importantly, the branch being checked out in another worktree and modified there should not break the bisection, EXCEPT that the final "git bisect reset" (without arguments) would fail if the other worktree removed the branch. So, how about removing the is_worktree_being_bisected() check from find_shared_symref(), so that not just "worktree add" and "bisect reset", but "checkout" and "switch" are allowed to make the branch current even it is being bisected elsewhere? That would affect the other topic, I suspect, as well. It may be a positive change. Or are there cases I missed, where the branch being bisected should not be touched from elsewhere, and we cannot remove the check from find_shared_symref()? Thanks.
Junio C Hamano <gitster@pobox.com> writes: > the branch itself. And more importantly, the branch being checked > out in another worktree and modified there should not break the > bisection, EXCEPT that the final "git bisect reset" (without > arguments) would fail if the other worktree removed the branch. And "bisect reset [<branch>]" (with or without arguments) should not ignore other worktrees when it runs "checkout" internally. You might have done * checkout 'work' in worktree A * start bisection of it there * checkout 'work' in worktree B * finish bisection of 'work' in worktree A * "git bisect reset" and the third step should allow you work on 'work' in the other worktree, but then the last step should not allow 'work' to be checked out in two places (it is OK for the user to use "git bisect reset main" and then "git checkout --ignore-other work" to work it around, of course, but the default should be safe). Hmm?
On 26-ene-2023 09:06:28, Junio C Hamano wrote: > > But, and this is what makes me think that "checking out will fail" is the wrong > > choice for "bisect", while bisecting, with the worktree in a detached HEAD > > state, the branch to which "bisect reset" will check out back (BISECT_START), > > is still considered checked out in the working tree: > > > > $ git checkout -b work > > $ git bisect start HEAD HEAD~3 > > ... bisect detaches the current worktree ... > > $ git worktree add work > > Preparing worktree (checkout out 'work') > > fatal: 'work' is already checked out at ... > > > > So, checking out back to the implicitly checked out branch sounds like it > > should not fail. > > If that is what you are aiming at, I suspect that the posted patch > is doing it in a wrong way. Instead, we should just declare that > the branch being bisected does not mean the branch cannot be checked > out elsewhere, so that > > $ git worktree add --detach ../another HEAD^0 > $ git checkout -b work > $ git bisect start work work~3 > ... detaches ... > $ git -C ../another checkout work > > should just work, no? Sorry, I should have been more explicit, I meant: "checking out back to the implicitly checked out branch sounds like it should not fail in the worktree where the user is bisecting". So, to your question: no, in another worktree should not work without the --ignore-other-worktrees. But $ git checkout -b work $ git worktree add -f ../another work $ git -C ../another bisect start work work~3 ... detaches ... $ git -C ../another bisect reset should work. > So, how about removing the is_worktree_being_bisected() check from > find_shared_symref(), so that not just "worktree add" and "bisect > reset", but "checkout" and "switch" are allowed to make the branch > current even it is being bisected elsewhere? The devil is in the details: "git branch -m", "git branch -d". We're not ready to have BISECT_START pointing to a deleted branch, or renaming a branch pointed by it. Also the inconvenience that, if we allow the user to checkout normally the same branch in another worktree, we are not providing any facility in "git bisect reset" to override the "already checked out". We are forcing the user to "git bisect reset HEAD~0 && git checkout --ignore-other-worktrees ...". Which, to me, sound as an inconvenience. > > That would affect the other topic, I suspect, as well. I'm not sure. The other topic is somewhat independent of what we decide here. This series started because the other topic is going to affect "git bisect", not the other way around. But this series can be considered even if the other topic is discarded. Now, "git bisect reset" with a branch checked out multiple times, works in the first worktree that has the branch checkedout (the main tree is always the first), and fails in the next ones. This is due to a bug the other topic is fixing. This series aims to make "git bisect reset" to the original branch, work in all worktrees. Independently of the other topic.
Rubén Justo <rjusto@gmail.com> writes: > The devil is in the details: "git branch -m", "git branch -d". > > We're not ready to have BISECT_START pointing to a deleted branch, or > renaming a branch pointed by it. It indicates that the callers of find_shared_symref() to see if "is this branch being actively used by checked out, bisected, or rebased, and I shouldn't touch it?" need to know more than what find_shared_symref() interface gives them---namely, "can I repoint it to a different commit?" and "can I make it disappear?" are different conditions they need to be able to learn. Until that distinction becomes expressible, I am actually OK with forbidding both operations, i.e. while a branch is being bisected elsewhere, we should be able to update its tip to point at a different commit, but it is OK to forbid that because we cannot allow the branch be renamed away or removed. Thanks.
diff --git a/builtin/bisect.c b/builtin/bisect.c index cc9483e851..56da34648b 100644 --- a/builtin/bisect.c +++ b/builtin/bisect.c @@ -245,7 +245,8 @@ static int bisect_reset(const char *commit) struct child_process cmd = CHILD_PROCESS_INIT; cmd.git_cmd = 1; - strvec_pushl(&cmd.args, "checkout", branch.buf, "--", NULL); + strvec_pushl(&cmd.args, "checkout", "--ignore-other-worktrees", + branch.buf, "--", NULL); if (run_command(&cmd)) { error(_("could not check out original" " HEAD '%s'. Try 'git bisect" diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh index 98a72ff78a..cc8acbab18 100755 --- a/t/t6030-bisect-porcelain.sh +++ b/t/t6030-bisect-porcelain.sh @@ -122,6 +122,29 @@ test_expect_success 'bisect start without -- takes unknown arg as pathspec' ' grep bar ".git/BISECT_NAMES" ' +test_expect_success 'bisect reset: back in a branch checked out also elsewhere' ' + echo "shared" > branch.expect && + test_bisect_reset() { + git -C $1 bisect start && + git -C $1 bisect good $HASH1 && + git -C $1 bisect bad $HASH3 && + git -C $1 bisect reset && + git -C $1 branch --show-current > branch.output && + cmp branch.expect branch.output + } && + test_when_finished " + git worktree remove wt1 && + git worktree remove wt2 && + git branch -d shared + " && + git worktree add wt1 -b shared && + git worktree add wt2 -f shared && + # we test in both worktrees to ensure that works + # as expected with "first" and "next" worktrees + test_bisect_reset wt1 && + test_bisect_reset wt2 +' + test_expect_success 'bisect reset: back in the main branch' ' git bisect reset && echo "* main" > branch.expect &&
Since 1d0fa89 (checkout: add --ignore-other-wortrees, 2015-01-03) we have a safety valve in checkout/switch to prevent the same branch from being checked out simultaneously in multiple worktrees. If a branch is bisected in a worktree while also being checked out in another worktree; when the bisection is finished, checking out the branch back in the current worktree may fail. Let's teach bisect to use the "--ignore-other-worktrees" flag. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- builtin/bisect.c | 3 ++- t/t6030-bisect-porcelain.sh | 23 +++++++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-)