* [PATCH 1/5] git rev-list: fix invalid typecast
2012-02-13 20:17 ` [PATCH 0/5] verify refs early Clemens Buchacher
@ 2012-02-13 20:17 ` Clemens Buchacher
2012-02-13 20:48 ` Junio C Hamano
2012-02-13 20:17 ` [PATCH 2/5] do not override receive-pack errors Clemens Buchacher
` (3 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Clemens Buchacher @ 2012-02-13 20:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
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
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 2/5] do not override receive-pack errors
2012-02-13 20:17 ` [PATCH 0/5] verify refs early Clemens Buchacher
2012-02-13 20:17 ` [PATCH 1/5] git rev-list: fix invalid typecast Clemens Buchacher
@ 2012-02-13 20:17 ` Clemens Buchacher
2012-02-13 21:41 ` Junio C Hamano
2012-02-13 20:17 ` [PATCH 3/5] git push: verify refs early Clemens Buchacher
` (2 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Clemens Buchacher @ 2012-02-13 20:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
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
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 2/5] do not override receive-pack errors
2012-02-13 20:17 ` [PATCH 2/5] do not override receive-pack errors Clemens Buchacher
@ 2012-02-13 21:41 ` Junio C Hamano
2012-02-14 8:33 ` Clemens Buchacher
0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2012-02-13 21:41 UTC (permalink / raw)
To: Clemens Buchacher; +Cc: git
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.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/5] do not override receive-pack errors
2012-02-13 21:41 ` Junio C Hamano
@ 2012-02-14 8:33 ` Clemens Buchacher
2012-02-14 19:06 ` Junio C Hamano
0 siblings, 1 reply; 16+ messages in thread
From: Clemens Buchacher @ 2012-02-14 8:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
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.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/5] do not override receive-pack errors
2012-02-14 8:33 ` Clemens Buchacher
@ 2012-02-14 19:06 ` Junio C Hamano
0 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2012-02-14 19:06 UTC (permalink / raw)
To: Clemens Buchacher; +Cc: git
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/5] git push: verify refs early
2012-02-13 20:17 ` [PATCH 0/5] verify refs early Clemens Buchacher
2012-02-13 20:17 ` [PATCH 1/5] git rev-list: fix invalid typecast Clemens Buchacher
2012-02-13 20:17 ` [PATCH 2/5] do not override receive-pack errors Clemens Buchacher
@ 2012-02-13 20:17 ` Clemens Buchacher
2012-02-13 22:16 ` Junio C Hamano
2012-02-13 20:17 ` [PATCH 4/5] t5541: use configured port number Clemens Buchacher
2012-02-13 20:17 ` [PATCH 5/5] push/fetch/clone --no-progress suppresses progress output Clemens Buchacher
4 siblings, 1 reply; 16+ messages in thread
From: Clemens Buchacher @ 2012-02-13 20:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
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
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 3/5] git push: verify refs early
2012-02-13 20:17 ` [PATCH 3/5] git push: verify refs early Clemens Buchacher
@ 2012-02-13 22:16 ` Junio C Hamano
2012-02-14 8:59 ` Clemens Buchacher
0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2012-02-13 22:16 UTC (permalink / raw)
To: Clemens Buchacher; +Cc: git, spearce
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,
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 3/5] git push: verify refs early
2012-02-13 22:16 ` Junio C Hamano
@ 2012-02-14 8:59 ` Clemens Buchacher
0 siblings, 0 replies; 16+ messages in thread
From: Clemens Buchacher @ 2012-02-14 8:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, spearce
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 4/5] t5541: use configured port number
2012-02-13 20:17 ` [PATCH 0/5] verify refs early Clemens Buchacher
` (2 preceding siblings ...)
2012-02-13 20:17 ` [PATCH 3/5] git push: verify refs early Clemens Buchacher
@ 2012-02-13 20:17 ` Clemens Buchacher
2012-02-13 21:23 ` Junio C Hamano
2012-02-13 20:17 ` [PATCH 5/5] push/fetch/clone --no-progress suppresses progress output Clemens Buchacher
4 siblings, 1 reply; 16+ messages in thread
From: Clemens Buchacher @ 2012-02-13 20:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
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
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 5/5] push/fetch/clone --no-progress suppresses progress output
2012-02-13 20:17 ` [PATCH 0/5] verify refs early Clemens Buchacher
` (3 preceding siblings ...)
2012-02-13 20:17 ` [PATCH 4/5] t5541: use configured port number Clemens Buchacher
@ 2012-02-13 20:17 ` Clemens Buchacher
2012-02-13 21:13 ` Junio C Hamano
4 siblings, 1 reply; 16+ messages in thread
From: Clemens Buchacher @ 2012-02-13 20:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
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
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 5/5] push/fetch/clone --no-progress suppresses progress output
2012-02-13 20:17 ` [PATCH 5/5] push/fetch/clone --no-progress suppresses progress output Clemens Buchacher
@ 2012-02-13 21:13 ` Junio C Hamano
0 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2012-02-13 21:13 UTC (permalink / raw)
To: Clemens Buchacher; +Cc: git
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.
^ permalink raw reply [flat|nested] 16+ messages in thread