[RFC PATCH 0/4] deny push to current branch of non-bare repo

[RFC PATCH 0/4] deny push to current branch of non-bare repo

[Jeff King]

The short of it is that it's dangerous, we see people confused by it
(there was another one just yesterday), and it's a FAQ:

  http://git.or.cz/gitwiki/GitFaq#head-b96f48bc9c925074be9f95c0fce69bcece5f6e73

The FAQ even says "don't do this until you know what you are doing." So
the safety valve is configurable, so that those who know what they are
doing can switch it off.

And it's even on Sam's "UI improvements" list. :)

Patch 4/4 is the interesting one. 1/4 is a cleanup I saw while fixing
tests. 2/4 is a cleanup to prepare for 3/4. And 3/4 fixes a bunch of
tests which were inadvertently doing such a push (but didn't care
because they didn't look at the working directory).

-Peff

[PATCH 1/4] t5400: expect success for denying deletion

[Jeff King]

Commit a240de11 introduced this test and the code to make it
successful.

Signed-off-by: Jeff King <peff@peff.net>
---
Reading over the mailing list postings which led to a240de11, I think it
is simply a case that Jan didn't fully understand what expect_failure
meant (it means "this is a test that is currently broken, but we hope to
fix in the future", and not anything to do with the test_must_fail in
the test itself).

 t/t5400-send-pack.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index 6fe2f87..da69f08 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -103,7 +103,7 @@ unset GIT_CONFIG GIT_CONFIG_LOCAL
 HOME=`pwd`/no-such-directory
 export HOME ;# this way we force the victim/.git/config to be used.
 
-test_expect_failure \
+test_expect_success \
 	'pushing a delete should be denied with denyDeletes' '
 	cd victim &&
 	git config receive.denyDeletes true &&
-- 
1.6.0.3.866.gc189b

[PATCH 2/4] t5516: refactor oddball tests

[Jeff King]

t5516 sets up some utility functions for starting each test
with a clean slate. However, there were a few tests added
that do not use these functions, but instead make their own
repositories.

Let's bring these in line with the rest of the tests. Not
only do we reduce the number of lines, but these tests will
benefit from any further enhancements to the utility
scripts.

The conversion is pretty straightforward. Most of the tests
created a parent/child clone relationship, for which we now
use 'testrepo' as the parent.  One test looked in testrepo,
but relied on previous tests to have set it up; it now sets
up testrepo explicitly, which makes it a bit more robust to
changes in the script, as well.

Signed-off-by: Jeff King <peff@peff.net>
---
This is on top of 'next' to pick up the recent test from Clemens
Buchacher.

A few oddities here while I was digging in the history:

  - I actually introduced the first of these tests for local tracking
    refs in 09fba7a59 (and the others, being related, copied the style).
    But then I reverted them in 0673c96, because Alex had added other
    similar tests in t5404.  However, these tests ended up being
    re-added by Dscho in 28391a80, which adds a totally unrelated test.
    I think it's the result of a bad patch application (IIRC, he marked
    up my tests to avoid having them chdir for the whole test script.
    During application, Junio would see them going from tweaks to whole
    creation, and presumably just resolved the conflict that way).

    So my initial thought was to simply delete these tests. But since
    then, other related tests have been added to this script, and we do
    want to keep those. So I decided to keep them all, as they form a
    logical progression related to tracking refs. So while there is some
    duplication with t5404, I don't think it is a problem.

  - One of the tests called 'pwd', and I can't see that it would do
    anything useful. I assume it was just leftover debugging cruft
    (especially since it is from that same commit by Dscho).

 t/t5516-fetch-push.sh |   50 ++++++++++++++++++++----------------------------
 1 files changed, 21 insertions(+), 29 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 598664c..3411107 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -39,6 +39,11 @@ mk_test () {
 	)
 }
 
+mk_child() {
+	rm -rf "$1" &&
+	git clone testrepo "$1"
+}
+
 check_push_result () {
 	(
 		cd testrepo &&
@@ -425,13 +430,10 @@ test_expect_success 'push with dry-run' '
 
 test_expect_success 'push updates local refs' '
 
-	rm -rf parent child &&
-	mkdir parent &&
-	(cd parent && git init &&
-		echo one >foo && git add foo && git commit -m one) &&
-	git clone parent child &&
+	mk_test heads/master &&
+	mk_child child &&
 	(cd child &&
-		echo two >foo && git commit -a -m two &&
+		git pull .. master &&
 		git push &&
 	test $(git rev-parse master) = $(git rev-parse remotes/origin/master))
 
@@ -439,15 +441,10 @@ test_expect_success 'push updates local refs' '
 
 test_expect_success 'push updates up-to-date local refs' '
 
-	rm -rf parent child &&
-	mkdir parent &&
-	(cd parent && git init &&
-		echo one >foo && git add foo && git commit -m one) &&
-	git clone parent child1 &&
-	git clone parent child2 &&
-	(cd child1 &&
-		echo two >foo && git commit -a -m two &&
-		git push) &&
+	mk_test heads/master &&
+	mk_child child1 &&
+	mk_child child2 &&
+	(cd child1 && git pull .. master && git push) &&
 	(cd child2 &&
 		git pull ../child1 master &&
 		git push &&
@@ -457,11 +454,8 @@ test_expect_success 'push updates up-to-date local refs' '
 
 test_expect_success 'push preserves up-to-date packed refs' '
 
-	rm -rf parent child &&
-	mkdir parent &&
-	(cd parent && git init &&
-		echo one >foo && git add foo && git commit -m one) &&
-	git clone parent child &&
+	mk_test heads/master &&
+	mk_child child &&
 	(cd child &&
 		git push &&
 	! test -f .git/refs/remotes/origin/master)
@@ -470,15 +464,13 @@ test_expect_success 'push preserves up-to-date packed refs' '
 
 test_expect_success 'push does not update local refs on failure' '
 
-	rm -rf parent child &&
-	mkdir parent &&
-	(cd parent && git init &&
-		echo one >foo && git add foo && git commit -m one &&
-		echo exit 1 >.git/hooks/pre-receive &&
-		chmod +x .git/hooks/pre-receive) &&
-	git clone parent child &&
+	mk_test heads/master &&
+	mk_child child &&
+	mkdir testrepo/.git/hooks &&
+	echo exit 1 >testrepo/.git/hooks/pre-receive &&
+	chmod +x testrepo/.git/hooks/pre-receive &&
 	(cd child &&
-		echo two >foo && git commit -a -m two &&
+		git pull .. master
 		test_must_fail git push &&
 		test $(git rev-parse master) != \
 			$(git rev-parse remotes/origin/master))
@@ -487,7 +479,7 @@ test_expect_success 'push does not update local refs on failure' '
 
 test_expect_success 'allow deleting an invalid remote ref' '
 
-	pwd &&
+	mk_test heads/master &&
 	rm -f testrepo/.git/objects/??/* &&
 	git push testrepo :refs/heads/master &&
 	(cd testrepo && test_must_fail git rev-parse --verify refs/heads/master)
-- 
1.6.0.3.866.gc189b

[PATCH 3/4] tests: avoid pushing to current branch of non-bare repo

[Jeff King]

Many tests create a new repo, and then push into the master
branch (sometimes after making some commits on that branch).
After such a push the index and working tree of the
receiving repo are out of sync with the HEAD. This isn't a
problem for most tests, since they don't bother looking at
the working tree after such a push.  But this is generally a
dangerous behavior, and the tests would break if we later
decided to put in a safety valve.

Depending on the situation, this patch takes one of two
approaches:

  - creates the pushed-to repo as a bare repository. This
    works if we don't actually want to create our own
    commits in the repo.

  - switches the pushed-to repo to another branch before
    pushing. Since we never look at the working tree after
    the push anyway, this doesn't impact the test results.

Signed-off-by: Jeff King <peff@peff.net>
---
This is not the _most_ minimal patch, since when changing a non-bare
repo to a bare one, the name of the git dir changed (e.g.,
s{victim/.git}{victim}), causing a lot of textual changes. We could
technically call the bare clone "victim/.git", but I think this is less
confusing (if a bit harder to read the diff).

 t/t5400-send-pack.sh        |   30 ++++++++++++----------
 t/t5401-update-hooks.sh     |   58 +++++++++++++++++++++---------------------
 t/t5405-send-pack-rewind.sh |    3 +-
 t/t5516-fetch-push.sh       |    3 +-
 t/t5517-push-mirror.sh      |    2 +-
 5 files changed, 50 insertions(+), 46 deletions(-)

diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index da69f08..6bcb4df 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -31,7 +31,7 @@ test_expect_success setup '
 	    parent=$commit || return 1
 	done &&
 	git update-ref HEAD "$commit" &&
-	git clone ./. victim &&
+	git clone --bare ./. victim &&
 	cd victim &&
 	git log &&
 	cd .. &&
@@ -68,7 +68,7 @@ test_expect_success 'pack the destination repository' '
 test_expect_success \
         'pushing rewound head should not barf but require --force' '
 	# should not fail but refuse to update.
-	if git send-pack ./victim/.git/ master
+	if git send-pack ./victim/ master
 	then
 		# now it should fail with Pasky patch
 		echo >&2 Gaah, it should have failed.
@@ -77,7 +77,7 @@ test_expect_success \
 		echo >&2 Thanks, it correctly failed.
 		true
 	fi &&
-	if cmp victim/.git/refs/heads/master .git/refs/heads/master
+	if cmp victim/refs/heads/master .git/refs/heads/master
 	then
 		# should have been left as it was!
 		false
@@ -85,8 +85,8 @@ test_expect_success \
 		true
 	fi &&
 	# this should update
-	git send-pack --force ./victim/.git/ master &&
-	cmp victim/.git/refs/heads/master .git/refs/heads/master
+	git send-pack --force ./victim/ master &&
+	cmp victim/refs/heads/master .git/refs/heads/master
 '
 
 test_expect_success \
@@ -94,14 +94,14 @@ test_expect_success \
 	cd victim &&
 	git branch extra master &&
 	cd .. &&
-	test -f victim/.git/refs/heads/extra &&
-	git send-pack ./victim/.git/ :extra master &&
-	! test -f victim/.git/refs/heads/extra
+	test -f victim/refs/heads/extra &&
+	git send-pack ./victim/ :extra master &&
+	! test -f victim/refs/heads/extra
 '
 
 unset GIT_CONFIG GIT_CONFIG_LOCAL
 HOME=`pwd`/no-such-directory
-export HOME ;# this way we force the victim/.git/config to be used.
+export HOME ;# this way we force the victim/config to be used.
 
 test_expect_success \
 	'pushing a delete should be denied with denyDeletes' '
@@ -109,10 +109,10 @@ test_expect_success \
 	git config receive.denyDeletes true &&
 	git branch extra master &&
 	cd .. &&
-	test -f victim/.git/refs/heads/extra &&
-	test_must_fail git send-pack ./victim/.git/ :extra master
+	test -f victim/refs/heads/extra &&
+	test_must_fail git send-pack ./victim/ :extra master
 '
-rm -f victim/.git/refs/heads/extra
+rm -f victim/refs/heads/extra
 
 test_expect_success \
         'pushing with --force should be denied with denyNonFastforwards' '
@@ -120,14 +120,15 @@ test_expect_success \
 	git config receive.denyNonFastforwards true &&
 	cd .. &&
 	git update-ref refs/heads/master master^ || return 1
-	git send-pack --force ./victim/.git/ master && return 1
-	! test_cmp .git/refs/heads/master victim/.git/refs/heads/master
+	git send-pack --force ./victim/ master && return 1
+	! test_cmp .git/refs/heads/master victim/refs/heads/master
 '
 
 test_expect_success \
 	'pushing does not include non-head refs' '
 	mkdir parent && cd parent &&
 	git init && touch file && git add file && git commit -m add &&
+	git checkout -b otherbranch &&
 	cd .. &&
 	git clone parent child && cd child && git push --all &&
 	cd ../parent &&
@@ -139,6 +140,7 @@ rewound_push_setup() {
 	mkdir parent && cd parent &&
 	git init && echo one >file && git add file && git commit -m one &&
 	echo two >file && git commit -a -m two &&
+	git checkout -b otherbranch
 	cd .. &&
 	git clone parent child && cd child && git reset --hard HEAD^
 }
diff --git a/t/t5401-update-hooks.sh b/t/t5401-update-hooks.sh
index 64f66c9..ae1aa77 100755
--- a/t/t5401-update-hooks.sh
+++ b/t/t5401-update-hooks.sh
@@ -17,22 +17,22 @@ test_expect_success setup '
 	commit1=$(echo modify | git commit-tree $tree1 -p $commit0) &&
 	git update-ref refs/heads/master $commit0 &&
 	git update-ref refs/heads/tofail $commit1 &&
-	git clone ./. victim &&
-	GIT_DIR=victim/.git git update-ref refs/heads/tofail $commit1 &&
+	git clone --bare ./. victim &&
+	GIT_DIR=victim git update-ref refs/heads/tofail $commit1 &&
 	git update-ref refs/heads/master $commit1 &&
 	git update-ref refs/heads/tofail $commit0
 '
 
-cat >victim/.git/hooks/pre-receive <<'EOF'
+cat >victim/hooks/pre-receive <<'EOF'
 #!/bin/sh
 printf %s "$@" >>$GIT_DIR/pre-receive.args
 cat - >$GIT_DIR/pre-receive.stdin
 echo STDOUT pre-receive
 echo STDERR pre-receive >&2
 EOF
-chmod u+x victim/.git/hooks/pre-receive
+chmod u+x victim/hooks/pre-receive
 
-cat >victim/.git/hooks/update <<'EOF'
+cat >victim/hooks/update <<'EOF'
 #!/bin/sh
 echo "$@" >>$GIT_DIR/update.args
 read x; printf %s "$x" >$GIT_DIR/update.stdin
@@ -40,77 +40,77 @@ echo STDOUT update $1
 echo STDERR update $1 >&2
 test "$1" = refs/heads/master || exit
 EOF
-chmod u+x victim/.git/hooks/update
+chmod u+x victim/hooks/update
 
-cat >victim/.git/hooks/post-receive <<'EOF'
+cat >victim/hooks/post-receive <<'EOF'
 #!/bin/sh
 printf %s "$@" >>$GIT_DIR/post-receive.args
 cat - >$GIT_DIR/post-receive.stdin
 echo STDOUT post-receive
 echo STDERR post-receive >&2
 EOF
-chmod u+x victim/.git/hooks/post-receive
+chmod u+x victim/hooks/post-receive
 
-cat >victim/.git/hooks/post-update <<'EOF'
+cat >victim/hooks/post-update <<'EOF'
 #!/bin/sh
 echo "$@" >>$GIT_DIR/post-update.args
 read x; printf %s "$x" >$GIT_DIR/post-update.stdin
 echo STDOUT post-update
 echo STDERR post-update >&2
 EOF
-chmod u+x victim/.git/hooks/post-update
+chmod u+x victim/hooks/post-update
 
 test_expect_success push '
-	test_must_fail git send-pack --force ./victim/.git \
+	test_must_fail git send-pack --force ./victim \
 		master tofail >send.out 2>send.err
 '
 
 test_expect_success 'updated as expected' '
-	test $(GIT_DIR=victim/.git git rev-parse master) = $commit1 &&
-	test $(GIT_DIR=victim/.git git rev-parse tofail) = $commit1
+	test $(GIT_DIR=victim git rev-parse master) = $commit1 &&
+	test $(GIT_DIR=victim git rev-parse tofail) = $commit1
 '
 
 test_expect_success 'hooks ran' '
-	test -f victim/.git/pre-receive.args &&
-	test -f victim/.git/pre-receive.stdin &&
-	test -f victim/.git/update.args &&
-	test -f victim/.git/update.stdin &&
-	test -f victim/.git/post-receive.args &&
-	test -f victim/.git/post-receive.stdin &&
-	test -f victim/.git/post-update.args &&
-	test -f victim/.git/post-update.stdin
+	test -f victim/pre-receive.args &&
+	test -f victim/pre-receive.stdin &&
+	test -f victim/update.args &&
+	test -f victim/update.stdin &&
+	test -f victim/post-receive.args &&
+	test -f victim/post-receive.stdin &&
+	test -f victim/post-update.args &&
+	test -f victim/post-update.stdin
 '
 
 test_expect_success 'pre-receive hook input' '
 	(echo $commit0 $commit1 refs/heads/master;
 	 echo $commit1 $commit0 refs/heads/tofail
-	) | test_cmp - victim/.git/pre-receive.stdin
+	) | test_cmp - victim/pre-receive.stdin
 '
 
 test_expect_success 'update hook arguments' '
 	(echo refs/heads/master $commit0 $commit1;
 	 echo refs/heads/tofail $commit1 $commit0
-	) | test_cmp - victim/.git/update.args
+	) | test_cmp - victim/update.args
 '
 
 test_expect_success 'post-receive hook input' '
 	echo $commit0 $commit1 refs/heads/master |
-	test_cmp - victim/.git/post-receive.stdin
+	test_cmp - victim/post-receive.stdin
 '
 
 test_expect_success 'post-update hook arguments' '
 	echo refs/heads/master |
-	test_cmp - victim/.git/post-update.args
+	test_cmp - victim/post-update.args
 '
 
 test_expect_success 'all hook stdin is /dev/null' '
-	! test -s victim/.git/update.stdin &&
-	! test -s victim/.git/post-update.stdin
+	! test -s victim/update.stdin &&
+	! test -s victim/post-update.stdin
 '
 
 test_expect_success 'all *-receive hook args are empty' '
-	! test -s victim/.git/pre-receive.args &&
-	! test -s victim/.git/post-receive.args
+	! test -s victim/pre-receive.args &&
+	! test -s victim/post-receive.args
 '
 
 test_expect_success 'send-pack produced no output' '
diff --git a/t/t5405-send-pack-rewind.sh b/t/t5405-send-pack-rewind.sh
index cb9aacc..2ad080f 100755
--- a/t/t5405-send-pack-rewind.sh
+++ b/t/t5405-send-pack-rewind.sh
@@ -16,7 +16,8 @@ test_expect_success setup '
 	) &&
 
 	>file2 && git add file2 && test_tick &&
-	git commit -m Second
+	git commit -m Second &&
+	git checkout -b otherbranch
 
 '
 
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 3411107..7070171 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -12,7 +12,8 @@ mk_empty () {
 	(
 		cd testrepo &&
 		git init &&
-		mv .git/hooks .git/hooks-disabled
+		mv .git/hooks .git/hooks-disabled &&
+		git symbolic-ref HEAD refs/heads/nonexistent
 	)
 }
 
diff --git a/t/t5517-push-mirror.sh b/t/t5517-push-mirror.sh
index ea49ded..5536077 100755
--- a/t/t5517-push-mirror.sh
+++ b/t/t5517-push-mirror.sh
@@ -19,7 +19,7 @@ mk_repo_pair () {
 	mkdir mirror &&
 	(
 		cd mirror &&
-		git init
+		git --bare init
 	) &&
 	mkdir master &&
 	(
-- 
1.6.0.3.866.gc189b

[PATCH 4/4] receive-pack: deny push to current branch of non-bare repo

[Jeff King]

Pushing into the currently checked out branch of a non-bare
repository can be dangerous; the HEAD then loses sync with
the index and working tree, and it looks in the receiving
repo as if the pushed changes have been reverted in the
index (since they were never there in the first place).

This patch adds a safety valve that checks for this
condition and denies the push. We trigger the check only on
a non-bare repository, since a bare does not have a working
tree (and in fact, pushing to the HEAD branch is a common
workflow for publishing repositories).

This behavior is still configurable, though, since some very
specific setups may want to allow such a push if they know
they will take action to reconcile the working tree and HEAD
afterwards (e.g., a post-receive hook that does "git reset
--hard").

Signed-off-by: Jeff King <peff@peff.net>
---
My feeling is that this is dangerous behavior that we see new users
confused by, so it is worth addressing. The other obvious route is to
at least _try_ the merge, and if it comes out cleanly, to allow it.

But it looks like Sam is promoting that as a hook, which makes a lot
more sense to me. And we can still support that, but the user of the
hook must now not only install the hook, but also set the config value.

I am open to comments on the name of the config value. Somebody at the
GitTogether suggested (possibly under the influence of beer) that it be
receive.PEBKAC (since you should only turn it off if you really know
what you're doing, you would set PEBKAC to "false"), but I didn't want
to give the impression that git wasn't user-friendly. ;)

One final issue: do we need to make a special exception for "branch yet
to be born"? I believe we do so for the analagous "fetch" situation.

 Documentation/config.txt |    8 ++++++++
 builtin-receive-pack.c   |   16 ++++++++++++++++
 t/t5516-fetch-push.sh    |   21 +++++++++++++++++++++
 3 files changed, 45 insertions(+), 0 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 965ed74..971f01e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1198,6 +1198,14 @@ receive.denyNonFastForwards::
 	even if that push is forced. This configuration variable is
 	set when initializing a shared repository.
 
+receive.denyCurrentBranch::
+	If set to true, receive-pack will deny a ref update to the
+	currently checked out branch of a non-bare repository. Such a
+	push is potentially dangerous because it brings the HEAD out of
+	sync with the index and working tree; only set this to "false"
+	if you know what you are doing (e.g., you have a post-receive
+	hook which resets the working tree). Defaults to "true".
+
 transfer.unpackLimit::
 	When `fetch.unpackLimit` or `receive.unpackLimit` are
 	not set, the value of this variable is used instead.
diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index 7f9f134..06ad545 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -13,6 +13,7 @@ static const char receive_pack_usage[] = "git-receive-pack <git-dir>";
 
 static int deny_deletes = 0;
 static int deny_non_fast_forwards = 0;
+static int deny_current_branch = 1;
 static int receive_fsck_objects;
 static int receive_unpack_limit = -1;
 static int transfer_unpack_limit = -1;
@@ -49,6 +50,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (!strcmp(var, "receive.denycurrentbranch")) {
+		deny_current_branch = git_config_bool(var, value);
+		return 0;
+	}
+
 	return git_default_config(var, value, cb);
 }
 
@@ -186,6 +192,16 @@ static const char *update(struct command *cmd)
 		return "funny refname";
 	}
 
+	if (deny_current_branch && !is_bare_repository()) {
+		unsigned char sha1[20];
+		const char *head = resolve_ref("HEAD", sha1, 0, NULL);
+		if (!strcmp(head, name)) {
+			error("refusing to update checked out branch: %s",
+				name);
+			return "branch is currently checked out";
+		}
+	}
+
 	if (!is_null_sha1(new_sha1) && !has_sha1_file(new_sha1)) {
 		error("unpack should have generated %s, "
 		      "but I can't find it!", sha1_to_hex(new_sha1));
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 7070171..579c3d8 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -487,4 +487,25 @@ test_expect_success 'allow deleting an invalid remote ref' '
 
 '
 
+test_expect_success 'deny push to HEAD to non-bare repository' '
+	mk_test heads/master
+	(cd testrepo && git checkout master) &&
+	test_must_fail git push testrepo master
+'
+
+test_expect_success 'allow push to HEAD of bare repository' '
+	mk_test heads/master
+	(cd testrepo && git checkout master && git config core.bare true) &&
+	git push testrepo master
+'
+
+test_expect_success 'allow push to HEAD of non-bare repository w/ config' '
+	mk_test heads/master
+	(cd testrepo &&
+		git checkout master &&
+		git config receive.denyCurrentBranch false
+	) &&
+	git push testrepo master
+'
+
 test_done
-- 
1.6.0.3.866.gc189b

Re: [RFC PATCH 0/4] deny push to current branch of non-bare repo

[Mark Burton]

Jeff King <peff <at> peff.net> writes:

> 
> The short of it is that it's dangerous, we see people confused by it
> (there was another one just yesterday), and it's a FAQ:
> 
>   http://git.or.cz/gitwiki/GitFaq#head-b96f48bc9c925074be9f95c0fce69bcece5f6e73
> 
> The FAQ even says "don't do this until you know what you are doing." So
> the safety valve is configurable, so that those who know what they are
> doing can switch it off.

When I first tried to use git I was bitten by exactly this problem. I know,
RTFM, but when everything is new, it's easy to undervalue the words of wisdom
when you don't understand the bigger picture and the rational behind the advice.

I now happily work with non-bare repositories on my main machine that I push to
from my satellite development machines but, of course, I don't push to the head
branches but, instead, to remote branches and then merge on the main machine.

I wouldn't have wasted as much time getting my head around this if git had
refused to accept the push to the current branch but, instead, issued a suitable
message telling me I probably didn't want to be doing that.

So, from my own experience, I would say this would be a good feature to add.

Cheers,

Mark

Re: [RFC PATCH 0/4] deny push to current branch of non-bare repo

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The FAQ even says "don't do this until you know what you are doing." So
> the safety valve is configurable, so that those who know what they are
> doing can switch it off.

"We are breaking your existing working setup but you can add a new
configuration to unbreak it" should not be done lightly.  I think as the
end result it is a reasonable thing to aim for for this particular
feature, but we do need a transition plan patch in between that introduces
a step that warns but not forbids.  We can ship 1.6.1 with it and then
switch the default to forbid in 1.6.3, for example.

> Patch 4/4 is the interesting one. 1/4 is a cleanup I saw while fixing
> tests. 2/4 is a cleanup to prepare for 3/4. And 3/4 fixes a bunch of
> tests which were inadvertently doing such a push (but didn't care
> because they didn't look at the working directory).

I wonder if you can use the tests 3/4 touches as the test for your "keep
existing setup" configuration variable, pretending that they are old
timer's repositories?

Re: [RFC PATCH 0/4] deny push to current branch of non-bare repo

[Jeff King]

On Fri, Nov 07, 2008 at 03:16:53PM -0800, Junio C Hamano wrote:

> > The FAQ even says "don't do this until you know what you are doing." So
> > the safety valve is configurable, so that those who know what they are
> > doing can switch it off.
> 
> "We are breaking your existing working setup but you can add a new
> configuration to unbreak it" should not be done lightly.  I think as the
> end result it is a reasonable thing to aim for for this particular
> feature, but we do need a transition plan patch in between that introduces
> a step that warns but not forbids.  We can ship 1.6.1 with it and then
> switch the default to forbid in 1.6.3, for example.

Yeah, I was kind of hoping we could assume that anybody relying on this
behavior was somewhat insane, and wouldn't be too upset when it broke.
But you're probably right that we should be more conservative. I'll
rework it with a "yes/no/warn" option for the config, and we can set it
to "warn" (and those who really do want it can shut off the warning with
"no"). Or we can even start with just leaving it on "no", but I think
the deprecation period should begin when we switch it to "warn".

> > Patch 4/4 is the interesting one. 1/4 is a cleanup I saw while fixing
> > tests. 2/4 is a cleanup to prepare for 3/4. And 3/4 fixes a bunch of
> > tests which were inadvertently doing such a push (but didn't care
> > because they didn't look at the working directory).
> 
> I wonder if you can use the tests 3/4 touches as the test for your "keep
> existing setup" configuration variable, pretending that they are old
> timer's repositories?

Yes, they do break with 4/4 applied without 3/4 (that was how I found
them, but "git rebase -i" let me pretend I had the proper foresight. ;)
). We can keep 3/4 back until the switch from "warn" to "yes", if that's
what you are suggesting.

-Peff

Re: [RFC PATCH 0/4] deny push to current branch of non-bare repo

[Johannes Schindelin]

Hi,

On Sat, 8 Nov 2008, Jeff King wrote:

> On Fri, Nov 07, 2008 at 03:16:53PM -0800, Junio C Hamano wrote:
> 
> > > The FAQ even says "don't do this until you know what you are doing." 
> > > So the safety valve is configurable, so that those who know what 
> > > they are doing can switch it off.
> > 
> > "We are breaking your existing working setup but you can add a new 
> > configuration to unbreak it" should not be done lightly.  I think as 
> > the end result it is a reasonable thing to aim for for this particular 
> > feature, but we do need a transition plan patch in between that 
> > introduces a step that warns but not forbids.  We can ship 1.6.1 with 
> > it and then switch the default to forbid in 1.6.3, for example.
> 
> Yeah, I was kind of hoping we could assume that anybody relying on this
> behavior was somewhat insane, and wouldn't be too upset when it broke.

I think I have a repository with "git read-tree -u -m HEAD" as update hook 
for that kind of behavior.

But I will not be the person responsible to keep that behavior, if I am 
the only one relying on it.

I very much like the approach of defaulting to "warn" for quite some time 
(but setting the variable to "refuse" in git-init) and then adapt the 
default after some time.

Ciao,
Dscho

Re: [RFC PATCH 0/4] deny push to current branch of non-bare repo

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yes, they do break with 4/4 applied without 3/4 (that was how I found
> them, but "git rebase -i" let me pretend I had the proper foresight. ;)
> ). We can keep 3/4 back until the switch from "warn" to "yes", if that's
> what you are suggesting.

I meant to suggest that change contained in 3/4 can instead be "set the
configuration to allow such a dangerous push upfront, and make sure the
pushes the current tests perform actually are still allowed", _if_ you are
changing the default to forbid.

I think the default should be to warn for two release cycles during which
we will give deprecation notice, and then switch the default to forbid
(and we do not touch "git init/git clone" at all --- changing the default
to forbid in newly created repositories earlier than existing repositories
would be changing the behaviour of the command between old and new
repositories, which is madness).  If we are going this route, I think we
can modify the tests 3/4 touches to set the configuration to allow such a
push and make sure that such a push is still allowed.

Re: [RFC PATCH 0/4] deny push to current branch of non-bare repo

[Jeff King]

On Sat, Nov 08, 2008 at 12:49:21PM -0800, Junio C Hamano wrote:

> I meant to suggest that change contained in 3/4 can instead be "set the
> configuration to allow such a dangerous push upfront, and make sure the
> pushes the current tests perform actually are still allowed", _if_ you are
> changing the default to forbid.

Ah, I see. I did think about using the config variable for those tests,
but it felt too much like testing two things at once. That is, it is
nicer to debug if each test breaks only when the thing it is testing for
is broken, not some other random unrelated feature. Obviously that isn't
always possible, but it seemed kind of clumsy to me.

Anyway, with a default of "warn" the tests don't need any update at all
(and do serve as a test that we still haven't broken people), and I can
pass the decision off to whoever changes it to "refuse" after the
deprecation period. :)

> I think the default should be to warn for two release cycles during which
> we will give deprecation notice, and then switch the default to forbid

OK, the patch is below, replacing 4/4. 3/4 can simply be dropped at this
point (and I think 1/4 is a no-brainer to apply, and 2/4 is probably
worth it as cleanup).

I worded the warning to explain what happened so that the Frequently
Asking users might have a clue that something bad has happened. But
maybe it should also:

  - suggest "git reset --hard"; of course, then we need to explain that
    you would be losing your work, so we have to warn about that, too.

  - more explicitly warn that the behavior is deprecated.

Also, we could potentially note the deprecation in the documentation for
the config option.

> (and we do not touch "git init/git clone" at all --- changing the default
> to forbid in newly created repositories earlier than existing repositories
> would be changing the behaviour of the command between old and new
> repositories, which is madness).  If we are going this route, I think we

I agree. I suggested that for another config option recently, and I now
think I was wrong. It really doesn't dodge the "things are changing"
bullet. It just makes them change at a slightly different time, which
can be even more confusing (i.e., "this breaks in my repo, but when I
make a test repo it works" or vice versa).

I do feel like we made a config change like that at some point long ago,
but I can't recall for what, or the reasoning. Maybe
core.logallrefupdates, which does have clone-specific behavior.

> can modify the tests 3/4 touches to set the configuration to allow such a
> push and make sure that such a push is still allowed.

Again, I am not sure that is best, as above. But I tried to cover all
cases explicitly with my tests, so I think we should get good coverage
either way (and my tests don't depend on any particular default config
setting).

-- >8 --
receive-pack: detect push to current branch of non-bare repo

Pushing into the currently checked out branch of a non-bare
repository can be dangerous; the HEAD then loses sync with
the index and working tree, and it looks in the receiving
repo as if the pushed changes have been reverted in the
index (since they were never there in the first place).

This patch adds a safety valve that checks for this
condition and either generates a warning or denies the
update. We trigger the check only on a non-bare repository,
since a bare repo does not have a working tree (and in fact,
pushing to the HEAD branch is a common workflow for
publishing repositories).

The behavior is configurable via receive.denyCurrentBranch,
defaulting to "warn" so as not to break existing setups
(though it may, after a deprecation period, switch to
"refuse" by default). For users who know what they are doing
and want to silence the warning (e.g., because they have a
post-receive hook that reconciles the HEAD and working
tree), they can turn off the warning by setting it to false
or "ignore".

Signed-off-by: Jeff King <peff@peff.net>

---
 Documentation/config.txt |    9 +++++++
 builtin-receive-pack.c   |   59 ++++++++++++++++++++++++++++++++++++++++++++++
 t/t5516-fetch-push.sh    |   37 ++++++++++++++++++++++++++++
 3 files changed, 105 insertions(+), 0 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 965ed74..32dcd64 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1198,6 +1198,15 @@ receive.denyNonFastForwards::
 	even if that push is forced. This configuration variable is
 	set when initializing a shared repository.
 
+receive.denyCurrentBranch::
+	If set to true or "refuse", receive-pack will deny a ref update
+	to the currently checked out branch of a non-bare repository.
+	Such a push is potentially dangerous because it brings the HEAD
+	out of sync with the index and working tree. If set to "warn",
+	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 "warn".
+
 transfer.unpackLimit::
 	When `fetch.unpackLimit` or `receive.unpackLimit` are
 	not set, the value of this variable is used instead.
diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index 7f9f134..db67c31 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -11,8 +11,15 @@
 
 static const char receive_pack_usage[] = "git-receive-pack <git-dir>";
 
+enum deny_action {
+	DENY_IGNORE,
+	DENY_WARN,
+	DENY_REFUSE,
+};
+
 static int deny_deletes = 0;
 static int deny_non_fast_forwards = 0;
+static enum deny_action deny_current_branch = DENY_WARN;
 static int receive_fsck_objects;
 static int receive_unpack_limit = -1;
 static int transfer_unpack_limit = -1;
@@ -22,6 +29,21 @@ static int report_status;
 static char capabilities[] = " report-status delete-refs ";
 static int capabilities_sent;
 
+static enum deny_action parse_deny_action(const char *var, const char *value)
+{
+	if (value) {
+		if (!strcasecmp(value, "ignore"))
+			return DENY_IGNORE;
+		if (!strcasecmp(value, "warn"))
+			return DENY_WARN;
+		if (!strcasecmp(value, "refuse"))
+			return DENY_REFUSE;
+	}
+	if (git_config_bool(var, value))
+		return DENY_REFUSE;
+	return DENY_IGNORE;
+}
+
 static int receive_pack_config(const char *var, const char *value, void *cb)
 {
 	if (strcmp(var, "receive.denydeletes") == 0) {
@@ -49,6 +71,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (!strcmp(var, "receive.denycurrentbranch")) {
+		deny_current_branch = parse_deny_action(var, value);
+		return 0;
+	}
+
 	return git_default_config(var, value, cb);
 }
 
@@ -173,6 +200,20 @@ static int run_update_hook(struct command *cmd)
 	return hook_status(run_command(&proc), update_hook);
 }
 
+static int is_ref_checked_out(const char *ref)
+{
+	unsigned char sha1[20];
+	const char *head;
+
+	if (is_bare_repository())
+		return 0;
+
+	head = resolve_ref("HEAD", sha1, 0, NULL);
+	if (!head)
+		return 0;
+	return !strcmp(head, ref);
+}
+
 static const char *update(struct command *cmd)
 {
 	const char *name = cmd->ref_name;
@@ -186,6 +227,24 @@ static const char *update(struct command *cmd)
 		return "funny refname";
 	}
 
+	switch (deny_current_branch) {
+	case DENY_IGNORE:
+		break;
+	case DENY_WARN:
+		if (!is_ref_checked_out(name))
+			break;
+		warning("updating the currently checked out branch; this may"
+			" cause confusion,\n"
+			"as the index and working tree do not reflect changes"
+			" that are now in HEAD.");
+		break;
+	case DENY_REFUSE:
+		if (!is_ref_checked_out(name))
+			break;
+		error("refusing to update checked out branch: %s", name);
+		return "branch is currently checked out";
+	}
+
 	if (!is_null_sha1(new_sha1) && !has_sha1_file(new_sha1)) {
 		error("unpack should have generated %s, "
 		      "but I can't find it!", sha1_to_hex(new_sha1));
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 3411107..a6532cb 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -486,4 +486,41 @@ test_expect_success 'allow deleting an invalid remote ref' '
 
 '
 
+test_expect_success 'warn on push to HEAD of non-bare repository' '
+	mk_test heads/master
+	(cd testrepo &&
+		git checkout master &&
+		git config receive.denyCurrentBranch warn) &&
+	git push testrepo master 2>stderr &&
+	grep "warning.*this may cause confusion" stderr
+'
+
+test_expect_success 'deny push to HEAD of non-bare repository' '
+	mk_test heads/master
+	(cd testrepo &&
+		git checkout master &&
+		git config receive.denyCurrentBranch true) &&
+	test_must_fail git push testrepo master
+'
+
+test_expect_success 'allow push to HEAD of bare repository (bare)' '
+	mk_test heads/master
+	(cd testrepo &&
+		git checkout master &&
+		git config receive.denyCurrentBranch true &&
+		git config core.bare true) &&
+	git push testrepo master 2>stderr &&
+	! grep "warning.*this may cause confusion" stderr
+'
+
+test_expect_success 'allow push to HEAD of non-bare repository (config)' '
+	mk_test heads/master
+	(cd testrepo &&
+		git checkout master &&
+		git config receive.denyCurrentBranch false
+	) &&
+	git push testrepo master 2>stderr &&
+	! grep "warning.*this may cause confusion" stderr
+'
+
 test_done

Re: [PATCH 1/4] t5400: expect success for denying deletion

[Jan Krüger]

Hi,

On Fri, 7 Nov 2008 17:09:55 -0500, Jeff King <peff@peff.net> wrote:
> Reading over the mailing list postings which led to a240de11, I think
> it is simply a case that Jan didn't fully understand what
> expect_failure meant


Yes, that's exactly what happened, and it won't likely happen again.
Thanks for fixing and for the Cc.

-Jan

Re: [RFC PATCH 0/4] deny push to current branch of non-bare repo

[Junio C Hamano]

Thanks; will be in 'next'.

Re: [RFC PATCH 0/4] deny push to current branch of non-bare repo

[Kyle Moffett]

On Sat, Nov 8, 2008 at 8:49 PM, Jeff King <peff@peff.net> wrote:
> The behavior is configurable via receive.denyCurrentBranch,
> defaulting to "warn" so as not to break existing setups
> (though it may, after a deprecation period, switch to
> "refuse" by default). For users who know what they are doing
> and want to silence the warning (e.g., because they have a
> post-receive hook that reconciles the HEAD and working
> tree), they can turn off the warning by setting it to false
> or "ignore".

Hmm, I wonder if it would be possible to also add a "detach" variant;
which would create a detached-HEAD at the current commit when
automatically receiving a push to the working branch.  I have a
post-receive script that does so right now on a couple repositories.
It's still a little confusing to someone actively working in the
repository being pushed to, but it's much easier to explain than the
current default behavior.

Cheers,
Kyle Moffett

Re: [RFC PATCH 0/4] deny push to current branch of non-bare repo

[Jeff King]

On Tue, Nov 11, 2008 at 07:44:06PM -0500, Kyle Moffett wrote:

> Hmm, I wonder if it would be possible to also add a "detach" variant;
> which would create a detached-HEAD at the current commit when
> automatically receiving a push to the working branch.  I have a
> post-receive script that does so right now on a couple repositories.
> It's still a little confusing to someone actively working in the
> repository being pushed to, but it's much easier to explain than the
> current default behavior.

A neat idea, but I'm not sure what workflow that is meant to support.

Before you had:

  1. git push non-bare-remote theirHEAD
  2a. echo Oops, I've just screwed myself.
    3a. ssh remote 'git reset --soft HEAD@{1}'
  2b. echo Oops, I just screwed somebody else.
    3b. echo sorry | mail somebody.else

With "refuse" you have:

  1. git push non-bare-remote theirHEAD
  2. echo Oops, rejected.
  3. git push non-bare-remote theirHEAD:elsewhere
  4a. ssh remote 'git merge elsewhere'
  4b. echo 'please merge elsewhere' | mail somebody.else

which is an improvement. With "detach" you have:

  1. git push non-bare-remote theirHEAD
  2. echo Oh, now we've detached on the remote.
  3a. ssh remote 'git checkout theirHEAD'
  3b. echo 'please merge theirHEAD. BTW, you have been detached without
            realizing it, so make sure you didn't lose any commits.' |
            mail somebody.else

So I think in the case that you are working by yourself, you haven't
really saved much effort (you didn't have to repeat your push, but you
still have to go to the remote and checkout instead of merge). But if
you are pushing into somebody _else_'s repo, you have just mightily
confused them as they start to make commits on top of the detached HEAD.

Still, there may be some instances where moving to the detached HEAD is
preferable. But, like the "try to merge if we can" strategy, I think it
is better implemented by setting denyCurrentBranch to ignore and using a
hook for those instances. And if either hook becomes ubiquitous, maybe
it will be worth implementing within git itself (but I doubt it for
either, as the desired behavior is highly dependent on your personal
workflow).

-Peff

Re: [RFC PATCH 0/4] deny push to current branch of non-bare repo

[Kyle Moffett]

On Wed, Nov 12, 2008 at 3:44 AM, Jeff King <peff@peff.net> wrote:
> On Tue, Nov 11, 2008 at 07:44:06PM -0500, Kyle Moffett wrote:
>> Hmm, I wonder if it would be possible to also add a "detach" variant;
>> which would create a detached-HEAD at the current commit when
>> automatically receiving a push to the working branch.  I have a
>> post-receive script that does so right now on a couple repositories.
>> It's still a little confusing to someone actively working in the
>> repository being pushed to, but it's much easier to explain than the
>> current default behavior.
>
> A neat idea, but I'm not sure what workflow that is meant to support.

Basically, I have a remote tree on a fast multicore box used for runs
of a test suite on various peoples different branches.  When I want
somebody to push something for me to test, they push directly to that
repo, and when I'm done playing with a previous run I just do:

$ git checkout new/branch/to/test
$ make clean
$ ./configure
$ make
$ make check

Occasionally I notice a bug which I want to temporarily fix to let the
build continue, even though I will need to have the author merge that
fix as a part of his original buggy patch.  If nobody pushes the
branch I'm currently testing again, I can "git diff" just fine to see
what I had to fix.  If somebody pushes to a different branch than the
one I'm testing, it's also fine.  The inconsistency is pushing to the
branch I'm on.

So it would be handy to be able to mark that repository as
"detach-HEAD-on-push-of-current-branch", which would let me remember
where I was, even if that's not where that branch is anymore.

There are other ways I could probably do something very similar, but
since the config option was being added it seemed it would probably be
easy to extend.  If nobody else is interested in that behavior, I will
just keep maintaining my own hook, but I thought I'd mention it.

Cheers,
Kyle Moffett

Re: [RFC PATCH 0/4] deny push to current branch of non-bare repo

[Jeff King]

On Thu, Nov 13, 2008 at 12:22:20AM -0500, Kyle Moffett wrote:

> somebody to push something for me to test, they push directly to that
> repo, and when I'm done playing with a previous run I just do:
> 
> $ git checkout new/branch/to/test
> $ make clean
> $ ./configure
> $ make
> $ make check

OK, I see how using a detached HEAD makes sense. But I think just going
straight to a detached HEAD might make even more sense. With your
proposed behavior, you need to be prepared to unexpectedly and
asynchronously move to a detached HEAD at any time, so why not just
start there in the first place?

And then the "push to current branch" problem is neatly solved: you have
no current branch.

So:

  $ git checkout new/branch/to/test^0
  $ make, configure, etc

-Peff

Re: [RFC PATCH 0/4] deny push to current branch of non-bare repo

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> And then the "push to current branch" problem is neatly solved: you have
> no current branch.
>
> So:
>
>   $ git checkout new/branch/to/test^0
>   $ make, configure, etc

Exactly.

I keep a handful pseudo worktrees around (created with git-new-workdir on
top of a single repository) for quick patch test and build purposes.  I do
not push into them but pushing into a non-bare repository and checking out
the same branch twice in such a setup share exactly the same issue, and I
keep their HEADs all detached for exactly the same reason.

Re: [RFC PATCH 0/4] deny push to current branch of non-bare repo

[Kyle Moffett]

On Thu, Nov 13, 2008 at 1:14 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>> And then the "push to current branch" problem is neatly solved: you have
>> no current branch.
>>
>> So:
>>
>>   $ git checkout new/branch/to/test^0
>>   $ make, configure, etc
>
> Exactly.
>
> I keep a handful pseudo worktrees around (created with git-new-workdir on
> top of a single repository) for quick patch test and build purposes.  I do
> not push into them but pushing into a non-bare repository and checking out
> the same branch twice in such a setup share exactly the same issue, and I
> keep their HEADs all detached for exactly the same reason.

I guess the issue comes down to a UI complication.  It would very easy
for me to tell somebody how to check out and test their branch in my
testbed if I'm not around, except for that little bit of arcane
syntax.  Moreover, the consequences if they forget are really
frustrating and hard to figure out.  It's also very easy with a GUI to
do the simple *rightclick branch, click "Checkout"*, but would be much
harder to do the detached HEAD checkout correctly.

If it didn't involve reconfiguring a lot of other people's
repositories, I might consider having them push to "refs/remotes/*".
In theory that's actually much closer to what I'm doing anyways.  That
would force any checkouts to be bare, but it would require lots of
git-foo on the pushing side.  Perhaps some way to "git push" which
asks the remote repository where it wants the stuff?

Alternatively, it might be possible to add ref attributes or a config
option to force detached HEAD checkouts.

Cheers,
Kyle Moffett

Re: [RFC PATCH 0/4] deny push to current branch of non-bare repo

[Jeff King]

On Thu, Nov 13, 2008 at 08:58:50AM -0500, Kyle Moffett wrote:

> I guess the issue comes down to a UI complication.  It would very easy
> for me to tell somebody how to check out and test their branch in my
> testbed if I'm not around, except for that little bit of arcane
> syntax.  Moreover, the consequences if they forget are really

If the problem is merely the syntax, then perhaps that argues for "git
checkout -d" to force detaching.

> frustrating and hard to figure out.  It's also very easy with a GUI to
> do the simple *rightclick branch, click "Checkout"*, but would be much
> harder to do the detached HEAD checkout correctly.

And again, perhaps this argues for a "Detach" option in the GUI.

But I have to admit, this is a pretty infrequently-used use-case. I
detach all the time when looking at non-branches, but I can't think of
the last time I used "ref^0" to detach intentionally.

> If it didn't involve reconfiguring a lot of other people's
> repositories, I might consider having them push to "refs/remotes/*".
> In theory that's actually much closer to what I'm doing anyways.  That
> would force any checkouts to be bare, but it would require lots of
> git-foo on the pushing side.  Perhaps some way to "git push" which
> asks the remote repository where it wants the stuff?

Or git-receive could even just silently munge the incoming refs when
writing them out (i.e., it exposes "refs/test/*" as "refs/heads/*", and
when you ask to write "refs/heads/foo" it writes "refs/test/foo"
instead).

Though that sort of lying feels a little wrong to me, since the pushing
side will incorrectly update its tracking branches. It wouldn't so bad
if the "fetch" side respected the munging, too.

But again, this seems uncommon enough that it is not worth trying to
implement something too clever.

> Alternatively, it might be possible to add ref attributes or a config
> option to force detached HEAD checkouts.

I think that is a more sensible solution. Your workflow is not about
"sometimes I want to detach the HEAD" but rather "in this particular
repo, we should _always_ detach the HEAD." Which a config option
represents very nicely.

-Peff

Re: [RFC PATCH 0/4] deny push to current branch of non-bare repo

[Leo Razoumov]

On 11/8/08, Jeff King <peff@peff.net> wrote:
> On Fri, Nov 07, 2008 at 03:16:53PM -0800, Junio C Hamano wrote:
>
>  > > The FAQ even says "don't do this until you know what you are doing." So
>  > > the safety valve is configurable, so that those who know what they are
>  > > doing can switch it off.
>  >
>  > "We are breaking your existing working setup but you can add a new
>  > configuration to unbreak it" should not be done lightly.  I think as the
>  > end result it is a reasonable thing to aim for for this particular
>  > feature, but we do need a transition plan patch in between that introduces
>  > a step that warns but not forbids.  We can ship 1.6.1 with it and then
>  > switch the default to forbid in 1.6.3, for example.
>
>
> Yeah, I was kind of hoping we could assume that anybody relying on this
>  behavior was somewhat insane, and wouldn't be too upset when it broke.

I do not think that having a work-flow different from yours deserves a
"somewhat insane" label. But let us consider the consequences of
banning push into a (current branch) non-bare repo. To propagate
changes to such a non-bare repo there are two remaining alternatives
neither of which is fully satisfactory:

(1) Switch target's current branch to something else (prevent a
conflict) before pushing and then restore it back after the push

(2) Use git-fetch from the target.

Method (1) is no better than what is available today with "git reset
--hard" to sync working directory.
Method (2) is even worse, because git-fetch provides no control of
what branches/tags to fetch, it sucks everything in from all branches.
"git-push", OTOH, can be instructed to be very selective.

Here is an example of such a work-flow

Foo.git -- main bare repo of the project
Foo.wip -- everyday "work in progress" repo. Cloned from Foo.git.
Pushes to Foo.git
Foo.wip.insane -- experimental "crazy" stuff cloned from Foo.wip.
Pushed to Foo.wip

Proposed patch makes this work flow impossible (cannot push from
Foo.wip.insane to Foo.wip)

--Leo--

Re: [RFC PATCH 0/4] deny push to current branch of non-bare repo

[Junio C Hamano]

"Leo Razoumov" <slonik.az@gmail.com> writes:

> On 11/8/08, Jeff King <peff@peff.net> wrote:
>> On Fri, Nov 07, 2008 at 03:16:53PM -0800, Junio C Hamano wrote:
>>
>>  > > The FAQ even says "don't do this until you know what you are doing." So
>>  > > the safety valve is configurable, so that those who know what they are
>>  > > doing can switch it off.
>>  >
>>  > "We are breaking your existing working setup but you can add a new
>>  > configuration to unbreak it" should not be done lightly.  I think as the
>>  > end result it is a reasonable thing to aim for for this particular
>>  > feature, but we do need a transition plan patch in between that introduces
>>  > a step that warns but not forbids.  We can ship 1.6.1 with it and then
>>  > switch the default to forbid in 1.6.3, for example.
>>
>>
>> Yeah, I was kind of hoping we could assume that anybody relying on this
>>  behavior was somewhat insane, and wouldn't be too upset when it broke.
>
> I do not think that having a work-flow different from yours deserves a
> "somewhat insane" label. But let us consider the consequences of
> banning push into a (current branch) non-bare repo. To propagate
> changes to such a non-bare repo there are two remaining alternatives
> neither of which is fully satisfactory:
>
> (1) Switch target's current branch to something else (prevent a
> conflict) before pushing and then restore it back after the push
>
> (2) Use git-fetch from the target.

(3) set the config in the target repository to allow such a push
    regardless of the git version.

Remember, I am in the third camp in this topic myself.

Re: [RFC PATCH 0/4] deny push to current branch of non-bare repo

[Leo Razoumov]

On 12/1/08, Junio C Hamano <gitster@pobox.com> wrote:
> "Leo Razoumov" <slonik.az@gmail.com> writes:
>
>  > On 11/8/08, Jeff King <peff@peff.net> wrote:
>  >> On Fri, Nov 07, 2008 at 03:16:53PM -0800, Junio C Hamano wrote:
>  >>
>  >>  > > The FAQ even says "don't do this until you know what you are doing." So
>  >>  > > the safety valve is configurable, so that those who know what they are
>  >>  > > doing can switch it off.
>  >>  >
>  >>  > "We are breaking your existing working setup but you can add a new
>  >>  > configuration to unbreak it" should not be done lightly.  I think as the
>  >>  > end result it is a reasonable thing to aim for for this particular
>  >>  > feature, but we do need a transition plan patch in between that introduces
>  >>  > a step that warns but not forbids.  We can ship 1.6.1 with it and then
>  >>  > switch the default to forbid in 1.6.3, for example.
>  >>
>  >>
>  >> Yeah, I was kind of hoping we could assume that anybody relying on this
>  >>  behavior was somewhat insane, and wouldn't be too upset when it broke.
>  >
>  > I do not think that having a work-flow different from yours deserves a
>  > "somewhat insane" label. But let us consider the consequences of
>  > banning push into a (current branch) non-bare repo. To propagate
>  > changes to such a non-bare repo there are two remaining alternatives
>  > neither of which is fully satisfactory:
>  >
>  > (1) Switch target's current branch to something else (prevent a
>  > conflict) before pushing and then restore it back after the push
>  >
>  > (2) Use git-fetch from the target.
>
>
> (3) set the config in the target repository to allow such a push
>     regardless of the git version.
>
>  Remember, I am in the third camp in this topic myself.

Junio,
thanks for supporting the "third way". I am not sure whether I
interpret it correctly but in the same thread several message earlier
you wrote "We can ship 1.6.1 with it and then switch the default to
forbid in 1.6.3, for example". With the default set to "deny" it would
be useful if the git-push error message will indicate what config
variable to set in order to reverse the denial.

--Leo--

Re: [RFC PATCH 0/4] deny push to current branch of non-bare repo

[Jeff King]

On Mon, Dec 01, 2008 at 09:22:43PM -0500, Leo Razoumov wrote:

> I do not think that having a work-flow different from yours deserves a
> "somewhat insane" label. But let us consider the consequences of

  a) you are responding to a nearly month-old message. Please read the
     rest of the thread where we decide that it is not so insane, and
     that the behavior should be configurable with a default of "warn"
     at least for now.

  b) My comment was not that it is insane simply because it is different
     from mine. It is because it creates a dangerous situation (where
     dangerous implies changes might be silently lost) which requires
     manual intervention to fix, and which the user was given no warning
     whatsoever about. It is a direct response to frequent complaints on
     the list about users getting bit by this.

> (1) Switch target's current branch to something else (prevent a
> conflict) before pushing and then restore it back after the push
> 
> (2) Use git-fetch from the target.

(3) Use git-reset --hard, but set a config variable that says "I know
what I'm doing." You don't even have to do it per-repo, you can do it
per-user.

(4) Push into a non-current branch and merge from the target.

> Method (2) is even worse, because git-fetch provides no control of
> what branches/tags to fetch, it sucks everything in from all branches.
> "git-push", OTOH, can be instructed to be very selective.

Er, what? git-fetch takes a refspec very similar to the ones used by
git-push. The real reason that (2) is not an acceptable solution is that
you can't necessarily connect to the source repo (e.g., it is on your
workstation with no ssh or git server running).

-Peff

Re: [RFC PATCH 0/4] deny push to current branch of non-bare repo

[Leo Razoumov]

On 12/1/08, Jeff King <peff@peff.net> wrote:
> [..snip..]
>  >
> Er, what? git-fetch takes a refspec very similar to the ones used by
>  git-push. The real reason that (2) is not an acceptable solution is that
>  you can't necessarily connect to the source repo (e.g., it is on your
>  workstation with no ssh or git server running).
>
>  -Peff

I am sorry, I had to be more accurate in my wording. "git fetch" with
no explicit refspecs fetches everything in. It is quite cumbersome to
form a refspec for git-fetch operation if you are  not logged in into
the "source repo" machine. git-fetch does not have a --dry-run option
to help discover all the branch/tag names on the source side needed
for a meaningful refspec. "git-push -v --dry-run" allows one to
experiment and see what branches/tags exist at the destination and
form refspecs selectively. To the best of my knowledge, git-fetch does
not provide such discovery tools.

--Leo--