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
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 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
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 Fri, 21 Feb 2014, Michael Haggerty wrote: > Signed-off-by: Michael Haggerty Acked-by: Nicolas Pitre > --- > 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 statical
[PATCH 5/6] Document a bunch of functions defined in sha1_file.c
Signed-off-by: Michael Haggerty --- 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 - *
