[PATCH] optionally deny all pushes

[PATCH] optionally deny all pushes

[Clemens Buchacher]

For a mirror repository which is updated only by pulling from upstream,
it is convenient to disallow all pushes. An accidental push from a
downstream repository would mess up the mirror's state as well as future
updates.

Add a boolean configuration variable receive.denyAll. If enabled,
receive-pack will deny all ref updates.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 Documentation/config.txt               |    4 ++++
 Documentation/git-push.txt             |    1 +
 builtin/receive-pack.c                 |   11 +++++++++++
 contrib/completion/git-completion.bash |    1 +
 t/t5400-send-pack.sh                   |   17 +++++++++++++++++
 t/t5516-fetch-push.sh                  |   12 ++++++++++++
 6 files changed, 46 insertions(+), 0 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 04f5e19..edbddea 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1683,6 +1683,10 @@ receive.unpackLimit::
 	especially on slow filesystems.  If not set, the value of
 	`transfer.unpackLimit` is used instead.
 
+receive.denyAll::
+	If set to true, git-receive-pack will deny all ref updates.
+	The repository can not be updated remotely.
+
 receive.denyDeletes::
 	If set to true, git-receive-pack will deny a ref update that deletes
 	the ref. Use this to prevent such a ref deletion via a push.
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index aede488..7c6cc63 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -219,6 +219,7 @@ remote rejected::
 	The remote end refused the update.  Usually caused by a hook
 	on the remote side, or because the remote repository has one
 	of the following safety options in effect:
+	`receive.denyAll` (for all pushes),
 	`receive.denyCurrentBranch` (for pushes to the checked out
 	branch), `receive.denyNonFastForwards` (for forced
 	non-fast-forward updates), `receive.denyDeletes` or
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index d2dcb7e..9cd04f9 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -22,6 +22,7 @@ enum deny_action {
 	DENY_REFUSE
 };
 
+static int deny_all;
 static int deny_deletes;
 static int deny_non_fast_forwards;
 static enum deny_action deny_current_branch = DENY_UNCONFIGURED;
@@ -57,6 +58,11 @@ static enum deny_action parse_deny_action(const char *var, const char *value)
 
 static int receive_pack_config(const char *var, const char *value, void *cb)
 {
+	if (strcmp(var, "receive.denyall") == 0) {
+		deny_all = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (strcmp(var, "receive.denydeletes") == 0) {
 		deny_deletes = git_config_bool(var, value);
 		return 0;
@@ -401,6 +407,11 @@ static const char *update(struct command *cmd)
 	unsigned char *new_sha1 = cmd->new_sha1;
 	struct ref_lock *lock;
 
+	if (deny_all) {
+		rp_error("denying all updates");
+		return "update prohibited";
+	}
+
 	/* only refs/... are allowed */
 	if (prefixcmp(name, "refs/") || check_refname_format(name + 5, 0)) {
 		rp_error("refusing to create funny ref '%s' remotely", name);
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index b0062ba..9d63622 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2250,6 +2250,7 @@ _git_config ()
 		rebase.autosquash
 		rebase.stat
 		receive.autogc
+		receive.denyAll
 		receive.denyCurrentBranch
 		receive.denyDeleteCurrent
 		receive.denyDeletes
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index 0eace37..e8c9be2 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -222,4 +222,21 @@ test_expect_success 'deny pushing to delete current branch' '
 	)
 '
 
+test_expect_success 'deny pushing to any branch with denyAll' '
+	rewound_push_setup &&
+	(
+	    cd parent &&
+	    git config receive.denyCurrentBranch false &&
+	    git config receive.denyAll true
+	) &&
+	(
+	    cd child &&
+	    test_must_fail git send-pack ../parent master 2>errs
+	) &&
+	(
+	    cd child &&
+	    test_must_fail git send-pack ../parent :refs/heads/master 2>errs
+	)
+'
+
 test_done
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index b69cf57..d1117c0 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -815,6 +815,18 @@ test_expect_success 'allow push to HEAD of bare repository (bare)' '
 	! grep "warning: updating the current branch" stderr
 '
 
+test_expect_success 'deny push to HEAD of bare repository (denyAll)' '
+	mk_test heads/master &&
+	(
+		cd testrepo &&
+		git checkout master &&
+		git config receive.denyCurrentBranch false &&
+		git config receive.denyAll true &&
+		git config core.bare true
+	) &&
+	test_must_fail git push testrepo master
+'
+
 test_expect_success 'allow push to HEAD of non-bare repository (config)' '
 	mk_test heads/master &&
 	(
-- 
1.7.8

Re: [PATCH] optionally deny all pushes

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> For a mirror repository which is updated only by pulling from upstream,
> it is convenient to disallow all pushes. An accidental push from a
> downstream repository would mess up the mirror's state as well as future
> updates.
>
> Add a boolean configuration variable receive.denyAll. If enabled,
> receive-pack will deny all ref updates.
>
> Signed-off-by: Clemens Buchacher <drizzd@aon.at>
> ---

I do not like this at all.

Especially given that this seems equivalent to:

	$ echo exit 1 >.git/hooks/pre-receive
        $ chmod +x .git/hooks/pre-receive

this is a mere configuration bloat, isn't it?

Your patch however raises an interesting point. Our pre-receive rejection
infrastructure does have a room for improvement.

A typical "push" exchange goes like this:

    receiver: Here are the refs I have.
    sender:   I want to update your refs with these.
    sender:   Here is the object data to complete the updated refs.
    receiver: Let's see what you are trying to do... I may say:
              - I refuse your proposal to update/delete this branch,
                as it is the current branch.
              - My hook tells me to refuse your proposal to update this
                ref, as it does not fast-forward.

We can see that we could decide to reject the push to update the current
branch even before receiving the bulk of transfer. I suspect it might need
a protocol extension to give the receiver a chance to speak after hearing
the proposed set of updates before receiving the bulk of the data to add a
clean error codepath, but at the worset case, we could immediately close
the connection after hearing what the sender proposes to do tries to touch
the current branch in a way we do not like. In other words, the exchange
could become:

    receiver: Here are the refs I have.
    sender:   I want to update your refs with these.
    receiver: Let's see what you are trying to do... I may say:
              - I can tell you without actually looking at the commits you
                are proposing to place at the tips of my refs that I won't
                like this branch to be updated/deleted. Go away.
    sender:   Here is the object data to complete the updated refs.
    receiver: Let's see what you are trying to do...
              - My hook tells me to refuse your proposal to update this
                ref, as it does not fast-forward.

Your "reject any and all" better fits into that new rejection
codepath. Just like the configuration that rejects updates to the current
branch, the decision your patch wants to make can be made before we see
any object data from the sender.

Another thing to consider is that you do not have to be limited to "all".

Often people want to run "git fetch" on their mothership box receiving
data from their notebook, i.e.

    mothership$ git fetch notebook master:refs/remotes/notebook/master
    mothership$ git merge notebook/master

but are prevented from doing so due to networking issues (e.g. your
notebook may not allow incoming connections).  The right way to emulate
this is to initiate the connection in the reverse direction, running "git
push" on their notebook sending data into their mothership box, i.e.

    notebook$ git push mothership master:refs/remotes/notebook/master
    mothership$ git merge notebook/master

In such a situation, you may want to configure your mothership repository
to reject any push outside refs/remotes/ hierarchy to prevent mistakes.
We could add "receive.allowed = refs/remotes/*" or something to support
this use case.  It might turn out that we may even want to make this the
default for non-bare repositories.

Once that happens, you can naturally say "receive.allowed = none" to do
what your patch wants to do as a narrow special case. Wouldn't that be a
much better approach than a "reject any and all" that can only serve a
single narrow case that can already be done with a simple hook?

[PATCH 0/5] verify refs early

[Clemens Buchacher]

Hi Junio,

This series centers around "[PATCH 3/5] git push: verify refs early",
which implements your suggestions in reply to my "[PATCH] optionally
deny all pushes". I am not pursuing the deny config option further,
because as you noted, it is of course made redundant by a simple
pre-receive hook.

[PATCH 1/5] git rev-list: fix invalid typecast

I consider this one a fairly important bugfix. Although I was unable to
determine under which circumstances the bug takes effect, rev-list
probably takes a performance hit when it does.

[PATCH 2/5] do not override receive-pack errors
[PATCH 3/5] git push: verify refs early

Note that [PATCH 3/5] makes an extension to the transfer protocol.

[PATCH 4/5] t5541: use configured port number

A mostly unrelated bugfix.

[PATCH 5/5] push/fetch/clone --no-progress suppresses progress

A totally unrelated change. However, this conflicts textually
(lexically?) with [PATCH 3/5], and therefore I am including it here.

Clemens

[PATCH 1/5] git rev-list: fix invalid typecast

[Clemens Buchacher]

git rev-list passes rev_list_info, not rev_list objects. Without this
fix, rev-list enables or disables the --verify-objects option depending
on a read from an undefined memory location.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 builtin/rev-list.c |    4 ++--
 t/t1450-fsck.sh    |   26 ++++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index ab3be7c..264e3ae 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -180,10 +180,10 @@ static void show_object(struct object *obj,
 			const struct name_path *path, const char *component,
 			void *cb_data)
 {
-	struct rev_info *info = cb_data;
+	struct rev_list_info *info = cb_data;
 
 	finish_object(obj, path, component, cb_data);
-	if (info->verify_objects && !obj->parsed && obj->type != OBJ_COMMIT)
+	if (info->revs->verify_objects && !obj->parsed && obj->type != OBJ_COMMIT)
 		parse_object(obj->sha1);
 	show_object_with_name(stdout, obj, path, component);
 }
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 523ce9c..5b8ebd8 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -191,4 +191,30 @@ test_expect_success 'cleaned up' '
 	test_cmp empty actual
 '
 
+test_expect_success 'rev-list --verify-objects' '
+	git rev-list --verify-objects --all >/dev/null 2>out &&
+	test_cmp empty out
+'
+
+test_expect_success 'rev-list --verify-objects with bad sha1' '
+	sha=$(echo blob | git hash-object -w --stdin) &&
+	old=$(echo $sha | sed "s+^..+&/+") &&
+	new=$(dirname $old)/ffffffffffffffffffffffffffffffffffffff &&
+	sha="$(dirname $new)$(basename $new)" &&
+	mv .git/objects/$old .git/objects/$new &&
+	test_when_finished "remove_object $sha" &&
+	git update-index --add --cacheinfo 100644 $sha foo &&
+	test_when_finished "git read-tree -u --reset HEAD" &&
+	tree=$(git write-tree) &&
+	test_when_finished "remove_object $tree" &&
+	cmt=$(echo bogus | git commit-tree $tree) &&
+	test_when_finished "remove_object $cmt" &&
+	git update-ref refs/heads/bogus $cmt &&
+	test_when_finished "git update-ref -d refs/heads/bogus" &&
+
+	test_might_fail git rev-list --verify-objects refs/heads/bogus >/dev/null 2>out &&
+	cat out &&
+	grep -q "error: sha1 mismatch 63ffffffffffffffffffffffffffffffffffffff" out
+'
+
 test_done
-- 
1.7.9

[PATCH 2/5] do not override receive-pack errors

[Clemens Buchacher]

Receive runs rev-list --verify-objects in order to detect missing
objects. However, such errors are ignored and overridden later.
Instead, consequently ignore all update commands for which an error has
already been detected.

Some tests in t5504 are obsoleted by this change, because invalid
objects are detected even if fsck is not enabled. Instead, they now test
for different error messages depending on whether or not fsck is turned
on. A better fix would be to force a corruption that will be detected by
fsck but not by rev-list.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 builtin/receive-pack.c          |   24 +++++++++++++++++-------
 t/t5504-fetch-receive-strict.sh |   22 ++++++++++++++++++----
 2 files changed, 35 insertions(+), 11 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index fa7448b..0afb8b2 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -642,8 +642,10 @@ static void check_aliased_updates(struct command *commands)
 	}
 	sort_string_list(&ref_list);
 
-	for (cmd = commands; cmd; cmd = cmd->next)
-		check_aliased_update(cmd, &ref_list);
+	for (cmd = commands; cmd; cmd = cmd->next) {
+		if (!cmd->error_string)
+			check_aliased_update(cmd, &ref_list);
+	}
 
 	string_list_clear(&ref_list, 0);
 }
@@ -707,8 +709,10 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
 		set_connectivity_errors(commands);
 
 	if (run_receive_hook(commands, pre_receive_hook, 0)) {
-		for (cmd = commands; cmd; cmd = cmd->next)
-			cmd->error_string = "pre-receive hook declined";
+		for (cmd = commands; cmd; cmd = cmd->next) {
+			if (!cmd->error_string)
+				cmd->error_string = "pre-receive hook declined";
+		}
 		return;
 	}
 
@@ -717,9 +721,15 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
 	free(head_name_to_free);
 	head_name = head_name_to_free = resolve_refdup("HEAD", sha1, 0, NULL);
 
-	for (cmd = commands; cmd; cmd = cmd->next)
-		if (!cmd->skip_update)
-			cmd->error_string = update(cmd);
+	for (cmd = commands; cmd; cmd = cmd->next) {
+		if (cmd->error_string)
+			continue;
+
+		if (cmd->skip_update)
+			continue;
+
+		cmd->error_string = update(cmd);
+	}
 }
 
 static struct command *read_head_info(void)
diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index 8341fc4..35ec294 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -58,6 +58,11 @@ test_expect_success 'fetch with transfer.fsckobjects' '
 	)
 '
 
+cat >exp <<EOF
+To dst
+!	refs/heads/master:refs/heads/test	[remote rejected] (missing necessary objects)
+EOF
+
 test_expect_success 'push without strict' '
 	rm -rf dst &&
 	git init dst &&
@@ -66,7 +71,8 @@ test_expect_success 'push without strict' '
 		git config fetch.fsckobjects false &&
 		git config transfer.fsckobjects false
 	) &&
-	git push dst master:refs/heads/test
+	test_must_fail git push --porcelain dst master:refs/heads/test >act &&
+	test_cmp exp act
 '
 
 test_expect_success 'push with !receive.fsckobjects' '
@@ -77,9 +83,15 @@ test_expect_success 'push with !receive.fsckobjects' '
 		git config receive.fsckobjects false &&
 		git config transfer.fsckobjects true
 	) &&
-	git push dst master:refs/heads/test
+	test_must_fail git push --porcelain dst master:refs/heads/test >act &&
+	test_cmp exp act
 '
 
+cat >exp <<EOF
+To dst
+!	refs/heads/master:refs/heads/test	[remote rejected] (n/a (unpacker error))
+EOF
+
 test_expect_success 'push with receive.fsckobjects' '
 	rm -rf dst &&
 	git init dst &&
@@ -88,7 +100,8 @@ test_expect_success 'push with receive.fsckobjects' '
 		git config receive.fsckobjects true &&
 		git config transfer.fsckobjects false
 	) &&
-	test_must_fail git push dst master:refs/heads/test
+	test_must_fail git push --porcelain dst master:refs/heads/test >act &&
+	test_cmp exp act
 '
 
 test_expect_success 'push with transfer.fsckobjects' '
@@ -98,7 +111,8 @@ test_expect_success 'push with transfer.fsckobjects' '
 		cd dst &&
 		git config transfer.fsckobjects true
 	) &&
-	test_must_fail git push dst master:refs/heads/test
+	test_must_fail git push --porcelain dst master:refs/heads/test >act &&
+	test_cmp exp act
 '
 
 test_done
-- 
1.7.9

[PATCH 3/5] git push: verify refs early

[Clemens Buchacher]

Receive-pack waits for the transfer of all new object data to complete
before taking a closer look at the received refs. It may then decide to
refuse updates of refs without even looking at the data.

Instead, do as much ref validation as possible before new object data is
available, in order to avoid a potentially expensive transfer.
Receive-pack will then respond with "verify-refs ok" if at least one ref
update is valid. Otherwise, it will respond with "verify-refs no valid
refs".

On the client side, send-pack will wait for "verify-refs ok" or skip the
object transfer entirely if it reads a packet line that starts with
"verify-refs " but does not match "verify-refs ok". Any other response
is considered an error.

Early ref validation is disabled for the smart HTTP protocol, since it
tries to fit the entire push (ref info and object data) into a single
POST request.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

I suppose with some effort, this could be done for smart HTTP as well.
But I am not sure if we actually want the overhead of the additional
ping-pong for HTTP.

 builtin/receive-pack.c |   83 ++++++++++++++++++++++++++++++++++++++----------
 builtin/send-pack.c    |   43 +++++++++++++++++--------
 send-pack.h            |    3 +-
 3 files changed, 97 insertions(+), 32 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 0afb8b2..0129d9c 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -34,6 +34,8 @@ static int unpack_limit = 100;
 static int report_status;
 static int use_sideband;
 static int quiet;
+static int verify_refs;
+static int stateless_rpc;
 static int prefer_ofs_delta = 1;
 static int auto_update_server_info;
 static int auto_gc = 1;
@@ -123,7 +125,7 @@ static void show_ref(const char *path, const unsigned char *sha1)
 	else
 		packet_write(1, "%s %s%c%s%s\n",
 			     sha1_to_hex(sha1), path, 0,
-			     " report-status delete-refs side-band-64k quiet",
+			     " report-status delete-refs side-band-64k quiet verify-refs",
 			     prefer_ofs_delta ? " ofs-delta" : "");
 	sent_capabilities = 1;
 }
@@ -410,14 +412,13 @@ static void refuse_unconfigured_deny_delete_current(void)
 		rp_error("%s", refuse_unconfigured_deny_delete_current_msg[i]);
 }
 
-static const char *update(struct command *cmd)
+static const char *verify_ref(struct command *cmd)
 {
 	const char *name = cmd->ref_name;
 	struct strbuf namespaced_name_buf = STRBUF_INIT;
 	const char *namespaced_name;
 	unsigned char *old_sha1 = cmd->old_sha1;
 	unsigned char *new_sha1 = cmd->new_sha1;
-	struct ref_lock *lock;
 
 	/* only refs/... are allowed */
 	if (prefixcmp(name, "refs/") || check_refname_format(name + 5, 0)) {
@@ -444,12 +445,6 @@ static const char *update(struct command *cmd)
 		}
 	}
 
-	if (!is_null_sha1(new_sha1) && !has_sha1_file(new_sha1)) {
-		error("unpack should have generated %s, "
-		      "but I can't find it!", sha1_to_hex(new_sha1));
-		return "bad pack";
-	}
-
 	if (!is_null_sha1(old_sha1) && is_null_sha1(new_sha1)) {
 		if (deny_deletes && !prefixcmp(name, "refs/heads/")) {
 			rp_error("denying ref deletion for %s", name);
@@ -473,6 +468,27 @@ static const char *update(struct command *cmd)
 		}
 	}
 
+	return NULL;
+}
+
+static const char *update(struct command *cmd)
+{
+	const char *name = cmd->ref_name;
+	struct strbuf namespaced_name_buf = STRBUF_INIT;
+	const char *namespaced_name;
+	unsigned char *old_sha1 = cmd->old_sha1;
+	unsigned char *new_sha1 = cmd->new_sha1;
+	struct ref_lock *lock;
+
+	strbuf_addf(&namespaced_name_buf, "%s%s", get_git_namespace(), name);
+	namespaced_name = strbuf_detach(&namespaced_name_buf, NULL);
+
+	if (!is_null_sha1(new_sha1) && !has_sha1_file(new_sha1)) {
+		error("unpack should have generated %s, "
+		      "but I can't find it!", sha1_to_hex(new_sha1));
+		return "bad pack";
+	}
+
 	if (deny_non_fast_forwards && !is_null_sha1(new_sha1) &&
 	    !is_null_sha1(old_sha1) &&
 	    !prefixcmp(name, "refs/heads/")) {
@@ -692,10 +708,41 @@ static int iterate_receive_command_list(void *cb_data, unsigned char sha1[20])
 	return -1; /* end of list */
 }
 
+static int verify_ref_commands(struct command *commands)
+{
+	unsigned char sha1[20];
+	int commands_ok;
+	struct command *cmd;
+
+	free(head_name_to_free);
+	head_name = head_name_to_free = resolve_refdup("HEAD", sha1, 0, NULL);
+
+	commands_ok = 0;
+	for (cmd = commands; cmd; cmd = cmd->next) {
+		cmd->error_string = verify_ref(cmd);
+		if (!cmd->error_string)
+			commands_ok++;
+	}
+
+	if (verify_refs && !stateless_rpc) {
+		struct strbuf buf = STRBUF_INIT;
+
+		packet_buf_write(&buf, "verify-refs %s\n",
+			 commands_ok > 0 ? "ok" : "no valid refs");
+
+		if (use_sideband)
+			send_sideband(1, 1, buf.buf, buf.len, use_sideband);
+		else
+			safe_write(1, buf.buf, buf.len);
+		strbuf_release(&buf);
+	}
+
+	return commands_ok;
+}
+
 static void execute_commands(struct command *commands, const char *unpacker_error)
 {
 	struct command *cmd;
-	unsigned char sha1[20];
 
 	if (unpacker_error) {
 		for (cmd = commands; cmd; cmd = cmd->next)
@@ -718,9 +765,6 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
 
 	check_aliased_updates(commands);
 
-	free(head_name_to_free);
-	head_name = head_name_to_free = resolve_refdup("HEAD", sha1, 0, NULL);
-
 	for (cmd = commands; cmd; cmd = cmd->next) {
 		if (cmd->error_string)
 			continue;
@@ -766,6 +810,8 @@ static struct command *read_head_info(void)
 				use_sideband = LARGE_PACKET_MAX;
 			if (parse_feature_request(feature_list, "quiet"))
 				quiet = 1;
+			if (parse_feature_request(feature_list, "verify-refs"))
+				verify_refs = 1;
 		}
 		cmd = xcalloc(1, sizeof(struct command) + len - 80);
 		hashcpy(cmd->old_sha1, old_sha1);
@@ -905,7 +951,6 @@ static int delete_only(struct command *commands)
 int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 {
 	int advertise_refs = 0;
-	int stateless_rpc = 0;
 	int i;
 	char *dir = NULL;
 	struct command *commands;
@@ -962,11 +1007,15 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 		return 0;
 
 	if ((commands = read_head_info()) != NULL) {
+		int commands_ok;
 		const char *unpack_status = NULL;
 
-		if (!delete_only(commands))
-			unpack_status = unpack();
-		execute_commands(commands, unpack_status);
+		commands_ok = verify_ref_commands(commands);
+		if (!verify_refs || commands_ok > 0) {
+			if (!delete_only(commands))
+				unpack_status = unpack();
+			execute_commands(commands, unpack_status);
+		}
 		if (pack_lockfile)
 			unlink_or_warn(pack_lockfile);
 		if (report_status)
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 71f258e..7d514ca 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -265,6 +265,8 @@ int send_pack(struct send_pack_args *args,
 		use_sideband = 1;
 	if (!server_supports("quiet"))
 		args->quiet = 0;
+	if (server_supports("verify-refs"))
+		args->verify_refs = 1;
 
 	if (!remote_refs) {
 		fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
@@ -303,12 +305,13 @@ int send_pack(struct send_pack_args *args,
 			char *old_hex = sha1_to_hex(ref->old_sha1);
 			char *new_hex = sha1_to_hex(ref->new_sha1);
 
-			if (!cmds_sent && (status_report || use_sideband || args->quiet)) {
-				packet_buf_write(&req_buf, "%s %s %s%c%s%s%s",
+			if (!cmds_sent && (status_report || use_sideband || args->quiet || args->verify_refs)) {
+				packet_buf_write(&req_buf, "%s %s %s%c%s%s%s%s",
 					old_hex, new_hex, ref->name, 0,
 					status_report ? " report-status" : "",
 					use_sideband ? " side-band-64k" : "",
-					args->quiet ? " quiet" : "");
+					args->quiet ? " quiet" : "",
+					args->verify_refs ? " verify-refs" : "");
 			}
 			else
 				packet_buf_write(&req_buf, "%s %s %s",
@@ -341,17 +344,29 @@ int send_pack(struct send_pack_args *args,
 		in = demux.out;
 	}
 
-	if (new_refs && cmds_sent) {
-		if (pack_objects(out, remote_refs, extra_have, args) < 0) {
-			for (ref = remote_refs; ref; ref = ref->next)
-				ref->status = REF_STATUS_NONE;
-			if (args->stateless_rpc)
-				close(out);
-			if (git_connection_is_socket(conn))
-				shutdown(fd[0], SHUT_WR);
-			if (use_sideband)
-				finish_async(&demux);
-			return -1;
+	if (cmds_sent) {
+		int verify_refs_status = 0;
+
+		if (args->verify_refs && !args->stateless_rpc) {
+			char line[1000];
+			int len = packet_read_line(in, line, sizeof(line));
+			if (len < 15 || memcmp(line, "verify-refs ", 12))
+				return error("did not receive remote status");
+			verify_refs_status = memcmp(line, "verify-refs ok\n", 15);
+		}
+
+		if (!verify_refs_status && new_refs) {
+			if (pack_objects(out, remote_refs, extra_have, args) < 0) {
+				for (ref = remote_refs; ref; ref = ref->next)
+					ref->status = REF_STATUS_NONE;
+				if (args->stateless_rpc)
+					close(out);
+				if (git_connection_is_socket(conn))
+					shutdown(fd[0], SHUT_WR);
+				if (use_sideband)
+					finish_async(&demux);
+				return -1;
+			}
 		}
 	}
 	if (args->stateless_rpc && cmds_sent)
diff --git a/send-pack.h b/send-pack.h
index 05d7ab1..87edaa5 100644
--- a/send-pack.h
+++ b/send-pack.h
@@ -11,7 +11,8 @@ struct send_pack_args {
 		use_thin_pack:1,
 		use_ofs_delta:1,
 		dry_run:1,
-		stateless_rpc:1;
+		stateless_rpc:1,
+		verify_refs:1;
 };
 
 int send_pack(struct send_pack_args *args,
-- 
1.7.9

[PATCH 4/5] t5541: use configured port number

[Clemens Buchacher]

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 t/t5541-http-push.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
index d66ed24..cc6f081 100755
--- a/t/t5541-http-push.sh
+++ b/t/t5541-http-push.sh
@@ -106,7 +106,7 @@ cat >exp <<EOF
 remote: error: hook declined to update refs/heads/dev2
 To http://127.0.0.1:$LIB_HTTPD_PORT/smart/test_repo.git
  ! [remote rejected] dev2 -> dev2 (hook declined)
-error: failed to push some refs to 'http://127.0.0.1:5541/smart/test_repo.git'
+error: failed to push some refs to 'http://127.0.0.1:$LIB_HTTPD_PORT/smart/test_repo.git'
 EOF
 
 test_expect_success 'rejected update prints status' '
-- 
1.7.9

[PATCH 5/5] push/fetch/clone --no-progress suppresses progress output

[Clemens Buchacher]

By default, progress output is disabled if stderr is not a terminal.
The --progress option can be used to force progress output anyways.
Conversely, --no-progress does not force progress output. In particular,
if stderr is a terminal, progress output is enabled.

This is unintuitive. Change --no-progress to force output off.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 builtin/clone.c          |    6 +++---
 builtin/fetch-pack.c     |    2 +-
 builtin/fetch.c          |    4 ++--
 builtin/push.c           |    4 ++--
 builtin/send-pack.c      |   12 +++++++-----
 t/t5523-push-upstream.sh |    3 ++-
 transport.c              |    6 +++++-
 7 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index c62d4b5..09dbb09 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -45,7 +45,7 @@ static char *option_branch = NULL;
 static const char *real_git_dir;
 static char *option_upload_pack = "git-upload-pack";
 static int option_verbosity;
-static int option_progress;
+static int option_progress = -1;
 static struct string_list option_config;
 static struct string_list option_reference;
 
@@ -60,8 +60,8 @@ static int opt_parse_reference(const struct option *opt, const char *arg, int un
 
 static struct option builtin_clone_options[] = {
 	OPT__VERBOSITY(&option_verbosity),
-	OPT_BOOLEAN(0, "progress", &option_progress,
-			"force progress reporting"),
+	OPT_BOOL(0, "progress", &option_progress,
+		 "force progress reporting"),
 	OPT_BOOLEAN('n', "no-checkout", &option_no_checkout,
 		    "don't create a checkout"),
 	OPT_BOOLEAN(0, "bare", &option_bare, "create a bare repository"),
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 6207ecd..a4d3e90 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -736,7 +736,7 @@ static int get_pack(int xd[2], char **pack_lockfile)
 	}
 	else {
 		*av++ = "unpack-objects";
-		if (args.quiet)
+		if (args.quiet || args.no_progress)
 			*av++ = "-q";
 	}
 	if (*hdr_arg)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index ab18633..65f5f9b 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -30,7 +30,7 @@ enum {
 };
 
 static int all, append, dry_run, force, keep, multiple, prune, update_head_ok, verbosity;
-static int progress, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
+static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 static int tags = TAGS_DEFAULT;
 static const char *depth;
 static const char *upload_pack;
@@ -78,7 +78,7 @@ static struct option builtin_fetch_options[] = {
 	OPT_BOOLEAN('k', "keep", &keep, "keep downloaded pack"),
 	OPT_BOOLEAN('u', "update-head-ok", &update_head_ok,
 		    "allow updating of HEAD ref"),
-	OPT_BOOLEAN(0, "progress", &progress, "force progress reporting"),
+	OPT_BOOL(0, "progress", &progress, "force progress reporting"),
 	OPT_STRING(0, "depth", &depth, "depth",
 		   "deepen history of shallow clone"),
 	{ OPTION_STRING, 0, "submodule-prefix", &submodule_prefix, "dir",
diff --git a/builtin/push.c b/builtin/push.c
index 35cce53..6c373cf 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -19,7 +19,7 @@ static int thin;
 static int deleterefs;
 static const char *receivepack;
 static int verbosity;
-static int progress;
+static int progress = -1;
 
 static const char **refspec;
 static int refspec_nr;
@@ -260,7 +260,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		OPT_STRING( 0 , "exec", &receivepack, "receive-pack", "receive pack program"),
 		OPT_BIT('u', "set-upstream", &flags, "set upstream for git pull/status",
 			TRANSPORT_PUSH_SET_UPSTREAM),
-		OPT_BOOLEAN(0, "progress", &progress, "force progress reporting"),
+		OPT_BOOL(0, "progress", &progress, "force progress reporting"),
 		OPT_END()
 	};
 
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 7d514ca..8fc9789 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -58,7 +58,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
 		argv[i++] = "--thin";
 	if (args->use_ofs_delta)
 		argv[i++] = "--delta-base-offset";
-	if (args->quiet)
+	if (args->quiet || !args->progress)
 		argv[i++] = "-q";
 	if (args->progress)
 		argv[i++] = "--progress";
@@ -250,6 +250,7 @@ int send_pack(struct send_pack_args *args,
 	int allow_deleting_refs = 0;
 	int status_report = 0;
 	int use_sideband = 0;
+	int quiet_supported = 0;
 	unsigned cmds_sent = 0;
 	int ret;
 	struct async demux;
@@ -263,8 +264,8 @@ int send_pack(struct send_pack_args *args,
 		args->use_ofs_delta = 1;
 	if (server_supports("side-band-64k"))
 		use_sideband = 1;
-	if (!server_supports("quiet"))
-		args->quiet = 0;
+	if (server_supports("quiet"))
+		quiet_supported = 1;
 	if (server_supports("verify-refs"))
 		args->verify_refs = 1;
 
@@ -304,13 +305,14 @@ int send_pack(struct send_pack_args *args,
 		} else {
 			char *old_hex = sha1_to_hex(ref->old_sha1);
 			char *new_hex = sha1_to_hex(ref->new_sha1);
+			int quiet = quiet_supported && (args->quiet || !args->progress);
 
-			if (!cmds_sent && (status_report || use_sideband || args->quiet || args->verify_refs)) {
+			if (!cmds_sent && (status_report || use_sideband || quiet || args->verify_refs)) {
 				packet_buf_write(&req_buf, "%s %s %s%c%s%s%s%s",
 					old_hex, new_hex, ref->name, 0,
 					status_report ? " report-status" : "",
 					use_sideband ? " side-band-64k" : "",
-					args->quiet ? " quiet" : "",
+					quiet ? " quiet" : "",
 					args->verify_refs ? " verify-refs" : "");
 			}
 			else
diff --git a/t/t5523-push-upstream.sh b/t/t5523-push-upstream.sh
index 9ee52cf..3683df1 100755
--- a/t/t5523-push-upstream.sh
+++ b/t/t5523-push-upstream.sh
@@ -101,10 +101,11 @@ test_expect_success TTY 'push -q suppresses progress' '
 	! grep "Writing objects" err
 '
 
-test_expect_failure TTY 'push --no-progress suppresses progress' '
+test_expect_success TTY 'push --no-progress suppresses progress' '
 	ensure_fresh_upstream &&
 
 	test_terminal git push -u --no-progress upstream master >out 2>err &&
+	! grep "Unpacking objects" err &&
 	! grep "Writing objects" err
 '
 
diff --git a/transport.c b/transport.c
index cac0c06..6074ee9 100644
--- a/transport.c
+++ b/transport.c
@@ -994,10 +994,14 @@ void transport_set_verbosity(struct transport *transport, int verbosity,
 	 * when a rule is satisfied):
 	 *
 	 *   1. Report progress, if force_progress is 1 (ie. --progress).
+	 *   2. Don't report progress, if force_progress is 0 (ie. --no-progress).
 	 *   2. Don't report progress, if verbosity < 0 (ie. -q/--quiet ).
 	 *   3. Report progress if isatty(2) is 1.
 	 **/
-	transport->progress = force_progress || (verbosity >= 0 && isatty(2));
+	if (force_progress >= 0)
+		transport->progress = !!force_progress;
+	else
+		transport->progress = verbosity >= 0 && isatty(2);
 }
 
 int transport_push(struct transport *transport,
-- 
1.7.9

Re: [PATCH 1/5] git rev-list: fix invalid typecast

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> git rev-list passes rev_list_info, not rev_list objects. Without this
> fix, rev-list enables or disables the --verify-objects option depending
> on a read from an undefined memory location.
>
> Signed-off-by: Clemens Buchacher <drizzd@aon.at>
> ---

Wow, I simply couldn't believe it, but you are right.

Thanks.

Re: [PATCH 5/5] push/fetch/clone --no-progress suppresses progress output

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> diff --git a/t/t5523-push-upstream.sh b/t/t5523-push-upstream.sh
> index 9ee52cf..3683df1 100755
> --- a/t/t5523-push-upstream.sh
> +++ b/t/t5523-push-upstream.sh
> @@ -101,10 +101,11 @@ test_expect_success TTY 'push -q suppresses progress' '
>  	! grep "Writing objects" err
>  '
>  
> -test_expect_failure TTY 'push --no-progress suppresses progress' '
> +test_expect_success TTY 'push --no-progress suppresses progress' '
>  	ensure_fresh_upstream &&
>  
>  	test_terminal git push -u --no-progress upstream master >out 2>err &&
> +	! grep "Unpacking objects" err &&
>  	! grep "Writing objects" err
>  '

Very nice to see an old expect-failure turned into expect-success.

> diff --git a/transport.c b/transport.c
> index cac0c06..6074ee9 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -994,10 +994,14 @@ void transport_set_verbosity(struct transport *transport, int verbosity,
>  	 * when a rule is satisfied):
>  	 *
>  	 *   1. Report progress, if force_progress is 1 (ie. --progress).
> +	 *   2. Don't report progress, if force_progress is 0 (ie. --no-progress).
>  	 *   2. Don't report progress, if verbosity < 0 (ie. -q/--quiet ).
>  	 *   3. Report progress if isatty(2) is 1.
>  	 **/

I'll turn this into an unnumbered bulletted list, while I tease it apart
to remove textual dependency on the 'verify-refs' extension, which this
fix shouldn't depend on and taken hostage to.

Thanks.

Re: [PATCH 4/5] t5541: use configured port number

[Junio C Hamano]

Good eyes. Thanks.

Re: [PATCH 2/5] do not override receive-pack errors

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> Receive runs rev-list --verify-objects in order to detect missing
> objects. However, such errors are ignored and overridden later.

This makes me worried (not about the patch, but about the current code).

Are there codepaths where an earlier pass of verify-objects mark a cmd as
bad with a non-NULL error_string, and later code that checks other aspect
of the push says the update does not violate its criteria, and flips the
non-NULL error_string back to NULL?  Or is the only offence you found in
such later code that it fills error_string with its own non-NULL string
when it finds a violation (and otherwise does not touch error_string)?

In other words, is this really "ignored and overridden", not merely
"overwritten"?

In the following review, I assumed that you meant "overwritten".

> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index fa7448b..0afb8b2 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -642,8 +642,10 @@ static void check_aliased_updates(struct command *commands)
>  	}
>  	sort_string_list(&ref_list);
>  
> -	for (cmd = commands; cmd; cmd = cmd->next)
> -		check_aliased_update(cmd, &ref_list);
> +	for (cmd = commands; cmd; cmd = cmd->next) {
> +		if (!cmd->error_string)
> +			check_aliased_update(cmd, &ref_list);
> +	}

While I agree with the general concept of this patch (i.e. if we know an
error exists for a particular ref update, we would want to keep the first
one without overwriting it with another error), I am not sure if this hunk
is correct.  This checks cross reactivity between multiple cmds that can
arise when an update made by one will affect the previous value assumed
for another cmd because the former cmd updates a symref whose the target
is what the later cmd wants to update.  If we have already decided the
former cmd is deemed to fail and skip this check, we would not catch that
the latter cmd is trying to make an inconsistent update request, and we
would end up ignoring that case.  Is that the right thing to do?

> @@ -707,8 +709,10 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
>  		set_connectivity_errors(commands);
>  
>  	if (run_receive_hook(commands, pre_receive_hook, 0)) {
> -		for (cmd = commands; cmd; cmd = cmd->next)
> -			cmd->error_string = "pre-receive hook declined";
> +		for (cmd = commands; cmd; cmd = cmd->next) {
> +			if (!cmd->error_string)
> +				cmd->error_string = "pre-receive hook declined";
> +		}
>  		return;
>  	}
>  
> @@ -717,9 +721,15 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
>  	free(head_name_to_free);
>  	head_name = head_name_to_free = resolve_refdup("HEAD", sha1, 0, NULL);
>  
> -	for (cmd = commands; cmd; cmd = cmd->next)
> -		if (!cmd->skip_update)
> -			cmd->error_string = update(cmd);
> +	for (cmd = commands; cmd; cmd = cmd->next) {
> +		if (cmd->error_string)
> +			continue;
> +
> +		if (cmd->skip_update)
> +			continue;
> +
> +		cmd->error_string = update(cmd);
> +	}
>  }

These two hunks look good.

Re: [PATCH 3/5] git push: verify refs early

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> I suppose with some effort, this could be done for smart HTTP as well.
> But I am not sure if we actually want the overhead of the additional
> ping-pong for HTTP.

Hrm, I am confused.

The updated protocol exchange, if I am reading your patch correctly, would
go like this (S stands for the sender, R for the receiver):

        R: Here are the tips of my refs
        ----
        S: I'd like to update your refs this way
        ----
      + R: No you cannot because all updates will fail, go away
        or
      + R: You may proceed, as some updates may succeed
        ----
        S: Here is the packfile
        ----
        R: Here is how I processed your request

Given that this makes the sender stall for both smart HTTP and native
protocol, don't your worries about the additional ping-pong apply equally
to both transports?

If it is not worth doing for smart HTTP, I wonder if it is worth doing for
native transport.  After all, "all updates will fail" is hopefully the
less likely case, and with this protocol extension, we end up penalizing
the common case with an extra stall for everybody, regardless of the
transport.

I dunno.

All other patches in this "series" looked nice fixes and improvements, but
I am not sure about this change.  At least I am not yet convinced.

>  builtin/receive-pack.c |   83 ++++++++++++++++++++++++++++++++++++++----------
>  builtin/send-pack.c    |   43 +++++++++++++++++--------
>  send-pack.h            |    3 +-
>  3 files changed, 97 insertions(+), 32 deletions(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 0afb8b2..0129d9c 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -34,6 +34,8 @@ static int unpack_limit = 100;
>  static int report_status;
>  static int use_sideband;
>  static int quiet;
> +static int verify_refs;
> +static int stateless_rpc;
>  static int prefer_ofs_delta = 1;
>  static int auto_update_server_info;
>  static int auto_gc = 1;
> @@ -123,7 +125,7 @@ static void show_ref(const char *path, const unsigned char *sha1)
>  	else
>  		packet_write(1, "%s %s%c%s%s\n",
>  			     sha1_to_hex(sha1), path, 0,
> -			     " report-status delete-refs side-band-64k quiet",
> +			     " report-status delete-refs side-band-64k quiet verify-refs",
>  			     prefer_ofs_delta ? " ofs-delta" : "");
>  	sent_capabilities = 1;
>  }
> @@ -410,14 +412,13 @@ static void refuse_unconfigured_deny_delete_current(void)
>  		rp_error("%s", refuse_unconfigured_deny_delete_current_msg[i]);
>  }
>  
> -static const char *update(struct command *cmd)
> +static const char *verify_ref(struct command *cmd)
>  {
>  	const char *name = cmd->ref_name;
>  	struct strbuf namespaced_name_buf = STRBUF_INIT;
>  	const char *namespaced_name;
>  	unsigned char *old_sha1 = cmd->old_sha1;
>  	unsigned char *new_sha1 = cmd->new_sha1;
> -	struct ref_lock *lock;
>  
>  	/* only refs/... are allowed */
>  	if (prefixcmp(name, "refs/") || check_refname_format(name + 5, 0)) {
> @@ -444,12 +445,6 @@ static const char *update(struct command *cmd)
>  		}
>  	}
>  
> -	if (!is_null_sha1(new_sha1) && !has_sha1_file(new_sha1)) {
> -		error("unpack should have generated %s, "
> -		      "but I can't find it!", sha1_to_hex(new_sha1));
> -		return "bad pack";
> -	}
> -
>  	if (!is_null_sha1(old_sha1) && is_null_sha1(new_sha1)) {
>  		if (deny_deletes && !prefixcmp(name, "refs/heads/")) {
>  			rp_error("denying ref deletion for %s", name);
> @@ -473,6 +468,27 @@ static const char *update(struct command *cmd)
>  		}
>  	}
>  
> +	return NULL;
> +}
> +
> +static const char *update(struct command *cmd)
> +{
> +	const char *name = cmd->ref_name;
> +	struct strbuf namespaced_name_buf = STRBUF_INIT;
> +	const char *namespaced_name;
> +	unsigned char *old_sha1 = cmd->old_sha1;
> +	unsigned char *new_sha1 = cmd->new_sha1;
> +	struct ref_lock *lock;
> +
> +	strbuf_addf(&namespaced_name_buf, "%s%s", get_git_namespace(), name);
> +	namespaced_name = strbuf_detach(&namespaced_name_buf, NULL);
> +
> +	if (!is_null_sha1(new_sha1) && !has_sha1_file(new_sha1)) {
> +		error("unpack should have generated %s, "
> +		      "but I can't find it!", sha1_to_hex(new_sha1));
> +		return "bad pack";
> +	}
> +
>  	if (deny_non_fast_forwards && !is_null_sha1(new_sha1) &&
>  	    !is_null_sha1(old_sha1) &&
>  	    !prefixcmp(name, "refs/heads/")) {
> @@ -692,10 +708,41 @@ static int iterate_receive_command_list(void *cb_data, unsigned char sha1[20])
>  	return -1; /* end of list */
>  }
>  
> +static int verify_ref_commands(struct command *commands)
> +{
> +	unsigned char sha1[20];
> +	int commands_ok;
> +	struct command *cmd;
> +
> +	free(head_name_to_free);
> +	head_name = head_name_to_free = resolve_refdup("HEAD", sha1, 0, NULL);
> +
> +	commands_ok = 0;
> +	for (cmd = commands; cmd; cmd = cmd->next) {
> +		cmd->error_string = verify_ref(cmd);
> +		if (!cmd->error_string)
> +			commands_ok++;
> +	}
> +
> +	if (verify_refs && !stateless_rpc) {
> +		struct strbuf buf = STRBUF_INIT;
> +
> +		packet_buf_write(&buf, "verify-refs %s\n",
> +			 commands_ok > 0 ? "ok" : "no valid refs");
> +
> +		if (use_sideband)
> +			send_sideband(1, 1, buf.buf, buf.len, use_sideband);
> +		else
> +			safe_write(1, buf.buf, buf.len);
> +		strbuf_release(&buf);
> +	}
> +
> +	return commands_ok;
> +}
> +
>  static void execute_commands(struct command *commands, const char *unpacker_error)
>  {
>  	struct command *cmd;
> -	unsigned char sha1[20];
>  
>  	if (unpacker_error) {
>  		for (cmd = commands; cmd; cmd = cmd->next)
> @@ -718,9 +765,6 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
>  
>  	check_aliased_updates(commands);
>  
> -	free(head_name_to_free);
> -	head_name = head_name_to_free = resolve_refdup("HEAD", sha1, 0, NULL);
> -
>  	for (cmd = commands; cmd; cmd = cmd->next) {
>  		if (cmd->error_string)
>  			continue;
> @@ -766,6 +810,8 @@ static struct command *read_head_info(void)
>  				use_sideband = LARGE_PACKET_MAX;
>  			if (parse_feature_request(feature_list, "quiet"))
>  				quiet = 1;
> +			if (parse_feature_request(feature_list, "verify-refs"))
> +				verify_refs = 1;
>  		}
>  		cmd = xcalloc(1, sizeof(struct command) + len - 80);
>  		hashcpy(cmd->old_sha1, old_sha1);
> @@ -905,7 +951,6 @@ static int delete_only(struct command *commands)
>  int cmd_receive_pack(int argc, const char **argv, const char *prefix)
>  {
>  	int advertise_refs = 0;
> -	int stateless_rpc = 0;
>  	int i;
>  	char *dir = NULL;
>  	struct command *commands;
> @@ -962,11 +1007,15 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
>  		return 0;
>  
>  	if ((commands = read_head_info()) != NULL) {
> +		int commands_ok;
>  		const char *unpack_status = NULL;
>  
> -		if (!delete_only(commands))
> -			unpack_status = unpack();
> -		execute_commands(commands, unpack_status);
> +		commands_ok = verify_ref_commands(commands);
> +		if (!verify_refs || commands_ok > 0) {
> +			if (!delete_only(commands))
> +				unpack_status = unpack();
> +			execute_commands(commands, unpack_status);
> +		}
>  		if (pack_lockfile)
>  			unlink_or_warn(pack_lockfile);
>  		if (report_status)
> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> index 71f258e..7d514ca 100644
> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c
> @@ -265,6 +265,8 @@ int send_pack(struct send_pack_args *args,
>  		use_sideband = 1;
>  	if (!server_supports("quiet"))
>  		args->quiet = 0;
> +	if (server_supports("verify-refs"))
> +		args->verify_refs = 1;
>  
>  	if (!remote_refs) {
>  		fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
> @@ -303,12 +305,13 @@ int send_pack(struct send_pack_args *args,
>  			char *old_hex = sha1_to_hex(ref->old_sha1);
>  			char *new_hex = sha1_to_hex(ref->new_sha1);
>  
> -			if (!cmds_sent && (status_report || use_sideband || args->quiet)) {
> -				packet_buf_write(&req_buf, "%s %s %s%c%s%s%s",
> +			if (!cmds_sent && (status_report || use_sideband || args->quiet || args->verify_refs)) {
> +				packet_buf_write(&req_buf, "%s %s %s%c%s%s%s%s",
>  					old_hex, new_hex, ref->name, 0,
>  					status_report ? " report-status" : "",
>  					use_sideband ? " side-band-64k" : "",
> -					args->quiet ? " quiet" : "");
> +					args->quiet ? " quiet" : "",
> +					args->verify_refs ? " verify-refs" : "");
>  			}
>  			else
>  				packet_buf_write(&req_buf, "%s %s %s",
> @@ -341,17 +344,29 @@ int send_pack(struct send_pack_args *args,
>  		in = demux.out;
>  	}
>  
> -	if (new_refs && cmds_sent) {
> -		if (pack_objects(out, remote_refs, extra_have, args) < 0) {
> -			for (ref = remote_refs; ref; ref = ref->next)
> -				ref->status = REF_STATUS_NONE;
> -			if (args->stateless_rpc)
> -				close(out);
> -			if (git_connection_is_socket(conn))
> -				shutdown(fd[0], SHUT_WR);
> -			if (use_sideband)
> -				finish_async(&demux);
> -			return -1;
> +	if (cmds_sent) {
> +		int verify_refs_status = 0;
> +
> +		if (args->verify_refs && !args->stateless_rpc) {
> +			char line[1000];
> +			int len = packet_read_line(in, line, sizeof(line));
> +			if (len < 15 || memcmp(line, "verify-refs ", 12))
> +				return error("did not receive remote status");
> +			verify_refs_status = memcmp(line, "verify-refs ok\n", 15);
> +		}
> +
> +		if (!verify_refs_status && new_refs) {
> +			if (pack_objects(out, remote_refs, extra_have, args) < 0) {
> +				for (ref = remote_refs; ref; ref = ref->next)
> +					ref->status = REF_STATUS_NONE;
> +				if (args->stateless_rpc)
> +					close(out);
> +				if (git_connection_is_socket(conn))
> +					shutdown(fd[0], SHUT_WR);
> +				if (use_sideband)
> +					finish_async(&demux);
> +				return -1;
> +			}
>  		}
>  	}
>  	if (args->stateless_rpc && cmds_sent)
> diff --git a/send-pack.h b/send-pack.h
> index 05d7ab1..87edaa5 100644
> --- a/send-pack.h
> +++ b/send-pack.h
> @@ -11,7 +11,8 @@ struct send_pack_args {
>  		use_thin_pack:1,
>  		use_ofs_delta:1,
>  		dry_run:1,
> -		stateless_rpc:1;
> +		stateless_rpc:1,
> +		verify_refs:1;
>  };
>  
>  int send_pack(struct send_pack_args *args,

Re: [PATCH 2/5] do not override receive-pack errors

[Clemens Buchacher]

On Mon, Feb 13, 2012 at 01:41:38PM -0800, Junio C Hamano wrote:
> Clemens Buchacher <drizzd@aon.at> writes:
> 
> > Receive runs rev-list --verify-objects in order to detect missing
> > objects. However, such errors are ignored and overridden later.
> 
> This makes me worried (not about the patch, but about the current code).
> 
> Are there codepaths where an earlier pass of verify-objects mark a cmd as
> bad with a non-NULL error_string, and later code that checks other aspect
> of the push says the update does not violate its criteria, and flips the
> non-NULL error_string back to NULL?  Or is the only offence you found in
> such later code that it fills error_string with its own non-NULL string
> when it finds a violation (and otherwise does not touch error_string)?
> 
> In other words, is this really "ignored and overridden", not merely
> "overwritten"?

Yes, it really is. For example, in t5504 rev-list --verify-objects (it
was turned on for me if called from there) detects the corrupt object.
But the error string is later overwritten with the return value of
update, which is NULL in this case.

That is why I had to change the t5504 tests from a successful git push
to a test_must_fail git push with this fix. To keep the previous
behavior we would have to replace the corrupt blob with a more subtle
corruption that rev-list --verify-objects would not detect, but fsck
would (e.g., a malformed commit header).

> > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> > index fa7448b..0afb8b2 100644
> > --- a/builtin/receive-pack.c
> > +++ b/builtin/receive-pack.c
> > @@ -642,8 +642,10 @@ static void check_aliased_updates(struct command *commands)
> >  	}
> >  	sort_string_list(&ref_list);
> >  
> > -	for (cmd = commands; cmd; cmd = cmd->next)
> > -		check_aliased_update(cmd, &ref_list);
> > +	for (cmd = commands; cmd; cmd = cmd->next) {
> > +		if (!cmd->error_string)
> > +			check_aliased_update(cmd, &ref_list);
> > +	}
> 
[...]
> If we have already decided the former cmd is deemed to fail and skip
> this check, we would not catch that the latter cmd is trying to make
> an inconsistent update request, and we would end up ignoring that
> case.

Actually, check_alias_update searches for aliases of cmd in ref_list,
which is a list of refs from all commands, irrespective of their error
status. So this change is correct.

However, after re-reading the code I now have the impression that the
alias detection is not entirely correct. It does find aliases between
symrefs and regular refs.  But it does not find aliases between two
symrefs, because ref_list will not contain the actual ref pointed to,
and therefore the code considers neither symref an alias.

But that is independent of the hunk above.

Re: [PATCH 3/5] git push: verify refs early

[Clemens Buchacher]

On Mon, Feb 13, 2012 at 02:16:13PM -0800, Junio C Hamano wrote:
> Clemens Buchacher <drizzd@aon.at> writes:
> 
> > I suppose with some effort, this could be done for smart HTTP as well.
> > But I am not sure if we actually want the overhead of the additional
> > ping-pong for HTTP.
> 
> Hrm, I am confused.
> 
> The updated protocol exchange, if I am reading your patch correctly, would
> go like this (S stands for the sender, R for the receiver):
> 
>         R: Here are the tips of my refs
>         ----
>         S: I'd like to update your refs this way
>         ----
>       + R: No you cannot because all updates will fail, go away
>         or
>       + R: You may proceed, as some updates may succeed
>         ----
>         S: Here is the packfile
>         ----
>         R: Here is how I processed your request
> 
> Given that this makes the sender stall for both smart HTTP and native
> protocol, don't your worries about the additional ping-pong apply equally
> to both transports?

That is true. However, my assumption was that the overhead is greater
for HTTP, because the native protocol is full-duplex, while HTTP tears
down the connection and starts from scratch with each request. But to be
honest, I am not confident that this assumption is correct.

So, the stall might be an issue for both the native and the HTTP
protocol, or for neither. We should probably find out and then decide
whether to make this change for both protocols or not at all.

> If it is not worth doing for smart HTTP, I wonder if it is worth doing for
> native transport.  After all, "all updates will fail" is hopefully the
> less likely case, and with this protocol extension, we end up penalizing
> the common case with an extra stall for everybody, regardless of the
> transport.

Indeed. I wish we could make the ref validation asynchronous. The client
would start sending object data right away, while listening for an
"abort" command on the side-band. But if I understand correctly, that is
not possible for HTTP.

Re: [PATCH 2/5] do not override receive-pack errors

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> Yes, it really is. For example, in t5504 rev-list --verify-objects (it
> was turned on for me if called from there) detects the corrupt object.
> But the error string is later overwritten with the return value of
> update, which is NULL in this case.
> ...
> Actually, check_alias_update searches for aliases of cmd in ref_list,
> which is a list of refs from all commands, irrespective of their error
> status. So this change is correct.

Ok, thanks for clarificatin on both counts.