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

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