[PATCH 1/2] Introduce receive.guardCurrentBranch

[PATCH 1/2] Introduce receive.guardCurrentBranch

[Johannes Schindelin]

Setting this config variable to "true" makes git-receive-pack refuse
to update whatever happens to be the current branch.

This option can be used to avoid havoc in a non-bare repository into
which somebody pushes.

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

	I thought about this a while (see also
	http://thread.gmane.org/gmane.comp.version-control.git/66490),
	and I think it is time to push for this change.

	On the one hand, of course, it is nice to have so many users
	that not all of them know the Git source intimately.  On the
	other hand, we will have to introduce many more safeguards
	like this now.  Sigh...

	Maybe this is even 1.5.5 material.  I'm undecided.

 Documentation/config.txt           |    5 +++++
 Documentation/git-receive-pack.txt |    3 +++
 receive-pack.c                     |   18 ++++++++++++++++++
 t/t5400-send-pack.sh               |   13 +++++++++++++
 4 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5df8ea9..efde54d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1006,6 +1006,11 @@ receive.denyNonFastForwards::
 	even if that push is forced. This configuration variable is
 	set when initializing a shared repository.
 
+receive.guardCurrentBranch::
+	If set to true, git-receive-pack will deny to update the ref that
+	HEAD points to, if HEAD is not detached.  This configuration
+	variable is set when initializing a non-bare repository.
+
 transfer.unpackLimit::
 	When `fetch.unpackLimit` or `receive.unpackLimit` are
 	not set, the value of this variable is used instead.
diff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt
index 4111434..0c82af9 100644
--- a/Documentation/git-receive-pack.txt
+++ b/Documentation/git-receive-pack.txt
@@ -32,6 +32,9 @@ git-receive-pack honours the receive.denyNonFastForwards config
 option, which tells it if updates to a ref should be denied if they
 are not fast-forwards.
 
+git-receive-pack honors the receive.guardCurrentBranch config options,
+which tells it if it is okay to update the branch HEAD points to.
+
 OPTIONS
 -------
 <directory>::
diff --git a/receive-pack.c b/receive-pack.c
index 828d490..6423c7c 100644
--- a/receive-pack.c
+++ b/receive-pack.c
@@ -15,6 +15,7 @@ static int receive_unpack_limit = -1;
 static int transfer_unpack_limit = -1;
 static int unpack_limit = 100;
 static int report_status;
+static char *guard_current_branch;
 
 static char capabilities[] = " report-status delete-refs ";
 static int capabilities_sent;
@@ -41,6 +42,19 @@ static int receive_pack_config(const char *var, const char *value)
 		return 0;
 	}
 
+	if (strcmp(var, "receive.guardhead") == 0) {
+		guard_current_branch = NULL;
+		if (git_config_bool(var, value)) {
+			unsigned char sha1[20];
+			int flag;
+			const char *head = resolve_ref("HEAD", sha1, 0, &flag);
+			if (flag & REF_ISSYMREF)
+				guard_current_branch = xstrdup(head);
+		}
+
+		return 0;
+	}
+
 	return git_default_config(var, value);
 }
 
@@ -183,6 +197,10 @@ static const char *update(struct command *cmd)
 		      "but I can't find it!", sha1_to_hex(new_sha1));
 		return "bad pack";
 	}
+	if (guard_current_branch && !strcmp(name, guard_current_branch)) {
+		error("refusing to update current branch: '%s'", name);
+		return "current branch";
+	}
 	if (deny_non_fast_forwards && !is_null_sha1(new_sha1) &&
 	    !is_null_sha1(old_sha1) &&
 	    !prefixcmp(name, "refs/heads/")) {
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index 2b6b6e3..af8d5a3 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -171,4 +171,17 @@ test_expect_success \
 	rewound_push_succeeded
 '
 
+test_expect_success 'receive.guardCurrentBranch' '
+
+	rewound_push_setup &&
+	(cd ../parent &&
+	 git config receive.guardCurrentBranch true) &&
+	test_must_fail git-send-pack ../parent/.git refs/heads/master &&
+	rewound_push_failed &&
+	(cd ../parent &&
+	 git config receive.guardCurrentBranch false) &&
+	git-send-pack ../parent/.git +refs/heads/*:refs/heads/* &&
+	rewound_push_succeeded
+'
+
 test_done
-- 
1.5.5.rc1.174.g591a9

[PATCH 2/2] git-init: set receive.guardCurrentBranch = true for non-bare repositories

[Johannes Schindelin]

When initializing a non-bare repositories, you most likely want to avoid
pushing into the current branch (because a push does not touch the
working tree at all).

Let's be nice to those users who have not read that part of the
manual, and just outright refuse updating the current branch.

To be nice to those users who wrote that part of the manual, let's do
that in git-init, so that existing repositories are not affected.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/git-init.txt |    4 ++++
 builtin-init-db.c          |    1 +
 2 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-init.txt b/Documentation/git-init.txt
index 62914da..03a4f0e 100644
--- a/Documentation/git-init.txt
+++ b/Documentation/git-init.txt
@@ -56,6 +56,10 @@ By default, the configuration flag receive.denyNonFastForwards is enabled
 in shared repositories, so that you cannot force a non fast-forwarding push
 into it.
 
+If you are initializing a non-bare repository, the config variable
+`receive.guardCurrentBranch` is set to true.  This avoids problems with
+pushing into the current branch, which does not touch the working tree.
+
 --
 
 
diff --git a/builtin-init-db.c b/builtin-init-db.c
index 4f93d03..4542054 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -243,6 +243,7 @@ static int create_default_files(const char *template_path)
 	else {
 		const char *work_tree = get_git_work_tree();
 		git_config_set("core.bare", "false");
+		git_config_set("receive.guardCurrentBranch", "true");
 		/* allow template config file to override the default */
 		if (log_all_ref_updates == -1)
 		    git_config_set("core.logallrefupdates", "true");
-- 
1.5.5.rc1.174.g591a9

Re: [PATCH 2/2] git-init: set receive.guardCurrentBranch = true for non-bare repositories

[Junio C Hamano]

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

> When initializing a non-bare repositories, you most likely want to avoid
> pushing into the current branch (because a push does not touch the
> working tree at all).

As I described in my other message, I suspect that treating the current
branch specially like this is a wrong approach.  The configuration might
be a good idea, but shouldn't it prevent any local branch from getting
updated?  Push into non-bare repository is simply a fetch run in reverse
direction.

Re: [PATCH 2/2] git-init: set receive.guardCurrentBranch = true for non-bare repositories

[Johannes Schindelin]

Hi,

On Sun, 23 Mar 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > When initializing a non-bare repositories, you most likely want to 
> > avoid pushing into the current branch (because a push does not touch 
> > the working tree at all).
> 
> As I described in my other message, I suspect that treating the current 
> branch specially like this is a wrong approach.  The configuration might 
> be a good idea, but shouldn't it prevent any local branch from getting 
> updated?  Push into non-bare repository is simply a fetch run in reverse 
> direction.

That would break this work flow:

	# machine A
	$ git push B master:refs/heads/tmp

	# machine B
	$ git merge tmp
	$ git branch -d tmp

Besides, there is a _vital_ difference between the current branch, and any 
other local branch: pushing into other local branches cannot make your 
working tree stale.

Ciao,
Dscho

Re: [PATCH 2/2] git-init: set receive.guardCurrentBranch = true for non-bare repositories

[Junio C Hamano]

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

> On Sun, 23 Mar 2008, Junio C Hamano wrote:
> ...
>> As I described in my other message, I suspect that treating the current 
>> branch specially like this is a wrong approach.  The configuration might 
>> be a good idea, but shouldn't it prevent any local branch from getting 
>> updated?  Push into non-bare repository is simply a fetch run in reverse 
>> direction.
>
> That would break this work flow:
>
> 	# machine A
> 	$ git push B master:refs/heads/tmp
>
> 	# machine B
> 	$ git merge tmp
> 	$ git branch -d tmp

I am afraid that the above is irrelevant.

(1) You can push the temporary into anywhere outside refs/heads/ if that
    becomes the problem;

(2) Your change to forbid current branch already "breaks" another workflow
    (which I happen to use everyday) anyway:

	# machine A (primary development repository)
        $ git push k.org ;# master->master, next->next, ...

        # machine B (build test repository)
        $ git reset --hard
        $ for b in master maint next pu
          do git checkout $b && make clean test || break
	  done

(3) Because the proposed new feature is dependent on a configuration, both
    of the above can be "worked around" equally easily by disabling it.

> Besides, there is a _vital_ difference between the current branch,...

Actually that is exactly I used to think until I realized how flawed that
line of reasoning was.

Suppose you started treating the current branch any specially.  A few
issues immediately arise:

 * "I can update any other branches, but git rejected my push saying it
   does not want to update the current branch. why?"

 * "how do I know what branch is checked out currently over there to work
   around this limitation?"

 * "more importantly, how would I fix this?  do I ask the repository owner
   to check out different branch?"

The process of coming up with answers to these questions made me realize
that treating the current branch specially was just a hack to work around
only immediately observable effects of pushing into a live repository, and
not solving the real issue.  IOW, any of the answers I came up with
sounded like lame excuses.

Step back a bit and think _why_ you wanted to prevent current branch tip
from getting updated in the first place.  There are two issues:

 * Why is it _current_ branch, and not _these branches_, that can be
   configured by the user to be protected from a push from sideways?

 * Why is it undesirable for the work tree and the index to go out of sync
   with respect to the branch tip to begin with?

The latter is simpler to answer, so let's deal with it first.  The reason
why it is bad is because allowing a push to the current branch interferes
with the work actively being done in the repository, using the work tree
contents.  There is a person, you, who is actively editing the work tree
in order to advance the tip of the branch by making commits.  If the
branch tip moves without your knowing, that destabilizes your working
environment.  Your work tree wanted to make a new commit on top of some
known state, but that state was moved underneath you.  Not good.

When you are using the repository for real work (i.e. advance the tips of
its branches), you want a stable environment.  You do not want its HEAD
bobbing around outside your control, and silently detaching to cause your
later commits to go to unnamed branch without your knowing is just as bad
(which you already correctly objected to).

Now think.  What if one of these operations you do in the repository to
advance the tip was to merge from one of _your_ local branches?  Yes, you
end up merging something you did not expect to merge if you allowed a push
from sideways to affect that local branch, only because the branch
happened to be un-checked-out and you implemented this protection to
forbid only to current branch.  Allowing a push from sideways to any local
branch destabilizes your work environment, not just the current one.

The former question becomes easier to answer once you realize this.  It
was only because you were too narrowly focused on the immediately visible
effect of pushing into a live repository, and that is apparent in your
"there is a _vital_ difference" statement (again, I used to think the same
way, so it is not just you).  The difference is not _vital_ at all.  It
merely is being immediately observable.  The stability of the environment
you do real work in is more than just the branch currently checked out.
At least you would want "I do not want these refs to silently change
without me knowing".

Pushing to non-bare repository can happen in number of ways, and there are
a few _useful_ use cases to push into local branches of a live repository,
but the sane usage that allows such push has one common precondition: No
interesting edit is ever outstanding in the non-bare repository when push
happens.

For example, you could have a repository and its checkout of html branch
of git.git, and serve the former via dumb protocol for git-fetch clients,
while the latter is fed directly to web browsers.  Even if you push into
its html branch (that is checked out), you would not suffer, as _you
already know what you are doing_ and that is the reason you can even
arrange its post-update hook to run "reset --hard" for you.

The earlier "build test repository" example is the same way.  I might have
to debug problems that only manifest on the k.org machine, so it is unlike
I am forbidden from doing any "interesting edits" there, but when I do a
push from the primary machine, I know I am done with any edit on the k.org
repository (otherwise I'd still be logged in and working there, not
pushing from home), so I am sure "reset --hard" is safe and good.

These use cases share the precondition "no interesting edit is ever
outstanding".  But it is just an extension of a more general idea: Usually
nobody works in the work tree to advance its branches with commits made
there.  Instead, advancement of its branches always happen by push from
elsewhere.

If a real person works in the work tree, the story is different.  There
needs a boundary that defines his stable working environment that should
not be disrupted by a push from sideways.  You _could_ define it as "the
currently checked out branch, the index and the work tree".  I think it is
too narrow.  We could allow the user to define "this subset of local
branches", but I think it is one too many knob to tweak, and a reasonable
boundary is to define the boundary to include "local branches" into that
set, as they are also the ones you would update with the regular working
inside that work tree (i.e. checkout-hack-commit cycle).

In such a live repository, treating "git push" initiated from elsewhere
into it exactly the same way "git fetch" initiated in the repository in
the reverse direction would be the cleanest way to explain a non-confusing
workflow to the end users.  You are always in control of all of your local
branches, and you decide when to merge the tip of such a pushed-in ref
(this is the mothership-satellite configuration suggested in the FAQ entry
mentioned in the other thread).

I am not married to the idea of using refs/remotes/* for that purpose, and
it might turn out to be a good idea to have more elaborate setup, for
example, using refs/pushed/<branch-name>/ hierarchy to keep other's pushed
refs in there.  But one thing I am reasonably sure is that pushes should
not go to refs/heads/ directly whether the branch is checked out or not,
if the user of that pushed-into live repository wants to keep his sanity.

Re: [PATCH 2/2] git-init: set receive.guardCurrentBranch = true for non-bare repositories

[Johannes Schindelin]

Hi,

On Mon, 24 Mar 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Sun, 23 Mar 2008, Junio C Hamano wrote:
> > ...
> >> As I described in my other message, I suspect that treating the 
> >> current branch specially like this is a wrong approach.  The 
> >> configuration might be a good idea, but shouldn't it prevent any 
> >> local branch from getting updated?  Push into non-bare repository is 
> >> simply a fetch run in reverse direction.
> >
> > That would break this work flow:
> >
> > 	# machine A
> > 	$ git push B master:refs/heads/tmp
> >
> > 	# machine B
> > 	$ git merge tmp
> > 	$ git branch -d tmp
> 
> I am afraid that the above is irrelevant.
> 
> (1) You can push the temporary into anywhere outside refs/heads/ if that
>     becomes the problem;

Okay, didn't think about that one.

> (2) Your change to forbid current branch already "breaks" another workflow
>     (which I happen to use everyday) anyway:
> 
> 	# machine A (primary development repository)
>         $ git push k.org ;# master->master, next->next, ...
> 
>         # machine B (build test repository)
>         $ git reset --hard
>         $ for b in master maint next pu
>           do git checkout $b && make clean test || break
> 	  done

It would not, because it is not activated by default.  Only newly 
initialised repositories will have that config variable set.

> > Besides, there is a _vital_ difference between the current branch,...
> 
> [...]
>
> Now think.  What if one of these operations you do in the repository to 
> advance the tip was to merge from one of _your_ local branches?  Yes, 
> you end up merging something you did not expect to merge if you allowed 
> a push from sideways to affect that local branch, only because the 
> branch happened to be un-checked-out and you implemented this protection 
> to forbid only to current branch.  Allowing a push from sideways to any 
> local branch destabilizes your work environment, not just the current 
> one.

Okay, I am starting to see the light.

How about

	receive.localBranches = (refuse | allow)

with a default "allow"?  Then we could add more rope later with the 
"update" option, which would run "git read-tree -u -m HEAD" if the current 
branch is updated, and simply allow all other branches being updated.

Hmm?

Ciao,
Dscho

Re: [PATCH 2/2] git-init: set receive.guardCurrentBranch = true for non-bare repositories

[Daniel Barkalow]

On Mon, 24 Mar 2008, Johannes Schindelin wrote:

> Hi,
> 
> On Mon, 24 Mar 2008, Junio C Hamano wrote:
> 
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > 
> > > On Sun, 23 Mar 2008, Junio C Hamano wrote:
> > > ...
> > >> As I described in my other message, I suspect that treating the 
> > >> current branch specially like this is a wrong approach.  The 
> > >> configuration might be a good idea, but shouldn't it prevent any 
> > >> local branch from getting updated?  Push into non-bare repository is 
> > >> simply a fetch run in reverse direction.
> > >
> > > That would break this work flow:
> > >
> > > 	# machine A
> > > 	$ git push B master:refs/heads/tmp
> > >
> > > 	# machine B
> > > 	$ git merge tmp
> > > 	$ git branch -d tmp
> > 
> > I am afraid that the above is irrelevant.
> > 
> > (1) You can push the temporary into anywhere outside refs/heads/ if that
> >     becomes the problem;
> 
> Okay, didn't think about that one.
> 
> > (2) Your change to forbid current branch already "breaks" another workflow
> >     (which I happen to use everyday) anyway:
> > 
> > 	# machine A (primary development repository)
> >         $ git push k.org ;# master->master, next->next, ...
> > 
> >         # machine B (build test repository)
> >         $ git reset --hard
> >         $ for b in master maint next pu
> >           do git checkout $b && make clean test || break
> > 	  done
> 
> It would not, because it is not activated by default.  Only newly 
> initialised repositories will have that config variable set.
> 
> > > Besides, there is a _vital_ difference between the current branch,...
> > 
> > [...]
> >
> > Now think.  What if one of these operations you do in the repository to 
> > advance the tip was to merge from one of _your_ local branches?  Yes, 
> > you end up merging something you did not expect to merge if you allowed 
> > a push from sideways to affect that local branch, only because the 
> > branch happened to be un-checked-out and you implemented this protection 
> > to forbid only to current branch.  Allowing a push from sideways to any 
> > local branch destabilizes your work environment, not just the current 
> > one.
> 
> Okay, I am starting to see the light.
> 
> How about
> 
> 	receive.localBranches = (refuse | allow)
> 
> with a default "allow"?  Then we could add more rope later with the 
> "update" option, which would run "git read-tree -u -m HEAD" if the current 
> branch is updated, and simply allow all other branches being updated.

The use cases I've seen for pushing into a non-bare repository seem to be 
cases in which "refs/heads/" isn't really local; it's only updated by push 
from elsewhere, and it's named "refs/heads/" because that's where public 
branches are served from. This suggests we could have:

	core.noLocalBranches: true

with the implications:

 - it's definitely okay to push to refs/heads/
 - HEAD is always detached.

That seems to me to accurately describe a repository used to hold branches 
for public consumption and where there's a work tree for testing and 
building, rather than development. (I.e., work tree operations only read 
the repository.)

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH 2/2] git-init: set receive.guardCurrentBranch = true for non-bare repositories

[Johannes Schindelin]

Hi,

On Mon, 24 Mar 2008, Daniel Barkalow wrote:

> On Mon, 24 Mar 2008, Johannes Schindelin wrote:
> 
> > How about
> > 
> > 	receive.localBranches = (refuse | allow)
> > 
> > with a default "allow"?  Then we could add more rope later with the 
> > "update" option, which would run "git read-tree -u -m HEAD" if the 
> > current branch is updated, and simply allow all other branches being 
> > updated.
> 
> The use cases I've seen for pushing into a non-bare repository seem to 
> be cases in which "refs/heads/" isn't really local; it's only updated by 
> push from elsewhere, and it's named "refs/heads/" because that's where 
> public branches are served from. This suggests we could have:
> 
> 	core.noLocalBranches: true
> 
> with the implications:
> 
>  - it's definitely okay to push to refs/heads/
>  - HEAD is always detached.
> 
> That seems to me to accurately describe a repository used to hold 
> branches for public consumption and where there's a work tree for 
> testing and building, rather than development. (I.e., work tree 
> operations only read the repository.)

Funny.  I thought bare repositories were meant for public consumption.  
And for testing, you still can have a (non-public) repository with a 
work-tree.

I'd rather not try to tell users that it is okay to mix the two.

Ciao,
Dscho

Re: [PATCH 1/2] Introduce receive.guardCurrentBranch

[Miklos Vajna]

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

On Sun, Mar 23, 2008 at 09:43:43PM +0100, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> +receive.guardCurrentBranch::
> +	If set to true, git-receive-pack will deny to update the ref that
> +	HEAD points to, if HEAD is not detached.  This configuration
> +	variable is set when initializing a non-bare repository.

probably i want too much, but it would be really nice if receive-pack
could just make HEAD detached. so that the user later can do a 'git
checkout -m master' to resolve possible conflicts.

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

Re: [PATCH 1/2] Introduce receive.guardCurrentBranch

[Johannes Schindelin]

Hi,

On Mon, 24 Mar 2008, Miklos Vajna wrote:

> On Sun, Mar 23, 2008 at 09:43:43PM +0100, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > +receive.guardCurrentBranch::
> > +	If set to true, git-receive-pack will deny to update the ref that
> > +	HEAD points to, if HEAD is not detached.  This configuration
> > +	variable is set when initializing a non-bare repository.
> 
> probably i want too much, but it would be really nice if receive-pack 
> could just make HEAD detached. so that the user later can do a 'git 
> checkout -m master' to resolve possible conflicts.

As I wrote earlier, you can do that with a hook.

I do not want any official support for this, since I think it is an 
absolutely horrible idea.  I think I said that earlier, too.

Ciao,
Dscho

Re: [PATCH 1/2] Introduce receive.guardCurrentBranch

[Miklos Vajna]

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

On Mon, Mar 24, 2008 at 12:01:33PM +0100, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> As I wrote earlier, you can do that with a hook.
> 
> I do not want any official support for this, since I think it is an 
> absolutely horrible idea.  I think I said that earlier, too.

hm that makes sense. thanks for mentioning that it's possible via hooks,
in the "i know what i'm doing" case :)

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