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

Johannes Schindelin [off-list ref] writes:

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"?

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

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?

+	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.

Hi Junio,

On Fri, 7 Nov 2014, Junio C Hamano wrote:

[...]

I will address your concerns after the weekend.

Ciao,
Johannes

Hi Junio,

On Fri, 7 Nov 2014, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

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 ;-)

I am sorry, I should have been more explicit about the fact that other
"Operating System" includes also Windows, where it is a major hassle to
set up an ssh daemon, hence the asymmetry of ease to connect from one to
another machine. I really thought that was obvious, my apologies. My next
patch iteration will have the Windows scenario as motivation.

Note that I did not even dive into the problem of many loaner laptops
where you are not allowed to set up an ssh daemon, or even know the
user's password. I did not mention those problems because I again assumed
(again, apologies) that this was obvious because it occurred to me
automatically when thinking about the multi-laptop problem.

And I must apologize yet again, for I also failed to specify explicitly
another reason why pushing makes a *lot* more sense than pulling: at least
for me, personally, having to switch keyboards just to synchronize Git
checkouts from one to another computer *is* a hassle.

quoted

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.

I thank you for your assessment of my personal working style.

:-)

And yet again I have to apologize because I find that relying on Git's
fast-forward default (you need to force non-fast-forward ref updates,
which I don't, at least not by default) saves me from that lossage, and
will therefore not change my personal working style that served me so well
for years.

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.

The main justification is that it is safer than updateInstead because it
is guaranteed not to modify the working directory. I intended it for use
by developers who are uncomfortable with updating the working directory
through a push, and as uncomfortable with forgetting about temporary
branches pushed, say, via "git push other-computer HEAD:refs/heads/tmp".

However, I have to admit that I never used this myself after the first few
weeks of playing with this patch series.

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"?

I imagine that some developer might find that useful. If you insist, I
will implement it, even if I personally deem this mode way too complicated
a concept for myself to be used safely.

quoted

[... snip a lot of text that made me almost miss the comments below ...]

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.

Will fix in the next iteration.

quoted

@@ -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?

Will remove the space in the next iteration.

quoted

+	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?

I missed those developments, likewise the introduction of
CHILD_PROCESS_INIT. Fixed in the next iteration.

quoted

+	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.

The best information Git can give at that point is that the working tree
could not be merged with the new HEAD. I stripped "Good luck." in the next
iteration, although I mourn the loss of comedy in Git.

Ciao,
Johannes