[PATCH v2 0/2] More object-related docstrings

[PATCH v2 0/2] More object-related docstrings

[Michael Haggerty]

This patch series applies on top of mh/replace-refs-variable-rename,
simply because one of the comments refers to the global variable
check_replace_refs by its new name.

This is a re-roll of patches 1/6 and 6/6 of the series
"mh/object-code-cleanup" that I submitted earlier [1].  Patches 2-5 of
that series have already been queued.

The first patch emphasizes that do_lookup_replace_object() is only
meant for internal use, and moves the real docstring for that function
from the header file to the implementation file.

The second patch changes the docstring for hash_obj() to mention that
its return value is not consistent across architectures, and adds a
comment within the function explaining some points about the
implementation that were suggested in the discussion about v1.

Thanks to Junio, Christian, Nicolas, Jakub, and Jonathan for their
comments on v1.

Michael

[1] http://thread.gmane.org/gmane.comp.version-control.git/242469

Michael Haggerty (2):
  Add docstrings for lookup_replace_object() and
    do_lookup_replace_object()
  Document some functions defined in object.c

 cache.h          | 13 +++++++++++++
 object.c         | 29 ++++++++++++++++++++++++++++-
 object.h         |  7 +++++++
 replace_object.c |  7 +++++++
 4 files changed, 55 insertions(+), 1 deletion(-)

-- 
1.8.5.3

[PATCH v2 1/2] Add docstrings for lookup_replace_object() and do_lookup_replace_object()

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 cache.h          | 13 +++++++++++++
 replace_object.c |  7 +++++++
 2 files changed, 20 insertions(+)

diff --git a/cache.h b/cache.h
index b039abc..9407560 100644
--- a/cache.h
+++ b/cache.h
@@ -798,13 +798,26 @@ static inline void *read_sha1_file(const unsigned char *sha1, enum object_type *
 {
 	return read_sha1_file_extended(sha1, type, size, LOOKUP_REPLACE_OBJECT);
 }
+
+/*
+ * This internal function is only declared here for the benefit of
+ * lookup_replace_object().  Please do not call it directly.
+ */
 extern const unsigned char *do_lookup_replace_object(const unsigned char *sha1);
+
+/*
+ * If object sha1 should be replaced, return the replacement object's
+ * name (replaced recursively, if necessary).  The return value is
+ * either sha1 or a pointer to a permanently-allocated value.  When
+ * object replacement is suppressed, always return sha1.
+ */
 static inline const unsigned char *lookup_replace_object(const unsigned char *sha1)
 {
 	if (!check_replace_refs)
 		return sha1;
 	return do_lookup_replace_object(sha1);
 }
+
 static inline const unsigned char *lookup_replace_object_extended(const unsigned char *sha1, unsigned flag)
 {
 	if (!(flag & LOOKUP_REPLACE_OBJECT))
diff --git a/replace_object.c b/replace_object.c
index c5cf9f4..31fabde 100644
--- a/replace_object.c
+++ b/replace_object.c
@@ -92,6 +92,13 @@ static void prepare_replace_object(void)
 /* We allow "recursive" replacement. Only within reason, though */
 #define MAXREPLACEDEPTH 5
 
+/*
+ * If a replacement for object sha1 has been set up, return the
+ * replacement object's name (replaced recursively, if necessary).
+ * The return value is either sha1 or a pointer to a
+ * permanently-allocated value.  This function always respects replace
+ * references, regardless of the value of check_replace_refs.
+ */
 const unsigned char *do_lookup_replace_object(const unsigned char *sha1)
 {
 	int pos, depth = MAXREPLACEDEPTH;
-- 
1.8.5.3

Re: [PATCH v2 1/2] Add docstrings for lookup_replace_object() and do_lookup_replace_object()

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---

Reviewed-by: Junio C Hamano <gitster@pobox.com>

Thanks.

>  cache.h          | 13 +++++++++++++
>  replace_object.c |  7 +++++++
>  2 files changed, 20 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index b039abc..9407560 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -798,13 +798,26 @@ static inline void *read_sha1_file(const unsigned char *sha1, enum object_type *
>  {
>  	return read_sha1_file_extended(sha1, type, size, LOOKUP_REPLACE_OBJECT);
>  }
> +
> +/*
> + * This internal function is only declared here for the benefit of
> + * lookup_replace_object().  Please do not call it directly.
> + */
>  extern const unsigned char *do_lookup_replace_object(const unsigned char *sha1);
> +
> +/*
> + * If object sha1 should be replaced, return the replacement object's
> + * name (replaced recursively, if necessary).  The return value is
> + * either sha1 or a pointer to a permanently-allocated value.  When
> + * object replacement is suppressed, always return sha1.
> + */
>  static inline const unsigned char *lookup_replace_object(const unsigned char *sha1)
>  {
>  	if (!check_replace_refs)
>  		return sha1;
>  	return do_lookup_replace_object(sha1);
>  }
> +
>  static inline const unsigned char *lookup_replace_object_extended(const unsigned char *sha1, unsigned flag)
>  {
>  	if (!(flag & LOOKUP_REPLACE_OBJECT))
> diff --git a/replace_object.c b/replace_object.c
> index c5cf9f4..31fabde 100644
> --- a/replace_object.c
> +++ b/replace_object.c
> @@ -92,6 +92,13 @@ static void prepare_replace_object(void)
>  /* We allow "recursive" replacement. Only within reason, though */
>  #define MAXREPLACEDEPTH 5
>  
> +/*
> + * If a replacement for object sha1 has been set up, return the
> + * replacement object's name (replaced recursively, if necessary).
> + * The return value is either sha1 or a pointer to a
> + * permanently-allocated value.  This function always respects replace
> + * references, regardless of the value of check_replace_refs.
> + */
>  const unsigned char *do_lookup_replace_object(const unsigned char *sha1)
>  {
>  	int pos, depth = MAXREPLACEDEPTH;

[PATCH v2 2/2] Document some functions defined in object.c

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 object.c | 29 ++++++++++++++++++++++++++++-
 object.h |  7 +++++++
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/object.c b/object.c
index 584f7ac..57a0890 100644
--- a/object.c
+++ b/object.c
@@ -43,14 +43,32 @@ int type_from_string(const char *str)
 	die("invalid object type \"%s\"", str);
 }
 
+/*
+ * Return a numerical hash value between 0 and n-1 for the object with
+ * the specified sha1.  n must be a power of 2.  Please note that the
+ * return value is *not* consistent across computer architectures.
+ */
 static unsigned int hash_obj(const unsigned char *sha1, unsigned int n)
 {
 	unsigned int hash;
+
+	/*
+	 * Since the sha1 is essentially random, we just take the
+	 * required number of bits directly from the first
+	 * sizeof(unsigned int) bytes of sha1.  First we have to copy
+	 * the bytes into a properly aligned integer.  If we cared
+	 * about getting consistent results across architectures, we
+	 * would have to call ntohl() here, too.
+	 */
 	memcpy(&hash, sha1, sizeof(unsigned int));
-	/* Assumes power-of-2 hash sizes in grow_object_hash */
 	return hash & (n - 1);
 }
 
+/*
+ * Insert obj into the hash table hash, which has length size (which
+ * must be a power of 2).  On collisions, simply overflow to the next
+ * empty bucket.
+ */
 static void insert_obj_hash(struct object *obj, struct object **hash, unsigned int size)
 {
 	unsigned int j = hash_obj(obj->sha1, size);
@@ -63,6 +81,10 @@ static void insert_obj_hash(struct object *obj, struct object **hash, unsigned i
 	hash[j] = obj;
 }
 
+/*
+ * Look up the record for the given sha1 in the hash map stored in
+ * obj_hash.  Return NULL if it was not found.
+ */
 struct object *lookup_object(const unsigned char *sha1)
 {
 	unsigned int i, first;
@@ -92,6 +114,11 @@ struct object *lookup_object(const unsigned char *sha1)
 	return obj;
 }
 
+/*
+ * Increase the size of the hash map stored in obj_hash to the next
+ * power of 2 (but at least 32).  Copy the existing values to the new
+ * hash map.
+ */
 static void grow_object_hash(void)
 {
 	int i;
diff --git a/object.h b/object.h
index dc5df8c..732bf4d 100644
--- a/object.h
+++ b/object.h
@@ -42,7 +42,14 @@ struct object {
 extern const char *typename(unsigned int type);
 extern int type_from_string(const char *str);
 
+/*
+ * Return the current number of buckets in the object hashmap.
+ */
 extern unsigned int get_max_object_index(void);
+
+/*
+ * Return the object from the specified bucket in the object hashmap.
+ */
 extern struct object *get_indexed_object(unsigned int);
 
 /*
-- 
1.8.5.3

Re: [PATCH v2 2/2] Document some functions defined in object.c

[Nicolas Pitre]

On Fri, 28 Feb 2014, Michael Haggerty wrote:

> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

Acked-by: Nicolas Pitre <nico@fluxnic.net>

> ---
>  object.c | 29 ++++++++++++++++++++++++++++-
>  object.h |  7 +++++++
>  2 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/object.c b/object.c
> index 584f7ac..57a0890 100644
> --- a/object.c
> +++ b/object.c
> @@ -43,14 +43,32 @@ int type_from_string(const char *str)
>  	die("invalid object type \"%s\"", str);
>  }
>  
> +/*
> + * Return a numerical hash value between 0 and n-1 for the object with
> + * the specified sha1.  n must be a power of 2.  Please note that the
> + * return value is *not* consistent across computer architectures.
> + */
>  static unsigned int hash_obj(const unsigned char *sha1, unsigned int n)
>  {
>  	unsigned int hash;
> +
> +	/*
> +	 * Since the sha1 is essentially random, we just take the
> +	 * required number of bits directly from the first
> +	 * sizeof(unsigned int) bytes of sha1.  First we have to copy
> +	 * the bytes into a properly aligned integer.  If we cared
> +	 * about getting consistent results across architectures, we
> +	 * would have to call ntohl() here, too.
> +	 */
>  	memcpy(&hash, sha1, sizeof(unsigned int));
> -	/* Assumes power-of-2 hash sizes in grow_object_hash */
>  	return hash & (n - 1);
>  }
>  
> +/*
> + * Insert obj into the hash table hash, which has length size (which
> + * must be a power of 2).  On collisions, simply overflow to the next
> + * empty bucket.
> + */
>  static void insert_obj_hash(struct object *obj, struct object **hash, unsigned int size)
>  {
>  	unsigned int j = hash_obj(obj->sha1, size);
> @@ -63,6 +81,10 @@ static void insert_obj_hash(struct object *obj, struct object **hash, unsigned i
>  	hash[j] = obj;
>  }
>  
> +/*
> + * Look up the record for the given sha1 in the hash map stored in
> + * obj_hash.  Return NULL if it was not found.
> + */
>  struct object *lookup_object(const unsigned char *sha1)
>  {
>  	unsigned int i, first;
> @@ -92,6 +114,11 @@ struct object *lookup_object(const unsigned char *sha1)
>  	return obj;
>  }
>  
> +/*
> + * Increase the size of the hash map stored in obj_hash to the next
> + * power of 2 (but at least 32).  Copy the existing values to the new
> + * hash map.
> + */
>  static void grow_object_hash(void)
>  {
>  	int i;
> diff --git a/object.h b/object.h
> index dc5df8c..732bf4d 100644
> --- a/object.h
> +++ b/object.h
> @@ -42,7 +42,14 @@ struct object {
>  extern const char *typename(unsigned int type);
>  extern int type_from_string(const char *str);
>  
> +/*
> + * Return the current number of buckets in the object hashmap.
> + */
>  extern unsigned int get_max_object_index(void);
> +
> +/*
> + * Return the object from the specified bucket in the object hashmap.
> + */
>  extern struct object *get_indexed_object(unsigned int);
>  
>  /*
> -- 
> 1.8.5.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>