[PATCH 0/6] Add a bunch of docstrings and make a few minor cleanups

[PATCH 0/6] Add a bunch of docstrings and make a few minor cleanups

[Michael Haggerty]

I was reading around in the neighborhood of fsck, objects, and packs
and I had the familiar and discouraging experience of having to read
code all the way up and down the callstack to understand *anything*.
Please let's all make more of an effort to document functions,
especially things that are not obvious from the name and signature,
like who owns the memory that is being referred to.

This is my attempt to document a number of the functions that I was
looking at based on what I inferred from my reading.  It is also a
selfish trick to get other people to double-check my understanding.

I also fixed up a couple of small things that I noticed along the way:
"refactoring for understanding".

Michael Haggerty (6):
  Add docstrings for lookup_replace_object() and
    do_lookup_replace_object()
  replace_object: use struct members instead of an array
  find_pack_entry(): document last_found_pack
  sha1_file_name(): declare to return a const string
  Document a bunch of functions defined in sha1_file.c
  Document some functions defined in object.c

 cache.h          | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
 http.c           |  2 +-
 object.c         | 23 +++++++++++++++-
 object.h         |  7 +++++
 replace_object.c | 17 ++++++++----
 sha1_file.c      | 66 ++++++++++++++++++++++++--------------------
 6 files changed, 157 insertions(+), 42 deletions(-)

-- 
1.8.5.3

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

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 cache.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/cache.h b/cache.h
index dc040fb..0ecd1c8 100644
--- a/cache.h
+++ b/cache.h
@@ -788,13 +788,29 @@ static inline void *read_sha1_file(const unsigned char *sha1, enum object_type *
 {
 	return read_sha1_file_extended(sha1, type, size, LOOKUP_REPLACE_OBJECT);
 }
+
+/*
+ * 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.
+ */
 extern const unsigned char *do_lookup_replace_object(const unsigned char *sha1);
+
+/*
+ * If object sha1 should be replaced, return the replacement object's
+ * name.  This function is similar to do_lookup_replace_object(),
+ * except that it when object replacement is suppressed, it always
+ * returns its argument unchanged.
+ */
 static inline const unsigned char *lookup_replace_object(const unsigned char *sha1)
 {
 	if (!read_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))
-- 
1.8.5.3

Re: [PATCH 1/6] 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>
> ---
>  cache.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index dc040fb..0ecd1c8 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -788,13 +788,29 @@ static inline void *read_sha1_file(const unsigned char *sha1, enum object_type *
>  {
>  	return read_sha1_file_extended(sha1, type, size, LOOKUP_REPLACE_OBJECT);
>  }
> +
> +/*
> + * 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.
> + */
>  extern const unsigned char *do_lookup_replace_object(const unsigned char *sha1);
> +
> +/*
> + * If object sha1 should be replaced, return the replacement object's
> + * name.  This function is similar to do_lookup_replace_object(),
> + * except that it when object replacement is suppressed, it always
> + * returns its argument unchanged.
> + */
>  static inline const unsigned char *lookup_replace_object(const unsigned char *sha1)
>  {
>  	if (!read_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))

The above description is good, but after reading e1111cef (inline
lookup_replace_object() calls, 2011-05-15) that introduced this
ugliness, I have to wonder if do_lookup_replace(), which nobody
except lookup_replace_object() ever calls, is better removed from
the public API, making lookup_replace_object() an extern definition.

We do name functions that are purely helpers that are internal
implementation detals of the API as "do_blah", but exporting that
kind of name as if that is part of the API people are expected to
call feels very wrong.

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

[Michael Haggerty]

On 02/21/2014 07:21 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>>  cache.h | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/cache.h b/cache.h
>> index dc040fb..0ecd1c8 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -788,13 +788,29 @@ static inline void *read_sha1_file(const unsigned char *sha1, enum object_type *
>>  {
>>  	return read_sha1_file_extended(sha1, type, size, LOOKUP_REPLACE_OBJECT);
>>  }
>> +
>> +/*
>> + * 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.
>> + */
>>  extern const unsigned char *do_lookup_replace_object(const unsigned char *sha1);
>> +
>> +/*
>> + * If object sha1 should be replaced, return the replacement object's
>> + * name.  This function is similar to do_lookup_replace_object(),
>> + * except that it when object replacement is suppressed, it always
>> + * returns its argument unchanged.
>> + */
>>  static inline const unsigned char *lookup_replace_object(const unsigned char *sha1)
>>  {
>>  	if (!read_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))
> 
> The above description is good, but after reading e1111cef (inline
> lookup_replace_object() calls, 2011-05-15) that introduced this
> ugliness, I have to wonder if do_lookup_replace(), which nobody
> except lookup_replace_object() ever calls, is better removed from
> the public API, making lookup_replace_object() an extern definition.
> 
> We do name functions that are purely helpers that are internal
> implementation detals of the API as "do_blah", but exporting that
> kind of name as if that is part of the API people are expected to
> call feels very wrong.

I assume that the current design was to avoid the overhead of a function
call in the case that no replace references exist.  If we're willing to
eat that cost, then sure, we should bury do_lookup_replace_object() in
the implementation file.

Unless you say otherwise, I will work that change into my patch series.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

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

[Christian Couder]

On Fri, Feb 21, 2014 at 5:32 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  cache.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index dc040fb..0ecd1c8 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -788,13 +788,29 @@ static inline void *read_sha1_file(const unsigned char *sha1, enum object_type *
>  {
>         return read_sha1_file_extended(sha1, type, size, LOOKUP_REPLACE_OBJECT);
>  }
> +
> +/*
> + * 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.

Here you talk about "check_replace_refs" ...

> + */
>  extern const unsigned char *do_lookup_replace_object(const unsigned char *sha1);
> +
> +/*
> + * If object sha1 should be replaced, return the replacement object's
> + * name.  This function is similar to do_lookup_replace_object(),
> + * except that it when object replacement is suppressed, it always
> + * returns its argument unchanged.
> + */
>  static inline const unsigned char *lookup_replace_object(const unsigned char *sha1)
>  {
>         if (!read_replace_refs)

... but here "read_replace_refs" is used.

>                 return sha1;
>         return do_lookup_replace_object(sha1);
>  }

Thanks,
Christian.

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

[Michael Haggerty]

On 02/24/2014 10:24 AM, Christian Couder wrote:
> On Fri, Feb 21, 2014 at 5:32 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>>  cache.h | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/cache.h b/cache.h
>> index dc040fb..0ecd1c8 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -788,13 +788,29 @@ static inline void *read_sha1_file(const unsigned char *sha1, enum object_type *
>>  {
>>         return read_sha1_file_extended(sha1, type, size, LOOKUP_REPLACE_OBJECT);
>>  }
>> +
>> +/*
>> + * 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.
> 
> Here you talk about "check_replace_refs" ...
> 
>> + */
>>  extern const unsigned char *do_lookup_replace_object(const unsigned char *sha1);
>> +
>> +/*
>> + * If object sha1 should be replaced, return the replacement object's
>> + * name.  This function is similar to do_lookup_replace_object(),
>> + * except that it when object replacement is suppressed, it always
>> + * returns its argument unchanged.
>> + */
>>  static inline const unsigned char *lookup_replace_object(const unsigned char *sha1)
>>  {
>>         if (!read_replace_refs)
> 
> ... but here "read_replace_refs" is used.
> 
>>                 return sha1;
>>         return do_lookup_replace_object(sha1);
>>  }

You're right; thanks for noticing.  I originally implemented this patch
on top of mh/replace-refs-variable-rename but then separated them after
all, in the hopes that the latter would be straightforward enough to be
merged quickly, before conflicting patch series appear.

Junio, what would be easiest for you?  I suggest that I rebase this
patch series back on top of mh/replace-refs-variable-rename when re-rolling.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

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

[Junio C Hamano]

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

> Junio, what would be easiest for you?  I suggest that I rebase this
> patch series back on top of mh/replace-refs-variable-rename when re-rolling.

Hmph, I suspect I do not care too deeply either way, as a mismerge
would be fairly obvious (nobody should be adding any new string
"read_replace_refs" or deleting existing ones and the linker would
catch it even if I don't), but having on top of the patch that
renames the variable would make sense.

Thanks.

[PATCH 2/6] replace_object: use struct members instead of an array

[Michael Haggerty]

Give the poor humans some names to help them make sense of things.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 replace_object.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/replace_object.c b/replace_object.c
index cdcaf8c..6fc3ff4 100644
--- a/replace_object.c
+++ b/replace_object.c
@@ -3,8 +3,13 @@
 #include "refs.h"
 #include "commit.h"
 
+/*
+ * An array of replacements.  The array is kept sorted by the original
+ * sha1.
+ */
 static struct replace_object {
-	unsigned char sha1[2][20];
+	unsigned char original[20];
+	unsigned char replacement[20];
 } **replace_object;
 
 static int replace_object_alloc, replace_object_nr;
@@ -12,7 +17,7 @@ static int replace_object_alloc, replace_object_nr;
 static const unsigned char *replace_sha1_access(size_t index, void *table)
 {
 	struct replace_object **replace = table;
-	return replace[index]->sha1[0];
+	return replace[index]->original;
 }
 
 static int replace_object_pos(const unsigned char *sha1)
@@ -24,7 +29,7 @@ static int replace_object_pos(const unsigned char *sha1)
 static int register_replace_object(struct replace_object *replace,
 				   int ignore_dups)
 {
-	int pos = replace_object_pos(replace->sha1[0]);
+	int pos = replace_object_pos(replace->original);
 
 	if (0 <= pos) {
 		if (ignore_dups)
@@ -60,14 +65,14 @@ static int register_replace_ref(const char *refname,
 	const char *hash = slash ? slash + 1 : refname;
 	struct replace_object *repl_obj = xmalloc(sizeof(*repl_obj));
 
-	if (strlen(hash) != 40 || get_sha1_hex(hash, repl_obj->sha1[0])) {
+	if (strlen(hash) != 40 || get_sha1_hex(hash, repl_obj->original)) {
 		free(repl_obj);
 		warning("bad replace ref name: %s", refname);
 		return 0;
 	}
 
 	/* Copy sha1 from the read ref */
-	hashcpy(repl_obj->sha1[1], sha1);
+	hashcpy(repl_obj->replacement, sha1);
 
 	/* Register new object */
 	if (register_replace_object(repl_obj, 1))
@@ -107,7 +112,7 @@ const unsigned char *do_lookup_replace_object(const unsigned char *sha1)
 
 		pos = replace_object_pos(cur);
 		if (0 <= pos)
-			cur = replace_object[pos]->sha1[1];
+			cur = replace_object[pos]->replacement;
 	} while (0 <= pos);
 
 	return cur;
-- 
1.8.5.3

Re: [PATCH 2/6] replace_object: use struct members instead of an array

[Junio C Hamano]

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

> Give the poor humans some names to help them make sense of things.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---

Good.

Reviewed-by: me.

>  replace_object.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/replace_object.c b/replace_object.c
> index cdcaf8c..6fc3ff4 100644
> --- a/replace_object.c
> +++ b/replace_object.c
> @@ -3,8 +3,13 @@
>  #include "refs.h"
>  #include "commit.h"
>  
> +/*
> + * An array of replacements.  The array is kept sorted by the original
> + * sha1.
> + */
>  static struct replace_object {
> -	unsigned char sha1[2][20];
> +	unsigned char original[20];
> +	unsigned char replacement[20];
>  } **replace_object;
>  
>  static int replace_object_alloc, replace_object_nr;
> @@ -12,7 +17,7 @@ static int replace_object_alloc, replace_object_nr;
>  static const unsigned char *replace_sha1_access(size_t index, void *table)
>  {
>  	struct replace_object **replace = table;
> -	return replace[index]->sha1[0];
> +	return replace[index]->original;
>  }
>  
>  static int replace_object_pos(const unsigned char *sha1)
> @@ -24,7 +29,7 @@ static int replace_object_pos(const unsigned char *sha1)
>  static int register_replace_object(struct replace_object *replace,
>  				   int ignore_dups)
>  {
> -	int pos = replace_object_pos(replace->sha1[0]);
> +	int pos = replace_object_pos(replace->original);
>  
>  	if (0 <= pos) {
>  		if (ignore_dups)
> @@ -60,14 +65,14 @@ static int register_replace_ref(const char *refname,
>  	const char *hash = slash ? slash + 1 : refname;
>  	struct replace_object *repl_obj = xmalloc(sizeof(*repl_obj));
>  
> -	if (strlen(hash) != 40 || get_sha1_hex(hash, repl_obj->sha1[0])) {
> +	if (strlen(hash) != 40 || get_sha1_hex(hash, repl_obj->original)) {
>  		free(repl_obj);
>  		warning("bad replace ref name: %s", refname);
>  		return 0;
>  	}
>  
>  	/* Copy sha1 from the read ref */
> -	hashcpy(repl_obj->sha1[1], sha1);
> +	hashcpy(repl_obj->replacement, sha1);
>  
>  	/* Register new object */
>  	if (register_replace_object(repl_obj, 1))
> @@ -107,7 +112,7 @@ const unsigned char *do_lookup_replace_object(const unsigned char *sha1)
>  
>  		pos = replace_object_pos(cur);
>  		if (0 <= pos)
> -			cur = replace_object[pos]->sha1[1];
> +			cur = replace_object[pos]->replacement;
>  	} while (0 <= pos);
>  
>  	return cur;

[PATCH 3/6] find_pack_entry(): document last_found_pack

[Michael Haggerty]

Add a comment at the declaration of last_found_pack and where it is
used in find_pack_entry().  In the latter, separate the cases (1) to
make a place for the new comment and (2) to turn the success case into
affirmative logic.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 sha1_file.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 6e8c05d..0910939 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -60,6 +60,12 @@ static struct cached_object empty_tree = {
 	0
 };
 
+/*
+ * A pointer to the last packed_git in which an object was found.
+ * When an object is sought, we look in this packfile first, because
+ * objects that are looked up at similar times are often in the same
+ * packfile as one another.
+ */
 static struct packed_git *last_found_pack;
 
 static struct cached_object *find_cached_object(const unsigned char *sha1)
@@ -2460,11 +2466,13 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)
 		return 1;
 
 	for (p = packed_git; p; p = p->next) {
-		if (p == last_found_pack || !fill_pack_entry(sha1, e, p))
-			continue;
+		if (p == last_found_pack)
+			continue; /* we already checked this one */
 
-		last_found_pack = p;
-		return 1;
+		if (fill_pack_entry(sha1, e, p)) {
+			last_found_pack = p;
+			return 1;
+		}
 	}
 	return 0;
 }
-- 
1.8.5.3

Re: [PATCH 3/6] find_pack_entry(): document last_found_pack

[Nicolas Pitre]

On Fri, 21 Feb 2014, Michael Haggerty wrote:

> Add a comment at the declaration of last_found_pack and where it is
> used in find_pack_entry().  In the latter, separate the cases (1) to
> make a place for the new comment and (2) to turn the success case into
> affirmative logic.
> 
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

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


> ---
>  sha1_file.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/sha1_file.c b/sha1_file.c
> index 6e8c05d..0910939 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -60,6 +60,12 @@ static struct cached_object empty_tree = {
>  	0
>  };
>  
> +/*
> + * A pointer to the last packed_git in which an object was found.
> + * When an object is sought, we look in this packfile first, because
> + * objects that are looked up at similar times are often in the same
> + * packfile as one another.
> + */
>  static struct packed_git *last_found_pack;
>  
>  static struct cached_object *find_cached_object(const unsigned char *sha1)
> @@ -2460,11 +2466,13 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)
>  		return 1;
>  
>  	for (p = packed_git; p; p = p->next) {
> -		if (p == last_found_pack || !fill_pack_entry(sha1, e, p))
> -			continue;
> +		if (p == last_found_pack)
> +			continue; /* we already checked this one */
>  
> -		last_found_pack = p;
> -		return 1;
> +		if (fill_pack_entry(sha1, e, p)) {
> +			last_found_pack = p;
> +			return 1;
> +		}
>  	}
>  	return 0;
>  }
> -- 
> 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
> 

[PATCH 4/6] sha1_file_name(): declare to return a const string

[Michael Haggerty]

Change the return value of sha1_file_name() to (const char *).
(Callers have no business mucking about here.)  Change callers
accordingly, deleting a few superfluous temporary variables along the
way.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 cache.h     |  2 +-
 http.c      |  2 +-
 sha1_file.c | 24 +++++++++---------------
 3 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/cache.h b/cache.h
index 0ecd1c8..1663478 100644
--- a/cache.h
+++ b/cache.h
@@ -659,7 +659,7 @@ extern char *git_path(const char *fmt, ...) __attribute__((format (printf, 1, 2)
 extern char *git_path_submodule(const char *path, const char *fmt, ...)
 	__attribute__((format (printf, 2, 3)));
 
-extern char *sha1_file_name(const unsigned char *sha1);
+extern const char *sha1_file_name(const unsigned char *sha1);
 extern char *sha1_pack_name(const unsigned char *sha1);
 extern char *sha1_pack_index_name(const unsigned char *sha1);
 extern const char *find_unique_abbrev(const unsigned char *sha1, int);
diff --git a/http.c b/http.c
index 70eaa26..faa9dc8 100644
--- a/http.c
+++ b/http.c
@@ -1384,7 +1384,7 @@ struct http_object_request *new_http_object_request(const char *base_url,
 	unsigned char *sha1)
 {
 	char *hex = sha1_to_hex(sha1);
-	char *filename;
+	const char *filename;
 	char prevfile[PATH_MAX];
 	int prevlocal;
 	char prev_buf[PREV_BUF_SIZE];
diff --git a/sha1_file.c b/sha1_file.c
index 0910939..ba62804 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -194,7 +194,7 @@ static void fill_sha1_path(char *pathbuf, const unsigned char *sha1)
  * DB_ENVIRONMENT environment variable if it is not found in
  * the primary object database.
  */
-char *sha1_file_name(const unsigned char *sha1)
+const char *sha1_file_name(const unsigned char *sha1)
 {
 	static char buf[PATH_MAX];
 	const char *objdir;
@@ -444,8 +444,7 @@ void prepare_alt_odb(void)
 
 static int has_loose_object_local(const unsigned char *sha1)
 {
-	char *name = sha1_file_name(sha1);
-	return !access(name, F_OK);
+	return !access(sha1_file_name(sha1), F_OK);
 }
 
 int has_loose_object_nonlocal(const unsigned char *sha1)
@@ -1420,17 +1419,15 @@ static int git_open_noatime(const char *name)
 
 static int stat_sha1_file(const unsigned char *sha1, struct stat *st)
 {
-	char *name = sha1_file_name(sha1);
 	struct alternate_object_database *alt;
 
-	if (!lstat(name, st))
+	if (!lstat(sha1_file_name(sha1), st))
 		return 0;
 
 	prepare_alt_odb();
 	errno = ENOENT;
 	for (alt = alt_odb_list; alt; alt = alt->next) {
-		name = alt->name;
-		fill_sha1_path(name, sha1);
+		fill_sha1_path(alt->name, sha1);
 		if (!lstat(alt->base, st))
 			return 0;
 	}
@@ -1441,18 +1438,16 @@ static int stat_sha1_file(const unsigned char *sha1, struct stat *st)
 static int open_sha1_file(const unsigned char *sha1)
 {
 	int fd;
-	char *name = sha1_file_name(sha1);
 	struct alternate_object_database *alt;
 
-	fd = git_open_noatime(name);
+	fd = git_open_noatime(sha1_file_name(sha1));
 	if (fd >= 0)
 		return fd;
 
 	prepare_alt_odb();
 	errno = ENOENT;
 	for (alt = alt_odb_list; alt; alt = alt->next) {
-		name = alt->name;
-		fill_sha1_path(name, sha1);
+		fill_sha1_path(alt->name, sha1);
 		fd = git_open_noatime(alt->base);
 		if (fd >= 0)
 			return fd;
@@ -2687,7 +2682,6 @@ void *read_sha1_file_extended(const unsigned char *sha1,
 			      unsigned flag)
 {
 	void *data;
-	char *path;
 	const struct packed_git *p;
 	const unsigned char *repl = lookup_replace_object_extended(sha1, flag);
 
@@ -2705,7 +2699,8 @@ void *read_sha1_file_extended(const unsigned char *sha1,
 		    sha1_to_hex(repl), sha1_to_hex(sha1));
 
 	if (has_loose_object(repl)) {
-		path = sha1_file_name(sha1);
+		const char *path = sha1_file_name(sha1);
+
 		die("loose object %s (stored in %s) is corrupt",
 		    sha1_to_hex(repl), path);
 	}
@@ -2903,10 +2898,9 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,
 	git_zstream stream;
 	git_SHA_CTX c;
 	unsigned char parano_sha1[20];
-	char *filename;
 	static char tmp_file[PATH_MAX];
+	const char *filename = sha1_file_name(sha1);
 
-	filename = sha1_file_name(sha1);
 	fd = create_tmpfile(tmp_file, sizeof(tmp_file), filename);
 	if (fd < 0) {
 		if (errno == EACCES)
-- 
1.8.5.3

[PATCH 5/6] Document a bunch of functions defined in sha1_file.c

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 cache.h     | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 sha1_file.c | 26 +++++++++++++-----------
 2 files changed, 78 insertions(+), 14 deletions(-)

diff --git a/cache.h b/cache.h
index 1663478..e62fdec 100644
--- a/cache.h
+++ b/cache.h
@@ -659,9 +659,28 @@ extern char *git_path(const char *fmt, ...) __attribute__((format (printf, 1, 2)
 extern char *git_path_submodule(const char *path, const char *fmt, ...)
 	__attribute__((format (printf, 2, 3)));
 
+/*
+ * Return the name of the file in the local object database that would
+ * be used to store a loose object with the specified sha1.  The
+ * return value is a pointer to a statically allocated buffer that is
+ * overwritten each time the function is called.
+ */
 extern const char *sha1_file_name(const unsigned char *sha1);
+
+/*
+ * Return the name of the (local) packfile with the specified sha1 in
+ * its name.  The return value is a pointer to memory that is
+ * overwritten each time this function is called.
+ */
 extern char *sha1_pack_name(const unsigned char *sha1);
+
+/*
+ * Return the name of the (local) pack index file with the specified
+ * sha1 in its name.  The return value is a pointer to memory that is
+ * overwritten each time this function is called.
+ */
 extern char *sha1_pack_index_name(const unsigned char *sha1);
+
 extern const char *find_unique_abbrev(const unsigned char *sha1, int);
 extern const unsigned char null_sha1[20];
 
@@ -836,7 +855,19 @@ extern int check_sha1_signature(const unsigned char *sha1, void *buf, unsigned l
 extern int move_temp_to_file(const char *tmpfile, const char *filename);
 
 extern int has_sha1_pack(const unsigned char *sha1);
+
+/*
+ * Return true iff we have an object named sha1, whether local or in
+ * an alternate object database, and whether packed or loose.  This
+ * function does not respect replace references.
+ */
 extern int has_sha1_file(const unsigned char *sha1);
+
+/*
+ * Return true iff an alternate object database has a loose object
+ * with the specified name.  This function does not respect replace
+ * references.
+ */
 extern int has_loose_object_nonlocal(const unsigned char *sha1);
 
 extern int has_pack_index(const unsigned char *sha1);
@@ -1099,17 +1130,46 @@ extern struct packed_git *find_sha1_pack(const unsigned char *sha1,
 					 struct packed_git *packs);
 
 extern void pack_report(void);
+
+/*
+ * mmap the index file for the specified packfile (if it is not
+ * already mmapped).  Return 0 on success.
+ */
 extern int open_pack_index(struct packed_git *);
+
+/*
+ * munmap the index file for the specified packfile (if it is
+ * currently mmapped).
+ */
 extern void close_pack_index(struct packed_git *);
+
 extern unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, unsigned long *);
 extern void close_pack_windows(struct packed_git *);
 extern void unuse_pack(struct pack_window **);
 extern void free_pack_by_name(const char *);
 extern void clear_delta_base_cache(void);
 extern struct packed_git *add_packed_git(const char *, int, int);
-extern const unsigned char *nth_packed_object_sha1(struct packed_git *, uint32_t);
-extern off_t nth_packed_object_offset(const struct packed_git *, uint32_t);
-extern off_t find_pack_entry_one(const unsigned char *, struct packed_git *);
+
+/*
+ * Return the SHA-1 of the nth object within the specified packfile.
+ * Open the index if it is not already open.  The return value points
+ * at the SHA-1 within the mmapped index.  Return NULL if there is an
+ * error.
+ */
+extern const unsigned char *nth_packed_object_sha1(struct packed_git *, uint32_t n);
+
+/*
+ * Return the offset of the nth object within the specified packfile.
+ * The index must already be opened.
+ */
+extern off_t nth_packed_object_offset(const struct packed_git *, uint32_t n);
+
+/*
+ * If the object named sha1 is present in the specified packfile,
+ * return its offset within the packfile; otherwise, return 0.
+ */
+extern off_t find_pack_entry_one(const unsigned char *sha1, struct packed_git *);
+
 extern int is_pack_valid(struct packed_git *);
 extern void *unpack_entry(struct packed_git *, off_t, enum object_type *, unsigned long *);
 extern unsigned long unpack_object_header_buffer(const unsigned char *buf, unsigned long len, enum object_type *type, unsigned long *sizep);
diff --git a/sha1_file.c b/sha1_file.c
index ba62804..bb9f097 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -184,16 +184,6 @@ static void fill_sha1_path(char *pathbuf, const unsigned char *sha1)
 	}
 }
 
-/*
- * NOTE! This returns a statically allocated buffer, so you have to be
- * careful about using it. Do an "xstrdup()" if you need to save the
- * filename.
- *
- * Also note that this returns the location for creating.  Reading
- * SHA1 file can happen from any alternate directory listed in the
- * DB_ENVIRONMENT environment variable if it is not found in
- * the primary object database.
- */
 const char *sha1_file_name(const unsigned char *sha1)
 {
 	static char buf[PATH_MAX];
@@ -214,6 +204,11 @@ const char *sha1_file_name(const unsigned char *sha1)
 	return buf;
 }
 
+/*
+ * Return the name of the pack or index file with the specified sha1
+ * in its filename.  *base and *name are scratch space that must be
+ * provided by the caller.  which should be "pack" or "idx".
+ */
 static char *sha1_get_pack_name(const unsigned char *sha1,
 				char **name, char **base, const char *which)
 {
@@ -496,7 +491,12 @@ void pack_report(void)
 		sz_fmt(pack_mapped), sz_fmt(peak_pack_mapped));
 }
 
-static int check_packed_git_idx(const char *path,  struct packed_git *p)
+/*
+ * Open and mmap the index file at path, perform a couple of
+ * consistency checks, then record its information to p.  Return 0 on
+ * success.
+ */
+static int check_packed_git_idx(const char *path, struct packed_git *p)
 {
 	void *idx_map;
 	struct pack_idx_header *hdr;
@@ -2449,6 +2449,10 @@ static int fill_pack_entry(const unsigned char *sha1,
 	return 1;
 }
 
+/*
+ * Iff a pack file contains the object named by sha1, return true and
+ * store its location to e.
+ */
 static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)
 {
 	struct packed_git *p;
-- 
1.8.5.3

Re: [PATCH 5/6] Document a bunch of functions defined in sha1_file.c

[Nicolas Pitre]

On Fri, 21 Feb 2014, Michael Haggerty wrote:

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

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


> ---
>  cache.h     | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  sha1_file.c | 26 +++++++++++++-----------
>  2 files changed, 78 insertions(+), 14 deletions(-)
> 
> diff --git a/cache.h b/cache.h
> index 1663478..e62fdec 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -659,9 +659,28 @@ extern char *git_path(const char *fmt, ...) __attribute__((format (printf, 1, 2)
>  extern char *git_path_submodule(const char *path, const char *fmt, ...)
>  	__attribute__((format (printf, 2, 3)));
>  
> +/*
> + * Return the name of the file in the local object database that would
> + * be used to store a loose object with the specified sha1.  The
> + * return value is a pointer to a statically allocated buffer that is
> + * overwritten each time the function is called.
> + */
>  extern const char *sha1_file_name(const unsigned char *sha1);
> +
> +/*
> + * Return the name of the (local) packfile with the specified sha1 in
> + * its name.  The return value is a pointer to memory that is
> + * overwritten each time this function is called.
> + */
>  extern char *sha1_pack_name(const unsigned char *sha1);
> +
> +/*
> + * Return the name of the (local) pack index file with the specified
> + * sha1 in its name.  The return value is a pointer to memory that is
> + * overwritten each time this function is called.
> + */
>  extern char *sha1_pack_index_name(const unsigned char *sha1);
> +
>  extern const char *find_unique_abbrev(const unsigned char *sha1, int);
>  extern const unsigned char null_sha1[20];
>  
> @@ -836,7 +855,19 @@ extern int check_sha1_signature(const unsigned char *sha1, void *buf, unsigned l
>  extern int move_temp_to_file(const char *tmpfile, const char *filename);
>  
>  extern int has_sha1_pack(const unsigned char *sha1);
> +
> +/*
> + * Return true iff we have an object named sha1, whether local or in
> + * an alternate object database, and whether packed or loose.  This
> + * function does not respect replace references.
> + */
>  extern int has_sha1_file(const unsigned char *sha1);
> +
> +/*
> + * Return true iff an alternate object database has a loose object
> + * with the specified name.  This function does not respect replace
> + * references.
> + */
>  extern int has_loose_object_nonlocal(const unsigned char *sha1);
>  
>  extern int has_pack_index(const unsigned char *sha1);
> @@ -1099,17 +1130,46 @@ extern struct packed_git *find_sha1_pack(const unsigned char *sha1,
>  					 struct packed_git *packs);
>  
>  extern void pack_report(void);
> +
> +/*
> + * mmap the index file for the specified packfile (if it is not
> + * already mmapped).  Return 0 on success.
> + */
>  extern int open_pack_index(struct packed_git *);
> +
> +/*
> + * munmap the index file for the specified packfile (if it is
> + * currently mmapped).
> + */
>  extern void close_pack_index(struct packed_git *);
> +
>  extern unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, unsigned long *);
>  extern void close_pack_windows(struct packed_git *);
>  extern void unuse_pack(struct pack_window **);
>  extern void free_pack_by_name(const char *);
>  extern void clear_delta_base_cache(void);
>  extern struct packed_git *add_packed_git(const char *, int, int);
> -extern const unsigned char *nth_packed_object_sha1(struct packed_git *, uint32_t);
> -extern off_t nth_packed_object_offset(const struct packed_git *, uint32_t);
> -extern off_t find_pack_entry_one(const unsigned char *, struct packed_git *);
> +
> +/*
> + * Return the SHA-1 of the nth object within the specified packfile.
> + * Open the index if it is not already open.  The return value points
> + * at the SHA-1 within the mmapped index.  Return NULL if there is an
> + * error.
> + */
> +extern const unsigned char *nth_packed_object_sha1(struct packed_git *, uint32_t n);
> +
> +/*
> + * Return the offset of the nth object within the specified packfile.
> + * The index must already be opened.
> + */
> +extern off_t nth_packed_object_offset(const struct packed_git *, uint32_t n);
> +
> +/*
> + * If the object named sha1 is present in the specified packfile,
> + * return its offset within the packfile; otherwise, return 0.
> + */
> +extern off_t find_pack_entry_one(const unsigned char *sha1, struct packed_git *);
> +
>  extern int is_pack_valid(struct packed_git *);
>  extern void *unpack_entry(struct packed_git *, off_t, enum object_type *, unsigned long *);
>  extern unsigned long unpack_object_header_buffer(const unsigned char *buf, unsigned long len, enum object_type *type, unsigned long *sizep);
> diff --git a/sha1_file.c b/sha1_file.c
> index ba62804..bb9f097 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -184,16 +184,6 @@ static void fill_sha1_path(char *pathbuf, const unsigned char *sha1)
>  	}
>  }
>  
> -/*
> - * NOTE! This returns a statically allocated buffer, so you have to be
> - * careful about using it. Do an "xstrdup()" if you need to save the
> - * filename.
> - *
> - * Also note that this returns the location for creating.  Reading
> - * SHA1 file can happen from any alternate directory listed in the
> - * DB_ENVIRONMENT environment variable if it is not found in
> - * the primary object database.
> - */
>  const char *sha1_file_name(const unsigned char *sha1)
>  {
>  	static char buf[PATH_MAX];
> @@ -214,6 +204,11 @@ const char *sha1_file_name(const unsigned char *sha1)
>  	return buf;
>  }
>  
> +/*
> + * Return the name of the pack or index file with the specified sha1
> + * in its filename.  *base and *name are scratch space that must be
> + * provided by the caller.  which should be "pack" or "idx".
> + */
>  static char *sha1_get_pack_name(const unsigned char *sha1,
>  				char **name, char **base, const char *which)
>  {
> @@ -496,7 +491,12 @@ void pack_report(void)
>  		sz_fmt(pack_mapped), sz_fmt(peak_pack_mapped));
>  }
>  
> -static int check_packed_git_idx(const char *path,  struct packed_git *p)
> +/*
> + * Open and mmap the index file at path, perform a couple of
> + * consistency checks, then record its information to p.  Return 0 on
> + * success.
> + */
> +static int check_packed_git_idx(const char *path, struct packed_git *p)
>  {
>  	void *idx_map;
>  	struct pack_idx_header *hdr;
> @@ -2449,6 +2449,10 @@ static int fill_pack_entry(const unsigned char *sha1,
>  	return 1;
>  }
>  
> +/*
> + * Iff a pack file contains the object named by sha1, return true and
> + * store its location to e.
> + */
>  static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)
>  {
>  	struct packed_git *p;
> -- 
> 1.8.5.3
> 

Re: [PATCH 5/6] Document a bunch of functions defined in sha1_file.c

[Jakub Narębski]

Michael Haggerty wrote:

> -/*
> - * NOTE! This returns a statically allocated buffer, so you have to be
> - * careful about using it. Do an "xstrdup()" if you need to save the
> - * filename.
> - *
> - * Also note that this returns the location for creating.  Reading
> - * SHA1 file can happen from any alternate directory listed in the
> - * DB_ENVIRONMENT environment variable if it is not found in
> - * the primary object database.
> - */
>   const char *sha1_file_name(const unsigned char *sha1)

Has this changed?

-- 
Jakub Narębski

Re: [PATCH 5/6] Document a bunch of functions defined in sha1_file.c

[Michael Haggerty]

On 02/24/2014 07:18 PM, Jakub Narębski wrote:
> Michael Haggerty wrote:
> 
>> -/*
>> - * NOTE! This returns a statically allocated buffer, so you have to be
>> - * careful about using it. Do an "xstrdup()" if you need to save the
>> - * filename.
>> - *
>> - * Also note that this returns the location for creating.  Reading
>> - * SHA1 file can happen from any alternate directory listed in the
>> - * DB_ENVIRONMENT environment variable if it is not found in
>> - * the primary object database.
>> - */
>>   const char *sha1_file_name(const unsigned char *sha1)
> 
> Has this changed?

No, this hasn't changed.  I've been documenting public functions in the
header files above the declaration, and private ones where they are
defined.  So I moved the documentation for this function to cache.h:

+/*
+ * Return the name of the file in the local object database that would
+ * be used to store a loose object with the specified sha1.  The
+ * return value is a pointer to a statically allocated buffer that is
+ * overwritten each time the function is called.
+ */
 extern const char *sha1_file_name(const unsigned char *sha1);

I also rewrite the comment, as you can see.  The "NOTE!" seemed a bit
overboard to me, given that there are a lot of functions in our codebase
that behave similarly.  So I toned the warning down, and tightened up
the comment overall.

Let me know if you think I've made it less helpful.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH 5/6] Document a bunch of functions defined in sha1_file.c

[Jonathan Nieder]

Hi,

Michael Haggerty wrote:

> No, this hasn't changed.  I've been documenting public functions in the
> header files above the declaration, and private ones where they are
> defined.  So I moved the documentation for this function to cache.h:
>
> +/*
> + * Return the name of the file in the local object database that would
> + * be used to store a loose object with the specified sha1.  The
> + * return value is a pointer to a statically allocated buffer that is
> + * overwritten each time the function is called.
> + */
>  extern const char *sha1_file_name(const unsigned char *sha1);
>
> I also rewrite the comment, as you can see.  The "NOTE!" seemed a bit
> overboard to me, given that there are a lot of functions in our codebase
> that behave similarly.  So I toned the warning down, and tightened up
> the comment overall.
>
> Let me know if you think I've made it less helpful.

In the present state of the codebase, where many important functions
have no documentation or out-of-date documentation, the first place I
look to understand a function is its point of definition.  So I do
think that moving docs to the header file makes it less helpful.  I'd
prefer a mass move in the opposite direction (from header files to the
point of definition).

On the other hand I don't feel strongly about it.

Hope that helps,
Jonathan

Re: [PATCH 5/6] Document a bunch of functions defined in sha1_file.c

[Michael Haggerty]

On 02/24/2014 09:08 PM, Jonathan Nieder wrote:
> Michael Haggerty wrote:
>> [...]  I've been documenting public functions in the
>> header files above the declaration, and private ones where they are
>> defined.  [...]
>>
>> Let me know if you think I've made it less helpful.
> 
> In the present state of the codebase, where many important functions
> have no documentation or out-of-date documentation, the first place I
> look to understand a function is its point of definition.  So I do
> think that moving docs to the header file makes it less helpful.  I'd
> prefer a mass move in the opposite direction (from header files to the
> point of definition).
> 
> On the other hand I don't feel strongly about it.

Jonathan,

I see your point.  But I'd rather that we, as a project, strive to make
our header files good tables of contents of the publicly-accessible
functionality, including decent documentation for each function.  I try
to add comments to everything I touch, and I wish other developers would
too.

[What we really need is a comment fascist who patrols patch submissions
making sure that they add docstrings for new functions.  If I only had
the time and the jackboots for it...]

So I'd rather leave the comments for public functions in the header
files.  But if other regular developers prefer that comments be by the
function definitions, of course I can live with that, too.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

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

[Michael Haggerty]

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

diff --git a/object.c b/object.c
index 584f7ac..c34e225 100644
--- a/object.c
+++ b/object.c
@@ -43,14 +43,26 @@ 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.
+ *
+ * Since the sha1 is essentially random, we just take the required
+ * bits from the first sizeof(unsigned int) bytes of sha1.
+ */
 static unsigned int hash_obj(const unsigned char *sha1, unsigned int n)
 {
 	unsigned int hash;
+
 	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 +75,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 +108,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 6/6] Document some functions defined in object.c

[Nicolas Pitre]

On Fri, 21 Feb 2014, Michael Haggerty wrote:

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

Minor nits below.


> ---
>  object.c | 23 ++++++++++++++++++++++-
>  object.h |  7 +++++++
>  2 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/object.c b/object.c
> index 584f7ac..c34e225 100644
> --- a/object.c
> +++ b/object.c
> @@ -43,14 +43,26 @@ 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.
> + *
> + * Since the sha1 is essentially random, we just take the required
> + * bits from the first sizeof(unsigned int) bytes of sha1.

This might be improved a little.  The only reason for the sizeof() is 
actually to copy those bits into a properly aligned integer.  Some 
architectures have alignment restrictions that incure a significant cost 
when integer operations are performed on unaligned data whereas sha1 
pointers don't have any particular alignment requirements.  Once upon a 
time this used to simply be:

	return *(unsigned int *)sha1 & (n - 1);

The memcpy is there only to avoid unaligned accesses.


> + */
>  static unsigned int hash_obj(const unsigned char *sha1, unsigned int n)
>  {
>  	unsigned int hash;
> +
>  	memcpy(&hash, sha1, sizeof(unsigned int));
> -	/* Assumes power-of-2 hash sizes in grow_object_hash */
>  	return hash & (n - 1);
>  }

Other than that...

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



>  
> +/*
> + * 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 +75,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 +108,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 6/6] Document some functions defined in object.c

[Michael Haggerty]

Nicolas, thanks for the very fast feedback!

On 02/21/2014 06:33 PM, Nicolas Pitre wrote:
> On Fri, 21 Feb 2014, Michael Haggerty wrote:
> 
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> 
> Minor nits below.
> 
> 
>> ---
>>  object.c | 23 ++++++++++++++++++++++-
>>  object.h |  7 +++++++
>>  2 files changed, 29 insertions(+), 1 deletion(-)
>>
>> diff --git a/object.c b/object.c
>> index 584f7ac..c34e225 100644
>> --- a/object.c
>> +++ b/object.c
>> @@ -43,14 +43,26 @@ 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.
>> + *
>> + * Since the sha1 is essentially random, we just take the required
>> + * bits from the first sizeof(unsigned int) bytes of sha1.
> 
> This might be improved a little.  The only reason for the sizeof() is 
> actually to copy those bits into a properly aligned integer.  Some 
> architectures have alignment restrictions that incure a significant cost 
> when integer operations are performed on unaligned data whereas sha1 
> pointers don't have any particular alignment requirements.  Once upon a 
> time this used to simply be:
> 
> 	return *(unsigned int *)sha1 & (n - 1);
> 
> The memcpy is there only to avoid unaligned accesses.

I understand all that; it's clear that the old code is not correct C,
isn't it?  And ISTM that the use of memcpy() is an implementation detail
that is not relevant to callers and so not needed in the docstring.  So
I suggest that your note be added as comments within the function; what
do you think?

Contrariwise, I thought about it again and believe that it *is*
important for the docstring to mention explicitly that the return value
is architecture-dependent (it depends on endianness and possibly
sizeof(unsigned int)).  Presumably the function is only used within one
Git invocation, so this isn't a problem, but we should warn callers.
Alternatively, we could stick a call to ntohl() in the function to make
the return value consistent, but this would cost a little bit on
little-endian computers.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

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

[Junio C Hamano]

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

> Contrariwise, I thought about it again and believe that it *is*
> important for the docstring to mention explicitly that the return value
> is architecture-dependent

As it gives an internal hash value which should not leak to the
outside world (e.g. stored in a file or sent over the wire later to
be read by other platforms), I think that is a good idea.

Re: [PATCH 0/6] Add a bunch of docstrings and make a few minor cleanups

[Junio C Hamano]

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

> I was reading around in the neighborhood of fsck, objects, and packs
> and I had the familiar and discouraging experience of having to read
> code all the way up and down the callstack to understand *anything*.
> Please let's all make more of an effort to document functions,
> especially things that are not obvious from the name and signature,
> like who owns the memory that is being referred to.
>
> This is my attempt to document a number of the functions that I was
> looking at based on what I inferred from my reading.  It is also a
> selfish trick to get other people to double-check my understanding.
>
> I also fixed up a couple of small things that I noticed along the way:
> "refactoring for understanding".
>
> Michael Haggerty (6):
>   Add docstrings for lookup_replace_object() and
>     do_lookup_replace_object()
>   replace_object: use struct members instead of an array
>   find_pack_entry(): document last_found_pack
>   sha1_file_name(): declare to return a const string
>   Document a bunch of functions defined in sha1_file.c
>   Document some functions defined in object.c

Queued 2, 3, 4, and 5, expecting 1 and 6 will be rerolled.

Thanks.

>
>  cache.h          | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  http.c           |  2 +-
>  object.c         | 23 +++++++++++++++-
>  object.h         |  7 +++++
>  replace_object.c | 17 ++++++++----
>  sha1_file.c      | 66 ++++++++++++++++++++++++--------------------
>  6 files changed, 157 insertions(+), 42 deletions(-)