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

Nanako Shiraishi [off-list ref] 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.

On Sat, 2009-01-31 at 18:06 -0800, Junio C Hamano wrote:

Nanako Shiraishi [off-list ref] writes:

quoted

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.

Sam Vilain [off-list ref] 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";
 		}
 	}

On Sun, 2009-02-01 at 13:33 -0800, Junio C Hamano wrote:

quoted

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