[PATCH] Switch receive.denyCurrentBranch to "refuse"

[PATCH] Switch receive.denyCurrentBranch to "refuse"

[Johannes Schindelin]

Many, many users set up non-bare repositories on their server, and are
confused that the working directory is not updated.

The reason may be that they did not read the manual, or that they found
"helpful" walk-throughs via Google, or that they did not understand the
concepts behind Git.

Or, the reason could be that we made a design mistake, and that the
number of puzzled new users should tell us something.

Granted, we wanted to have a longer grace period for old-timers, but
let's face it:

- old-timers will have to edit their configs at some stage anyway,

- for old-timers, it will be a matter of less than a minute,

- new-timers will not spend frustrated hours, and

- since there are many more new-timers now than old-timers, we should
  cater for them anyway.

To make it easier for old-timers, the error message was enhanced to
suggest how to allow updating the current branch easily.

For the tests relying on the old behavior, receive.denyCurrentBranch
was set to false, to avoid breaking them.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	Let's be honest here, I have not much respect for users who fail 
	to read up enough to understand what they are doing.

	But hearing from those users constantly is really unnerving.  And 
	it would be a one-time cost to old-timers.

 builtin-receive-pack.c      |    9 ++++++---
 t/t5400-send-pack.sh        |    5 ++++-
 t/t5401-update-hooks.sh     |    1 +
 t/t5405-send-pack-rewind.sh |    1 +
 t/t5516-fetch-push.sh       |    1 +
 t/t5517-push-mirror.sh      |    3 ++-
 t/t5521-pull-symlink.sh     |    1 +
 t/t5701-clone-local.sh      |    2 +-
 8 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index 6564a97..e9510fc 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -19,7 +19,7 @@ enum deny_action {
 
 static int deny_deletes = 0;
 static int deny_non_fast_forwards = 0;
-static enum deny_action deny_current_branch = DENY_WARN;
+static enum deny_action deny_current_branch = DENY_REFUSE;
 static int receive_fsck_objects;
 static int receive_unpack_limit = -1;
 static int transfer_unpack_limit = -1;
@@ -239,9 +239,12 @@ static const char *update(struct command *cmd)
 			" that are now in HEAD.");
 		break;
 	case DENY_REFUSE:
-		if (!is_ref_checked_out(name))
+		if (is_bare_repository() || !is_ref_checked_out(name))
 			break;
-		error("refusing to update checked out branch: %s", name);
+		error("refusing to update checked out branch: %s\n"
+			"if you know what you are doing, you can allow it by "
+			"setting\n\n"
+			"\tgit config receive.denyCurrentBranch true\n", name);
 		return "branch is currently checked out";
 	}
 
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index b21317d..240380c 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -33,6 +33,7 @@ test_expect_success setup '
 	git update-ref HEAD "$commit" &&
 	git clone ./. victim &&
 	cd victim &&
+	git config receive.denyCurrentBranch false &&
 	git log &&
 	cd .. &&
 	git update-ref HEAD "$zero" &&
@@ -131,13 +132,15 @@ test_expect_success \
 	cd .. &&
 	git clone parent child && cd child && git push --all &&
 	cd ../parent &&
-	git branch -a >branches && ! grep origin/master branches
+	git branch -a >branches && ! grep origin/master branches &&
+	cd ..
 '
 
 rewound_push_setup() {
 	rm -rf parent child &&
 	mkdir parent && cd parent &&
 	git init && echo one >file && git add file && git commit -m one &&
+	git config receive.denyCurrentBranch false &&
 	echo two >file && git commit -a -m two &&
 	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..7f04b64 100755
--- a/t/t5401-update-hooks.sh
+++ b/t/t5401-update-hooks.sh
@@ -18,6 +18,7 @@ test_expect_success setup '
 	git update-ref refs/heads/master $commit0 &&
 	git update-ref refs/heads/tofail $commit1 &&
 	git clone ./. victim &&
+	GIT_DIR=victim/.git git config receive.denyCurrentBranch false &&
 	GIT_DIR=victim/.git git update-ref refs/heads/tofail $commit1 &&
 	git update-ref refs/heads/master $commit1 &&
 	git update-ref refs/heads/tofail $commit0
diff --git a/t/t5405-send-pack-rewind.sh b/t/t5405-send-pack-rewind.sh
index cb9aacc..37c1f23 100755
--- a/t/t5405-send-pack-rewind.sh
+++ b/t/t5405-send-pack-rewind.sh
@@ -6,6 +6,7 @@ test_description='forced push to replace commit we do not have'
 
 test_expect_success setup '
 
+	git config receive.denyCurrentBranch false &&
 	>file1 && git add file1 && test_tick &&
 	git commit -m Initial &&
 
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 4426df9..9ca2730 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -12,6 +12,7 @@ mk_empty () {
 	(
 		cd testrepo &&
 		git init &&
+		git config receive.denyCurrentBranch false &&
 		mv .git/hooks .git/hooks-disabled
 	)
 }
diff --git a/t/t5517-push-mirror.sh b/t/t5517-push-mirror.sh
index ea49ded..0a31a4e 100755
--- a/t/t5517-push-mirror.sh
+++ b/t/t5517-push-mirror.sh
@@ -19,7 +19,8 @@ mk_repo_pair () {
 	mkdir mirror &&
 	(
 		cd mirror &&
-		git init
+		git init &&
+		git config receive.denyCurrentBranch false
 	) &&
 	mkdir master &&
 	(
diff --git a/t/t5521-pull-symlink.sh b/t/t5521-pull-symlink.sh
index 5672b51..736c24e 100755
--- a/t/t5521-pull-symlink.sh
+++ b/t/t5521-pull-symlink.sh
@@ -14,6 +14,7 @@ test_description='pulling from symlinked subdir'
 #
 # The working directory is subdir-link.
 
+git config receive.denyCurrentBranch false
 mkdir subdir
 echo file >subdir/file
 git add subdir/file
diff --git a/t/t5701-clone-local.sh b/t/t5701-clone-local.sh
index 3559d17..06b2f13 100755
--- a/t/t5701-clone-local.sh
+++ b/t/t5701-clone-local.sh
@@ -119,7 +119,7 @@ test_expect_success 'bundle clone with nonexistent HEAD' '
 test_expect_success 'clone empty repository' '
 	cd "$D" &&
 	mkdir empty &&
-	(cd empty && git init) &&
+	(cd empty && git init && git config receive.denyCurrentBranch false) &&
 	git clone empty empty-clone &&
 	test_tick &&
 	(cd empty-clone
-- 
1.6.1.2.531.g6f52

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Jay Soffian]

On Thu, Jan 29, 2009 at 7:34 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> Or, the reason could be that we made a design mistake, and that the
> number of puzzled new users should tell us something.

I happen to have spent some time looking at Mercurial the other day
since I was curious how it's evolved since I last played with it, and
so w/that perspective, I think that git did make a small design
mistake. With mercurial, pull and push are symmetric opposites.
Neither, by default, updates the working copy.

(Confusingly for users of both mercurial and git, the mercurial
equivalent of "git pull" is "hg fetch". Doh.)

Anyway, I think that this may be what leads to confusion. git pull
updates the working copy, and beginners I think expect that push,
which sounds like the opposite of pull, ought do the same thing.

j.

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Asheesh Laroia]

On Fri, 30 Jan 2009, Johannes Schindelin wrote:

> 	case DENY_REFUSE:
> -		if (!is_ref_checked_out(name))
> +		if (is_bare_repository() || !is_ref_checked_out(name))
> 			break;
> -		error("refusing to update checked out branch: %s", name);
> +		error("refusing to update checked out branch: %s\n"
> +			"if you know what you are doing, you can allow it by "
> +			"setting\n\n"
> +			"\tgit config receive.denyCurrentBranch true\n", name);

It seems like those new users you're trying to protect could use an 
additional sentence, like:

 	"A bare repository would not have this issue."

or

 	"You may prefer to have a bare repository instead."

Being told how to do it right is even better than being told that you're 
doing it wrong. (-:

-- Asheesh.

-- 
Fame is a vapor; popularity an accident; the only earthly certainty is
oblivion.
 		-- Mark Twain

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Dave Abrahams]

Asheesh Laroia <asheesh <at> asheesh.org> writes:


> Being told how to do it right is even better than being told that you're 
> doing it wrong. 

I would prefer if Git would simply "not refuse," but barring that, a more
verbose error would be helpful.  I knew what I was doing in
http://github.com/jelmer/dulwich/issues/issue/17, but it took me a
long time  to even notice git's message.

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Junio C Hamano]

Asheesh Laroia <asheesh@asheesh.org> writes:

> On Fri, 30 Jan 2009, Johannes Schindelin wrote:
>
>> 	case DENY_REFUSE:
>> +		if (is_bare_repository() || !is_ref_checked_out(name))
>> 			break;
>> +		error("refusing to update checked out branch: %s\n"
>> +			"if you know what you are doing, you can allow it by "
>> +			"setting\n\n"
>> +			"\tgit config receive.denyCurrentBranch true\n", name);
>
> Being told how to do it right is even better than being told that
> you're doing it wrong. (-:

Of course you are correct, but there are two _right ways_ that are
completely different, depending on how the repository you are pushing
into is meant to be used:

 - If you are using it as a shared central repository, a distribution
   point, or a back-up location, you don't need a working tree, and
   as you say, the "checked out branch" condition will not trigger, if
   you made it a bare one.

 - People do wish a way to keep a repository with a checkout, and that is
   often the reason why this codepath is triggered.  They want a checkout
   in the repository (perhaps they are serving the files in them from a
   webserver).  For them, "pushing into it" is not the ultimate goal, but
   "having its working tree and keeping it up-to-date" is.  For that,
   pushing into a "reception branch" and merging that to the checkout from
   the post-update hook is probably the right way (Cf. [*1*] especially is
   "See also ...").

Also I do not think it would help users to suggest "bare repository"
even for the first class of users.

 - If the user knows what a "bare" repository is, the user would realize
   "Hmm, I am not allowed to push to the checked out branch?  Wait, this
   repository does not even need a working tree, so if I make it a bare
   one, I wouldn't have any checked out branch by definition and I
   wouldn't have this issue" without being told.

 - If the user does not know what a "bare" repository is, the user may not
   even realize that the target repository does not have to have a working
   tree.  In such a case, there won't be a mental "click" between "checked
   out" and "bare" anyway.  The added message to suggest "bare" will be
   another line of unintelligible gitspeak in the message to them.


[Reference]

*1* https://git.wiki.kernel.org/index.php/GitFaq#Why_won.27t_I_see_changes_in_the_remote_repo_after_.22git_push.22.3F

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Junio C Hamano]

I do not think this improves anything.

@@ -239,9 +239,12 @@ static const char *update(struct command *cmd)
 			" that are now in HEAD.");
 		break;
 	case DENY_REFUSE:
-		if (!is_ref_checked_out(name))
+		if (is_bare_repository() || !is_ref_checked_out(name))
 			break;
-		error("refusing to update checked out branch: %s", name);
+		error("refusing to update checked out branch: %s\n"
+			"if you know what you are doing, you can allow it by "
+			"setting\n\n"
+			"\tgit config receive.denyCurrentBranch true\n", name);
 		return "branch is currently checked out";
 	}
 
As the message I am currently getting from such a push is:

$ git push ../victim-010 next:master
Total 0 (delta 0), reused 0 (delta 0)
warning: updating the currently checked out branch; this may cause confusion,
as the index and working tree do not reflect changes that are now in HEAD.
To ../victim-010
   a34a9db..d79e69c  next -> master

which is much better than what you did.  It at least tries to explain why
it is warning, even though I think it has a huge room for improvement.

Saying "If you know what you are doing" never works in practice.  It can
serve as an excuse for you to later say, "See, I told you so", but that is
the only usefulness of the expression, and everybody, especially the most
clueless people, *think* they know what they are doing.


You alluded that we wanted to make grace period much longer, but you want
to cut it short.  I think it is a huge mistake.  The warning has only been
there for the last two months, and only can be seen from v1.6.1-rc1 or
newer software.  These new people even haven't a chance to learn from the
existing warning.


I think what would work much better would be a patch that keeps the
warn-but-allow as the default, but clarifies the warning message.  Say
these things in separate paragraphs, perhaps in red blinking letters:

 (1) what symptoms, that are easily observable by the most novice users,
     are caused by "index and work tree going out of sync" the warning
     talks about, and why that would not be what they want;

 (2) if the user did not mean to do it (and the user can tell by observing
     the symptom described in the previous point), describe what needs to
     be done to recover from the fallout this push has caused (we do not
     need a recipe; pointing at a URL or manpage is fine), and what switch
     to flip to prevent herself from doing it again in the future;

 (3) if the user did mean it, and finds the above two big warning
     annoying, what switch to flip to squelch the warning for future
     pushes.

The goal of the warning should be to *force* people *choose*, either to
silently-allow (aka DENY_IGNORE) or refuse (DENY_REFUSE), and give enough
information for them to make an informed decision.  We can afford to be
annoyingly long, loud and verbose there.

On the other hand, you cannot make the message for DENY_REFUSE annoyingly
long, as people may have already chosen to say "please refuse my push into
a live branch".

If you are making "refuse" the default, an annoyingly long message is even
worse.  "Yeah, thanks for stopping me, but you do not have to remind me
every time that I made a mistake in large red letters.  I perfectly well
know what I am doing, I perfectly well know that I did not want to push
into that branch, I just made a mistake---you do not have to be so loud".

I suspect that you cannot even be long enough to be informative, not to
annoy people.

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Johannes Schindelin]

Hi,

On Thu, 29 Jan 2009, Junio C Hamano wrote:

> @@ -239,9 +239,12 @@ static const char *update(struct command *cmd)
>  			" that are now in HEAD.");
>  		break;
>  	case DENY_REFUSE:
> -		if (!is_ref_checked_out(name))
> +		if (is_bare_repository() || !is_ref_checked_out(name))
>  			break;
> -		error("refusing to update checked out branch: %s", name);
> +		error("refusing to update checked out branch: %s\n"
> +			"if you know what you are doing, you can allow it by "
> +			"setting\n\n"
> +			"\tgit config receive.denyCurrentBranch true\n", name);
>  		return "branch is currently checked out";
>  	}
>  
> As the message I am currently getting from such a push is:
> 
> $ git push ../victim-010 next:master
> Total 0 (delta 0), reused 0 (delta 0)
> warning: updating the currently checked out branch; this may cause confusion,
> as the index and working tree do not reflect changes that are now in HEAD.
> To ../victim-010
>    a34a9db..d79e69c  next -> master
> 
> which is much better than what you did.  It at least tries to explain why
> it is warning, even though I think it has a huge room for improvement.

I do not really care about the output.  You are probably right, it should 
be different from what I proposed.

> You alluded that we wanted to make grace period much longer, but you 
> want to cut it short.  I think it is a huge mistake.  The warning has 
> only been there for the last two months, and only can be seen from 
> v1.6.1-rc1 or newer software.  These new people even haven't a chance to 
> learn from the existing warning.

In reality, people will not learn from the warning.  Those that are 
old-timers (and who should be warned in the first place, instead of 
refuses) will just happily ignore that there was a warning: the command 
they used so often and the worked all the time just happened to work -- 
again -- no matter what the output is.

But we are really hurting new users, and let's face it, the balance of 
time cost currently is in a huge favor of the old-timers there.

Not only are there many more newbies these days than old timers.

No, the _time_ spent by an old-timer to read an appropriate message and 
fix the setup would be _substantially_ shorter than the _hours_ of 
frustration a newbie spends on the issue.

And we claim to make decentralized repositories easy.

> I think what would work much better would be a patch that keeps the
> warn-but-allow as the default, but clarifies the warning message.

As I said, I am _convinced_ that a warning will do nothing at all.  Just 
like the warning about the dashed commands, nobody who should be concerned 
will notice it.

>  (1) what symptoms, that are easily observable by the most novice users,
>      are caused by "index and work tree going out of sync" the warning
>      talks about, and why that would not be what they want;
> 
>  (2) if the user did not mean to do it (and the user can tell by observing
>      the symptom described in the previous point), describe what needs to
>      be done to recover from the fallout this push has caused (we do not
>      need a recipe; pointing at a URL or manpage is fine), and what switch
>      to flip to prevent herself from doing it again in the future;
> 
>  (3) if the user did mean it, and finds the above two big warning
>      annoying, what switch to flip to squelch the warning for future
>      pushes.
> 
> The goal of the warning should be to *force* people *choose*, either to
> silently-allow (aka DENY_IGNORE) or refuse (DENY_REFUSE), and give enough
> information for them to make an informed decision.  We can afford to be
> annoyingly long, loud and verbose there.
> 
> On the other hand, you cannot make the message for DENY_REFUSE annoyingly
> long, as people may have already chosen to say "please refuse my push into
> a live branch".
> 
> If you are making "refuse" the default, an annoyingly long message is even
> worse.  "Yeah, thanks for stopping me, but you do not have to remind me
> every time that I made a mistake in large red letters.  I perfectly well
> know what I am doing, I perfectly well know that I did not want to push
> into that branch, I just made a mistake---you do not have to be so loud".
> 
> I suspect that you cannot even be long enough to be informative, not to
> annoy people.

Let's reap all the opinions about this issue, and then I'll do the wrap-up 
patch.

But this is a serious issue that seriously needs to be coped with.  We are 
getting another generation of "Git is difficult" users that way.

Ciao,
Dscho

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Jeff King]

On Fri, Jan 30, 2009 at 02:24:57PM +0100, Johannes Schindelin wrote:

> Let's reap all the opinions about this issue, and then I'll do the wrap-up 
> patch.

I thought what Junio said was very reasonable (improve the message and
give it some more time to work).

But I honestly do not care that much either way. I probably would have
made the original patch default to "deny" if not for discussion
recommending to be conservative. On the other hand, I don't think we
have really given the "warning" approach enough time to see whether it
is working (and I don't necessarily disagree with your gut feeling that
it won't work; I am undecided, which leads me to want more data).

-Peff

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Johannes Schindelin]

Hi,

On Fri, 30 Jan 2009, Jeff King wrote:

> On Fri, Jan 30, 2009 at 02:24:57PM +0100, Johannes Schindelin wrote:
> 
> > Let's reap all the opinions about this issue, and then I'll do the 
> > wrap-up patch.
> 
> I thought what Junio said was very reasonable (improve the message and 
> give it some more time to work).
> 
> But I honestly do not care that much either way. I probably would have 
> made the original patch default to "deny" if not for discussion 
> recommending to be conservative. On the other hand, I don't think we 
> have really given the "warning" approach enough time to see whether it 
> is working (and I don't necessarily disagree with your gut feeling that 
> it won't work; I am undecided, which leads me to want more data).

It is not working:

http://groups.google.com/group/msysgit/msg/55b1aa03fbbbefba?dmode=source

(I am simply assuming that the mentioned 1.6.1-preview has the 
warning, since denyCurrentBranch is in v1.6.1-rc1~59^2, and I am too short 
on time to check it in detail (which would mean finding a Windows machine 
and running a test)).

Ciao,
Dscho

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Miklos Vajna]

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

On Fri, Jan 30, 2009 at 01:34:28AM +0100, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
> -		error("refusing to update checked out branch: %s", name);
> +		error("refusing to update checked out branch: %s\n"
> +			"if you know what you are doing, you can allow it by "
> +			"setting\n\n"
> +			"\tgit config receive.denyCurrentBranch true\n", name);

Shouldn't this be

git config receive.denyCurrentBranch ignore

instead of "true"?

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

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Johannes Schindelin]

Hi,

On Fri, 30 Jan 2009, Miklos Vajna wrote:

> On Fri, Jan 30, 2009 at 01:34:28AM +0100, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
> > -		error("refusing to update checked out branch: %s", name);
> > +		error("refusing to update checked out branch: %s\n"
> > +			"if you know what you are doing, you can allow it by "
> > +			"setting\n\n"
> > +			"\tgit config receive.denyCurrentBranch true\n", name);
> 
> Shouldn't this be
> 
> git config receive.denyCurrentBranch ignore
> 
> instead of "true"?

Right.

However, as Junio pointed out, we do not want to give this resolution in 
the error message.  I am now leaning more to something like

	refusing to update checked out branch '%s' in non-bare repository

Hmm?

Old-timers will know "oh, what the hell, I did not mark my repository as 
bare!", and new-timers will no longer be confused.

Ciao,
Dscho

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Miklos Vajna]

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

[ Sorry for the late reply, I did not read my mail recently. ]

On Fri, Jan 30, 2009 at 02:28:39PM +0100, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > Shouldn't this be
> > 
> > git config receive.denyCurrentBranch ignore
> > 
> > instead of "true"?
> 
> Right.
> 
> However, as Junio pointed out, we do not want to give this resolution in 
> the error message.  I am now leaning more to something like
> 
> 	refusing to update checked out branch '%s' in non-bare repository
> 
> Hmm?
> 
> Old-timers will know "oh, what the hell, I did not mark my repository as 
> bare!", and new-timers will no longer be confused.

So in an "I know what I'm doing" mode, is "git config core.bare true" in
a non-bare repo considered as a better workaround than using "git config
receive.denyCurrentBranch ignore"?

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

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> [ Sorry for the late reply, I did not read my mail recently. ]
>
> On Fri, Jan 30, 2009 at 02:28:39PM +0100, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>> > Shouldn't this be
>> > 
>> > git config receive.denyCurrentBranch ignore
>> > 
>> > instead of "true"?
>> 
>> Right.
>> 
>> However, as Junio pointed out, we do not want to give this resolution in 
>> the error message.  I am now leaning more to something like
>> 
>> 	refusing to update checked out branch '%s' in non-bare repository
>> 
>> Hmm?
>> 
>> Old-timers will know "oh, what the hell, I did not mark my repository as 
>> bare!", and new-timers will no longer be confused.
>
> So in an "I know what I'm doing" mode, is "git config core.bare true" in
> a non-bare repo considered as a better workaround than using "git config
> receive.denyCurrentBranch ignore"?

If you have to ask, you do not know what you're doing ;-)

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Jeff King]

On Fri, Jan 30, 2009 at 01:34:28AM +0100, Johannes Schindelin wrote:

> 	Let's be honest here, I have not much respect for users who fail 
> 	to read up enough to understand what they are doing.
> 
> 	But hearing from those users constantly is really unnerving.  And 
> 	it would be a one-time cost to old-timers.

I am not personally opposed to changing this default. I seem to
recall some opposition when this was brought up initially, but I don't
recall any specific reason besides "change is bad". Maybe those who
oppose want to summarize their arguments here.

I was hoping that introducing the warning would cause new users to "get
it". But since this warning was put in place, I think we have still
gotten a few questions on the list about this. I don't know if it simply
because they are on older versions, or if the warning is insufficient.
If the former, then perhaps that argues for leaving it a little longer.

>  	case DENY_REFUSE:
> -		if (!is_ref_checked_out(name))
> +		if (is_bare_repository() || !is_ref_checked_out(name))

Now what is this change about?

> --- a/t/t5701-clone-local.sh
> +++ b/t/t5701-clone-local.sh
> @@ -119,7 +119,7 @@ test_expect_success 'bundle clone with nonexistent HEAD' '
>  test_expect_success 'clone empty repository' '
>  	cd "$D" &&
>  	mkdir empty &&
> -	(cd empty && git init) &&
> +	(cd empty && git init && git config receive.denyCurrentBranch false) &&
>  	git clone empty empty-clone &&
>  	test_tick &&
>  	(cd empty-clone

Perhaps some of these tests would do better to actually just use a bare
repository. That would better match the expected workflow for cloning
empty, anyway.

-Peff

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Johannes Schindelin]

Hi,

On Thu, 29 Jan 2009, Jeff King wrote:

> On Fri, Jan 30, 2009 at 01:34:28AM +0100, Johannes Schindelin wrote:
> 
> > 	Let's be honest here, I have not much respect for users who fail 
> > 	to read up enough to understand what they are doing.
> > 
> > 	But hearing from those users constantly is really unnerving.  And 
> > 	it would be a one-time cost to old-timers.
> 
> I am not personally opposed to changing this default. I seem to
> recall some opposition when this was brought up initially, but I don't
> recall any specific reason besides "change is bad". Maybe those who
> oppose want to summarize their arguments here.

We like to play it safe when changing behavior that does not meet 
expectations of old-timers.

For example, all those early adopters who have forks of the linux-2.6 
repository (and probably that repository itself, too) do not have 
core.bare set.

So whenever an old-timer would upgrade to a new Git _with_ my patch, they 
would need to change their setup.

A one-time cost.

And far easier to accomodate than the push for non-dashed commands (which 
people still seem to grumble about, even if they should have realized by 
now that calling Git through the wrapper exclusively brings so many 
advantages).

> I was hoping that introducing the warning would cause new users to "get 
> it". But since this warning was put in place, I think we have still 
> gotten a few questions on the list about this. I don't know if it simply 
> because they are on older versions, or if the warning is insufficient. 
> If the former, then perhaps that argues for leaving it a little longer.

I would argue it is because users cannot read :-)

> >  	case DENY_REFUSE:
> > -		if (!is_ref_checked_out(name))
> > +		if (is_bare_repository() || !is_ref_checked_out(name))
> 
> Now what is this change about?

I missed the fact that is_ref_checked_out() already checked for that.

> > --- a/t/t5701-clone-local.sh
> > +++ b/t/t5701-clone-local.sh
> > @@ -119,7 +119,7 @@ test_expect_success 'bundle clone with nonexistent HEAD' '
> >  test_expect_success 'clone empty repository' '
> >  	cd "$D" &&
> >  	mkdir empty &&
> > -	(cd empty && git init) &&
> > +	(cd empty && git init && git config receive.denyCurrentBranch false) &&
> >  	git clone empty empty-clone &&
> >  	test_tick &&
> >  	(cd empty-clone
> 
> Perhaps some of these tests would do better to actually just use a bare
> repository.

Right.  I just ran out of time, but did not want to hide the patch from 
the community.

> That would better match the expected workflow for cloning empty, anyway.

Well, I did not want to mix up the two of them.  Besides, I have this 
patch in my personal tree for quite some time now, always wanting to clean 
it up enough to send it...)

Ciao,
Dscho

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Johannes Sixt]

Johannes Schindelin schrieb:
> +		error("refusing to update checked out branch: %s\n"
> +			"if you know what you are doing, you can allow it by "
> +			"setting\n\n"
> +			"\tgit config receive.denyCurrentBranch true\n", name);

Oh, fscking hell, I should have screamed out loudly when Jeff named this
option "denyCurrentBranch" instead of "allowCurrentBranch". It's all too
easy to fall into the trap, like you here.

Sigh.

-- J we-don't-need-no-double-negations 6t

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Jeff King]

On Fri, Jan 30, 2009 at 08:17:48AM +0100, Johannes Sixt wrote:

> Johannes Schindelin schrieb:
> > +		error("refusing to update checked out branch: %s\n"
> > +			"if you know what you are doing, you can allow it by "
> > +			"setting\n\n"
> > +			"\tgit config receive.denyCurrentBranch true\n", name);
> 
> Oh, fscking hell, I should have screamed out loudly when Jeff named this
> option "denyCurrentBranch" instead of "allowCurrentBranch". It's all too
> easy to fall into the trap, like you here.

Sorry. ;P

On the other hand, you also missed the boat on receive.denyDeletes and
receive.denyNonFastForwards.

-Peff

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Johannes Schindelin]

Hi,

On Fri, 30 Jan 2009, Jeff King wrote:

> On Fri, Jan 30, 2009 at 08:17:48AM +0100, Johannes Sixt wrote:
> 
> > Johannes Schindelin schrieb:
> > > +		error("refusing to update checked out branch: %s\n"
> > > +			"if you know what you are doing, you can allow it by "
> > > +			"setting\n\n"
> > > +			"\tgit config receive.denyCurrentBranch true\n", name);
> > 
> > Oh, fscking hell, I should have screamed out loudly when Jeff named this
> > option "denyCurrentBranch" instead of "allowCurrentBranch". It's all too
> > easy to fall into the trap, like you here.
> 
> Sorry. ;P
> 
> On the other hand, you also missed the boat on receive.denyDeletes and
> receive.denyNonFastForwards.

The idea with these is that they are _booleans_, and therefore

	[receive]
		denyDeletes

is something natural to write, because "denyDeletes" is _not_ the default.

However, with denyCurrentBranch we wanted to change the default in the 
long run, so I agree it was a not-so-brilliant choice.

Ciao,
Dscho

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Jeff King]

On Fri, Jan 30, 2009 at 02:23:02PM +0100, Johannes Schindelin wrote:

> > On the other hand, you also missed the boat on receive.denyDeletes and
> > receive.denyNonFastForwards.
> 
> The idea with these is that they are _booleans_, and therefore
> 
> 	[receive]
> 		denyDeletes
> 
> is something natural to write, because "denyDeletes" is _not_ the default.
> 
> However, with denyCurrentBranch we wanted to change the default in the 
> long run, so I agree it was a not-so-brilliant choice.

Good point. I do agree that allowCurrentBranch would have been a better
name, but I don't know that it is worth the pain now of adding new
config plus supporting the old name forever.

-Peff

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Jay Soffian]

On Thu, Jan 29, 2009 at 7:34 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> Many, many users set up non-bare repositories on their server, and are
> confused that the working directory is not updated.

This comes up on the list from time-to-time and is even in the FAQ. It
has even been suggested that HEAD be detached when pushing into a
non-bare repository, but I am not suggesting that again.

I wonder if it might be helpful to teach clone to setup a push line in
the cloned repo. i.e.:

[remote "origin"]
	url = ...
	fetch = +refs/heads/*:refs/remotes/origin/*
	push = refs/heads/*:refs/remotes/origin/*

This could be a configurable default behavior when cloning from a
non-bare repo (can that be determined?) and/or as a switch
(--satellite perhaps?).

j.

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Jeff King]

On Fri, Jan 30, 2009 at 11:17:49AM -0500, Jay Soffian wrote:

> I wonder if it might be helpful to teach clone to setup a push line in
> the cloned repo. i.e.:
> 
> [remote "origin"]
> 	url = ...
> 	fetch = +refs/heads/*:refs/remotes/origin/*
> 	push = refs/heads/*:refs/remotes/origin/*

That refspec doesn't make sense, since the downstream is not the
"origin" to the upstream repo. But I don't think this is a good
solution; it is fundamentally changing the layout of pushed branches in
the upstream repo, which is going to cause a lot of confusion.

> This could be a configurable default behavior when cloning from a
> non-bare repo (can that be determined?) and/or as a switch
> (--satellite perhaps?).

I don't think you can tell whether a repo you are cloning is bare.

-Peff

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Johannes Schindelin]

Hi,

On Fri, 30 Jan 2009, Jay Soffian wrote:

> On Thu, Jan 29, 2009 at 7:34 PM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > Many, many users set up non-bare repositories on their server, and are 
> > confused that the working directory is not updated.
> 
> This comes up on the list from time-to-time and is even in the FAQ.

So much so that it is high time we admitted that we have a design bug 
there.

> It has even been suggested that HEAD be detached when pushing into a 
> non-bare repository, but I am not suggesting that again.

No, because that would be as wrong as trying to update the working 
directory in any other way.  (Not only is it possible that you are a 
git-shell user, in which case you have no business meddling with the 
working directory -- or the symbolic ref HEAD -- to begin with, but you 
also run into the problem that you might not know where the working 
directory is at all, let alone if there is one.)

So it is a good thing you are not suggesting it again.

> I wonder if it might be helpful to teach clone to setup a push line in
> the cloned repo. i.e.:
> 
> [remote "origin"]
> 	url = ...
> 	fetch = +refs/heads/*:refs/remotes/origin/*
> 	push = refs/heads/*:refs/remotes/origin/*
> 
> This could be a configurable default behavior when cloning from a
> non-bare repo (can that be determined?) and/or as a switch
> (--satellite perhaps?).

As Peff commented, this would be horribly wrong if the remote has a 
different "origin" remote.  Not forcing the push does not help either, it 
is still wrong.

But I think there is an even more fundamental problem: You do not want 
that default push.  We have "push only those refs the remote and the local 
repository share" rule for a reason.  It is way too easy to publish 
something you did not mean to publish otherwise.

Ciao,
Dscho

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Jay Soffian]

On Fri, Jan 30, 2009 at 12:01 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> As Peff commented, this would be horribly wrong if the remote has a
> different "origin" remote.  Not forcing the push does not help either, it
> is still wrong.

Got it. Here was my impression of the work-flow we're trying to help
beginners with:

machineA$ mkdir repo
machineA$ cd repo
machineA$ git init
machineA$ add, commit, add, commit...

machineB$ git clone ssh://machine1/repo
machineB$ add, commit, add, commit...
machineB$ git push

(And if my impression is wrong, then stop me right here and I'll
shut-up on this thread.)

In this case, the clone operation sets up the repo on B to fetch all
of the branches from the repo on A. But it doesn't do anything to help
the user with pushing the repo from B back to machine A. So perhaps:

git clone --origin machineA --push-as machineB ssh://machineA/repo

[remote "machineA"]
	url = ...
	fetch = +refs/heads/*:refs/remotes/machineA/*
	push = +refs/heads/*:refs/remotes/machineB/*

Now fetch and push are symmetric operations on machineB.

> But I think there is an even more fundamental problem: You do not want
> that default push.  We have "push only those refs the remote and the local
> repository share" rule for a reason.  It is way too easy to publish
> something you did not mean to publish otherwise.

I don't have a good answer for that, other than to say that if user is
setting up symmetric repositories, user wants to push everything.

j.

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Johannes Schindelin]

Hi,

On Fri, 30 Jan 2009, Jay Soffian wrote:

> On Fri, Jan 30, 2009 at 12:01 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > As Peff commented, this would be horribly wrong if the remote has a 
> > different "origin" remote.  Not forcing the push does not help either, 
> > it is still wrong.
> 
> Got it. Here was my impression of the work-flow we're trying to help 
> beginners with:
> 
> machineA$ mkdir repo
> machineA$ cd repo
> machineA$ git init
> machineA$ add, commit, add, commit...
> 
> machineB$ git clone ssh://machine1/repo
> machineB$ add, commit, add, commit...
> machineB$ git push
> 
> (And if my impression is wrong, then stop me right here and I'll
> shut-up on this thread.)

I think your impression is not wrong.

BUT.

You cannot just cater for one workflow and fsck the other workflows over.

You'll have to devise a method that helps the workflow you are interested 
in, but leaves the others alone.

Example: the thing I heard most often was "I want to start this 
repository, but there is nothing in there yet, yet I want other people to 
clone it already so they'll see something when I do."

I admit, it does not strike me sensible, but so does cloning an empty 
repository.  As I could not understand how people would want to vote for 
Bush.  Yet they did, so I guess I'll have to live with it.

Ciao,
Dscho

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Nanako Shiraishi]

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

> You cannot just cater for one workflow and fsck the other workflows over.
>
> You'll have to devise a method that helps the workflow you are interested 
> in, but leaves the others alone.

I think you'd want to repeat that to yourself when you propose to switch
the default for denyCurrentcurrentBranch config to "true" too hastily the
next time?

I don't think your patch matches the tradition of how defaults are changed
in git project. You don't introduce a large change just after the maintainer
hints about going into a freeze for 1.X.Y release when Y isn't zero.

I assume that everybody, including the maintainer who is too heavyweight
and has too much inertia to accept too sudden a change of the course,
wants to eventually make the default to deny pushing to the current
branch. But I think such a change should come at 1.7.0 release at the
earliest, and a constructive thing to do is to put in a patch to 1.6.2
that helps the users with the eventual transition.

How about doing these before the 1.7.0 release?

 1. Add some code to git-clone to set the config to "deny" if it is
    not a bare repository. The reason I think this makes sense is
    because the reason why old-timers want to push into the current
    branch is because they are used to the old layout that doesn't use
    separate remotes. If they use today's git-clone and still want to
    use the old layout, they need to update the config file in the new
    clone anyway. The "deny" is just another thing for them to fix at
    that point.

    I suspect that Junio will not like this in 1.6.2 because it is an
    unannounced and unplanned change in behavior, but I think it is a
    reasonable preparatory step, probably in 1.6.3, before you change
    the default to deny in release 1.7.0.

 2. Reword the warning message as Junio suggested in his response. I
    don't know the details of the code very well, but I think you can
    tell a repository that doesn't have the config at all from a
    repository that has the config set to "warn", and you can use
    "annoyingly long" (in Junio's words) message to force the user set
    the config to a desired value only when pushing into the former
    kind, and say that the default will change to deny in release
    1.7.0. When pushing into the latter, the warning message can be
    shorter (probably you can say "warning: updating the current
    branch in a non-bare repository" and nothing else).

 3. Reword the error message as you proposed to say "error: won't
    update the current branch in a non-bare repository", without
    saying anything else. You want to eventually change the default to
    deny, and there is no point to teach how to allow it to people who
    set the config to deny themselves, nor to new people who created
    their repository with updated git-clone.

    I think this makes sense to do in 1.6.2 release, because the only
    people who will see this message will be the people who set the
    config to deny themselves, especially if you postpone the change
    to git-clone for the upcoming release.

What do people think?

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Junio C Hamano]

Nanako Shiraishi <nanako3@lavabit.com> writes:

> How about doing these before the 1.7.0 release?
> ...
> What do people think?

I haven't manged to convince myself about the "git init" change (I have
the code and also I've looked at the extent of damage the change causes to
the existing test suite), but at least I think it is a sensible suggestion
to differentiate between unconfigured-wwwand-defaults-to-warn case and
configured-to-warn-so-we-warn case.  Something like this.

-- >8 --
Subject: [PATCH] receive-pack: explain what to do when push updates the current branch

This makes "git push" issue a more detailed instruction when a user pushes
into the current branch of a non-bare repository without having an
explicit configuration set to receive.denycurrentbranch.  In such a case,
it will also tell the user that the default will change to refusal in a
future version of git.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-receive-pack.c |   58 +++++++++++++++++++++++++++++++++++------------
 t/t5516-fetch-push.sh  |    6 ++--
 2 files changed, 46 insertions(+), 18 deletions(-)

diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index 6564a97..f2c94fc 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -12,6 +12,7 @@
 static const char receive_pack_usage[] = "git-receive-pack <git-dir>";
 
 enum deny_action {
+	DENY_UNCONFIGURED,
 	DENY_IGNORE,
 	DENY_WARN,
 	DENY_REFUSE,
@@ -19,7 +20,7 @@ enum deny_action {
 
 static int deny_deletes = 0;
 static int deny_non_fast_forwards = 0;
-static enum deny_action deny_current_branch = DENY_WARN;
+static enum deny_action deny_current_branch = DENY_UNCONFIGURED;
 static int receive_fsck_objects;
 static int receive_unpack_limit = -1;
 static int transfer_unpack_limit = -1;
@@ -214,6 +215,35 @@ static int is_ref_checked_out(const char *ref)
 	return !strcmp(head, ref);
 }
 
+static char *warn_unconfigured_deny_msg[] = {
+	"Updating the currently checked out branch may cause confusion,",
+	"as the index and work tree do not reflect changes that are in HEAD."
+	"As a result, you may see the changes you just pushed into it",
+	"reverted when you run 'git diff' over there, and you may want",
+	"to run 'git reset --hard' before starting to work to recover.",
+	"",
+	"You can set 'receive.denyCurrentBranch' configuration variable to",
+	"'refuse' in the repository to forbid pushing into the current branch",
+	"of it."
+	"",
+	"To allow pushing into the current branch, you can set it to 'ignore';",
+	"but this is not recommended unless you really know what you are doing.",
+	"",
+	"To squelch this message, you can set it to 'warn'.",
+	"",
+	"Note that the default will change in a future version of git",
+	"to refuse updating the currentbranch unless you have the",
+	"configuration variable set to either 'ignore' or 'warn'."
+};
+
+static void warn_unconfigured_deny(void)
+{
+	int i;
+	for (i = 0; i < ARRAY_SIZE(warn_unconfigured_deny_msg); i++)
+		warning(warn_unconfigured_deny_msg[i]);
+}
+
+
 static const char *update(struct command *cmd)
 {
 	const char *name = cmd->ref_name;
@@ -227,22 +257,20 @@ 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))
+	if (is_ref_checked_out(name)) {
+		switch (deny_current_branch) {
+		case DENY_IGNORE:
 			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))
+		case DENY_UNCONFIGURED:
+		case DENY_WARN:
+			warning("updating the current branch");
+			if (deny_current_branch == DENY_UNCONFIGURED)
+				warn_unconfigured_deny();
 			break;
-		error("refusing to update checked out branch: %s", name);
-		return "branch is currently checked out";
+		case DENY_REFUSE:
+			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)) {
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 4426df9..89649e7 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -492,7 +492,7 @@ test_expect_success 'warn on push to HEAD of non-bare repository' '
 		git checkout master &&
 		git config receive.denyCurrentBranch warn) &&
 	git push testrepo master 2>stderr &&
-	grep "warning.*this may cause confusion" stderr
+	grep "warning: updating the current branch" stderr
 '
 
 test_expect_success 'deny push to HEAD of non-bare repository' '
@@ -510,7 +510,7 @@ test_expect_success 'allow push to HEAD of bare repository (bare)' '
 		git config receive.denyCurrentBranch true &&
 		git config core.bare true) &&
 	git push testrepo master 2>stderr &&
-	! grep "warning.*this may cause confusion" stderr
+	! grep "warning: updating the current branch" stderr
 '
 
 test_expect_success 'allow push to HEAD of non-bare repository (config)' '
@@ -520,7 +520,7 @@ test_expect_success 'allow push to HEAD of non-bare repository (config)' '
 		git config receive.denyCurrentBranch false
 	) &&
 	git push testrepo master 2>stderr &&
-	! grep "warning.*this may cause confusion" stderr
+	! grep "warning: updating the current branch" stderr
 '
 
 test_expect_success 'fetch with branches' '
-- 
1.6.1.2.312.g5be3c

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Junio C Hamano]

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

> I haven't manged to convince myself about the "git init" change (I have
> the code and also I've looked at the extent of damage the change causes to
> the existing test suite),...

And here is such a patch.

-- >8 --
Subject: [PATCH] Set receive.denyCurrentBranch to true in a new non-bare repository

This prepares new people to get used to the default planned for 1.7.0;
necessary adjustments are done to many tests, as they all assumed the
traditional "only warn but allow updating" semantics.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-init-db.c           |    2 ++
 t/t5400-send-pack.sh        |    2 ++
 t/t5401-update-hooks.sh     |    1 +
 t/t5405-send-pack-rewind.sh |    1 +
 t/t5516-fetch-push.sh       |    1 +
 t/t5517-push-mirror.sh      |    3 ++-
 t/t5521-pull-symlink.sh     |   20 +++++++++++++-------
 t/t5701-clone-local.sh      |    4 +++-
 8 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/builtin-init-db.c b/builtin-init-db.c
index ee3911f..26c10cc 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -250,6 +250,8 @@ static int create_default_files(const char *template_path)
 		    strcmp(git_dir + strlen(work_tree), "/.git")) {
 			git_config_set("core.worktree", work_tree);
 		}
+		if (!reinit)
+			git_config_set("receive.denyCurrentBranch", "refuse");
 	}
 
 	if (!reinit) {
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index b21317d..5c9c277 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -33,6 +33,7 @@ test_expect_success setup '
 	git update-ref HEAD "$commit" &&
 	git clone ./. victim &&
 	cd victim &&
+	git config receive.denyCurrentBranch warn &&
 	git log &&
 	cd .. &&
 	git update-ref HEAD "$zero" &&
@@ -138,6 +139,7 @@ rewound_push_setup() {
 	rm -rf parent child &&
 	mkdir parent && cd parent &&
 	git init && echo one >file && git add file && git commit -m one &&
+	git config receive.denyCurrentBranch warn &&
 	echo two >file && git commit -a -m two &&
 	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..325714e 100755
--- a/t/t5401-update-hooks.sh
+++ b/t/t5401-update-hooks.sh
@@ -18,6 +18,7 @@ test_expect_success setup '
 	git update-ref refs/heads/master $commit0 &&
 	git update-ref refs/heads/tofail $commit1 &&
 	git clone ./. victim &&
+	GIT_DIR=victim/.git git config receive.denyCurrentBranch warn &&
 	GIT_DIR=victim/.git git update-ref refs/heads/tofail $commit1 &&
 	git update-ref refs/heads/master $commit1 &&
 	git update-ref refs/heads/tofail $commit0
diff --git a/t/t5405-send-pack-rewind.sh b/t/t5405-send-pack-rewind.sh
index cb9aacc..4bda18a 100755
--- a/t/t5405-send-pack-rewind.sh
+++ b/t/t5405-send-pack-rewind.sh
@@ -8,6 +8,7 @@ test_expect_success setup '
 
 	>file1 && git add file1 && test_tick &&
 	git commit -m Initial &&
+	git config receive.denyCurrentBranch warn &&
 
 	mkdir another && (
 		cd another &&
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 89649e7..a67ebd0 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -12,6 +12,7 @@ mk_empty () {
 	(
 		cd testrepo &&
 		git init &&
+		git config receive.denyCurrentBranch warn &&
 		mv .git/hooks .git/hooks-disabled
 	)
 }
diff --git a/t/t5517-push-mirror.sh b/t/t5517-push-mirror.sh
index ea49ded..e2ad260 100755
--- a/t/t5517-push-mirror.sh
+++ b/t/t5517-push-mirror.sh
@@ -19,7 +19,8 @@ mk_repo_pair () {
 	mkdir mirror &&
 	(
 		cd mirror &&
-		git init
+		git init &&
+		git config receive.denyCurrentBranch warn
 	) &&
 	mkdir master &&
 	(
diff --git a/t/t5521-pull-symlink.sh b/t/t5521-pull-symlink.sh
index 5672b51..66b5ac1 100755
--- a/t/t5521-pull-symlink.sh
+++ b/t/t5521-pull-symlink.sh
@@ -14,13 +14,19 @@ test_description='pulling from symlinked subdir'
 #
 # The working directory is subdir-link.
 
-mkdir subdir
-echo file >subdir/file
-git add subdir/file
-git commit -q -m file
-git clone -q . clone-repo
-ln -s clone-repo/subdir/ subdir-link
-
+test_expect_success setup '
+	mkdir subdir &&
+	echo file >subdir/file &&
+	git add subdir/file &&
+	git commit -q -m file &&
+	git clone -q . clone-repo &&
+	ln -s clone-repo/subdir/ subdir-link &&
+	(
+		cd clone-repo &&
+		git config receive.denyCurrentBranch warn
+	) &&
+	git config receive.denyCurrentBranch warn
+'
 
 # Demonstrate that things work if we just avoid the symlink
 #
diff --git a/t/t5701-clone-local.sh b/t/t5701-clone-local.sh
index 3559d17..10accc2 100755
--- a/t/t5701-clone-local.sh
+++ b/t/t5701-clone-local.sh
@@ -119,7 +119,9 @@ test_expect_success 'bundle clone with nonexistent HEAD' '
 test_expect_success 'clone empty repository' '
 	cd "$D" &&
 	mkdir empty &&
-	(cd empty && git init) &&
+	(cd empty &&
+	 git init &&
+	 git config receive.denyCurrentBranch warn) &&
 	git clone empty empty-clone &&
 	test_tick &&
 	(cd empty-clone
-- 
1.6.1.2.312.g5be3c

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Jeff King]

On Sat, Jan 31, 2009 at 05:27:45PM -0800, Junio C Hamano wrote:

> I haven't manged to convince myself about the "git init" change (I have
> the code and also I've looked at the extent of damage the change causes to

I think the "git init" change doesn't make sense. In fact, I don't think
such a proposal ever really makes sense (and I have even proposed it in
the past, but others arguments have changed my way of thinking).

The reason is that you are just moving the breakage to a different point
in their workflow. The claim is that it's not OK for this to break:

  cd foo && git init
  git push ;# ok
  ... time passes, git upgrade ...
  git push ;# broken

but it somehow _is_ OK for this to break:

  cd foo && git init
  git push ;# ok
  ... time passes, git upgrade ...
  cd bar && git init
  git push ;# broken

In both cases, you have a sequence of commands that does one thing with
one git version, and something else with another git version. The only
difference is whether your sequence includes git init. So while you
don't break people with existing repositories, you are still breaking
anybody who creates a new one and gets confused when there is new
behavior (or even has scripts which involve repository creation).

So in my opinion either the breakage is serious enough not to allow the
change, or minor enough (compared to the benefit) to allow it. But
changing default config in git init is:

  - a half-way solution that leaves some workflows broken and some not

  - possibly even _worse_, since now we have sacrificed consistency. So
    now users wonder why some of their repos show breakage and some
    don't. Or why a particular behavior goes away when they try to write
    a test case that involves creating a new test repo.

And note that it doesn't matter whether you think the right path is to
make the change or not: I am only arguing here against this sort of
half-way technique.

> -- >8 --
> Subject: [PATCH] receive-pack: explain what to do when push updates the current branch
> 
> This makes "git push" issue a more detailed instruction when a user pushes
> into the current branch of a non-bare repository without having an
> explicit configuration set to receive.denycurrentbranch.  In such a case,
> it will also tell the user that the default will change to refusal in a
> future version of git.

I think this is a definite improvement over the current behavior. As I
said before, I am not sure what is the right path (though I think I am
leaning towards leaving the warning longer based on the recent
discussion), but if we are to leave the default to warn and not refuse,
I think this should definitely be applied.

A few comments on the specific message:

>  }
>  
> +static char *warn_unconfigured_deny_msg[] = {
> +	"Updating the currently checked out branch may cause confusion,",
> +	"as the index and work tree do not reflect changes that are in HEAD."
> +	"As a result, you may see the changes you just pushed into it",

Missing comma between lines 2 and 3, which results in an overly long
line in the output.

> +	"You can set 'receive.denyCurrentBranch' configuration variable to",
> +	"'refuse' in the repository to forbid pushing into the current branch",
> +	"of it."

Maybe this should specifically say "remote repository". If you
understand how the feature works, it is obvious that you must do it that
way, but for less advanced users it is not even clear that this text is
being generated by the remote end.

> +	"To allow pushing into the current branch, you can set it to 'ignore';",
> +	"but this is not recommended unless you really know what you are doing."

I thought somebody (you?) argued against the phrase "unless you really
know what you are doing". And it is better here in context explaining
the general issue. But as a user, now I have to ask: do I know what I am
doing, and if not, how do I find out?

The two obvious solutions for people who "know what they are doing" are
running "git reset --hard", and installing a hook that does something
sensible. I don't know if it is worth mentioning them here (the former
is mentioned earlier in the message, but that doesn't necessarily mean
the user understands all the implications). Since there are so many
subtleties to explain, maybe it make sense to simply put in a pointer to
an expanded discussion in the "git push" manpage?

-Peff

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> In both cases, you have a sequence of commands that does one thing with
> one git version, and something else with another git version. The only
> difference is whether your sequence includes git init.

I vaguely remember arguing against different behaviour between a new
repository and an existing one in the past on a different topic myself.  I
am not married to the idea of effectively flipping the default to "refuse"
in a new repository early, and do not mind dropping the "git init" change
at all.  I do not like the inconsistency myself.

The only reason why I did that "git init" patch was because I just thought
that it might be a good way to help new people sooner, who will start
using git after 1.6.2 gets released but before 1.7.0 flips the default for
everybody, while explaining people older than 1.6.2 what is happening in
the warning/error message during the transition period.  I suspect it
could be argued that with an extra line that says "the default will change
to 'refuse' in 1.7.0 for all repositories, but we are making the change
early for newly created repositories to help new people", the main idea of
the patch may be salvageable, but I do not deeply care either way.

By the way, I just realized one thing.

When we flip the default to "refuse" in 1.7.0 for everybody, we will need
the explanation and instruction on how to get a non-default behaviour and
how to squelch the message when we "refuse by defaut", just like my first
patch did when we "warn by default".  It is entirely possible some people
simply skip 1.6.2 and directly jump to 1.7.0, and while we cannot help
them avoid the surprise caused by the change in behaviour, we cannot be
silent in such a situation.

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Junio C Hamano]

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

> By the way, I just realized one thing.
>
> When we flip the default to "refuse" in 1.7.0 for everybody, we will need
> the explanation and instruction on how to get a non-default behaviour and
> how to squelch the message when we "refuse by defaut", just like my first
> patch did when we "warn by default".  It is entirely possible some people
> simply skip 1.6.2 and directly jump to 1.7.0, and while we cannot help
> them avoid the surprise caused by the change in behaviour, we cannot be
> silent in such a situation.

And this is meant for 1.7.0, and is queued at the tip of 'pu' just for the
heck of it.

It will flip the default to "refuse", but explains why the operation that
used to succeed suddenly started failing, how to get the older behaviour
back (but with caveats), and how to squelch this message.

Adjustments to the tests were resurrected from the previous "init-db"
patch.  Since this is meant for 1.7.0 that does flip the default for
everybody, there is no need to touch init-db anymore.

-- >8 --
[PATCH] Refuse updating the current branch in a non-bare repository via push

This makes git-push to refuse pushing-push into a non-bare repository to
update its current branch by default.  To help people who are used to
be able to do this (and later "reset --hard" it in some other way),
a big error message is issued when this refusal is triggered.

Hosting sites that do not give the users direct access to customize their
repositories (e.g. repo.or.cz, gitorious, github etc.) may further want to
explicitly set the configuration variable to "refuse" for their customers'
repositories.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-receive-pack.c      |   40 +++++++++++++++++-----------------------
 t/t5400-send-pack.sh        |    2 ++
 t/t5401-update-hooks.sh     |    1 +
 t/t5405-send-pack-rewind.sh |    1 +
 t/t5516-fetch-push.sh       |    1 +
 t/t5517-push-mirror.sh      |    3 ++-
 t/t5521-pull-symlink.sh     |   20 +++++++++++++-------
 t/t5701-clone-local.sh      |    4 +++-
 8 files changed, 40 insertions(+), 32 deletions(-)

diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index 6f61c45..17ab4f5 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -215,33 +215,27 @@ static int is_ref_checked_out(const char *ref)
 	return !strcmp(head, ref);
 }
 
-static char *warn_unconfigured_deny_msg[] = {
-	"Updating the currently checked out branch may cause confusion,",
-	"as the index and work tree do not reflect changes that are in HEAD.",
-	"As a result, you may see the changes you just pushed into it",
-	"reverted when you run 'git diff' over there, and you may want",
-	"to run 'git reset --hard' before starting to work to recover.",
+static char *refuse_unconfigured_deny_msg[] = {
+	"By default, updating the current branch in a non-bare repository",
+	"is denied, because it will make the index and work tree inconsistent",
+	"with what you pushed, and will require 'git reset --hard' to match",
+	"the work tree to HEAD.",
 	"",
 	"You can set 'receive.denyCurrentBranch' configuration variable to",
-	"'refuse' in the remote repository to forbid pushing into its",
-	"current branch."
+	"'ignore' or 'warn' in the remote repository to allow pushing into",
+	"its current branch; however, this is not recommended unless you",
+	"arranged to update its work tree to match what you pushed in some",
+	"other way.",
 	"",
-	"To allow pushing into the current branch, you can set it to 'ignore';",
-	"but this is not recommended unless you arranged to update its work",
-	"tree to match what you pushed in some other way.",
-	"",
-	"To squelch this message, you can set it to 'warn'.",
-	"",
-	"Note that the default will change in a future version of git",
-	"to refuse updating the current branch unless you have the",
-	"configuration variable set to either 'ignore' or 'warn'."
+	"To squelch this message, you can set the configuration variable to",
+	"'refuse'."
 };
 
-static void warn_unconfigured_deny(void)
+static void refuse_unconfigured_deny(void)
 {
 	int i;
-	for (i = 0; i < ARRAY_SIZE(warn_unconfigured_deny_msg); i++)
-		warning(warn_unconfigured_deny_msg[i]);
+	for (i = 0; i < ARRAY_SIZE(refuse_unconfigured_deny_msg); i++)
+		error(refuse_unconfigured_deny_msg[i]);
 }
 
 static const char *update(struct command *cmd)
@@ -261,14 +255,14 @@ static const char *update(struct command *cmd)
 		switch (deny_current_branch) {
 		case DENY_IGNORE:
 			break;
-		case DENY_UNCONFIGURED:
 		case DENY_WARN:
 			warning("updating the current branch");
-			if (deny_current_branch == DENY_UNCONFIGURED)
-				warn_unconfigured_deny();
 			break;
 		case DENY_REFUSE:
+		case DENY_UNCONFIGURED:
 			error("refusing to update checked out branch: %s", name);
+			if (deny_current_branch == DENY_UNCONFIGURED)
+				refuse_unconfigured_deny();
 			return "branch is currently checked out";
 		}
 	}
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index b21317d..5c9c277 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -33,6 +33,7 @@ test_expect_success setup '
 	git update-ref HEAD "$commit" &&
 	git clone ./. victim &&
 	cd victim &&
+	git config receive.denyCurrentBranch warn &&
 	git log &&
 	cd .. &&
 	git update-ref HEAD "$zero" &&
@@ -138,6 +139,7 @@ rewound_push_setup() {
 	rm -rf parent child &&
 	mkdir parent && cd parent &&
 	git init && echo one >file && git add file && git commit -m one &&
+	git config receive.denyCurrentBranch warn &&
 	echo two >file && git commit -a -m two &&
 	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..325714e 100755
--- a/t/t5401-update-hooks.sh
+++ b/t/t5401-update-hooks.sh
@@ -18,6 +18,7 @@ test_expect_success setup '
 	git update-ref refs/heads/master $commit0 &&
 	git update-ref refs/heads/tofail $commit1 &&
 	git clone ./. victim &&
+	GIT_DIR=victim/.git git config receive.denyCurrentBranch warn &&
 	GIT_DIR=victim/.git git update-ref refs/heads/tofail $commit1 &&
 	git update-ref refs/heads/master $commit1 &&
 	git update-ref refs/heads/tofail $commit0
diff --git a/t/t5405-send-pack-rewind.sh b/t/t5405-send-pack-rewind.sh
index cb9aacc..4bda18a 100755
--- a/t/t5405-send-pack-rewind.sh
+++ b/t/t5405-send-pack-rewind.sh
@@ -8,6 +8,7 @@ test_expect_success setup '
 
 	>file1 && git add file1 && test_tick &&
 	git commit -m Initial &&
+	git config receive.denyCurrentBranch warn &&
 
 	mkdir another && (
 		cd another &&
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 89649e7..a67ebd0 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -12,6 +12,7 @@ mk_empty () {
 	(
 		cd testrepo &&
 		git init &&
+		git config receive.denyCurrentBranch warn &&
 		mv .git/hooks .git/hooks-disabled
 	)
 }
diff --git a/t/t5517-push-mirror.sh b/t/t5517-push-mirror.sh
index ea49ded..e2ad260 100755
--- a/t/t5517-push-mirror.sh
+++ b/t/t5517-push-mirror.sh
@@ -19,7 +19,8 @@ mk_repo_pair () {
 	mkdir mirror &&
 	(
 		cd mirror &&
-		git init
+		git init &&
+		git config receive.denyCurrentBranch warn
 	) &&
 	mkdir master &&
 	(
diff --git a/t/t5521-pull-symlink.sh b/t/t5521-pull-symlink.sh
index 5672b51..66b5ac1 100755
--- a/t/t5521-pull-symlink.sh
+++ b/t/t5521-pull-symlink.sh
@@ -14,13 +14,19 @@ test_description='pulling from symlinked subdir'
 #
 # The working directory is subdir-link.
 
-mkdir subdir
-echo file >subdir/file
-git add subdir/file
-git commit -q -m file
-git clone -q . clone-repo
-ln -s clone-repo/subdir/ subdir-link
-
+test_expect_success setup '
+	mkdir subdir &&
+	echo file >subdir/file &&
+	git add subdir/file &&
+	git commit -q -m file &&
+	git clone -q . clone-repo &&
+	ln -s clone-repo/subdir/ subdir-link &&
+	(
+		cd clone-repo &&
+		git config receive.denyCurrentBranch warn
+	) &&
+	git config receive.denyCurrentBranch warn
+'
 
 # Demonstrate that things work if we just avoid the symlink
 #
diff --git a/t/t5701-clone-local.sh b/t/t5701-clone-local.sh
index 3559d17..10accc2 100755
--- a/t/t5701-clone-local.sh
+++ b/t/t5701-clone-local.sh
@@ -119,7 +119,9 @@ test_expect_success 'bundle clone with nonexistent HEAD' '
 test_expect_success 'clone empty repository' '
 	cd "$D" &&
 	mkdir empty &&
-	(cd empty && git init) &&
+	(cd empty &&
+	 git init &&
+	 git config receive.denyCurrentBranch warn) &&
 	git clone empty empty-clone &&
 	test_tick &&
 	(cd empty-clone
-- 
1.6.1.2.346.g115a3

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Jeff King]

On Tue, Feb 03, 2009 at 09:45:54AM -0800, Junio C Hamano wrote:

> And this is meant for 1.7.0, and is queued at the tip of 'pu' just for the
> heck of it.

Looks sane to me for 1.7.0, but it will probably require tweaking of
which tests are fixed at that time (and actually, I think some can just
be cleaned up to use bare repositories instead of adding the config --
but let's deal with that when the patch is ready to be applied).

> This makes git-push to refuse pushing-push into a non-bare repository to

As opposed to non-pushing push?

-Peff

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> This makes git-push to refuse pushing-push into a non-bare repository to
>
> As opposed to non-pushing push?

Editor/finger slippage.  Thanks for catching it.

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I think this is a definite improvement over the current behavior. As I
> said before, I am not sure what is the right path (though I think I am
> leaning towards leaving the warning longer based on the recent
> discussion), but if we are to leave the default to warn and not refuse,
> I think this should definitely be applied.
>
> A few comments on the specific message:

Thanks.

The commit was only in 'pu', so I'll be amending it instead of applying an
incremental, but here is the interdiff to incorporate your comments.

 builtin-receive-pack.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git c/builtin-receive-pack.c w/builtin-receive-pack.c
index f2c94fc..09c07d9 100644
--- c/builtin-receive-pack.c
+++ w/builtin-receive-pack.c
@@ -217,17 +217,18 @@ static int is_ref_checked_out(const char *ref)
 
 static char *warn_unconfigured_deny_msg[] = {
 	"Updating the currently checked out branch may cause confusion,",
-	"as the index and work tree do not reflect changes that are in HEAD."
+	"as the index and work tree do not reflect changes that are in HEAD.",
 	"As a result, you may see the changes you just pushed into it",
 	"reverted when you run 'git diff' over there, and you may want",
 	"to run 'git reset --hard' before starting to work to recover.",
 	"",
 	"You can set 'receive.denyCurrentBranch' configuration variable to",
-	"'refuse' in the repository to forbid pushing into the current branch",
-	"of it."
+	"'refuse' in the remote repository to forbid pushing into the",
+	"current branch of it."
 	"",
 	"To allow pushing into the current branch, you can set it to 'ignore';",
-	"but this is not recommended unless you really know what you are doing.",
+	"but this is not recommended unless you arranged its work tree to get",
+	"updated to match what you pushed in some other way.",
 	"",
 	"To squelch this message, you can set it to 'warn'.",
 	"",

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Jeff King]

On Tue, Feb 03, 2009 at 12:01:43AM -0800, Junio C Hamano wrote:

>  	"To allow pushing into the current branch, you can set it to 'ignore';",
> -	"but this is not recommended unless you really know what you are doing.",
> +	"but this is not recommended unless you arranged its work tree to get",
> +	"updated to match what you pushed in some other way.",

This is much better, but I believe it needs to be "...arranged _for_
its work tree to get updated..." to be grammatically correct.

And as a nit (which I seem to be full of tonight), you can get rid of
the passive voice by saying:

 but this is not recommended unless you arranged to update its work
 tree to match what you pushed in some other way.

which is slightly more clear, IMHO.

-Peff

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Feb 03, 2009 at 12:01:43AM -0800, Junio C Hamano wrote:
>
>>  	"To allow pushing into the current branch, you can set it to 'ignore';",
>> -	"but this is not recommended unless you really know what you are doing.",
>> +	"but this is not recommended unless you arranged its work tree to get",
>> +	"updated to match what you pushed in some other way.",
>
> This is much better, but I believe it needs to be "...arranged _for_
> its work tree to get updated..." to be grammatically correct.
>
> And as a nit (which I seem to be full of tonight), you can get rid of
> the passive voice by saying:
>
>  but this is not recommended unless you arranged to update its work
>  tree to match what you pushed in some other way.
>
> which is slightly more clear, IMHO.

Much more clear.  I overuse the passive voice, I know it is a bad habit I
somehow cannot shake off.

Thanks.

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Junio C Hamano]

Nanako Shiraishi <nanako3@lavabit.com> writes:

> I assume that everybody, including the maintainer who is too heavyweight
> and has too much inertia to accept too sudden a change of the course,
> wants to eventually make the default to deny pushing to the current
> branch. But I think such a change should come at 1.7.0 release at the
> earliest, and a constructive thing to do is to put in a patch to 1.6.2
> that helps the users with the eventual transition.

I am not opposed to eventually change the default to refuse at some point,
but I have to say that now would not be the best time to do so.  Jeff's
986e823 (receive-pack: detect push to current branch of non-bare repo,
2008-11-08) that is v1.6.1-rc1~59^2 was the one we started warning about
this, and we only had one major release since then, and I'd love to see a
solid rc or even the final release by mid February.

By the way, I do not appreciate other people who I have never met
speculate about my body mass very much.  I am on the skinner end of the
spectrum, if you need to know.

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Sam Vilain]

On Sat, 2009-01-31 at 18:06 -0800, Junio C Hamano wrote:
> Nanako Shiraishi <nanako3@lavabit.com> writes:
> 
> > I assume that everybody, including the maintainer who is too heavyweight
> > and has too much inertia to accept too sudden a change of the course,
> > wants to eventually make the default to deny pushing to the current
> > branch. But I think such a change should come at 1.7.0 release at the
> > earliest, and a constructive thing to do is to put in a patch to 1.6.2
> > that helps the users with the eventual transition.
> 
> I am not opposed to eventually change the default to refuse at some point,
> but I have to say that now would not be the best time to do so.  Jeff's
> 986e823 (receive-pack: detect push to current branch of non-bare repo,
> 2008-11-08) that is v1.6.1-rc1~59^2 was the one we started warning about
> this, and we only had one major release since then, and I'd love to see a
> solid rc or even the final release by mid February.

Personally I think it's worth fast tracking, because I think very few
people are actually using push to a checked out branch whereas many
people are confused by the behaviour.  I just can't understand the
resistance to this safety feature.  People who encounter the bug can
just change the setting and move on... it seems like an argument based
on "principles", usually a sign that one has run out of actual
arguments..

> By the way, I do not appreciate other people who I have never met
> speculate about my body mass very much.  I am on the skinner end of the
> spectrum, if you need to know.

lol.  It was a metaphorical use of the term from my reading ;-)

Sam.

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Junio C Hamano]

Sam Vilain <sam@vilain.net> writes:

> I just can't understand the
> resistance to this safety feature.  People who encounter the bug can
> just change the setting and move on... it seems like an argument based
> on "principles", usually a sign that one has run out of actual
> arguments..

There is no resitance to any safety feature.  The resistance is to
something entirely different.

The change we all share as the desired end result is to introduce a
different behaviour, even if it is a better one, that will deliberately
break people's working setup.

The end result being better does not justify breaking people's setup.  You
need to help people whose setups will be broken prepare for such a change.

Perhaps you already forgot the fiasco after 1.6.0, which moved tons of
git-foo scripts out of the users' way.  It resulted in a better layout in
the end, but we knew it would break people's working setup from the
beginning.

It was a change that many people argued for, saying that too many commands
in the path scared new people, that we thought we planned very carefully,
and that we thought we gave ample warning to existing users.  Yet it
resulted in a huge fallout.

You probably forgot about it already, but that is because you weren't the
one who had to take the flak.  I was, and I haven't forgotten.

The resistance is to an irresponsible transition strategy that causes pain
unnecessarily.  We need to improve the situation incrementally, helping
new people a bit in one step while not hurting old people along the way,
aiming to help everybody more in the end.

One of the concluding comments after the 1.6.0 fiasco was that the
old-timers _heard about_ the upcoming change, but were too busy to stop
and think to realize that it is any urgent that they need to prepare for
it, and we should have warned them more actively.  In the end, they didn't
mind the change itself per-se because we had an escape hatch (i.e. to
prepend the output from "git --exec-path" to the PATH in your script), but
the primary pain was having to adjust their setup on _our_ timetable, not
theirs.

Even k.org, which is one of the early adopters of new releases among the
larger sites (with larger proportion of old timers), has started using the
version with the "updating the branch you have checked out" warning (which
happened in v1.6.1) fairly recently, and during the discussion we noticed
that the warning didn't say we will be switching the default to "refuse"
any time soon.  We haven't yet given them enough advance warning telling
them they will have to go running around flipping the bit in their
repositories.  Dismissing the issue by saying "old-timers can simply flip
the configuration once" makes an irresponsible argument for repeating the
same mistake of 1.6.0.  That is what I am resisting to.

I think the plan outlined in this thread would ease the transition in much
better way, and the patch I sent yesterday uses a deliberately loooooooong
warning message whose primary purpose is to be annoyingly obvious that the
users need to adjust their configuration now, so that the eventual change
in the default we will make won't inconvenience them.

I was reluctant to change the default for new repositories to refuse
during 1.6.2 timeframe, but I think with the attached patch on top of the
second patch I sent out earlier, it would force people choose before they
do any real damange to their repositories, and having it early would make
the overall transition plan smoother.

 builtin-init-db.c      |    2 +-
 builtin-receive-pack.c |   26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 1 deletions(-)

diff --git c/builtin-init-db.c w/builtin-init-db.c
index 26c10cc..ea2765c 100644
--- c/builtin-init-db.c
+++ w/builtin-init-db.c
@@ -251,7 +251,7 @@ static int create_default_files(const char *template_path)
 			git_config_set("core.worktree", work_tree);
 		}
 		if (!reinit)
-			git_config_set("receive.denyCurrentBranch", "refuse");
+			git_config_set("receive.denyCurrentBranch", "refuse-with-insn");
 	}
 
 	if (!reinit) {
diff --git c/builtin-receive-pack.c w/builtin-receive-pack.c
index f2c94fc..82d372f 100644
--- c/builtin-receive-pack.c
+++ w/builtin-receive-pack.c
@@ -16,6 +16,7 @@ enum deny_action {
 	DENY_IGNORE,
 	DENY_WARN,
 	DENY_REFUSE,
+	DENY_REFUSE_WITH_INSN,
 };
 
 static int deny_deletes = 0;
@@ -39,6 +40,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, "refuse-with-insn"))
+			return DENY_REFUSE_WITH_INSN;
 	}
 	if (git_config_bool(var, value))
 		return DENY_REFUSE;
@@ -236,6 +239,20 @@ static char *warn_unconfigured_deny_msg[] = {
 	"configuration variable set to either 'ignore' or 'warn'."
 };
 
+static char *refuse_current_insn_msg[] = {
+	"By default, updating the current branch in a non-bare repository",
+	"is denied, because it will make the index and work tree inconsistent",
+	"with what you pushed, and will require 'git reset --hard' to match",
+	"the work tree to HEAD.",
+	"",
+	"You can set 'receive.denyCurrentBranch' configuration variable to",
+	"'ignore' or 'warn' in the repository to allow pushing into the",
+	"current branch of it; this is not recommended unless you really know",
+	"what you are doing.",
+	"",
+	"To squelch this message, you can set it to 'refuse'.",
+};
+
 static void warn_unconfigured_deny(void)
 {
 	int i;
@@ -243,6 +260,12 @@ static void warn_unconfigured_deny(void)
 		warning(warn_unconfigured_deny_msg[i]);
 }
 
+static void refuse_current_insn(void)
+{
+	int i;
+	for (i = 0; i < ARRAY_SIZE(refuse_current_insn_msg); i++)
+		error(refuse_current_insn_msg[i]);
+}
 
 static const char *update(struct command *cmd)
 {
@@ -268,7 +291,10 @@ static const char *update(struct command *cmd)
 				warn_unconfigured_deny();
 			break;
 		case DENY_REFUSE:
+		case DENY_REFUSE_WITH_INSN:
 			error("refusing to update checked out branch: %s", name);
+			if (deny_current_branch == DENY_REFUSE_WITH_INSN)
+				refuse_current_insn();
 			return "branch is currently checked out";
 		}
 	}

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Sam Vilain]

On Sun, 2009-02-01 at 13:33 -0800, Junio C Hamano wrote:
> > I just can't understand the
> > resistance to this safety feature.  People who encounter the bug can
> > just change the setting and move on... it seems like an argument based
> > on "principles", usually a sign that one has run out of actual
> > arguments..
> 
> There is no resitance to any safety feature.  The resistance is to
> something entirely different.
  [...]
> Perhaps you already forgot the fiasco after 1.6.0, which moved tons of
> git-foo scripts out of the users' way.  It resulted in a better layout in
> the end, but we knew it would break people's working setup from the
> beginning.
  [...]

Yeah sure but the changes are a bit different aren't they.  One affected
all users who used the previously documented way to access subcommands
(and the names that the man pages all still retain).  The other affects
a small number of users who are doing something which is labeled in many
places as a bad thing to want to do.

That being said, I think I like the copy and design of the patch you
just posted.  If the path of caution is to be followed for this, then
the way you propose seems a good way to do it.

Sam

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Junio C Hamano]

Sam Vilain <sam@vilain.net> writes:

> The other affects
> a small number of users who are doing something which is labeled in many
> places as a bad thing to want to do.

Sorry, but I do not agree with this.

What is bad is to push into a repository that is used for editing.  That
is labelled as a bad thing to want to do.

It is often the easiest to push and then run "reset --hard" (perhaps from
the post-update script) to propagate your change to a repository that is
not usually used for editing.  E.g. that has always been the way I update
my private repository at k.org that I use for final testing before pushing
out the results that I built and tested on my personal machine.  People
who have live web pages served from a checkout do that, too.  It is not a
bad thing to do at all, and you can find many instructions with google
without spending a lot of time to do exactly that.

    http://kerneltrap.org/mailarchive/git/2008/7/1/2315924
    http://utsl.gen.nz/git/post-update
    http://groups.google.com/group/sl-ugr/browse_thread/thread/04e4c4bd6ce174af

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Sam Vilain]

On Mon, 2009-02-02 at 00:32 -0800, Junio C Hamano wrote:
> > The other affects
> > a small number of users who are doing something which is labeled in many
> > places as a bad thing to want to do.
> 
> Sorry, but I do not agree with this.
> 
> What is bad is to push into a repository that is used for editing.  That
> is labelled as a bad thing to want to do.
> 
> It is often the easiest to push and then run "reset --hard" (perhaps from
> the post-update script) to propagate your change to a repository that is
> not usually used for editing.  E.g. that has always been the way I update
> my private repository at k.org that I use for final testing before pushing
> out the results that I built and tested on my personal machine.  People
> who have live web pages served from a checkout do that, too.  It is not a
> bad thing to do at all, and you can find many instructions with google
> without spending a lot of time to do exactly that.
> 
>     http://kerneltrap.org/mailarchive/git/2008/7/1/2315924
>     http://utsl.gen.nz/git/post-update

Heh, thanks for referring me to my own script ;-)

I think a "repository that is used for editing" can be practically
defined as one which does not have any dirty local files.  Or, if there
are dirty local files then they are none of the files which would be
changed by the push, or none of them would be changed by the push.
Similar to the check that 'git merge' does.

With that definition, if receive.denyCurrentBranch is set to "update" it
could be pretty much automagic, perhaps even good enough behaviour to
consider making it the default.  This kind of behaviour is what my
post-update hook tries to achieve, but it really needs a corresponding
piece in the update hook, and I didn't code all of the conditions above
into it.

Sam.

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Johannes Schindelin]

Hi,

On Sat, 31 Jan 2009, Nanako Shiraishi wrote:

> Quoting Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> 
> > You cannot just cater for one workflow and fsck the other workflows 
> > over.
> >
> > You'll have to devise a method that helps the workflow you are 
> > interested in, but leaves the others alone.
> 
> I think you'd want to repeat that to yourself when you propose to switch 
> the default for denyCurrentcurrentBranch config to "true" too hastily 
> the next time?

Nanako, what exactly do you think I did before writing these lines:

    Granted, we wanted to have a longer grace period for old-timers, but
    let's face it:
    [... a discussion on the pros and cons ...]

?  Do you think I did that just on a whim, or do you rather assume that I 
thought long and hard about it?

> I don't think your patch matches the tradition of how defaults are 
> changed in git project. You don't introduce a large change just after 
> the maintainer hints about going into a freeze for 1.X.Y release when Y 
> isn't zero.

Indeed.  That is why I wrote "Granted, we wanted to have a longer grace 
period"!

> I assume that everybody, including the maintainer who is too heavyweight

I saw Junio.  He is in no way heavyweight.  He is actually rather skinny.

> and has too much inertia to accept too sudden a change of the course,
> wants to eventually make the default to deny pushing to the current
> branch. But I think such a change should come at 1.7.0 release at the
> earliest, and a constructive thing to do is to put in a patch to 1.6.2
> that helps the users with the eventual transition.

So what do you want to achieve?  Annoy me?  Annoy Git newbies?  Annoy Git 
oldtimers?

Eventually, it will boil down to

- who
- when

to annoy.

And I have a strong suspicion that it does not help the reputation of Git 
at all, if we annoy

- new Git users
- for a long time

Rather, I'd like to annoy only

- a few oldtimers who should know better by now
- just once, when they upgrade to a new minor release and see that they 
  forgot to mark their repository as "bare".

If you would think about it as long and hard as I did, you would see that 
we have to annoy

- a few oldtimers
- at some stage

anyway, but in the meantime, we could avoid to annoy

- a lot of new Git users
- for a long time

at the cost of annoying

- a few oldtimers
- now, instead of later

which cost will come to

- us
- anyway

Frankly, I am surprised that people do not agree with me on this point.

> What do people think?

Seriously, when it comes to the Git users I interact with, they think 
"what the bl**dy fsck did the Git people smoke when they made it _so_ hard 
on new Git users, I am certainly not the only person bitten by 
this."

I know, because they let me in on their thoughts, but are too shy to 
mention them here on the Git list.

And as everybody knows, I am a nice guy, and I listen.

Ciao,
Dscho

Re: [PATCH] Switch receive.denyCurrentBranch to "refuse"

[Junio C Hamano]

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

> at the cost of annoying
>
> - a few oldtimers
> - now, instead of later

Nobody seems to have realized this, but suddenly changing the default to
refuse without giving people enough advance warning to adjust will hurt
not just the old-timers (a rough definition is people who are from the
kernel circle and have been using git since summer of 2005), but people
who picked up a recipe from various how-to web pages to push to a live
repository and updating the checkout that is otherwise never touched by
the humans with its post-update hook running "reset --hard".  Old timers
may be savvy enough to know what has changed and may be able to grudgingly
react, but what is your plans for these recipe following kids?

How many times do I have to repeat that it is much worse to break a
working setup of people without advance warning and sound transition
guidance than having a known breakage that users can be trained to avoid?

And realize that I am not saying we need to keep the known breakage
forever.

The only thing I am saying is that you need to have a smooth transition
plan for changing the default, and a mechanism to guide people in place.

I'll ignore you if you keep repeating "all it takes is for old timers to
flip a switch".  Such an argument shows that you didn't learn a thing
after the 1.6.0 fallout.