optionally deny all pushes

PATCH In review Clemens Buchacher 2 participants 16 messages Jun 15, 2016 Documentation

Share & tools

Clemens Buchacher

author

2016-06-15 22:52 UTC #0 lore raw

PATCH 1/1 optionally deny all pushes

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 <[email protected]>

---
 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(-)

Documentation/config.txt +4 -0

index 04f5e19..edbddea 100644
@@ -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.

Documentation/git-push.txt +1 -0

index aede488..7c6cc63 100644
@@ -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

builtin/receive-pack.c +11 -0

index d2dcb7e..9cd04f9 100644
@@ -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);

contrib/completion/git-completion.bash +1 -0

index b0062ba..9d63622 100755
@@ -2250,6 +2250,7 @@ _git_config ()
 		rebase.autosquash
 		rebase.stat
 		receive.autogc
+		receive.denyAll
 		receive.denyCurrentBranch
 		receive.denyDeleteCurrent
 		receive.denyDeletes

t/t5400-send-pack.sh +17 -0

index 0eace37..e8c9be2 100755
@@ -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

t/t5516-fetch-push.sh +12 -0

index b69cf57..d1117c0 100755
@@ -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 &&
 	(

Signature · 1 line

1.7.8

Junio C Hamano

2016-06-15 22:52 UTC #1 lore raw

Quoted 10 lines · 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 <[email protected]>
---

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?

Clemens Buchacher

author

2016-06-15 22:53 UTC #2 lore raw

cover verify refs early

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

Clemens Buchacher

author

2016-06-15 22:53 UTC #3 lore raw

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

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 <[email protected]>

---
 builtin/rev-list.c |    4 ++--
 t/t1450-fsck.sh    |   26 ++++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 2 deletions(-)

builtin/rev-list.c +2 -2

index ab3be7c..264e3ae 100644
@@ -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);
 }

t/t1450-fsck.sh +26 -0

index 523ce9c..5b8ebd8 100755
@@ -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

Signature · 1 line

1.7.9

Junio C Hamano

2016-06-15 22:53 UTC #4 lore raw

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

Quoted 6 lines · 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 <[email protected]>
---

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

Thanks.

Clemens Buchacher

author

2016-06-15 22:53 UTC #5 lore raw

PATCH 2/5 do not override receive-pack errors

In reply to 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 <[email protected]>

---
 builtin/receive-pack.c          |   24 +++++++++++++++++-------
 t/t5504-fetch-receive-strict.sh |   22 ++++++++++++++++++----
 2 files changed, 35 insertions(+), 11 deletions(-)

builtin/receive-pack.c +17 -7

index fa7448b..0afb8b2 100644
@@ -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)

t/t5504-fetch-receive-strict.sh +18 -4

index 8341fc4..35ec294 100755
@@ -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

Signature · 1 line

1.7.9

Junio C Hamano

2016-06-15 22:53 UTC #6 lore raw

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

Quoted 2 lines · Clemens Buchacher

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".

Quoted 5 lines

builtin/receive-pack.c +0 -0
index fa7448b..0afb8b2 100644

builtin/receive-pack.c static void check_aliased_updates(struct command *commands) · line 648

Quoted 9 lines

builtin/receive-pack.c +4 -2

@@ -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?

Quoted 20 lines

builtin/receive-pack.c +4 -5

@@ -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);

builtin/receive-pack.c static void execute_commands(struct command *commands, const char *unpacker_erro · line 733

Quoted 10 lines

builtin/receive-pack.c +9 -0

+	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.

Clemens Buchacher

author

2016-06-15 22:53 UTC #7 lore raw

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

Quoted 14 lines · Junio C Hamano

Quoted 2 lines · Clemens Buchacher
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)?

Quoted 2 lines

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).

Quoted 15 lines

Quoted 14 lines

builtin/receive-pack.c +4 -2

index fa7448b..0afb8b2 100644
@@ -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);
+	}

[...]

Quoted 4 lines

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.

Junio C Hamano

2016-06-15 22:53 UTC #8 lore raw

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

Quoted 8 lines · Clemens Buchacher

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.

Clemens Buchacher

author

2016-06-15 22:53 UTC #9 lore raw

PATCH 3/5 git push: verify refs early

In reply to 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 <[email protected]>

---

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(-)

builtin/receive-pack.c +66 ~~-17~~

index 0afb8b2..0129d9c 100644
@@ -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)

builtin/send-pack.c +29 ~~-14~~

64 lines not on this page · raw message

send-pack.h +2 -1

index 05d7ab1..87edaa5 100644
@@ -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,

Signature · 1 line

1.7.9

Junio C Hamano

2016-06-15 22:53 UTC #10 lore raw

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

Quoted 3 lines · Clemens Buchacher

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.

Quoted 258 lines

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

builtin/receive-pack.c +66 ~~-17~~

index 0afb8b2..0129d9c 100644
@@ -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)

builtin/send-pack.c +29 ~~-14~~

index 71f258e..7d514ca 100644
@@ -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)

send-pack.h +2 -1

index 05d7ab1..87edaa5 100644
@@ -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,

Clemens Buchacher

author

2016-06-15 22:53 UTC #11 lore raw

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

Quoted 23 lines · Junio C Hamano

Quoted 3 lines · Clemens Buchacher
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

Quoted 3 lines

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.

Quoted 5 lines

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.

Clemens Buchacher

author

2016-06-15 22:53 UTC #12 lore raw

PATCH 4/5 t5541: use configured port number

In reply to Clemens Buchacher

Signed-off-by Clemens Buchacher <[email protected]>

---
 t/t5541-http-push.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

t/t5541-http-push.sh +1 -1

index d66ed24..cc6f081 100755
@@ -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' '

Signature · 1 line

1.7.9

Junio C Hamano

2016-06-15 22:53 UTC #13 lore raw

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

Good eyes. Thanks.

Clemens Buchacher

author

2016-06-15 22:53 UTC #14 lore raw

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

In reply to 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 <[email protected]>

---
 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(-)

builtin/clone.c +3 -3

index c62d4b5..09dbb09 100644
@@ -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"),

builtin/fetch-pack.c +1 -1

index 6207ecd..a4d3e90 100644
@@ -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)

builtin/fetch.c +2 -2

index ab18633..65f5f9b 100644
@@ -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",

builtin/push.c +2 -2

index 35cce53..6c373cf 100644
@@ -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()
 	};

builtin/send-pack.c +7 -5

index 7d514ca..8fc9789 100644
@@ -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

t/t5523-push-upstream.sh +2 -1

index 9ee52cf..3683df1 100755
@@ -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
 '

transport.c +5 -1

index cac0c06..6074ee9 100644
@@ -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,

Signature · 1 line

1.7.9

Junio C Hamano

2016-06-15 22:53 UTC #15 lore raw

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

Quoted 6 lines · Clemens Buchacher

t/t5523-push-upstream.sh +0 -0

index 9ee52cf..3683df1 100755
@@ -101,10 +101,11 @@ test_expect_success TTY 'push -q suppresses progress' '
 	! grep "Writing objects" err

t/t5523-push-upstream.sh test_expect_success TTY 'push -q suppresses progress' ' · line 110

Quoted 10 lines

t/t5523-push-upstream.sh +2 -1

 '
 
-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.

transport.c void transport_set_verbosity(struct transport *transport, int verbosity, · line 1000

Quoted 12 lines

transport.c +1 -0

index cac0c06..6074ee9 100644
@@ -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.