[GSoC] receive.denyCurrentBranch: respect all worktrees #535

harry-hov · 2020-01-21T21:40:34Z

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

When receive.denyCurrentBranch is set to updateInstead, a push that
tries to update the branch that is currently checked out is accepted only
when the index and the working tree exactly matches the currently
checked out commit, in which case the index and the working tree are
updated to match the pushed commit. Otherwise, the push is refused.

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

dscho

Did you get a chance to investigate why t5516.93 receive.denyCurrentBranch = updateInstead and t5516.94 updateInstead with push-to-checkout hook fail?

builtin/receive-pack.c

dscho · 2020-01-22T18:30:02Z

In this comment, I had also suggested to look for a test case to imitate. I see that you did not do that yet. So let me guide you a little more closely here:

$ git grep -i denycurrentbranch upstream/master -- t/
upstream/master:t/t5400-send-pack.sh:   ( cd victim && git config receive.denyCurrentBranch warn && git log ) &&
upstream/master:t/t5400-send-pack.sh:       git config receive.denyCurrentBranch warn &&
upstream/master:t/t5405-send-pack-rewind.sh:    git config receive.denyCurrentBranch warn &&
upstream/master:t/t5507-remote-environment.sh:# due to receive.denyCurrentBranch=true
upstream/master:t/t5507-remote-environment.sh:  test_must_fail git -c receive.denyCurrentBranch=false push remote
upstream/master:t/t5507-remote-environment.sh:  test_must_fail git -c receive.denyCurrentBranch=false push host:remote
upstream/master:t/t5516-fetch-push.sh:          git config receive.denyCurrentBranch warn &&
upstream/master:t/t5516-fetch-push.sh:          git config receive.denyCurrentBranch warn
upstream/master:t/t5516-fetch-push.sh:          git config receive.denyCurrentBranch true
upstream/master:t/t5516-fetch-push.sh:          git config receive.denyCurrentBranch true &&
upstream/master:t/t5516-fetch-push.sh:          git config receive.denyCurrentBranch false
upstream/master:t/t5516-fetch-push.sh:          git config receive.denyCurrentBranch false
upstream/master:t/t5516-fetch-push.sh:          git config receive.denyCurrentBranch false
upstream/master:t/t5516-fetch-push.sh:          git config receive.denyCurrentBranch true) &&
upstream/master:t/t5516-fetch-push.sh:          git config receive.denyCurrentBranch true) &&
upstream/master:t/t5516-fetch-push.sh:test_expect_success 'receive.denyCurrentBranch = updateInstead' '
upstream/master:t/t5516-fetch-push.sh:          git config receive.denyCurrentBranch updateInstead
upstream/master:t/t5516-fetch-push.sh:          git config receive.denyCurrentBranch updateInstead
upstream/master:t/t5516-fetch-push.sh:          git config receive.denyCurrentBranch updateInstead &&
upstream/master:t/t5516-fetch-push.sh:          git config receive.denyCurrentBranch updateInstead &&
upstream/master:t/t5517-push-mirror.sh:         git config receive.denyCurrentBranch warn
upstream/master:t/t5522-pull-symlink.sh:                git config receive.denyCurrentBranch warn
upstream/master:t/t5522-pull-symlink.sh:        git config receive.denyCurrentBranch warn
upstream/master:t/t5543-atomic-push.sh:         git config receive.denyCurrentBranch warn
upstream/master:t/t5545-push-options.sh:                git config receive.denyCurrentBranch warn &&
upstream/master:t/t5605-clone-local.sh:  git config receive.denyCurrentBranch warn) &&

There are a lot of hits, and the updateInstead hits are the ones you are definitely interested in. In particular this one. It is very long, and you definitely do not have to repeat it entirely.

However, what you want to do is to initialize a worktree with a new branch, then clone into a separate worktree (probably with git clone --single-branch --branch <that-worktree-branch . deny-current-worktree-branch), then commit one change and try to push with receive.denyCurrentBranch = refuse on the receiving side (i.e. not in the clone), expecting it to fail (maybe find something in the output to test_i18ngrep for), then change to receive.denyCurrentBranch = updateInstead and verify that it succeeds, and then also verify that the worktree received the update.

harry-hov · 2020-01-22T19:05:34Z

let me guide you a little more closely

I fricking appreciate it...

However, what you want to do is to initialize a worktree with a new branch, then clone into a separate worktree (probably with git clone --single-branch --branch <that-worktree-branch . deny-current-worktree-branch), then commit one change and try to push with receive.denyCurrentBranch = refuse on the receiving side (i.e. not in the clone), expecting it to fail (maybe find something in the output to test_i18ngrep for), then change to receive.denyCurrentBranch = updateInstead and verify that it succeeds, and then also verify that the worktree received the update.

I'm on it, senpai !!!

harry-hov · 2020-01-23T20:43:51Z

There are a lot of hits, and the updateInstead hits are the ones you are definitely interested in. In particular this one. It is very long, and you definitely do not have to repeat it entirely.

I need to edit this one??
or do I need to write a new one (say, test_#95)

Also, is there any way to check why my test failed (error message or something).

dscho · 2020-01-24T09:45:10Z

There are a lot of hits, and the updateInstead hits are the ones you are definitely interested in. In particular this one. It is very long, and you definitely do not have to repeat it entirely.

I need to edit this one??
or do I need to write a new one (say, test_#95)

Copy-edit it, or maybe just imitate it and add a new one. Like,

initialize a worktree with a new branch, then
clone into a separate worktree (probably with git clone --single-branch --branch <that-worktree-branch . deny-current-worktree-branch), then
commit one change and
try to push with receive.denyCurrentBranch = refuse on the receiving side (i.e. not in the clone), expecting it to fail
(maybe find something in the output to test_i18ngrep for), then
change to receive.denyCurrentBranch = updateInstead and
verify that it succeeds, and then also
verify that the worktree received the update.

dscho · 2020-01-24T09:46:28Z

I need to edit this one??

BTW that one seems to fail with your patch. Have you analyzed the difference in the output of -i -v -x between a run with your patch applied vs without your patch applied?

harry-hov · 2020-01-24T21:37:30Z

I need to edit this one??

BTW that one seems to fail with your patch. Have you analyzed the difference in the output of -i -v -x between a run with your patch applied vs without your patch applied?

Yep, this is the output after patch applied.

+ git push testrepo master
Enumerating objects: 5, done.
Counting objects: 100% (5/5), done.
Delta compression using up to 8 threads
Compressing objects: 100% (2/2), done.
Writing objects: 100% (3/3), 274 bytes | 274.00 KiB/s, done.
Total 3 (delta 0), reused 0 (delta 0)
To testrepo
 ! [remote rejected] master -> master (Working directory has unstaged changes)
error: failed to push some refs to 'testrepo'

During execution of this command
Does it had something to do with non-bare repo ?

dscho · 2020-01-24T23:24:12Z

Does it had something to do with non-bare repo ?

I don't think so. It tries to push into testrepo which is initialized as a non-bare repository even before your patch.

If I were you, I would instrument the code both before and after the patch to show a bit more, e.g. inserting error("work_tree: %s", work_tree); into update_worktree() (I am lazy and using error() instead of fprintf(stderr, ...) here because it is shorter to type), and possibly also showing the git dir that is used. There is probably an unintended difference in those values.

harry-hov · 2020-01-25T09:36:23Z

If I were you, I would instrument the code both before and after the patch to show a bit more, e.g. inserting error("work_tree: %s", work_tree); into update_worktree() (I am lazy and using error() instead of fprintf(stderr, ...) here because it is shorter to type), and possibly also showing the git dir that is used. There is probably an unintended difference in those values.

Before the patch applied:

error: work_tree: ..
error: git_dir: .

After the patch applied:

error: work_tree: /home/harry/Desktop/git/t/trash directory.t5516-fetch-push/testrepo/.git
error: git_dir: .

Still, I'm unable to figure the things out.
Researching more on worktrees. smh

dscho · 2020-01-26T06:43:50Z

Sounds like the worktree path is wrong: if . is the git dir, then that absolute path sure looks like the same, not like the work tree.

This looks like it might be a bug in worktree.c or setup.c, uncovered by your patch.

builtin/receive-pack.c

harry-hov · 2020-01-26T17:12:59Z

Sounds like the worktree path is wrong: if . is the git dir, then that absolute path sure looks like the same, not like the work tree.

This looks like it might be a bug in worktree.c or setup.c, uncovered by your patch.

How should I proceed further?

dscho · 2020-01-26T21:37:18Z

This looks like it might be a bug in worktree.c or setup.c, uncovered by your patch.

How should I proceed further?

Investigate why the Git directory is mistaken for a worktree?

worktree.c

harry-hov · 2020-01-28T13:20:17Z

initialize a worktree with a new branch, then

clone into a separate worktree (probably with git clone --single-branch --branch <that-worktree-branch . deny-current-worktree-branch), then

commit one change and

try to push with receive.denyCurrentBranch = refuse on the receiving side (i.e. not in the clone), expecting it to fail

(maybe find something in the output to test_i18ngrep for), then

change to receive.denyCurrentBranch = updateInstead and

verify that it succeeds, and then also

verify that the worktree received the update.

So, now I'm left with test ?

dscho · 2020-01-28T23:12:51Z

Woot, the PR build passed!

initialize a worktree with a new branch, then

clone into a separate worktree (probably with git clone --single-branch --branch <that-worktree-branch . deny-current-worktree-branch), then

commit one change and

try to push with receive.denyCurrentBranch = refuse on the receiving side (i.e. not in the clone), expecting it to fail

(maybe find something in the output to test_i18ngrep for), then

change to receive.denyCurrentBranch = updateInstead and

verify that it succeeds, and then also

verify that the worktree received the update.

So, now I'm left with test ?

Yep. This is roughly how it should start:

test_expect_success 'denyCurrentBranch and worktrees' '
    git worktree add -b new-wt &&
    git clone . cloned &&
[...]

(Those lines already implement the first two bullet points, hopefully illustrating the idea.)

worktree.c

harry-hov · 2020-01-29T21:32:15Z

I'm really having a hard time while writing test for this.

test_expect_success 'denyCurrentBranch and worktrees' '
    git worktree add new-wt &&
    git clone . cloned &&
    test_commit first &&
    cd new-wt &&
    git config receive.denyCurrentBranch refuse &&
    cd .. &&
    test_must_fail git push cloned new-wt &&
    git config receive.denyCurrentBranch updateInstead &&
    git push cloned new-wt
'

I tried many other ways, but still unable to figure things out. where did I go wrong?

dscho · 2020-01-30T09:33:12Z

test_expect_success 'denyCurrentBranch and worktrees' '
git worktree add new-wt &&
git clone . cloned &&
test_commit first &&
cd new-wt &&
git config receive.denyCurrentBranch refuse &&
cd .. &&
test_must_fail git push cloned new-wt &&
git config receive.denyCurrentBranch updateInstead &&
git push cloned new-wt
'

That's a pretty good start already! Only a couple changes are necessary:

You will want to create the commit in the cloned repository, i.e. test_commit -C cloned first.
We try to avoid cd .. and prefer subshells, but in this case it is not even necessary, as the config is shared between the main worktree and the new-wt one.
When pushing, you will want to make sure that the just-committed first is pushed to that worktree branch (currently you push the worktree branch into the cloned repository, which is the opposite direction), i.e. git -C cloned push origin HEAD:new-wt.

dscho · 2020-01-30T09:33:43Z

Oh, also: you will want to use test_config instead of git config (the former will automatically unset the config variable after the test case).

harry-hov · 2020-01-30T13:14:24Z

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
'

push is still passing even after pushing with receive.denyCurrentBranch = refuse

Here are the logs:

+ git config receive.denyCurrentBranch refuse
+ test_must_fail git -C cloned push origin HEAD:new-wt
+ _test_ok=
+ git -C cloned push origin HEAD:new-wt
Enumerating objects: 4, done.
Counting objects: 100% (4/4), done.
Delta compression using up to 8 threads
Compressing objects: 100% (2/2), done.
Writing objects: 100% (3/3), 330 bytes | 330.00 KiB/s, done.
Total 3 (delta 0), reused 0 (delta 0)
To /home/harry/Desktop/git/t/trash directory.t5516-fetch-push/.
   99d9406..fc16dd4  HEAD -> new-wt
+ exit_code=0
+ test 0 -eq 0
+ list_contains  success
+ return 1
+ echo test_must_fail: command succeeded: git -C cloned push origin HEAD:new-wt
test_must_fail: command succeeded: git -C cloned push origin HEAD:new-wt
+ return 1
error: last command exited with $?=1
not ok 95 - denyCurrentBranch and worktrees

dscho · 2020-01-30T20:16:56Z

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
'

push is still passing even after pushing with receive.denyCurrentBranch = refuse

Good thing we have that regression test to catch this, eh?

The next step is to instrument the code (i.e. to insert debug print statements) to figure out what is going wrong, or maybe to debug interactively.

I fear that we cannot really debug interactively in this instance because the push process will spawn the receive-pack process that we would have to start in the debugger. So print statements it is.

dscho · 2020-01-30T20:35:28Z

The work-in-progress diff would look somewhat like this:

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 3f8b4e5c904..eb473905270 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1003,12 +1003,14 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w
 	const char *retval, *work_tree, *git_dir = NULL;
 	struct argv_array env = ARGV_ARRAY_INIT;
 
+error("worktree: %p (path: %s)", worktree, worktree ? worktree->path : "(none)");
 	if (worktree && worktree->path)
 		work_tree = worktree->path;
 	else if (git_work_tree_cfg)
 		work_tree = git_work_tree_cfg;
 	else
 		work_tree = "..";
+error("work_tree: %s", work_tree);
 
 	if (is_bare_repository())
 		return "denyCurrentBranch = updateInstead needs a worktree";
@@ -1017,6 +1019,7 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w
 		git_dir = get_worktree_git_dir(worktree);
 	if (!git_dir)
 		git_dir = get_git_dir();
+error("git_dir: %s", git_dir);
 
 	argv_array_pushf(&env, "GIT_DIR=%s", absolute_path(git_dir));
 
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index c81ca360ac4..2642212d330 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -130,6 +130,16 @@ test_expect_success setup '
 
 '
 
+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_expect_success 'fetch without wildcard' '
 	mk_empty testrepo &&
 	(

And to accelerate the edit/build/run loop, I would run t5516 once, then run only the failing command (after resetting the new-wt branch):

git -C ./trash\ directory.t5516-fetch-push/new-wt/ reset --hard master &&
../git --exec-path=$PWD/.. -C ./trash\ directory.t5516-fetch-push/cloned/ push origin HEAD:new-wt

I use multiple tricks here to make this (manual) loop more efficient:

I use a && to combine the commands, so that I do not accidentally run the second command if the first one fails
In the second command, I use the just-built git in-place: ../git refers to the executable (I am still in t/ because I just ran t5516-fetch-push.sh -i -v -x -d)
I tell Git to use the just-compiled git when spawning other Git processeses, via the --exec-path=$PWD/.. option
I use the -C <directory> option to avoid having to cd around.
You will note that I use error() to display the debug information. That's pure laziness on my part, as none of them are errors, but error( is just quicker to write than fprintf(stderr, .

To make things a bit more convenient yet, I can prefix those two commands with make -j8 -C .. && to make sure that I re-build Git whenever I modified the source code (e.g. adding yet another debug statement).

However, my first attempt to actually run this looks a bit funny because none of the stuff is actually printed...

dscho · 2020-01-30T21:00:12Z

However, my first attempt to actually run this looks a bit funny because none of the stuff is actually printed...

Aha!

I added two more debug print statements, one in cmd_receive_pack() to verify that I can actually see the output from error() (I can, verified by one more manual edit/build/run), and another one two lines before the switch (deny_current_branch) line:

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 3f8b4e5c904..e31c424b6af 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1003,12 +1003,14 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w
 	const char *retval, *work_tree, *git_dir = NULL;
 	struct argv_array env = ARGV_ARRAY_INIT;
 
+error("worktree: %p (path: %s)", worktree, worktree ? worktree->path : "(none)");
 	if (worktree && worktree->path)
 		work_tree = worktree->path;
 	else if (git_work_tree_cfg)
 		work_tree = git_work_tree_cfg;
 	else
 		work_tree = "..";
+error("work_tree: %s", work_tree);
 
 	if (is_bare_repository())
 		return "denyCurrentBranch = updateInstead needs a worktree";
@@ -1017,6 +1019,7 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w
 		git_dir = get_worktree_git_dir(worktree);
 	if (!git_dir)
 		git_dir = get_git_dir();
+error("git_dir: %s", git_dir);
 
 	argv_array_pushf(&env, "GIT_DIR=%s", absolute_path(git_dir));
 
@@ -1049,6 +1052,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 	free(namespaced_name);
 	namespaced_name = strbuf_detach(&namespaced_name_buf, NULL);
 
+error("%s:%d HERE: is_ref_checked_out says %d", __FILE__, __LINE__, is_ref_checked_out(namespaced_name));
 	if (is_ref_checked_out(namespaced_name)) {
 		switch (deny_current_branch) {
 		case DENY_IGNORE:
@@ -1952,6 +1956,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+error("%s:%d: HERE", __FILE__, __LINE__);
 	packet_trace_identity("receive-pack");
 
 	argc = parse_options(argc, argv, prefix, options, receive_pack_usage, 0);

And I do see this in the output:

error: builtin/receive-pack.c:1055 HERE: is_ref_checked_out says 0

Which means that receive-pack does not get into the code path you modified at all! Which also means that we're doing this at the point.

Instead of asking is_ref_checked_out(), we should find the worktree already at the start of the update() function, and replace that if (is_ref_checked_out(namespaced_name)) by if (worktree).

Of course, this was the only caller of is_ref_checked_out(), so that function can go, too. When I look at that function, I see that I overlooked something crucial, too: you call find_share_symref() always, but that should not be done in a bare repository. So I would propose something like this:

        worktree = is_bare_repository() ? NULL : find_shared_symref("HEAD", name);

I also cannot fail to notice that the find_shared_symref() function works on the name while the is_ref_checked_out() parameter was namespaced_name. Maybe we should use that as parameter to find_shared_symref(), too.

Finally, I looked around whether namespaced_name is used anywhere else in that update() function, and it totally is: in the part that wants to prevent a checked-out branch (or the current branch in a bare repository) from being deleted via git push --delete <remote> <branch-name>. I think we will need to adjust this one, too, e.g. via

-                if (head_name && !strcmp(namespaced_name, head_name)) {
+                if (worktree || (head_name && !strcmp(namespaced_name, head_name))) {

and then we definitely want to test for that, too, e.g. via

test_must_fail git -C cloned push --delete origin new-wt

gitgitgadget · 2020-02-25T18:51:27Z

This patch series was integrated into pu via git@ced34bb.

harry-hov · 2020-02-25T19:45:01Z

git init temp &&
(
	cd temp &&
	test_commit first &&
	git worktree add ../new-wt &&
	git worktree list &&
	git -C .git worktree list
)

Output

With current changes

/*git worktree list*/
/home/harry/Desktop/git/t/trash directory.t2402-worktree-list/temp    1e96f59 [master]
/home/harry/Desktop/git/t/trash directory.t2402-worktree-list/new-wt  1e96f59 [new-wt]

/*git -C .git worktree list*/
/home/harry/Desktop/git/t/trash directory.t2402-worktree-list/temp    1e96f59 [master]
/home/harry/Desktop/git/t/trash directory.t2402-worktree-list/new-wt  1e96f59 [new-wt]

Without this patch

/*git worktree list*/
/home/harry/Desktop/git/t/trash directory.t2402-worktree-list/temp    1e96f59 [master]
/home/harry/Desktop/git/t/trash directory.t2402-worktree-list/new-wt  1e96f59 [new-wt]

/*git -C .git worktree list*/
/home/harry/Desktop/git/t/trash directory.t2402-worktree-list/temp/.git  1e96f59 [master]
/home/harry/Desktop/git/t/trash directory.t2402-worktree-list/new-wt     1e96f59 [new-wt]

harry-hov · 2020-02-25T19:49:33Z

All I need to do in this test is to compare output of git worktree list and git -C .git worktree list?

harry-hov · 2020-02-25T19:55:57Z

What Eric suggested will also produce the same output as current changes.
How one can judge which one is correct?
As both strips extra .git eventually.

gitgitgadget · 2020-02-25T20:09:51Z

This patch series was integrated into pu via git@32e8f7f.

gitgitgadget · 2020-02-25T20:09:53Z

This patch series was integrated into next via git@84e35c4.

dscho · 2020-02-26T20:58:17Z

git init temp &&
(
	cd temp &&
	test_commit first &&
	git worktree add ../new-wt &&
	git worktree list &&
	git -C .git worktree list
)

Would it not be enough to just call git -C .git worktree list >out and then verify with an appropriately crafted grep that the .git suffix is not shown?

gitgitgadget · 2020-02-26T23:13:08Z

This patch series was integrated into pu via git@4c19b1b.

dscho · 2020-02-27T14:39:41Z

Hrm. Since this was already merged to next, I think you will need to add this as an add-on patch. Patches that have been merged to next are not rewritten anymore, unlike pu.

harry-hov · 2020-02-27T20:41:47Z

Would it not be enough to just call git -C .git worktree list >out and then verify with an appropriately crafted grep that the .git suffix is not shown?

got it

Hrm. Since this was already merged to next, I think you will need to add this as an add-on patch. Patches that have been merged to next are not rewritten anymore, unlike pu.

Means opening another PR (based on next) and submit ?

gitgitgadget · 2020-02-28T00:01:31Z

This patch series was integrated into pu via git@ef4d67c.

dscho · 2020-02-28T06:39:49Z

I would target not next, but the branch name Junio gave your patches (GitGitGadget mentioned it earlier, I can't copy-paste it because I'm on a phone right now). But yes, a new PR.

dscho · 2020-02-28T09:14:10Z

I would target not next, but the branch name Junio gave your patches (GitGitGadget mentioned it earlier, I can't copy-paste it because I'm on a phone right now).

(back on a device with a proper keyboard)

What I meant was this comment by the gitgitgadget bot:

This branch is now known as hv/receive-denycurrent-everywhere.

In essence, I would suggest to add the remote git remote add gitgitgadget https://github.com/gitgitgadget/git and then start your branch via git fetch gitgitgadget hv/receive-denycurrent-everywhere && git switch -c verify-get_main_worktree-fix gitgitgadget/hv/receive-denycurrent-everywhere.

harry-hov · 2020-02-28T21:43:37Z

test_expect_success 'worktree path when called in .git directory' '
	git init repo &&
	(
		cd repo &&
		test_commit first &&
		git worktree add new-wt &&
		git -C .git worktree list> out &&
		test_must_fail grep -Fc ".git" out
	)
'

this will do the job ?

dscho · 2020-02-28T23:13:12Z

test_expect_success 'worktree path when called in .git directory' '
	git init repo &&
	(
		cd repo &&
		test_commit first &&
		git worktree add new-wt &&
		git -C .git worktree list> out &&
		test_must_fail grep -Fc ".git" out
	)
'

this will do the job ?

I don't actually think so...

First of all, you do not need to initialize a new repository. That's just wasteful. Just use the one in .

Second, you do not even need a new worktree to reproduce the bug, so why add one? All you need to do is to list the worktrees when the current working directory is inside the gitdir, i.e. git -C .git worktree list >out.

Lastly, grep takes a regular expression. And .git is both too lax (because . matches any character, not just .git) and too restrictive (it is possible to check out Git into the path "/home/harry-hov/my-clone-of-git`, and the "-git" part of this absolute path matches that regular expression.

I guess the best way forward is to verify that git worktree list and git -C .git worktree list print the exact same output.

harry-hov · 2020-02-29T09:47:44Z

this will do the job ?

I don't actually think so...

First of all, you do not need to initialize a new repository. That's just wasteful. Just use the one in .

Second, you do not even need a new worktree to reproduce the bug, so why add one? All you need to do is to list the worktrees when the current working directory is inside the gitdir, i.e. git -C .git worktree list >out.

+1

Lastly, grep takes a regular expression. And .git is both too lax (because . matches any character, not just .git) and too restrictive (it is possible to check out Git into the path "/home/harry-hov/my-clone-of-git`, and the "-git" part of this absolute path matches that regular expression.

I guess that's not the case here. Indeed grep takes regular expression. But -F(--fixed-strings) in grep -Fc ".git" out will take care of that. So it will only match .git not -git

I guess the best way forward is to verify that git worktree list and git -C .git worktree list print the exact same output.

We can go this way too.

dscho · 2020-02-29T14:27:28Z

My bad, missed the -F. Still, it would not only match a trailing /.git in the path, would it?

harry-hov · 2020-02-29T18:02:35Z

I guess the best way forward is to verify that git worktree list and git -C .git worktree list print the exact same output.

test_expect_success 'worktree path when called in .git directory' '
	git worktree list> list1 &&
	git -C .git worktree list> list2 &&
	test_cmp list1 list2
'

that will suffice ? 🤔

dscho · 2020-02-29T21:31:05Z

Yep :-)

harry-hov · 2020-02-29T22:58:07Z

In essence, I would suggest to add the remote git remote add gitgitgadget https://github.com/gitgitgadget/git and then start your branch via git fetch gitgitgadget hv/receive-denycurrent-everywhere && git switch -c verify-get_main_worktree-fix gitgitgadget/hv/receive-denycurrent-everywhere.

I've added test to 3rd last commit.
I don't know how to proceed further 😵

dscho · 2020-03-02T12:53:25Z

I've added test to 3rd last commit.

Your branch is already in next, so you cannot amend anything at this stage. You will need to start a new branch via

git remote add gitgitgadget https://github.com/gitgitgadget/git
git fetch gitgitgadget hv/receive-denycurrent-everywhere
git switch -c verify-get_main_worktree-fix gitgitgadget/hv/receive-denycurrent-everywhere

and then cherry-pick the previous commit via git cherry-pick @{-1}

Technically, you only want the test part of that commit. However, the rest of the commit is already applied in that branch, so the cherry-pick will end up having only the test.

You will then need to amend the commit message to reflect that you're only adding the forgotten test, and then open a new PR.

harry-hov · 2020-03-02T18:01:42Z

Superseded by #570
[I'm not sure whether I did it the right way?]

gitgitgadget · 2020-03-03T02:39:31Z

This patch series was integrated into pu via git@a258e25.

gitgitgadget · 2020-03-05T22:03:40Z

This patch series was integrated into pu via git@4a2e91d.

gitgitgadget · 2020-03-05T22:03:42Z

This patch series was integrated into master via git@4a2e91d.

gitgitgadget · 2020-03-05T22:03:45Z

Closed via 4a2e91d.

dscho requested changes Jan 22, 2020

View reviewed changes

builtin/receive-pack.c Outdated Show resolved Hide resolved

builtin/receive-pack.c Outdated Show resolved Hide resolved

builtin/receive-pack.c Outdated Show resolved Hide resolved

builtin/receive-pack.c Outdated Show resolved Hide resolved

harry-hov force-pushed the deny branch from a0a9c92 to 19827f2 Compare January 22, 2020 18:18

dscho reviewed Jan 22, 2020

View reviewed changes

builtin/receive-pack.c Outdated Show resolved Hide resolved

harry-hov force-pushed the deny branch from 19827f2 to e27fef7 Compare January 22, 2020 18:32

harry-hov commented Jan 26, 2020

View reviewed changes

builtin/receive-pack.c Outdated Show resolved Hide resolved

harry-hov force-pushed the deny branch from dd1af27 to 018c2e7 Compare January 27, 2020 19:32

harry-hov commented Jan 27, 2020

View reviewed changes

worktree.c Outdated Show resolved Hide resolved

harry-hov force-pushed the deny branch from 018c2e7 to d2a1c59 Compare January 28, 2020 13:07

dscho reviewed Jan 28, 2020

View reviewed changes

worktree.c Outdated Show resolved Hide resolved

harry-hov force-pushed the deny branch from d2a1c59 to 3f8b739 Compare January 29, 2020 16:46

gitgitgadget bot added the next label Feb 25, 2020

gitgitgadget bot added the master label Mar 5, 2020

gitgitgadget bot closed this Mar 5, 2020

harry-hov deleted the deny branch March 18, 2020 19:38

[GSoC] receive.denyCurrentBranch: respect all worktrees #535

[GSoC] receive.denyCurrentBranch: respect all worktrees #535

Uh oh!

Conversation

harry-hov commented Jan 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dscho left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dscho commented Jan 22, 2020

Uh oh!

harry-hov commented Jan 22, 2020

Uh oh!

harry-hov commented Jan 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dscho commented Jan 24, 2020

Uh oh!

dscho commented Jan 24, 2020

Uh oh!

harry-hov commented Jan 24, 2020

Uh oh!

dscho commented Jan 24, 2020

Uh oh!

harry-hov commented Jan 25, 2020

Uh oh!

dscho commented Jan 26, 2020

Uh oh!

Uh oh!

harry-hov commented Jan 26, 2020

Uh oh!

dscho commented Jan 26, 2020

Uh oh!

Uh oh!

harry-hov commented Jan 28, 2020

Uh oh!

dscho commented Jan 28, 2020

Uh oh!

Uh oh!

harry-hov commented Jan 29, 2020

Uh oh!

dscho commented Jan 30, 2020

Uh oh!

dscho commented Jan 30, 2020

Uh oh!

harry-hov commented Jan 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dscho commented Jan 30, 2020

Uh oh!

dscho commented Jan 30, 2020

Uh oh!

dscho commented Jan 30, 2020

Uh oh!

gitgitgadget bot commented Feb 25, 2020

Uh oh!

harry-hov commented Feb 25, 2020

Output

Uh oh!

harry-hov commented Feb 25, 2020

Uh oh!

harry-hov commented Feb 25, 2020

Uh oh!

gitgitgadget bot commented Feb 25, 2020

Uh oh!

gitgitgadget bot commented Feb 25, 2020

Uh oh!

dscho commented Feb 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gitgitgadget bot commented Feb 26, 2020

Uh oh!

dscho commented Feb 27, 2020

Uh oh!

harry-hov commented Jan 21, 2020 •

edited

Loading

harry-hov commented Jan 23, 2020 •

edited

Loading

harry-hov commented Jan 30, 2020 •

edited

Loading

dscho commented Feb 26, 2020 •

edited

Loading