receive.denyCurrentBranch: respect all worktrees

---
 worktree.c | 1 +
 1 file changed, 1 insertion(+)

worktree.c +1 -0

index 5b4793caa3..7c8cd21317 100644
@@ -51,6 +51,7 @@ static struct worktree *get_main_worktree(void)
 	struct strbuf worktree_path = STRBUF_INIT;
 
 	strbuf_add_absolute_path(&worktree_path, get_git_common_dir());
+	strbuf_strip_suffix(&worktree_path, "/.");
 	if (!strbuf_strip_suffix(&worktree_path, "/.git"))
 		strbuf_strip_suffix(&worktree_path, "/.");

Signature · 1 line

gitgitgadget

Hariom Verma via GitGitGadget

author

2020-02-13 18:59 UTC #2 lore raw

PATCH 2/3 t5509: initialized `pushee` as bare repository

From: Hariom Verma <[email protected]>

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. This would cause t5509 to fail once that bug is fixed because it pushes into an unborn current branch.

In t5509, no operations are performed inside pushee, as it is only a target for git push and git ls-remote calls. Therefore it does not need to have a worktree. So, it is safe to change pushee to a bare repository.

In reply to Junio C Hamano

---
 t/t5509-fetch-push-namespaces.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/t5509-fetch-push-namespaces.sh +1 -1

index 75cbfcc392..e3975bd21d 100755
@@ -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
 '

Signature · 1 line

gitgitgadget

Junio C Hamano

2020-02-13 20:14 UTC #3 lore raw

Re: [PATCH 2/3] t5509: initialized `pushee` as bare repository

Quoted 5 lines · Hariom Verma via GitGitGadget

From: Hariom Verma <[email protected]>
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

2020-02-13 20:34 UTC #4 lore raw

Re: [PATCH 2/3] t5509: initialized `pushee` as bare repository

Quoted 8 lines · Junio C Hamano

Quoted 5 lines · Hariom Verma via GitGitGadget
From: Hariom Verma <[email protected]>
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.

Quoted 7 lines

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.

Johannes Schindelin

2020-02-14 11:59 UTC #5 lore raw

Re: [PATCH 2/3] t5509: initialized `pushee` as bare repository

Hi Junio,

Quoted 9 lines · Junio C Hamano

Quoted 5 lines · Hariom Verma via GitGitGadget
From: Hariom Verma <[email protected]>
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.

Quoted 6 lines

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

Quoted 3 lines

Thanks.

Junio C Hamano

2020-02-14 15:03 UTC #6 lore raw

Re: [PATCH 2/3] t5509: initialized `pushee` as bare repository

Quoted 6 lines · Johannes Schindelin

Quoted 1 line
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.

Hariom verma

2020-02-15 21:53 UTC #7 lore raw

Re: [PATCH 2/3] t5509: initialized `pushee` as bare repository

Quoted 9 lines · Junio C Hamano

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.

Quoted 6 lines

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

Junio C Hamano

2020-02-16 23:49 UTC #8 lore raw

Re: [PATCH 2/3] t5509: initialized `pushee` as bare repository

Quoted 17 lines · Hariom verma

Quoted 9 lines · Junio C Hamano
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.
Quoted 6 lines
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.

Quoted 7 lines

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.

Hariom verma

2020-02-22 22:56 UTC #9 lore raw

Re: [PATCH 2/3] t5509: initialized `pushee` as bare repository

Quoted 11 lines · Junio C Hamano

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/[email protected]/

Hariom Verma via GitGitGadget

author

2020-02-13 18:59 UTC #10 lore raw

PATCH 3/3 receive.denyCurrentBranch: respect all worktrees

From: Hariom Verma <[email protected]>

The receive.denyCurrentBranch config option controls what happens if you push to a branch that is checked out into a non-bare repository. By default, it rejects it. It can be disabled via ignore or warn. Another yet trickier option is updateInstead.

However, this setting was forgotten when the git worktree command was
introduced: only the main worktree's current branch is respected.

With this change, all worktrees are respected.

That change also leads to revealing another bug,
i.e. receive.denyCurrentBranch = true was ignored when pushing into a non-bare repository's unborn current branch. As is_ref_checked_out() returns 0 which means receive-pack does not get into conditional statement to switch deny_current_branch accordingly(ignore, warn, refuse, unconfigured, updateInstead).

receive.denyCurrentBranch uses the function refs_resolve_ref_unsafe() (called via resolve_refdup()) to resolve the symbolic ref HEAD, but that function fails when HEAD does not point at a valid commit. As we replace the call to refs_resolve_ref_unsafe() with find_shared_symref(), which has no problem finding the worktree for a given branch even if it is unborn yet, this bug is fixed at the same
time: receive.denyCurrentBranch now also handles worktrees with unborn branches as intended.

---
 builtin/receive-pack.c | 37 ++++++++++++++++++++-----------------
 t/t5516-fetch-push.sh  | 11 +++++++++++
 2 files changed, 31 insertions(+), 17 deletions(-)

builtin/receive-pack.c +20 ~~-17~~

index 411e0b4d99..b5ca3123b7 100644
@@ -27,6 +27,7 @@
 #include "object-store.h"
 #include "protocol.h"
 #include "commit-reach.h"
+#include "worktree.h"
 
 static const char * const receive_pack_usage[] = {
 	N_("git receive-pack <git-dir>"),

@@ -816,16 +817,6 @@ static int run_update_hook(struct command *cmd)
 	return finish_command(&proc);
 }
 
-static int is_ref_checked_out(const char *ref)
-{
-	if (is_bare_repository())
-		return 0;
-
-	if (!head_name)
-		return 0;
-	return !strcmp(head_name, ref);
-}
-
 static char *refuse_unconfigured_deny_msg =
 	N_("By default, updating the current branch in a non-bare repository\n"
 	   "is denied, because it will make the index and work tree inconsistent\n"

@@ -997,16 +988,27 @@ static const char *push_to_checkout(unsigned char *hash,
 		return NULL;
 }
 
-static const char *update_worktree(unsigned char *sha1)
+static const char *update_worktree(unsigned char *sha1, const struct worktree *worktree)
 {
-	const char *retval;
-	const char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : "..";
+	const char *retval, *work_tree, *git_dir = NULL;
 	struct argv_array env = ARGV_ARRAY_INIT;
 
+	if (worktree && worktree->path)
+		work_tree = worktree->path;
+	else if (git_work_tree_cfg)
+		work_tree = git_work_tree_cfg;
+	else
+		work_tree = "..";
+
 	if (is_bare_repository())
 		return "denyCurrentBranch = updateInstead needs a worktree";
+	
+	if (worktree)
+		git_dir = get_worktree_git_dir(worktree);
+	if (!git_dir)
+		git_dir = get_git_dir();
 
-	argv_array_pushf(&env, "GIT_DIR=%s", absolute_path(get_git_dir()));
+	argv_array_pushf(&env, "GIT_DIR=%s", absolute_path(git_dir));
 
 	if (!find_hook(push_to_checkout_hook))
 		retval = push_to_deploy(sha1, &env, work_tree);

@@ -1026,6 +1028,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 	struct object_id *old_oid = &cmd->old_oid;
 	struct object_id *new_oid = &cmd->new_oid;
 	int do_update_worktree = 0;
+	const struct worktree *worktree = is_bare_repository() ? NULL : find_shared_symref("HEAD", name);
 
 	/* only refs/... are allowed */
 	if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {

@@ -1037,7 +1040,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 	free(namespaced_name);
 	namespaced_name = strbuf_detach(&namespaced_name_buf, NULL);
 
-	if (is_ref_checked_out(namespaced_name)) {
+	if (worktree) {
 		switch (deny_current_branch) {
 		case DENY_IGNORE:
 			break;

@@ -1069,7 +1072,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 			return "deletion prohibited";
 		}
 
-		if (head_name && !strcmp(namespaced_name, head_name)) {
+		if (worktree || (head_name && !strcmp(namespaced_name, head_name))) {
 			switch (deny_delete_current) {
 			case DENY_IGNORE:
 				break;

@@ -1118,7 +1121,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 	}
 
 	if (do_update_worktree) {
-		ret = update_worktree(new_oid->hash);
+		ret = update_worktree(new_oid->hash, find_shared_symref("HEAD", name));
 		if (ret)
 			return ret;
 	}

t/t5516-fetch-push.sh +11 -0

index c81ca360ac..6608e391f0 100755
@@ -1712,4 +1712,15 @@ test_expect_success 'updateInstead with push-to-checkout hook' '
 	)
 '
 
+test_expect_success 'denyCurrentBranch and worktrees' '
+    git worktree add new-wt &&
+	git clone . cloned &&
+	test_commit -C cloned first &&
+	test_config receive.denyCurrentBranch refuse &&
+	test_must_fail git -C cloned push origin HEAD:new-wt &&
+	test_config receive.denyCurrentBranch updateInstead &&
+	git -C cloned push origin HEAD:new-wt &&
+	test_must_fail git -C cloned push --delete origin new-wt
+'
+
 test_done

Signature · 1 line

gitgitgadget

Hariom Verma via GitGitGadget

author

2020-02-22 22:35 UTC #11 lore raw

v2 cover receive.denyCurrentBranch: respect all worktrees

However, this setting was forgotten when the git worktree command was
introduced: only the main worktree's current branch is respected. [ fixes:
#331 ]

Incidently, this change also fixes another bug i.e. receive.denyCurrentBranch = true was ignored when pushing into a non-bare repository using ref namespaces.

Thanks, @dscho for helping me out.

Regards, Hariom

Hariom Verma (4):
get_main_worktree(): allow it to be called in the Git directory
t5509: initialized pushee as bare repository
bug: denyCurrentBranch and unborn branch with ref namespace receive.denyCurrentBranch: respect all worktrees

 builtin/receive-pack.c           | 37 +++++++++++++++++---------------
 t/t5509-fetch-push-namespaces.sh | 11 +++++++++-
 t/t5516-fetch-push.sh            | 11 ++++++++++
 worktree.c                       |  1 +
 4 files changed, 42 insertions(+), 18 deletions(-)

base-commit: 232378479ee6c66206d47a9be175e3a39682aea6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-535%2Fharry-hov%2Fdeny-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-535/harry-hov/deny-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/535

Range-diff vs v1:

 1:  902c8a3f171 = 1:  8718facbc95 get_main_worktree(): allow it to be called in the Git directory
 2:  d156d04ca87 ! 2:  5c749e044a3 t5509: initialized `pushee` as bare repository

@@ -3,9 +3,9 @@
t5509: initialized pushee as bare repository

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. This would cause t5509 to fail once that bug is fixed
- because it pushes into an unborn current branch.
+ into non-bare repository using namespaces as long as it does not have any
+ commits. This would cause t5509 to fail once that bug is fixed because it
+ pushes into an unborn current branch.

In t5509, no operations are performed inside pushee, as it is only a target for git push and git ls-remote calls. Therefore it does not

 -:  ----------- > 3:  b3e573d44a9 bug: denyCurrentBranch and unborn branch with ref namespace
 3:  3352c0bffc1 ! 4:  61e5f75a6f9 receive.denyCurrentBranch: respect all worktrees

@@ -14,10 +14,10 @@

That change also leads to revealing another bug,
i.e. receive.denyCurrentBranch = true was ignored when pushing into a
- non-bare repository's unborn current branch. As is_ref_checked_out()
- returns 0 which means receive-pack does not get into conditional
- statement to switch deny_current_branch accordingly(ignore, warn,
- refuse, unconfigured, updateInstead).
+ non-bare repository's unborn current branch using ref namespaces. As
+ is_ref_checked_out() returns 0 which means receive-pack does not get
+ into conditional statement to switch deny_current_branch accordingly
+ (ignore, warn, refuse, unconfigured, updateInstead).

receive.denyCurrentBranch uses the function refs_resolve_ref_unsafe() (called via resolve_refdup()) to resolve the symbolic ref HEAD, but
@@ -26,7 +26,7 @@
find_shared_symref(), which has no problem finding the worktree for a given branch even if it is unborn yet, this bug is fixed at the same
time: receive.denyCurrentBranch now also handles worktrees with unborn
- branches as intended.
+ branches as intended even while using ref namespaces.

@@ -127,6 +127,19 @@
return ret;
}

+ diff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh
+ --- a/t/t5509-fetch-push-namespaces.sh
+ +++ b/t/t5509-fetch-push-namespaces.sh
+@@
+ test_cmp expect actual
+ '
+
+-test_expect_failure 'denyCurrentBranch and unborn branch with ref namespace' ' ++test_expect_success 'denyCurrentBranch and unborn branch with ref namespace' '
+ cd original &&
+ git init unborn &&
+ git remote add unborn-namespaced "ext::git --namespace=namespace %s unborn" &&
+
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -135,7 +148,7 @@
'

+test_expect_success 'denyCurrentBranch and worktrees' '
-+ git worktree add new-wt &&
++ git worktree add new-wt &&
+ git clone . cloned &&
+ test_commit -C cloned first &&
+ test_config receive.denyCurrentBranch refuse &&

Signature · 1 line

gitgitgadget

Hariom Verma via GitGitGadget

author

2020-02-22 22:35 UTC #12 lore raw

PATCH v2 1/4 get_main_worktree(): allow it to be called in the Git directory

From: Hariom Verma <[email protected]>

When called in the Git directory of a non-bare repository, this function would not return the directory of the main worktree, but of the Git directory instead.

Let's fix this.

---
 worktree.c | 1 +
 1 file changed, 1 insertion(+)

worktree.c +1 -0

index 5b4793caa34..7c8cd213171 100644
@@ -51,6 +51,7 @@ static struct worktree *get_main_worktree(void)
 	struct strbuf worktree_path = STRBUF_INIT;
 
 	strbuf_add_absolute_path(&worktree_path, get_git_common_dir());
+	strbuf_strip_suffix(&worktree_path, "/.");
 	if (!strbuf_strip_suffix(&worktree_path, "/.git"))
 		strbuf_strip_suffix(&worktree_path, "/.");

Signature · 1 line

gitgitgadget

Hariom Verma via GitGitGadget

author

2020-02-22 22:35 UTC #13 lore raw

PATCH v2 2/4 t5509: initialized `pushee` as bare repository

From: Hariom Verma <[email protected]>

receive.denyCurrentBranch currently has a bug where it allows pushing into non-bare repository using namespaces as long as it does not have any commits. This would cause t5509 to fail once that bug is fixed because it pushes into an unborn current branch.

---
 t/t5509-fetch-push-namespaces.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/t5509-fetch-push-namespaces.sh +1 -1

index 75cbfcc392c..e3975bd21de 100755
@@ -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
 '

Signature · 1 line

gitgitgadget

Junio C Hamano

2020-02-23 06:24 UTC #14 lore raw

Re: [PATCH v2 2/4] t5509: initialized `pushee` as bare repository

Quoted 2 lines · Hariom Verma via GitGitGadget

From: Hariom Verma <[email protected]>
Subject: Re: [PATCH v2 2/4] t5509: initialized pushee as bare repository

s/initialized/initialize/ at least, perhaps.

Subject: [PATCH v2 2/4] t5509: use a bare repository for test push target

may be easier to understand, though. Then the first paragraph of the body of the proposed message, which gives an excellent description of how the current tests rely on a bug that we plan to fix in a later step of the series, explains why we do not want to push into a non-bare repository.

Quoted 4 lines

receive.denyCurrentBranch currently has a bug where it allows pushing into non-bare repository using namespaces as long as it does not have any commits. This would cause t5509 to fail once that bug is fixed because it pushes into an unborn current branch.

And then you give a good description why not just it is safe, but it makes more sense. Very well explained.

Quoted 24 lines

In t5509, no operations are performed inside pushee, as it is only a target for git push and git ls-remote calls. Therefore it does not need to have a worktree. So, it is safe to change pushee to a bare repository.
Helped-by Johannes Schindelin <[email protected]>
Signed-off-by Hariom Verma <[email protected]>
---
 t/t5509-fetch-push-namespaces.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
t/t5509-fetch-push-namespaces.sh +1 -1
index 75cbfcc392c..e3975bd21de 100755
@@ -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
 '

Hariom Verma via GitGitGadget

author

2020-02-22 22:35 UTC #15 lore raw

PATCH v2 3/4 bug: denyCurrentBranch and unborn branch with ref namespace

From: Hariom Verma <[email protected]>

git supports an interesting feature 'Git namespaces' that allows dividing the refs of a single repository into multiple namespaces, each of which has its own branches, tags, and HEAD. But unfortunately, there exists a bug in denyCurrentBranch which allows pushing into a non-bare repository using a ref namespace even if it does not have any commits

Here is a nice and short demonstration of that bug.

---
 t/t5509-fetch-push-namespaces.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

t/t5509-fetch-push-namespaces.sh +9 -0

index e3975bd21de..c89483fdba2 100755
@@ -152,4 +152,13 @@ test_expect_success 'clone chooses correct HEAD (v2)' '
 	test_cmp expect actual
 '
 
+test_expect_failure 'denyCurrentBranch and unborn branch with ref namespace' '
+	cd original &&
+	git init unborn &&
+	git remote add unborn-namespaced "ext::git --namespace=namespace %s unborn" &&
+	test_must_fail git push unborn-namespaced HEAD:master &&
+	test_config -C unborn receive.denyCurrentBranch updateInstead &&
+	git push unborn-namespaced HEAD:master
+'
+
 test_done

Signature · 1 line

gitgitgadget

Junio C Hamano

2020-02-23 06:11 UTC #16 lore raw

Re: [PATCH v2 3/4] bug: denyCurrentBranch and unborn branch with ref namespace

Quoted 1 line · Hariom Verma via GitGitGadget

+test_expect_failure 'denyCurrentBranch and unborn branch with ref namespace' '

Please do not chdir around in the test script. The next person who adds new test after this test will be surprised that his/her test does not start at the top-level of the test/trash directory, but in the "original" subdirectory.

And no, adding "&& cd .." at the end of this &&-cascade is *not* a fix---when any of the steps chained with && fails, such a "we have moved the process to a wrong place, so let's move back with 'cd ..'" will not get executed.

t/t5509-fetch-push-namespaces.sh test_expect_success 'clone chooses correct HEAD (v2)' ' · line 162

Quoted 7 lines

t/t5509-fetch-push-namespaces.sh +7 -0

+	cd original &&
+	git init unborn &&
+	git remote add unborn-namespaced "ext::git --namespace=namespace %s unborn" &&
+	test_must_fail git push unborn-namespaced HEAD:master &&
+	test_config -C unborn receive.denyCurrentBranch updateInstead &&
+	git push unborn-namespaced HEAD:master
+'

What is often done to fix is to execute what you need to run in a subshell, e.g.

	test_expect_success 'demonstration' '
		(
			cd original &&
			git init unborn &&
			...
			test_must_fail git push ... &&
			git -C unborn config ... &&
			git push ...
		)
	'

The use of "git config" instead of "test_config" in the above illustration is deliberate---the latter does not work and should not be used inside a subshell.

Quoted 2 lines

+
test_done

Hariom Verma via GitGitGadget

author

2020-02-22 22:35 UTC #17 lore raw

PATCH v2 4/4 receive.denyCurrentBranch: respect all worktrees

From: Hariom Verma <[email protected]>

However, this setting was forgotten when the git worktree command was
introduced: only the main worktree's current branch is respected.

With this change, all worktrees are respected.

That change also leads to revealing another bug,
i.e. receive.denyCurrentBranch = true was ignored when pushing into a non-bare repository's unborn current branch using ref namespaces. As is_ref_checked_out() returns 0 which means receive-pack does not get into conditional statement to switch deny_current_branch accordingly (ignore, warn, refuse, unconfigured, updateInstead).

---
 builtin/receive-pack.c           | 37 +++++++++++++++++---------------
 t/t5509-fetch-push-namespaces.sh |  2 +-
 t/t5516-fetch-push.sh            | 11 ++++++++++
 3 files changed, 32 insertions(+), 18 deletions(-)

builtin/receive-pack.c +20 ~~-17~~

index 411e0b4d999..b5ca3123b78 100644
@@ -27,6 +27,7 @@
 #include "object-store.h"
 #include "protocol.h"
 #include "commit-reach.h"
+#include "worktree.h"
 
 static const char * const receive_pack_usage[] = {
 	N_("git receive-pack <git-dir>"),

@@ -816,16 +817,6 @@ static int run_update_hook(struct command *cmd)
 	return finish_command(&proc);
 }
 
-static int is_ref_checked_out(const char *ref)
-{
-	if (is_bare_repository())
-		return 0;
-
-	if (!head_name)
-		return 0;
-	return !strcmp(head_name, ref);
-}
-
 static char *refuse_unconfigured_deny_msg =
 	N_("By default, updating the current branch in a non-bare repository\n"
 	   "is denied, because it will make the index and work tree inconsistent\n"

@@ -997,16 +988,27 @@ static const char *push_to_checkout(unsigned char *hash,
 		return NULL;
 }
 
-static const char *update_worktree(unsigned char *sha1)
+static const char *update_worktree(unsigned char *sha1, const struct worktree *worktree)
 {
-	const char *retval;
-	const char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : "..";
+	const char *retval, *work_tree, *git_dir = NULL;
 	struct argv_array env = ARGV_ARRAY_INIT;
 
+	if (worktree && worktree->path)
+		work_tree = worktree->path;
+	else if (git_work_tree_cfg)
+		work_tree = git_work_tree_cfg;
+	else
+		work_tree = "..";
+
 	if (is_bare_repository())
 		return "denyCurrentBranch = updateInstead needs a worktree";
+	
+	if (worktree)
+		git_dir = get_worktree_git_dir(worktree);
+	if (!git_dir)
+		git_dir = get_git_dir();
 
-	argv_array_pushf(&env, "GIT_DIR=%s", absolute_path(get_git_dir()));
+	argv_array_pushf(&env, "GIT_DIR=%s", absolute_path(git_dir));
 
 	if (!find_hook(push_to_checkout_hook))
 		retval = push_to_deploy(sha1, &env, work_tree);

@@ -1026,6 +1028,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 	struct object_id *old_oid = &cmd->old_oid;
 	struct object_id *new_oid = &cmd->new_oid;
 	int do_update_worktree = 0;
+	const struct worktree *worktree = is_bare_repository() ? NULL : find_shared_symref("HEAD", name);
 
 	/* only refs/... are allowed */
 	if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {

@@ -1037,7 +1040,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 	free(namespaced_name);
 	namespaced_name = strbuf_detach(&namespaced_name_buf, NULL);
 
-	if (is_ref_checked_out(namespaced_name)) {
+	if (worktree) {
 		switch (deny_current_branch) {
 		case DENY_IGNORE:
 			break;

@@ -1069,7 +1072,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 			return "deletion prohibited";
 		}
 
-		if (head_name && !strcmp(namespaced_name, head_name)) {
+		if (worktree || (head_name && !strcmp(namespaced_name, head_name))) {
 			switch (deny_delete_current) {
 			case DENY_IGNORE:
 				break;

@@ -1118,7 +1121,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 	}
 
 	if (do_update_worktree) {
-		ret = update_worktree(new_oid->hash);
+		ret = update_worktree(new_oid->hash, find_shared_symref("HEAD", name));
 		if (ret)
 			return ret;
 	}

t/t5509-fetch-push-namespaces.sh +1 -1

index c89483fdba2..6270fb7b576 100755
@@ -152,7 +152,7 @@ test_expect_success 'clone chooses correct HEAD (v2)' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'denyCurrentBranch and unborn branch with ref namespace' '
+test_expect_success 'denyCurrentBranch and unborn branch with ref namespace' '
 	cd original &&
 	git init unborn &&
 	git remote add unborn-namespaced "ext::git --namespace=namespace %s unborn" &&

t/t5516-fetch-push.sh +11 -0

index c81ca360ac4..49982b0fd90 100755
@@ -1712,4 +1712,15 @@ test_expect_success 'updateInstead with push-to-checkout hook' '
 	)
 '
 
+test_expect_success 'denyCurrentBranch and worktrees' '
+	git worktree add new-wt &&
+	git clone . cloned &&
+	test_commit -C cloned first &&
+	test_config receive.denyCurrentBranch refuse &&
+	test_must_fail git -C cloned push origin HEAD:new-wt &&
+	test_config receive.denyCurrentBranch updateInstead &&
+	git -C cloned push origin HEAD:new-wt &&
+	test_must_fail git -C cloned push --delete origin new-wt
+'
+
 test_done

Signature · 1 line

gitgitgadget

Junio C Hamano

2020-02-23 06:18 UTC #18 lore raw

Quoted 23 lines · Hariom Verma via GitGitGadget

t/t5509-fetch-push-namespaces.sh +1 -1

index c89483fdba2..6270fb7b576 100755
@@ -152,7 +152,7 @@ test_expect_success 'clone chooses correct HEAD (v2)' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'denyCurrentBranch and unborn branch with ref namespace' '
+test_expect_success 'denyCurrentBranch and unborn branch with ref namespace' '
 	cd original &&
 	git init unborn &&
 	git remote add unborn-namespaced "ext::git --namespace=namespace %s unborn" &&

t/t5516-fetch-push.sh +2 -0

index c81ca360ac4..49982b0fd90 100755
@@ -1712,4 +1712,15 @@ test_expect_success 'updateInstead with push-to-checkout hook' '
 	)
 '
 
+test_expect_success 'denyCurrentBranch and worktrees' '
+	git worktree add new-wt &&

t/t5516-fetch-push.sh test_expect_success 'updateInstead with push-to-checkout hook' ' · line 1726

Quoted 10 lines

t/t5516-fetch-push.sh +9 -0

+	git clone . cloned &&
+	test_commit -C cloned first &&
+	test_config receive.denyCurrentBranch refuse &&
+	test_must_fail git -C cloned push origin HEAD:new-wt &&
+	test_config receive.denyCurrentBranch updateInstead &&
+	git -C cloned push origin HEAD:new-wt &&
+	test_must_fail git -C cloned push --delete origin new-wt
+'
+
 test_done

This adds one new test and also flips a test that was added in a separate step that expected a failure to expect success, which looks a bit strange.

For a series this small, having a test that demonstrates that the updated code works as expected together with the fix to the code in a single patch is easier to manage. After applying a single test+fix patch, you can easily apply the same patch except for the test part in reverse on top, if you need to see in what way the code without the change breaks by running the test.

On a truly large fix, sometimes it may make sense to add a failing test and nothing else and then a separate step that changes the code and flips the expectation of the test from failure->success, but I think a change this size is easier to handle without such an artificial split.

Thanks.

Hariom Verma via GitGitGadget

author

2020-02-23 18:57 UTC #19 lore raw

v3 cover receive.denyCurrentBranch: respect all worktrees

However, this setting was forgotten when the git worktree command was
introduced: only the main worktree's current branch is respected. [ fixes:
#331 ]

Incidently, this change also fixes another bug i.e. receive.denyCurrentBranch = true was ignored when pushing into a non-bare repository using ref namespaces.

Thanks, @dscho for helping me out.

Regards, Hariom

Hariom Verma (3):
get_main_worktree(): allow it to be called in the Git directory
t5509: use a bare repository for test push target receive.denyCurrentBranch: respect all worktrees

 builtin/receive-pack.c           | 37 +++++++++++++++++---------------
 t/t5509-fetch-push-namespaces.sh | 13 ++++++++++-
 t/t5516-fetch-push.sh            | 11 ++++++++++
 worktree.c                       |  1 +
 4 files changed, 44 insertions(+), 18 deletions(-)

base-commit: 232378479ee6c66206d47a9be175e3a39682aea6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-535%2Fharry-hov%2Fdeny-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-535/harry-hov/deny-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/535

Range-diff vs v2:

 1:  8718facbc95 = 1:  8718facbc95 get_main_worktree(): allow it to be called in the Git directory
 2:  5c749e044a3 ! 2:  ae749310f06 t5509: initialized `pushee` as bare repository

@@ -1,6 +1,6 @@
Author: Hariom Verma <[email protected]>

- t5509: initialized pushee as bare repository
+ t5509: use a bare repository for test push target

receive.denyCurrentBranch currently has a bug where it allows pushing into non-bare repository using namespaces as long as it does not have any

 3:  b3e573d44a9 < -:  ----------- bug: denyCurrentBranch and unborn branch with ref namespace
 4:  61e5f75a6f9 ! 3:  d21a590d6c2 receive.denyCurrentBranch: respect all worktrees
     @@ -134,11 +134,18 @@
       	test_cmp expect actual
       '
       
     --test_expect_failure 'denyCurrentBranch and unborn branch with ref namespace' '
      +test_expect_success 'denyCurrentBranch and unborn branch with ref namespace' '
     - 	cd original &&
     - 	git init unborn &&
     - 	git remote add unborn-namespaced "ext::git --namespace=namespace %s unborn" &&
     ++	(
     ++		cd original &&
     ++		git init unborn &&
     ++		git remote add unborn-namespaced "ext::git --namespace=namespace %s unborn" &&
     ++		test_must_fail git push unborn-namespaced HEAD:master &&
     ++		git -C unborn config receive.denyCurrentBranch updateInstead &&
     ++		git push unborn-namespaced HEAD:master
     ++	)
     ++'
     ++
     + test_done
      
       diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
       --- a/t/t5516-fetch-push.sh

Signature · 1 line

gitgitgadget

Hariom Verma via GitGitGadget

author

2020-02-23 18:57 UTC #20 lore raw

PATCH v3 1/3 get_main_worktree(): allow it to be called in the Git directory

From: Hariom Verma <[email protected]>

When called in the Git directory of a non-bare repository, this function would not return the directory of the main worktree, but of the Git directory instead.

Let's fix this.

---
 worktree.c | 1 +
 1 file changed, 1 insertion(+)

worktree.c +1 -0

index 5b4793caa34..7c8cd213171 100644
@@ -51,6 +51,7 @@ static struct worktree *get_main_worktree(void)
 	struct strbuf worktree_path = STRBUF_INIT;
 
 	strbuf_add_absolute_path(&worktree_path, get_git_common_dir());
+	strbuf_strip_suffix(&worktree_path, "/.");
 	if (!strbuf_strip_suffix(&worktree_path, "/.git"))
 		strbuf_strip_suffix(&worktree_path, "/.");

Signature · 1 line

gitgitgadget

Eric Sunshine

2020-02-24 01:42 UTC #21 lore raw

Re: [PATCH v3 1/3] get_main_worktree(): allow it to be called in the Git directory

Quoted 1 line · Hariom Verma via GitGitGadget

get_main_worktree(): allow it to be called in the Git directory

This title is a bit too generic; it fails to explain what this patch is really fixing. Perhaps:

get_main_worktree: correctly normalize worktree path when in .git dir

or something.

Quoted 3 lines

When called in the Git directory of a non-bare repository, this function would not return the directory of the main worktree, but of the Git directory instead.

"Git directory" is imprecise. As a reader, I can't tell if this means the main worktree into which the project is checked out or the .git directory itself. Please write it instead as ".git directory".

worktree.c static struct worktree *get_main_worktree(void) · line 54

Quoted 13 lines

The reason: when the Git directory is the current working directory, the absolute path of the common directory will be reported with a trailing /.git/., which the code of get_main_worktree() does not handle correctly.
Signed-off-by Hariom Verma <[email protected]>
---
worktree.c +1 -0
@@ -51,6 +51,7 @@ static struct worktree *get_main_worktree(void)
        strbuf_add_absolute_path(&worktree_path, get_git_common_dir());
+       strbuf_strip_suffix(&worktree_path, "/.");
        if (!strbuf_strip_suffix(&worktree_path, "/.git"))
                strbuf_strip_suffix(&worktree_path, "/.");

This change makes the code unnecessarily confusing and effectively turns the final line into dead code. I would much rather see the three cases spelled out explicitly, perhaps like this:

    if (!strbuf_strip_suffix(&worktree_path, "/.git/.") && /* in .git dir */
        !strbuf_strip_suffix(&worktree_path, "/.git/")) /* in worktree */
            strbuf_strip_suffix(&worktree_path, "/."); /* in bare repo */

Also, please add a test to ensure that this behavior doesn't regress in the future. You can probably test it via the "git worktree list" command, so perhaps add the test to t/t2402-worktree-list.sh.

Hariom verma

2020-02-24 11:09 UTC #22 lore raw

Re: [PATCH v3 1/3] get_main_worktree(): allow it to be called in the Git directory

Hi Eric,

Quoted 16 lines · Eric Sunshine

This title is a bit too generic; it fails to explain what this patch is really fixing. Perhaps:
get_main_worktree: correctly normalize worktree path when in .git dir
or something.
"Git directory" is imprecise. As a reader, I can't tell if this means the main worktree into which the project is checked out or the .git directory itself. Please write it instead as ".git directory". [...]
This change makes the code unnecessarily confusing and effectively turns the final line into dead code. I would much rather see the three cases spelled out explicitly, perhaps like this:

Quoted 3 lines

    if (!strbuf_strip_suffix(&worktree_path, "/.git/.") && /* in .git dir */
        !strbuf_strip_suffix(&worktree_path, "/.git/")) /* in worktree */
            strbuf_strip_suffix(&worktree_path, "/."); /* in bare repo */

I'll implement these comments in the next revision for sure.

Quoted 3 lines

Also, please add a test to ensure that this behavior doesn't regress in the future. You can probably test it via the "git worktree list" command, so perhaps add the test to t/t2402-worktree-list.sh.

There already exists tests in "t/t2402-worktree-list.sh" which lists and verifies all worktrees. Does this make sense to write a new test that also does kinda the same thing?

Thanks,
Hariom

Eric Sunshine

2020-02-24 17:00 UTC #23 lore raw

Re: [PATCH v3 1/3] get_main_worktree(): allow it to be called in the Git directory

Quoted 5 lines · Hariom verma

Quoted 4 lines · Eric Sunshine

    if (!strbuf_strip_suffix(&worktree_path, "/.git/.") && /* in .git dir */
        !strbuf_strip_suffix(&worktree_path, "/.git/")) /* in worktree */
            strbuf_strip_suffix(&worktree_path, "/."); /* in bare repo */

Quoted 7 lines

Quoted 3 lines
Also, please add a test to ensure that this behavior doesn't regress in the future. You can probably test it via the "git worktree list" command, so perhaps add the test to t/t2402-worktree-list.sh.
There already exists tests in "t/t2402-worktree-list.sh" which lists and verifies all worktrees. Does this make sense to write a new test that also does kinda the same thing?

The change this patch is making is to correctly strip the suffix "/.git/." from the main worktree's path since that suffix was not getting stripped correctly by the existing code. We want a test that verifies that the "/.git/." suffix is indeed being stripped once this change is applied. If there is an existing test which already checks the output of "git worktree list" when invoked from within the .git directory, then that test should suffice, but then I would have expected that you would have had to tweak the existing test to make it succeed after this change. If there is no such test which verifies that "/.git/." is being stripped, then this patch should add one. "git worktree list" would be a possible way to implement such a test.

Johannes Schindelin

2020-02-24 18:58 UTC #24 lore raw

Re: [PATCH v3 1/3] get_main_worktree(): allow it to be called in the Git directory

In reply to Hariom verma

Hi,

Quoted 17 lines · Hariom verma

Quoted 16 lines · Eric Sunshine
This title is a bit too generic; it fails to explain what this patch is really fixing. Perhaps:
get_main_worktree: correctly normalize worktree path when in .git dir
or something.
"Git directory" is imprecise. As a reader, I can't tell if this means the main worktree into which the project is checked out or the .git directory itself. Please write it instead as ".git directory". [...]
This change makes the code unnecessarily confusing and effectively turns the final line into dead code. I would much rather see the three cases spelled out explicitly, perhaps like this:

Quoted 3 lines

    if (!strbuf_strip_suffix(&worktree_path, "/.git/.") && /* in .git dir */
        !strbuf_strip_suffix(&worktree_path, "/.git/")) /* in worktree */
            strbuf_strip_suffix(&worktree_path, "/."); /* in bare repo */

I would be really cautious about that.

To me, the originally proposed change says: strip /., if any. Then, strip /.git, and if successful, strip another /., if any.

That reads pretty fine to me. It makes sense.

Above-mentioned proposal, however, puts quite a few twists into my brain, as is a "if neither X nor Y then Z", and I find the code comments outright confusing.

Quoted 1 line

I'll implement these comments in the next revision for sure.

I'd like to suggest taking a step back and reflecting whether _you_ like the suggested version better. It is just a suggestion, after all, and if it was up to me, I would argue against it.

Quoted 7 lines

Quoted 3 lines
Also, please add a test to ensure that this behavior doesn't regress in the future. You can probably test it via the "git worktree list" command, so perhaps add the test to t/t2402-worktree-list.sh.
There already exists tests in "t/t2402-worktree-list.sh" which lists and verifies all worktrees. Does this make sense to write a new test that also does kinda the same thing?

The scenario in which we found the buggy behavior involved calling find_shared_symref(). I imagine that we could use git branch -D inside the .git directory for the new regression test.

But yes, in my testing, git worktree list and git -C .git worktree list do show a different top-level directory (the latter shows an incorrect one). Such a test case would find a splendid home in t2402.

Ciao,
Dscho

Eric Sunshine

2020-02-24 22:27 UTC #25 lore raw

Re: [PATCH v3 1/3] get_main_worktree(): allow it to be called in the Git directory

Quoted 5 lines · Johannes Schindelin

Quoted 5 lines · Eric Sunshine
Quoted 4 lines
This change makes the code unnecessarily confusing and effectively turns the final line into dead code. I would much rather see the three cases spelled out explicitly, perhaps like this:

Quoted 8 lines

Quoted 3 lines
Quoted 3 lines
    if (!strbuf_strip_suffix(&worktree_path, "/.git/.") && /* in .git dir */
        !strbuf_strip_suffix(&worktree_path, "/.git/")) /* in worktree */
            strbuf_strip_suffix(&worktree_path, "/."); /* in bare repo */
I would be really cautious about that.
To me, the originally proposed change says: strip /., if any. Then, strip /.git, and if successful, strip another /., if any.

That's not at all what the original said, which is reproduced here:

    if (!strbuf_strip_suffix(&worktree_path, "/.git"))
        strbuf_strip_suffix(&worktree_path, "/.");

It says "try stripping '/.git'; if that fails, try stripping '/.'". That is, it recognizes and handles two distinct cases: (1) the path to the .git directory of a non-bare repository, which always ends with "/.git", and (2) the path to a bare git repository, which always ends with "/.". So, the original code wasn't doing any sort of incremental stripping of suffixes; it was just handling two known distinct cases.

Perhaps you missed the '!' in the conditional?

Quoted 1 line

That reads pretty fine to me. It makes sense.

To me, it doesn't make sense to update the code as done by the patch since that just muddies the issue by making it seem as if get_git_common_dir() is indeterminately tacking on various suffixes rather than giving us deterministic results.

Quoted 3 lines

Above-mentioned proposal, however, puts quite a few twists into my brain, as is a "if neither X nor Y then Z", and I find the code comments outright confusing.

It's just three distinct cases my proposed code is handling; there are no twists.

Quoted 7 lines

The scenario in which we found the buggy behavior involved calling find_shared_symref(). I imagine that we could use git branch -D inside the .git directory for the new regression test.
But yes, in my testing, git worktree list and git -C .git worktree list do show a different top-level directory (the latter shows an incorrect one). Such a test case would find a splendid home in t2402.

I don't have strong feelings about how it is tested, but would like to see some sort of test proving that it works as expected following this change.

Johannes Schindelin

2020-02-27 15:59 UTC #26 lore raw

Re: [PATCH v3 1/3] get_main_worktree(): allow it to be called in the Git directory

Hi Eric,

Quoted 21 lines · Eric Sunshine

Quoted 5 lines · Johannes Schindelin
Quoted 5 lines · Eric Sunshine
Quoted 4 lines
This change makes the code unnecessarily confusing and effectively turns the final line into dead code. I would much rather see the three cases spelled out explicitly, perhaps like this:
Quoted 8 lines
Quoted 3 lines
Quoted 3 lines
    if (!strbuf_strip_suffix(&worktree_path, "/.git/.") && /* in .git dir */
        !strbuf_strip_suffix(&worktree_path, "/.git/")) /* in worktree */
            strbuf_strip_suffix(&worktree_path, "/."); /* in bare repo */
I would be really cautious about that.
To me, the originally proposed change says: strip /., if any. Then, strip /.git, and if successful, strip another /., if any.
That's not at all what the original said, which is reproduced here:
    if (!strbuf_strip_suffix(&worktree_path, "/.git"))
        strbuf_strip_suffix(&worktree_path, "/.");

Quoted 8 lines

It says "try stripping '/.git'; if that fails, try stripping '/.'". That is, it recognizes and handles two distinct cases: (1) the path to the .git directory of a non-bare repository, which always ends with "/.git", and (2) the path to a bare git repository, which always ends with "/.". So, the original code wasn't doing any sort of incremental stripping of suffixes; it was just handling two known distinct cases.
Perhaps you missed the '!' in the conditional?

I totally did. Sorry!

Ciao,
Dscho

Junio C Hamano

2020-02-24 19:13 UTC #27 lore raw

Re: [PATCH v3 1/3] get_main_worktree(): allow it to be called in the Git directory

In reply to Hariom verma

Quoted 25 lines · Hariom verma

Hi Eric,
Quoted 16 lines · Eric Sunshine
This title is a bit too generic; it fails to explain what this patch is really fixing. Perhaps:
get_main_worktree: correctly normalize worktree path when in .git dir
or something.
"Git directory" is imprecise. As a reader, I can't tell if this means the main worktree into which the project is checked out or the .git directory itself. Please write it instead as ".git directory". [...]
This change makes the code unnecessarily confusing and effectively turns the final line into dead code. I would much rather see the three cases spelled out explicitly, perhaps like this:
Quoted 3 lines
    if (!strbuf_strip_suffix(&worktree_path, "/.git/.") && /* in .git dir */
        !strbuf_strip_suffix(&worktree_path, "/.git/")) /* in worktree */
            strbuf_strip_suffix(&worktree_path, "/."); /* in bare repo */
I'll implement these comments in the next revision for sure.

Quoted 7 lines

Quoted 3 lines
Also, please add a test to ensure that this behavior doesn't regress in the future. You can probably test it via the "git worktree list" command, so perhaps add the test to t/t2402-worktree-list.sh.
There already exists tests in "t/t2402-worktree-list.sh" which lists and verifies all worktrees. Does this make sense to write a new test that also does kinda the same thing?

I'd read Eric's suggestion as "please make sure we have a test to ensure...". If there already are tests that protects the behaviour we care about here, there is no need to duplicate it.

Thanks for working on this topic.

Hariom Verma via GitGitGadget

author

2020-02-23 18:57 UTC #28 lore raw

PATCH v3 2/3 t5509: use a bare repository for test push target

From: Hariom Verma <[email protected]>

---
 t/t5509-fetch-push-namespaces.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/t5509-fetch-push-namespaces.sh +1 -1

index 75cbfcc392c..e3975bd21de 100755
@@ -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
 '

Signature · 1 line

gitgitgadget

Hariom Verma via GitGitGadget

author

2020-02-23 18:57 UTC #29 lore raw

PATCH v3 3/3 receive.denyCurrentBranch: respect all worktrees

From: Hariom Verma <[email protected]>

However, this setting was forgotten when the git worktree command was
introduced: only the main worktree's current branch is respected.

With this change, all worktrees are respected.