| Message ID | 20190604021330.16130-6-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix fetch regression with transport helpers | expand |
On Mon, Jun 03, 2019 at 09:13:30PM -0500, Felipe Contreras wrote: > Commit e198b3a740 changed the behavior of fetch with regards to tags. > Before, null oids where not ignored, now they are, regardless of whether > the refs have been explicitly cleared or not. > > e198b3a740 (fetch: replace string-list used as a look-up table with a hashmap) > > When using a transport helper the oids can certainly be null. So now > tags are ignored and fetching them is impossible. > > This patch fixes that by having a specific flag that is set only when we > explicitly want to ignore the refs, restoring the original behavior. Makes sense. Prior to e198b3a740, we indicated "ignored" by setting the oid (in item->util) to NULL. But since it's no longer a pointer after that commit, we can't do so. So the obvious alternative to this is going back to having it as a pointer. I think I prefer your solution, though, since it keeps the memory management a bit simpler, and more clearly expresses the intent. If we add any new code that has to similarly ignore an item in the hash, it will have to remember to use clear_item() rather than oidclr(). But I don't think that's a big risk. > builtin/fetch.c | 5 +++-- > t/t5801-remote-helpers.sh | 2 +- > 2 files changed, 4 insertions(+), 3 deletions(-) The code change itself looks obviously correct. -Peff
Felipe Contreras <felipe.contreras@gmail.com> writes: > Commit e198b3a740 changed the behavior of fetch with regards to tags. > Before, null oids where not ignored, now they are, regardless of whether > the refs have been explicitly cleared or not. > > e198b3a740 (fetch: replace string-list used as a look-up table with a hashmap) > > When using a transport helper the oids can certainly be null. So now > tags are ignored and fetching them is impossible. > > This patch fixes that by having a specific flag that is set only when we > explicitly want to ignore the refs, restoring the original behavior. > > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > --- > builtin/fetch.c | 5 +++-- > t/t5801-remote-helpers.sh | 2 +- > 2 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/builtin/fetch.c b/builtin/fetch.c > index 9dc551551e..f2be50a4a3 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -239,6 +239,7 @@ static int will_fetch(struct ref **head, const unsigned char *sha1) > struct refname_hash_entry { > struct hashmap_entry ent; /* must be the first member */ > struct object_id oid; > + int ignore; > char refname[FLEX_ARRAY]; > }; > > @@ -289,7 +290,7 @@ static int refname_hash_exists(struct hashmap *map, const char *refname) > > static void clear_item(struct refname_hash_entry *item) > { > - oidclr(&item->oid); > + item->ignore = 1; > } > > static void find_non_local_tags(const struct ref *refs, > @@ -374,7 +375,7 @@ static void find_non_local_tags(const struct ref *refs, > BUG("unseen remote ref?"); > > /* Unless we have already decided to ignore this item... */ > - if (is_null_oid(&item->oid)) > + if (item->ignore) > continue; Yeah, we should have added a bit like this to preserve the distinction between a NULL pointer for "struct object_id" and oid that has 0{40} in the old code, which was lost in the conversion. Thanks for spotting and fixing.
diff --git a/builtin/fetch.c b/builtin/fetch.c index 9dc551551e..f2be50a4a3 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -239,6 +239,7 @@ static int will_fetch(struct ref **head, const unsigned char *sha1) struct refname_hash_entry { struct hashmap_entry ent; /* must be the first member */ struct object_id oid; + int ignore; char refname[FLEX_ARRAY]; }; @@ -289,7 +290,7 @@ static int refname_hash_exists(struct hashmap *map, const char *refname) static void clear_item(struct refname_hash_entry *item) { - oidclr(&item->oid); + item->ignore = 1; } static void find_non_local_tags(const struct ref *refs, @@ -374,7 +375,7 @@ static void find_non_local_tags(const struct ref *refs, BUG("unseen remote ref?"); /* Unless we have already decided to ignore this item... */ - if (is_null_oid(&item->oid)) + if (item->ignore) continue; rm = alloc_ref(item->refname); diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh index 238774bc17..2d6c4a281e 100755 --- a/t/t5801-remote-helpers.sh +++ b/t/t5801-remote-helpers.sh @@ -303,7 +303,7 @@ test_expect_success 'fetch url' ' compare_refs server HEAD local FETCH_HEAD ' -test_expect_failure 'fetch tag' ' +test_expect_success 'fetch tag' ' (cd server && git tag v1.0 ) &&
Commit e198b3a740 changed the behavior of fetch with regards to tags. Before, null oids where not ignored, now they are, regardless of whether the refs have been explicitly cleared or not. e198b3a740 (fetch: replace string-list used as a look-up table with a hashmap) When using a transport helper the oids can certainly be null. So now tags are ignored and fetching them is impossible. This patch fixes that by having a specific flag that is set only when we explicitly want to ignore the refs, restoring the original behavior. Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- builtin/fetch.c | 5 +++-- t/t5801-remote-helpers.sh | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-)