receive.denyCurrentBranch: respect all worktrees

[dscho]

Currently, the receive.denyCurrentBranch config setting determines what Git should do when pushing a branch into a non-bare repository when said branch is checked out.

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

For interested first contributors: use git grep -i receive.denycurrentbranch and follow the trail to find the code locations where that config setting modifies behavior. Then look for the code that prevents branches to be checked out in a worktree when they are already checked out in another worktree (git grep "is already checked out at" will lead you there). Then imitate that, to address this issue. (And throw in a test case or two, imitating the test cases in t/ that already test that config setting.)

[sam09]

Can I pick this up? First time contributor here, so not sure what's the correct way to get is assigned to me.

[dscho]

@sam09 sure, go ahead! ;-) (In this project, nobody assigns projects to contributors...)

[harry-hov]

I was able to figure out that the receive.denyCurrentBranch config setting modifies the behavior here: https://github.com/gitgitgadget/git/blob/master/builtin/receive-pack.c#L173

the parse_deny_action() function assigns the variable deny_current_branch with one of the four values i.e. ignore, warn, refuse, updateinstead.

And this code snippet in branch.c prevents branch to be checked out when they are already checked out in another working tree. i.e here:

void die_if_checked_out(const char *branch, int ignore_current_worktree)
{
	const struct worktree *wt;

	wt = find_shared_symref("HEAD", branch);
	if (!wt || (ignore_current_worktree && wt->is_current))
		return;
	skip_prefix(branch, "refs/heads/", &branch);
	die(_("'%s' is already checked out at '%s'"),
	    branch, wt->path);
}

But still, I was not able to figure out where to imitate that logic (maybe in worktree.c or builtin/worktree.c or somewhere else) and how.

I would also like to know what is the difference between the command implemented in /builtin and in root directory.

[dscho]

But still, I was not able to figure out where to imitate that logic (maybe in worktree.c or builtin/worktree.c or somewhere else) and how.

The code that is supposed to check whether the branch is checked out is here: (you could find that by using git grep -C5 deny_current_branch):

git/builtin/receive-pack.c

Line 1040 in 2323784

if (is_ref_checked_out(namespaced_name)) {

.

A more tricky thing, however, is the updateInstead value of receive.denyCurrentBranch. For this, the is_ref_checked_out() function will need to learn to accept and fill a parameter of type const struct worktree ** which the code that updates the worktree will have to learn to respect.

Meaning: update_worktree() will need to accept a const struct worktree *worktree parameter and prefer worktree->path over its work_tree and get_worktree_git_dir(worktree) over its own idea what GIT_DIR should be like.

Note that get_worktree_git_dir(worktree) might be relative to worktree->path, I have not checked that. If that is the case, I think you will need to call one of the functions defined in path.c to make it either relative to the current directory or absolute.

But still, I was not able to figure out where to imitate that logic (maybe in worktree.c or builtin/worktree.c or somewhere else) and how.

I would also like to know what is the difference between the command implemented in /builtin and in root directory.

The git worktree command is implemented in builtin/worktree.c. The file worktree.c contains worktree-related functions that are useful to other commands, too, such as git branch or -- you guessed it -- git receive-pack.

[dscho]

Oh yeah, and you will want to implement a test case or two before starting to patch the code.

[harry-hov]

A more tricky thing, however, is the updateInstead value of receive.denyCurrentBranch. For this, the is_ref_checked_out() function will need to learn to accept and fill a parameter of type const struct worktree ** which the code that updates the worktree will have to learn to respect.

assign worktree type parameter with find_shared_symref();
correct ?

Oh yeah, and you will want to implement a test case or two before starting to patch the code.

I am also having trouble with writing test cases. I would like to have reference regarding that.

[dscho]

A more tricky thing, however, is the updateInstead value of receive.denyCurrentBranch. For this, the is_ref_checked_out() function will need to learn to accept and fill a parameter of type const struct worktree ** which the code that updates the worktree will have to learn to respect.

assign worktree type parameter with find_shared_symref();
correct ?

That should work, yes.

Oh yeah, and you will want to implement a test case or two before starting to patch the code.

I am also having trouble with writing test cases. I would like to have reference regarding that.

There should already be a test case about the denyCurrentBranch = updateInstead setting. Why don't you git grep your way to find it, and then read, understand and imitate it?

[harry-hov]

The more I try to sink my teeth deeper into this, the more confused I become.

A more tricky thing, however, is the updateInstead value of receive.denyCurrentBranch. For this, the is_ref_checked_out() function will need to learn to accept and fill a parameter of type const struct worktree ** which the code that updates the worktree will have to learn to respect.

That means is_ref_checked_out() function will need to learn to accept and fill (means assign??) parameter of type const struct worktree **

Meaning: update_worktree() will need to accept a const struct worktree *worktree parameter and prefer worktree->path over its work_tree and get_worktree_git_dir(worktree) over its own idea what GIT_DIR should be like.

update_worktree() will need to accept const struct worktree *worktree ?
Also, what arguments can i pass in find_shared_symref( "HEAD" , ? ); while assigning worktree type parameter.

prefer worktree->path over its work_tree

means this line
const char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : ".."
replaced by

if (worktree->path)
	work_tree = worktree->path;
else if (git_work_tree_cfg)
	work_tree = git_work_tree_cfg;
else
	work_tree = "..";

get_worktree_git_dir(worktree) over its own idea what GIT_DIR should be like.

argv_array_pushf(&env, "GIT_DIR=%s", absolute_path(get_git_dir()));

argv_array_pushf(&env, "GIT_DIR=%s", absolute_path(get_worktree_git_dir(worktree)?get_worktree_git_dir(worktree):get_git_dir()));

[harry-hov]

Can I open draft PR regarding this? So my work can be reviewed and corrected more easily.

[dscho]

Can I open draft PR regarding this? So my work can be reviewed and corrected more easily.

Of course!

[harry-hov]

[@dscho] I guess, this issue must be closed by now.

[dscho]

I guess so! Well done!