[PATCH 0/2] Support updating working trees when pushing into non-bare repos

[PATCH 0/2] Support updating working trees when pushing into non-bare repos

[Johannes Schindelin]

A few years ago, this developer was convinced that it was a bad idea to
auto-update working directories when pushing into the current branch, and
that an excellent way to prove this was to implement that feature. To his
surprise, it turned out to be the one thing he misses most in upstream Git.

So here goes: this patch series adds support for two new
receive.denyCurrentBranch settings: one to update the working directory
(which must be clean, i.e. there must not be any uncommitted changes) when
pushing into the current branch, the other setting detaches the HEAD
instead.

The scenario in which in particular the 'updateInstead' setting became a
boon in this developer's daily work is a multi-laptop one, where working
directories need to be updated between computers without a hassle.

Johannes Schindelin (2):
  Add a few more values for receive.denyCurrentBranch
  Let deny.currentBranch=updateInstead ignore submodules

 Documentation/config.txt |  5 +++++
 builtin/receive-pack.c   | 58 ++++++++++++++++++++++++++++++++++++++++++++++--
 t/t5516-fetch-push.sh    | 36 ++++++++++++++++++++++++++++++
 3 files changed, 97 insertions(+), 2 deletions(-)

-- 
2.0.0.rc3.9669.g840d1f9

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

[Johannes Schindelin]

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.

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.

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,
 };
 
 static int deny_deletes;
@@ -120,7 +122,12 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
 	}
 
 	if (!strcmp(var, "receive.denycurrentbranch")) {
-		deny_current_branch = parse_deny_action(var, value);
+		if (value && !strcasecmp(value, "updateinstead"))
+			deny_current_branch = DENY_UPDATE_INSTEAD;
+		else if (value && !strcasecmp(value, "detachinstead"))
+			deny_current_branch = DENY_DETACH_INSTEAD;
+		else
+			deny_current_branch = parse_deny_action(var, value);
 		return 0;
 	}
 
@@ -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");
+
+	strbuf_addf(&git_env, "GIT_DIR=%s", absolute_path(get_git_dir()));
+	env[0] = git_env.buf;
+	env[1] = NULL;
+
+	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.");
+
+	strbuf_release(&git_env);
+}
+
 static const char *update(struct command *cmd, struct shallow_info *si)
 {
 	const char *name = cmd->ref_name;
@@ -760,6 +805,13 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 			if (deny_current_branch == DENY_UNCONFIGURED)
 				refuse_unconfigured_deny();
 			return "branch is currently checked out";
+		case DENY_UPDATE_INSTEAD:
+			merge_worktree(new_sha1);
+			break;
+		case DENY_DETACH_INSTEAD:
+			update_ref("push into current branch (detach)", "HEAD",
+				old_sha1, NULL, REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
+			break;
 		}
 	}
 
@@ -788,6 +840,8 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 					refuse_unconfigured_deny_delete_current();
 				rp_error("refusing to delete the current branch: %s", name);
 				return "deletion of the current branch prohibited";
+			default:
+				die ("Invalid denyDeleteCurrent setting");
 			}
 		}
 	}
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index f4da20a..3981d1b 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1330,4 +1330,40 @@ test_expect_success 'fetch into bare respects core.logallrefupdates' '
 	)
 '
 
+test_expect_success 'receive.denyCurrentBranch = updateInstead' '
+	git push testrepo master &&
+	(cd testrepo &&
+		git reset --hard &&
+		git config receive.denyCurrentBranch updateInstead
+	) &&
+	test_commit third path2 &&
+	git push testrepo master &&
+	test $(git rev-parse HEAD) = $(cd testrepo && git rev-parse HEAD) &&
+	test third = "$(cat testrepo/path2)" &&
+	(cd testrepo &&
+		git update-index --refresh &&
+		git diff-files --quiet &&
+		git diff-index --cached HEAD --
+	)
+'
+
+test_expect_success 'receive.denyCurrentBranch = detachInstead' '
+	(cd testrepo &&
+		git reset --hard &&
+		git config receive.denyCurrentBranch detachInstead
+	) &&
+	OLDHEAD=$(cd testrepo && git rev-parse HEAD) &&
+	test_commit fourth path2 &&
+	test fourth = "$(cat path2)" &&
+	git push testrepo master &&
+	test $OLDHEAD = $(cd testrepo && git rev-parse HEAD) &&
+	test fourth != "$(cat testrepo/path2)" &&
+	(cd testrepo &&
+		test_must_fail git symbolic-ref HEAD &&
+		git update-index --refresh &&
+		git diff-files --quiet &&
+		git diff-index --cached HEAD --
+	)
+'
+
 test_done
-- 
2.0.0.rc3.9669.g840d1f9

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

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> 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"?

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

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

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

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

[Johannes Schindelin]

Hi Junio,

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

> [...]

I will address your concerns after the weekend.

Ciao,
Johannes

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

[Johannes Schindelin]

Hi Junio,

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

> Johannes Schindelin <johannes.schindelin@gmx.de> 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 ;-)

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.

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

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

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

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

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

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

[Junio C Hamano]

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

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

You have been around long enough to know that adding a feature of an
unknown value is the last thing I would ask, don't you?

I am essentially saying this:

    I do not see why and the patch and its documentation do not
    explain why it is useful, but Dscho wouldn't have added the
    feature without a reason better than "just because we can do
    so", so let's assume the mode is useful for some unknown reason.
    Would people find "updateInstead if able otherwise
    detachInstead" even more useful for that same unknown reason?

So a good answer would have been one of these:

 * detachInstead is not backed by a use case as useful as
   updateInstead, and was done more from "because we can while at
   it".  Let's drop it for now.

 * detachInstead is a good thing and here is an updated patch to
   better explain the readers of our documentation the workflow to
   use to the feature well.  updateIfAbleOrDetachInstead would be
   useful for the same reason stated in the updated documentation,
   but let's not make the scope of the topic too wide and stop at
   mentioning the possibility in the log message for now.
   

 * detachInstead is a good thing and here is an updated patch to
   better explain the readers of our documentation the workflow to
   use to the feature well.  Once a reader understands this, she
   would realize why updateIfAbleOrDetachInstead would not be
   useful, so it is not even worth mentioning the possibility.

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

Being humourous is good, but "Good luck" while refusing to be
helpful is not humourous; it is just being hostile to the user.

We would be showing the string as the reason for push failure to
"git push" in the new code that does not die but signal the failure
to our caller, so "could not merge working tree with new HEAD" is
good (we shouldn't be doing the "advice" thing here).

Thanks.

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

[Johannes Schindelin]

Hi Junio,

On Mon, 10 Nov 2014, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> 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.
> 
> You have been around long enough to know that adding a feature of an
> unknown value is the last thing I would ask, don't you?

Given that you actually did ask me to add such a feature when I simply
wanted to get a bug fix for fast-export into Git to support Sverre's
remote-hg (that he abandoned because of my failure to get the bug fix in),
I have to respectfully declare that I do not know that, no, sorry!

> I am essentially saying this:
> 
>     I do not see why and the patch and its documentation do not
>     explain why it is useful, but Dscho wouldn't have added the
>     feature without a reason better than "just because we can do
>     so", so let's assume the mode is useful for some unknown reason.
>     Would people find "updateInstead if able otherwise
>     detachInstead" even more useful for that same unknown reason?

Okay, here is my explanation: at the time I wanted to disprove that
updateInstead could make sense, I wanted to offer a milder version of
updating the current branch that left the working directory alone:
detachInstead.

Now, I never used it myself, but I use updateInstead extensively.

So now I offer you two choices:

- help me come up with a good scenario where it makes sense to
  detachInstead, or

- tell me to drop it.

I have no preference.

Ciao,
Johannes

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

[Junio C Hamano]

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

> Okay, here is my explanation: at the time I wanted to disprove that
> updateInstead could make sense, I wanted to offer a milder version of
> updating the current branch that left the working directory alone:
> detachInstead.
>
> Now, I never used it myself, but I use updateInstead extensively.

Sounds like updateInstead turned out to be useful enough to make
a "possibly more cautious" detachInstead unnecessary?  It probably
makes sense not to add it in that case, I would think.

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

[Jeff King]

On Fri, Nov 07, 2014 at 02:58:17PM +0100, Johannes Schindelin wrote:

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

FWIW, I do this without a third server (and without resorting to pull),
with:

  host1$ git push host2 master:refs/remotes/host1/master
  host2$ git merge host1/master

You can even set up a push refspec to make "git push host2" do the right
thing.

That being said, I do like the premise of your patch, as it eliminates
the extra step on the remote side (which is not that big a deal in
itself, but when you realize that host2 _did_ have some changes on it,
then you end up doing the merge there, when in general I'd prefer to do
all the work on host1 via "git pull").

-Peff

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1488 bytes --]

On Sat, Nov 08, 2014 at 06:18:55AM -0500, Jeff King wrote:
> On Fri, Nov 07, 2014 at 02:58:17PM +0100, Johannes Schindelin wrote:
> 
> > 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.
> 
> FWIW, I do this without a third server (and without resorting to pull),
> with:
> 
>   host1$ git push host2 master:refs/remotes/host1/master
>   host2$ git merge host1/master
> 
> You can even set up a push refspec to make "git push host2" do the right
> thing.

I do something similar, but it's inconvenient when the repo you're
pushing into is $HOME, since you have to type something like "exec zsh
-l" in order to fix things up.

> That being said, I do like the premise of your patch, as it eliminates
> the extra step on the remote side (which is not that big a deal in
> itself, but when you realize that host2 _did_ have some changes on it,
> then you end up doing the merge there, when in general I'd prefer to do
> all the work on host1 via "git pull").

I agree.  This is very useful.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Johannes Schindelin]

Hi Peff,

On Sat, 8 Nov 2014, Jeff King wrote:

> On Fri, Nov 07, 2014 at 02:58:17PM +0100, Johannes Schindelin wrote:
> 
> > 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.
> 
> FWIW, I do this without a third server (and without resorting to pull),
> with:
> 
>   host1$ git push host2 master:refs/remotes/host1/master
>   host2$ git merge host1/master
> 
> You can even set up a push refspec to make "git push host2" do the right
> thing.
> 
> That being said, I do like the premise of your patch, as it eliminates
> the extra step on the remote side (which is not that big a deal in
> itself, but when you realize that host2 _did_ have some changes on it,
> then you end up doing the merge there, when in general I'd prefer to do
> all the work on host1 via "git pull").

Plus: you have the luxury of working on an OS that makes ssh'ing from
another machine relatively easy. At least if you have the root password on
your machine. Which, I hate to point it out, is not too common a
commodity.

Ciao,
Dscho

[PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules

[Johannes Schindelin]

They are not affected by the update anyway.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/receive-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index be4172f..4ba51df 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -740,7 +740,7 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
 static void merge_worktree(unsigned char *sha1)
 {
 	const char *update_refresh[] = {
-		"update-index", "--refresh", NULL
+		"update-index", "--ignore-submodules", "--refresh", NULL
 	};
 	const char *read_tree[] = {
 		"read-tree", "-u", "-m", sha1_to_hex(sha1), NULL
-- 
2.0.0.rc3.9669.g840d1f9

Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules

[Junio C Hamano]

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

> They are not affected by the update anyway.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/receive-pack.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index be4172f..4ba51df 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -740,7 +740,7 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
>  static void merge_worktree(unsigned char *sha1)
>  {
>  	const char *update_refresh[] = {
> -		"update-index", "--refresh", NULL
> +		"update-index", "--ignore-submodules", "--refresh", NULL
>  	};
>  	const char *read_tree[] = {
>  		"read-tree", "-u", "-m", sha1_to_hex(sha1), NULL

I suspect that you did not squash this into 1/2 on purpose, and I am
guessing the reason is because you were unsure what should happen
when there were differences in submodules' working trees (otherwise,
you would have simply squashed without "oops it was a thinko to
forget passing this option" as a separate patch).  I am not sure
either.

By the way, if the expected use case of updateInstead is what I
outlined in the previous message, would it make more sense not to
fail with "update-index --refresh" failure (i.e. the working tree
files have no changes since the index)?

Thinking about it a bit more, checking with "update-index --refresh"
feels doubly wrong.  You not just want the working tree files to be
pristine with respect to the index, but also you do not want to see
any change between the index and the original HEAD, i.e.

	$ git reset --hard && echo >>Makefile ; git add Makefile
        $ git update-index --refresh ; echo $?
        0

this is not a good state from which you would want to update the
working tree.

Wouldn't the two-tree form "read-tree -u -m" that is the equivalent
to branch switching do a sufficient check?

Also, regarding the new calls to die() in the main patch, shouldn't
they just be returning the error reason in string form, just like
DENY_REFUSE returns "branch is currently checked out" to signal a
push failure to the caller?

Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules

[Jens Lehmann]

Am 07.11.2014 um 20:20 schrieb Junio C Hamano:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
>> They are not affected by the update anyway.
>>
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> ---
>>   builtin/receive-pack.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
>> index be4172f..4ba51df 100644
>> --- a/builtin/receive-pack.c
>> +++ b/builtin/receive-pack.c
>> @@ -740,7 +740,7 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
>>   static void merge_worktree(unsigned char *sha1)
>>   {
>>   	const char *update_refresh[] = {
>> -		"update-index", "--refresh", NULL
>> +		"update-index", "--ignore-submodules", "--refresh", NULL
>>   	};
>>   	const char *read_tree[] = {
>>   		"read-tree", "-u", "-m", sha1_to_hex(sha1), NULL
>
> I suspect that you did not squash this into 1/2 on purpose, and I am
> guessing the reason is because you were unsure what should happen
> when there were differences in submodules' working trees (otherwise,
> you would have simply squashed without "oops it was a thinko to
> forget passing this option" as a separate patch).  I am not sure
> either.

I think --ignore-submodules is currently the right thing to do here
and would rather squash this into the first commit.

> By the way, if the expected use case of updateInstead is what I
> outlined in the previous message, would it make more sense not to
> fail with "update-index --refresh" failure (i.e. the working tree
> files have no changes since the index)?
>
> Thinking about it a bit more, checking with "update-index --refresh"
> feels doubly wrong.  You not just want the working tree files to be
> pristine with respect to the index, but also you do not want to see
> any change between the index and the original HEAD, i.e.
>
> 	$ git reset --hard && echo >>Makefile ; git add Makefile
>          $ git update-index --refresh ; echo $?
>          0
>
> this is not a good state from which you would want to update the
> working tree.
>
> Wouldn't the two-tree form "read-tree -u -m" that is the equivalent
> to branch switching do a sufficient check?
>
> Also, regarding the new calls to die() in the main patch, shouldn't
> they just be returning the error reason in string form, just like
> DENY_REFUSE returns "branch is currently checked out" to signal a
> push failure to the caller?
>
>

Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules

[Johannes Schindelin]

Hi Jens,

On Sun, 9 Nov 2014, Jens Lehmann wrote:

> Am 07.11.2014 um 20:20 schrieb Junio C Hamano:
> > Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> >
> > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > > ---
> > >   builtin/receive-pack.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> > > index be4172f..4ba51df 100644
> > > --- a/builtin/receive-pack.c
> > > +++ b/builtin/receive-pack.c
> > > @@ -740,7 +740,7 @@ static int update_shallow_ref(struct command *cmd,
> > > struct shallow_info *si)
> > >   static void merge_worktree(unsigned char *sha1)
> > >   {
> > >   	const char *update_refresh[] = {
> > > -		"update-index", "--refresh", NULL
> > > +		"update-index", "--ignore-submodules", "--refresh", NULL
> > >    };
> > >    const char *read_tree[] = {
> > >     "read-tree", "-u", "-m", sha1_to_hex(sha1), NULL
> >
> > I suspect that you did not squash this into 1/2 on purpose, and I am
> > guessing the reason is because you were unsure what should happen
> > when there were differences in submodules' working trees (otherwise,
> > you would have simply squashed without "oops it was a thinko to
> > forget passing this option" as a separate patch).  I am not sure
> > either.
> 
> I think --ignore-submodules is currently the right thing to do here
> and would rather squash this into the first commit.

Done.

Thanks,
Dscho

Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules

[Johannes Schindelin]

Hi Junio,

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

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  builtin/receive-pack.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> > index be4172f..4ba51df 100644
> > --- a/builtin/receive-pack.c
> > +++ b/builtin/receive-pack.c
> > @@ -740,7 +740,7 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
> >  static void merge_worktree(unsigned char *sha1)
> >  {
> >  	const char *update_refresh[] = {
> > -		"update-index", "--refresh", NULL
> > +		"update-index", "--ignore-submodules", "--refresh", NULL
> >  	};
> >  	const char *read_tree[] = {
> >  		"read-tree", "-u", "-m", sha1_to_hex(sha1), NULL
> 
> I suspect that you did not squash this into 1/2 on purpose, and I am
> guessing the reason is because you were unsure what should happen
> when there were differences in submodules' working trees (otherwise,
> you would have simply squashed without "oops it was a thinko to
> forget passing this option" as a separate patch).  I am not sure
> either.

This change is squashed into the first patch in the next iteration.

> By the way, if the expected use case of updateInstead is what I
> outlined in the previous message, would it make more sense not to
> fail with "update-index --refresh" failure (i.e. the working tree
> files have no changes since the index)?
> 
> Thinking about it a bit more, checking with "update-index --refresh"
> feels doubly wrong.  You not just want the working tree files to be
> pristine with respect to the index, but also you do not want to see
> any change between the index and the original HEAD, i.e.
> 
> 	$ git reset --hard && echo >>Makefile ; git add Makefile
>         $ git update-index --refresh ; echo $?
>         0
> 
> this is not a good state from which you would want to update the
> working tree.
> 
> Wouldn't the two-tree form "read-tree -u -m" that is the equivalent
> to branch switching do a sufficient check?

That is indeed what the patched code calls.

> Also, regarding the new calls to die() in the main patch, shouldn't
> they just be returning the error reason in string form, just like
> DENY_REFUSE returns "branch is currently checked out" to signal a
> push failure to the caller?

Changed in the next iteration.

Ciao,
Johannes

Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules

[Junio C Hamano]

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

>> By the way, if the expected use case of updateInstead is what I
>> outlined in the previous message, would it make more sense not to
>> fail with "update-index --refresh" failure (i.e. the working tree
>> files have no changes since the index)?
>> 
>> Thinking about it a bit more, checking with "update-index --refresh"
>> feels doubly wrong.  You not just want the working tree files to be
>> pristine with respect to the index, but also you do not want to see
>> any change between the index and the original HEAD, i.e.
>> 
>> 	$ git reset --hard && echo >>Makefile ; git add Makefile
>>         $ git update-index --refresh ; echo $?
>>         0
>> 
>> this is not a good state from which you would want to update the
>> working tree.
>> 
>> Wouldn't the two-tree form "read-tree -u -m" that is the equivalent
>> to branch switching do a sufficient check?
>
> That is indeed what the patched code calls.

I know that ;-), but I think I wasn't clear enough.

I am not saying you are not using two-tree read-tree.  I am saying
the check with update-index --refresh before read-tree seems
dubious.

There are three "cleanliness" we require in various occasions:

 (1) rebase asks you to have your index and the working tree match
     HEAD exactly, as if just after "reset --hard HEAD".

 (2) merge asks you to have your index match HEAD exactly (i.e. no
     output from "diff --cached HEAD"), and have no changes in the
     working tree at paths that are involved in the operation.  It
     is OK to have local changes in the working tree (but not in the
     index) at paths that are not involved in a mergy operation.

 (3) checkout asks you to have your index and the working tree match
     either HEAD or the commit you are switching to exactly at paths
     that are involved in the operation.  It is OK to have local
     changes in the working tree and in the index at paths that are
     not different between the commits you are moving between, and
     it is also OK to have the contents from the "new" commit in the
     working tree and the index at paths that are different between
     the two.

Dying when "update-index --refresh" signals a difference is an
attempt to mimic #1, but it is in line with the spirit of the reason
why a user would want to use updateInstead, I think.  The situation
is more like the person who pushed into your repository from
sideline did a "checkout -B $current_branch $new_commit" to update
the HEAD, the index and the working tree, to let you pretend as if
you based your work on the commit he pushed to you.

While you still need to error out when your local work does not
satisfy the cleanliness criteria #3 above, I do not think you would
want to stop the operation when "checkout" would not fail, e.g. you
have a local change that does not interfere with the update between
the two commits, with this one:

+	if (run_command(&child))
+		die ("Could not refresh the index");

When refreshed the index successfully, we signal that there were
differences between the index and the working tree with a non-zero
return value, so "Could not refresh" is not quite right, either.

But this one that checks the exit status from two-tree read-tree

+	if (run_command(&child))
+		die ("Could not merge working tree with new HEAD.  Good luck.");

is checking the right condition, i.e. cleanliness #3.  The
disposition should not be "die", but an error return to tell the
caller to abort the push as we discussed earlier.

Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Dying when "update-index --refresh" signals a difference is an
> attempt to mimic #1, but it is in line with the spirit of the reason
> why a user would want to use updateInstead, I think.  The situation
> is more like the person who pushed into your repository from
> sideline did a "checkout -B $current_branch $new_commit" to update
> the HEAD, the index and the working tree, to let you pretend as if
> you based your work on the commit he pushed to you.
>
> While you still need to error out when your local work does not
> satisfy the cleanliness criteria #3 above, I do not think you would
> want to stop the operation when "checkout" would not fail, e.g. you
> have a local change that does not interfere with the update between
> the two commits, with this one:
>
> +	if (run_command(&child))
> +		die ("Could not refresh the index");
>
> When refreshed the index successfully, we signal that there were
> differences between the index and the working tree with a non-zero
> return value, so "Could not refresh" is not quite right, either.

Just to make sure.  I am *not* saying that you do not need to run
"update-index --refresh".  It is necessary before running read-tree
to avoid false dirtyness, so you do need to run it.

I am only saying that it is too strict to fail the operation when
the command reports that you have a local modification in the
working tree.

> But this one that checks the exit status from two-tree read-tree
>
> +	if (run_command(&child))
> +		die ("Could not merge working tree with new HEAD.  Good luck.");
>
> is checking the right condition, i.e. cleanliness #3.  The
> disposition should not be "die", but an error return to tell the
> caller to abort the push as we discussed earlier.

Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules

[Johannes Schindelin]

Hi Junio,

On Mon, 10 Nov 2014, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Dying when "update-index --refresh" signals a difference is an
> > attempt to mimic #1, but it is in line with the spirit of the reason
> > why a user would want to use updateInstead, I think.  The situation
> > is more like the person who pushed into your repository from
> > sideline did a "checkout -B $current_branch $new_commit" to update
> > the HEAD, the index and the working tree, to let you pretend as if
> > you based your work on the commit he pushed to you.
> >
> > While you still need to error out when your local work does not
> > satisfy the cleanliness criteria #3 above, I do not think you would
> > want to stop the operation when "checkout" would not fail, e.g. you
> > have a local change that does not interfere with the update between
> > the two commits, with this one:
> >
> > +	if (run_command(&child))
> > +		die ("Could not refresh the index");
> >
> > When refreshed the index successfully, we signal that there were
> > differences between the index and the working tree with a non-zero
> > return value, so "Could not refresh" is not quite right, either.
> 
> Just to make sure.  I am *not* saying that you do not need to run
> "update-index --refresh".  It is necessary before running read-tree
> to avoid false dirtyness, so you do need to run it.
> 
> I am only saying that it is too strict to fail the operation when
> the command reports that you have a local modification in the
> working tree.

Okay, now I am even more puzzled. I guess you actually meant to say that I
need to convert the die() into a return? If so, I agree fully.

Ciao,
Johannes

Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules

[Junio C Hamano]

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

> Hi Junio,
>
> On Mon, 10 Nov 2014, Junio C Hamano wrote:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>> 
>> > Dying when "update-index --refresh" signals a difference is an
>> > attempt to mimic #1, but it is in line with the spirit of the reason
>> > why a user would want to use updateInstead, I think.  The situation
>> > is more like the person who pushed into your repository from
>> > sideline did a "checkout -B $current_branch $new_commit" to update
>> > the HEAD, the index and the working tree, to let you pretend as if
>> > you based your work on the commit he pushed to you.
>> >
>> > While you still need to error out when your local work does not
>> > satisfy the cleanliness criteria #3 above, I do not think you would
>> > want to stop the operation when "checkout" would not fail, e.g. you
>> > have a local change that does not interfere with the update between
>> > the two commits, with this one:
>> >
>> > +	if (run_command(&child))
>> > +		die ("Could not refresh the index");
>> >
>> > When refreshed the index successfully, we signal that there were
>> > differences between the index and the working tree with a non-zero
>> > return value, so "Could not refresh" is not quite right, either.
>> 
>> Just to make sure.  I am *not* saying that you do not need to run
>> "update-index --refresh".  It is necessary before running read-tree
>> to avoid false dirtyness, so you do need to run it.
>> 
>> I am only saying that it is too strict to fail the operation when
>> the command reports that you have a local modification in the
>> working tree.
>
> Okay, now I am even more puzzled. I guess you actually meant to say that I
> need to convert the die() into a return? If so, I agree fully.

No.

"update-index --refresh" does two things.

 (a) For performance reasons, plumbing commands such as diff-files
     and read-tree do not refresh the stat bits in the index.  They
     expect to be run from scripted Porcelain commands, and expect
     that the caller would refresh the stat bits before they are
     called to prevent them from mistakingly seeing that an
     unmodified existing file, after "touch existing-file", as
     modified.

     And "update-index --refresh" is the way for the caller to do so.

 (b) "update-index --refresh" indicates with its exit status if the
     working tree files match what is recorded in the index.  This
     can be used to see if "diff-files" would report difference.

As you are going to run "read-tree -m -u", you need to refresh the
stat bits for purpose (a) above, i.e. to avoid "read-tree" from
failing due to a difference that does not exist.  Because I am
assuming that your "cleanliness" requirement to update the working
tree is criterion #3 in the previous message, I do not think you
would want to abort the update only because there are some
difference between the index and the working tree.  That means that
checking the exit status of "update-index --refresh" and dying (or
signaling the failure to the caller by returning a non-NULL string,
in the context of this call path) is not what you want.  You may
have a local change to Makefile in the working tree of the
repository that you are pushing into, and there may not be any
change to the Makefile between the original HEAD the working tree is
based on and the updated HEAD you are pushing into the repository.
"update-index --refresh" will say "You have a local change." and
exit with non-zero status, but just like "git checkout another" to
switch to another branch while you have some local change that does
not overlap with the difference between branches does not fail, you
would want to allow the update.

You may be trying to use a cleanliness requirement that is different
from criterion #3 in the previous message, but checking the exit
status from "update-index --refresh" does not make much sense in
that case either.  I do not think you want to have:

 * pushing into a repository that did "edit Makefile; git add Makefile"
   succeeds.

 * pushing into a repository that did "edit Makefile" without "git
   add Makefile" fails.

but that is what you will get, because "update-index --refresh"
would say "Your working tree matches the index" by exiting with 0 in
the former case, and you will end up running "read-tree -m -u".

Having said all that.

Instead of running "update-index --refresh; read-tree -m -u", using
"reset --keep" may be a better implementation of what you are trying
to do here.

I think a "checkout -B <current-branch> <new-commit>" is what you
want to run when a push attempts to update the current branch from
sideways with updateInstead, and "reset --keep <new-commit>" lets
you run an equivalent of the "checkout -B <current-branch>" but you
do not have to know the name of the <current-branch>.  Also by using
"reset --keep", you do not have to worry about refreshing the index.

Thanks.

Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules

[Johannes Schindelin]

Hi Junio,

On Wed, 12 Nov 2014, Junio C Hamano wrote:

> Instead of running "update-index --refresh; read-tree -m -u", using
> "reset --keep" may be a better implementation of what you are trying to
> do here.

I do not think that `reset --keep` is what I want. I really want to update
only if the working directory is clean. So I guess I will have to bite the
bullet and test the output of `update-index --refresh`, `diff-index
--quiet --cached HEAD --`.

In my case, the lacking test whether there are staged changes did not
matter, just because I pretty much never leave staged changes around.

What did matter, however, was to make sure that I did not update the
working directory carelessly. In one case, that `update-index --refresh`
test really helped me out because I was about to push  into a working
directory with uncommitted changes inside some web space. That push would
have broken the web application because of the local changes, so I was
really, really happy that I decided to be quite strict in the
implementation of `updateInstead`.

Due to that experience, the documentation also states pretty clearly that
`updateInstead` succeeds only in updating the current branch if the
working directory is clean.

Ciao,
Johannes

Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules

[Johannes Schindelin]

Hi Junio,

On Thu, 13 Nov 2014, Johannes Schindelin wrote:

> Due to that experience, the documentation also states pretty clearly that
> `updateInstead` succeeds only in updating the current branch if the
> working directory is clean.

To clarify why `updateInstead` is stricter than the `merge` scenario: when
pushing into a remote repository with attached working directory, we
cannot simply clean up merge conflicts or other problems introduced by a
merge. Indeed, it is even possible to push without having any option to
fix files in the working directory afterwards. Therefore, the
`updateInstead` feature really needs to be adamant about the working
directory being clean.

Ciao,
Johannes

Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules

[Junio C Hamano]

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

> On Thu, 13 Nov 2014, Johannes Schindelin wrote:
>
>> Due to that experience, the documentation also states pretty clearly that
>> `updateInstead` succeeds only in updating the current branch if the
>> working directory is clean.
>
> To clarify why `updateInstead` is stricter than the `merge` scenario: when
> pushing into a remote repository with attached working directory, we
> cannot simply clean up merge conflicts or other problems introduced by a
> merge. Indeed, it is even possible to push without having any option to
> fix files in the working directory afterwards. Therefore, the
> `updateInstead` feature really needs to be adamant about the working
> directory being clean.

As the stricter check (which I think is unnecessarily strict and
which you don't) can be loosened later without making something the
user used to be able to do in an early version unable to do later,
I am OK to accept the design as-is.

But after reading this addendum, I feel the need to double check.

A "reset --keep <new-commit>", which is the same as "checkout -B
<current> <new-commit>" (note the lack of "-m"), does not have
any "merge conflict" issues.

To see what I mean, follow along this simple experiment.

    # Just make sure we start clean
    $ git reset --hard

    # Create a playpen
    $ git checkout -b throwaway master

    # Pick one random path that is different.  We pretend that
    # somebody pushed the commit at the tip of 'next' from the side
    $ git diff --name-only next | head -n 1
    Documentation/git-clone.txt

    # Make sure another random path used in the experiment is unchanged
    $ git diff next -- COPYING | wc -l
    0

    # Smudge a path not involved in the branch/HEAD switching
    $ echo >>COPYING

    # attempt to updateInstead to the other version succeeds.
    $ git reset --keep next; echo $?
    0
    $ git status -suno
     M COPYING
    # Notice that we did not get into a funny state.
    # You can verify it with "git diff".

    # Go back and try smudging what would need a real merge
    $ git reset --hard master
    $ echo >>Documentation/git-clone.txt

    # attempt to updateInstead to the other version
    $ git reset --keep next; echo $?
    error: Entry 'Documentation/git-clone.txt' not uptodate. Cannot merge.
    fatal: Could not reset index file to revision 'next'.
    128

    # See that HEAD did not move
    $ git log HEAD...master | wc -l
    0

    # See that the working tree change is intact
    $ git status -suno
     M Documentation/git-clone.txt
    # Notice that we did not get into a funny state.
    # You can verify it with "git diff".

The semantics is just as safe as "checkout <another-branch>" without
the "-m" option; if a local change may be clobbered or needs real
file-level merge, the operation fails without touching anything to
preserve the local state.

It is OK for us to agree to disagree, and I am willing to accept the
stricter design for an initial version because loosening it later is
possible without harming users.

But I wanted to first make sure that your disagreement is an
informed one.  Do you still feel the need for "absolutely clean",
even if you take the safety built into "reset --keep" shown in the
above experiment into account?

Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5197 bytes --]

Hi Junio,

On Thu, 13 Nov 2014, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Thu, 13 Nov 2014, Johannes Schindelin wrote:
> >
> >> Due to that experience, the documentation also states pretty clearly that
> >> `updateInstead` succeeds only in updating the current branch if the
> >> working directory is clean.
> >
> > To clarify why `updateInstead` is stricter than the `merge` scenario: when
> > pushing into a remote repository with attached working directory, we
> > cannot simply clean up merge conflicts or other problems introduced by a
> > merge. Indeed, it is even possible to push without having any option to
> > fix files in the working directory afterwards. Therefore, the
> > `updateInstead` feature really needs to be adamant about the working
> > directory being clean.
> 
> As the stricter check (which I think is unnecessarily strict and
> which you don't) can be loosened later without making something the
> user used to be able to do in an early version unable to do later,
> I am OK to accept the design as-is.

Thanks for mentioning this. I would like to ask not to loosen this later.
Let me try to explain in more detail than before why I think it would make
*my* life hard if it ever were loosened.

Let's start with a hypothetical change to notes.c, a change, say, that
uses part of the diff machinery. So I am doing that in my working
directory, happily testing stuff, but not quite happy with the result just
yet, staging a couple of things after a debug run so I can "git stash -k"
away the debugging statements.

Please note that while the scenario I just illustrated is purely
hypothetical, made up from thin air, the workflow and the technique is
very much my daily bread, though. I do use "git stash -k" quite frequently
to stash away stuff that I do not want, after staging what I want to keep
first.

Now let's continue by saying that I get interrupted with that notes.c work
and only come back to it much, much later, say, a couple of months,
because I have been working on a different computer in the meantime,
say, fixing Git for Windows bugs like mad.

Okay, so now I am back and I "git pull" your 'master'. Now, for the sake
of the argument, let's say that the pull does not introduce any changes to
notes.c, but instead that there has been an improvement in the diff
machinery, one that required a change of the diff machinery's API and all
of a sudden, my code does not compile anymore. It merged cleanly,
Git is completely groovy, but the code just does not compile any more. Not
a big deal: I can inspect the changes via "git diff
junio/master@{1}..junio/master" and find out very easily what went wrong
and fix my code, compile, stage, and maybe even commit because the code
now works.

Splendid.

But it would be different if the working directory was on another computer
than the one I am operating from. I could not easily access the working
directory, let alone the Git history, if all I did was a "git push".

Now let's modify the scenario a little bit further, still. Let's not talk
about git.git, but about a project implementing a web application. And
let's say that I did not dabble with notes.c but I had to work on the
production setup for a really, really urgent hotfix where the speed of a
fix was more important than not touching the production setup at all.

Please note that this is *not* a hypothetical scenario, but a true account
of what I had to go through a couple of times (don't try this at home, it
causes a lot of gray hair).

Now let's assume that I forgot to commit and push, and that nobody noticed
– and let's just draw the curtain of charity over the reasons why. Also,
let's assume that a couple of months later, I am asked to implement a new
feature for the very same web application, this time without frantic "You.
Have. To. Fix. This. ASAP!" stuff going on. So I develop this on a test
machine, test thoroughly, everything's groovy, and then I push, using the
'updateInstead' feature introduced by the patch in question (and which was
applied to that machine's Git at that time, before some joker updated it
with the official Git).

And now when I try to push, Git complains that the working directory is
not clean, which is *just* fine by me, because after further inspection it
turns out that the uncommitted changes – which are in a different file
than the changes introducing my new feature – would have borked the
production system rather thoroughly.

Please note again that I am talking about something that really happened.

So I would really like to have 'updateInstead', but only in the safe
version, i.e. refusing to update anything but a clean working directory,
and it would be a major regression for me, affecting my workflow rather
negatively, if those strict checks were loosened.

In my mind, when (and if!) a less strict version is desired, it should be
added as another denyCurrentBranch value and 'updateInstead' should be
unaffected, otherwise, speaking for me personally, all my work in this
patch series would be for naught.

Ciao,
Johannes

Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules

[Junio C Hamano]

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

> Thanks for mentioning this. I would like to ask not to loosen this later.
> Let me try to explain in more detail than before why I think it would make
> *my* life hard if it ever were loosened.
> ...
> And now when I try to push, Git complains that the working directory is
> not clean, which is *just* fine by me, because after further inspection it
> turns out that the uncommitted changes – which are in a different file
> than the changes introducing my new feature – would have borked the
> production system rather thoroughly.
> ...
> In my mind, when (and if!) a less strict version is desired, it should be
> added as another denyCurrentBranch value and 'updateInstead' should be
> unaffected, otherwise, speaking for me personally, all my work in this
> patch series would be for naught.

Thanks for an explanation why the updateInstead mode must require a
pristine working tree (and the index).  I *now* agree why such a
mode would be useful, *after* reading it.  I did not understand why
*after* reading only the patch, the documentation updates and the
log message.

That tells us something, doesn't it? ;-)

Also the failure case test must protect us from making a change you
fear in the future.  The interdiff you sent in a separate message
was to smudge path2 that is modified in the 'fourth' commit, which
should fail with either your "OK only if really clean" requirement
or the other "OK as long as it does not interfere with the switch"
criterion.  Checking that is a good step, but you would want to have
a separate "Smudge a path that is unchanged with the switch and see
the push updateInstead fail" to protect the current semantics.

I agree that a new value "mergeInstead" or something should be
invented when/if different workflows want a looser semantics.
People would rely not only on "being able to push when clean" but
also on "being safely prevented from pushing when not" (and that is
where my earlier comment to test both sides comes from).  Loosening
the check to an existing "updateInstead" would break users who have
been using updateInstead.

Also I suspect that people would want to send a patch to add "-q" to
your "update-index --refresh" invocation, at which time you would
need to add a call to "diff-files" to check that the working tree
and the index match.  You may want to add that "diff-files" now, or
at least have a test that is designed to break when "-q" is added to
"update-index --refresh" without adding the necessary "diff-files".

Thanks.

Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Thanks for an explanation why the updateInstead mode must require a
> pristine working tree (and the index).  I *now* agree why such a
> mode would be useful, *after* reading it.  I did not understand why
> *after* reading only the patch, the documentation updates and the
> log message.
>
> That tells us something, doesn't it? ;-)

Just to be less cryptic, I meant that the documentation since v2 is
probably insufficient.

> I agree that a new value "mergeInstead" or something should be
> invented when/if different workflows want a looser semantics.
> People would rely not only on "being able to push when clean" but
> also on "being safely prevented from pushing when not" (and that is
> where my earlier comment to test both sides comes from).  Loosening
> the check to an existing "updateInstead" would break users who have
> been using updateInstead.

And thinking about the names again, I have a feeling that
updateInstead and mergeInstead are both probably misnomer.  The
"make sure there is no modification and then do an equivalent of
reset --hard there" option makes sense only in push-to-deploy
scenario (perhaps "resetInstead"?)  Compared to that, "do an
equivalent of checkout as long as it can be done without involving
real merges" sounds more deserving the "updateInstead" name.

But I am not very good at names (and I suspect you feel you yourself
aren't, either), so perhaps somebody listening from the sideline can
chime in.

Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> I agree that a new value "mergeInstead" or something should be
>> invented when/if different workflows want a looser semantics.
>> People would rely not only on "being able to push when clean" but
>> also on "being safely prevented from pushing when not" (and that is
>> where my earlier comment to test both sides comes from).  Loosening
>> the check to an existing "updateInstead" would break users who have
>> been using updateInstead.
>
> And thinking about the names again, I have a feeling that
> updateInstead and mergeInstead are both probably misnomer.

Let me take this part back.  After all, I do not think I would
design the mechanism to implement an alternative logic that decides
when it is safe to allow the update of the ref and to reflect the
changes to the working tree, and that actually does the checkout to
the working tree by using a new value like mergeInstead.  So we
would only need a single name, and updateInstead is not too bad.

The word "update" is so heavily loaded in the context of accepting a
push (i.e. it is unclear what update we are talking about---updating
the ref that we normally refuse to update?  updating the index?
updating the working tree?  Some combination of them?), so as a
single word, "checkoutInstead" may probably be a better one, though.
Upon hearing "checkout", by definition anybody would know that we
are involving the working tree.

The mechanism I would employ when doing an alternative logic,
possibly looser one but does not necessarily so, would be to have a
hook script "push-to-checkout".  When denyCurrentBranch is set to
updateInstead, if there is no such hook, the "working tree has to be
absolutely clean and we would do a 'read-tree -m -u $old $new'
(which is the same as 'reset --hard $new' under the precondition)"
you implemented will be used as the "logic that decides when it is
safe, and that does the checkout to the working tree".  When the
"push-to-checkout" hook exists, however, we just invoke that hook
with $new as argument, and it can decide when it is safe in whatever
way it chooses to, and it can checkout the $new to the working tree
in whatever way it wants.  The users of "mergeInstead" (now a dead
and unnecessary name) mode would just have a single-liner

	#!/bin/sh
	git reset --keep "$1" --

in there, as this single command would both decide when it is safe
and does the safe (according to its own definition) updating of the
working tree.

In your other example (not the "deploy to live website" one) of
unidirectional SSH connection, you would be able to connect from
machine A to machine B but not the other way, so while sitting on
machine A you would typically have one SSH session to have an
interactive shell session running on machine B.  You may have local
modification on machine B but your changes to history on machine A
cannot be pulled, so you would emulate it by pushing from A into B.
In such a case, unlike the "live website" example, it would be
useful to loosen the condition even more than "reset --keep" (which
is an equivalent of "checkout -B $current_branch $new_commit").

In such a case, what you want to do is to simulate "git pull" that
could conflict and give you a chance to resolve with a push in the
reverse direction.  You want to run an equivalent of the same
"checkout -B" command but with the "-m" option when accepting such a
push.

There are other definitions of what is safe and how update should
happen depending on the user, and such a logic can be placed in the
push-to-checkout hook without harming other users.

Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> And thinking about the names again, I have a feeling that
>> updateInstead and mergeInstead are both probably misnomer.
>
> Let me take this part back.  After all, I do not think I would
> design the mechanism to implement an alternative logic that decides
> when it is safe to allow the update of the ref and to reflect the
> changes to the working tree, and that actually does the checkout to
> the working tree by using a new value like mergeInstead.  So we
> would only need a single name, and updateInstead is not too bad.
> ...
> The mechanism I would employ when doing an alternative logic,
> possibly looser one but does not necessarily so, would be to have a
> hook script "push-to-checkout".  When denyCurrentBranch is set to
> updateInstead, if there is no such hook, the "working tree has to be
> absolutely clean and we would do a 'read-tree -m -u $old $new'
> (which is the same as 'reset --hard $new' under the precondition)"
> you implemented will be used as the "logic that decides when it is
> safe, and that does the checkout to the working tree".  When the
> "push-to-checkout" hook exists, however, we just invoke that hook
> with $new as argument, and it can decide when it is safe in whatever
> way it chooses to, and it can checkout the $new to the working tree
> in whatever way it wants.

So here comes a two-patch series on top of your series (with the
test update I sent earlier).  As I never do "push to deploy" that
requires no changes to the working tree or to the index, while I
have seem myself in a situation where I have to emulate a "git pull"
with a "git push" in the opposite direction (and work it around if
the target of the 'git pull' I wanted to do were the current branch,
by first pushing into a throw-away branch, because of denyCurrent),
I could imagine myself using this variant.  Having said that, this
is primarily so that I do not want to forget and discard the brain
cycles we spent discussing this to the waste, more than that I
cannot wait to use this feature myself ;-)

The first one here is a pure refactoring.  The second one is the
real fun.

-- >8 --
Subject: [PATCH 1/2] receive-pack: refactor updateInstead codepath

Keep the "there is nothing to update in a bare repository", "when
the check and update process runs, here are the GIT_DIR and
GIT_WORK_TREE" logic, which will be common regardless of how the
decision to update and the actual update are done, in the original
update_worktree() function, and split out the "working tree and
the index must match the original HEAD exactly" and "use two-way
read-tree to update the working tree" into a new push_to_deploy()
helper function.  This will allow customizing the logic more cleanly
and easily.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/receive-pack.c | 53 ++++++++++++++++++++++++++------------------------
 1 file changed, 28 insertions(+), 25 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c047418..11800cd 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -733,7 +733,9 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
 	return 0;
 }
 
-static const char *update_worktree(unsigned char *sha1)
+static const char *push_to_deploy(unsigned char *sha1,
+				  struct argv_array *env,
+				  const char *work_tree)
 {
 	const char *update_refresh[] = {
 		"update-index", "-q", "--ignore-submodules", "--refresh", NULL
@@ -748,69 +750,70 @@ static const char *update_worktree(unsigned char *sha1)
 	const char *read_tree[] = {
 		"read-tree", "-u", "-m", NULL, NULL
 	};
-	const char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : "..";
-	struct argv_array env = ARGV_ARRAY_INIT;
 	struct child_process child = CHILD_PROCESS_INIT;
 
-	if (is_bare_repository())
-		return "denyCurrentBranch = updateInstead needs a worktree";
-
-	argv_array_pushf(&env, "GIT_DIR=%s", absolute_path(get_git_dir()));
-
 	child.argv = update_refresh;
-	child.env = env.argv;
+	child.env = env->argv;
 	child.dir = work_tree;
 	child.no_stdin = 1;
 	child.stdout_to_stderr = 1;
 	child.git_cmd = 1;
-	if (run_command(&child)) {
-		argv_array_clear(&env);
+	if (run_command(&child))
 		return "Up-to-date check failed";
-	}
 
 	/* run_command() does not clean up completely; reinitialize */
 	child_process_init(&child);
 	child.argv = diff_files;
-	child.env = env.argv;
+	child.env = env->argv;
 	child.dir = work_tree;
 	child.no_stdin = 1;
 	child.stdout_to_stderr = 1;
 	child.git_cmd = 1;
-	if (run_command(&child)) {
-		argv_array_clear(&env);
+	if (run_command(&child))
 		return "Working directory has unstaged changes";
-	}
 
 	child_process_init(&child);
 	child.argv = diff_index;
-	child.env = env.argv;
+	child.env = env->argv;
 	child.no_stdin = 1;
 	child.no_stdout = 1;
 	child.stdout_to_stderr = 0;
 	child.git_cmd = 1;
-	if (run_command(&child)) {
-		argv_array_clear(&env);
+	if (run_command(&child))
 		return "Working directory has staged changes";
-	}
 
 	read_tree[3] = sha1_to_hex(sha1);
 	child_process_init(&child);
 	child.argv = read_tree;
-	child.env = env.argv;
+	child.env = env->argv;
 	child.dir = work_tree;
 	child.no_stdin = 1;
 	child.no_stdout = 1;
 	child.stdout_to_stderr = 0;
 	child.git_cmd = 1;
-	if (run_command(&child)) {
-		argv_array_clear(&env);
+	if (run_command(&child))
 		return "Could not update working tree to new HEAD";
-	}
 
-	argv_array_clear(&env);
 	return NULL;
 }
 
+static const char *update_worktree(unsigned char *sha1)
+{
+	const char *retval;
+	const char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : "..";
+	struct argv_array env = ARGV_ARRAY_INIT;
+
+	if (is_bare_repository())
+		return "denyCurrentBranch = updateInstead needs a worktree";
+
+	argv_array_pushf(&env, "GIT_DIR=%s", absolute_path(get_git_dir()));
+
+	retval = push_to_deploy(sha1, &env, work_tree);
+
+	argv_array_clear(&env);
+	return retval;
+}
+
 static const char *update(struct command *cmd, struct shallow_info *si)
 {
 	const char *name = cmd->ref_name;
-- 
2.2.0-141-gd3f4719

[PATCH 2/2] receive-pack: support push-to-checkout hook

[Junio C Hamano]

When receive.denyCurrentBranch is set to updateInstead, this hook
can be used to override the built-in "push-to-deploy" logic, which
insists that the working tree and the index must be unchanged
relative to HEAD.  The hook receives the commit with which the
tip of the current is going to be updated, and is responsible to
make any necessary changes to the working tree and to the index to
bring them to the desired state when the tip of the current branch
is updated to the new commit.

For example, the hook can simply run "git read-tree -u -m HEAD $1"
to the workflow to emulate "'git fetch' going in the reverse
direction with 'git push'" better than the push-to-deploy logic, as
the two-tree form of "read-tree -u -m" is essentially the same as
"git checkout" that switches branches while keeping the local
changes in the working tree that do not interfere with the
difference between the branches.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/receive-pack.c | 19 ++++++++++++++-
 t/t5516-fetch-push.sh  | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 81 insertions(+), 1 deletion(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 11800cd..fc8ec9c 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -797,6 +797,20 @@ static const char *push_to_deploy(unsigned char *sha1,
 	return NULL;
 }
 
+static const char *push_to_checkout_hook = "push-to-checkout";
+
+static const char *push_to_checkout(unsigned char *sha1,
+				    struct argv_array *env,
+				    const char *work_tree)
+{
+	argv_array_pushf(env, "GIT_WORK_TREE=%s", absolute_path(work_tree));
+	if (run_hook_le(env->argv, push_to_checkout_hook,
+			sha1_to_hex(sha1), NULL))
+		return "push-to-checkout hook declined";
+	else
+		return NULL;
+}
+
 static const char *update_worktree(unsigned char *sha1)
 {
 	const char *retval;
@@ -808,7 +822,10 @@ static const char *update_worktree(unsigned char *sha1)
 
 	argv_array_pushf(&env, "GIT_DIR=%s", absolute_path(get_git_dir()));
 
-	retval = push_to_deploy(sha1, &env, work_tree);
+	if (!find_hook(push_to_checkout_hook))
+		retval = push_to_deploy(sha1, &env, work_tree);
+	else
+		retval = push_to_checkout(sha1, &env, work_tree);
 
 	argv_array_clear(&env);
 	return retval;
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 85c7fec..e4436c1 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1434,4 +1434,67 @@ test_expect_success 'receive.denyCurrentBranch = updateInstead' '
 
 '
 
+test_expect_success 'updateInstead with push-to-checkout hook' '
+	rm -fr testrepo &&
+	git init testrepo &&
+	(
+		cd testrepo &&
+		git pull .. master &&
+		git reset --hard HEAD^^ &&
+		git tag initial &&
+		git config receive.denyCurrentBranch updateInstead &&
+		write_script .git/hooks/push-to-checkout <<-\EOF
+		echo >&2 updating from $(git rev-parse HEAD)
+		echo >&2 updating to "$1"
+
+		git update-index -q --refresh &&
+		git read-tree -u -m HEAD "$1" || {
+			status=$?
+			echo >&2 read-tree failed
+			exit $status
+		}
+		EOF
+	) &&
+
+	# Try pushing into a pristine
+	git push testrepo master &&
+	(
+		cd testrepo &&
+		git diff --quiet &&
+		git diff HEAD --quiet &&
+		test $(git -C .. rev-parse HEAD) = $(git rev-parse HEAD)
+	) &&
+
+	# Try pushing into a repository with conflicting change
+	(
+		cd testrepo &&
+		git reset --hard initial &&
+		echo conflicting >path2
+	) &&
+	test_must_fail git push testrepo master &&
+	(
+		cd testrepo &&
+		test $(git rev-parse initial) = $(git rev-parse HEAD) &&
+		test conflicting = "$(cat path2)" &&
+		git diff-index --quiet --cached HEAD
+	) &&
+
+	# Try pushing into a repository with unrelated change
+	(
+		cd testrepo &&
+		git reset --hard initial &&
+		echo unrelated >path1 &&
+		echo irrelevant >path5 &&
+		git add path5
+	) &&
+	git push testrepo master &&
+	(
+		cd testrepo &&
+		test "$(cat path1)" = unrelated &&
+		test "$(cat path5)" = irrelevant &&
+		test "$(git diff --name-only --cached HEAD)" = path5 &&
+		test $(git -C .. rev-parse HEAD) = $(git rev-parse HEAD)
+	)
+'
+
 test_done
-- 
2.2.0-141-gd3f4719

Re: [PATCH 2/2] receive-pack: support push-to-checkout hook

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3365 bytes --]

Hi Junio,

On Mon, 1 Dec 2014, Junio C Hamano wrote:

> When receive.denyCurrentBranch is set to updateInstead, this hook
> can be used to override the built-in "push-to-deploy" logic, which
> insists that the working tree and the index must be unchanged
> relative to HEAD.  The hook receives the commit with which the
> tip of the current is going to be updated, and is responsible to
> make any necessary changes to the working tree and to the index to
> bring them to the desired state when the tip of the current branch
> is updated to the new commit.
> 
> For example, the hook can simply run "git read-tree -u -m HEAD $1"
> to the workflow to emulate "'git fetch' going in the reverse
> direction with 'git push'" better than the push-to-deploy logic, as
> the two-tree form of "read-tree -u -m" is essentially the same as
> "git checkout" that switches branches while keeping the local
> changes in the working tree that do not interfere with the
> difference between the branches.

I like it.

The only sad part is that the already huge test suite is enlarged by yet
another extensive set of test cases (and those tests might not really
need to be that extensive because they essentially only need to make sure
that the hook is run successfully *instead* of trying to update the
working directory, i.e. a simple 'touch yep' hook would have been enough).
It starts to be painful to run the complete test suite, not only on
Windows (where this has been a multi-hour endeavor for me for ages
already). BuildHive (CloudBees' very kind offer of Jenkins CI for Open
Source, integrated conveniently with GitHub) already takes over an hour to
run the Git test suite – and BuildHive runs on Linux, not Windows!

That means that everytime I push into a GitHub Pull Request, I have to
wait for a full hour to know whether everything is groovy.

Worse: when Git for Windows contributors (yes! they exist!) push into
their Pull Requests, I have to wait for a full hour before I can merge,
unless I want to merge something that the test suite did not validate!

So maybe it is time to start thinking about conciser tests that verify the
bare minimum, especially for rarely exercised functionality? I mean,
testing is always a balance between too much and too little. And at this
point, I am afraid that several well-intended, but overly extensive tests
increase the overall runtime of "make test" to a point where developers
*avoid* running it, costing more time in the long run than necessary.

In this particular case, I think that we really, really *just* need to
verify that the presence of the hook switches off the default behavior of
updateInstead. *Nothing* else is needed to verify that this particular
functionality hasn't regressed. I.e. something like:

+test_expect_success 'updateInstead with push-to-checkout hook' '
+	rm -fr testrepo &&
+	git clone . testrepo &&
+	(
+		cd testrepo &&
+		echo unclean > path1 &&
+		git config receive.denyCurrentBranch updateInstead &&
+		echo 'touch yep' | write_script .git/hooks/push-to-checkout
+	) &&
+	git push testrepo HEAD^:refs/heads/master &&
+	test unclean = $(cat testrepo/path1) &&
+	test -f testrepo/yep
+'

would be more appropriate (although it probably has one or three bugs,
given that I wrote this in the mailer).

Ciao,
Johannes

Re: [PATCH 2/2] receive-pack: support push-to-checkout hook

[Michael J Gruber]

Johannes Schindelin schrieb am 02.12.2014 um 09:47:
> Hi Junio,
> 
> On Mon, 1 Dec 2014, Junio C Hamano wrote:
> 
>> When receive.denyCurrentBranch is set to updateInstead, this hook
>> can be used to override the built-in "push-to-deploy" logic, which
>> insists that the working tree and the index must be unchanged
>> relative to HEAD.  The hook receives the commit with which the
>> tip of the current is going to be updated, and is responsible to
>> make any necessary changes to the working tree and to the index to
>> bring them to the desired state when the tip of the current branch
>> is updated to the new commit.
>>
>> For example, the hook can simply run "git read-tree -u -m HEAD $1"
>> to the workflow to emulate "'git fetch' going in the reverse
>> direction with 'git push'" better than the push-to-deploy logic, as
>> the two-tree form of "read-tree -u -m" is essentially the same as
>> "git checkout" that switches branches while keeping the local
>> changes in the working tree that do not interfere with the
>> difference between the branches.
> 
> I like it.
> 
> The only sad part is that the already huge test suite is enlarged by yet
> another extensive set of test cases (and those tests might not really
> need to be that extensive because they essentially only need to make sure
> that the hook is run successfully *instead* of trying to update the
> working directory, i.e. a simple 'touch yep' hook would have been enough).
> It starts to be painful to run the complete test suite, not only on
> Windows (where this has been a multi-hour endeavor for me for ages
> already). BuildHive (CloudBees' very kind offer of Jenkins CI for Open
> Source, integrated conveniently with GitHub) already takes over an hour to
> run the Git test suite – and BuildHive runs on Linux, not Windows!
> 
> That means that everytime I push into a GitHub Pull Request, I have to
> wait for a full hour to know whether everything is groovy.
> 
> Worse: when Git for Windows contributors (yes! they exist!) push into
> their Pull Requests, I have to wait for a full hour before I can merge,
> unless I want to merge something that the test suite did not validate!
> 
> So maybe it is time to start thinking about conciser tests that verify the
> bare minimum, especially for rarely exercised functionality? I mean,
> testing is always a balance between too much and too little. And at this
> point, I am afraid that several well-intended, but overly extensive tests
> increase the overall runtime of "make test" to a point where developers
> *avoid* running it, costing more time in the long run than necessary.
> 
> In this particular case, I think that we really, really *just* need to
> verify that the presence of the hook switches off the default behavior of
> updateInstead. *Nothing* else is needed to verify that this particular
> functionality hasn't regressed. I.e. something like:
> 
> +test_expect_success 'updateInstead with push-to-checkout hook' '
> +	rm -fr testrepo &&
> +	git clone . testrepo &&
> +	(
> +		cd testrepo &&
> +		echo unclean > path1 &&
> +		git config receive.denyCurrentBranch updateInstead &&
> +		echo 'touch yep' | write_script .git/hooks/push-to-checkout
> +	) &&
> +	git push testrepo HEAD^:refs/heads/master &&
> +	test unclean = $(cat testrepo/path1) &&
> +	test -f testrepo/yep
> +'
> 
> would be more appropriate (although it probably has one or three bugs,
> given that I wrote this in the mailer).
> 
> Ciao,
> Johannes
> 

How about reusing the prerequisites feature for that? We could either
mark the minimal tests, or mark the others similar to how we do with the
(extra) expensive tests. Your config.mk would then determine which tests
are executed.

Michael

Re: [PATCH 2/2] receive-pack: support push-to-checkout hook

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1725 bytes --]

Hi Michael,

On Tue, 2 Dec 2014, Michael J Gruber wrote:

> Johannes Schindelin schrieb am 02.12.2014 um 09:47:
> 
> > The only sad part is that the already huge test suite is enlarged by
> > yet another extensive set of test cases (and those tests might not
> > really need to be that extensive because they essentially only need to
> > make sure that the hook is run successfully *instead* of trying to
> > update the working directory, i.e. a simple 'touch yep' hook would
> > have been enough).  It starts to be painful to run the complete test
> > suite, not only on Windows (where this has been a multi-hour endeavor
> > for me for ages already). BuildHive (CloudBees' very kind offer of
> > Jenkins CI for Open Source, integrated conveniently with GitHub)
> > already takes over an hour to run the Git test suite – and BuildHive
> > runs on Linux, not Windows!
> 
> How about reusing the prerequisites feature for that? We could either
> mark the minimal tests, or mark the others similar to how we do with the
> (extra) expensive tests. Your config.mk would then determine which tests
> are executed.

In general, you are correct. And we already have the test_have_prereq
EXPENSIVE precedent.

In this particular case, I question the value of the extent of the tests:
the only thing we really need to test is that the new hook really
overrides the default behavior, not all kinds of real-world simulations
that *use* that behavior.

In other words, it is my opinion that the difference between the "touch
yep" test I demonstrated and the test originally suggested is the amount
of time it takes to run, not the extent to which the new code ist actually
verified.

Ciao,
Johannes

Re: [PATCH 2/2] receive-pack: support push-to-checkout hook

[Junio C Hamano]

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

> In this particular case, I think that we really, really *just* need to
> verify that the presence of the hook switches off the default behavior of
> updateInstead. *Nothing* else is needed to verify that this particular
> functionality hasn't regressed. I.e. something like:
>
> +test_expect_success 'updateInstead with push-to-checkout hook' '
> +	rm -fr testrepo &&
> +	git clone . testrepo &&
> +	(
> +		cd testrepo &&
> +		echo unclean > path1 &&
> +		git config receive.denyCurrentBranch updateInstead &&
> +		echo 'touch yep' | write_script .git/hooks/push-to-checkout
> +	) &&
> +	git push testrepo HEAD^:refs/heads/master &&
> +	test unclean = $(cat testrepo/path1) &&
> +	test -f testrepo/yep
> +'
>
> would be more appropriate (although it probably has one or three bugs,
> given that I wrote this in the mailer).

Not really.  You need to remember that we write tests not to show
off the new shiny, but to protect essential invariants from being
broken by careless others attempting to rewrite the implementation
in the future.

What needs to be tested for the feature are at the minimum:

 * The hook is not triggered when denycurrent is set to
   updateInstead; the posted version does not even test this, but it
   should.

 * The hook is run with the right settings, so that git commands it
   runs will operate on the right repository and its working tree,
   but without overspecifying how that "right settings" happens [*1*].

 * Non-zero exit from the hook will fail "git push", and zero exit
   from the hook will allow "git push" to pass.

 * Whether the hook returns a success or a failure, the working tree
   and the index is in the state the hook expects to give the user
   (i.e. nobody else further munges the working tree and the index
   after hook returns) [*2*].

The patch I posted is minimum to do so.  Compared to that, the "yep"
test is not checking anything of the importance, and only insists on
a single immaterial detail (i.e. hook must be run after cd'ing to
the top of the working tree).

I fail to see why it could be more appropriate.


[Footnote]

*1* Your version above assumes that the hook must run in the working
    tree, but it does not have to, if GIT_DIR and GIT_WORK_TREE are
    both exported; your test is overspecifying what should happen,
    and will reject a legitimate implementation of the feature.

*2* A hook may muck with the index or with the working tree and then
    return a failure.  I do not offhand see why a failing push
    should be allowed to contaminate the working tree that way, but
    whatever the hook does is what the user wishes, and we should
    not lose data coming from that wish.  The hook the test uses
    refrains from touching the working tree or the index when it
    fails, so the test checks that the working tree and the index
    are left as-is when it happens.

Re: [PATCH 2/2] receive-pack: support push-to-checkout hook

[Johannes Schindelin]

Hi Junio,

On Tue, 2 Dec 2014, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > In this particular case, I think that we really, really *just* need to
> > verify that the presence of the hook switches off the default behavior of
> > updateInstead. *Nothing* else is needed to verify that this particular
> > functionality hasn't regressed. I.e. something like:
> >
> > +test_expect_success 'updateInstead with push-to-checkout hook' '
> > +	rm -fr testrepo &&
> > +	git clone . testrepo &&
> > +	(
> > +		cd testrepo &&
> > +		echo unclean > path1 &&
> > +		git config receive.denyCurrentBranch updateInstead &&
> > +		echo 'touch yep' | write_script .git/hooks/push-to-checkout
> > +	) &&
> > +	git push testrepo HEAD^:refs/heads/master &&
> > +	test unclean = $(cat testrepo/path1) &&
> > +	test -f testrepo/yep
> > +'
> >
> > would be more appropriate (although it probably has one or three bugs,
> > given that I wrote this in the mailer).
> 
> Not really.  You need to remember that we write tests not to show
> off the new shiny, but to protect essential invariants from being
> broken by careless others attempting to rewrite the implementation
> in the future.

Fair enough. You are the boss.

I am not, therefore it does not matter what I think,
Johannes

Re: [PATCH 2/2] receive-pack: support push-to-checkout hook

[Junio C Hamano]

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

>> Not really.  You need to remember that we write tests not to show
>> off the new shiny, but to protect essential invariants from being
>> broken by careless others attempting to rewrite the implementation
>> in the future.
>
> Fair enough. You are the boss.
>
> I am not, therefore it does not matter what I think,

It is not that it does not matter because you are not the boss; it
is just that when you are wrong, you are wrong.

You said in your response to Michael:

    the difference between the "touch yep" ... and
    the test originally suggested is ...
    not the extent to which the new code is actually verified.

If your "verification" is based on "faith", you are correct.  You
may be "verifying" the code to the right extent, i.e. "Yes, what I
wrote is actually run, and I write perfect code every time, so what
it does must be correct, as long as it gets run".

But I do not have faith in people who will be touching the relevant
code in the future; "Yes, it is triggered" is far from satisfactory
without faith like yours in the code being tested.

I actually do not have much faith in what I write myself ;-).  That
is why we have tests.

Re: [PATCH 2/2] receive-pack: support push-to-checkout hook

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1611 bytes --]

Hi Junio,

On Tue, 2 Dec 2014, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> Not really.  You need to remember that we write tests not to show
> >> off the new shiny, but to protect essential invariants from being
> >> broken by careless others attempting to rewrite the implementation
> >> in the future.
> >
> > Fair enough. You are the boss.
> >
> > I am not, therefore it does not matter what I think,
> 
> It is not that it does not matter because you are not the boss; it
> is just that when you are wrong, you are wrong.

Please, there is no need to get emotional, let alone personal.

I am not really interested in challenging your policy regarding the test
suite, even if it does hurt my development style where I want to run the
test suite frequently but its tests just take too long because their focus
is more on thoroughness rather than trying to save time in the manner I
suggested (i.e. by only lightly testing obscure code paths that will be
executed rarely, risking bugs in favor of adding tests when fixing said
bugs when – and if – they arise). There is nothing inherently wrong in the
way you want to have the test suite, it is a matter of preference, that is
all. I would like a more light-weight test suite that runs much faster,
you want a thorough one, even if it takes more time to run.

So: you are the boss, you do the things you do, and my opinion does not
matter. I say this most pragmatically, to save more time by ending this
discussion now. There are no hard feelings on my side.

Ciao,
Johannes

Re: [PATCH 2/2] receive-pack: support push-to-checkout hook

[Junio C Hamano]

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

> ... (i.e. by only lightly testing obscure code paths that will be
> executed rarely, risking bugs in favor of adding tests when fixing said
> bugs when – and if – they arise).

I'd like to learn a bit more about this part, not because I want to
say you are wrong, but because I want to find out what you say is
practically useful and can be adopted by the project.

The part I find most troublesome is this.  Without tests covering
"obscure" cases, how would you expect to notice when the codebase
regresses, at which point you plan to fix and add tests for the fix?

Not that I think the "minimally these need to be tested" list I sent
earlier are anything obscure (they are all essential part of the
feature; without them working, the feature would not be useful).

Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules

[Junio C Hamano]

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

> On Thu, 13 Nov 2014, Johannes Schindelin wrote:
>
>> Due to that experience, the documentation also states pretty clearly that
>> `updateInstead` succeeds only in updating the current branch if the
>> working directory is clean.
>
> To clarify why `updateInstead` is stricter than the `merge` scenario: when
> pushing into a remote repository with attached working directory, we
> cannot simply clean up merge conflicts or other problems introduced by a
> merge. Indeed, it is even possible to push without having any option to
> fix files in the working directory afterwards. Therefore, the
> `updateInstead` feature really needs to be adamant about the working
> directory being clean.

As the stricter check (which I think is unnecessarily strict and
which you don't) can be loosened later without making something the
user used to be able to do in an early version unable to do later,
I am OK to accept the design as-is.

But after reading this addendum, I feel the need to double check.

A "reset --keep <new-commit>", which is the same as "checkout -B
<current> <new-commit>" (note the lack of "-m"), does not have
any "merge conflict" issues.

To see what I mean, follow along this simple experiment.

    # Just make sure we start clean
    $ git reset --hard

    # Create a playpen
    $ git checkout -b throwaway master

    # Pick one random path that is different.  We pretend that
    # somebody pushed the commit at the tip of 'next' from the side
    $ git diff --name-only next | head -n 1
    Documentation/git-clone.txt

    # Make sure another random path used in the experiment is unchanged
    $ git diff next -- COPYING | wc -l
    0

    # Smudge a path not involved in the branch/HEAD switching
    $ echo >>COPYING

    # attempt to updateInstead to the other version succeeds.
    $ git reset --keep next; echo $?
    0
    $ git status -suno
     M COPYING
    # Notice that we did not get into a funny state.
    # You can verify it with "git diff".

    # Go back and try smudging what would need a real merge
    $ git reset --hard master
    $ echo >>Documentation/git-clone.txt

    # attempt to updateInstead to the other version
    $ git reset --keep next; echo $?
    error: Entry 'Documentation/git-clone.txt' not uptodate. Cannot merge.
    fatal: Could not reset index file to revision 'next'.
    128

    # See that HEAD did not move
    $ git log HEAD...master | wc -l
    0

    # See that the working tree change is intact
    $ git status -suno
     M Documentation/git-clone.txt
    # Notice that we did not get into a funny state.
    # You can verify it with "git diff".

The semantics is just as safe as "checkout <another-branch>" without
the "-m" option; if a local change may be clobbered or needs real
file-level merge, the operation fails without touching anything to
preserve the local state.

It is OK for us to agree to disagree, and I am willing to accept the
stricter design for an initial version because loosening it later is
possible without harming users.

But I wanted to first make sure that your disagreement is an
informed one.  Do you still feel the need for "absolutely clean",
even if you take the safety built into "reset --keep" shown in the
above demonstration into account?

Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules

[Johannes Schindelin]

Hi Junio,

On Mon, 10 Nov 2014, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> By the way, if the expected use case of updateInstead is what I
> >> outlined in the previous message, would it make more sense not to
> >> fail with "update-index --refresh" failure (i.e. the working tree
> >> files have no changes since the index)?
> >> 
> >> Thinking about it a bit more, checking with "update-index --refresh"
> >> feels doubly wrong.  You not just want the working tree files to be
> >> pristine with respect to the index, but also you do not want to see
> >> any change between the index and the original HEAD, i.e.
> >> 
> >> 	$ git reset --hard && echo >>Makefile ; git add Makefile
> >>         $ git update-index --refresh ; echo $?
> >>         0
> >> 
> >> this is not a good state from which you would want to update the
> >> working tree.
> >> 
> >> Wouldn't the two-tree form "read-tree -u -m" that is the equivalent
> >> to branch switching do a sufficient check?
> >
> > That is indeed what the patched code calls.
> 
> I know that ;-), but I think I wasn't clear enough.
> 
> I am not saying you are not using two-tree read-tree.  I am saying
> the check with update-index --refresh before read-tree seems
> dubious.
> 
> There are three "cleanliness" we require in various occasions:
> 
>  (1) rebase asks you to have your index and the working tree match
>      HEAD exactly, as if just after "reset --hard HEAD".
> 
>  (2) merge asks you to have your index match HEAD exactly (i.e. no
>      output from "diff --cached HEAD"), and have no changes in the
>      working tree at paths that are involved in the operation.  It
>      is OK to have local changes in the working tree (but not in the
>      index) at paths that are not involved in a mergy operation.
> 
>  (3) checkout asks you to have your index and the working tree match
>      either HEAD or the commit you are switching to exactly at paths
>      that are involved in the operation.  It is OK to have local
>      changes in the working tree and in the index at paths that are
>      not different between the commits you are moving between, and
>      it is also OK to have the contents from the "new" commit in the
>      working tree and the index at paths that are different between
>      the two.
> 
> Dying when "update-index --refresh" signals a difference is an
> attempt to mimic #1, but it is in line with the spirit of the reason
> why a user would want to use updateInstead, I think.  The situation
> is more like the person who pushed into your repository from
> sideline did a "checkout -B $current_branch $new_commit" to update
> the HEAD, the index and the working tree, to let you pretend as if
> you based your work on the commit he pushed to you.
> 
> While you still need to error out when your local work does not
> satisfy the cleanliness criteria #3 above, I do not think you would
> want to stop the operation when "checkout" would not fail, e.g. you
> have a local change that does not interfere with the update between
> the two commits, with this one:
> 
> +	if (run_command(&child))
> +		die ("Could not refresh the index");
> 
> When refreshed the index successfully, we signal that there were
> differences between the index and the working tree with a non-zero
> return value, so "Could not refresh" is not quite right, either.
> 
> But this one that checks the exit status from two-tree read-tree
> 
> +	if (run_command(&child))
> +		die ("Could not merge working tree with new HEAD.  Good luck.");
> 
> is checking the right condition, i.e. cleanliness #3.  The
> disposition should not be "die", but an error return to tell the
> caller to abort the push as we discussed earlier.

I have to say that I am quite puzzled now: I guess that you want me to
drop the update-index --refresh call?

Ciao,
Johannes

[PATCH v2 0/2] Support updating working trees when pushing into non-bare repos

[Johannes Schindelin]

This patch series adds support for two new receive.denyCurrentBranch settings:
one to update the working directory (which must be clean, i.e. there must not
be any uncommitted changes) when pushing into the current branch, the other
setting detaches the HEAD instead.

The scenario in which in particular the 'updateInstead' setting became a
boon in this developer's daily work is when trying to get a bug fix from a
Windows computer, a virtual machine or a user's machine onto his main
machine (in all of those cases it is only possible to connect via ssh in one
direction, but not in the reverse direction).

Interdiff vs v1 below the diffstat.

Johannes Schindelin (2):
  Clean stale environment pointer in finish_command()
  Add a few more options for receive.denyCurrentBranch

 Documentation/config.txt |  9 ++++++++
 builtin/receive-pack.c   | 60 ++++++++++++++++++++++++++++++++++++++++++++++--
 run-command.c            |  3 +++
 t/t5516-fetch-push.sh    | 36 +++++++++++++++++++++++++++++
 4 files changed, 106 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 6fc0d6d..fc9b8db 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2130,10 +2130,14 @@ receive.denyCurrentBranch::
 	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.
+Another option is "updateInstead" which will update the working
+directory (must be clean) if pushing into the current branch. This option is
+intended for synchronizing working directories when one side is not easily
+accessible via ssh (e.g. inside a VM).
++
+Yet another option is "detachInstead" which will detach the HEAD if updates
+are pushed into the current branch; That way, the current revision, the
+index and the working directory are always left untouched by pushes.
 
 receive.denyNonFastForwards::
 	If set to true, git-receive-pack will deny a ref update which is
diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
index 2b2d797..7db860f 100755
--- a/GIT-VERSION-GEN
+++ b/GIT-VERSION-GEN
@@ -1,7 +1,7 @@
 #!/bin/sh
 
 GVF=GIT-VERSION-FILE
-DEF_VER=v2.2.0-rc0
+DEF_VER=v2.2.0-rc1
 
 LF='
 '
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 4ba51df..4534e88 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -28,7 +28,7 @@ enum deny_action {
 	DENY_WARN,
 	DENY_REFUSE,
 	DENY_UPDATE_INSTEAD,
-	DENY_DETACH_INSTEAD,
+	DENY_DETACH_INSTEAD
 };
 
 static int deny_deletes;
@@ -737,7 +737,7 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
 	return 0;
 }
 
-static void merge_worktree(unsigned char *sha1)
+static const char *merge_worktree(unsigned char *sha1)
 {
 	const char *update_refresh[] = {
 		"update-index", "--ignore-submodules", "--refresh", NULL
@@ -745,41 +745,36 @@ static void merge_worktree(unsigned char *sha1)
 	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];
+	struct child_process child = CHILD_PROCESS_INIT;
 
 	if (is_bare_repository())
-		die ("denyCurrentBranch = updateInstead needs a worktree");
-
-	strbuf_addf(&git_env, "GIT_DIR=%s", absolute_path(get_git_dir()));
-	env[0] = git_env.buf;
-	env[1] = NULL;
+		return "denyCurrentBranch = updateInstead needs a worktree";
 
-	memset(&child, 0, sizeof(child));
+	argv_array_pushf(&child.env_array, "GIT_DIR=%s", absolute_path(get_git_dir()));
 	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");
+		die("Could not refresh the index");
 
+	/* finish_command cleared the environment; reinitialize */
+	argv_array_pushf(&child.env_array, "GIT_DIR=%s", absolute_path(get_git_dir()));
 	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.");
+		die("Could not merge working tree with new HEAD.");
 
-	strbuf_release(&git_env);
+	return NULL;
 }
 
 static const char *update(struct command *cmd, struct shallow_info *si)
 {
 	const char *name = cmd->ref_name;
 	struct strbuf namespaced_name_buf = STRBUF_INIT;
-	const char *namespaced_name;
+	const char *namespaced_name, *ret;
 	unsigned char *old_sha1 = cmd->old_sha1;
 	unsigned char *new_sha1 = cmd->new_sha1;
 
@@ -806,11 +801,17 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 				refuse_unconfigured_deny();
 			return "branch is currently checked out";
 		case DENY_UPDATE_INSTEAD:
-			merge_worktree(new_sha1);
+			ret = merge_worktree(new_sha1);
+			if (ret)
+				return ret;
 			break;
 		case DENY_DETACH_INSTEAD:
-			update_ref("push into current branch (detach)", "HEAD",
-				old_sha1, NULL, REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
+			ret = update_ref("push into current branch (detach)",
+				"HEAD", old_sha1, NULL, REF_NODEREF,
+				UPDATE_REFS_DIE_ON_ERR) ?
+				"Could not detach HEAD" : NULL;
+			if (ret)
+				return ret;
 			break;
 		}
 	}
diff --git a/run-command.c b/run-command.c
index 79a0a76..85578da 100644
--- a/run-command.c
+++ b/run-command.c
@@ -555,6 +555,9 @@ int finish_command(struct child_process *cmd)
 {
 	int ret = wait_or_whine(cmd->pid, cmd->argv[0]);
 	argv_array_clear(&cmd->args);
+	/* Avoid pointing to a stale environment */
+	if (cmd->env == cmd->env_array.argv)
+		cmd->env = NULL;
 	argv_array_clear(&cmd->env_array);
 	return ret;
 }

-- 
2.0.0.rc3.9669.g840d1f9

[PATCH v3 0/1] Support updating working trees when pushing into non-bare repos

[Johannes Schindelin]

This patch series adds support for a new receive.denyCurrentBranch setting
to update the working directory (which must be clean, i.e. there must not be
any uncommitted changes) when pushing into the current branch.

The scenario in which the 'updateInstead' setting became a boon in this
developer's daily work is when trying to get a bug fix from a Windows
computer, a virtual machine or a user's machine onto his main machine (in
all of those cases it is only possible to connect via ssh in one direction,
but not in the reverse direction).

Interdiff vs v2 below the diffstat.

Johannes Schindelin (1):
  Add another option for receive.denyCurrentBranch

 Documentation/config.txt |  5 ++++
 builtin/receive-pack.c   | 78 ++++++++++++++++++++++++++++++++++++++++++++++--
 t/t5516-fetch-push.sh    | 17 +++++++++++
 3 files changed, 98 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 4f9fe81..c384515 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2134,10 +2134,6 @@ Another option is "updateInstead" which will update the working
 directory (must be clean) if pushing into the current branch. This option is
 intended for synchronizing working directories when one side is not easily
 accessible via ssh (e.g. inside a VM).
-+
-Yet another option is "detachInstead" which will detach the HEAD if updates
-are pushed into the current branch; That way, the current revision, the
-index and the working directory are always left untouched by pushes.
 
 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 4534e88..836720f 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -27,8 +27,7 @@ enum deny_action {
 	DENY_IGNORE,
 	DENY_WARN,
 	DENY_REFUSE,
-	DENY_UPDATE_INSTEAD,
-	DENY_DETACH_INSTEAD
+	DENY_UPDATE_INSTEAD
 };
 
 static int deny_deletes;
@@ -78,6 +77,8 @@ static enum deny_action parse_deny_action(const char *var, const char *value)
 			return DENY_WARN;
 		if (!strcasecmp(value, "refuse"))
 			return DENY_REFUSE;
+		if (!strcasecmp(value, "updateinstead"))
+			return DENY_UPDATE_INSTEAD;
 	}
 	if (git_config_bool(var, value))
 		return DENY_REFUSE;
@@ -122,12 +123,7 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
 	}
 
 	if (!strcmp(var, "receive.denycurrentbranch")) {
-		if (value && !strcasecmp(value, "updateinstead"))
-			deny_current_branch = DENY_UPDATE_INSTEAD;
-		else if (value && !strcasecmp(value, "detachinstead"))
-			deny_current_branch = DENY_DETACH_INSTEAD;
-		else
-			deny_current_branch = parse_deny_action(var, value);
+		deny_current_branch = parse_deny_action(var, value);
 		return 0;
 	}
 
@@ -737,36 +733,66 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
 	return 0;
 }
 
-static const char *merge_worktree(unsigned char *sha1)
+static const char *update_worktree(unsigned char *sha1)
 {
 	const char *update_refresh[] = {
 		"update-index", "--ignore-submodules", "--refresh", NULL
 	};
+	const char *diff_index[] = {
+		"diff-index", "--quiet", "--cached", "--ignore-submodules",
+		"HEAD", "--", NULL
+	};
 	const char *read_tree[] = {
 		"read-tree", "-u", "-m", sha1_to_hex(sha1), NULL
 	};
+	const char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : "..";
+	struct argv_array env = ARGV_ARRAY_INIT;
 	struct child_process child = CHILD_PROCESS_INIT;
 
 	if (is_bare_repository())
 		return "denyCurrentBranch = updateInstead needs a worktree";
 
-	argv_array_pushf(&child.env_array, "GIT_DIR=%s", absolute_path(get_git_dir()));
+	argv_array_pushf(&env, "GIT_DIR=%s", absolute_path(get_git_dir()));
+
 	child.argv = update_refresh;
-	child.dir = git_work_tree_cfg ? git_work_tree_cfg : "..";
+	child.env = env.argv;
+	child.dir = work_tree;
+	child.no_stdin = 1;
 	child.stdout_to_stderr = 1;
 	child.git_cmd = 1;
-	if (run_command(&child))
-		die("Could not refresh the index");
+	if (run_command(&child)) {
+		argv_array_clear(&env);
+		return "Up-to-date check failed";
+	}
 
-	/* finish_command cleared the environment; reinitialize */
-	argv_array_pushf(&child.env_array, "GIT_DIR=%s", absolute_path(get_git_dir()));
+	/* run_command() does not clean up completely; reinitialize */
+	child_process_init(&child);
+	child.argv = diff_index;
+	child.env = env.argv;
+	child.no_stdin = 1;
+	child.no_stdout = 1;
+	child.stdout_to_stderr = 0;
+	child.git_cmd = 1;
+	if (run_command(&child)) {
+		argv_array_clear(&env);
+		return "Working directory not clean";
+	}
+
+	/* run_command() does not clean up completely; reinitialize */
+	child_process_init(&child);
 	child.argv = read_tree;
+	child.env = env.argv;
+	child.dir = work_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.");
+	child.git_cmd = 1;
+	if (run_command(&child)) {
+		argv_array_clear(&env);
+		return "Could not update working tree to new HEAD";
+	}
 
+	argv_array_clear(&env);
 	return NULL;
 }
 
@@ -801,15 +827,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 				refuse_unconfigured_deny();
 			return "branch is currently checked out";
 		case DENY_UPDATE_INSTEAD:
-			ret = merge_worktree(new_sha1);
-			if (ret)
-				return ret;
-			break;
-		case DENY_DETACH_INSTEAD:
-			ret = update_ref("push into current branch (detach)",
-				"HEAD", old_sha1, NULL, REF_NODEREF,
-				UPDATE_REFS_DIE_ON_ERR) ?
-				"Could not detach HEAD" : NULL;
+			ret = update_worktree(new_sha1);
 			if (ret)
 				return ret;
 			break;
@@ -837,12 +855,13 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 				break;
 			case DENY_REFUSE:
 			case DENY_UNCONFIGURED:
+			case DENY_UPDATE_INSTEAD:
 				if (deny_delete_current == DENY_UNCONFIGURED)
 					refuse_unconfigured_deny_delete_current();
 				rp_error("refusing to delete the current branch: %s", name);
 				return "deletion of the current branch prohibited";
 			default:
-				die ("Invalid denyDeleteCurrent setting");
+				return "Invalid denyDeleteCurrent setting";
 			}
 		}
 	}
diff --git a/run-command.c b/run-command.c
index 551c6c2..a476999 100644
--- a/run-command.c
+++ b/run-command.c
@@ -555,9 +555,6 @@ int finish_command(struct child_process *cmd)
 {
 	int ret = wait_or_whine(cmd->pid, cmd->argv[0]);
 	argv_array_clear(&cmd->args);
-	/* Avoid pointing to a stale environment */
-	if (cmd->env == cmd->env_array.argv)
-		cmd->env = NULL;
 	argv_array_clear(&cmd->env_array);
 	return ret;
 }
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 3981d1b..ba002a9 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1347,23 +1347,4 @@ test_expect_success 'receive.denyCurrentBranch = updateInstead' '
 	)
 '
 
-test_expect_success 'receive.denyCurrentBranch = detachInstead' '
-	(cd testrepo &&
-		git reset --hard &&
-		git config receive.denyCurrentBranch detachInstead
-	) &&
-	OLDHEAD=$(cd testrepo && git rev-parse HEAD) &&
-	test_commit fourth path2 &&
-	test fourth = "$(cat path2)" &&
-	git push testrepo master &&
-	test $OLDHEAD = $(cd testrepo && git rev-parse HEAD) &&
-	test fourth != "$(cat testrepo/path2)" &&
-	(cd testrepo &&
-		test_must_fail git symbolic-ref HEAD &&
-		git update-index --refresh &&
-		git diff-files --quiet &&
-		git diff-index --cached HEAD --
-	)
-'
-
 test_done

-- 
2.0.0.rc3.9669.g840d1f9

[PATCH v3 1/1] Add another option for receive.denyCurrentBranch

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 6199 bytes --]

When synchronizing between working directories, it can be handy to update
the current branch via 'push' rather than 'pull', e.g. when pushing a fix
from inside a VM, or when pushing a fix made on a user's machine (where
the developer is not at liberty to install an ssh daemon let alone know
the user's password).

The common workaround – pushing into a temporary branch and then merging
on the other machine – is no longer necessary with this patch.

The new option is:

'updateInstead':
	Update the working tree accordingly, but refuse to do so if there
	are any uncommitted changes.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/config.txt |  5 ++++
 builtin/receive-pack.c   | 78 ++++++++++++++++++++++++++++++++++++++++++++++--
 t/t5516-fetch-push.sh    | 17 +++++++++++
 3 files changed, 98 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9220725..c384515 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".
++
+Another option is "updateInstead" which will update the working
+directory (must be clean) if pushing into the current branch. This option is
+intended for synchronizing working directories when one side is not easily
+accessible via ssh (e.g. inside a VM).
 
 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..836720f 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -26,7 +26,8 @@ enum deny_action {
 	DENY_UNCONFIGURED,
 	DENY_IGNORE,
 	DENY_WARN,
-	DENY_REFUSE
+	DENY_REFUSE,
+	DENY_UPDATE_INSTEAD
 };
 
 static int deny_deletes;
@@ -76,6 +77,8 @@ static enum deny_action parse_deny_action(const char *var, const char *value)
 			return DENY_WARN;
 		if (!strcasecmp(value, "refuse"))
 			return DENY_REFUSE;
+		if (!strcasecmp(value, "updateinstead"))
+			return DENY_UPDATE_INSTEAD;
 	}
 	if (git_config_bool(var, value))
 		return DENY_REFUSE;
@@ -730,11 +733,74 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
 	return 0;
 }
 
+static const char *update_worktree(unsigned char *sha1)
+{
+	const char *update_refresh[] = {
+		"update-index", "--ignore-submodules", "--refresh", NULL
+	};
+	const char *diff_index[] = {
+		"diff-index", "--quiet", "--cached", "--ignore-submodules",
+		"HEAD", "--", NULL
+	};
+	const char *read_tree[] = {
+		"read-tree", "-u", "-m", sha1_to_hex(sha1), NULL
+	};
+	const char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : "..";
+	struct argv_array env = ARGV_ARRAY_INIT;
+	struct child_process child = CHILD_PROCESS_INIT;
+
+	if (is_bare_repository())
+		return "denyCurrentBranch = updateInstead needs a worktree";
+
+	argv_array_pushf(&env, "GIT_DIR=%s", absolute_path(get_git_dir()));
+
+	child.argv = update_refresh;
+	child.env = env.argv;
+	child.dir = work_tree;
+	child.no_stdin = 1;
+	child.stdout_to_stderr = 1;
+	child.git_cmd = 1;
+	if (run_command(&child)) {
+		argv_array_clear(&env);
+		return "Up-to-date check failed";
+	}
+
+	/* run_command() does not clean up completely; reinitialize */
+	child_process_init(&child);
+	child.argv = diff_index;
+	child.env = env.argv;
+	child.no_stdin = 1;
+	child.no_stdout = 1;
+	child.stdout_to_stderr = 0;
+	child.git_cmd = 1;
+	if (run_command(&child)) {
+		argv_array_clear(&env);
+		return "Working directory not clean";
+	}
+
+	/* run_command() does not clean up completely; reinitialize */
+	child_process_init(&child);
+	child.argv = read_tree;
+	child.env = env.argv;
+	child.dir = work_tree;
+	child.no_stdin = 1;
+	child.no_stdout = 1;
+	child.stdout_to_stderr = 0;
+	child.git_cmd = 1;
+	if (run_command(&child)) {
+		argv_array_clear(&env);
+		return "Could not update working tree to new HEAD";
+	}
+
+	argv_array_clear(&env);
+	return NULL;
+}
+
 static const char *update(struct command *cmd, struct shallow_info *si)
 {
 	const char *name = cmd->ref_name;
 	struct strbuf namespaced_name_buf = STRBUF_INIT;
-	const char *namespaced_name;
+	const char *namespaced_name, *ret;
 	unsigned char *old_sha1 = cmd->old_sha1;
 	unsigned char *new_sha1 = cmd->new_sha1;
 
@@ -760,6 +826,11 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 			if (deny_current_branch == DENY_UNCONFIGURED)
 				refuse_unconfigured_deny();
 			return "branch is currently checked out";
+		case DENY_UPDATE_INSTEAD:
+			ret = update_worktree(new_sha1);
+			if (ret)
+				return ret;
+			break;
 		}
 	}
 
@@ -784,10 +855,13 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 				break;
 			case DENY_REFUSE:
 			case DENY_UNCONFIGURED:
+			case DENY_UPDATE_INSTEAD:
 				if (deny_delete_current == DENY_UNCONFIGURED)
 					refuse_unconfigured_deny_delete_current();
 				rp_error("refusing to delete the current branch: %s", name);
 				return "deletion of the current branch prohibited";
+			default:
+				return "Invalid denyDeleteCurrent setting";
 			}
 		}
 	}
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index f4da20a..ba002a9 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1330,4 +1330,21 @@ test_expect_success 'fetch into bare respects core.logallrefupdates' '
 	)
 '
 
+test_expect_success 'receive.denyCurrentBranch = updateInstead' '
+	git push testrepo master &&
+	(cd testrepo &&
+		git reset --hard &&
+		git config receive.denyCurrentBranch updateInstead
+	) &&
+	test_commit third path2 &&
+	git push testrepo master &&
+	test $(git rev-parse HEAD) = $(cd testrepo && git rev-parse HEAD) &&
+	test third = "$(cat testrepo/path2)" &&
+	(cd testrepo &&
+		git update-index --refresh &&
+		git diff-files --quiet &&
+		git diff-index --cached HEAD --
+	)
+'
+
 test_done
-- 
2.0.0.rc3.9669.g840d1f9

Re: [PATCH v3 1/1] Add another option for receive.denyCurrentBranch

[Junio C Hamano]

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

> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index f4da20a..ba002a9 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1330,4 +1330,21 @@ test_expect_success 'fetch into bare respects core.logallrefupdates' '
>  	)
>  '
>  
> +test_expect_success 'receive.denyCurrentBranch = updateInstead' '
> +	git push testrepo master &&
> +	(cd testrepo &&
> +		git reset --hard &&
> +		git config receive.denyCurrentBranch updateInstead
> +	) &&
> +	test_commit third path2 &&
> +	git push testrepo master &&
> +	test $(git rev-parse HEAD) = $(cd testrepo && git rev-parse HEAD) &&
> +	test third = "$(cat testrepo/path2)" &&
> +	(cd testrepo &&
> +		git update-index --refresh &&
> +		git diff-files --quiet &&
> +		git diff-index --cached HEAD --
> +	)
> +'
> +

This new feature has two sides.  Update when we can and more
importantly fail the update safely.  This tests the "success" case,
but not the "safely fail" one.

For the latter "test_must_fail git push" on the sending side, and
"original HEAD stays the same and the working tree changes are
preserved when there are local changes before the push" on the
receiving side needs to be tested.

Thanks.

Re: [PATCH v3 1/1] Add another option for receive.denyCurrentBranch

[Johannes Schindelin]

Hi Junio,

On Thu, 13 Nov 2014, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> > index f4da20a..ba002a9 100755
> > --- a/t/t5516-fetch-push.sh
> > +++ b/t/t5516-fetch-push.sh
> > @@ -1330,4 +1330,21 @@ test_expect_success 'fetch into bare respects core.logallrefupdates' '
> >  	)
> >  '
> >  
> > +test_expect_success 'receive.denyCurrentBranch = updateInstead' '
> > +	git push testrepo master &&
> > +	(cd testrepo &&
> > +		git reset --hard &&
> > +		git config receive.denyCurrentBranch updateInstead
> > +	) &&
> > +	test_commit third path2 &&
> > +	git push testrepo master &&
> > +	test $(git rev-parse HEAD) = $(cd testrepo && git rev-parse HEAD) &&
> > +	test third = "$(cat testrepo/path2)" &&
> > +	(cd testrepo &&
> > +		git update-index --refresh &&
> > +		git diff-files --quiet &&
> > +		git diff-index --cached HEAD --
> > +	)
> > +'
> > +
> 
> This new feature has two sides.  Update when we can and more
> importantly fail the update safely.  This tests the "success" case,
> but not the "safely fail" one.
> 
> For the latter "test_must_fail git push" on the sending side, and
> "original HEAD stays the same and the working tree changes are
> preserved when there are local changes before the push" on the
> receiving side needs to be tested.

Right.

I have amended this for the upcoming v4 (but I'll wait whether there are
other things I need to change before submitting that):

-- snipsnap --
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index ba002a9..b8df39c 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1343,8 +1343,12 @@ test_expect_success 'receive.denyCurrentBranch =
updateInstead' '
 	(cd testrepo &&
 		git update-index --refresh &&
 		git diff-files --quiet &&
-		git diff-index --cached HEAD --
-	)
+		git diff-index --cached HEAD -- &&
+		echo changed > path2 &&
+		git add path2
+	) &&
+	test_commit fourth path2 &&
+	test_must_fail git push testrepo master
 '
 
 test_done

Re: [PATCH v3 0/1] Support updating working trees when pushing into non-bare repos

[Junio C Hamano]

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

> This patch series adds support for a new receive.denyCurrentBranch setting
> to update the working directory (which must be clean, i.e. there must not be
> any uncommitted changes) when pushing into the current branch.
>
> The scenario in which the 'updateInstead' setting became a boon in this
> developer's daily work is when trying to get a bug fix from a Windows
> computer, a virtual machine or a user's machine onto his main machine (in
> all of those cases it is only possible to connect via ssh in one direction,
> but not in the reverse direction).
>
> Interdiff vs v2 below the diffstat.
>
> Johannes Schindelin (1):
>   Add another option for receive.denyCurrentBranch
>
>  Documentation/config.txt |  5 ++++
>  builtin/receive-pack.c   | 78 ++++++++++++++++++++++++++++++++++++++++++++++--
>  t/t5516-fetch-push.sh    | 17 +++++++++++
>  3 files changed, 98 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 4f9fe81..c384515 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2134,10 +2134,6 @@ Another option is "updateInstead" which will update the working
>  directory (must be clean) if pushing into the current branch. This option is
>  intended for synchronizing working directories when one side is not easily
>  accessible via ssh (e.g. inside a VM).
> -+
> -Yet another option is "detachInstead" which will detach the HEAD if updates
> -are pushed into the current branch; That way, the current revision, the
> -index and the working directory are always left untouched by pushes.

I think we had an exchange to clarify the workflow in which
updateInstead is useful and how to help readers, but I do not see
any change on that in this part of documentation.  Forgot to revise?

> @@ -737,36 +733,66 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
>  	return 0;
>  }
>  
> -static const char *merge_worktree(unsigned char *sha1)
> +static const char *update_worktree(unsigned char *sha1)
>  {
>  	const char *update_refresh[] = {
>  		"update-index", "--ignore-submodules", "--refresh", NULL
>  	};
> +	const char *diff_index[] = {
> +		"diff-index", "--quiet", "--cached", "--ignore-submodules",
> +		"HEAD", "--", NULL
> +	};
>  	const char *read_tree[] = {
>  		"read-tree", "-u", "-m", sha1_to_hex(sha1), NULL
>  	};

OK.

"update-index --refresh && diff-files && diff-index --cached" is how
we traditionally ensure the working tree is absolutely clean (see
require_clean_work_tree in git-sh-setup.sh), but I do not think of a
reason why diff-files step is not redundant.  As a totally separate
topic outside this series, we may want to visit that shell function.

Re: [PATCH v3 0/1] Support updating working trees when pushing into non-bare repos

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> -static const char *merge_worktree(unsigned char *sha1)
>> +static const char *update_worktree(unsigned char *sha1)
>>  {
>>  	const char *update_refresh[] = {
>>  		"update-index", "--ignore-submodules", "--refresh", NULL
>>  	};
>> +	const char *diff_index[] = {
>> +		"diff-index", "--quiet", "--cached", "--ignore-submodules",
>> +		"HEAD", "--", NULL
>> +	};
>>  	const char *read_tree[] = {
>>  		"read-tree", "-u", "-m", sha1_to_hex(sha1), NULL
>>  	};
>
> OK.
>
> "update-index --refresh && diff-files && diff-index --cached" is how
> we traditionally ensure the working tree is absolutely clean (see
> require_clean_work_tree in git-sh-setup.sh), but I do not think of a
> reason why diff-files step is not redundant.  As a totally separate
> topic outside this series, we may want to visit that shell function.

Ahh, scratch that.  Over there we use 'update-index -q --refresh',
so the difference between the index and the working tree is not
noticed and we do need diff-files.  Here, the code runs update-index
without -q, so we do catch such a difference without diff-files.

Sorry for the noise ;-)

Re: [PATCH v3 0/1] Support updating working trees when pushing into non-bare repos

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4019 bytes --]

Hi Junio,

On Thu, 13 Nov 2014, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > This patch series adds support for a new receive.denyCurrentBranch setting
> > to update the working directory (which must be clean, i.e. there must not be
> > any uncommitted changes) when pushing into the current branch.
> >
> > The scenario in which the 'updateInstead' setting became a boon in this
> > developer's daily work is when trying to get a bug fix from a Windows
> > computer, a virtual machine or a user's machine onto his main machine (in
> > all of those cases it is only possible to connect via ssh in one direction,
> > but not in the reverse direction).
> >
> > Interdiff vs v2 below the diffstat.
> >
> > Johannes Schindelin (1):
> >   Add another option for receive.denyCurrentBranch
> >
> >  Documentation/config.txt |  5 ++++
> >  builtin/receive-pack.c   | 78 ++++++++++++++++++++++++++++++++++++++++++++++--
> >  t/t5516-fetch-push.sh    | 17 +++++++++++
> >  3 files changed, 98 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/config.txt b/Documentation/config.txt
> > index 4f9fe81..c384515 100644
> > --- a/Documentation/config.txt
> > +++ b/Documentation/config.txt
> > @@ -2134,10 +2134,6 @@ Another option is "updateInstead" which will update the working
> >  directory (must be clean) if pushing into the current branch. This option is
> >  intended for synchronizing working directories when one side is not easily
> >  accessible via ssh (e.g. inside a VM).
> > -+
> > -Yet another option is "detachInstead" which will detach the HEAD if updates
> > -are pushed into the current branch; That way, the current revision, the
> > -index and the working directory are always left untouched by pushes.
> 
> I think we had an exchange to clarify the workflow in which
> updateInstead is useful and how to help readers, but I do not see
> any change on that in this part of documentation.  Forgot to revise?

I had revised it for v2 already. It now reads:

-- snip --
Another option is "updateInstead" which will update the working directory
(must be clean) if pushing into the current branch. This option is
intended for synchronizing working directories when one side is not easily
accessible via ssh (e.g. inside a VM).
-- snap --

In my mind, this strikes the balance between sketching a scenario where
the setting makes sense on the one hand and abducting config.txt to tell
my life's story on the other.

> > @@ -737,36 +733,66 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
> >  	return 0;
> >  }
> >  
> > -static const char *merge_worktree(unsigned char *sha1)
> > +static const char *update_worktree(unsigned char *sha1)
> >  {
> >  	const char *update_refresh[] = {
> >  		"update-index", "--ignore-submodules", "--refresh", NULL
> >  	};
> > +	const char *diff_index[] = {
> > +		"diff-index", "--quiet", "--cached", "--ignore-submodules",
> > +		"HEAD", "--", NULL
> > +	};
> >  	const char *read_tree[] = {
> >  		"read-tree", "-u", "-m", sha1_to_hex(sha1), NULL
> >  	};
> 
> OK.
> 
> "update-index --refresh && diff-files && diff-index --cached" is how
> we traditionally ensure the working tree is absolutely clean (see
> require_clean_work_tree in git-sh-setup.sh), but I do not think of a
> reason why diff-files step is not redundant.

I fear that my double-negation-fu is still stuck somewhere in dreamland.
Do you mean to say "I could imagine that the diff-files step is
redundant"? If that is what you are telling me, then your explanation of
the exit code of update-index --refresh would suggest so, and so would
https://github.com/git/git/blob/f5709437/read-cache.c#L1201-L1230 *except*
in the case where refresh_cache_ent() returns an updated cache entry:
https://github.com/git/git/blob/f5709437/read-cache.c#L1116-L1131 – but I
could not figure out quickly when this code path is hit.

Ciao,
Johannes

[PATCH v4] Support updating working trees when pushing into non-bare repos

[Johannes Schindelin]

This patch "series" adds support for a new receive.denyCurrentBranch setting
to update the working directory (which must be clean, i.e. there must not be
any uncommitted changes) when pushing into the current branch.

The scenario in which the 'updateInstead' setting became a boon in this
developer's daily work is when trying to get a bug fix from a Windows
computer, a virtual machine, or when getting a bug fix from a user's machine
onto his main machine (in all of those cases it is only possible to connect
via ssh in one direction, but not in the reverse direction). It also comes
in handy when updating a live web site via push (in which case a clean
working directory is an absolute must).

As to the name 'updateInstead': since I do not want the option to perform
the equivalent of a checkout (where staged changes would be okay), I stuck
to the name I use in all of my $HOME/.gitconfigs. Hopefully you don't
mind.

Interdiff vs v3 below the diffstat

Johannes Schindelin (1):
  Add another option for receive.denyCurrentBranch

 Documentation/config.txt |  7 ++++
 builtin/receive-pack.c   | 94 ++++++++++++++++++++++++++++++++++++++++++++++--
 t/t5516-fetch-push.sh    | 21 +++++++++++
 3 files changed, 120 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index c384515..0519073 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2133,7 +2133,9 @@ receive.denyCurrentBranch::
 Another option is "updateInstead" which will update the working
 directory (must be clean) if pushing into the current branch. This option is
 intended for synchronizing working directories when one side is not easily
-accessible via ssh (e.g. inside a VM).
+accessible via interactive ssh (e.g. a live web site, hence the requirement
+that the working directory be clean). This mode also comes in handy when
+developing inside a VM to test and fix code on different Operating Systems.
 
 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 836720f..1b05e4e 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -736,7 +736,10 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
 static const char *update_worktree(unsigned char *sha1)
 {
 	const char *update_refresh[] = {
-		"update-index", "--ignore-submodules", "--refresh", NULL
+		"update-index", "--ignore-submodules", "--refresh", "-q", NULL
+	};
+	const char *diff_files[] = {
+		"diff-files", "--quiet", "--ignore-submodules", "--", NULL
 	};
 	const char *diff_index[] = {
 		"diff-index", "--quiet", "--cached", "--ignore-submodules",
@@ -767,6 +770,19 @@ static const char *update_worktree(unsigned char *sha1)
 
 	/* run_command() does not clean up completely; reinitialize */
 	child_process_init(&child);
+	child.argv = diff_files;
+	child.env = env.argv;
+	child.dir = work_tree;
+	child.no_stdin = 1;
+	child.stdout_to_stderr = 1;
+	child.git_cmd = 1;
+	if (run_command(&child)) {
+		argv_array_clear(&env);
+		return "Working directory not clean";
+	}
+
+	/* run_command() does not clean up completely; reinitialize */
+	child_process_init(&child);
 	child.argv = diff_index;
 	child.env = env.argv;
 	child.no_stdin = 1;
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index ba002a9..b8df39c 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1343,8 +1343,12 @@ test_expect_success 'receive.denyCurrentBranch = updateInstead' '
 	(cd testrepo &&
 		git update-index --refresh &&
 		git diff-files --quiet &&
-		git diff-index --cached HEAD --
-	)
+		git diff-index --cached HEAD -- &&
+		echo changed > path2 &&
+		git add path2
+	) &&
+	test_commit fourth path2 &&
+	test_must_fail git push testrepo master
 '
 
 test_done

-- 
2.0.0.rc3.9669.g840d1f9

[PATCH v4] Add another option for receive.denyCurrentBranch

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 6983 bytes --]

When synchronizing between working directories, it can be handy to update
the current branch via 'push' rather than 'pull', e.g. when pushing a fix
from inside a VM, or when pushing a fix made on a user's machine (where
the developer is not at liberty to install an ssh daemon let alone know
the user's password).

The common workaround – pushing into a temporary branch and then merging
on the other machine – is no longer necessary with this patch.

The new option is:

'updateInstead':
	Update the working tree accordingly, but refuse to do so if there
	are any uncommitted changes.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/config.txt |  7 ++++
 builtin/receive-pack.c   | 94 ++++++++++++++++++++++++++++++++++++++++++++++--
 t/t5516-fetch-push.sh    | 21 +++++++++++
 3 files changed, 120 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9220725..0519073 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2129,6 +2129,13 @@ 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".
++
+Another option is "updateInstead" which will update the working
+directory (must be clean) if pushing into the current branch. This option is
+intended for synchronizing working directories when one side is not easily
+accessible via interactive ssh (e.g. a live web site, hence the requirement
+that the working directory be clean). This mode also comes in handy when
+developing inside a VM to test and fix code on different Operating Systems.
 
 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 e908d07..7ff8be5 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -26,7 +26,8 @@ enum deny_action {
 	DENY_UNCONFIGURED,
 	DENY_IGNORE,
 	DENY_WARN,
-	DENY_REFUSE
+	DENY_REFUSE,
+	DENY_UPDATE_INSTEAD
 };
 
 static int deny_deletes;
@@ -76,6 +77,8 @@ static enum deny_action parse_deny_action(const char *var, const char *value)
 			return DENY_WARN;
 		if (!strcasecmp(value, "refuse"))
 			return DENY_REFUSE;
+		if (!strcasecmp(value, "updateinstead"))
+			return DENY_UPDATE_INSTEAD;
 	}
 	if (git_config_bool(var, value))
 		return DENY_REFUSE;
@@ -730,11 +733,90 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
 	return 0;
 }
 
+static const char *update_worktree(unsigned char *sha1)
+{
+	const char *update_refresh[] = {
+		"update-index", "--ignore-submodules", "--refresh", "-q", NULL
+	};
+	const char *diff_files[] = {
+		"diff-files", "--quiet", "--ignore-submodules", "--", NULL
+	};
+	const char *diff_index[] = {
+		"diff-index", "--quiet", "--cached", "--ignore-submodules",
+		"HEAD", "--", NULL
+	};
+	const char *read_tree[] = {
+		"read-tree", "-u", "-m", sha1_to_hex(sha1), NULL
+	};
+	const char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : "..";
+	struct argv_array env = ARGV_ARRAY_INIT;
+	struct child_process child = CHILD_PROCESS_INIT;
+
+	if (is_bare_repository())
+		return "denyCurrentBranch = updateInstead needs a worktree";
+
+	argv_array_pushf(&env, "GIT_DIR=%s", absolute_path(get_git_dir()));
+
+	child.argv = update_refresh;
+	child.env = env.argv;
+	child.dir = work_tree;
+	child.no_stdin = 1;
+	child.stdout_to_stderr = 1;
+	child.git_cmd = 1;
+	if (run_command(&child)) {
+		argv_array_clear(&env);
+		return "Up-to-date check failed";
+	}
+
+	/* run_command() does not clean up completely; reinitialize */
+	child_process_init(&child);
+	child.argv = diff_files;
+	child.env = env.argv;
+	child.dir = work_tree;
+	child.no_stdin = 1;
+	child.stdout_to_stderr = 1;
+	child.git_cmd = 1;
+	if (run_command(&child)) {
+		argv_array_clear(&env);
+		return "Working directory not clean";
+	}
+
+	/* run_command() does not clean up completely; reinitialize */
+	child_process_init(&child);
+	child.argv = diff_index;
+	child.env = env.argv;
+	child.no_stdin = 1;
+	child.no_stdout = 1;
+	child.stdout_to_stderr = 0;
+	child.git_cmd = 1;
+	if (run_command(&child)) {
+		argv_array_clear(&env);
+		return "Working directory not clean";
+	}
+
+	/* run_command() does not clean up completely; reinitialize */
+	child_process_init(&child);
+	child.argv = read_tree;
+	child.env = env.argv;
+	child.dir = work_tree;
+	child.no_stdin = 1;
+	child.no_stdout = 1;
+	child.stdout_to_stderr = 0;
+	child.git_cmd = 1;
+	if (run_command(&child)) {
+		argv_array_clear(&env);
+		return "Could not update working tree to new HEAD";
+	}
+
+	argv_array_clear(&env);
+	return NULL;
+}
+
 static const char *update(struct command *cmd, struct shallow_info *si)
 {
 	const char *name = cmd->ref_name;
 	struct strbuf namespaced_name_buf = STRBUF_INIT;
-	const char *namespaced_name;
+	const char *namespaced_name, *ret;
 	unsigned char *old_sha1 = cmd->old_sha1;
 	unsigned char *new_sha1 = cmd->new_sha1;
 
@@ -760,6 +842,11 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 			if (deny_current_branch == DENY_UNCONFIGURED)
 				refuse_unconfigured_deny();
 			return "branch is currently checked out";
+		case DENY_UPDATE_INSTEAD:
+			ret = update_worktree(new_sha1);
+			if (ret)
+				return ret;
+			break;
 		}
 	}
 
@@ -784,10 +871,13 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 				break;
 			case DENY_REFUSE:
 			case DENY_UNCONFIGURED:
+			case DENY_UPDATE_INSTEAD:
 				if (deny_delete_current == DENY_UNCONFIGURED)
 					refuse_unconfigured_deny_delete_current();
 				rp_error("refusing to delete the current branch: %s", name);
 				return "deletion of the current branch prohibited";
+			default:
+				return "Invalid denyDeleteCurrent setting";
 			}
 		}
 	}
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index f4da20a..b8df39c 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1330,4 +1330,25 @@ test_expect_success 'fetch into bare respects core.logallrefupdates' '
 	)
 '
 
+test_expect_success 'receive.denyCurrentBranch = updateInstead' '
+	git push testrepo master &&
+	(cd testrepo &&
+		git reset --hard &&
+		git config receive.denyCurrentBranch updateInstead
+	) &&
+	test_commit third path2 &&
+	git push testrepo master &&
+	test $(git rev-parse HEAD) = $(cd testrepo && git rev-parse HEAD) &&
+	test third = "$(cat testrepo/path2)" &&
+	(cd testrepo &&
+		git update-index --refresh &&
+		git diff-files --quiet &&
+		git diff-index --cached HEAD -- &&
+		echo changed > path2 &&
+		git add path2
+	) &&
+	test_commit fourth path2 &&
+	test_must_fail git push testrepo master
+'
+
 test_done
-- 
2.0.0.rc3.9669.g840d1f9

Re: [PATCH v4] Add another option for receive.denyCurrentBranch

[Junio C Hamano]

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

> +Another option is "updateInstead" which will update the working
> +directory (must be clean) if pushing into the current branch. This option is
> +intended for synchronizing working directories when one side is not easily
> +accessible via interactive ssh (e.g. a live web site, hence the requirement
> +that the working directory be clean). This mode also comes in handy when
> +developing inside a VM to test and fix code on different Operating Systems.

Thanks; this explains the intent very clearly.

> @@ -730,11 +733,90 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
>  	return 0;
>  }
>  
> +static const char *update_worktree(unsigned char *sha1)
> +{
> +	const char *update_refresh[] = {
> +		"update-index", "--ignore-submodules", "--refresh", "-q", NULL
> +	};

Please have "-q" as the first parameter.

    $ git reset --hard
    $ echo >Makefile
    $ git update-index -q --refresh ; echo $?
    0
    $ git update-index --refresh -q ; echo $?
    Makefile: needs update
    1

Yes, I understand and agree that this is a "WAT???".

But update-index has processed its options from left to right from
the beginning of time, which we may want to change someday, but this
topic is more important than updating that "WAT???".

> +...
> +	const char *read_tree[] = {
> +		"read-tree", "-u", "-m", sha1_to_hex(sha1), NULL
> +	};

Is everybody's compiler OK with this initialization with computed
value?  Just checking to see if somebody else says "that would not
work for my setting".

> +	/* run_command() does not clean up completely; reinitialize */
> +	child_process_init(&child);

I do not think you need that comment.  The name run_command() does
not imply "run and clean up for reuse" at all, at least to me.

> +	child.argv = diff_files;
> +	...
> +	if (run_command(&child)) {
> +		argv_array_clear(&env);
> +		return "Working directory not clean";
> +	}
> +
> +	/* run_command() does not clean up completely; reinitialize */
> +	child_process_init(&child);
> +	child.argv = diff_index;
> +	...
> +	if (run_command(&child)) {
> +		argv_array_clear(&env);
> +		return "Working directory not clean";
> +	}

Do we want to give the same message for these two cases?

> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index f4da20a..b8df39c 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1330,4 +1330,25 @@ test_expect_success 'fetch into bare respects core.logallrefupdates' '
>  	)
>  '
>  
> +test_expect_success 'receive.denyCurrentBranch = updateInstead' '
> +	git push testrepo master &&
> +	(cd testrepo &&
> +		git reset --hard &&
> +		git config receive.denyCurrentBranch updateInstead
> +	) &&
> +	test_commit third path2 &&
> +	git push testrepo master &&
> +	test $(git rev-parse HEAD) = $(cd testrepo && git rev-parse HEAD) &&
> +	test third = "$(cat testrepo/path2)" &&
> +	(cd testrepo &&
> +		git update-index --refresh &&
> +		git diff-files --quiet &&
> +		git diff-index --cached HEAD -- &&

This "diff-index", without "--quiet" would not signal if the index
matches HEAD with its exit status.

> +		echo changed > path2 &&

s/> />/;

> +		git add path2
> +	) &&

This made sure that the update happens in a clean state.

> +	test_commit fourth path2 &&
> +	test_must_fail git push testrepo master

And this made sure that the push fails, but does not check what
happened on the receiving repository; minimally something like this
perhaps?

	test_must_fail git push testrepo master &&
        test $(git rev-parse HEAD^) = $(git -C testrepo rev-parse HEAD) &&
	(
		cd testrepo &&
		git diff --quiet &&
		test changed = "$(cat path2)"
	)

That is, we expect that "third" is still the HEAD in testrepo, there
is no difference between the working tree and the index (because you
did "git add path2" over there previously), and path2 still has the
locally updated string in the working tree.

> +'
> +
>  test_done

Other than that, this looks pretty well done.

Thanks.

P.S. I'll be doing 2.2 final today, so I won't have time to apply
this with local tweaking to address the issues above.

[PATCH v5] Support updating working trees when pushing into non-bare repos

[Johannes Schindelin]

This patch series adds support for a new receive.denyCurrentBranch setting
to update the working directory (which must be clean, i.e. there must not be
any uncommitted changes) when pushing into the current branch.

The scenario in which the 'updateInstead' setting became a boon in this
developer's daily work is when trying to get a bug fix from a Windows
computer, a virtual machine, or when getting a bug fix from a user's machine
onto his main machine (in all of those cases it is only possible to connect
via ssh in one direction, but not in the reverse direction). It also comes
in handy when updating a live web site via push (in which case a clean
working directory is an absolute must).

Interdiff vs v4 below diffstat.

Johannes Schindelin (1):
  Add another option for receive.denyCurrentBranch

 Documentation/config.txt |  7 ++++
 builtin/receive-pack.c   | 93 ++++++++++++++++++++++++++++++++++++++++++++++--
 t/t5516-fetch-push.sh    | 26 ++++++++++++++
 3 files changed, 124 insertions(+), 2 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 7ff8be5..bbd9ba3 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -736,7 +736,7 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
 static const char *update_worktree(unsigned char *sha1)
 {
 	const char *update_refresh[] = {
-		"update-index", "--ignore-submodules", "--refresh", "-q", NULL
+		"update-index", "-q", "--ignore-submodules", "--refresh", NULL
 	};
 	const char *diff_files[] = {
 		"diff-files", "--quiet", "--ignore-submodules", "--", NULL
@@ -746,7 +746,7 @@ static const char *update_worktree(unsigned char *sha1)
 		"HEAD", "--", NULL
 	};
 	const char *read_tree[] = {
-		"read-tree", "-u", "-m", sha1_to_hex(sha1), NULL
+		"read-tree", "-u", "-m", NULL, NULL
 	};
 	const char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : "..";
 	struct argv_array env = ARGV_ARRAY_INIT;
@@ -778,10 +778,9 @@ static const char *update_worktree(unsigned char *sha1)
 	child.git_cmd = 1;
 	if (run_command(&child)) {
 		argv_array_clear(&env);
-		return "Working directory not clean";
+		return "Working directory has unstaged changes";
 	}
 
-	/* run_command() does not clean up completely; reinitialize */
 	child_process_init(&child);
 	child.argv = diff_index;
 	child.env = env.argv;
@@ -791,10 +790,10 @@ static const char *update_worktree(unsigned char *sha1)
 	child.git_cmd = 1;
 	if (run_command(&child)) {
 		argv_array_clear(&env);
-		return "Working directory not clean";
+		return "Working directory has staged changes";
 	}
 
-	/* run_command() does not clean up completely; reinitialize */
+	read_tree[3] = sha1_to_hex(sha1);
 	child_process_init(&child);
 	child.argv = read_tree;
 	child.env = env.argv;
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index b8df39c..7b353d0 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1341,14 +1341,19 @@ test_expect_success 'receive.denyCurrentBranch = updateInstead' '
 	test $(git rev-parse HEAD) = $(cd testrepo && git rev-parse HEAD) &&
 	test third = "$(cat testrepo/path2)" &&
 	(cd testrepo &&
-		git update-index --refresh &&
-		git diff-files --quiet &&
-		git diff-index --cached HEAD -- &&
-		echo changed > path2 &&
+		git update-index -q --refresh &&
+		git diff-files --quiet -- &&
+		git diff-index --quiet --cached HEAD -- &&
+		echo changed >path2 &&
 		git add path2
 	) &&
 	test_commit fourth path2 &&
-	test_must_fail git push testrepo master
+	test_must_fail git push testrepo master &&
+	test $(git rev-parse HEAD^) = $(git -C testrepo rev-parse HEAD) &&
+	(cd testrepo &&
+		git diff --quiet &&
+		test changed = "$(cat path2)"
+	)
 '
 
 test_done

-- 
2.0.0.rc3.9669.g840d1f9

[PATCH v5] Add another option for receive.denyCurrentBranch

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 7066 bytes --]

When synchronizing between working directories, it can be handy to update
the current branch via 'push' rather than 'pull', e.g. when pushing a fix
from inside a VM, or when pushing a fix made on a user's machine (where
the developer is not at liberty to install an ssh daemon let alone know
the user's password).

The common workaround – pushing into a temporary branch and then merging
on the other machine – is no longer necessary with this patch.

The new option is:

'updateInstead':
	Update the working tree accordingly, but refuse to do so if there
	are any uncommitted changes.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/config.txt |  7 ++++
 builtin/receive-pack.c   | 93 ++++++++++++++++++++++++++++++++++++++++++++++--
 t/t5516-fetch-push.sh    | 26 ++++++++++++++
 3 files changed, 124 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9220725..0519073 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2129,6 +2129,13 @@ 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".
++
+Another option is "updateInstead" which will update the working
+directory (must be clean) if pushing into the current branch. This option is
+intended for synchronizing working directories when one side is not easily
+accessible via interactive ssh (e.g. a live web site, hence the requirement
+that the working directory be clean). This mode also comes in handy when
+developing inside a VM to test and fix code on different Operating Systems.
 
 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 e908d07..bbd9ba3 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -26,7 +26,8 @@ enum deny_action {
 	DENY_UNCONFIGURED,
 	DENY_IGNORE,
 	DENY_WARN,
-	DENY_REFUSE
+	DENY_REFUSE,
+	DENY_UPDATE_INSTEAD
 };
 
 static int deny_deletes;
@@ -76,6 +77,8 @@ static enum deny_action parse_deny_action(const char *var, const char *value)
 			return DENY_WARN;
 		if (!strcasecmp(value, "refuse"))
 			return DENY_REFUSE;
+		if (!strcasecmp(value, "updateinstead"))
+			return DENY_UPDATE_INSTEAD;
 	}
 	if (git_config_bool(var, value))
 		return DENY_REFUSE;
@@ -730,11 +733,89 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
 	return 0;
 }
 
+static const char *update_worktree(unsigned char *sha1)
+{
+	const char *update_refresh[] = {
+		"update-index", "-q", "--ignore-submodules", "--refresh", NULL
+	};
+	const char *diff_files[] = {
+		"diff-files", "--quiet", "--ignore-submodules", "--", NULL
+	};
+	const char *diff_index[] = {
+		"diff-index", "--quiet", "--cached", "--ignore-submodules",
+		"HEAD", "--", NULL
+	};
+	const char *read_tree[] = {
+		"read-tree", "-u", "-m", NULL, NULL
+	};
+	const char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : "..";
+	struct argv_array env = ARGV_ARRAY_INIT;
+	struct child_process child = CHILD_PROCESS_INIT;
+
+	if (is_bare_repository())
+		return "denyCurrentBranch = updateInstead needs a worktree";
+
+	argv_array_pushf(&env, "GIT_DIR=%s", absolute_path(get_git_dir()));
+
+	child.argv = update_refresh;
+	child.env = env.argv;
+	child.dir = work_tree;
+	child.no_stdin = 1;
+	child.stdout_to_stderr = 1;
+	child.git_cmd = 1;
+	if (run_command(&child)) {
+		argv_array_clear(&env);
+		return "Up-to-date check failed";
+	}
+
+	/* run_command() does not clean up completely; reinitialize */
+	child_process_init(&child);
+	child.argv = diff_files;
+	child.env = env.argv;
+	child.dir = work_tree;
+	child.no_stdin = 1;
+	child.stdout_to_stderr = 1;
+	child.git_cmd = 1;
+	if (run_command(&child)) {
+		argv_array_clear(&env);
+		return "Working directory has unstaged changes";
+	}
+
+	child_process_init(&child);
+	child.argv = diff_index;
+	child.env = env.argv;
+	child.no_stdin = 1;
+	child.no_stdout = 1;
+	child.stdout_to_stderr = 0;
+	child.git_cmd = 1;
+	if (run_command(&child)) {
+		argv_array_clear(&env);
+		return "Working directory has staged changes";
+	}
+
+	read_tree[3] = sha1_to_hex(sha1);
+	child_process_init(&child);
+	child.argv = read_tree;
+	child.env = env.argv;
+	child.dir = work_tree;
+	child.no_stdin = 1;
+	child.no_stdout = 1;
+	child.stdout_to_stderr = 0;
+	child.git_cmd = 1;
+	if (run_command(&child)) {
+		argv_array_clear(&env);
+		return "Could not update working tree to new HEAD";
+	}
+
+	argv_array_clear(&env);
+	return NULL;
+}
+
 static const char *update(struct command *cmd, struct shallow_info *si)
 {
 	const char *name = cmd->ref_name;
 	struct strbuf namespaced_name_buf = STRBUF_INIT;
-	const char *namespaced_name;
+	const char *namespaced_name, *ret;
 	unsigned char *old_sha1 = cmd->old_sha1;
 	unsigned char *new_sha1 = cmd->new_sha1;
 
@@ -760,6 +841,11 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 			if (deny_current_branch == DENY_UNCONFIGURED)
 				refuse_unconfigured_deny();
 			return "branch is currently checked out";
+		case DENY_UPDATE_INSTEAD:
+			ret = update_worktree(new_sha1);
+			if (ret)
+				return ret;
+			break;
 		}
 	}
 
@@ -784,10 +870,13 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 				break;
 			case DENY_REFUSE:
 			case DENY_UNCONFIGURED:
+			case DENY_UPDATE_INSTEAD:
 				if (deny_delete_current == DENY_UNCONFIGURED)
 					refuse_unconfigured_deny_delete_current();
 				rp_error("refusing to delete the current branch: %s", name);
 				return "deletion of the current branch prohibited";
+			default:
+				return "Invalid denyDeleteCurrent setting";
 			}
 		}
 	}
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index f4da20a..7b353d0 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1330,4 +1330,30 @@ test_expect_success 'fetch into bare respects core.logallrefupdates' '
 	)
 '
 
+test_expect_success 'receive.denyCurrentBranch = updateInstead' '
+	git push testrepo master &&
+	(cd testrepo &&
+		git reset --hard &&
+		git config receive.denyCurrentBranch updateInstead
+	) &&
+	test_commit third path2 &&
+	git push testrepo master &&
+	test $(git rev-parse HEAD) = $(cd testrepo && git rev-parse HEAD) &&
+	test third = "$(cat testrepo/path2)" &&
+	(cd testrepo &&
+		git update-index -q --refresh &&
+		git diff-files --quiet -- &&
+		git diff-index --quiet --cached HEAD -- &&
+		echo changed >path2 &&
+		git add path2
+	) &&
+	test_commit fourth path2 &&
+	test_must_fail git push testrepo master &&
+	test $(git rev-parse HEAD^) = $(git -C testrepo rev-parse HEAD) &&
+	(cd testrepo &&
+		git diff --quiet &&
+		test changed = "$(cat path2)"
+	)
+'
+
 test_done
-- 
2.0.0.rc3.9669.g840d1f9

Re: [PATCH v5] Add another option for receive.denyCurrentBranch

[Junio C Hamano]

Thanks, will queue.

I think we would need a bit more tests to protect the feature from
future changes, if you care about the cleanliness requirement of
this feature which is a lot stricter than that of "git checkout".

Perhaps like this one on top.

-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Date: Sun, 30 Nov 2014 17:54:30 -0800
Subject: [PATCH] t5516: more tests for receive.denyCurrentBranch=updateInstead

The previous one tests only the case where a path to be updated by
the push-to-deploy has an incompatible change in the target's
working tree that has already been added to the index, but the
feature itself wants to require the working tree to be a lot cleaner
than what is tested.  Add a handful more tests to protect the
feature from future changes that mistakenly (from the viewpoint of
the inventor of the feature) loosens the cleanliness requirement,
namely:

 - A change only to the working tree but not to the index is still a
   change to be protected;

 - An untracked file in the working tree that would be overwritten
   by a push-to-deploy needs to be protected;

 - A change that happens to make a file identical to what is being
   pushed is still a change to be protected (i.e. the feature's
   cleanliness requirement is more strict than that of checkout).

Also, test that a stat-only change to the working tree is not a
reason to reject a push-to-deploy.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5516-fetch-push.sh | 96 ++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 87 insertions(+), 9 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 7b353d0..85c7fec 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1332,28 +1332,106 @@ test_expect_success 'fetch into bare respects core.logallrefupdates' '
 
 test_expect_success 'receive.denyCurrentBranch = updateInstead' '
 	git push testrepo master &&
-	(cd testrepo &&
+	(
+		cd testrepo &&
 		git reset --hard &&
 		git config receive.denyCurrentBranch updateInstead
 	) &&
 	test_commit third path2 &&
+
+	# Try pushing into a repository with pristine working tree
 	git push testrepo master &&
-	test $(git rev-parse HEAD) = $(cd testrepo && git rev-parse HEAD) &&
-	test third = "$(cat testrepo/path2)" &&
-	(cd testrepo &&
+	(
+		cd testrepo &&
+		git update-index -q --refresh &&
+		git diff-files --quiet -- &&
+		git diff-index --quiet --cached HEAD -- &&
+		test third = "$(cat path2)" &&
+		test $(git -C .. rev-parse HEAD) = $(git rev-parse HEAD)
+	) &&
+
+	# Try pushing into a repository with working tree needing a refresh
+	(
+		cd testrepo &&
+		git reset --hard HEAD^ &&
+		test $(git -C .. rev-parse HEAD^) = $(git rev-parse HEAD) &&
+		test-chmtime +100 path1
+	) &&
+	git push testrepo master &&
+	(
+		cd testrepo &&
 		git update-index -q --refresh &&
 		git diff-files --quiet -- &&
 		git diff-index --quiet --cached HEAD -- &&
-		echo changed >path2 &&
-		git add path2
+		test_cmp ../path1 path1 &&
+		test third = "$(cat path2)" &&
+		test $(git -C .. rev-parse HEAD) = $(git rev-parse HEAD)
 	) &&
+
+	# Update what is to be pushed
 	test_commit fourth path2 &&
+
+	# Try pushing into a repository with a dirty working tree
+	# (1) the working tree updated
+	(
+		cd testrepo &&
+		echo changed >path1
+	) &&
 	test_must_fail git push testrepo master &&
-	test $(git rev-parse HEAD^) = $(git -C testrepo rev-parse HEAD) &&
-	(cd testrepo &&
+	(
+		cd testrepo &&
+		test $(git -C .. rev-parse HEAD^) = $(git rev-parse HEAD) &&
+		git diff --quiet --cached &&
+		test changed = "$(cat path1)"
+	) &&
+
+	# (2) the index updated
+	(
+		cd testrepo &&
+		echo changed >path1 &&
+		git add path1
+	) &&
+	test_must_fail git push testrepo master &&
+	(
+		cd testrepo &&
+		test $(git -C .. rev-parse HEAD^) = $(git rev-parse HEAD) &&
+		git diff --quiet &&
+		test changed = "$(cat path1)"
+	) &&
+
+	# Introduce a new file in the update
+	test_commit fifth path3 &&
+
+	# (3) the working tree has an untracked file that would interfere
+	(
+		cd testrepo &&
+		git reset --hard &&
+		echo changed >path3
+	) &&
+	test_must_fail git push testrepo master &&
+	(
+		cd testrepo &&
+		test $(git -C .. rev-parse HEAD^^) = $(git rev-parse HEAD) &&
+		git diff --quiet &&
+		git diff --quiet --cached &&
+		test changed = "$(cat path3)"
+	) &&
+
+	# (4) the target changes to what gets pushed but it still is a change
+	(
+		cd testrepo &&
+		git reset --hard &&
+		echo fifth >path3 &&
+		git add path3
+	) &&
+	test_must_fail git push testrepo master &&
+	(
+		cd testrepo &&
+		test $(git -C .. rev-parse HEAD^^) = $(git rev-parse HEAD) &&
 		git diff --quiet &&
-		test changed = "$(cat path2)"
+		test fifth = "$(cat path3)"
 	)
+
 '
 
 test_done
-- 
2.2.0-117-g2c5ea17

Re: [PATCH v5] Add another option for receive.denyCurrentBranch

[Johannes Schindelin]

Hi Junio,

On Sun, 30 Nov 2014, Junio C Hamano wrote:

> Thanks, will queue.

Thanks!

> I think we would need a bit more tests to protect the feature from
> future changes, if you care about the cleanliness requirement of
> this feature which is a lot stricter than that of "git checkout".
> 
> Perhaps like this one on top.

Thanks again!
Dscho

Re: [PATCH v5] Add another option for receive.denyCurrentBranch

[Junio C Hamano]

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

> +static const char *update_worktree(unsigned char *sha1)
> +{
> +...
> +	const char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : "..";

I overlooked this one, but is there a reason why this has to look at
an internal implementatino detail which is git_work_tree_cfg,
instead of asking get_git_work_tree()?

I am wondering if that reason is a valid rationale to fix whatever
makes get_git_work_tree() unsuitable for this purpose.

Cc'ing Duy who has been working on the part of the system to
determine and set-up work-tree and git-dir.

Re: [PATCH v5] Add another option for receive.denyCurrentBranch

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
>> +static const char *update_worktree(unsigned char *sha1)
>> +{
>> +...
>> +	const char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : "..";
>
> I overlooked this one, but is there a reason why this has to look at
> an internal implementatino detail which is git_work_tree_cfg,
> instead of asking get_git_work_tree()?
>
> I am wondering if that reason is a valid rationale to fix whatever
> makes get_git_work_tree() unsuitable for this purpose.
>
> Cc'ing Duy who has been working on the part of the system to
> determine and set-up work-tree and git-dir.

Answering myself...

This is because you know receive-pack runs inside the $GIT_DIR,
whether it is a bare or non-bare repository, so either core.worktree
points at a directory that is otherwise unrelated to the $GIT_DIR
(but is the correct $GIT_WORK_TREE), or the top of the working tree
must be ".." for a non-bare repository.  I haven't checked the code,
but we probably are not even doing the repository/working-tree
discovery to set up the data necessary for get_git_work_tree() to
give us a correct answer.

Doing an optional set-up to make get_git_work_tree() work would
involve more damage to the codebase than necessary, I would suspect,
so let's keep this part of the code as-is.

That is, unless I am over-estimating the "damage" necessary.

Thanks.

Re: [PATCH v5] Add another option for receive.denyCurrentBranch

[Johannes Schindelin]

Hi Junio,

On Mon, 1 Dec 2014, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> >
> >> +static const char *update_worktree(unsigned char *sha1)
> >> +{
> >> +...
> >> +	const char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : "..";
> >
> > I overlooked this one, but is there a reason why this has to look at
> > an internal implementatino detail which is git_work_tree_cfg,
> > instead of asking get_git_work_tree()?
> >
> > I am wondering if that reason is a valid rationale to fix whatever
> > makes get_git_work_tree() unsuitable for this purpose.
> >
> > Cc'ing Duy who has been working on the part of the system to
> > determine and set-up work-tree and git-dir.
> 
> Answering myself...
> 
> This is because you know receive-pack runs inside the $GIT_DIR,
> whether it is a bare or non-bare repository, so either core.worktree
> points at a directory that is otherwise unrelated to the $GIT_DIR
> (but is the correct $GIT_WORK_TREE), or the top of the working tree
> must be ".." for a non-bare repository.  I haven't checked the code,
> but we probably are not even doing the repository/working-tree
> discovery to set up the data necessary for get_git_work_tree() to
> give us a correct answer.

Excellent analysis. Indeed, we do not enter the
setup_git_directory_gently() path that would interpret core.worktree and
call set_git_work_tree() explicitly. Instead, we are using the
enter_repo() code path in cmd_receive_pack() because only minimal setup is
required for the default operation of git receive-pack.

> Doing an optional set-up to make get_git_work_tree() work would
> involve more damage to the codebase than necessary, I would suspect,
> so let's keep this part of the code as-is.

Indeed, that is the exact reason why I did not want to go down that rabbit
hole. There are so many things that are skipped when using enter_repo()
instead of setup_git_directory() that the performance impact of switching
alone would probably pose a major regression for big hosters like GitHub
or Atlassian.

I have to admit also that I never used the work tree feature myself,
therefore the support for it is pretty much untested (I *think* that I
briefly tested it years ago, when I came up with the original
updateInstead patch, but that is *long* ago). We could of course simply go
the safe route and error out if we detect that git_work_tree_cfg is set,
leaving the work to support it and to add a proper unit test to somebody
who actually needs this. Hmm?

Ciao,
Johannes

Re: [PATCH v5] Add another option for receive.denyCurrentBranch

[Junio C Hamano]

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

>> This is because you know receive-pack runs inside the $GIT_DIR,
>> whether it is a bare or non-bare repository, so either core.worktree
>> points at a directory that is otherwise unrelated to the $GIT_DIR
>> (but is the correct $GIT_WORK_TREE), or the top of the working tree
>> must be ".." for a non-bare repository....

And this reasoning may be broken, unfortunately.

In a repository with separate-git-dir, we enter $GIT_DIR that is
pointed by the "gitdir: $over_there" thing, and once we go there,
we have no linkage back to find where the working tree is unless
there is core.worktree set, do we?

This feature (with or without the push-to-checkout hook, as that
shares exactly the same logic that discovers, or fails to do so,
where the working tree is) needs to be documented with an entry in
the BUGS section, saying that it will not work in a repository that
is tied to its working tree via the "gitdir:" mechanism.

It actually is a lot worse than merely "it will not work", when this
problem ever manifests itself.  The use of this mechanism in such a
repository will destroy the contents of a wrong directory that
happens to be the parent directory of a repository pointed by the
"gitdir:" mechanism, unless core.worktree is set.  Fortunately, real
submodule directories found in the ".git/modules/" directory of the
superproject, even though they are bound to their checkout locations
in the working tree of the superproject using "gitdir:" mechanism,
do maintain their core.worktree when "git submodule" manages them,
so the use of the mechanism in submodule setting may be safe.

Re: [PATCH v5] Add another option for receive.denyCurrentBranch

[Johannes Schindelin]

Hi Junio,

On Tue, 2 Dec 2014, Junio C Hamano wrote:

> This feature [...] needs to be documented with an entry in the BUGS
> section, saying that it will not work in a repository that is tied to
> its working tree via the "gitdir:" mechanism.

Fair enough. But which BUGS section? Should I add one to `git-push.txt` or
`git-receive-pack.txt`? Technically, it should be the latter, but nobody's
gonna find it there...

Ciao,
Johannes

Re: [PATCH v5] Add another option for receive.denyCurrentBranch

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>>> This is because you know receive-pack runs inside the $GIT_DIR,
>>> whether it is a bare or non-bare repository, so either core.worktree
>>> points at a directory that is otherwise unrelated to the $GIT_DIR
>>> (but is the correct $GIT_WORK_TREE), or the top of the working tree
>>> must be ".." for a non-bare repository....
>
> And this reasoning may be broken, unfortunately.
>
> In a repository with separate-git-dir, we enter $GIT_DIR that is
> pointed by the "gitdir: $over_there" thing, and once we go there,
> we have no linkage back to find where the working tree is unless
> there is core.worktree set, do we?
>
> This feature (with or without the push-to-checkout hook, as that
> shares exactly the same logic that discovers, or fails to do so,
> where the working tree is) needs to be documented with an entry in
> the BUGS section, saying that it will not work in a repository that
> is tied to its working tree via the "gitdir:" mechanism.
>
> It actually is a lot worse than merely "it will not work", when this
> problem ever manifests itself.  The use of this mechanism in such a
> repository will destroy the contents of a wrong directory that
> happens to be the parent directory of a repository pointed by the
> "gitdir:" mechanism, unless core.worktree is set.  Fortunately, real
> submodule directories found in the ".git/modules/" directory of the
> superproject, even though they are bound to their checkout locations
> in the working tree of the superproject using "gitdir:" mechanism,
> do maintain their core.worktree when "git submodule" manages them,
> so the use of the mechanism in submodule setting may be safe.

Actually the other part of the system that uses "gitdir:" mechanism,
i.e. "git init --separate-git-dir $real $worktree", does add the
core.worktree configuration in $real/config pointing at $worktree,
so the above is simply me being overly worried.  If somebody uses
the "gitdir:" mechanism without setting core.worktree to point back,
that is a misconfigured repository/worktree pair, so there is no
need for any BUGS entry.

Sorry about the noise.

[PATCH v2 1/2] Clean stale environment pointer in finish_command()

[Johannes Schindelin]

In start_command(), unset "env" fields are initialized via "env_array". In
finish_command(), the "env_array" is cleared, therefore the "env" field
will point to free()d data.

However, start_command() will set "env" to env_array.argv only if "env"
was unset to begin with, and if it was already set, the caller will need
the original value. Therefore, we need to be very careful only to reset
"env" in finish_command() when it has been initialized in start_command().

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 run-command.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/run-command.c b/run-command.c
index 79a0a76..85578da 100644
--- a/run-command.c
+++ b/run-command.c
@@ -555,6 +555,9 @@ int finish_command(struct child_process *cmd)
 {
 	int ret = wait_or_whine(cmd->pid, cmd->argv[0]);
 	argv_array_clear(&cmd->args);
+	/* Avoid pointing to a stale environment */
+	if (cmd->env == cmd->env_array.argv)
+		cmd->env = NULL;
 	argv_array_clear(&cmd->env_array);
 	return ret;
 }
-- 
2.0.0.rc3.9669.g840d1f9

Re: [PATCH v2 1/2] Clean stale environment pointer in finish_command()

[Johannes Schindelin]

Hi,

On Mon, 10 Nov 2014, Johannes Schindelin wrote:

> In start_command(), unset "env" fields are initialized via "env_array". In
> finish_command(), the "env_array" is cleared, therefore the "env" field
> will point to free()d data.
> 
> However, start_command() will set "env" to env_array.argv only if "env"
> was unset to begin with, and if it was already set, the caller will need
> the original value. Therefore, we need to be very careful only to reset
> "env" in finish_command() when it has been initialized in start_command().

In case it was unclear: this is needed for the the suggested switch from the
previous method to construct the environment to the new env_array method
to work.

(The env_array method unfortunately requires the code to initialize the
environment twice because finish_command() insists on always releasing the
env_array, even if the caller may want to reuse the generated array).

Ciao,
Johannes

Re: [PATCH v2 1/2] Clean stale environment pointer in finish_command()

[Jeff King]

On Mon, Nov 10, 2014 at 03:41:09PM +0100, Johannes Schindelin wrote:

> > However, start_command() will set "env" to env_array.argv only if "env"
> > was unset to begin with, and if it was already set, the caller will need
> > the original value. Therefore, we need to be very careful only to reset
> > "env" in finish_command() when it has been initialized in start_command().
> 
> In case it was unclear: this is needed for the the suggested switch from the
> previous method to construct the environment to the new env_array method
> to work.
> 
> (The env_array method unfortunately requires the code to initialize the
> environment twice because finish_command() insists on always releasing the
> env_array, even if the caller may want to reuse the generated array).

I don't think this is "unfortunately"; freeing the memory was the entire
purpose in adding env_array. If you want to easily reuse the same
environment in multiple commands, it is still perfectly fine to use
"env" directly, like:

  struct argv_array env = ARGV_ARRAY_INIT;
  struct child_process one = CHILD_PROCESS_INIT;
  struct child_process two = CHILD_PROCESS_INIT;

  ... setup env with argv_array_push ...

  one.argv = foo;
  one.env = env.argv;
  run_command(&one);

  two.argv = bar;
  two.env = env.argv;
  run_command(&two);

  argv_array_clear(&env);

You do not get the benefit of the auto-cleanup (you have to call
argv_array_clear yourself), but that is less bad than repeating the
setup of "env" twice.

It may be that Documentation/technical/api-run-command.txt needs to
make this more explicit (I just read over it and it looks OK to me. But
since I am the one who designed the feature, I am not the best test of
whether it is sufficiently clear).

-Peff

Re: [PATCH v2 1/2] Clean stale environment pointer in finish_command()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I don't think this is "unfortunately"; freeing the memory was the entire
> purpose in adding env_array. If you want to easily reuse the same
> environment in multiple commands, it is still perfectly fine to use
> "env" directly, like:
>
>   struct argv_array env = ARGV_ARRAY_INIT;
>   struct child_process one = CHILD_PROCESS_INIT;
>   struct child_process two = CHILD_PROCESS_INIT;
>
>   ... setup env with argv_array_push ...
>
>   one.argv = foo;
>   one.env = env.argv;
>   run_command(&one);
>
>   two.argv = bar;
>   two.env = env.argv;
>   run_command(&two);
>
>   argv_array_clear(&env);
>
> You do not get the benefit of the auto-cleanup (you have to call
> argv_array_clear yourself), but that is less bad than repeating the
> setup of "env" twice.

Yeah, the above looks like the best option, better than using a
single child_process and having to re-initialize fds and envs.

Thanks for helping.

Re: [PATCH v2 1/2] Clean stale environment pointer in finish_command()

[Johannes Schindelin]

Hi,

On Tue, 11 Nov 2014, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I don't think this is "unfortunately"; freeing the memory was the entire
> > purpose in adding env_array. If you want to easily reuse the same
> > environment in multiple commands, it is still perfectly fine to use
> > "env" directly, like:
> >
> >   struct argv_array env = ARGV_ARRAY_INIT;
> >   struct child_process one = CHILD_PROCESS_INIT;
> >   struct child_process two = CHILD_PROCESS_INIT;
> >
> >   ... setup env with argv_array_push ...
> >
> >   one.argv = foo;
> >   one.env = env.argv;
> >   run_command(&one);
> >
> >   two.argv = bar;
> >   two.env = env.argv;
> >   run_command(&two);
> >
> >   argv_array_clear(&env);
> >
> > You do not get the benefit of the auto-cleanup (you have to call
> > argv_array_clear yourself), but that is less bad than repeating the
> > setup of "env" twice.
> 
> Yeah, the above looks like the best option, better than using a
> single child_process and having to re-initialize fds and envs.

Okay, I have to say that I was led to believe that reusing the
child_process struct is okay because argv_array_clear() explicitly
reinitializes the env_array field, something that is useless churn unless
you plan to reuse the memory.

However, my personal taste says that reusing the same memory is more
elegant than to waste extra memory unnecessarily, so I will go with the
child_process_init() solution.

Ciao,
Dscho

Re: [PATCH v2 1/2] Clean stale environment pointer in finish_command()

[Jeff King]

On Wed, Nov 12, 2014 at 11:45:19AM +0100, Johannes Schindelin wrote:

> Okay, I have to say that I was led to believe that reusing the
> child_process struct is okay because argv_array_clear() explicitly
> reinitializes the env_array field, something that is useless churn unless
> you plan to reuse the memory.

The argv_array code prepares its data structure for reuse after freeing.
But child_process which uses it does not make any such promises. If
there were an argv_array_free(), child_process could use it. But there
is only argv_array_clear().

I don't have a problem with finish_command leaving its child_process in
a known usable state (e.g., by calling child_process_init). But I also
don't see much benefit.

> However, my personal taste says that reusing the same memory is more
> elegant than to waste extra memory unnecessarily, so I will go with the
> child_process_init() solution.

I do not mind much either way. But I doubt that a single extra struct on
the stack will break the bank, compared to the fact that we are forking
and execing a new program. I'd also not be surprised if a smart compiler
could notice that the variables are used exclusively in non-overlapping
bits of the code, and just reuse the stack space.

-Peff

Re: [PATCH v2 1/2] Clean stale environment pointer in finish_command()

[Jeff King]

On Wed, Nov 12, 2014 at 05:52:29AM -0500, Jeff King wrote:

> > However, my personal taste says that reusing the same memory is more
> > elegant than to waste extra memory unnecessarily, so I will go with the
> > child_process_init() solution.
> 
> I do not mind much either way. But I doubt that a single extra struct on
> the stack will break the bank, compared to the fact that we are forking
> and execing a new program. I'd also not be surprised if a smart compiler
> could notice that the variables are used exclusively in non-overlapping
> bits of the code, and just reuse the stack space.

Actually, I take that back. We are passing a pointer to a struct, rather
than by-value, so the compiler cannot know that the sub-function does
not store that pointer in a static variable, and reference it in the
next call. It must use two variables if it cannot see the definition of
run_command.

I still think it's pointless optimization to worry about, and you should
write whichever is the most readable and maintainable.

-Peff

Re: [PATCH v2 1/2] Clean stale environment pointer in finish_command()

[Junio C Hamano]

On Wed, Nov 12, 2014 at 2:59 AM, Jeff King <peff@peff.net> wrote:
>>
>> I do not mind much either way. But I doubt that a single extra struct on
>> the stack will break the bank, compared to the fact that we are forking
>> and execing a new program. I'd also not be surprised if a smart compiler
>> could notice that the variables are used exclusively in non-overlapping
>> bits of the code, and just reuse the stack space.
>
> Actually, I take that back. We are passing a pointer to a struct, rather
> than by-value, so the compiler cannot know that the sub-function does
> not store that pointer in a static variable, and reference it in the
> next call. It must use two variables if it cannot see the definition of
> run_command.
>
> I still think it's pointless optimization to worry about, and you should
> write whichever is the most readable and maintainable.

Amen.  I do not have strong preference either way as long as the result
is readable, correct and maintainlable ;-).

Re: [PATCH v2 1/2] Clean stale environment pointer in finish_command()

[Junio C Hamano]

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

> In start_command(), unset "env" fields are initialized via "env_array". In
> finish_command(), the "env_array" is cleared, therefore the "env" field
> will point to free()d data.
>
> However, start_command() will set "env" to env_array.argv only if "env"
> was unset to begin with, and if it was already set, the caller will need
> the original value. Therefore, we need to be very careful only to reset
> "env" in finish_command() when it has been initialized in start_command().

Hmph.  Does the same observation apply to cmd->argv that is
initialied to point to cmd->args.argv only when it is unset?

These managed argv/env arrays originate from c460c0ec (run-command:
store an optional argv_array, 2014-05-15), so I am CC'ing Peff.  I
think this change makes sense but I suspect we should do the same
for args.

>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  run-command.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/run-command.c b/run-command.c
> index 79a0a76..85578da 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -555,6 +555,9 @@ int finish_command(struct child_process *cmd)
>  {
>  	int ret = wait_or_whine(cmd->pid, cmd->argv[0]);
>  	argv_array_clear(&cmd->args);
> +	/* Avoid pointing to a stale environment */
> +	if (cmd->env == cmd->env_array.argv)
> +		cmd->env = NULL;
>  	argv_array_clear(&cmd->env_array);
>  	return ret;
>  }

Re: [PATCH v2 1/2] Clean stale environment pointer in finish_command()

[Jeff King]

On Mon, Nov 10, 2014 at 01:44:24PM -0800, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > In start_command(), unset "env" fields are initialized via "env_array". In
> > finish_command(), the "env_array" is cleared, therefore the "env" field
> > will point to free()d data.
> >
> > However, start_command() will set "env" to env_array.argv only if "env"
> > was unset to begin with, and if it was already set, the caller will need
> > the original value. Therefore, we need to be very careful only to reset
> > "env" in finish_command() when it has been initialized in start_command().
> 
> Hmph.  Does the same observation apply to cmd->argv that is
> initialied to point to cmd->args.argv only when it is unset?

Yes, they behave exactly the same (I think Dscho just doesn't hit it in
his patch because he assigns argv manually).

I don't have a real problem with going in this direction as a safety
measure, but I am not sure that it is safe to reuse a child_process
after finish_command in general, without an intervening
child_process_init. For instance, calling start_command will convert a
"child_process.in" value of "-1" instead a pipe, and overwrite that "-1"
with the descriptor of the pipe. A subsequent use of the same
child_process struct will ask the second child to use that pipe (the
write-half of the pipe, mind you) as its stdin, which is nonsensical.

So I think you are much better off just using two child_process structs
(or a single one and reinitializing in between calls).

-Peff

[PATCH v2 2/2] Add a few more options for receive.denyCurrentBranch

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 7181 bytes --]

When synchronizing between working directories, it can be handy to update
the current branch via 'push' rather than 'pull', e.g. when pushing a fix
from inside a VM, or when pushing a fix made on a user's machine (where
the developer is not at liberty to install an ssh daemon let alone know
the user's password).

The common workaround – pushing into a temporary branch and then merging
on the other machine – is no longer necessary with this patch.

For developers who are uncomfortable with letting pushes update the
working directory, but who are equally uncomfortable with the idea of
pushing into a temporary ref that will be readily forgotten, there is now
also an option to detach the HEAD if a push wants to update the current
branch (no working directory update is required in such a case because the
branch is no longer current after detaching the HEAD).

The new options are:

'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 |  9 +++++++
 builtin/receive-pack.c   | 61 +++++++++++++++++++++++++++++++++++++++++++++---
 t/t5516-fetch-push.sh    | 36 ++++++++++++++++++++++++++++
 3 files changed, 103 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index e8dd76d..fc9b8db 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2129,6 +2129,15 @@ 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".
++
+Another option is "updateInstead" which will update the working
+directory (must be clean) if pushing into the current branch. This option is
+intended for synchronizing working directories when one side is not easily
+accessible via ssh (e.g. inside a VM).
++
+Yet another option is "detachInstead" which will detach the HEAD if updates
+are pushed into the current branch; That way, the current revision, the
+index and the working directory are always left untouched by pushes.
 
 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..4534e88 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
 };
 
 static int deny_deletes;
@@ -120,7 +122,12 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
 	}
 
 	if (!strcmp(var, "receive.denycurrentbranch")) {
-		deny_current_branch = parse_deny_action(var, value);
+		if (value && !strcasecmp(value, "updateinstead"))
+			deny_current_branch = DENY_UPDATE_INSTEAD;
+		else if (value && !strcasecmp(value, "detachinstead"))
+			deny_current_branch = DENY_DETACH_INSTEAD;
+		else
+			deny_current_branch = parse_deny_action(var, value);
 		return 0;
 	}
 
@@ -730,11 +737,44 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
 	return 0;
 }
 
+static const char *merge_worktree(unsigned char *sha1)
+{
+	const char *update_refresh[] = {
+		"update-index", "--ignore-submodules", "--refresh", NULL
+	};
+	const char *read_tree[] = {
+		"read-tree", "-u", "-m", sha1_to_hex(sha1), NULL
+	};
+	struct child_process child = CHILD_PROCESS_INIT;
+
+	if (is_bare_repository())
+		return "denyCurrentBranch = updateInstead needs a worktree";
+
+	argv_array_pushf(&child.env_array, "GIT_DIR=%s", absolute_path(get_git_dir()));
+	child.argv = update_refresh;
+	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");
+
+	/* finish_command cleared the environment; reinitialize */
+	argv_array_pushf(&child.env_array, "GIT_DIR=%s", absolute_path(get_git_dir()));
+	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.");
+
+	return NULL;
+}
+
 static const char *update(struct command *cmd, struct shallow_info *si)
 {
 	const char *name = cmd->ref_name;
 	struct strbuf namespaced_name_buf = STRBUF_INIT;
-	const char *namespaced_name;
+	const char *namespaced_name, *ret;
 	unsigned char *old_sha1 = cmd->old_sha1;
 	unsigned char *new_sha1 = cmd->new_sha1;
 
@@ -760,6 +800,19 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 			if (deny_current_branch == DENY_UNCONFIGURED)
 				refuse_unconfigured_deny();
 			return "branch is currently checked out";
+		case DENY_UPDATE_INSTEAD:
+			ret = merge_worktree(new_sha1);
+			if (ret)
+				return ret;
+			break;
+		case DENY_DETACH_INSTEAD:
+			ret = update_ref("push into current branch (detach)",
+				"HEAD", old_sha1, NULL, REF_NODEREF,
+				UPDATE_REFS_DIE_ON_ERR) ?
+				"Could not detach HEAD" : NULL;
+			if (ret)
+				return ret;
+			break;
 		}
 	}
 
@@ -788,6 +841,8 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 					refuse_unconfigured_deny_delete_current();
 				rp_error("refusing to delete the current branch: %s", name);
 				return "deletion of the current branch prohibited";
+			default:
+				die ("Invalid denyDeleteCurrent setting");
 			}
 		}
 	}
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index f4da20a..3981d1b 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1330,4 +1330,40 @@ test_expect_success 'fetch into bare respects core.logallrefupdates' '
 	)
 '
 
+test_expect_success 'receive.denyCurrentBranch = updateInstead' '
+	git push testrepo master &&
+	(cd testrepo &&
+		git reset --hard &&
+		git config receive.denyCurrentBranch updateInstead
+	) &&
+	test_commit third path2 &&
+	git push testrepo master &&
+	test $(git rev-parse HEAD) = $(cd testrepo && git rev-parse HEAD) &&
+	test third = "$(cat testrepo/path2)" &&
+	(cd testrepo &&
+		git update-index --refresh &&
+		git diff-files --quiet &&
+		git diff-index --cached HEAD --
+	)
+'
+
+test_expect_success 'receive.denyCurrentBranch = detachInstead' '
+	(cd testrepo &&
+		git reset --hard &&
+		git config receive.denyCurrentBranch detachInstead
+	) &&
+	OLDHEAD=$(cd testrepo && git rev-parse HEAD) &&
+	test_commit fourth path2 &&
+	test fourth = "$(cat path2)" &&
+	git push testrepo master &&
+	test $OLDHEAD = $(cd testrepo && git rev-parse HEAD) &&
+	test fourth != "$(cat testrepo/path2)" &&
+	(cd testrepo &&
+		test_must_fail git symbolic-ref HEAD &&
+		git update-index --refresh &&
+		git diff-files --quiet &&
+		git diff-index --cached HEAD --
+	)
+'
+
 test_done
-- 
2.0.0.rc3.9669.g840d1f9