GIT updates working tree with receive.denyCurrentBranch=updateInstead even for rejected pushes (receive.denyNonFastForwards=true)

Question

I posted this question/bug to the GIT official channel but got no response. Hoping someone here can help me.

The below behavior seems incorrect to me when receive.denyCurrentBranch is set to updateInstead and receive.denyNonFastForwards is set to true. Below are the steps to reproduce the scenario.

Step 1 - Setup remote repository (remote host):

git config --global receive.denyCurrentBranch updateInstead
git config --global receive.denyNonFastForwards true
mkdir /tmp/hello
cd /tmp/hello
git init
echo hello > hello.txt
git add . && git commit -m "hello.txt"

Step 2 - Create 2 Clones (local host):

git clone ssh://REMOTEIP/tmp/hello /tmp/hello1
git clone ssh://REMOTEIP/tmp/hello /tmp/hello2

Step 3 - Push a commit from Clone 1

cd /tmp/hello1
echo hello1 > hello1.txt
git add . && git commit -m "hello1.txt"
git push

At this point server working tree contains hello1.txt which is expected

Step 4: Try to force push a commit from Clone 2

cd /tmp/hello2
echo hello2 > hello2.txt
git add . && git commit -m "hello2.txt"
git push

Remote rejects with message that remote contains work i do not have locally which is valid. Now I force the push.

git push --force

Remote rejects again with error: denying non-fast-forward refs/heads/master (you should pull first)

At this point, since the push is rejected, I expect the servers working tree to not contain any rejected changes. BUT the servers working tree got updated to delete hello1.txt and create hello2.txt. Push rejected but not really.

I also noticed the same behavior (incorrect) when the update hook rejects changes on the server (but not the pre-receive hook).

Answer 1

The problem is that these two configurations are in conflict (though I agree that in principle they shouldn't be):

receive.denyCurrentBranch is set to updateInstead

In this case, during the receive, Git notices that the target is the current branch, so Git checks out the commit.

and receive.denyNonFastForwards is set to true.

This happens separately: the change to the name is rejected. The commit was already taken, and the repository checkout happened, and now the change to the name is rejected.

I also noticed the same behavior (incorrect) when the update hook rejects changes on the server (but not the pre-receive hook).

This is the same issue: the pre-receive hook runs once, before any individual reference updates, and can reject the entire push. Then, if the pre-receive hook has cleared things to proceed, the update hooks can reject any individual ref-name update. However, updateInstead happens separately from the branch name update being accepted or rejected.

It would probably be better for Git to internally avoid the working-tree change until after vetting the reference update. This would require some rework in Git's internals. If this isn't considered an outright bug in Git, it's at least quite surprising. In fact, all of this code needs some work anyway because if you use git worktree add, Git fails to consider the added worktrees' HEADs as current branches. Given all of these caveats, I'd recommend using only bare repositories as push targets, with post-receive hooks to direct updates in other repositories or work-trees.

---

Aside (this was a comment, but that was too short to express it very well):

git config --global receive.denyCurrentBranch updateInstead
git config --global receive.denyNonFastForwards true

Although this is not tied to the specific behavior you've observed, this is definitely the wrong thing to do. Running git config --global sets configuration items for you, personally. That is, these go into /home/rajesh (or wherever your home directory configuration resides). But receive.* settings should be per-repository.

Since you are using ssh as yourself to do the push, these configuration parameters did take effect—but if you were ever to push via some other method, they might not. Any per-repository settings, made with git config without --global, would still take effect.

Answer 2

I expect the servers working tree to not contain any rejected changes. BUT the servers working tree got updated to delete hello1.txt and create hello2.txt.
Push rejected but not really.

From Git 2.20 onwards (Q4 2018), the push will really be rejected.

Before, the receive.denyCurrentBranch=updateInstead codepath kicked in even when the push should have been rejected due to other reasons, such as it does not fast-forward or the update-hook rejects it, which has been corrected.

See commit b072a25 (19 Oct 2018) by Junio C Hamano (gitster).
^{(Merged by Junio C Hamano -- gitster -- in commit 4c7f544, 30 Oct 2018)}

receive: denyCurrentBranch=updateinstead should not blindly update

The handling of receive.denyCurrentBranch=updateInstead was added to a switch statement that handles other values of the variable, but all the other case arms only checked a condition to reject the attempted push, or let later logic in the same function to still intervene, so that a push that does not fast-forward (which is checked after the switch statement in question) is still rejected.

But the handling of updateInstead incorrectly took immediate effect, without giving other checks a chance to intervene.

Instead of calling update_worktree() that causes the side effect immediately, just note the fact that we will need to call the function later, and first give other checks a chance to reject the request.
After the update-hook gets a chance to reject the push (which happens as the last step in a series of checks), call update_worktree() when we earlier detected the need to.

In your case, your working tree will not contain any rejected change, as expected, considering your push was rejected.

---

Torek added:

In fact, all of this code needs some work anyway because if you use git worktree add, Git fails to consider the added worktrees' HEADs as current branches

Well no more.

With Git 2.26 (Q1 2020), "git push" should stop from updating a branch that is checked out when receive.denyCurrentBranch configuration is set, but it failed to pay attention to checkouts in secondary worktrees.

This has been corrected.

See commit 4d86489 (04 Mar 2020), and commit 4ef3464, commit f869211, commit 45f274f (23 Feb 2020) by Hariom Verma (harry-hov).
^{(Merged by Junio C Hamano -- gitster -- in commit 4a2e91d, 05 Mar 2020)}

receive.denyCurrentBranch: respect all worktrees

^{Helped-by: Johannes Schindelin
Signed-off-by: Hariom Verma}

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 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 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 even while using ref namespaces.