Re: [PATCH 06/17] Let fetch_pack() inform caller about number of unique heads
On 08/23/2012 10:54 AM, Jeff King wrote: > On Thu, Aug 23, 2012 at 10:10:31AM +0200, mhag...@alum.mit.edu wrote: > >> From: Michael Haggerty >> >> fetch_pack() remotes duplicates from the list (nr_heads, heads), >> thereby shrinking the list. But previously, the caller was not >> informed about the shrinkage. This would cause a spurious error >> message to be emitted by cmd_fetch_pack() if "git fetch-pack" is >> called with duplicate refnames. >> >> So change the signature of fetch_pack() to accept nr_heads by >> reference, and if any duplicates were removed then modify it to >> reflect the number of remaining references. >> >> The last test of t5500 inexplicably *required* "git fetch-pack" to >> fail when fetching a list of references that contains duplicates; >> i.e., it insisted on the buggy behavior. So change the test to expect >> the correct behavior. > > Eek, yeah, the current behavior is obviously wrong. The > remove_duplicates code comes from 310b86d (fetch-pack: do not barf when > duplicate re patterns are given, 2006-11-25) and clearly meant for > fetch-pack to handle this case gracefully. > >> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh >> index 3cc3346..0d4edcb 100755 >> --- a/t/t5500-fetch-pack.sh >> +++ b/t/t5500-fetch-pack.sh >> @@ -391,7 +391,7 @@ test_expect_success 'fetch mixed refs from cmdline and >> stdin' ' >> test_expect_success 'test duplicate refs from stdin' ' >> ( >> cd client && >> -test_must_fail git fetch-pack --stdin --no-progress .. <../input.dup >> +git fetch-pack --stdin --no-progress .. <../input.dup >> ) >output && >> cut -d " " -f 2 actual && >> test_cmp expect actual > > It's interesting that the output was the same before and after the fix. > I guess that is because the error comes at the very end, when we are > making sure all of the provided heads have been consumed. "git fetch-pack" emits information about successfully-received references regardless of whether some requested references were not received. The "no such remote ref %s" output goes to stderr. So the only difference between before/after fix should be what is written to stderr, whereas the test only looks at stdout. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/17] Let fetch_pack() inform caller about number of unique heads
On Thu, Aug 23, 2012 at 10:10:31AM +0200, mhag...@alum.mit.edu wrote: > From: Michael Haggerty > > fetch_pack() remotes duplicates from the list (nr_heads, heads), > thereby shrinking the list. But previously, the caller was not > informed about the shrinkage. This would cause a spurious error > message to be emitted by cmd_fetch_pack() if "git fetch-pack" is > called with duplicate refnames. > > So change the signature of fetch_pack() to accept nr_heads by > reference, and if any duplicates were removed then modify it to > reflect the number of remaining references. > > The last test of t5500 inexplicably *required* "git fetch-pack" to > fail when fetching a list of references that contains duplicates; > i.e., it insisted on the buggy behavior. So change the test to expect > the correct behavior. Eek, yeah, the current behavior is obviously wrong. The remove_duplicates code comes from 310b86d (fetch-pack: do not barf when duplicate re patterns are given, 2006-11-25) and clearly meant for fetch-pack to handle this case gracefully. > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh > index 3cc3346..0d4edcb 100755 > --- a/t/t5500-fetch-pack.sh > +++ b/t/t5500-fetch-pack.sh > @@ -391,7 +391,7 @@ test_expect_success 'fetch mixed refs from cmdline and > stdin' ' > test_expect_success 'test duplicate refs from stdin' ' > ( > cd client && > - test_must_fail git fetch-pack --stdin --no-progress .. <../input.dup > + git fetch-pack --stdin --no-progress .. <../input.dup > ) >output && > cut -d " " -f 2 actual && > test_cmp expect actual It's interesting that the output was the same before and after the fix. I guess that is because the error comes at the very end, when we are making sure all of the provided heads have been consumed. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 06/17] Let fetch_pack() inform caller about number of unique heads
From: Michael Haggerty
fetch_pack() remotes duplicates from the list (nr_heads, heads),
thereby shrinking the list. But previously, the caller was not
informed about the shrinkage. This would cause a spurious error
message to be emitted by cmd_fetch_pack() if "git fetch-pack" is
called with duplicate refnames.
So change the signature of fetch_pack() to accept nr_heads by
reference, and if any duplicates were removed then modify it to
reflect the number of remaining references.
The last test of t5500 inexplicably *required* "git fetch-pack" to
fail when fetching a list of references that contains duplicates;
i.e., it insisted on the buggy behavior. So change the test to expect
the correct behavior.
Signed-off-by: Michael Haggerty
---
builtin/fetch-pack.c | 12 ++--
fetch-pack.h | 2 +-
t/t5500-fetch-pack.sh | 2 +-
transport.c | 2 +-
4 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index a6406e7..3c93ec4 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -1021,7 +1021,7 @@ int cmd_fetch_pack(int argc, const char **argv, const
char *prefix)
get_remote_heads(fd[0], &ref, 0, NULL);
ref = fetch_pack(&args, fd, conn, ref, dest,
- nr_heads, heads, pack_lockfile_ptr);
+ &nr_heads, heads, pack_lockfile_ptr);
if (pack_lockfile) {
printf("lock %s\n", pack_lockfile);
fflush(stdout);
@@ -1062,7 +1062,7 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
int fd[], struct child_process *conn,
const struct ref *ref,
const char *dest,
- int nr_heads,
+ int *nr_heads,
char **heads,
char **pack_lockfile)
{
@@ -1077,16 +1077,16 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
st.st_mtime = 0;
}
- if (heads && nr_heads) {
- qsort(heads, nr_heads, sizeof(*heads), compare_heads);
- nr_heads = remove_duplicates(nr_heads, heads);
+ if (heads && *nr_heads) {
+ qsort(heads, *nr_heads, sizeof(*heads), compare_heads);
+ *nr_heads = remove_duplicates(*nr_heads, heads);
}
if (!ref) {
packet_flush(fd[1]);
die("no matching remote head");
}
- ref_cpy = do_fetch_pack(fd, ref, nr_heads, heads, pack_lockfile);
+ ref_cpy = do_fetch_pack(fd, ref, *nr_heads, heads, pack_lockfile);
if (args.depth > 0) {
struct cache_time mtime;
diff --git a/fetch-pack.h b/fetch-pack.h
index 1dbe90f..a9d505b 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -21,7 +21,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
int fd[], struct child_process *conn,
const struct ref *ref,
const char *dest,
- int nr_heads,
+ int *nr_heads,
char **heads,
char **pack_lockfile);
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 3cc3346..0d4edcb 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -391,7 +391,7 @@ test_expect_success 'fetch mixed refs from cmdline and
stdin' '
test_expect_success 'test duplicate refs from stdin' '
(
cd client &&
- test_must_fail git fetch-pack --stdin --no-progress .. <../input.dup
+ git fetch-pack --stdin --no-progress .. <../input.dup
) >output &&
cut -d " " -f 2 actual &&
test_cmp expect actual
diff --git a/transport.c b/transport.c
index 1811b50..f7bbf58 100644
--- a/transport.c
+++ b/transport.c
@@ -548,7 +548,7 @@ static int fetch_refs_via_pack(struct transport *transport,
refs = fetch_pack(&args, data->fd, data->conn,
refs_tmp ? refs_tmp : transport->remote_refs,
- dest, nr_heads, heads, &transport->pack_lockfile);
+ dest, &nr_heads, heads, &transport->pack_lockfile);
close(data->fd[0]);
close(data->fd[1]);
if (finish_connect(data->conn))
--
1.7.11.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
