From: Michael Haggerty <redacted>
There were various confusing things (and a couple of bugs) in the way
that fetch_pack() handled the list (nr_heads, heads) of references to
be sought from the remote:
* Different names were used for the list in different functions for no
special reason.
* fetch_pack() modified the list in-place:
* It moved entries around
* It sometimes shrunk the list without informing the caller (which
could lead to spurious error messages)
* It overwrote the first byte of matching entries with NUL, leaving
a sparse list that the caller had to interpret
* Its interaction with the list was documented
* No error was reported if *all* requested references were missing
from the remote.
I'm still suspicious about the logic related to args.fetch_all and
args.depth, but I don't think I've made anything worse.
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 match_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
fetch_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>
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>
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>
Once a match has been found at match_pos, the entry is zeroed and no
future attempts will match that entry. So increment match_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[match_pos][0]='\0';return_refs[match_pos]=ref;+heads[match_pos++][0]='\0';break;}else/* might have it; keep looking */
From: Michael Haggerty <redacted>
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 <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>
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>
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 | 27 +++++++++++++--------------
1 file changed, 13 insertions(+), 14 deletions(-)
@@ -556,12 +556,12 @@ 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(match_pos<nr_heads){+cmp=strcmp(ref->name,heads[match_pos]);if(cmp<0)/* definitely do not have it */break;elseif(cmp==0){/* definitely have it */-match[match_pos][0]='\0';+heads[match_pos][0]='\0';return_refs[match_pos]=ref;break;}
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 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>
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[match_pos]=ref;+return_refs[matched++]=ref;heads[match_pos++][0]='\0';break;}
@@ -556,7 +556,7 @@ static void filter_refs(struct ref **refs, int nr_heads, char **heads)}else{intcmp=-1;-while(match_pos<nr_heads){+while(match_pos<*nr_heads){cmp=strcmp(ref->name,heads[match_pos]);if(cmp<0)/* definitely do not have it */break;
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;-intmatch_pos=0,matched=0,unmatched=0;--if(*nr_heads&&!args.fetch_all)-return_refs=xcalloc(*nr_heads,sizeof(structref*));-else-return_refs=NULL;+intmatch_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(match_pos<*nr_heads){-cmp=strcmp(ref->name,heads[match_pos]);-if(cmp<0)/* definitely do not have it */+intcmp=strcmp(ref->name,heads[match_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[match_pos++]);+keep_ref=1;break;-}-else{/* might have it; keep looking */+}else{/* might have it; keep looking */heads[unmatched++]=heads[match_pos++];}}-if(!cmp)-continue;/* we will link it later */-}-free(ref);-}--if(!args.fetch_all){-inti;-/* copy remaining unmatched heads: */-while(match_pos<*nr_heads)-heads[unmatched++]=heads[match_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(match_pos<*nr_heads)+heads[unmatched++]=heads[match_pos++];+*nr_heads=unmatched;+*refs=newlist;}
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 the 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>
---
This change forces callers to allocate names on the heap. But the two
existing callers did so already, and the function already modified the
list, so I think the new style is no more intrusive than the old.
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>
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[match_pos++][0]='\0';+match_pos++;break;}-else/* might have it; keep looking */-match_pos++;+else{/* might have it; keep looking */+heads[unmatched++]=heads[match_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: Jeff King <hidden> Date: 2016-06-15 22:54:33
On Thu, Aug 23, 2012 at 10:10:29AM +0200, mhagger@alum.mit.edu wrote:
From: Michael Haggerty <redacted>
Use the names (nr_heads, heads) consistently across functions, instead
of sometimes naming the same values (nr_match, match).
From: Jeff King <hidden> Date: 2016-06-15 22:54:33
On Thu, Aug 23, 2012 at 10:10:30AM +0200, mhagger@alum.mit.edu wrote:
From: Michael Haggerty <redacted>
Once a match has been found at match_pos, the entry is zeroed and no
future attempts will match that entry. So increment match_pos to
avoid checking against the zeroed-out entry during the next iteration.
Good catch.
A subtle side effect of this zero-ing (not introduced by your patch, but
something I noticed while re-reading the code) is that we implicitly
eliminate duplicate entries from the list of remote refs. There
shouldn't generally be any duplicates, of course, but I think skipping
them is probably sane.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:54:33
On Thu, Aug 23, 2012 at 10:10:31AM +0200, mhagger@alum.mit.edu wrote:
From: Michael Haggerty <redacted>
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.
@@ -391,7 +391,7 @@ test_expect_success 'fetch mixed refs from cmdline and stdin' ' test_expect_success'test duplicate refs from stdin''(cdclient&&-test_must_failgitfetch-pack--stdin--no-progress..<../input.dup+gitfetch-pack--stdin--no-progress..<../input.dup)>output&&cut-d" "-f2<output|sort>actual&&test_cmpexpectactual
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
From: Jeff King <hidden> Date: 2016-06-15 22:54:33
On Thu, Aug 23, 2012 at 10:10:41AM +0200, mhagger@alum.mit.edu wrote:
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 the all of the names.
From: Jeff King <hidden> Date: 2016-06-15 22:54:33
On Thu, Aug 23, 2012 at 10:10:25AM +0200, mhagger@alum.mit.edu wrote:
There were various confusing things (and a couple of bugs) in the way
that fetch_pack() handled the list (nr_heads, heads) of references to
be sought from the remote:
Aside from the minor comments I made to individual patches, this all
looks good. As usual, thanks for breaking it down; I wish all series
were as easy to review as this.
I'm still suspicious about the logic related to args.fetch_all and
args.depth, but I don't think I've made anything worse.
I think the point of that is that when doing "git fetch-pack --all
--depth=1", the meaning of "--all" is changed from "all refs" to
"everything but tags".
Which I kind of see the point of, because you don't want to grab ancient
tags that will be expensive. But wouldn't it make more sense to limit it
only to the contents of refs/heads in that case? Surely you wouldn't
want refs/notes, refs/remotes, or other hierarchies.
I suspect this code is never even run at all these days. All of the
callers inside git should actually provide a real list of refs, not
"--all". So it is really historical cruft for anybody who calls the
fetch-pack plumbing (I wonder if any third-party callers even exist;
this is such a deep part of the network infrastructure that any sane
scripts would probably just be calling fetch).
-Peff
From: Philip Oakley <hidden> Date: 2016-06-15 22:54:33
From: "Jeff King" <redacted>
Sent: Thursday, August 23, 2012 10:26 AM
On Thu, Aug 23, 2012 at 10:10:25AM +0200, mhagger@alum.mit.edu wrote:
quoted
There were various confusing things (and a couple of bugs) in the way
that fetch_pack() handled the list (nr_heads, heads) of references to
be sought from the remote:
Aside from the minor comments I made to individual patches, this all
looks good. As usual, thanks for breaking it down; I wish all series
were as easy to review as this.
quoted
I'm still suspicious about the logic related to args.fetch_all and
args.depth, but I don't think I've made anything worse.
I think the point of that is that when doing "git fetch-pack --all
--depth=1", the meaning of "--all" is changed from "all refs" to
"everything but tags".
There is a comment in \git\Documentation\technical\shallow.txt that
"- If you deepen a history, you'd want to get the tags of the
newly stored (but older!) commits. This does not work right now."
which may be the source of this restriction. That is, how is the depth
of the tag fetching to be restricted to the requested depth count?
[assuming I've undestood the problem correctly]
It may be (?) that it is a good time to think about a 'datedepth'
capability to bypass the current counted-depth shallow fetch that can
cause so much trouble. With a date limited depth the relevant tags could
also be fetched.
Which I kind of see the point of, because you don't want to grab
ancient
tags that will be expensive. But wouldn't it make more sense to limit
it
only to the contents of refs/heads in that case? Surely you wouldn't
want refs/notes, refs/remotes, or other hierarchies.
I suspect this code is never even run at all these days. All of the
callers inside git should actually provide a real list of refs, not
"--all". So it is really historical cruft for anybody who calls the
fetch-pack plumbing (I wonder if any third-party callers even exist;
this is such a deep part of the network infrastructure that any sane
scripts would probably just be calling fetch).
-Peff
--
From: Jeff King <hidden> Date: 2016-06-15 22:54:33
On Thu, Aug 23, 2012 at 08:13:29PM +0100, Philip Oakley wrote:
quoted
quoted
I'm still suspicious about the logic related to args.fetch_all and
args.depth, but I don't think I've made anything worse.
I think the point of that is that when doing "git fetch-pack --all
--depth=1", the meaning of "--all" is changed from "all refs" to
"everything but tags".
There is a comment in \git\Documentation\technical\shallow.txt that
"- If you deepen a history, you'd want to get the tags of the
newly stored (but older!) commits. This does not work right now."
which may be the source of this restriction. That is, how is the depth
of the tag fetching to be restricted to the requested depth count?
[assuming I've undestood the problem correctly]
I don't think this is about deepening, but rather about making sure we
remain shallow for the initial fetch. Remember that this is on the
"fetch-pack --all" code path, which used to be used by "git clone" when
it was a shell script (these days, clone is a C builtin and will
actually feed the list of refs to fetch-pack).
This code blames back to:
commit 4bcb310c2539b66d535e87508d1b7a90fe29c083
Author: Alexandre Julliard [off-list ref]
Date: Fri Nov 24 16:00:13 2006 +0100
fetch-pack: Do not fetch tags for shallow clones.
A better fix may be to only fetch tags that point to commits that we
are downloading, but git-clone doesn't have support for following
tags. This will happen automatically on the next git-fetch though.
So it is about making sure that "git clone --depth=1" does not
accidentally pull a single commit from v1.0, v1.1, v1.2, and so forth,
losing the purpose of using --depth in the first place. These days it is
largely irrelevant, since this code path is not followed by clone, and
clone will automatically restrict its list of fetched refs to a single
branch if --depth is used.
The bug that shallow.txt talks about (and which is mentioned in that
commit message) is that we will not properly auto-follow tags during
such a clone (i.e., when we fetch a tag because it is pointing to a
commit that we already have or are already pulling). I'm not sure if
that is still the case or not. But assuming your workflow is something
like:
[make an initial, cheap clone]
git clone --depth=1 $repo
[the next day, you do a regular fetch, which will just get new stuff
on top of what you already have]
git fetch
Then that second fetch will auto-follow the tags, anyway. And that is
what the commit message is pointing: it's a bug, but one you can work
around.
It may be (?) that it is a good time to think about a 'datedepth'
capability to bypass the current counted-depth shallow fetch that can
cause so much trouble. With a date limited depth the relevant tags
could also be fetched.
I don't have anything against such an idea, but I think it is orthogonal
to the issue being discussed here.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:54:33
On Thu, Aug 23, 2012 at 03:56:48PM -0400, Jeff King wrote:
This code blames back to:
commit 4bcb310c2539b66d535e87508d1b7a90fe29c083
Author: Alexandre Julliard [off-list ref]
Date: Fri Nov 24 16:00:13 2006 +0100
fetch-pack: Do not fetch tags for shallow clones.
A better fix may be to only fetch tags that point to commits that we
are downloading, but git-clone doesn't have support for following
tags. This will happen automatically on the next git-fetch though.
So it is about making sure that "git clone --depth=1" does not
accidentally pull a single commit from v1.0, v1.1, v1.2, and so forth,
losing the purpose of using --depth in the first place. These days it is
largely irrelevant, since this code path is not followed by clone, and
clone will automatically restrict its list of fetched refs to a single
branch if --depth is used.
I think part of the confusion of this code is that inside the loop over
the refs it is sometimes checking aspects of the ref, and sometimes
checking invariants of the loop (like args.fetch_all). Splitting it into
separate loops makes it easier to see what is going on, like the patch
below (on top of Michael's series).
I'm not sure if it ends up being more readable, since the generic "cut
down this linked list" function has to operate through callbacks with a
void pointer. On the other hand, that function could also be used
elsewhere.
@@ -521,51 +521,80 @@ static void mark_recent_complete_commits(unsigned long cutoff)}}-staticvoidfilter_refs(structref**refs,int*nr_heads,char**heads)+staticvoidfilter_refs_callback(structref**refs,+int(*want)(structref*,void*),+void*data){-structref*newlist=NULL;-structref**newtail=&newlist;+structref**tail=refs;structref*ref,*next;-intmatch_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/")))-keep_ref=1;-else-while(match_pos<*nr_heads){-intcmp=strcmp(ref->name,heads[match_pos]);-if(cmp<0){/* definitely do not have it */-break;-}elseif(cmp==0){/* definitely have it */-free(heads[match_pos++]);-keep_ref=1;-break;-}else{/* might have it; keep looking */-heads[unmatched++]=heads[match_pos++];-}-}--if(keep_ref){-*newtail=ref;-ref->next=NULL;-newtail=&ref->next;-}else{+if(want(ref,data))+tail=&ref->next;+else{free(ref);+*tail=next;}}+}-/* copy any remaining unmatched heads: */-while(match_pos<*nr_heads)-heads[unmatched++]=heads[match_pos++];-*nr_heads=unmatched;+staticintref_name_is_ok(structref*ref,void*data)+{+returnmemcmp(ref->name,"refs/",5)||+!check_refname_format(ref->name,0);+}++staticintref_ok_for_shallow(structref*ref,void*data)+{+returnprefixcmp(ref->name,"refs/tags/");+}-*refs=newlist;+structfilter_by_name_data{+char**heads;+intnr_heads;+intmatch_pos;+intunmatched;+};++staticintwant_ref_name(structref*ref,void*data)+{+structfilter_by_name_data*f=data;++while(f->match_pos<f->nr_heads){+intcmp=strcmp(ref->name,f->heads[f->match_pos]);+if(cmp<0)/* definitely do not have it */+return0;+elseif(cmp==0){/* definitely have it */+free(f->heads[f->match_pos++]);+return1;+}else/* might have it; keep looking */+f->heads[f->unmatched++]=f->heads[f->match_pos++];+}+return0;+}++staticvoidfilter_refs(structref**refs,int*nr_heads,char**heads)+{+structfilter_by_name_dataf;++filter_refs_callback(refs,ref_name_is_ok,NULL);++if(args.fetch_all){+if(args.depth)+filter_refs_callback(refs,ref_ok_for_shallow,NULL);+return;+}++memset(&f,0,sizeof(f));+f.heads=heads;+f.nr_heads=*nr_heads;+filter_refs_callback(refs,want_ref_name,&f);++/* copy any remaining unmatched heads: */+while(f.match_pos<f.nr_heads)+heads[f.unmatched++]=heads[f.match_pos++];+*nr_heads=f.unmatched;}staticvoidmark_alternate_complete(conststructref*ref,void*unused)
On Thu, Aug 23, 2012 at 08:13:29PM +0100, Philip Oakley wrote:
quoted
quoted
quoted
I'm still suspicious about the logic related to args.fetch_all and
args.depth, but I don't think I've made anything worse.
I think the point of that is that when doing "git fetch-pack --all
--depth=1", the meaning of "--all" is changed from "all refs" to
"everything but tags".
There is a comment in \git\Documentation\technical\shallow.txt that
"- If you deepen a history, you'd want to get the tags of the
newly stored (but older!) commits. This does not work right now."
which may be the source of this restriction. That is, how is the
depth
of the tag fetching to be restricted to the requested depth count?
[assuming I've undestood the problem correctly]
I don't think this is about deepening, but rather about making sure we
remain shallow for the initial fetch. Remember that this is on the
"fetch-pack --all" code path, which used to be used by "git clone"
when
it was a shell script (these days, clone is a C builtin and will
actually feed the list of refs to fetch-pack).
OK
This code blames back to:
commit 4bcb310c2539b66d535e87508d1b7a90fe29c083
Author: Alexandre Julliard [off-list ref]
Date: Fri Nov 24 16:00:13 2006 +0100
fetch-pack: Do not fetch tags for shallow clones.
A better fix may be to only fetch tags that point to commits that
we
are downloading, but git-clone doesn't have support for following
tags. This will happen automatically on the next git-fetch though.
So it is about making sure that "git clone --depth=1" does not
accidentally pull a single commit from v1.0, v1.1, v1.2, and so forth,
losing the purpose of using --depth in the first place. These days it
is
largely irrelevant, since this code path is not followed by clone, and
clone will automatically restrict its list of fetched refs to a single
branch if --depth is used.
The bug that shallow.txt talks about (and which is mentioned in that
commit message) is that we will not properly auto-follow tags during
such a clone (i.e., when we fetch a tag because it is pointing to a
commit that we already have or are already pulling). I'm not sure if
that is still the case or not. But assuming your workflow is something
like:
[make an initial, cheap clone]
git clone --depth=1 $repo
[the next day, you do a regular fetch, which will just get new stuff
on top of what you already have]
git fetch
Then that second fetch will auto-follow the tags, anyway. And that is
what the commit message is pointing: it's a bug, but one you can work
around.
I hadn't appreciated that the fetch would limit itself to the original
shallow
depth. I'd gained the impression that one need to use the --depth to
control what was being fetched.
quoted
It may be (?) that it is a good time to think about a 'datedepth'
capability to bypass the current counted-depth shallow fetch that can
cause so much trouble. With a date limited depth the relevant tags
could also be fetched.
I don't have anything against such an idea, but I think it is
orthogonal
to the issue being discussed here.
From: Michael Haggerty <hidden> Date: 2016-06-15 22:54:34
On 08/23/2012 10:39 AM, Jeff King wrote:
On Thu, Aug 23, 2012 at 10:10:29AM +0200, mhagger@alum.mit.edu wrote:
quoted
From: Michael Haggerty <redacted>
Use the names (nr_heads, heads) consistently across functions, instead
of sometimes naming the same values (nr_match, match).
Would be:
while (head_pos < nr_heads)
which makes more sense to me.
I was up in the air about this, because match_pos *is* the position at
which a match is attempted. But since the name also strikes you as
wrong, I will change it in the next version.
Thanks for this and all of your other comments!
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
From: Michael Haggerty <hidden> Date: 2016-06-15 22:54:34
On 08/23/2012 10:54 AM, Jeff King wrote:
On Thu, Aug 23, 2012 at 10:10:31AM +0200, mhagger@alum.mit.edu wrote:
quoted
From: Michael Haggerty <redacted>
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.
@@ -391,7 +391,7 @@ test_expect_success 'fetch mixed refs from cmdline and stdin' ' test_expect_success'test duplicate refs from stdin''(cdclient&&-test_must_failgitfetch-pack--stdin--no-progress..<../input.dup+gitfetch-pack--stdin--no-progress..<../input.dup)>output&&cut-d" "-f2<output|sort>actual&&test_cmpexpectactual
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
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
Yes, thanks. Also thanks for the typo correction in [PATCH 16/17], and
in general for your review of the patch series. Re-roll to follow shortly.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
From: Michael Haggerty <hidden> Date: 2016-06-15 22:54:34
On 08/23/2012 10:31 PM, Jeff King wrote:
I think part of the confusion of this code is that inside the loop over
the refs it is sometimes checking aspects of the ref, and sometimes
checking invariants of the loop (like args.fetch_all). Splitting it into
separate loops makes it easier to see what is going on, like the patch
below (on top of Michael's series).
I'm not sure if it ends up being more readable, since the generic "cut
down this linked list" function has to operate through callbacks with a
void pointer. On the other hand, that function could also be used
elsewhere.
[...]
Despite requiring a bit more boilerplate, I think that your change makes
the logic clearer.
*If* we want to switch to using callbacks, then we can get even more
bang for the buck, as in the attached patch (which applies on top of my
patch v2). Beyond your suggestion, this patch:
* Inlines your filter_refs() into everything_local(), because (a) it's
short and (b) the policy work implemented there logically belongs
higher-up in the call chain.
* Renames your filter_refs_callback() to filter_refs().
* Moves the initialization of the filter_by_name_data structure
(including sorting and de-duping) all the way up to fetch_pack(), and
passes a filter_by_name_data* (rather than (nr_heads, heads)) down to
the callees.
If you like this change, let me know and I'll massage it into a
digestible patch series.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
From: Michael Haggerty <hidden> Date: 2016-06-15 22:54:37
On 08/23/2012 10:31 PM, Jeff King wrote:
quoted hunk
On Thu, Aug 23, 2012 at 03:56:48PM -0400, Jeff King wrote:
quoted
This code blames back to:
commit 4bcb310c2539b66d535e87508d1b7a90fe29c083
Author: Alexandre Julliard [off-list ref]
Date: Fri Nov 24 16:00:13 2006 +0100
fetch-pack: Do not fetch tags for shallow clones.
A better fix may be to only fetch tags that point to commits that we
are downloading, but git-clone doesn't have support for following
tags. This will happen automatically on the next git-fetch though.
So it is about making sure that "git clone --depth=1" does not
accidentally pull a single commit from v1.0, v1.1, v1.2, and so forth,
losing the purpose of using --depth in the first place. These days it is
largely irrelevant, since this code path is not followed by clone, and
clone will automatically restrict its list of fetched refs to a single
branch if --depth is used.
I think part of the confusion of this code is that inside the loop over
the refs it is sometimes checking aspects of the ref, and sometimes
checking invariants of the loop (like args.fetch_all). Splitting it into
separate loops makes it easier to see what is going on, like the patch
below (on top of Michael's series).
I'm not sure if it ends up being more readable, since the generic "cut
down this linked list" function has to operate through callbacks with a
void pointer. On the other hand, that function could also be used
elsewhere.
@@ -521,51 +521,80 @@ static void mark_recent_complete_commits(unsigned long cutoff)}}-staticvoidfilter_refs(structref**refs,int*nr_heads,char**heads)+staticvoidfilter_refs_callback(structref**refs,+int(*want)(structref*,void*),+void*data){-structref*newlist=NULL;-structref**newtail=&newlist;+structref**tail=refs;structref*ref,*next;-intmatch_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/")))-keep_ref=1;-else-while(match_pos<*nr_heads){-intcmp=strcmp(ref->name,heads[match_pos]);-if(cmp<0){/* definitely do not have it */-break;-}elseif(cmp==0){/* definitely have it */-free(heads[match_pos++]);-keep_ref=1;-break;-}else{/* might have it; keep looking */-heads[unmatched++]=heads[match_pos++];-}-}--if(keep_ref){-*newtail=ref;-ref->next=NULL;-newtail=&ref->next;-}else{+if(want(ref,data))+tail=&ref->next;+else{free(ref);+*tail=next;}}+}-/* copy any remaining unmatched heads: */-while(match_pos<*nr_heads)-heads[unmatched++]=heads[match_pos++];-*nr_heads=unmatched;+staticintref_name_is_ok(structref*ref,void*data)+{+returnmemcmp(ref->name,"refs/",5)||+!check_refname_format(ref->name,0);+}++staticintref_ok_for_shallow(structref*ref,void*data)+{+returnprefixcmp(ref->name,"refs/tags/");+}-*refs=newlist;+structfilter_by_name_data{+char**heads;+intnr_heads;+intmatch_pos;+intunmatched;+};++staticintwant_ref_name(structref*ref,void*data)+{+structfilter_by_name_data*f=data;++while(f->match_pos<f->nr_heads){+intcmp=strcmp(ref->name,f->heads[f->match_pos]);+if(cmp<0)/* definitely do not have it */+return0;+elseif(cmp==0){/* definitely have it */+free(f->heads[f->match_pos++]);+return1;+}else/* might have it; keep looking */+f->heads[f->unmatched++]=f->heads[f->match_pos++];+}+return0;+}++staticvoidfilter_refs(structref**refs,int*nr_heads,char**heads)+{+structfilter_by_name_dataf;++filter_refs_callback(refs,ref_name_is_ok,NULL);++if(args.fetch_all){+if(args.depth)+filter_refs_callback(refs,ref_ok_for_shallow,NULL);+return;+}++memset(&f,0,sizeof(f));+f.heads=heads;+f.nr_heads=*nr_heads;+filter_refs_callback(refs,want_ref_name,&f);++/* copy any remaining unmatched heads: */+while(f.match_pos<f.nr_heads)+heads[f.unmatched++]=heads[f.match_pos++];+*nr_heads=f.unmatched;}staticvoidmark_alternate_complete(conststructref*ref,void*unused)
I need a sanity check here. I don't see that it is forbidden to call
"git fetch-pack --all --depth=N" and also specify some explicit
references. (This usage would make it possible for the user to do a
shallow clone while also retrieving some specific tags.)
However, if I try to do this I get (before your change)
$ git fetch-pack -v --all --depth=1 $URL refs/heads/master
Server supports multi_ack_detailed
Server supports side-band-64k
Server supports ofs-delta
want 9c9f2f4453a7260d0d60926c73811f025be98ded (HEAD)
want 9c9f2f4453a7260d0d60926c73811f025be98ded (refs/heads/master)
done
remote: Counting objects: 6, done.
remote: Compressing objects: 100% (4/4), done.
remote: Total 6 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (6/6), done.
error: no such remote ref refs/heads/master
9c9f2f4453a7260d0d60926c73811f025be98ded HEAD
9c9f2f4453a7260d0d60926c73811f025be98ded refs/heads/master
Note the error message "no such remote ref refs/heads/master", which is
incorrect. This happens because when the (fetch_all && (!depth ||
ref_non_tag(ref))) branch of the if statement succeeds, then the
look-in-heads-list branch is never executed, and therefore the requested
reference is never marked as having been found among the remote references.
On the other hand, if I try the same with an (existing) tag, before your
change I get
$ git fetch-pack -v --all --depth=1 $URL refs/tags/c
Server supports multi_ack_detailed
Server supports side-band-64k
Server supports ofs-delta
fatal: The remote end hung up unexpectedly
Now this is obviously broken in git-fetch-pack, because when fetch_all
is specified, return_refs is never initialized, but the
look-in-heads-list branch of the if is executed nevertheless.
It is not hard to fix these problems in the old code: (1)
unconditionally initialize return_refs, and (2) exchange the
look-in-heads-list handling and the fetch_all handling.
After your change the first problem (spurious error when asking for a
head) remains unchanged. The second problem is changed from a crash to
a misbehavior:
$ git fetch-pack -v --all --depth=1 $URL refs/tags/c
Initialized empty Git repository in /home/mhagger/tmp/cl/c/.git/
Server supports multi_ack_detailed
Server supports side-band-64k
Server supports ofs-delta
want 9c9f2f4453a7260d0d60926c73811f025be98ded (HEAD)
want 9c9f2f4453a7260d0d60926c73811f025be98ded (refs/heads/master)
done
remote: Counting objects: 6, done.
remote: Compressing objects: 100% (4/4), done.
remote: Total 6 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (6/6), done.
error: no such remote ref refs/tags/c
9c9f2f4453a7260d0d60926c73811f025be98ded HEAD
9c9f2f4453a7260d0d60926c73811f025be98ded refs/heads/master
Note that "refs/tags/c" is not obtained from the remote even though it
was specifically requested. The reason that your patch makes the crash
disappear is that when fetch_all is true, then the look-in-heads-list
branch is not executed at all -- but this is also why "refs/tags/c" is
not obtained.
So, assuming that we want to support
git fetch-pack --all --depth=N $URL refs/tags/TAG
correctly, then your filter_refs_callback() refactoring won't work, at
least not the way that you have written it. I will continue working on
this.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/