Re: [PATCH 5/6] Document a bunch of functions defined in sha1_file.c
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 mhag...@alum.mit.edu -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/6] Document a bunch of functions defined in sha1_file.c
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/6] Document a bunch of functions defined in sha1_file.c
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 mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/6] Document a bunch of functions defined in sha1_file.c
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/6] Document a bunch of functions defined in sha1_file.c
On Fri, 21 Feb 2014, Michael Haggerty wrote: Signed-off-by: Michael Haggerty mhag...@alum.mit.edu Acked-by: Nicolas Pitre n...@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
