Re: [RFC PATCH 01/10] pack: move pack name-related functions
On Fri, 11 Aug 2017 14:34:27 -0700 Junio C Hamano wrote: > Ben Peart writes: > > > On 8/9/2017 1:16 PM, Jonathan Tan wrote: > > > >> Ah, I forgot to mention this in the cover letter. I thought that one > >> header was sufficient to cover all pack-related things, so if we wanted > >> to know which files used pack-related things, we would only need to > >> search for one string instead of two. Also, the division between > >> "pack.h" and the hypothetical "packfile.h" was not so clear to me. > > > > I prefer having source and the header files that export the functions > > have matching names to make it easy to find them. I would prefer > > packfile.h vs pack.h myself. > > Meaning "If we have packfile.c, packfile.h is preferrable over pack.h"? > I tend to agree with that. Fair enough - I've changed it so that the functions now go into packfile.h. I'll send it out once I know what to base it on (at least jt/sha1-file-cleanup, and a few more branches that also modify sha1_file.c).
Re: [RFC PATCH 01/10] pack: move pack name-related functions
Ben Peart writes: > On 8/9/2017 1:16 PM, Jonathan Tan wrote: > >> Ah, I forgot to mention this in the cover letter. I thought that one >> header was sufficient to cover all pack-related things, so if we wanted >> to know which files used pack-related things, we would only need to >> search for one string instead of two. Also, the division between >> "pack.h" and the hypothetical "packfile.h" was not so clear to me. > > I prefer having source and the header files that export the functions > have matching names to make it easy to find them. I would prefer > packfile.h vs pack.h myself. Meaning "If we have packfile.c, packfile.h is preferrable over pack.h"? I tend to agree with that.
Re: [RFC PATCH 01/10] pack: move pack name-related functions
On 8/9/2017 1:16 PM, Jonathan Tan wrote: On Wed, 9 Aug 2017 14:00:40 +0200 Christian Couder wrote: On Tue, Aug 8, 2017 at 10:50 PM, Jonathan Tan wrote: On Tue, 8 Aug 2017 13:36:24 -0700 Stefan Beller wrote: There are also packed refs, so one could (like I did) think that pack.c is for generic packing of things, maybe packfile.c would be more clear? Good point. I'll use packfile.c and packfile.h in the next version. It looks like you used "packfile.c" and "pack.h" in v2. Is there a reason why it's not using "packfile.h"? Ah, I forgot to mention this in the cover letter. I thought that one header was sufficient to cover all pack-related things, so if we wanted to know which files used pack-related things, we would only need to search for one string instead of two. Also, the division between "pack.h" and the hypothetical "packfile.h" was not so clear to me. I prefer having source and the header files that export the functions have matching names to make it easy to find them. I would prefer packfile.h vs pack.h myself.
Re: [RFC PATCH 01/10] pack: move pack name-related functions
On Wed, 9 Aug 2017 14:00:40 +0200 Christian Couder wrote: > On Tue, Aug 8, 2017 at 10:50 PM, Jonathan Tan > wrote: > > On Tue, 8 Aug 2017 13:36:24 -0700 > > Stefan Beller wrote: > >> > >> There are also packed refs, so one could (like I did) think that > >> pack.c is for generic packing of things, maybe packfile.c > >> would be more clear? > > > > Good point. I'll use packfile.c and packfile.h in the next version. > > It looks like you used "packfile.c" and "pack.h" in v2. Is there a > reason why it's not using "packfile.h"? Ah, I forgot to mention this in the cover letter. I thought that one header was sufficient to cover all pack-related things, so if we wanted to know which files used pack-related things, we would only need to search for one string instead of two. Also, the division between "pack.h" and the hypothetical "packfile.h" was not so clear to me.
Re: [RFC PATCH 01/10] pack: move pack name-related functions
On Tue, Aug 8, 2017 at 10:50 PM, Jonathan Tan wrote: > On Tue, 8 Aug 2017 13:36:24 -0700 > Stefan Beller wrote: >> >> There are also packed refs, so one could (like I did) think that >> pack.c is for generic packing of things, maybe packfile.c >> would be more clear? > > Good point. I'll use packfile.c and packfile.h in the next version. It looks like you used "packfile.c" and "pack.h" in v2. Is there a reason why it's not using "packfile.h"?
Re: [RFC PATCH 01/10] pack: move pack name-related functions
On Tue, 8 Aug 2017 13:36:24 -0700 Stefan Beller wrote: > On Tue, Aug 8, 2017 at 12:32 PM, Jonathan Tan > wrote: > > Currently, sha1_file.c and cache.h contain many functions, both related > > to and unrelated to packfiles. This makes both files very large and > > causes an unclear separation of concerns. > > > > Create a new file, pack.c, to hold all packfile-related functions > > currently in sha1_file.c, and designate pack.h to hold these > > packfile-related functions. > > There are also packed refs, so one could (like I did) think that > pack.c is for generic packing of things, maybe packfile.c > would be more clear? Good point. I'll use packfile.c and packfile.h in the next version.
Re: [RFC PATCH 01/10] pack: move pack name-related functions
On Tue, Aug 8, 2017 at 12:32 PM, Jonathan Tan wrote: > Currently, sha1_file.c and cache.h contain many functions, both related > to and unrelated to packfiles. This makes both files very large and > causes an unclear separation of concerns. > > Create a new file, pack.c, to hold all packfile-related functions > currently in sha1_file.c, and designate pack.h to hold these > packfile-related functions. There are also packed refs, so one could (like I did) think that pack.c is for generic packing of things, maybe packfile.c would be more clear?
[RFC PATCH 01/10] pack: move pack name-related functions
Currently, sha1_file.c and cache.h contain many functions, both related
to and unrelated to packfiles. This makes both files very large and
causes an unclear separation of concerns.
Create a new file, pack.c, to hold all packfile-related functions
currently in sha1_file.c, and designate pack.h to hold these
packfile-related functions.
In this commit, the pack name-related functions are moved. Subsequent
commits will move the other functions.
Signed-off-by: Jonathan Tan
---
Makefile | 1 +
builtin/pack-redundant.c | 1 +
cache.h | 23 ---
pack.c | 23 +++
pack.h | 23 +++
sha1_file.c | 22 --
6 files changed, 48 insertions(+), 45 deletions(-)
create mode 100644 pack.c
diff --git a/Makefile b/Makefile
index 461c845d3..a7b901a18 100644
--- a/Makefile
+++ b/Makefile
@@ -816,6 +816,7 @@ LIB_OBJS += notes-merge.o
LIB_OBJS += notes-utils.o
LIB_OBJS += object.o
LIB_OBJS += oidset.o
+LIB_OBJS += pack.o
LIB_OBJS += pack-bitmap.o
LIB_OBJS += pack-bitmap-write.o
LIB_OBJS += pack-check.o
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index cb1df1c76..df36d10e7 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -7,6 +7,7 @@
*/
#include "builtin.h"
+#include "pack.h"
#define BLKSIZE 512
diff --git a/cache.h b/cache.h
index 71fe09264..1f0f47819 100644
--- a/cache.h
+++ b/cache.h
@@ -902,20 +902,6 @@ extern void check_repository_format(void);
*/
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);
-
/*
* Return an abbreviated sha1 unique within this repository's object database.
* The result will be at least `len` characters long, and will be NUL
@@ -1648,15 +1634,6 @@ extern void pack_report(void);
*/
extern int odb_mkstemp(struct strbuf *template, const char *pattern);
-/*
- * Generate the filename to be used for a pack file with checksum "sha1" and
- * extension "ext". The result is written into the strbuf "buf", overwriting
- * any existing contents. A pointer to buf->buf is returned as a convenience.
- *
- * Example: odb_pack_name(out, sha1, "idx") =>
".git/objects/pack/pack-1234..idx"
- */
-extern char *odb_pack_name(struct strbuf *buf, const unsigned char *sha1,
const char *ext);
-
/*
* Create a pack .keep file named "name" (which should generally be the output
* of odb_pack_name). Returns a file descriptor opened for writing, or -1 on
diff --git a/pack.c b/pack.c
new file mode 100644
index 0..0d191dfd6
--- /dev/null
+++ b/pack.c
@@ -0,0 +1,23 @@
+#include "cache.h"
+
+char *odb_pack_name(struct strbuf *buf,
+ const unsigned char *sha1,
+ const char *ext)
+{
+ strbuf_reset(buf);
+ strbuf_addf(buf, "%s/pack/pack-%s.%s", get_object_directory(),
+ sha1_to_hex(sha1), ext);
+ return buf->buf;
+}
+
+char *sha1_pack_name(const unsigned char *sha1)
+{
+ static struct strbuf buf = STRBUF_INIT;
+ return odb_pack_name(&buf, sha1, "pack");
+}
+
+char *sha1_pack_index_name(const unsigned char *sha1)
+{
+ static struct strbuf buf = STRBUF_INIT;
+ return odb_pack_name(&buf, sha1, "idx");
+}
diff --git a/pack.h b/pack.h
index 8294341af..63bfde00c 100644
--- a/pack.h
+++ b/pack.h
@@ -101,4 +101,27 @@ extern int read_pack_header(int fd, struct pack_header *);
extern struct sha1file *create_tmp_packfile(char **pack_tmp_name);
extern void finish_tmp_packfile(struct strbuf *name_buffer, const char
*pack_tmp_name, struct pack_idx_entry **written_list, uint32_t nr_written,
struct pack_idx_option *pack_idx_opts, unsigned char sha1[]);
+/*
+ * Generate the filename to be used for a pack file with checksum "sha1" and
+ * extension "ext". The result is written into the strbuf "buf", overwriting
+ * any existing contents. A pointer to buf->buf is returned as a convenience.
+ *
+ * Example: odb_pack_name(out, sha1, "idx") =>
".git/objects/pack/pack-1234..idx"
+ */
+extern char *odb_pack_name(struct strbuf *buf, const unsigned char *sha1,
const char *ext);
+
+/*
+ * 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 specifie
