[1/2] fetch-object: provide only one fetching function
diff mbox series

Message ID 72399daf65b9c0a7ed12589584f5108a9ff6fc26.1536767071.git.jonathantanmy@google.com
State New
Headers show
Series
  • Bugfix for partial clones and ref-in-want servers
Related show

Commit Message

Jonathan Tan Sept. 12, 2018, 3:47 p.m. UTC
fetch-object.h currently provides two functions (fetch_object() and
fetch_objects()) that could be replaced by a single, more flexible
function. Replace those two functions with the more flexible function.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 fetch-object.c | 16 +++++-----------
 fetch-object.h |  8 ++------
 sha1-file.c    |  2 +-
 unpack-trees.c |  2 +-
 4 files changed, 9 insertions(+), 19 deletions(-)

Comments

Junio C Hamano Sept. 12, 2018, 9:50 p.m. UTC | #1
Jonathan Tan <jonathantanmy@google.com> writes:

> fetch-object.h currently provides two functions (fetch_object() and
> fetch_objects()) that could be replaced by a single, more flexible
> function. Replace those two functions with the more flexible function.

The latter half of the first sentence and the second sentence are
pretty-much repeating the same thing.  

I think you wanted to justify two changes:

 (1) There are two helpers.  fetch_object() fetches a single object
     from a given remote; fetch_objects() fetches a set of objects
     from a given remote.  It is not like the former is implemented
     as a specialization of the latter (i.e. passing length=1
     array), and it is not like the former is optimized specially
     for a single object fetch that cannot be made into such a thin
     wrapper.  It is not like the latter is implemented as a
     repeated call to the former in a loop, either.  There is no
     justification to keep two independent implementations, and
     using a single helper that can be used by all the callers of
     these two would make sense.

 (2) The variant that fetches multiple objects takes oid_array.  To
     be used by all the existing callers of these two helpers, it is
     better to use a different calling convention, namely, a array[]
     and its length passed as separate parameters.
     
Instead of explaining why the new convention is better to justify
(2), the above three lines handwave by saying "more flexible"
twice.  We should do better.

	fetch-object: unify fetch_object[s] functions

	There are fetch_object() and fetch_objects() helpers in
	fetch-object.h; as the latter takes "struct oid_array",
	the former cannot be made into a thin wrapper around the
	latter without an extra allocation and set-up cost.

	Update fetch_objects() to take an array of "struct
	object_id" and number of elements in it as separate
	parameters, remove fetch_object(), and adjust all existing
	callers of these functions to use the new fetch_objects().

perhaps?

> diff --git a/sha1-file.c b/sha1-file.c
> index 97b742384..2edf4564f 100644
> --- a/sha1-file.c
> +++ b/sha1-file.c
> @@ -1317,7 +1317,7 @@ int oid_object_info_extended(struct repository *r, const struct object_id *oid,
>  			 * TODO Pass a repository struct through fetch_object,
>  			 * such that arbitrary repositories work.
>  			 */
> -			fetch_object(repository_format_partial_clone, real->hash);
> +			fetch_objects(repository_format_partial_clone, real, 1);
>  			already_retried = 1;
>  			continue;
>  		}
> diff --git a/unpack-trees.c b/unpack-trees.c
> index f25089b87..82a83dbc6 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -392,7 +392,7 @@ static int check_updates(struct unpack_trees_options *o)
>  		}
>  		if (to_fetch.nr)
>  			fetch_objects(repository_format_partial_clone,
> -				      &to_fetch);
> +				      to_fetch.oid, to_fetch.nr);
>  		fetch_if_missing = fetch_if_missing_store;
>  		oid_array_clear(&to_fetch);
>  	}

Changes to these two callers do explain why <ptr, nr> is an
interface that is easier to use.  Those who already have an
oid_array can pass its fields as separate parameters without too
much trouble, and those who only have an oid (or an array of oid and
knows how many are in the array) do not have to wrap them into an
oid_array.

Thanks.
Jonathan Tan Sept. 13, 2018, 6:09 p.m. UTC | #2
> Instead of explaining why the new convention is better to justify
> (2), the above three lines handwave by saying "more flexible"
> twice.  We should do better.
> 
> 	fetch-object: unify fetch_object[s] functions
> 
> 	There are fetch_object() and fetch_objects() helpers in
> 	fetch-object.h; as the latter takes "struct oid_array",
> 	the former cannot be made into a thin wrapper around the
> 	latter without an extra allocation and set-up cost.
> 
> 	Update fetch_objects() to take an array of "struct
> 	object_id" and number of elements in it as separate
> 	parameters, remove fetch_object(), and adjust all existing
> 	callers of these functions to use the new fetch_objects().
> 
> perhaps?

Thanks - your explanation is much clearer than mine. Let me know if you
want a reroll (or if you can update the commit message yourself, that's
fine too).
Junio C Hamano Sept. 13, 2018, 8:55 p.m. UTC | #3
Jonathan Tan <jonathantanmy@google.com> writes:

>> Instead of explaining why the new convention is better to justify
>> (2), the above three lines handwave by saying "more flexible"
>> twice.  We should do better.
>> 
>> 	fetch-object: unify fetch_object[s] functions
>> 
>> 	There are fetch_object() and fetch_objects() helpers in
>> 	fetch-object.h; as the latter takes "struct oid_array",
>> 	the former cannot be made into a thin wrapper around the
>> 	latter without an extra allocation and set-up cost.
>> 
>> 	Update fetch_objects() to take an array of "struct
>> 	object_id" and number of elements in it as separate
>> 	parameters, remove fetch_object(), and adjust all existing
>> 	callers of these functions to use the new fetch_objects().
>> 
>> perhaps?
>
> Thanks - your explanation is much clearer than mine. Let me know if you
> want a reroll (or if you can update the commit message yourself, that's
> fine too).

If there is no other change needed for either of the patches, I do
not mind rewording the 1/2 myself to save a round-trip.

Thanks.

Patch
diff mbox series

diff --git a/fetch-object.c b/fetch-object.c
index 853624f81..1af1bf857 100644
--- a/fetch-object.c
+++ b/fetch-object.c
@@ -23,21 +23,15 @@  static void fetch_refs(const char *remote_name, struct ref *ref)
 	fetch_if_missing = original_fetch_if_missing;
 }
 
-void fetch_object(const char *remote_name, const unsigned char *sha1)
-{
-	struct ref *ref = alloc_ref(sha1_to_hex(sha1));
-	hashcpy(ref->old_oid.hash, sha1);
-	fetch_refs(remote_name, ref);
-}
-
-void fetch_objects(const char *remote_name, const struct oid_array *to_fetch)
+void fetch_objects(const char *remote_name, const struct object_id *oids,
+		   int oid_nr)
 {
 	struct ref *ref = NULL;
 	int i;
 
-	for (i = 0; i < to_fetch->nr; i++) {
-		struct ref *new_ref = alloc_ref(oid_to_hex(&to_fetch->oid[i]));
-		oidcpy(&new_ref->old_oid, &to_fetch->oid[i]);
+	for (i = 0; i < oid_nr; i++) {
+		struct ref *new_ref = alloc_ref(oid_to_hex(&oids[i]));
+		oidcpy(&new_ref->old_oid, &oids[i]);
 		new_ref->next = ref;
 		ref = new_ref;
 	}
diff --git a/fetch-object.h b/fetch-object.h
index 4b269d07e..d2f996d4e 100644
--- a/fetch-object.h
+++ b/fetch-object.h
@@ -1,11 +1,7 @@ 
 #ifndef FETCH_OBJECT_H
 #define FETCH_OBJECT_H
 
-#include "sha1-array.h"
-
-extern void fetch_object(const char *remote_name, const unsigned char *sha1);
-
-extern void fetch_objects(const char *remote_name,
-			  const struct oid_array *to_fetch);
+void fetch_objects(const char *remote_name, const struct object_id *oids,
+		   int oid_nr);
 
 #endif
diff --git a/sha1-file.c b/sha1-file.c
index 97b742384..2edf4564f 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -1317,7 +1317,7 @@  int oid_object_info_extended(struct repository *r, const struct object_id *oid,
 			 * TODO Pass a repository struct through fetch_object,
 			 * such that arbitrary repositories work.
 			 */
-			fetch_object(repository_format_partial_clone, real->hash);
+			fetch_objects(repository_format_partial_clone, real, 1);
 			already_retried = 1;
 			continue;
 		}
diff --git a/unpack-trees.c b/unpack-trees.c
index f25089b87..82a83dbc6 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -392,7 +392,7 @@  static int check_updates(struct unpack_trees_options *o)
 		}
 		if (to_fetch.nr)
 			fetch_objects(repository_format_partial_clone,
-				      &to_fetch);
+				      to_fetch.oid, to_fetch.nr);
 		fetch_if_missing = fetch_if_missing_store;
 		oid_array_clear(&to_fetch);
 	}