Re: [PATCH v5] fetch-pack: always allow fetching of literal SHA1s

[Jonathan Nieder]

Jonathan Tan wrote:

[...]
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -15,6 +15,7 @@
>  #include "version.h"
>  #include "prio-queue.h"
>  #include "sha1-array.h"
> +#include "oidset.h"
>  
>  static int transfer_unpack_limit = -1;
>  static int fetch_unpack_limit = -1;
> @@ -592,13 +593,32 @@ static void mark_recent_complete_commits(struct 
> fetch_pack_args *args,
>       }
>  }
>  
> +static void add_refs_to_oidset(struct oidset *oids, struct ref *refs)
> +{
> +     for (; refs; refs = refs->next)
> +             oidset_insert(oids, &refs->old_oid);
> +}
> +
> +static int tip_oids_contain(struct oidset *tip_oids,
> +                         struct ref *unmatched, struct ref *newlist,
> +                         const struct object_id *id)
> +{
> +     if (!tip_oids->map.cmpfn) {

This feels like a layering violation.  Could it be e.g. a static inline
function oidset_is_initialized in oidset.h?

> +             add_refs_to_oidset(tip_oids, unmatched);
> +             add_refs_to_oidset(tip_oids, newlist);
> +     }
> +     return oidset_contains(tip_oids, id);
> +}

The rest looks good.

With or without that change,
Reviewed-by: Jonathan Nieder <jrnie...@gmail.com>

Thanks for your patient work.

diff --git i/fetch-pack.c w/fetch-pack.c
index 9dd430a65a..0394580434 100644
--- i/fetch-pack.c
+++ w/fetch-pack.c
@@ -603,7 +603,7 @@ static int tip_oids_contain(struct oidset *tip_oids,
                            struct ref *unmatched, struct ref *newlist,
                            const struct object_id *id)
 {
-       if (!tip_oids->map.cmpfn) {
+       if (!oidset_initialized(tip_oids)) {
                add_refs_to_oidset(tip_oids, unmatched);
                add_refs_to_oidset(tip_oids, newlist);
        }
diff --git i/oidset.c w/oidset.c
index ac169f05d3..f2a6753b8a 100644
--- i/oidset.c
+++ w/oidset.c
@@ -18,7 +18,7 @@ int oidset_contains(const struct oidset *set, const struct 
object_id *oid)
 {
        struct hashmap_entry key;
 
-       if (!set->map.cmpfn)
+       if (!oidset_initialized(set))
                return 0;
 
        hashmap_entry_init(&key, sha1hash(oid->hash));
@@ -29,7 +29,7 @@ int oidset_insert(struct oidset *set, const struct object_id 
*oid)
 {
        struct oidset_entry *entry;
 
-       if (!set->map.cmpfn)
+       if (!oidset_initialized(set))
                hashmap_init(&set->map, oidset_hashcmp, 0);
 
        if (oidset_contains(set, oid))
diff --git i/oidset.h w/oidset.h
index b7eaab5b88..2e7d889770 100644
--- i/oidset.h
+++ w/oidset.h
@@ -22,6 +22,16 @@ struct oidset {
 
 #define OIDSET_INIT { { NULL } }
 
+/**
+ * Returns true iff "set" has been initialized (for example by inserting
+ * an entry). An oidset is considered uninitialized if it hasn't had any
+ * oids inserted since it was last cleared.
+ */
+static inline int oidset_initialized(const struct oidset *set)
+{
+       return set->map.cmpfn ? 1 : 0;
+}
+
 /**
  * Returns true iff `set` contains `oid`.
  */