From: Michael Haggerty <redacted>
Re-roll, incorporating Jeff's suggestions. Some commit messages have
also been improved, but the only interdiff is that match_pos is
renamed to head_pos in filter_refs().
This patch series applies to the merge between master and
jc/maint-push-refs-all, though the dependency on the latter is only
textual.
Michael Haggerty (17):
t5500: add tests of error output for missing refs
Rename static function fetch_pack() to http_fetch_pack()
Fix formatting.
Name local variables more consistently
Do not check the same head_pos twice
Let fetch_pack() inform caller about number of unique heads
Pass nr_heads to do_pack_ref() by reference
Pass nr_heads to everything_local() by reference
Pass nr_heads to filter_refs() by reference
Remove ineffective optimization
filter_refs(): do not leave gaps in return_refs
filter_refs(): compress unmatched refs in heads array
cmd_fetch_pack: return early if finish_connect() returns an error
Report missing refs even if no existing refs were received
cmd_fetch_pack(): simplify computation of return value
fetch_pack(): free matching heads
filter_refs(): simplify logic
builtin/fetch-pack.c | 128 ++++++++++++++++++++------------------------------
fetch-pack.h | 19 +++++---
http-walker.c | 4 +-
t/t5500-fetch-pack.sh | 32 ++++++++++++-
transport.c | 8 ++--
5 files changed, 101 insertions(+), 90 deletions(-)
--
1.7.11.3
From: Michael Haggerty <redacted>
If "git fetch-pack" is called with reference names that do not exist
on the remote, then it should emit an error message
error: no such remote ref refs/heads/xyzzy
This is currently broken if *only* missing references are passed to
"git fetch-pack".
Signed-off-by: Michael Haggerty <redacted>
---
t/t5500-fetch-pack.sh | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
From: Michael Haggerty <redacted>
Avoid confusion with the non-static function of the same name from
fetch-pack.h.
Signed-off-by: Michael Haggerty <redacted>
---
http-walker.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Michael Haggerty <redacted>
Use the names (nr_heads, heads) consistently across functions, instead
of sometimes naming the same values (nr_match, match).
Signed-off-by: Michael Haggerty <redacted>
---
builtin/fetch-pack.c | 35 +++++++++++++++++------------------
1 file changed, 17 insertions(+), 18 deletions(-)
@@ -521,27 +521,27 @@ static void mark_recent_complete_commits(unsigned long cutoff)}}-staticvoidfilter_refs(structref**refs,intnr_match,char**match)+staticvoidfilter_refs(structref**refs,intnr_heads,char**heads){structref**return_refs;structref*newlist=NULL;structref**newtail=&newlist;structref*ref,*next;structref*fastarray[32];-intmatch_pos;+inthead_pos;-if(nr_match&&!args.fetch_all){-if(ARRAY_SIZE(fastarray)<nr_match)-return_refs=xcalloc(nr_match,sizeof(structref*));+if(nr_heads&&!args.fetch_all){+if(ARRAY_SIZE(fastarray)<nr_heads)+return_refs=xcalloc(nr_heads,sizeof(structref*));else{return_refs=fastarray;-memset(return_refs,0,sizeof(structref*)*nr_match);+memset(return_refs,0,sizeof(structref*)*nr_heads);}}elsereturn_refs=NULL;-match_pos=0;+head_pos=0;for(ref=*refs;ref;ref=next){next=ref->next;if(!memcmp(ref->name,"refs/",5)&&
@@ -556,17 +556,17 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)}else{intcmp=-1;-while(match_pos<nr_match){-cmp=strcmp(ref->name,match[match_pos]);+while(head_pos<nr_heads){+cmp=strcmp(ref->name,heads[head_pos]);if(cmp<0)/* definitely do not have it */break;elseif(cmp==0){/* definitely have it */-match[match_pos][0]='\0';-return_refs[match_pos]=ref;+heads[head_pos][0]='\0';+return_refs[head_pos]=ref;break;}else/* might have it; keep looking */-match_pos++;+head_pos++;}if(!cmp)continue;/* we will link it later */
From: Michael Haggerty <redacted>
This is the first of a few baby steps towards changing filter_refs()
to compress matched refs out of the list rather than overwriting the
first character of such references with '\0'.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/fetch-pack.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
From: Michael Haggerty <redacted>
I cannot find a scenario in which this function is called any
significant number of times, so simplify the code by always allocating
an array for return_refs rather than trying to use a stack-allocated
array for small lists.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/fetch-pack.c | 14 +++-----------
1 file changed, 3 insertions(+), 11 deletions(-)
From: Michael Haggerty <redacted>
This simplifies the logic without changing the behavior.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/fetch-pack.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
@@ -1025,10 +1025,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)close(fd[0]);close(fd[1]);if(finish_connect(conn))-ref=NULL;-ret=!ref;+return1;-if(!ret&&nr_heads){+ret=!ref;+if(ref&&nr_heads){/* If the heads to pull were given, we should have*consumedallofthembymatchingtheremote.*Otherwise,'gitfetchremoteno-such-ref'would
From: Michael Haggerty <redacted>
Remove any references that were received from the remote from the list
(*nr_heads, heads) of requested references by squeezing them out of
the list (rather than overwriting their names with NUL characters, as
before). On exit, *nr_heads is the number of requested references
that were not received.
Document this aspect of fetch_pack() in a comment in the header file.
(More documentation is obviously still needed.)
Signed-off-by: Michael Haggerty <redacted>
---
builtin/fetch-pack.c | 21 +++++++++++++--------
fetch-pack.h | 6 ++++++
transport.c | 3 ++-
3 files changed, 21 insertions(+), 9 deletions(-)
@@ -554,11 +554,12 @@ static void filter_refs(struct ref **refs, int *nr_heads, char **heads)break;elseif(cmp==0){/* definitely have it */return_refs[matched++]=ref;-heads[head_pos++][0]='\0';+head_pos++;break;}-else/* might have it; keep looking */-head_pos++;+else{/* might have it; keep looking */+heads[unmatched++]=heads[head_pos++];+}}if(!cmp)continue;/* we will link it later */
@@ -520,6 +520,7 @@ static int fetch_refs_via_pack(struct transport *transport,structgit_transport_data*data=transport->data;char**heads=xmalloc(nr_heads*sizeof(*heads));char**origh=xmalloc(nr_heads*sizeof(*origh));+intorig_nr_heads=nr_heads;conststructref*refs;char*dest=xstrdup(transport->url);structfetch_pack_argsargs;
@@ -558,7 +559,7 @@ static int fetch_refs_via_pack(struct transport *transport,free_refs(refs_tmp);-for(i=0;i<nr_heads;i++)+for(i=0;i<orig_nr_heads;i++)free(origh[i]);free(origh);free(heads);
From: Michael Haggerty <redacted>
This fixes a test in t5500.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/fetch-pack.c | 2 +-
t/t5500-fetch-pack.sh | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
@@ -1028,7 +1028,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)return1;ret=!ref;-if(ref&&nr_heads){+if(nr_heads){/* If the heads to pull were given, we should have*consumedallofthembymatchingtheremote.*Otherwise,'gitfetchremoteno-such-ref'would
From: Michael Haggerty <redacted>
Set the final value at initialization rather than initializing it then
sometimes changing it.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/fetch-pack.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
@@ -1027,17 +1027,16 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)if(finish_connect(conn))return1;-ret=!ref;-if(nr_heads){-/* If the heads to pull were given, we should have-*consumedallofthembymatchingtheremote.-*Otherwise,'gitfetchremoteno-such-ref'would-*silentlysucceedwithoutissuinganerror.-*/-for(i=0;i<nr_heads;i++)-error("no such remote ref %s",heads[i]);-ret=1;-}+ret=!ref||nr_heads;++/*+*Iftheheadstopullweregiven,weshouldhaveconsumed+*allofthembymatchingtheremote.Otherwise,'gitfetch+*remoteno-such-ref'wouldsilentlysucceedwithoutissuing+*anerror.+*/+for(i=0;i<nr_heads;i++)+error("no such remote ref %s",heads[i]);while(ref){printf("%s %s\n",sha1_to_hex(ref->old_sha1),ref->name);
From: Michael Haggerty <redacted>
fetch_pack() used to delete entries from the input list (*nr_heads,
heads) and drop them on the floor. (Even earlier versions dropped
some names on the floor and modified others.) This forced
fetch_refs_via_pack() to create a separate copy of the original list
so that it could free all of the names.
Instead, teach fetch_pack() to free any names that it discards from
the list, and change fetch_refs_via_pack() to free only the remaining
(unmatched) names.
Document the change in the function comment in the header file.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/fetch-pack.c | 4 +++-
fetch-pack.h | 7 ++++---
transport.c | 9 +++------
3 files changed, 10 insertions(+), 10 deletions(-)
@@ -519,8 +519,6 @@ static int fetch_refs_via_pack(struct transport *transport,{structgit_transport_data*data=transport->data;char**heads=xmalloc(nr_heads*sizeof(*heads));-char**origh=xmalloc(nr_heads*sizeof(*origh));-intorig_nr_heads=nr_heads;conststructref*refs;char*dest=xstrdup(transport->url);structfetch_pack_argsargs;
@@ -539,7 +537,7 @@ static int fetch_refs_via_pack(struct transport *transport,args.depth=data->options.depth;for(i=0;i<nr_heads;i++)-origh[i]=heads[i]=xstrdup(to_fetch[i]->name);+heads[i]=xstrdup(to_fetch[i]->name);if(!data->got_remote_heads){connect_setup(transport,0,0);
@@ -559,9 +557,8 @@ static int fetch_refs_via_pack(struct transport *transport,free_refs(refs_tmp);-for(i=0;i<orig_nr_heads;i++)-free(origh[i]);-free(origh);+for(i=0;i<nr_heads;i++)+free(heads[i]);free(heads);free(dest);return(refs?0:-1);
From: Michael Haggerty <redacted>
* Build linked list of return values as we go rather than recording
them in a temporary array and linking them up later.
* Handle ref in a single if...else statement in the main loop, to make
it clear that each ref has exactly two possible destinies.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/fetch-pack.c | 56 ++++++++++++++++++----------------------------------
1 file changed, 19 insertions(+), 37 deletions(-)
@@ -523,66 +523,48 @@ static void mark_recent_complete_commits(unsigned long cutoff)staticvoidfilter_refs(structref**refs,int*nr_heads,char**heads){-structref**return_refs;structref*newlist=NULL;structref**newtail=&newlist;structref*ref,*next;-inthead_pos=0,matched=0,unmatched=0;--if(*nr_heads&&!args.fetch_all)-return_refs=xcalloc(*nr_heads,sizeof(structref*));-else-return_refs=NULL;+inthead_pos=0,unmatched=0;for(ref=*refs;ref;ref=next){+intkeep_ref=0;next=ref->next;if(!memcmp(ref->name,"refs/",5)&&check_refname_format(ref->name,0));/* trash */elseif(args.fetch_all&&-(!args.depth||prefixcmp(ref->name,"refs/tags/"))){-*newtail=ref;-ref->next=NULL;-newtail=&ref->next;-continue;-}-else{-intcmp=-1;+(!args.depth||prefixcmp(ref->name,"refs/tags/")))+keep_ref=1;+elsewhile(head_pos<*nr_heads){-cmp=strcmp(ref->name,heads[head_pos]);-if(cmp<0)/* definitely do not have it */+intcmp=strcmp(ref->name,heads[head_pos]);+if(cmp<0){/* definitely do not have it */break;-elseif(cmp==0){/* definitely have it */-return_refs[matched++]=ref;+}elseif(cmp==0){/* definitely have it */free(heads[head_pos++]);+keep_ref=1;break;-}-else{/* might have it; keep looking */+}else{/* might have it; keep looking */heads[unmatched++]=heads[head_pos++];}}-if(!cmp)-continue;/* we will link it later */-}-free(ref);-}--if(!args.fetch_all){-inti;-/* copy remaining unmatched heads: */-while(head_pos<*nr_heads)-heads[unmatched++]=heads[head_pos++];-*nr_heads=unmatched;--for(i=0;i<matched;i++){-ref=return_refs[i];+if(keep_ref){*newtail=ref;ref->next=NULL;newtail=&ref->next;+}else{+free(ref);}-free(return_refs);}++/* copy any remaining unmatched heads: */+while(head_pos<*nr_heads)+heads[unmatched++]=heads[head_pos++];+*nr_heads=unmatched;+*refs=newlist;}
From: Michael Haggerty <redacted>
Once a match has been found at head_pos, the entry is zeroed and no
future attempts will match that entry. So increment head_pos to avoid
checking against the zeroed-out entry during the next iteration.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/fetch-pack.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
@@ -561,8 +561,8 @@ static void filter_refs(struct ref **refs, int nr_heads, char **heads)if(cmp<0)/* definitely do not have it */break;elseif(cmp==0){/* definitely have it */-heads[head_pos][0]='\0';return_refs[head_pos]=ref;+heads[head_pos++][0]='\0';break;}else/* might have it; keep looking */
@@ -556,7 +556,7 @@ static void filter_refs(struct ref **refs, int nr_heads, char **heads)}else{intcmp=-1;-while(head_pos<nr_heads){+while(head_pos<*nr_heads){cmp=strcmp(ref->name,heads[head_pos]);if(cmp<0)/* definitely do not have it */break;
From: Michael Haggerty <redacted>
fetch_pack() removes 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 <redacted>
---
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(-)
@@ -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))
From: Michael Haggerty <redacted>
It used to be that this function processed refnames in some arbitrary
order but wanted to return them in the order that they were requested,
not the order that they were processed. Now, the refnames are
processed in sorted order, so there is no reason to go to the extra
effort.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/fetch-pack.c | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)
@@ -554,7 +553,7 @@ static void filter_refs(struct ref **refs, int *nr_heads, char **heads)if(cmp<0)/* definitely do not have it */break;elseif(cmp==0){/* definitely have it */-return_refs[head_pos]=ref;+return_refs[matched++]=ref;heads[head_pos++][0]='\0';break;}