Deleting the "current" branch in remote bare repositories

Deleting the "current" branch in remote bare repositories

[Jan Krüger]

Hi everyone,

recently on IRC we had a case where someone had accidentally deleted
the "current" branch (i.e. thing pointed to by HEAD) by using "git push
origin :master". This broke the remote HEAD as well as the local
refs/remotes/origin/HEAD. Not good. I think we want to make it harder
to get into this situation.

Personally, without being aware of any potential counterindications, I
think the best solution from a usability point of view would
be to have receive-pack reject deletions of what's currently in HEAD.
The question is, of course: how do we go about situations where someone
actually wants to delete the branch HEAD points at?

1. reject deletion and point out a command to change HEAD first (I
don't think we've got a command to do this remotely; do we want one?)

2. automatically change HEAD to something else if there's any other
branch (eww)

3. accept the deletion but warn the user that she just broke the
repository (especially eww because it also breaks the local tracking
ref)

Any smart ideas?
-Jan

Re: Deleting the "current" branch in remote bare repositories

[Felipe Contreras]

On Sat, Feb 7, 2009 at 5:27 PM, Jan Krüger <jk@jk.gs> wrote:
> Hi everyone,
>
> recently on IRC we had a case where someone had accidentally deleted
> the "current" branch (i.e. thing pointed to by HEAD) by using "git push
> origin :master". This broke the remote HEAD as well as the local
> refs/remotes/origin/HEAD. Not good. I think we want to make it harder
> to get into this situation.
>
> Personally, without being aware of any potential counterindications, I
> think the best solution from a usability point of view would
> be to have receive-pack reject deletions of what's currently in HEAD.
> The question is, of course: how do we go about situations where someone
> actually wants to delete the branch HEAD points at?
>
> 1. reject deletion and point out a command to change HEAD first (I
> don't think we've got a command to do this remotely; do we want one?)
>
> 2. automatically change HEAD to something else if there's any other
> branch (eww)
>
> 3. accept the deletion but warn the user that she just broke the
> repository (especially eww because it also breaks the local tracking
> ref)
>
> Any smart ideas?

This was brought up before:
http://marc.info/?l=git&m=123254293910829&w=2

But I don't think it reached any conclusion.

-- 
Felipe Contreras

Re: Deleting the "current" branch in remote bare repositories

[Jan Krüger]

On Sun, 8 Feb 2009 00:05:05 +0200, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> This was brought up before:
> http://marc.info/?l=git&m=123254293910829&w=2
> 
> But I don't think it reached any conclusion.

Okay, somehow I missed that. To reiterate the things from that
discussion that I think are most reasonable:

1) a local broken symref should generally be ignored unless we actually
   need the symref.

2) there should be a more convenient (porcelain) way to change a
   refs/remotes/foo/HEAD symref, e.g. git remote set-default, possibly
   with an option to re-sync from the remote head (we could even make
   that an option for git remote update).

Regarding 2): if we managed to add an option to that to change the
remote HEAD, we could disallow deleting a remote branch that HEAD
points to, and refer to this command. I think the problem is that we
would have to add symref updating logic for all types of remote
protocols.

If people agree with these ideas I think I'll write up a couple of
patches to implement these changes. So, any protests?

-Jan

Re: Deleting the "current" branch in remote bare repositories

[Jeff King]

On Sun, Feb 08, 2009 at 01:18:02AM +0100, Jan Krüger wrote:

> Okay, somehow I missed that. To reiterate the things from that
> discussion that I think are most reasonable:
> 
> 1) a local broken symref should generally be ignored unless we actually
>    need the symref.

I think this is almost as easy as:

diff --git a/refs.c b/refs.c
index 024211d..9601101 100644
--- a/refs.c
+++ b/refs.c
@@ -276,7 +276,6 @@ static struct ref_list *get_ref_dir(const char *base, struct ref_list *list)
 				continue;
 			}
 			if (!resolve_ref(ref, sha1, 1, &flag)) {
-				error("%s points nowhere!", ref);
 				continue;
 			}
 			list = add_ref(ref, sha1, flag, list, NULL);

Since this is just called when enumerating all of the loose refs (via
get_loose_refs(), which is generally called from for_each_ref).

However, there is one other complication. rename_ref uses get_loose_refs
to check whether the destination space is available:

  if (!is_refname_available(newref, oldref, get_loose_refs(), 0))
        return 1;

so you can get funny behavior through:

  git branch -m foo bar

when "bar" is a symref pointing to a non-existent ref. Of course, we are
not _changing_ that behavior, since we always just ignored that symref.
But we are removing the warning message that might clue the user that
something confusing is about to happen.

> 2) there should be a more convenient (porcelain) way to change a
>    refs/remotes/foo/HEAD symref, e.g. git remote set-default, possibly
>    with an option to re-sync from the remote head (we could even make
>    that an option for git remote update).

Yes, I think that is a good idea (optionally with a switch to just
re-grab the information from the remote).

> Regarding 2): if we managed to add an option to that to change the
> remote HEAD, we could disallow deleting a remote branch that HEAD
> points to, and refer to this command. I think the problem is that we
> would have to add symref updating logic for all types of remote
> protocols.

Yes, the protocol support would make this a much bigger patch (and you
would have to handle the case where the remote side didn't support it).
But bear in mind that deleting the remote HEAD breaks things not just
for you, but for other people who are cloning that remote. Maybe we
should refuse such updates unless "-f" is given (similar to
non-fast-forward updates); I haven't looked to see if we even have the
remote's HEAD information during push, though.

> If people agree with these ideas I think I'll write up a couple of
> patches to implement these changes. So, any protests?

I say go for it.

-Peff

Re: Deleting the "current" branch in remote bare repositories

[Junio C Hamano]

"Jan Krüger" <jk@jk.gs> writes:

> 1) a local broken symref should generally be ignored unless we actually
>    need the symref.
>
> 2) there should be a more convenient (porcelain) way to change a
>    refs/remotes/foo/HEAD symref, e.g. git remote set-default, possibly
>    with an option to re-sync from the remote head (we could even make
>    that an option for git remote update).

It would be good to sugarcoat symbolic-ref with "git remote set-something",
although I am not sure "default" is a good term for it (it feels more
like "primary" to me).

> Regarding 2): if we managed to add an option to that to change the
> remote HEAD, we could disallow deleting a remote branch that HEAD
> points to, and refer to this command.

With (2), you would have the ability to pick the branch you are the most
interested in among the branches that particular remote 'foo' offers,
which is what remotes/foo/HEAD is about.  But the latter part of your
sentence is talking about disallowing the removal of a branch in the
remote 'foo'.  How would (2) help you if what you want is to delete one
particular branch at the remote 'foo'?  I do not think these two are
related at all.

Rather, if you reject deletion of the current branch at the remote, you
would not get into the situation where your remote/foo/HEAD points at
nonexisting tracking branch by your own "deleting the branch by pushing a
void to the remote", and you would reduce the need for (2).

I said "reduce" because you can get into the same situation by other
means.  For example, somebody else can remove the branch your
remotes/foo/HEAD points at.  Or the repository owner of the remote can say
"my HEAD is not 'master' anymore, it is 'next'" and delete 'master' from
there.  In either case, your next "git remote prune foo" will get you into
that situation, and you would need (2) to recover (or use symbolic-ref).

Forbidding the deletion of the current branch at remote and (2) do not
have anything to do with each other.

I think forbidding the removal of the current branch falls into the same
category as forbidding the updating of the current branch.  It is what you
would want to avoid in many workflows, and receive.denyDeleteCurrent that
defaults to refuse may eventually be a good way to do this, but we need a
transition plan for that escape hatch.  Most people may not see why they
would want to do such a thing, and many people may perceive that in *their
own* workflow such an operation can only be a mistake, but that is not a
good enough reason to suddenly start forbidding something other people
were allowed to do so far.

The "refer to this command" you mention can and should be issued when the
user actually uses "remotes/foo" (or "remotes/foo/HEAD"), expecting it to
resolve to the branch HEAD used to point at but now missing.  The current
errors you see in your issue (1) were *meant to* notify you of the
situation as soon as it occurs (i.e. it gives pre-emptive warning, even
before you actually refer to "remotes/foo/HEAD"), expecting that you know
what to do (namely, repoint HEAD with symbolic-ref, or remove HEAD), so
theoretically you could also issue the "refer to this command" there as
well.

But I agree the current error messages are a bit too loud, and it would be
better to squelch them and only give the instruction when the user refers
to "remotes/foo" or "remotes/foo/HEAD".

Re: Deleting the "current" branch in remote bare repositories

[Jeff King]

On Sun, Feb 08, 2009 at 01:42:07AM -0800, Junio C Hamano wrote:

> I think forbidding the removal of the current branch falls into the same
> category as forbidding the updating of the current branch.  It is what you
> would want to avoid in many workflows, and receive.denyDeleteCurrent that
> defaults to refuse may eventually be a good way to do this, but we need a
> transition plan for that escape hatch.  Most people may not see why they
> would want to do such a thing, and many people may perceive that in *their
> own* workflow such an operation can only be a mistake, but that is not a
> good enough reason to suddenly start forbidding something other people
> were allowed to do so far.

I thought of that, too, but note that receive.denyDeleteCurrent is about
preventing mistakes in a _non-bare_ repo. But deleting the HEAD branch
is also annoying in a bare repo (but not _as_ annoying; the real impact
is that cloners get a "could not checkout" message, but you don't have
the weird index-and-HEAD don't sync issue that non-bare repos get).
Should such a safety valve apply to both?

-Peff

Re: Deleting the "current" branch in remote bare repositories

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, Feb 08, 2009 at 01:42:07AM -0800, Junio C Hamano wrote:
>
>> I think forbidding the removal of the current branch falls into the same
>> category as forbidding the updating of the current branch.  It is what you
>> would want to avoid in many workflows, and receive.denyDeleteCurrent that
>> defaults to refuse may eventually be a good way to do this, but we need a
>> transition plan for that escape hatch.  Most people may not see why they
>> would want to do such a thing, and many people may perceive that in *their
>> own* workflow such an operation can only be a mistake, but that is not a
>> good enough reason to suddenly start forbidding something other people
>> were allowed to do so far.
>
> I thought of that, too, but note that receive.denyDeleteCurrent is about
> preventing mistakes in a _non-bare_ repo. But deleting the HEAD branch
> is also annoying in a bare repo (but not _as_ annoying; the real impact
> is that cloners get a "could not checkout" message, but you don't have
> the weird index-and-HEAD don't sync issue that non-bare repos get).
> Should such a safety valve apply to both?

If you remove the current branch from a repository, the impact that
operation causes the remote users of the repository would be the same
whether the repository has or does not have a work tree.  And in that
sense, I think it should apply equally.  The criteria is not "is it bare",
but "is it used by people remotely".  IOW, you refuse deletion of the
current branch if other people may fetch from it.

In addition, removing the current branch will affect local operations in
the repository --- the person who were working in the work tree to prepare
for the _next_ commit will now end up creating a root commit.  The effect
is similar, as you correctly point out, to the issue of the HEAD and the
work tree going out of sync that ends up creating a commit with a lot of
reverts.  They both cause the user on the work tree to create an undesired
commit.  Here, the criteria is "does this have a work tree".  I think the
current receive.denyCurrentBranch should also trigger in this case.

[PATCH 0/6] Deleting the "current" branch in a remote repository

[Junio C Hamano]

The first two are preparatory clean-up and bugfix patches.

The third one introduces receive.denyDeleteCurrent configuration that
defaults to "annoyingly loud warning", which we will flip to "refusal with
insn" at the end.

The fourth and fifth one are not about push and are more or less
independent.  They deal with what happens when you ended up with a
dangling symbolic ref in a tracking hierarchy.  I think a check
and warning similar to the fourth one may be needed in git-push (and
git-send-pack) when it pushes a void to remove a branch from a remote, and
in turn removes the corresponding tracking branch at the local end.

Then finally the last one flips the default for receive.denyDeleteCurrent
to refuse.

I think the first five ought to be in 1.6.2-rc1 but I lack the energy and
time to finish the testing, re-eyeballing and documentation tonight.  It
would be very nice to see friends from other timezones to help me with
these tasks ;-)


Junio C Hamano (6):
  builtin-receive-pack.c: do not initialize statics to 0
  t5400: allow individual tests to fail
  receive-pack: receive.denyDeleteCurrent
  remote prune: warn dangling symrefs
  Warn use of "origin" when remotes/origin/HEAD is dangling
  receive-pack: default receive.denyDeleteCurrent to refuse

 builtin-receive-pack.c |   70 ++++++++++++++++++++++++++++++++++++++--------
 builtin-remote.c       |    6 ++++
 refs.c                 |   72 ++++++++++++++++++++++++++++++++++++------------
 refs.h                 |    5 +++
 sha1_name.c            |    6 ++-
 t/t5400-send-pack.sh   |   44 ++++++++++++++++++++--------
 6 files changed, 157 insertions(+), 46 deletions(-)

[PATCH 1/6] builtin-receive-pack.c: do not initialize statics to 0

[Junio C Hamano]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-receive-pack.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index 6de186c..b2cb4ba 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -18,8 +18,8 @@ enum deny_action {
 	DENY_REFUSE,
 };
 
-static int deny_deletes = 0;
-static int deny_non_fast_forwards = 0;
+static int deny_deletes;
+static int deny_non_fast_forwards;
 static enum deny_action deny_current_branch = DENY_UNCONFIGURED;
 static int receive_fsck_objects;
 static int receive_unpack_limit = -1;
-- 
1.6.2.rc0.28.g2593d

[PATCH 2/6] t5400: allow individual tests to fail

[Junio C Hamano]

Each test chdir'ed around and ended up in a random place if any of the
test in the sequence failed but the entire test script was allowed to
run.  This wrapps each in a subshell as necessary.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5400-send-pack.sh |   37 ++++++++++++++++++++++++-------------
 1 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index b21317d..013aced 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -10,6 +10,7 @@ test_description='See why rewinding head breaks send-pack
 
 cnt=64
 test_expect_success setup '
+    (
 	test_tick &&
 	mkdir mozart mozart/is &&
 	echo "Commit #0" >mozart/is/pink &&
@@ -51,7 +52,9 @@ test_expect_success setup '
 	done &&
 	git update-ref HEAD "$commit" &&
 	echo Rebase &&
-	git log'
+	git log
+    )
+'
 
 test_expect_success 'pack the source repository' '
 	git repack -a -d &&
@@ -59,10 +62,12 @@ test_expect_success 'pack the source repository' '
 '
 
 test_expect_success 'pack the destination repository' '
+    (
 	cd victim &&
 	git repack -a -d &&
 	git prune &&
 	cd ..
+    )
 '
 
 test_expect_success \
@@ -89,49 +94,53 @@ test_expect_success \
 	cmp victim/.git/refs/heads/master .git/refs/heads/master
 '
 
-test_expect_success \
-        'push can be used to delete a ref' '
+test_expect_success 'push can be used to delete a ref' '
+    (
 	cd victim &&
 	git branch extra master &&
 	cd .. &&
 	test -f victim/.git/refs/heads/extra &&
 	git send-pack ./victim/.git/ :extra master &&
 	! test -f victim/.git/refs/heads/extra
+    )
 '
 
 unset GIT_CONFIG
 HOME=`pwd`/no-such-directory
 export HOME ;# this way we force the victim/.git/config to be used.
 
-test_expect_success \
-	'pushing a delete should be denied with denyDeletes' '
+test_expect_success 'pushing a delete should be denied with denyDeletes' '
+    (
 	cd victim &&
 	git config receive.denyDeletes true &&
 	git branch extra master &&
 	cd .. &&
 	test -f victim/.git/refs/heads/extra &&
 	test_must_fail git send-pack ./victim/.git/ :extra master
+    )
 '
 rm -f victim/.git/refs/heads/extra
 
-test_expect_success \
-        'pushing with --force should be denied with denyNonFastforwards' '
+test_expect_success 'pushing with --force should be denied with denyNonFastforwards' '
+    (
 	cd victim &&
 	git config receive.denyNonFastforwards true &&
 	cd .. &&
 	git update-ref refs/heads/master master^ || return 1
 	git send-pack --force ./victim/.git/ master && return 1
 	! test_cmp .git/refs/heads/master victim/.git/refs/heads/master
+    )
 '
 
-test_expect_success \
-	'pushing does not include non-head refs' '
+test_expect_success 'pushing does not include non-head refs' '
+    (
 	mkdir parent && cd parent &&
 	git init && touch file && git add file && git commit -m add &&
 	cd .. &&
 	git clone parent child && cd child && git push --all &&
 	cd ../parent &&
 	git branch -a >branches && ! grep origin/master branches
+    )
 '
 
 rewound_push_setup() {
@@ -156,8 +165,8 @@ rewound_push_failed() {
 	fi
 }
 
-test_expect_success \
-	'pushing explicit refspecs respects forcing' '
+test_expect_success 'pushing explicit refspecs respects forcing' '
+    (
 	rewound_push_setup &&
 	if git send-pack ../parent/.git refs/heads/master:refs/heads/master
 	then
@@ -167,10 +176,11 @@ test_expect_success \
 	fi && rewound_push_failed &&
 	git send-pack ../parent/.git +refs/heads/master:refs/heads/master &&
 	rewound_push_succeeded
+    )
 '
 
-test_expect_success \
-	'pushing wildcard refspecs respects forcing' '
+test_expect_success 'pushing wildcard refspecs respects forcing' '
+    (
 	rewound_push_setup &&
 	if git send-pack ../parent/.git refs/heads/*:refs/heads/*
 	then
@@ -180,6 +190,7 @@ test_expect_success \
 	fi && rewound_push_failed &&
 	git send-pack ../parent/.git +refs/heads/*:refs/heads/* &&
 	rewound_push_succeeded
+    )
 '
 
 test_done
-- 
1.6.2.rc0.28.g2593d

[PATCH 3/6] receive-pack: receive.denyDeleteCurrent

[Junio C Hamano]

This is a companion patch to the recent 3d95d92 (receive-pack: explain
what to do when push updates the current branch, 2009-01-31).

Deleting the current branch from a remote will result in the next clone
from it not check out anything, among other things.  It also is one of the
cause that makes remotes/origin/HEAD a dangling symbolic ref.  This patch
still allows the traditional behaviour but with a big warning, and promises
that the default will change to 'refuse' in a future release.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-receive-pack.c |   72 ++++++++++++++++++++++++++++++++++++++++-------
 t/t5400-send-pack.sh   |    7 ++++
 2 files changed, 68 insertions(+), 11 deletions(-)

diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index b2cb4ba..f7e04c4 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -21,11 +21,13 @@ enum deny_action {
 static int deny_deletes;
 static int deny_non_fast_forwards;
 static enum deny_action deny_current_branch = DENY_UNCONFIGURED;
+static enum deny_action deny_delete_current = DENY_UNCONFIGURED;
 static int receive_fsck_objects;
 static int receive_unpack_limit = -1;
 static int transfer_unpack_limit = -1;
 static int unpack_limit = 100;
 static int report_status;
+static const char *head_name;
 
 static char capabilities[] = " report-status delete-refs ";
 static int capabilities_sent;
@@ -77,6 +79,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (strcmp(var, "receive.denydeletecurrent") == 0) {
+		deny_delete_current = parse_deny_action(var, value);
+		return 0;
+	}
+
 	return git_default_config(var, value, cb);
 }
 
@@ -203,16 +210,12 @@ static int run_update_hook(struct command *cmd)
 
 static int is_ref_checked_out(const char *ref)
 {
-	unsigned char sha1[20];
-	const char *head;
-
 	if (is_bare_repository())
 		return 0;
 
-	head = resolve_ref("HEAD", sha1, 0, NULL);
-	if (!head)
+	if (!head_name)
 		return 0;
-	return !strcmp(head, ref);
+	return !strcmp(head_name, ref);
 }
 
 static char *warn_unconfigured_deny_msg[] = {
@@ -244,6 +247,32 @@ static void warn_unconfigured_deny(void)
 		warning(warn_unconfigured_deny_msg[i]);
 }
 
+static char *warn_unconfigured_deny_delete_current_msg[] = {
+	"Deleting the current branch can cause confusion by making the next",
+	"'git clone' not check out any file.",
+	"",
+	"You can set 'receive.denyDeleteCurrent' configuration variable to",
+	"'refuse' in the remote repository to disallow deleting the current",
+	"branch.",
+	"",
+	"You can set it to 'ignore' to allow such a delete without a warning.",
+	"",
+	"To make this warning message less loud, you can set it to 'warn'.",
+	"",
+	"Note that the default will change in a future version of git",
+	"to refuse deleting the current branch unless you have the",
+	"configuration variable set to either 'ignore' or 'warn'."
+};
+
+static void warn_unconfigured_deny_delete_current(void)
+{
+	int i;
+	for (i = 0;
+	     i < ARRAY_SIZE(warn_unconfigured_deny_delete_current_msg);
+	     i++)
+		warning(warn_unconfigured_deny_delete_current_msg[i]);
+}
+
 static const char *update(struct command *cmd)
 {
 	const char *name = cmd->ref_name;
@@ -278,12 +307,30 @@ static const char *update(struct command *cmd)
 		      "but I can't find it!", sha1_to_hex(new_sha1));
 		return "bad pack";
 	}
-	if (deny_deletes && is_null_sha1(new_sha1) &&
-	    !is_null_sha1(old_sha1) &&
-	    !prefixcmp(name, "refs/heads/")) {
-		error("denying ref deletion for %s", name);
-		return "deletion prohibited";
+
+	if (!is_null_sha1(old_sha1) && is_null_sha1(new_sha1)) {
+		if (deny_deletes && !prefixcmp(name, "refs/heads/")) {
+			error("denying ref deletion for %s", name);
+			return "deletion prohibited";
+		}
+
+		if (!strcmp(name, head_name)) {
+			switch (deny_delete_current) {
+			case DENY_IGNORE:
+				break;
+			case DENY_WARN:
+			case DENY_UNCONFIGURED:
+				if (deny_delete_current == DENY_UNCONFIGURED)
+					warn_unconfigured_deny_delete_current();
+				warning("deleting the current branch");
+				break;
+			case DENY_REFUSE:
+				error("refusing to delete the current branch: %s", name);
+				return "deletion of the current branch prohibited";
+			}
+		}
 	}
+
 	if (deny_non_fast_forwards && !is_null_sha1(new_sha1) &&
 	    !is_null_sha1(old_sha1) &&
 	    !prefixcmp(name, "refs/heads/")) {
@@ -377,6 +424,7 @@ static void run_update_post_hook(struct command *cmd)
 static void execute_commands(const char *unpacker_error)
 {
 	struct command *cmd = commands;
+	unsigned char sha1[20];
 
 	if (unpacker_error) {
 		while (cmd) {
@@ -394,6 +442,8 @@ static void execute_commands(const char *unpacker_error)
 		return;
 	}
 
+	head_name = resolve_ref("HEAD", sha1, 0, NULL);
+
 	while (cmd) {
 		cmd->error_string = update(cmd);
 		cmd = cmd->next;
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index 013aced..7b21f5f 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -193,4 +193,11 @@ test_expect_success 'pushing wildcard refspecs respects forcing' '
     )
 '
 
+test_expect_success 'deny pushing to delete current branch' '
+    (
+	rewound_push_setup &&
+	git send-pack ../parent/.git :refs/heads/master 2>errs
+    )
+'
+
 test_done
-- 
1.6.2.rc0.28.g2593d

[PATCH 4/6] remote prune: warn dangling symrefs

[Junio C Hamano]

If you prune from the remote "frotz" that deleted the ref your tracking
branch remotes/frotz/HEAD points at, the symbolic ref will become
dangling.  We used to detect this as an error condition and issued a
message every time refs are enumerated.

This stops the error message, but moves the warning to "remote prune".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-remote.c |    6 ++++
 refs.c           |   72 ++++++++++++++++++++++++++++++++++++++++-------------
 refs.h           |    5 +++
 3 files changed, 65 insertions(+), 18 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index db18bcf..ac69d37 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -756,12 +756,17 @@ static int prune(int argc, const char **argv)
 		OPT_END()
 	};
 	struct ref_states states;
+	const char *dangling_msg;
 
 	argc = parse_options(argc, argv, options, builtin_remote_usage, 0);
 
 	if (argc < 1)
 		usage_with_options(builtin_remote_usage, options);
 
+	dangling_msg = (dry_run
+			? " %s will become dangling!\n"
+			: " %s has become dangling!\n");
+
 	memset(&states, 0, sizeof(states));
 	for (; argc; argc--, argv++) {
 		int i;
@@ -784,6 +789,7 @@ static int prune(int argc, const char **argv)
 
 			printf(" * [%s] %s\n", dry_run ? "would prune" : "pruned",
 			       abbrev_ref(refname, "refs/remotes/"));
+			warn_dangling_symref(dangling_msg, refname);
 		}
 
 		/* NEEDSWORK: free remote */
diff --git a/refs.c b/refs.c
index 024211d..6eb5f53 100644
--- a/refs.c
+++ b/refs.c
@@ -275,10 +275,8 @@ static struct ref_list *get_ref_dir(const char *base, struct ref_list *list)
 				list = get_ref_dir(ref, list);
 				continue;
 			}
-			if (!resolve_ref(ref, sha1, 1, &flag)) {
-				error("%s points nowhere!", ref);
-				continue;
-			}
+			if (!resolve_ref(ref, sha1, 1, &flag))
+				hashclr(sha1);
 			list = add_ref(ref, sha1, flag, list, NULL);
 		}
 		free(ref);
@@ -287,6 +285,35 @@ static struct ref_list *get_ref_dir(const char *base, struct ref_list *list)
 	return sort_ref_list(list);
 }
 
+struct warn_if_dangling_data {
+	const char *refname;
+	const char *msg_fmt;
+};
+
+static int warn_if_dangling_symref(const char *refname, const unsigned char *sha1,
+				   int flags, void *cb_data)
+{
+	struct warn_if_dangling_data *d = cb_data;
+	const char *resolves_to;
+	unsigned char junk[20];
+
+	if (!(flags & REF_ISSYMREF))
+		return 0;
+
+	resolves_to = resolve_ref(refname, junk, 0, NULL);
+	if (!resolves_to || strcmp(resolves_to, d->refname))
+		return 0;
+
+	printf(d->msg_fmt, refname);
+	return 0;
+}
+
+void warn_dangling_symref(const char *msg_fmt, const char *refname)
+{
+	struct warn_if_dangling_data data = { refname, msg_fmt };
+	for_each_rawref(warn_if_dangling_symref, &data);
+}
+
 static struct ref_list *get_loose_refs(void)
 {
 	if (!cached_refs.did_loose) {
@@ -498,16 +525,19 @@ int read_ref(const char *ref, unsigned char *sha1)
 	return -1;
 }
 
+#define DO_FOR_EACH_INCLUDE_BROKEN 01
 static int do_one_ref(const char *base, each_ref_fn fn, int trim,
-		      void *cb_data, struct ref_list *entry)
+		      int flags, void *cb_data, struct ref_list *entry)
 {
 	if (strncmp(base, entry->name, trim))
 		return 0;
-	if (is_null_sha1(entry->sha1))
-		return 0;
-	if (!has_sha1_file(entry->sha1)) {
-		error("%s does not point to a valid object!", entry->name);
-		return 0;
+	if (!(flags & DO_FOR_EACH_INCLUDE_BROKEN)) {
+		if (is_null_sha1(entry->sha1))
+			return 0;
+		if (!has_sha1_file(entry->sha1)) {
+			error("%s does not point to a valid object!", entry->name);
+			return 0;
+		}
 	}
 	current_ref = entry;
 	return fn(entry->name + trim, entry->sha1, entry->flag, cb_data);
@@ -561,7 +591,7 @@ fallback:
 }
 
 static int do_for_each_ref(const char *base, each_ref_fn fn, int trim,
-			   void *cb_data)
+			   int flags, void *cb_data)
 {
 	int retval = 0;
 	struct ref_list *packed = get_packed_refs();
@@ -570,7 +600,7 @@ static int do_for_each_ref(const char *base, each_ref_fn fn, int trim,
 	struct ref_list *extra;
 
 	for (extra = extra_refs; extra; extra = extra->next)
-		retval = do_one_ref(base, fn, trim, cb_data, extra);
+		retval = do_one_ref(base, fn, trim, flags, cb_data, extra);
 
 	while (packed && loose) {
 		struct ref_list *entry;
@@ -586,13 +616,13 @@ static int do_for_each_ref(const char *base, each_ref_fn fn, int trim,
 			entry = packed;
 			packed = packed->next;
 		}
-		retval = do_one_ref(base, fn, trim, cb_data, entry);
+		retval = do_one_ref(base, fn, trim, flags, cb_data, entry);
 		if (retval)
 			goto end_each;
 	}
 
 	for (packed = packed ? packed : loose; packed; packed = packed->next) {
-		retval = do_one_ref(base, fn, trim, cb_data, packed);
+		retval = do_one_ref(base, fn, trim, flags, cb_data, packed);
 		if (retval)
 			goto end_each;
 	}
@@ -614,22 +644,28 @@ int head_ref(each_ref_fn fn, void *cb_data)
 
 int for_each_ref(each_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref("refs/", fn, 0, cb_data);
+	return do_for_each_ref("refs/", fn, 0, 0, cb_data);
 }
 
 int for_each_tag_ref(each_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref("refs/tags/", fn, 10, cb_data);
+	return do_for_each_ref("refs/tags/", fn, 10, 0, cb_data);
 }
 
 int for_each_branch_ref(each_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref("refs/heads/", fn, 11, cb_data);
+	return do_for_each_ref("refs/heads/", fn, 11, 0, cb_data);
 }
 
 int for_each_remote_ref(each_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref("refs/remotes/", fn, 13, cb_data);
+	return do_for_each_ref("refs/remotes/", fn, 13, 0, cb_data);
+}
+
+int for_each_rawref(each_ref_fn fn, void *cb_data)
+{
+	return do_for_each_ref("refs/", fn, 0,
+			       DO_FOR_EACH_INCLUDE_BROKEN, cb_data);
 }
 
 /*
diff --git a/refs.h b/refs.h
index 3bb529d..29bdcec 100644
--- a/refs.h
+++ b/refs.h
@@ -24,6 +24,11 @@ extern int for_each_tag_ref(each_ref_fn, void *);
 extern int for_each_branch_ref(each_ref_fn, void *);
 extern int for_each_remote_ref(each_ref_fn, void *);
 
+/* can be used to learn about broken ref and symref */
+extern int for_each_rawref(each_ref_fn, void *);
+
+extern void warn_dangling_symref(const char *msg_fmt, const char *refname);
+
 /*
  * Extra refs will be listed by for_each_ref() before any actual refs
  * for the duration of this process or until clear_extra_refs() is
-- 
1.6.2.rc0.28.g2593d

[PATCH 5/6] Warn use of "origin" when remotes/origin/HEAD is dangling

[Junio C Hamano]

The previous one squelched the diagnositic message we used to issue every
time we enumerated the refs and noticed a dangling ref.  This adds the
warning back to the place where the user actually attempts to use it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sha1_name.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 5d0ac02..3bd2ef0 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -268,16 +268,18 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)
 		char fullref[PATH_MAX];
 		unsigned char sha1_from_ref[20];
 		unsigned char *this_result;
+		int flag;
 
 		this_result = refs_found ? sha1_from_ref : sha1;
 		mksnpath(fullref, sizeof(fullref), *p, len, str);
-		r = resolve_ref(fullref, this_result, 1, NULL);
+		r = resolve_ref(fullref, this_result, 1, &flag);
 		if (r) {
 			if (!refs_found++)
 				*ref = xstrdup(r);
 			if (!warn_ambiguous_refs)
 				break;
-		}
+		} else if (flag & REF_ISSYMREF)
+			warning("ignoring dangling symref %s.", fullref);
 	}
 	free(last_branch);
 	return refs_found;
-- 
1.6.2.rc0.28.g2593d

[PATCH 6/6] receive-pack: default receive.denyDeleteCurrent to refuse

[Junio C Hamano]

This is for a future release that switches the default to refuse.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-receive-pack.c |   30 ++++++++++++------------------
 t/t5400-send-pack.sh   |    2 +-
 2 files changed, 13 insertions(+), 19 deletions(-)

diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index f7e04c4..44163ac 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -247,30 +247,24 @@ static void warn_unconfigured_deny(void)
 		warning(warn_unconfigured_deny_msg[i]);
 }
 
-static char *warn_unconfigured_deny_delete_current_msg[] = {
-	"Deleting the current branch can cause confusion by making the next",
-	"'git clone' not check out any file.",
+static char *refuse_unconfigured_deny_delete_current_msg[] = {
+	"By default, deleting the current branch is denied, because the next",
+	"'git clone' won't result in any file checked out, causing confusion.",
 	"",
 	"You can set 'receive.denyDeleteCurrent' configuration variable to",
-	"'refuse' in the remote repository to disallow deleting the current",
-	"branch.",
+	"'warn' or 'ignore' in the remote repository to allow deleting the",
+	"current branch, with or without a warning message.",
 	"",
-	"You can set it to 'ignore' to allow such a delete without a warning.",
-	"",
-	"To make this warning message less loud, you can set it to 'warn'.",
-	"",
-	"Note that the default will change in a future version of git",
-	"to refuse deleting the current branch unless you have the",
-	"configuration variable set to either 'ignore' or 'warn'."
+	"To squelch this message, you can set it to 'refuse'."
 };
 
-static void warn_unconfigured_deny_delete_current(void)
+static void refuse_unconfigured_deny_delete_current(void)
 {
 	int i;
 	for (i = 0;
-	     i < ARRAY_SIZE(warn_unconfigured_deny_delete_current_msg);
+	     i < ARRAY_SIZE(refuse_unconfigured_deny_delete_current_msg);
 	     i++)
-		warning(warn_unconfigured_deny_delete_current_msg[i]);
+		error(refuse_unconfigured_deny_delete_current_msg[i]);
 }
 
 static const char *update(struct command *cmd)
@@ -319,12 +313,12 @@ static const char *update(struct command *cmd)
 			case DENY_IGNORE:
 				break;
 			case DENY_WARN:
-			case DENY_UNCONFIGURED:
-				if (deny_delete_current == DENY_UNCONFIGURED)
-					warn_unconfigured_deny_delete_current();
 				warning("deleting the current branch");
 				break;
 			case DENY_REFUSE:
+			case DENY_UNCONFIGURED:
+				if (deny_delete_current == DENY_UNCONFIGURED)
+					refuse_unconfigured_deny_delete_current();
 				error("refusing to delete the current branch: %s", name);
 				return "deletion of the current branch prohibited";
 			}
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index 7b21f5f..6ef1f03 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -196,7 +196,7 @@ test_expect_success 'pushing wildcard refspecs respects forcing' '
 test_expect_success 'deny pushing to delete current branch' '
     (
 	rewound_push_setup &&
-	git send-pack ../parent/.git :refs/heads/master 2>errs
+	test_must_fail git send-pack ../parent/.git :refs/heads/master 2>errs
     )
 '
 
-- 
1.6.2.rc0.28.g2593d

Re: [PATCH 4/6] remote prune: warn dangling symrefs

[Jeff King]

On Mon, Feb 09, 2009 at 01:09:23AM -0800, Junio C Hamano wrote:

> If you prune from the remote "frotz" that deleted the ref your tracking
> branch remotes/frotz/HEAD points at, the symbolic ref will become
> dangling.  We used to detect this as an error condition and issued a
> message every time refs are enumerated.
> 
> This stops the error message, but moves the warning to "remote prune".

Very nice. As a bonus, this fixes certain (admittedly unlikely) renames,
too (which don't need to pass the BROKEN flag, since ref_rename uses
get_loose_refs directly):

  # without this patch
  $ git symbolic-ref refs/heads/foo/bar refs/heads/nonexistant
  $ git branch -m master foo
  error: refs/heads/foo/bar points nowhere!
  error: there are still refs under 'refs/heads/foo'
  error: unable to lock refs/heads/foo for update
  fatal: Branch rename failed

  # with this patch
  $ git branch -m master foo
  error: 'refs/heads/foo/bar' exists; cannot create 'refs/heads/foo'
  fatal: Branch rename failed

-Peff

Re: [PATCH 4/6] remote prune: warn dangling symrefs

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Feb 09, 2009 at 01:09:23AM -0800, Junio C Hamano wrote:
>
>> If you prune from the remote "frotz" that deleted the ref your tracking
>> branch remotes/frotz/HEAD points at, the symbolic ref will become
>> dangling.  We used to detect this as an error condition and issued a
>> message every time refs are enumerated.
>> 
>> This stops the error message, but moves the warning to "remote prune".
>
> Very nice. As a bonus, this fixes certain (admittedly unlikely) renames,
> too (which don't need to pass the BROKEN flag, since ref_rename uses
> get_loose_refs directly):
>
>   # without this patch
>   $ git symbolic-ref refs/heads/foo/bar refs/heads/nonexistant
>   $ git branch -m master foo
>   error: refs/heads/foo/bar points nowhere!
>   error: there are still refs under 'refs/heads/foo'
>   error: unable to lock refs/heads/foo for update
>   fatal: Branch rename failed
>
>   # with this patch
>   $ git branch -m master foo
>   error: 'refs/heads/foo/bar' exists; cannot create 'refs/heads/foo'
>   fatal: Branch rename failed

As a bonus, this issues unwarranted warning when creating the initial
commit in an empty repository.

The following fixes it.

-- >8 --
Subject: [PATCH] Squelch overzealous "ignoring dangling symref" in an empty repository

057e713 (Warn use of "origin" when remotes/origin/HEAD is dangling,
2009-02-08) tried to warn dangling refs/remotes/origin/HEAD only when
"origin" was used to refer to it.  There was one corner case a symref is
expected to be dangling and this warning is unwarranted: HEAD in an empty
repository.

This squelches the warning for this special case.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sha1_name.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 3bd2ef0..2f75179 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -278,7 +278,8 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)
 				*ref = xstrdup(r);
 			if (!warn_ambiguous_refs)
 				break;
-		} else if (flag & REF_ISSYMREF)
+		} else if ((flag & REF_ISSYMREF) &&
+			   (len != 4 || strcmp(str, "HEAD")))
 			warning("ignoring dangling symref %s.", fullref);
 	}
 	free(last_branch);
-- 
1.6.2.rc0.55.g7a105

Re: [PATCH 4/6] remote prune: warn dangling symrefs

[Jeff King]

On Wed, Feb 11, 2009 at 09:30:20AM -0800, Junio C Hamano wrote:

> Subject: [PATCH] Squelch overzealous "ignoring dangling symref" in an empty repository
> 
> 057e713 (Warn use of "origin" when remotes/origin/HEAD is dangling,
> 2009-02-08) tried to warn dangling refs/remotes/origin/HEAD only when
> "origin" was used to refer to it.  There was one corner case a symref is
> expected to be dangling and this warning is unwarranted: HEAD in an empty
> repository.
> 
> This squelches the warning for this special case.

Is the special case really about "this is HEAD", or is it about writing
versus reading? For example, in an empty repo, without this patch I now
get:

  $ git init && git show
  warning: ignoring dangling symref HEAD.
  fatal: bad default revision 'HEAD'

which makes a lot of sense. But _writing_ to any dangling symref
shouldn't trigger a warning.

Admittedly, we have gotten by without this warning until now, and I
doubt anyone will want to write to other symrefs that are branches to be
born, so I think in practice your patch is fine.

-Peff

Re: [PATCH 4/6] remote prune: warn dangling symrefs

[Jeff King]

On Wed, Feb 11, 2009 at 01:35:47PM -0500, Jeff King wrote:

> Is the special case really about "this is HEAD", or is it about writing
> versus reading? For example, in an empty repo, without this patch I now
> get:
> 
>   $ git init && git show
>   warning: ignoring dangling symref HEAD.
>   fatal: bad default revision 'HEAD'
> 
> which makes a lot of sense. But _writing_ to any dangling symref
> shouldn't trigger a warning.
> 
> Admittedly, we have gotten by without this warning until now, and I
> doubt anyone will want to write to other symrefs that are branches to be
> born, so I think in practice your patch is fine.

I looked into what it would take to fix this, and it's a little hairy.
"git commit" calls:

  if (get_sha1("HEAD", head_sha1))
          initial_commit = 1;

So it is fine with HEAD not resolving, but that intent is not passed to
lower-level code. So we could add a "flags" field, but the callstack
looks like:

  dwim_ref
  get_sha1_basic
  get_sha1_1
  get_sha1_with_mode
  get_sha1

all of which would need to pass the flag through, and all of which have
tons of callers (though we could do the get_sha1_with_flags trick).

I don't know if it's worth it.

-Peff

Re: [PATCH 3/6] receive-pack: receive.denyDeleteCurrent

[Jeff King]

On Mon, Feb 09, 2009 at 01:09:22AM -0800, Junio C Hamano wrote:

> This is a companion patch to the recent 3d95d92 (receive-pack: explain
> what to do when push updates the current branch, 2009-01-31).
> 
> Deleting the current branch from a remote will result in the next clone
> from it not check out anything, among other things.  It also is one of the
> cause that makes remotes/origin/HEAD a dangling symbolic ref.  This patch
> still allows the traditional behaviour but with a big warning, and promises
> that the default will change to 'refuse' in a future release.

This patch looks good to me. One comment:

> --- a/t/t5400-send-pack.sh
> +++ b/t/t5400-send-pack.sh
> @@ -193,4 +193,11 @@ test_expect_success 'pushing wildcard refspecs respects forcing' '
>      )
>  '
>  
> +test_expect_success 'deny pushing to delete current branch' '
> +    (
> +	rewound_push_setup &&
> +	git send-pack ../parent/.git :refs/heads/master 2>errs
> +    )
> +'

It was not immediately obvious why the subshell is needed here (for
those in the audience, it is because rewound_push_setup changes
directory). Perhaps 2/6 should be amended for rewound_push_setup to use
a subshell instead, which makes it less likely to be forgotten by new
tests.

-Peff

Re: [PATCH 3/6] receive-pack: receive.denyDeleteCurrent

[Jeff King]

On Mon, Feb 09, 2009 at 01:53:10PM -0500, Jeff King wrote:

> On Mon, Feb 09, 2009 at 01:09:22AM -0800, Junio C Hamano wrote:
> 
> > This is a companion patch to the recent 3d95d92 (receive-pack: explain
> > what to do when push updates the current branch, 2009-01-31).
> > 
> > Deleting the current branch from a remote will result in the next clone
> > from it not check out anything, among other things.  It also is one of the
> > cause that makes remotes/origin/HEAD a dangling symbolic ref.  This patch
> > still allows the traditional behaviour but with a big warning, and promises
> > that the default will change to 'refuse' in a future release.
> 
> This patch looks good to me. One comment:

Actually, one more comment on this one. If a user tries to delete the
current branch in a non-bare repository, they actually get _both_
gigantic warning messages (one for denyCurrentBranch and one for
denyDeleteCurrent).

Should the denyCurrentBranch code be triggering at all on a deletion?
That is, if I have:

  [receive]
    denyCurrentBranch = refuse
    denyDeleteCurrent = ignore

should such a deletion be refused or allowed?

-Peff

Re: [PATCH 3/6] receive-pack: receive.denyDeleteCurrent

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Should the denyCurrentBranch code be triggering at all on a deletion?
>
> That is, if I have:
>
>   [receive]
>     denyCurrentBranch = refuse
>     denyDeleteCurrent = ignore
>
> should such a deletion be refused or allowed?

I think denyCurrentBranch means do not touch the currently checked out
branch, so 'refuse' there should trump whatever denyDeleteCurrent says as
long as the repository has a work tree.

Perhaps the logic needs to be restructured to:

	if (the push affects the current branch) {
		if (in a repository with a work tree) {
                        decide if we want to refuse or allow;
                	decide what message to issue;
		}
		if (deletion and we decided not to refuse) {
                	decide if we want to refuse or allow;
                        decide what message to issue;
                }
                give message(s), possibly with a paragraph break in between;
                if (refuse)
                	refuse;
	}

Re: [PATCH 3/6] receive-pack: receive.denyDeleteCurrent

[Jeff King]

On Mon, Feb 09, 2009 at 01:38:28PM -0800, Junio C Hamano wrote:

> > Should the denyCurrentBranch code be triggering at all on a deletion?
> >
> > That is, if I have:
> >
> >   [receive]
> >     denyCurrentBranch = refuse
> >     denyDeleteCurrent = ignore
> >
> > should such a deletion be refused or allowed?
> 
> I think denyCurrentBranch means do not touch the currently checked out
> branch, so 'refuse' there should trump whatever denyDeleteCurrent says as
> long as the repository has a work tree.

I'm not sure "trump" is the right behavior. How would I specify "it is
OK to update this branch, but not to delete it, because I have installed
a hook that deals with the former but not the latter".

It seems like "delete" is a subset of "touch", so I think you probably
want to refuse if _either_ is refuse. Which I think maybe is what you
are saying here:

> Perhaps the logic needs to be restructured to:
> 
> 	if (the push affects the current branch) {
> 		if (in a repository with a work tree) {
>                         decide if we want to refuse or allow;
>                 	decide what message to issue;
> 		}
> 		if (deletion and we decided not to refuse) {
>                 	decide if we want to refuse or allow;
>                         decide what message to issue;
>                 }
>                 give message(s), possibly with a paragraph break in between;
>                 if (refuse)
>                 	refuse;
> 	}

but it was not immediately clear to me.

-Peff

Re: [PATCH 3/6] receive-pack: receive.denyDeleteCurrent

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> I think denyCurrentBranch means do not touch the currently checked out
>> branch, so 'refuse' there should trump whatever denyDeleteCurrent says as
>> long as the repository has a work tree.
>
> I'm not sure "trump" is the right behavior. How would I specify "it is
> OK to update this branch, but not to delete it, because I have installed
> a hook that deals with the former but not the latter".
>
> It seems like "delete" is a subset of "touch", so I think you probably
> want to refuse if _either_ is refuse. Which I think maybe is what you
> are saying here:

I meant "'refuse' in denyCurrentBranch trumps" and not "whatever is set to
denyCurrentBranch trumps".  IOW, I think we are in agreement.

Re: [PATCH 2/6] t5400: allow individual tests to fail

[Jeff King]

On Mon, Feb 09, 2009 at 01:09:21AM -0800, Junio C Hamano wrote:

> Each test chdir'ed around and ended up in a random place if any of the
> test in the sequence failed but the entire test script was allowed to
> run.  This wrapps each in a subshell as necessary.

Certainly a good cleanup, but...

> -test_expect_success \
> -        'push can be used to delete a ref' '
> +test_expect_success 'push can be used to delete a ref' '
> +    (
>  	cd victim &&
>  	git branch extra master &&
>  	cd .. &&
>  	test -f victim/.git/refs/heads/extra &&
>  	git send-pack ./victim/.git/ :extra master &&
>  	! test -f victim/.git/refs/heads/extra
> +    )
>  '

Wouldn't this be cleaner as:

  (
    cd victim &&
    git branch extra master
  ) &&
  ...

That is, it is not only safer but (IMHO) a bit easier to see which parts
are happening in which directory.

> +test_expect_success 'pushing a delete should be denied with denyDeletes' '
> +    (
>  	cd victim &&
>  	git config receive.denyDeletes true &&
>  	git branch extra master &&
>  	cd .. &&
>  	test -f victim/.git/refs/heads/extra &&
>  	test_must_fail git send-pack ./victim/.git/ :extra master
> +    )

Ditto (and there are more, but I won't quote each one).

> +test_expect_success 'pushing with --force should be denied with denyNonFastforwards' '
> +    (
>  	cd victim &&
>  	git config receive.denyNonFastforwards true &&
>  	cd .. &&
>  	git update-ref refs/heads/master master^ || return 1
>  	git send-pack --force ./victim/.git/ master && return 1
>  	! test_cmp .git/refs/heads/master victim/.git/refs/heads/master
> +    )

And here I don't know what in the world is going on with those "return
1" lines. Shouldn't this be a chain of &&'s with a test_must_fail?
I.e.,:

  ( cd victim && git config receive.denyNonFastforwards true ) &&
  git update-ref refs/heads/master master^ &&
  test_must_fail git send-pack --force ./victim/.git/ master &&
  ! test_cmp .git/refs/heads/master victim/.git/refs/heads/master

Not to mention that the final test_cmp would be more robust if written
to make sure the victim's master ref stayed the same (instead of just
making sure we didn't screw it up in one particular way). And it should
probably use a git command rather than looking at the refs files (to be
future-proof against any automatic ref-packing), but that is just
nit-picking.


All minor things, of course, but while we're cleaning up... :)

-Peff

Re: [PATCH 2/6] t5400: allow individual tests to fail

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Feb 09, 2009 at 01:09:21AM -0800, Junio C Hamano wrote:
>
>> Each test chdir'ed around and ended up in a random place if any of the
>> test in the sequence failed but the entire test script was allowed to
>> run.  This wrapps each in a subshell as necessary.
>
> Certainly a good cleanup, but...
>
>> -test_expect_success \
>> -        'push can be used to delete a ref' '
>> +test_expect_success 'push can be used to delete a ref' '
>> +    (
>>  	cd victim &&
>>  	git branch extra master &&
>>  	cd .. &&
>>  	test -f victim/.git/refs/heads/extra &&
>>  	git send-pack ./victim/.git/ :extra master &&
>>  	! test -f victim/.git/refs/heads/extra
>> +    )
>>  '
>
> Wouldn't this be cleaner as:
>
>   (
>     cd victim &&
>     git branch extra master
>   ) &&
>   ...
>
> That is, it is not only safer but (IMHO) a bit easier to see which parts
> are happening in which directory.
>
>> +test_expect_success 'pushing a delete should be denied with denyDeletes' '
>> +    (
>>  	cd victim &&
>>  	git config receive.denyDeletes true &&
>>  	git branch extra master &&
>>  	cd .. &&
>>  	test -f victim/.git/refs/heads/extra &&
>>  	test_must_fail git send-pack ./victim/.git/ :extra master
>> +    )
>
> Ditto (and there are more, but I won't quote each one).
>
>> +test_expect_success 'pushing with --force should be denied with denyNonFastforwards' '
>> +    (
>>  	cd victim &&
>>  	git config receive.denyNonFastforwards true &&
>>  	cd .. &&
>>  	git update-ref refs/heads/master master^ || return 1
>>  	git send-pack --force ./victim/.git/ master && return 1
>>  	! test_cmp .git/refs/heads/master victim/.git/refs/heads/master
>> +    )
>
> And here I don't know what in the world is going on with those "return
> 1" lines. Shouldn't this be a chain of &&'s with a test_must_fail?
> I.e.,:
>
>   ( cd victim && git config receive.denyNonFastforwards true ) &&
>   git update-ref refs/heads/master master^ &&
>   test_must_fail git send-pack --force ./victim/.git/ master &&
>   ! test_cmp .git/refs/heads/master victim/.git/refs/heads/master
>
> Not to mention that the final test_cmp would be more robust if written
> to make sure the victim's master ref stayed the same (instead of just
> making sure we didn't screw it up in one particular way). And it should
> probably use a git command rather than looking at the refs files (to be
> future-proof against any automatic ref-packing), but that is just
> nit-picking.
>
>
> All minor things, of course, but while we're cleaning up... :)

Sure.  This was made as a quick-fix to a mess others created, so I did not
study them very deeply.

Will reroll if I have the time but it is likely that I may be tending
other topics first.

Re: [PATCH 2/6] t5400: allow individual tests to fail

[Junio C Hamano]

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

> Jeff King <peff@peff.net> writes:
>
>> All minor things, of course, but while we're cleaning up... :)
>
> Sure.  This was made as a quick-fix to a mess others created, so I did not
> study them very deeply.
>
> Will reroll if I have the time but it is likely that I may be tending
> other topics first.

-- >8 --
Subject: [PATCH] Modernize t5400 test script

Many tests checked for failure by hand without using test_must_fail (they
probably predate the shell function).

When we know the desired outcome, explicitly check for it, instead of
checking if the result does not match one possible incorrect outcome.
E.g. if you expect a push to be refused, you do not test if the result is
different from what was pushed.  Instead, make sure that the ref did not
before and after the push.

The test sequence chdir'ed around and any failure at one point could have
started the next test in an unexpected directory.  Fix this problem by
using subshells as necessary.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5400-send-pack.sh |  180 ++++++++++++++++++++++++++------------------------
 1 files changed, 94 insertions(+), 86 deletions(-)

diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index b21317d..785c2f4 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -32,9 +32,7 @@ test_expect_success setup '
 	done &&
 	git update-ref HEAD "$commit" &&
 	git clone ./. victim &&
-	cd victim &&
-	git log &&
-	cd .. &&
+	( cd victim && git log ) &&
 	git update-ref HEAD "$zero" &&
 	parent=$zero &&
 	i=0 &&
@@ -59,88 +57,84 @@ test_expect_success 'pack the source repository' '
 '
 
 test_expect_success 'pack the destination repository' '
+    (
 	cd victim &&
 	git repack -a -d &&
-	git prune &&
-	cd ..
+	git prune
+    )
 '
 
-test_expect_success \
-        'pushing rewound head should not barf but require --force' '
-	# should not fail but refuse to update.
-	if git send-pack ./victim/.git/ master
-	then
-		# now it should fail with Pasky patch
-		echo >&2 Gaah, it should have failed.
-		false
-	else
-		echo >&2 Thanks, it correctly failed.
-		true
-	fi &&
-	if cmp victim/.git/refs/heads/master .git/refs/heads/master
-	then
-		# should have been left as it was!
-		false
-	else
-		true
-	fi &&
+test_expect_success 'refuse pushing rewound head without --force' '
+	pushed_head=$(git rev-parse --verify master) &&
+	victim_orig=$(cd victim && git rev-parse --verify master) &&
+	test_must_fail git send-pack ./victim master &&
+	victim_head=$(cd victim && git rev-parse --verify master) &&
+	test "$victim_head" = "$victim_orig" &&
 	# this should update
-	git send-pack --force ./victim/.git/ master &&
-	cmp victim/.git/refs/heads/master .git/refs/heads/master
+	git send-pack --force ./victim master &&
+	victim_head=$(cd victim && git rev-parse --verify master) &&
+	test "$victim_head" = "$pushed_head"
 '
 
 test_expect_success \
         'push can be used to delete a ref' '
-	cd victim &&
-	git branch extra master &&
-	cd .. &&
-	test -f victim/.git/refs/heads/extra &&
-	git send-pack ./victim/.git/ :extra master &&
-	! test -f victim/.git/refs/heads/extra
+	( cd victim && git branch extra master ) &&
+	git send-pack ./victim :extra master &&
+	( cd victim &&
+	  test_must_fail git rev-parse --verify extra )
 '
 
-unset GIT_CONFIG
-HOME=`pwd`/no-such-directory
-export HOME ;# this way we force the victim/.git/config to be used.
-
-test_expect_success \
-	'pushing a delete should be denied with denyDeletes' '
-	cd victim &&
-	git config receive.denyDeletes true &&
-	git branch extra master &&
-	cd .. &&
-	test -f victim/.git/refs/heads/extra &&
-	test_must_fail git send-pack ./victim/.git/ :extra master
+test_expect_success 'refuse deleting push with denyDeletes' '
+	(
+	    cd victim &&
+	    ( git branch -D extra || : ) &&
+	    git config receive.denyDeletes true &&
+	    git branch extra master
+	) &&
+	test_must_fail git send-pack ./victim :extra master
 '
-rm -f victim/.git/refs/heads/extra
 
-test_expect_success \
-        'pushing with --force should be denied with denyNonFastforwards' '
-	cd victim &&
-	git config receive.denyNonFastforwards true &&
-	cd .. &&
-	git update-ref refs/heads/master master^ || return 1
-	git send-pack --force ./victim/.git/ master && return 1
-	! test_cmp .git/refs/heads/master victim/.git/refs/heads/master
+test_expect_success 'denyNonFastforwards trumps --force' '
+	(
+	    cd victim &&
+	    ( git branch -D extra || : ) &&
+	    git config receive.denyNonFastforwards true
+	) &&
+	victim_orig=$(cd victim && git rev-parse --verify master) &&
+	test_must_fail git send-pack --force ./victim master^:master &&
+	victim_head=$(cd victim && git rev-parse --verify master) &&
+	test "$victim_orig" = "$victim_head"
 '
 
-test_expect_success \
-	'pushing does not include non-head refs' '
-	mkdir parent && cd parent &&
-	git init && touch file && git add file && git commit -m add &&
-	cd .. &&
-	git clone parent child && cd child && git push --all &&
-	cd ../parent &&
-	git branch -a >branches && ! grep origin/master branches
+test_expect_success 'push --all excludes remote tracking hierarchy' '
+	mkdir parent &&
+	(
+	    cd parent &&
+	    git init && : >file && git add file && git commit -m add
+	) &&
+	git clone parent child &&
+	(
+	    cd child && git push --all
+	) &&
+	(
+	    cd parent &&
+	    test -z "$(git for-each-ref refs/remotes/origin)"
+	)
 '
 
 rewound_push_setup() {
 	rm -rf parent child &&
-	mkdir parent && cd parent &&
-	git init && echo one >file && git add file && git commit -m one &&
-	echo two >file && git commit -a -m two &&
-	cd .. &&
-	git clone parent child && cd child && git reset --hard HEAD^
+	mkdir parent &&
+	(
+	    cd parent &&
+	    git init &&
+	    echo one >file && git add file && git commit -m one &&
+	    echo two >file && git commit -a -m two
+	) &&
+	git clone parent child &&
+	(
+	    cd child && git reset --hard HEAD^
+	)
 }
 
 rewound_push_succeeded() {
@@ -156,30 +150,44 @@ rewound_push_failed() {
 	fi
 }
 
-test_expect_success \
-	'pushing explicit refspecs respects forcing' '
+test_expect_success 'pushing explicit refspecs respects forcing' '
 	rewound_push_setup &&
-	if git send-pack ../parent/.git refs/heads/master:refs/heads/master
-	then
-		false
-	else
-		true
-	fi && rewound_push_failed &&
-	git send-pack ../parent/.git +refs/heads/master:refs/heads/master &&
-	rewound_push_succeeded
+	parent_orig=$(cd parent && git rev-parse --verify master) &&
+	(
+	    cd child &&
+	    test_must_fail git send-pack ../parent \
+		refs/heads/master:refs/heads/master
+	) &&
+	parent_head=$(cd parent && git rev-parse --verify master) &&
+	test "$parent_orig" = "$parent_head"
+	(
+	    cd child &&
+	    git send-pack ../parent \
+	        +refs/heads/master:refs/heads/master
+	) &&
+	parent_head=$(cd parent && git rev-parse --verify master) &&
+	child_head=$(cd parent && git rev-parse --verify master) &&
+	test "$parent_head" = "$child_head"
 '
 
-test_expect_success \
-	'pushing wildcard refspecs respects forcing' '
+test_expect_success 'pushing wildcard refspecs respects forcing' '
 	rewound_push_setup &&
-	if git send-pack ../parent/.git refs/heads/*:refs/heads/*
-	then
-		false
-	else
-		true
-	fi && rewound_push_failed &&
-	git send-pack ../parent/.git +refs/heads/*:refs/heads/* &&
-	rewound_push_succeeded
+	parent_orig=$(cd parent && git rev-parse --verify master) &&
+	(
+	    cd child &&
+	    test_must_fail git send-pack ../parent \
+	        "refs/heads/*:refs/heads/*"
+	) &&
+	parent_head=$(cd parent && git rev-parse --verify master) &&
+	test "$parent_orig" = "$parent_head"
+	(
+	    cd child &&
+	    git send-pack ../parent \
+	        "+refs/heads/*:refs/heads/*"
+	) &&
+	parent_head=$(cd parent && git rev-parse --verify master) &&
+	child_head=$(cd parent && git rev-parse --verify master) &&
+	test "$parent_head" = "$child_head"
 '
 
 test_done
-- 
1.6.2.rc0.36.g8307

Re: [PATCH 2/6] t5400: allow individual tests to fail

[Jeff King]

On Mon, Feb 09, 2009 at 01:39:52PM -0800, Junio C Hamano wrote:

> > Will reroll if I have the time but it is likely that I may be tending
> > other topics first.
> 
> -- >8 --
> Subject: [PATCH] Modernize t5400 test script

Much nicer, thanks.

-Peff

Re: Deleting the "current" branch in remote bare repositories

[Jeff King]

On Sun, Feb 08, 2009 at 11:05:40AM -0800, Junio C Hamano wrote:

> If you remove the current branch from a repository, the impact that
> operation causes the remote users of the repository would be the same
> whether the repository has or does not have a work tree.  And in that
> sense, I think it should apply equally.  The criteria is not "is it bare",
> but "is it used by people remotely".  IOW, you refuse deletion of the
> current branch if other people may fetch from it.

Here are two reasons why we might not want to conflate the two safety
valves:

 1. I'm concerned about it becoming more confusing for end users. Right
    now we don't even bother to look at denyCurrentBranch if we are in a
    bare repo. That is, it is easily explained as "this prevents a
    dangerous operation pushing into a non-bare repo, and if you have a
    bare repo, you don't have to care about or set this at all." But
    having it cover HEAD deletion, too, means that it is now "prevent
    some dangerous operations; some things are dangerous if your repo is
    non-bare, but safe otherwise.  But some of the operations are
    dangerous all the time".

    But maybe that is not really a concern. Most users should leave it
    set to the default unless they are clueful enough to know which
    operations are safe and which are not.

 2. It may not be fine-grained enough. If I am a clueful user with a
    post-receive hook, then I may want to set receive.denyCurrentBranch
    to "ignore". But that doesn't necessarily mean my hook gracefully
    handles _deletion_ of the branch. I may want to keep that set to
    "refuse", since it makes no sense for my workflow.

    Such a user is better served by a separate receive.denyDeleteHEAD
    option.

-Peff

Re: Deleting the "current" branch in remote bare repositories

[Jeff King]

On Mon, Feb 09, 2009 at 01:28:43PM -0500, Jeff King wrote:

> Here are two reasons why we might not want to conflate the two safety
> valves:

Argh, sorry, I'm an idiot. For some reason when I saw
"receive.denyDeleteCurrent" I read "receive.denyCurrentBranch" (and if
you look carefully in the thread, I even _typed_ it wrong, too!).

So I read your emails as "we should put this new safety valve under the
same option as the other one" which you are of course not proposing at
all.

Please disregard this last email (and the one before it, but you have
already responded to that ;) ). And I will ask a doctor to remove the brain
tumor that is clearly inhibiting my reading comprehension.

-Peff