Re: [PATCH 1/2] Add a few more values for receive.denyCurrentBranch

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 7, 2014, 18:49 UTC
Message-ID: <xmqqvbmqg81g.fsf@gitster.dls.corp.google.com>
In-Reply-To: <f82aedcb632571d0b756d62c58479c0aab35b026.1415368490.git.johannes.schindelin@gmx.de>

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

Show 5 quoted lines

> Under certain circumstances, it makes a *lot* of sense to allow pushing
> into the current branch. For example, when two machines with different
> Operating Systems are required for testing, it makes much more sense to
> synchronize between working directories than having to go via a third
> server.

Even if you do not have a third server, each time you decide to do such a push, you have a single source of truth, i.e. the repository you are pushing from, so instead of doing that push, all the others could instead pull from that single source of truth. In that sense, even though I wouldn't say it is wrong to use "push" in the opposite direction for this synchronization, I have to say that the above is not a very strong argument. It certainly does not deserve "*lot*" in bold font ;-)

> Under different circumstances, the working directory needs to be left
> untouched, for example when a bunch of VMs need to be shut down to save
> RAM and one needs to push everything out into the host's non-bare
> repositories quickly.

For this use case, if you assume that the tips of branches in the repositories on "a bunch of VMs" could be pointing at different commits (i.e. each of them has acquired more work without coordination), you are risking lossage by pushing into refs/heads/, not refs/remotes/vm$n/, aren't you? When you want to save things away "quickly", you do not want to be worried about a push from VM1 stomping on what was stored from VM0, and using separate remotes, i.e. refs/remotes/vm$n/, has been the recommended way to do so. So this is not a very strong argument, either.

Doing things only within refs/heads/ without using any refs/remotes/ however is a handy option, especially for those who know what they are doing, and I do not have problem with an effort to help improving such a workflow. It just feels unnecessary, and inviting mistakes, to try to mix that with checked out branches.

For example, if these VMs are mostly for building and making fix-up commits, I would imagine another possible flow might be:

 * Each repository on these bochs would have refs/heads/* but not
   refs/remotes/* and is on detached HEAD.

 * Pushing into these repositories will trigger a hook that performs
   a checkout of the commit at the tip of the updated branch to the
   detached HEAD, and use of "git checkout" in that hook will
   prevent lossage of local changes automatically.

 * After making fix-up commits on one VMs repository, you would
   first locally push HEAD:$branch to update the refs/heads/$branch
   and propagate that to others by pushing $branch.

Now, with "updateInstead", you can keep the target branch checked out, instead of detaching HEAD at the tip of it and having a hook to run "git checkout", and automatically update the working tree. The workflow would become:

 * Each repository on these bochs would have refs/heads/* but not
   refs/remotes/*.  Each have a branch checked out and some or all
   may be on the same branch.

 * Pushing into these repositories will trigger updateInstead to
   update the working tree in a safe way.

 * After making fix-up commits on one VMs repository, propagate that
   change to other VMs by pushing the branch out.

which is a definite improvement.

Explained in that way, I would say "updateInstead" is a welcome addition. The documentation would probably need to explain an expected use (something like what I gave above), so that this can be useful not only by those who know what they are doing, though.

I do not think of a good justification of detachInstead offhand, but you must have thought things through a lot more than I did, so you can come up with a work flow description that is more usable by mere mortals to justify that mode. Without such a description to justify why detachInstead makes sense, for example, I cannot even answer this simple question:

    Would it make sense to have a third mode, "check out if the
    working tree is clean, detach otherwise"?

Show 46 quoted lines

> This change supports both workflows by offering two new values for the
> denyCurrentBranch setting:
>
> 'updateInstead':
> 	Update the working tree accordingly, but refuse to do so if there
> 	are any uncommitted changes.
>
> 'detachInstead':
> 	Detach the HEAD, thereby keeping currently checked-out revision,
> 	index and working directory unchanged.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  Documentation/config.txt |  5 +++++
>  builtin/receive-pack.c   | 58 ++++++++++++++++++++++++++++++++++++++++++++++--
>  t/t5516-fetch-push.sh    | 36 ++++++++++++++++++++++++++++++
>  3 files changed, 97 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index e8dd76d..6fc0d6d 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2129,6 +2129,11 @@ receive.denyCurrentBranch::
>  	print a warning of such a push to stderr, but allow the push to
>  	proceed. If set to false or "ignore", allow such pushes with no
>  	message. Defaults to "refuse".
> ++
> +There are two more options: "updateInstead" which will update the working
> +directory (must be clean) if pushing into the current branch, and
> +"detachInstead" which will leave the working directory untouched, detaching
> +the HEAD so it does not need to change.
>  
>  receive.denyNonFastForwards::
>  	If set to true, git-receive-pack will deny a ref update which is
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 32fc540..be4172f 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -26,7 +26,9 @@ enum deny_action {
>  	DENY_UNCONFIGURED,
>  	DENY_IGNORE,
>  	DENY_WARN,
> -	DENY_REFUSE
> +	DENY_REFUSE,
> +	DENY_UPDATE_INSTEAD,
> +	DENY_DETACH_INSTEAD,

Don't add trailing comma after the last element of enum.

Show 18 quoted lines

> @@ -730,6 +737,44 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
>  	return 0;
>  }
>  
> +static void merge_worktree(unsigned char *sha1)
> +{
> +	const char *update_refresh[] = {
> +		"update-index", "--refresh", NULL
> +	};
> +	const char *read_tree[] = {
> +		"read-tree", "-u", "-m", sha1_to_hex(sha1), NULL
> +	};
> +	struct child_process child;
> +	struct strbuf git_env = STRBUF_INIT;
> +	const char *env[2];
> +
> +	if (is_bare_repository())
> +		die ("denyCurrentBranch = updateInstead needs a worktree");

Why have extra SP only after "die" but not other function names?

> +	strbuf_addf(&git_env, "GIT_DIR=%s", absolute_path(get_git_dir()));
> +	env[0] = git_env.buf;
> +	env[1] = NULL;

Doesn't child.env have managed argv_array these days?

Show 15 quoted lines

> +	memset(&child, 0, sizeof(child));
> +	child.argv = update_refresh;
> +	child.env = env;
> +	child.dir = git_work_tree_cfg ? git_work_tree_cfg : "..";
> +	child.stdout_to_stderr = 1;
> +	child.git_cmd = 1;
> +	if (run_command(&child))
> +		die ("Could not refresh the index");
> +
> +	child.argv = read_tree;
> +	child.no_stdin = 1;
> +	child.no_stdout = 1;
> +	child.stdout_to_stderr = 0;
> +	if (run_command(&child))
> +		die ("Could not merge working tree with new HEAD.  Good luck.");

Drop "Good luck." and replace it with a more useful info. At least, tell the user what state you left her repository in by this botched attempt, so that she can manually recover.

I am guessing that you have already updated HEAD, you are not telling her what commit the index and the working tree is originally based on (which would be very useful to know for manual recovery from this state), but you haven't touched either the index or the working tree at this point in the code.

Other than that, I like what this "updateInstead" code path does.

Thanks.