* [PATCH] bisect: fix "reset" when branch is checked out elsewhere @ 2023-01-22 1:38 Rubén Justo 2023-01-23 2:01 ` Junio C Hamano 2023-02-04 22:57 ` [PATCH v2] " Rubén Justo 0 siblings, 2 replies; 15+ messages in thread From: Rubén Justo @ 2023-01-22 1:38 UTC (permalink / raw) To: Git List; +Cc: Junio C Hamano, Phillip Wood 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(-) 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 && -- 2.39.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] bisect: fix "reset" when branch is checked out elsewhere 2023-01-22 1:38 [PATCH] bisect: fix "reset" when branch is checked out elsewhere Rubén Justo @ 2023-01-23 2:01 ` Junio C Hamano 2023-01-26 2:18 ` Rubén Justo 2023-02-04 22:57 ` [PATCH v2] " Rubén Justo 1 sibling, 1 reply; 15+ messages in thread From: Junio C Hamano @ 2023-01-23 2:01 UTC (permalink / raw) To: Rubén Justo; +Cc: Git List, Phillip Wood 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? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] bisect: fix "reset" when branch is checked out elsewhere 2023-01-23 2:01 ` Junio C Hamano @ 2023-01-26 2:18 ` Rubén Justo 2023-01-26 17:06 ` Junio C Hamano 0 siblings, 1 reply; 15+ messages in thread From: Rubén Justo @ 2023-01-26 2:18 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git List, Phillip Wood 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] bisect: fix "reset" when branch is checked out elsewhere 2023-01-26 2:18 ` Rubén Justo @ 2023-01-26 17:06 ` Junio C Hamano 2023-01-26 17:13 ` Junio C Hamano 2023-02-04 22:46 ` Rubén Justo 0 siblings, 2 replies; 15+ messages in thread From: Junio C Hamano @ 2023-01-26 17:06 UTC (permalink / raw) To: Rubén Justo; +Cc: Git List, Phillip Wood 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] bisect: fix "reset" when branch is checked out elsewhere 2023-01-26 17:06 ` Junio C Hamano @ 2023-01-26 17:13 ` Junio C Hamano 2023-02-04 22:46 ` Rubén Justo 1 sibling, 0 replies; 15+ messages in thread From: Junio C Hamano @ 2023-01-26 17:13 UTC (permalink / raw) To: Rubén Justo; +Cc: Git List, Phillip Wood 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? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] bisect: fix "reset" when branch is checked out elsewhere 2023-01-26 17:06 ` Junio C Hamano 2023-01-26 17:13 ` Junio C Hamano @ 2023-02-04 22:46 ` Rubén Justo 2023-02-06 19:04 ` Junio C Hamano 1 sibling, 1 reply; 15+ messages in thread From: Rubén Justo @ 2023-02-04 22:46 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git List, Phillip Wood 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] bisect: fix "reset" when branch is checked out elsewhere 2023-02-04 22:46 ` Rubén Justo @ 2023-02-06 19:04 ` Junio C Hamano 0 siblings, 0 replies; 15+ messages in thread From: Junio C Hamano @ 2023-02-06 19:04 UTC (permalink / raw) To: Rubén Justo; +Cc: Git List, Phillip Wood 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2] bisect: fix "reset" when branch is checked out elsewhere 2023-01-22 1:38 [PATCH] bisect: fix "reset" when branch is checked out elsewhere Rubén Justo 2023-01-23 2:01 ` Junio C Hamano @ 2023-02-04 22:57 ` Rubén Justo 2023-02-06 22:29 ` Junio C Hamano ` (2 more replies) 1 sibling, 3 replies; 15+ messages in thread From: Rubén Justo @ 2023-02-04 22:57 UTC (permalink / raw) To: Git List; +Cc: Junio C Hamano, Phillip Wood 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> --- Range-diff against v1: 1: f902db6bfb ! 1: 72e1526313 bisect: fix "reset" when branch is checked out elsewhere @@ builtin/bisect.c: static int bisect_reset(const char *commit) cmd.git_cmd = 1; - strvec_pushl(&cmd.args, "checkout", branch.buf, "--", NULL); -+ strvec_pushl(&cmd.args, "checkout", "--ignore-other-worktrees", -+ branch.buf, "--", NULL); ++ strvec_pushl(&cmd.args, "checkout", NULL); ++ if (!commit) ++ strvec_pushl(&cmd.args, "--ignore-other-worktrees", NULL); ++ strvec_pushl(&cmd.args, branch.buf, "--", NULL); if (run_command(&cmd)) { error(_("could not check out original" " HEAD '%s'. Try 'git bisect" builtin/bisect.c | 5 ++++- t/t6030-bisect-porcelain.sh | 23 +++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/builtin/bisect.c b/builtin/bisect.c index 7301740267..46fba8db50 100644 --- a/builtin/bisect.c +++ b/builtin/bisect.c @@ -244,7 +244,10 @@ 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", NULL); + if (!commit) + strvec_pushl(&cmd.args, "--ignore-other-worktrees", NULL); + strvec_pushl(&cmd.args, 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 3ba4fdf615..fb01bd6abc 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 && -- 2.39.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2] bisect: fix "reset" when branch is checked out elsewhere 2023-02-04 22:57 ` [PATCH v2] " Rubén Justo @ 2023-02-06 22:29 ` Junio C Hamano 2023-02-08 0:30 ` Rubén Justo 2023-02-15 4:52 ` Eric Sunshine 2023-02-20 22:53 ` [PATCH v3] " Rubén Justo 2 siblings, 1 reply; 15+ messages in thread From: Junio C Hamano @ 2023-02-06 22:29 UTC (permalink / raw) To: Rubén Justo; +Cc: Git List, Phillip Wood 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. Sorry for asking possibly the same question again (which may mean that the phrasing of this paragraph is misleading), but isn't it a good thing if in this sequence: - I checkout 'main' and start bisecting (BISECT_HEAD says 'main'); - I then checkout 'main' in another worktree; I may even make a commit or two, or even rename 'main' to 'master'. - I finish bisection and "bisect reset" tries to take me back to 'main', which may notice that 'main' is checked out in the other worktree, and fail. the last one failed? After the above sequence, I now have two worktrees, both checking out 'main', and it is exactly the situation the safety valve tries to prevent from occuring, no? Or is the new behaviour considered better because the third step would try to check out 'main' that is checked out elsewhere only if the second step was forced, so the person who decided to touch 'main' in another worktree should already be aware of the risk and we should disable the safety valve in the third step automatically? I am not sure if that is a sensible argument, but if that is the case, let's spell it out in the proposed log message. Thanks. > builtin/bisect.c | 5 ++++- > t/t6030-bisect-porcelain.sh | 23 +++++++++++++++++++++++ > 2 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/builtin/bisect.c b/builtin/bisect.c > index 7301740267..46fba8db50 100644 > --- a/builtin/bisect.c > +++ b/builtin/bisect.c > @@ -244,7 +244,10 @@ 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", NULL); > + if (!commit) > + strvec_pushl(&cmd.args, "--ignore-other-worktrees", NULL); > + strvec_pushl(&cmd.args, branch.buf, "--", NULL); OK, so this time around "git bisect reset" to go back to the original branch gets --ignore-other-worktrees but "git bisect reset HEAD" or other forms that names a branch still gets the safety. That makes the blast radius smaller, but I am not 100% sure if loosening the safety is a good thing. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] bisect: fix "reset" when branch is checked out elsewhere 2023-02-06 22:29 ` Junio C Hamano @ 2023-02-08 0:30 ` Rubén Justo 2023-02-08 5:16 ` Junio C Hamano 0 siblings, 1 reply; 15+ messages in thread From: Rubén Justo @ 2023-02-08 0:30 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git List, Phillip Wood On 06-feb-2023 14:29:03, 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. > > Sorry for asking possibly the same question again (which may mean No problem. I am sorry because I don't understand what's worrying you. > that the phrasing of this paragraph is misleading), but isn't it a > good thing if in this sequence: > > - I checkout 'main' and start bisecting (BISECT_HEAD says 'main'); > > - I then checkout 'main' in another worktree; I may even make a > commit or two, or even rename 'main' to 'master'. > > - I finish bisection and "bisect reset" tries to take me back to > 'main', which may notice that 'main' is checked out in the other > worktree, and fail. > > the last one failed? After the above sequence, I now have two > worktrees, both checking out 'main', and it is exactly the situation > the safety valve tries to prevent from occuring, no? We are considering the initial branch (BISECT_START) as a branch checked out _implicitly_ in the worktree that is bisecting. Doesn't that provide us and the user enough safety? And is this not enough safety for us to allow "git bisect reset"? Just "git bisect reset", without any other argument. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] bisect: fix "reset" when branch is checked out elsewhere 2023-02-08 0:30 ` Rubén Justo @ 2023-02-08 5:16 ` Junio C Hamano 2023-02-08 21:54 ` Rubén Justo 0 siblings, 1 reply; 15+ messages in thread From: Junio C Hamano @ 2023-02-08 5:16 UTC (permalink / raw) To: Rubén Justo; +Cc: Git List, Phillip Wood Rubén Justo <rjusto@gmail.com> writes: > No problem. I am sorry because I don't understand what's worrying you. > >> that the phrasing of this paragraph is misleading), but isn't it a >> good thing if in this sequence: >> >> - I checkout 'main' and start bisecting (BISECT_HEAD says 'main'); >> >> - I then checkout 'main' in another worktree; I may even make a >> commit or two, or even rename 'main' to 'master'. >> >> - I finish bisection and "bisect reset" tries to take me back to >> 'main', which may notice that 'main' is checked out in the other >> worktree, and fail. >> >> the last one failed? After the above sequence, I now have two >> worktrees, both checking out 'main', and it is exactly the situation >> the safety valve tries to prevent from occuring, no? > > We are considering the initial branch (BISECT_START) as a branch checked > out _implicitly_ in the worktree that is bisecting. Doesn't that > provide us and the user enough safety? If that is a question, then the answer is no. If that is rhetorical, then I just do not see how it gives us any safety. In the end, if you allow "bisect reset" to check out 'main' in the worktree you used to run bisection, the 'main' branch is checked out twice, once there, and another checkout in the other worktree. That is exactly what "git checkout 'main'" in one worktree while 'main' is already checked out in another would prevent from happening, no? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] bisect: fix "reset" when branch is checked out elsewhere 2023-02-08 5:16 ` Junio C Hamano @ 2023-02-08 21:54 ` Rubén Justo 0 siblings, 0 replies; 15+ messages in thread From: Rubén Justo @ 2023-02-08 21:54 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git List, Phillip Wood On 07-feb-2023 21:16:59, Junio C Hamano wrote: > Rubén Justo <rjusto@gmail.com> writes: > > > No problem. I am sorry because I don't understand what's worrying you. > > > >> that the phrasing of this paragraph is misleading), but isn't it a > >> good thing if in this sequence: > >> > >> - I checkout 'main' and start bisecting (BISECT_HEAD says 'main'); > >> > >> - I then checkout 'main' in another worktree; I may even make a > >> commit or two, or even rename 'main' to 'master'. > >> > >> - I finish bisection and "bisect reset" tries to take me back to > >> 'main', which may notice that 'main' is checked out in the other > >> worktree, and fail. > >> > >> the last one failed? After the above sequence, I now have two > >> worktrees, both checking out 'main', and it is exactly the situation > >> the safety valve tries to prevent from occuring, no? > > > > We are considering the initial branch (BISECT_START) as a branch checked > > out _implicitly_ in the worktree that is bisecting. Doesn't that > > provide us and the user enough safety? > > If that is a question, then the answer is no. If that is > rhetorical, then I just do not see how it gives us any safety. > > In the end, if you allow "bisect reset" to check out 'main' in the > worktree you used to run bisection, the 'main' branch is checked out > twice, once there, and another checkout in the other worktree. That > is exactly what "git checkout 'main'" in one worktree while 'main' > is already checked out in another would prevent from happening, no? Yes, but I still don't understand what you are worried about. If we compare with "rebase": "git rebase --abort" does checkout back to the original branch too. But as "git rebase" is in a more evolved "builtin transition", and uses reset_head() instead of spawing a new Git with "checkout", it avoids the "--ignore-other-worktrees". The safety I'm considering with "git bisect reset" is that while a branch is being bisected in a worktree, that branch cannot be (without forcing): checked out in another worktree, deleted or renamed. And this safety is enough, to me, to alleviate the user from having to "git bisect reset" to a "safe" place and then "git checkout --ignore-other-worktrees" to have the BISECT_START branch checked out again. Also, note that the aim of this patch is not to introduce a new behavior, but fix how "git bisect reset" works with multiple worktrees. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] bisect: fix "reset" when branch is checked out elsewhere 2023-02-04 22:57 ` [PATCH v2] " Rubén Justo 2023-02-06 22:29 ` Junio C Hamano @ 2023-02-15 4:52 ` Eric Sunshine 2023-02-15 22:20 ` Rubén Justo 2023-02-20 22:53 ` [PATCH v3] " Rubén Justo 2 siblings, 1 reply; 15+ messages in thread From: Eric Sunshine @ 2023-02-15 4:52 UTC (permalink / raw) To: Rubén Justo; +Cc: Git List, Junio C Hamano, Phillip Wood On Sat, Feb 4, 2023 at 6:02 PM Rubén Justo <rjusto@gmail.com> wrote: > 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> > --- > diff --git 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' ' > +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 > + " && As mentioned in my review[1] of one of your other patches, &&-chaining within the argument to test_when_finished() is probably undesirable in this case since failure of any cleanup command would cause test_when_finish() to fail, which would cause the test to fail overall. [1]: https://lore.kernel.org/git/CAPig+cQpizjmhmDKb=HPrcYqqRq7JpvC-NZvY7B9eBbG+NrfKw@mail.gmail.com/ > + 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 > +' ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] bisect: fix "reset" when branch is checked out elsewhere 2023-02-15 4:52 ` Eric Sunshine @ 2023-02-15 22:20 ` Rubén Justo 0 siblings, 0 replies; 15+ messages in thread From: Rubén Justo @ 2023-02-15 22:20 UTC (permalink / raw) To: Eric Sunshine; +Cc: Git List, Junio C Hamano, Phillip Wood On 14-feb-2023 23:52:08, Eric Sunshine wrote: > On Sat, Feb 4, 2023 at 6:02 PM Rubén Justo <rjusto@gmail.com> wrote: > > 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> > > --- > > diff --git 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' ' > > +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 > > + " && > > As mentioned in my review[1] of one of your other patches, &&-chaining > within the argument to test_when_finished() is probably undesirable in > this case since failure of any cleanup command would cause > test_when_finish() to fail, which would cause the test to fail > overall. As I said in my other response, if there is no argument against this change, I'll reroll with it. Thanks. > > [1]: https://lore.kernel.org/git/CAPig+cQpizjmhmDKb=HPrcYqqRq7JpvC-NZvY7B9eBbG+NrfKw@mail.gmail.com/ > > > + 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 > > +' ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3] bisect: fix "reset" when branch is checked out elsewhere 2023-02-04 22:57 ` [PATCH v2] " Rubén Justo 2023-02-06 22:29 ` Junio C Hamano 2023-02-15 4:52 ` Eric Sunshine @ 2023-02-20 22:53 ` Rubén Justo 2 siblings, 0 replies; 15+ messages in thread From: Rubén Justo @ 2023-02-20 22:53 UTC (permalink / raw) To: Git List; +Cc: Junio C Hamano, Phillip Wood, Eric Sunshine When the user starts a bisection in a worktree, we save the branch currently checked out in that worktree (BISECT_START) to allow the user to go back to that branch later on. From that moment and until the bisection ends, the branch is no longer checked out in that worktree, but we give it the same consideration as if it was checked out, which means that we prevent: - deleting that branch and - checking out that branch in another worktree, unless the user force the ckeck out. "bisect" was introduced as a helper script in 8cc6a08 ([PATCH] Making it easier to find which change introduced a bug, 2005-07-30), and originally based its functioning in Git builtin commands. Although "bisect" is now itself a builtin command, it still spawns Git sub-processes to use other builtin commands, like "checkout". Since 1d0fa89 (checkout: add --ignore-other-worktrees, 2015-01-03) we have a safety valve in "checkout" and "switch" commands to prevent the user from normally checking out simultaneously the same branch in multiple worktrees. If the user, using "--ignore-other-worktrees", checks out a branch before or while bisecting that same branch in another worktree, we may fail when the user ends the bisection using "git bisect reset": $ git worktree add work ../work/ $ git checkout --ignore-other-worktrees work $ git -C ../work bisect start HEAD HEAD~3 ... the user inspects some commits ... $ git -C ../work bisect reset fatal: 'work' is already checked out at ... error: could not check out original HEAD 'work'. Try 'git bisect reset <commit>'. Thanks to the special consideration we give to the branch while being bisected, and the safety valve introduced in 1d0fa89, we can assume the user is aware and responsible of the "multiple checked out branch" situation. This makes sensible to allow the user to go back to the original branch using "git bisect reset" and avoid him a somewhat less intuitive sequence like: "git bisect reset work~0 && git checkout --ignore-other-worktree work". Let's teach "bisect" to use the "--ignore-other-worktrees" when the user wants to end the bisection. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- Changes since v2 - Reworded the message to be more descriptive. - Using "||:" to chain commands in the test_when_finished block, suggested by Eric. This series started because of a bug, being fixed in another series, allowed "git bisect reset" to work in certain cases where multiple branches were checked out. Once the bug is fixed, "git bisect reset" will no longer work in those scenarios. Which is not bad, but taking into account the different scenarios and the fact that some users may rely on this bug as a feature, I think that the current patch proposes a sensible and convenient change for the user. Range-diff against v2: 1: 0fe0bc70dd ! 1: 3b2ac1aa8f bisect: fix "reset" when branch is checked out elsewhere @@ Metadata ## Commit message ## bisect: fix "reset" when branch is checked out elsewhere - 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. + When the user starts a bisection in a worktree, we save the branch + currently checked out in that worktree (BISECT_START) to allow the user + to go back to that branch later on. - 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. + From that moment and until the bisection ends, the branch is no longer + checked out in that worktree, but we give it the same consideration as + if it was checked out, which means that we prevent: + - deleting that branch and + - checking out that branch in another worktree, unless the user force + the ckeck out. - Let's teach bisect to use the "--ignore-other-worktrees" flag. + "bisect" was introduced as a helper script in 8cc6a08 ([PATCH] Making it + easier to find which change introduced a bug, 2005-07-30), and + originally based its functioning in Git builtin commands. Although + "bisect" is now itself a builtin command, it still spawns Git + sub-processes to use other builtin commands, like "checkout". + + Since 1d0fa89 (checkout: add --ignore-other-worktrees, 2015-01-03) we + have a safety valve in "checkout" and "switch" commands to prevent the + user from normally checking out simultaneously the same branch in + multiple worktrees. + + If the user, using "--ignore-other-worktrees", checks out a branch + before or while bisecting that same branch in another worktree, we may + fail when the user ends the bisection using "git bisect reset": + + $ git worktree add work ../work/ + $ git checkout --ignore-other-worktrees work + $ git -C ../work bisect start HEAD HEAD~3 + ... the user inspects some commits ... + $ git -C ../work bisect reset + fatal: 'work' is already checked out at ... + error: could not check out original HEAD 'work'. Try 'git bisect reset <commit>'. + + Thanks to the special consideration we give to the branch while being + bisected, and the safety valve introduced in 1d0fa89, we can assume the + user is aware and responsible of the "multiple checked out branch" + situation. This makes sensible to allow the user to go back to the + original branch using "git bisect reset" and avoid him a somewhat less + intuitive sequence like: "git bisect reset work~0 && git checkout + --ignore-other-worktree work". + + Let's teach "bisect" to use the "--ignore-other-worktrees" when the user + wants to end the bisection. Signed-off-by: Rubén Justo <rjusto@gmail.com> @@ builtin/bisect.c: static int bisect_reset(const char *commit) - strvec_pushl(&cmd.args, "checkout", branch.buf, "--", NULL); + strvec_pushl(&cmd.args, "checkout", NULL); + if (!commit) -+ strvec_pushl(&cmd.args, "--ignore-other-worktrees", NULL); ++ strvec_pushl(&cmd.args, "--ignore-other-worktrees", ++ NULL); + strvec_pushl(&cmd.args, branch.buf, "--", NULL); if (run_command(&cmd)) { error(_("could not check out original" @@ t/t6030-bisect-porcelain.sh: test_expect_success 'bisect start without -- takes + cmp branch.expect branch.output + } && + test_when_finished " -+ git worktree remove wt1 && -+ git worktree remove wt2 && -+ git branch -d shared ++ git worktree remove wt1 ||: ++ git worktree remove wt2 ||: ++ git branch -d shared ||: + " && + git worktree add wt1 -b shared && + git worktree add wt2 -f shared && builtin/bisect.c | 6 +++++- t/t6030-bisect-porcelain.sh | 23 +++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/builtin/bisect.c b/builtin/bisect.c index 7301740267..011f674b08 100644 --- a/builtin/bisect.c +++ b/builtin/bisect.c @@ -244,7 +244,11 @@ 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", NULL); + if (!commit) + strvec_pushl(&cmd.args, "--ignore-other-worktrees", + NULL); + strvec_pushl(&cmd.args, 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 3ba4fdf615..1d047f1c1a 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 && -- 2.34.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
end of thread, other threads:[~2023-02-20 22:53 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-01-22 1:38 [PATCH] bisect: fix "reset" when branch is checked out elsewhere Rubén Justo 2023-01-23 2:01 ` Junio C Hamano 2023-01-26 2:18 ` Rubén Justo 2023-01-26 17:06 ` Junio C Hamano 2023-01-26 17:13 ` Junio C Hamano 2023-02-04 22:46 ` Rubén Justo 2023-02-06 19:04 ` Junio C Hamano 2023-02-04 22:57 ` [PATCH v2] " Rubén Justo 2023-02-06 22:29 ` Junio C Hamano 2023-02-08 0:30 ` Rubén Justo 2023-02-08 5:16 ` Junio C Hamano 2023-02-08 21:54 ` Rubén Justo 2023-02-15 4:52 ` Eric Sunshine 2023-02-15 22:20 ` Rubén Justo 2023-02-20 22:53 ` [PATCH v3] " Rubén Justo
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).