Message ID | d156d04ca87f9fcffb1c08a08576dddcdc64c055.1581620351.git.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | receive.denyCurrentBranch: respect all worktrees | expand |
"Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Hariom Verma <hariom18599@gmail.com> > > `receive.denyCurrentBranch` currently has a bug where it allows pushing > into the current branch of a non-bare repository as long as it does not > have any commits. Can patch 3/3 be split into two, so that the fix would protect an already populated branch that is checked out anywhere (not in the primary worktree--which is the bug you are fixing) from getting updated but still allow an unborn branch to be updated, and then have patch 4/3 that forbids an update to even an unborn branch "checked out" in any working tree? This update to the test can then become part of patch 4/3. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > "Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: Hariom Verma <hariom18599@gmail.com> >> >> `receive.denyCurrentBranch` currently has a bug where it allows pushing >> into the current branch of a non-bare repository as long as it does not >> have any commits. > > Can patch 3/3 be split into two, so that the fix would protect an > already populated branch that is checked out anywhere (not in the > primary worktree--which is the bug you are fixing) from getting > updated but still allow an unborn branch to be updated, and then > have patch 4/3 that forbids an update to even an unborn branch > "checked out" in any working tree? This update to the test can then > become part of patch 4/3. Oh, another thing. The patch 4/3 that starts forbidding a push into a checked out unborn branch should also have a test that makes sure that such an attempt fails. IOW, making the test repository used in the test you changed to a bare one, to allow existing test to still test what it wants to test, like you did in this patch is OK, but we would want to have a new test that prepares a repository with the primary and the secondary worktrees, check out an unborn branch in each of the worktrees, and make sure receive.denyCurrentBranch would prevent "git push" to update these branches. Thanks.
Hi Junio, On Thu, 13 Feb 2020, Junio C Hamano wrote: > "Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Hariom Verma <hariom18599@gmail.com> > > > > `receive.denyCurrentBranch` currently has a bug where it allows pushing > > into the current branch of a non-bare repository as long as it does not > > have any commits. > > Can patch 3/3 be split into two, I actually don't think so. The `refs_resolve_unsafe()` function simply requires a tip commit, so it is the wrong function to call in this context. And the fix for it is to use a more appropriate function, which 3/3 already does (although for an unrelated reason). In other words, a fix for one bug would be a fix for the other, and (probably) vice versa. > so that the fix would protect an already populated branch that is > checked out anywhere (not in the primary worktree--which is the bug you > are fixing) from getting updated but still allow an unborn branch to be > updated, and then have patch 4/3 that forbids an update to even an > unborn branch "checked out" in any working tree? This update to the > test can then become part of patch 4/3. I agree that this merits a regression test. Thanks, Dscho > > Thanks. >
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> Can patch 3/3 be split into two, > > I actually don't think so. The `refs_resolve_unsafe()` function simply > ... > In other words, a fix for one bug would be a fix for the other, and > (probably) vice versa. What mislead me was the way this step presented itself. It sounded as if the primary (and possibly the only) thing the series wanted to fix was to make .denyCurrentBranch pay attention to other worktrees, and while fixing that, it broke as collateral damage a "feature" that denyCurrentBranch allows an unborn branch to be updated no matter what and called it a bugfix when it was not a bug. If the series is fixing two bugs, perhaps 2/3 can first fix it for a primary worktree case by seeing what HEAD symref for the primary worktree points at is the target of a push without iterating over all the worktrees, have the test change in 2/3 (i.e. "fixing the 'unborn' case revealed a wrong expectation in an existing test"), and a couple of new tests to see what a push from sideways would do to an unborn branch that is checked out in the primary worktree when .denyCurrentBranch is and isn't in effect. Then 3/3 can use the same logic to see if one worktree is OK with the proposed ref update by the push used in 2/3 (which no longer uses refs_resolve_unsafe()') to check for all worktrees. The new tests introduced in 2/3 would be extended to see what happens when the unborn branch getting updated by the push happens to be checked out in a secondary worktree. That would have avoided misleading this reader. Thanks.
On Fri, Feb 14, 2020 at 8:33 PM Junio C Hamano <gitster@pobox.com> wrote: > If the series is fixing two bugs, perhaps 2/3 can first fix it for a > primary worktree case by seeing what HEAD symref for the primary > worktree points at is the target of a push without iterating over > all the worktrees, have the test change in 2/3 (i.e. "fixing the > 'unborn' case revealed a wrong expectation in an existing test"), > and a couple of new tests to see what a push from sideways would do > to an unborn branch that is checked out in the primary worktree when > .denyCurrentBranch is and isn't in effect. > > Then 3/3 can use the same logic to see if one worktree is OK with > the proposed ref update by the push used in 2/3 (which no longer > uses refs_resolve_unsafe()') to check for all worktrees. The new > tests introduced in 2/3 would be extended to see what happens when > the unborn branch getting updated by the push happens to be checked > out in a secondary worktree. As far as my understanding goes, what we want is: 1) fixing `.denyCurrentBranch` for unborn branches in primary worktree. (2/3) 2) writing test (expect it to fail if `unborn` & 'non-bare' case) (2/3) 3) making `.denyCurrentBranch` respect all worktrees. (3/3) 4) extending tests written in step 2 for secondary worktrees. (3/3) Correct me if I'm wrong. As I'm not entirely familiar with working and structure of `.denyCurrentBranch`. So I might need more explicit explanation. Thanks, Hariom
Hariom verma <hariom18599@gmail.com> writes: > On Fri, Feb 14, 2020 at 8:33 PM Junio C Hamano <gitster@pobox.com> wrote: >> If the series is fixing two bugs, perhaps 2/3 can first fix it for a >> primary worktree case by seeing what HEAD symref for the primary >> worktree points at is the target of a push without iterating over >> all the worktrees, have the test change in 2/3 (i.e. "fixing the >> 'unborn' case revealed a wrong expectation in an existing test"), >> and a couple of new tests to see what a push from sideways would do >> to an unborn branch that is checked out in the primary worktree when >> .denyCurrentBranch is and isn't in effect. >> >> Then 3/3 can use the same logic to see if one worktree is OK with >> the proposed ref update by the push used in 2/3 (which no longer >> uses refs_resolve_unsafe()') to check for all worktrees. The new >> tests introduced in 2/3 would be extended to see what happens when >> the unborn branch getting updated by the push happens to be checked >> out in a secondary worktree. > > As far as my understanding goes, what we want is: > 1) fixing `.denyCurrentBranch` for unborn branches in primary worktree. (2/3) > 2) writing test (expect it to fail if `unborn` & 'non-bare' case) (2/3) > 3) making `.denyCurrentBranch` respect all worktrees. (3/3) > 4) extending tests written in step 2 for secondary worktrees. (3/3) > > Correct me if I'm wrong. If the above is what _you_ want, then there is nothing for me to correct ;-) What I suggested was somewhat different, though. 1) get_main_worktree() fix you have as [1/3] in the current round. 2) fix `.denyCurrentBranch` for unborn branches in the primary worktree, new tests for the cases I outlined in the message you are responding to, and adjusting the test (i.e. what you have as [2/3] in the current round). 3) fix `.denyCurrentBranch` to pay attention to HEAD of not just the primary worktree but of all the worktrees, and add tests. Thanks.
On Mon, Feb 17, 2020 at 5:19 AM Junio C Hamano <gitster@pobox.com> wrote: > What I suggested was somewhat different, though. > > 1) get_main_worktree() fix you have as [1/3] in the current round. > > 2) fix `.denyCurrentBranch` for unborn branches in the primary > worktree, new tests for the cases I outlined in the message you > are responding to, and adjusting the test (i.e. what you have > as [2/3] in the current round). > > 3) fix `.denyCurrentBranch` to pay attention to HEAD of not just > the primary worktree but of all the worktrees, and add tests. I doubt that it's possible to solve these 2 issues separately. As dscho said: "a fix for one bug would be a fix for the other, and (probably) vice versa." As I discussed with dscho, the best possible solution for this situation is to demonstrate the bug and fix it in succeeding commit. I have sent this v2[1] for this patch. Thanks, Hariom [1]: https://lore.kernel.org/git/pull.535.v2.git.1582410908.gitgitgadget@gmail.com/
diff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh index 75cbfcc392..e3975bd21d 100755 --- a/t/t5509-fetch-push-namespaces.sh +++ b/t/t5509-fetch-push-namespaces.sh @@ -20,7 +20,7 @@ test_expect_success setup ' ) && commit0=$(cd original && git rev-parse HEAD^) && commit1=$(cd original && git rev-parse HEAD) && - git init pushee && + git init --bare pushee && git init puller '