[PATCH 0/6] minor pack-name cleanups

[PATCH 0/6] minor pack-name cleanups

[Jeff King]

This started by cleaning up some unchecked snprintf() calls, but grew a
few related cleanups nearby.  I doubt the snprintf truncation affected
anybody in practice, but I think the result is nicer to read.

  [1/6]: index-pack: factor out pack/idx finalization
  [2/6]: move odb_* declarations out of git-compat-util.h
  [3/6]: sha1_file.c: make pack-name helper globally accessible
  [4/6]: index-pack: drop fixed-size buffer for pack filenames
  [5/6]: fast-import: replace fixed buffer with odb_pack_name
  [6/6]: odb_pack_keep(): stop generating keepfile name

 builtin/index-pack.c | 48 +++++++++++++++++++++---------------------------
 cache.h              | 21 +++++++++++++++++++++
 environment.c        |  6 ++----
 fast-import.c        | 26 +++++++++++++-------------
 git-compat-util.h    |  2 --
 sha1_file.c          | 17 ++++++-----------
 6 files changed, 63 insertions(+), 57 deletions(-)

-Peff

[PATCH 1/6] index-pack: factor out pack/idx finalization

[Jeff King]

The procedure for moving the ".pack" and the ".idx" files
into place is the same. Rather than repeat the code, let's
factor it into a helper function.

This has the added benefit of clarifying the lifetime of
"final_pack_name". In the original code it is sometimes
redirected to our reusable "name" buffer; after we reuse
that buffer, the contents are nonsense (but nobody looks at
it, so this is not a bug, just a maintenance issue). In the
new code, the pointer modification is contained in the
helper function.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/index-pack.c | 40 +++++++++++++++++++---------------------
 1 file changed, 19 insertions(+), 21 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index f4b87c6c9..187c0550c 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1380,6 +1380,23 @@ static void fix_unresolved_deltas(struct sha1file *f)
 	free(sorted_by_pos);
 }
 
+static void finalize_file(const char *final_name, const char *curr_name,
+			  unsigned char *sha1, const char *ext)
+{
+	if (final_name != curr_name) {
+		char name[PATH_MAX];
+		if (!final_name) {
+			snprintf(name, sizeof(name), "%s/pack/pack-%s.%s",
+				 get_object_directory(), sha1_to_hex(sha1),
+				 ext);
+			final_name = name;
+		}
+		if (finalize_object_file(curr_name, final_name))
+			die(_("cannot store %s file"), ext);
+	} else if (from_stdin)
+		chmod(final_name, 0444);
+}
+
 static void final(const char *final_pack_name, const char *curr_pack_name,
 		  const char *final_index_name, const char *curr_index_name,
 		  const char *keep_name, const char *keep_msg,
@@ -1422,27 +1439,8 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 		}
 	}
 
-	if (final_pack_name != curr_pack_name) {
-		if (!final_pack_name) {
-			snprintf(name, sizeof(name), "%s/pack/pack-%s.pack",
-				 get_object_directory(), sha1_to_hex(sha1));
-			final_pack_name = name;
-		}
-		if (finalize_object_file(curr_pack_name, final_pack_name))
-			die(_("cannot store pack file"));
-	} else if (from_stdin)
-		chmod(final_pack_name, 0444);
-
-	if (final_index_name != curr_index_name) {
-		if (!final_index_name) {
-			snprintf(name, sizeof(name), "%s/pack/pack-%s.idx",
-				 get_object_directory(), sha1_to_hex(sha1));
-			final_index_name = name;
-		}
-		if (finalize_object_file(curr_index_name, final_index_name))
-			die(_("cannot store index file"));
-	} else
-		chmod(final_index_name, 0444);
+	finalize_file(final_pack_name, curr_pack_name, sha1, "pack");
+	finalize_file(final_index_name, curr_index_name, sha1, "idx");
 
 	if (!from_stdin) {
 		printf("%s\n", sha1_to_hex(sha1));
-- 
2.12.0.613.g6e7c52a0d

[PATCH 2/6] move odb_* declarations out of git-compat-util.h

[Jeff King]

These functions were originally conceived as wrapper
functions similar to xmkstemp(). They were later moved by
463db9b10 (wrapper: move odb_* to environment.c,
2010-11-06). The more appropriate place for a declaration is
in cache.h.

While we're at it, let's add some basic docstrings.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h           | 12 ++++++++++++
 git-compat-util.h |  2 --
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index c95826971..68ad06e15 100644
--- a/cache.h
+++ b/cache.h
@@ -1634,6 +1634,18 @@ extern struct packed_git *find_sha1_pack(const unsigned char *sha1,
 
 extern void pack_report(void);
 
+/*
+ * Create a temporary file rooted in the object database directory.
+ */
+extern int odb_mkstemp(char *template, size_t limit, const char *pattern);
+
+/*
+ * Create a pack .keep file in the object database's pack directory, for
+ * a pack with checksum "sha1". The return value is a file descriptor opened
+ * for writing, or -1 on error. The name of the keep file is written to "name".
+ */
+extern int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1);
+
 /*
  * mmap the index file for the specified packfile (if it is not
  * already mmapped).  Return 0 on success.
diff --git a/git-compat-util.h b/git-compat-util.h
index e626851fe..8a4a3f85e 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -798,8 +798,6 @@ extern FILE *xfopen(const char *path, const char *mode);
 extern FILE *xfdopen(int fd, const char *mode);
 extern int xmkstemp(char *template);
 extern int xmkstemp_mode(char *template, int mode);
-extern int odb_mkstemp(char *template, size_t limit, const char *pattern);
-extern int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1);
 extern char *xgetcwd(void);
 extern FILE *fopen_for_writing(const char *path);
 
-- 
2.12.0.613.g6e7c52a0d

[PATCH 3/6] sha1_file.c: make pack-name helper globally accessible

[Jeff King]

We provide sha1_pack_name() and sha1_pack_index_name(), but
the more generic form (which takes its own strbuf and an
arbitrary extension) is only used to implement the other
two.  Let's make it available, but clean up a few things:

  1. Name it odb_pack_name(), as the original
     sha1_get_pack_name() is long but not all that
     descriptive.

  2. Switch the strbuf argument to the beginning, so that it
     matches similar path-building functions like
     git_path_buf().

  3. Clean up the out-dated docstring and move it to the
     public declaration.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h     |  9 +++++++++
 sha1_file.c | 17 ++++++-----------
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/cache.h b/cache.h
index 68ad06e15..97896e2e5 100644
--- a/cache.h
+++ b/cache.h
@@ -1639,6 +1639,15 @@ extern void pack_report(void);
  */
 extern int odb_mkstemp(char *template, size_t limit, 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 in the object database's pack directory, for
  * a pack with checksum "sha1". The return value is a file descriptor opened
diff --git a/sha1_file.c b/sha1_file.c
index 2ee3c617a..56ef09cd3 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -277,31 +277,26 @@ static const char *alt_sha1_path(struct alternate_object_database *alt,
 	return buf->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,
-				struct strbuf *buf,
-				const char *which)
+ 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), which);
+		    sha1_to_hex(sha1), ext);
 	return buf->buf;
 }
 
 char *sha1_pack_name(const unsigned char *sha1)
 {
 	static struct strbuf buf = STRBUF_INIT;
-	return sha1_get_pack_name(sha1, &buf, "pack");
+	return odb_pack_name(&buf, sha1, "pack");
 }
 
 char *sha1_pack_index_name(const unsigned char *sha1)
 {
 	static struct strbuf buf = STRBUF_INIT;
-	return sha1_get_pack_name(sha1, &buf, "idx");
+	return odb_pack_name(&buf, sha1, "idx");
 }
 
 struct alternate_object_database *alt_odb_list;
-- 
2.12.0.613.g6e7c52a0d

[PATCH 4/6] index-pack: drop fixed-size buffer for pack filenames

[Jeff King]

We write the name of the pack filename into a fixed-size
buffer using snprintf(), but do not check the return value.
As a result, a very long object directory could cause us to
quietly truncate the pack filename (leading to a corrupted
repository, as the packfile would be missing its .pack
extension).

We can use odb_pack_name() to fix this (and make the code
simpler, too).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/index-pack.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 187c0550c..b6e7ac331 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1384,13 +1384,9 @@ static void finalize_file(const char *final_name, const char *curr_name,
 			  unsigned char *sha1, const char *ext)
 {
 	if (final_name != curr_name) {
-		char name[PATH_MAX];
-		if (!final_name) {
-			snprintf(name, sizeof(name), "%s/pack/pack-%s.%s",
-				 get_object_directory(), sha1_to_hex(sha1),
-				 ext);
-			final_name = name;
-		}
+		struct strbuf buf = STRBUF_INIT;
+		if (!final_name)
+			final_name = odb_pack_name(&buf, sha1, ext);
 		if (finalize_object_file(curr_name, final_name))
 			die(_("cannot store %s file"), ext);
 	} else if (from_stdin)
-- 
2.12.0.613.g6e7c52a0d

[PATCH 5/6] fast-import: replace fixed buffer with odb_pack_name

[Jeff King]

This removes the possibility of truncating the filename
without realizing it.

Signed-off-by: Jeff King <peff@peff.net>
---
 fast-import.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 6c13472c4..61be8b76e 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -966,15 +966,15 @@ static char *keep_pack(const char *curr_index_name)
 
 static void unkeep_all_packs(void)
 {
-	static char name[PATH_MAX];
+	struct strbuf name = STRBUF_INIT;
 	int k;
 
 	for (k = 0; k < pack_id; k++) {
 		struct packed_git *p = all_packs[k];
-		snprintf(name, sizeof(name), "%s/pack/pack-%s.keep",
-			 get_object_directory(), sha1_to_hex(p->sha1));
-		unlink_or_warn(name);
+		odb_pack_name(&name, p->sha1, "keep");
+		unlink_or_warn(name.buf);
 	}
+	strbuf_release(&name);
 }
 
 static int loosen_small_pack(const struct packed_git *p)
-- 
2.12.0.613.g6e7c52a0d

[PATCH 6/6] odb_pack_keep(): stop generating keepfile name

[Jeff King]

The odb_pack_keep() function generates the name of a .keep
file and opens it. This has two problems:

  1. It requires a fixed-size buffer to create the filename
     and doesn't notice when the result is truncated.

  2. Of the two callers, one sometimes wants to open a
     filename it already has, which makes things awkward (it
     has to do so manually, and skips the leading-directory
     creation).

Instead, let's have odb_pack_keep() just open the file.
Callers can use odb_pack_name() separately to generate the
name. This simplifies the callers, and lets us drop any
buffer-size limitations.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/index-pack.c | 12 ++++++------
 cache.h              |  8 ++++----
 environment.c        |  6 ++----
 fast-import.c        | 18 +++++++++---------
 4 files changed, 21 insertions(+), 23 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index b6e7ac331..e0f70836d 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1399,7 +1399,6 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 		  unsigned char *sha1)
 {
 	const char *report = "pack";
-	char name[PATH_MAX];
 	int err;
 
 	if (!from_stdin) {
@@ -1412,17 +1411,17 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 	}
 
 	if (keep_msg) {
+		struct strbuf name = STRBUF_INIT;
 		int keep_fd, keep_msg_len = strlen(keep_msg);
 
 		if (!keep_name)
-			keep_fd = odb_pack_keep(name, sizeof(name), sha1);
-		else
-			keep_fd = open(keep_name, O_RDWR|O_CREAT|O_EXCL, 0600);
+			keep_name = odb_pack_name(&name, sha1, "keep");
 
+		keep_fd = odb_pack_keep(keep_name);
 		if (keep_fd < 0) {
 			if (errno != EEXIST)
 				die_errno(_("cannot write keep file '%s'"),
-					  keep_name ? keep_name : name);
+					  keep_name);
 		} else {
 			if (keep_msg_len > 0) {
 				write_or_die(keep_fd, keep_msg, keep_msg_len);
@@ -1430,9 +1429,10 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 			}
 			if (close(keep_fd) != 0)
 				die_errno(_("cannot close written keep file '%s'"),
-					  keep_name ? keep_name : name);
+					  keep_name);
 			report = "keep";
 		}
+		strbuf_release(&name);
 	}
 
 	finalize_file(final_pack_name, curr_pack_name, sha1, "pack");
diff --git a/cache.h b/cache.h
index 97896e2e5..cb8f48f6f 100644
--- a/cache.h
+++ b/cache.h
@@ -1649,11 +1649,11 @@ extern int odb_mkstemp(char *template, size_t limit, const char *pattern);
 extern char *odb_pack_name(struct strbuf *buf, const unsigned char *sha1, const char *ext);
 
 /*
- * Create a pack .keep file in the object database's pack directory, for
- * a pack with checksum "sha1". The return value is a file descriptor opened
- * for writing, or -1 on error. The name of the keep file is written to "name".
+ * 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
+ * error.
  */
-extern int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1);
+extern int odb_pack_keep(const char *name);
 
 /*
  * mmap the index file for the specified packfile (if it is not
diff --git a/environment.c b/environment.c
index 42dc3106d..2fdba7622 100644
--- a/environment.c
+++ b/environment.c
@@ -296,18 +296,16 @@ int odb_mkstemp(char *template, size_t limit, const char *pattern)
 	return xmkstemp_mode(template, mode);
 }
 
-int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1)
+int odb_pack_keep(const char *name)
 {
 	int fd;
 
-	snprintf(name, namesz, "%s/pack/pack-%s.keep",
-		 get_object_directory(), sha1_to_hex(sha1));
 	fd = open(name, O_RDWR|O_CREAT|O_EXCL, 0600);
 	if (0 <= fd)
 		return fd;
 
 	/* slow path */
-	safe_create_leading_directories(name);
+	safe_create_leading_directories_const(name);
 	return open(name, O_RDWR|O_CREAT|O_EXCL, 0600);
 }
 
diff --git a/fast-import.c b/fast-import.c
index 61be8b76e..41a539f97 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -940,28 +940,27 @@ static const char *create_index(void)
 
 static char *keep_pack(const char *curr_index_name)
 {
-	static char name[PATH_MAX];
 	static const char *keep_msg = "fast-import";
+	struct strbuf name = STRBUF_INIT;
 	int keep_fd;
 
-	keep_fd = odb_pack_keep(name, sizeof(name), pack_data->sha1);
+	odb_pack_name(&name, pack_data->sha1, "keep");
+	keep_fd = odb_pack_keep(name.buf);
 	if (keep_fd < 0)
 		die_errno("cannot create keep file");
 	write_or_die(keep_fd, keep_msg, strlen(keep_msg));
 	if (close(keep_fd))
 		die_errno("failed to write keep file");
 
-	snprintf(name, sizeof(name), "%s/pack/pack-%s.pack",
-		 get_object_directory(), sha1_to_hex(pack_data->sha1));
-	if (finalize_object_file(pack_data->pack_name, name))
+	odb_pack_name(&name, pack_data->sha1, "pack");
+	if (finalize_object_file(pack_data->pack_name, name.buf))
 		die("cannot store pack file");
 
-	snprintf(name, sizeof(name), "%s/pack/pack-%s.idx",
-		 get_object_directory(), sha1_to_hex(pack_data->sha1));
-	if (finalize_object_file(curr_index_name, name))
+	odb_pack_name(&name, pack_data->sha1, "idx");
+	if (finalize_object_file(curr_index_name, name.buf))
 		die("cannot store index file");
 	free((void *)curr_index_name);
-	return name;
+	return strbuf_detach(&name, NULL);
 }
 
 static void unkeep_all_packs(void)
@@ -1033,6 +1032,7 @@ static void end_packfile(void)
 			die("core git rejected index %s", idx_name);
 		all_packs[pack_id] = new_p;
 		install_packed_git(new_p);
+		free(idx_name);
 
 		/* Print the boundary */
 		if (pack_edges) {
-- 
2.12.0.613.g6e7c52a0d

Re: [PATCH 1/6] index-pack: factor out pack/idx finalization

[Ramsay Jones]

On 15/03/17 21:27, Jeff King wrote:
> The procedure for moving the ".pack" and the ".idx" files
> into place is the same. Rather than repeat the code, let's
> factor it into a helper function.
> 
> This has the added benefit of clarifying the lifetime of
> "final_pack_name". In the original code it is sometimes
> redirected to our reusable "name" buffer; after we reuse
> that buffer, the contents are nonsense (but nobody looks at
> it, so this is not a bug, just a maintenance issue). In the
> new code, the pointer modification is contained in the
> helper function.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/index-pack.c | 40 +++++++++++++++++++---------------------
>  1 file changed, 19 insertions(+), 21 deletions(-)
> 
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index f4b87c6c9..187c0550c 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -1380,6 +1380,23 @@ static void fix_unresolved_deltas(struct sha1file *f)
>  	free(sorted_by_pos);
>  }
>  
> +static void finalize_file(const char *final_name, const char *curr_name,
> +			  unsigned char *sha1, const char *ext)
> +{
> +	if (final_name != curr_name) {
> +		char name[PATH_MAX];
> +		if (!final_name) {
> +			snprintf(name, sizeof(name), "%s/pack/pack-%s.%s",
> +				 get_object_directory(), sha1_to_hex(sha1),
> +				 ext);
> +			final_name = name;
> +		}
> +		if (finalize_object_file(curr_name, final_name))
> +			die(_("cannot store %s file"), ext);
> +	} else if (from_stdin)
> +		chmod(final_name, 0444);
> +}
> +
>  static void final(const char *final_pack_name, const char *curr_pack_name,
>  		  const char *final_index_name, const char *curr_index_name,
>  		  const char *keep_name, const char *keep_msg,
> @@ -1422,27 +1439,8 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
>  		}
>  	}
>  
> -	if (final_pack_name != curr_pack_name) {
> -		if (!final_pack_name) {
> -			snprintf(name, sizeof(name), "%s/pack/pack-%s.pack",
> -				 get_object_directory(), sha1_to_hex(sha1));
> -			final_pack_name = name;
> -		}
> -		if (finalize_object_file(curr_pack_name, final_pack_name))
> -			die(_("cannot store pack file"));
> -	} else if (from_stdin)
> -		chmod(final_pack_name, 0444);
> -
> -	if (final_index_name != curr_index_name) {
> -		if (!final_index_name) {
> -			snprintf(name, sizeof(name), "%s/pack/pack-%s.idx",
> -				 get_object_directory(), sha1_to_hex(sha1));
> -			final_index_name = name;
> -		}
> -		if (finalize_object_file(curr_index_name, final_index_name))
> -			die(_("cannot store index file"));
> -	} else
> -		chmod(final_index_name, 0444);

Is from_stdin always true if final_index_name == curr_index_name?
Was the original asymmetry deliberate?

> +	finalize_file(final_pack_name, curr_pack_name, sha1, "pack");
> +	finalize_file(final_index_name, curr_index_name, sha1, "idx");
>  
>  	if (!from_stdin) {
>  		printf("%s\n", sha1_to_hex(sha1));
> 

ATB,
Ramsay Jones

Re: [PATCH 1/6] index-pack: factor out pack/idx finalization

[Jeff King]

On Wed, Mar 15, 2017 at 10:03:56PM +0000, Ramsay Jones wrote:

> > -	if (final_pack_name != curr_pack_name) {
> > -		if (!final_pack_name) {
> > -			snprintf(name, sizeof(name), "%s/pack/pack-%s.pack",
> > -				 get_object_directory(), sha1_to_hex(sha1));
> > -			final_pack_name = name;
> > -		}
> > -		if (finalize_object_file(curr_pack_name, final_pack_name))
> > -			die(_("cannot store pack file"));
> > -	} else if (from_stdin)
> > -		chmod(final_pack_name, 0444);
> > -
> > -	if (final_index_name != curr_index_name) {
> > -		if (!final_index_name) {
> > -			snprintf(name, sizeof(name), "%s/pack/pack-%s.idx",
> > -				 get_object_directory(), sha1_to_hex(sha1));
> > -			final_index_name = name;
> > -		}
> > -		if (finalize_object_file(curr_index_name, final_index_name))
> > -			die(_("cannot store index file"));
> > -	} else
> > -		chmod(final_index_name, 0444);
> 
> Is from_stdin always true if final_index_name == curr_index_name?
> Was the original asymmetry deliberate?

Hrm, good question.

I think the logic on the pack side is that:

  git index-pack --stdin

will write a new pack, and we should chmod it as appropriate. But it
would be wrong to do so when generating an index for an existing pack:

  git index-pack foo.pack

Whatever wrote foo.pack is responsible for the chmod then.

So the flip side (and what your question is asking) is whether it is
safe to skip the .idx chmod in the non-stdin case.

Usually when we don't have an existing .idx file, we write to a new
tempfile.  We obviously want to chmod there, but it would fall under the
"final_index_name != curr_index_name" case.

But it looks like you can give "-o", or we can derive the name from the
.pack name. So if we do:

  git pack-objects --all --stdout >foo.pack
  git index-pack foo.pack

Then we'd expect the newly-created .idx to have mode 0444, but it
doesn't. So yeah, I think the distinction does matter.

I'm not sure if the best path is to include that flag in the
finalize_file() helper, or just ditch the helper.  Its main purpose was
cleanup so that the odb_pack_name() refactor didn't have to happen
twice. But after that refactor, the amount of shared code is relatively
small.

-Peff

Re: [PATCH 1/6] index-pack: factor out pack/idx finalization

[Jeff King]

On Wed, Mar 15, 2017 at 06:22:23PM -0400, Jeff King wrote:

> Then we'd expect the newly-created .idx to have mode 0444, but it
> doesn't. So yeah, I think the distinction does matter.
> 
> I'm not sure if the best path is to include that flag in the
> finalize_file() helper, or just ditch the helper.  Its main purpose was
> cleanup so that the odb_pack_name() refactor didn't have to happen
> twice. But after that refactor, the amount of shared code is relatively
> small.

I think I'm leaning towards just ditching the helper. The resulting
change looks pretty good:

@@ -1423,22 +1424,16 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 	}
 
 	if (final_pack_name != curr_pack_name) {
-		if (!final_pack_name) {
-			snprintf(name, sizeof(name), "%s/pack/pack-%s.pack",
-				 get_object_directory(), sha1_to_hex(sha1));
-			final_pack_name = name;
-		}
+		if (!final_pack_name)
+			final_pack_name = odb_pack_name(&name, sha1, "pack");
 		if (finalize_object_file(curr_pack_name, final_pack_name))
 			die(_("cannot store pack file"));
 	} else if (from_stdin)
 		chmod(final_pack_name, 0444);
 
 	if (final_index_name != curr_index_name) {
-		if (!final_index_name) {
-			snprintf(name, sizeof(name), "%s/pack/pack-%s.idx",
-				 get_object_directory(), sha1_to_hex(sha1));
-			final_index_name = name;
-		}
+		if (!final_index_name)
+			final_index_name = odb_pack_name(&name, sha1, "idx");
 		if (finalize_object_file(curr_index_name, final_index_name))
 			die(_("cannot store index file"));
 	} else

And we just end up sharing the same name buffer (and the "keep" code
path can use the same one, too).

I'll wait until tomorrow to collect any more comments and then send out
a re-roll.

Thanks for reading so carefully.

-Peff

Re: [PATCH 6/6] odb_pack_keep(): stop generating keepfile name

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The odb_pack_keep() function generates the name of a .keep
> file and opens it. This has two problems:
>
>   1. It requires a fixed-size buffer to create the filename
>      and doesn't notice when the result is truncated.
>
>   2. Of the two callers, one sometimes wants to open a
>      filename it already has, which makes things awkward (it
>      has to do so manually, and skips the leading-directory
>      creation).
>
> Instead, let's have odb_pack_keep() just open the file.
> Callers can use odb_pack_name() separately to generate the
> name. This simplifies the callers, and lets us drop any
> buffer-size limitations.

That's sensible (and all the others made the resulting code much
more pleasant to the eyes).

[PATCH v2 0/5] minor pack-name cleanups

[Jeff King]

Here's a re-roll of the series from:

  http://public-inbox.org/git/20170315212617.6x57bvltinuozv4q@sigill.intra.peff.net/

The general gist is the same, but there are a number of changes:

  - I dropped the first patch factoring out finalize_file(), as Ramsay
    pointed out a subtle difference between the index and pack handling.

  - I added an extra patch on top (5/5 here) to get the same
    pointer-aliasing safety that the factored-out function got us
    (more so, actually, as it protects keep_msg, too).

  - I re-ordered the odb_pack_keep() cleanup before the odb_pack_name()
    cleanups, which lets us do the latter in one swoop (and avoids
    explaining "well, we can't do .keep yet, because..." in the commit
    message)

  - The original had two patches doing the odb_pack_name() conversion.
    Now that it has fewer caveats, I felt comfortable lumping it all
    into one (patch 4/5 here).

  [1/5]: move odb_* declarations out of git-compat-util.h
  [2/5]: sha1_file.c: make pack-name helper globally accessible
  [3/5]: odb_pack_keep(): stop generating keepfile name
  [4/5]: replace snprintf with odb_pack_name()
  [5/5]: index-pack: make pointer-alias fallbacks safer

 builtin/index-pack.c | 31 +++++++++++++++----------------
 cache.h              | 21 +++++++++++++++++++++
 environment.c        |  6 ++----
 fast-import.c        | 26 +++++++++++++-------------
 git-compat-util.h    |  2 --
 sha1_file.c          | 17 ++++++-----------
 6 files changed, 57 insertions(+), 46 deletions(-)

[PATCH v2 1/5] move odb_* declarations out of git-compat-util.h

[Jeff King]

These functions were originally conceived as wrapper
functions similar to xmkstemp(). They were later moved by
463db9b10 (wrapper: move odb_* to environment.c,
2010-11-06). The more appropriate place for a declaration is
in cache.h.

While we're at it, let's add some basic docstrings.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h           | 12 ++++++++++++
 git-compat-util.h |  2 --
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index c95826971..68ad06e15 100644
--- a/cache.h
+++ b/cache.h
@@ -1634,6 +1634,18 @@ extern struct packed_git *find_sha1_pack(const unsigned char *sha1,
 
 extern void pack_report(void);
 
+/*
+ * Create a temporary file rooted in the object database directory.
+ */
+extern int odb_mkstemp(char *template, size_t limit, const char *pattern);
+
+/*
+ * Create a pack .keep file in the object database's pack directory, for
+ * a pack with checksum "sha1". The return value is a file descriptor opened
+ * for writing, or -1 on error. The name of the keep file is written to "name".
+ */
+extern int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1);
+
 /*
  * mmap the index file for the specified packfile (if it is not
  * already mmapped).  Return 0 on success.
diff --git a/git-compat-util.h b/git-compat-util.h
index e626851fe..8a4a3f85e 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -798,8 +798,6 @@ extern FILE *xfopen(const char *path, const char *mode);
 extern FILE *xfdopen(int fd, const char *mode);
 extern int xmkstemp(char *template);
 extern int xmkstemp_mode(char *template, int mode);
-extern int odb_mkstemp(char *template, size_t limit, const char *pattern);
-extern int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1);
 extern char *xgetcwd(void);
 extern FILE *fopen_for_writing(const char *path);
 
-- 
2.12.0.623.g86ec6c963

[PATCH v2 2/5] sha1_file.c: make pack-name helper globally accessible

[Jeff King]

We provide sha1_pack_name() and sha1_pack_index_name(), but
the more generic form (which takes its own strbuf and an
arbitrary extension) is only used to implement the other
two.  Let's make it available, but clean up a few things:

  1. Name it odb_pack_name(), as the original
     sha1_get_pack_name() is long but not all that
     descriptive.

  2. Switch the strbuf argument to the beginning, so that it
     matches similar path-building functions like
     git_path_buf().

  3. Clean up the out-dated docstring and move it to the
     public declaration.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h     |  9 +++++++++
 sha1_file.c | 17 ++++++-----------
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/cache.h b/cache.h
index 68ad06e15..97896e2e5 100644
--- a/cache.h
+++ b/cache.h
@@ -1639,6 +1639,15 @@ extern void pack_report(void);
  */
 extern int odb_mkstemp(char *template, size_t limit, 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 in the object database's pack directory, for
  * a pack with checksum "sha1". The return value is a file descriptor opened
diff --git a/sha1_file.c b/sha1_file.c
index 2ee3c617a..56ef09cd3 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -277,31 +277,26 @@ static const char *alt_sha1_path(struct alternate_object_database *alt,
 	return buf->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,
-				struct strbuf *buf,
-				const char *which)
+ 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), which);
+		    sha1_to_hex(sha1), ext);
 	return buf->buf;
 }
 
 char *sha1_pack_name(const unsigned char *sha1)
 {
 	static struct strbuf buf = STRBUF_INIT;
-	return sha1_get_pack_name(sha1, &buf, "pack");
+	return odb_pack_name(&buf, sha1, "pack");
 }
 
 char *sha1_pack_index_name(const unsigned char *sha1)
 {
 	static struct strbuf buf = STRBUF_INIT;
-	return sha1_get_pack_name(sha1, &buf, "idx");
+	return odb_pack_name(&buf, sha1, "idx");
 }
 
 struct alternate_object_database *alt_odb_list;
-- 
2.12.0.623.g86ec6c963

[PATCH v2 3/5] odb_pack_keep(): stop generating keepfile name

[Jeff King]

The odb_pack_keep() function generates the name of a .keep
file and opens it. This has two problems:

  1. It requires a fixed-size buffer to create the filename
     and doesn't notice when the result is truncated.

  2. Of the two callers, one sometimes wants to open a
     filename it already has, which makes things awkward (it
     has to do so manually, and skips the leading-directory
     creation).

Instead, let's have odb_pack_keep() just open the file.
Generating the name isn't hard, and a future patch will
switch callers over to odb_pack_name() anyway.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/index-pack.c | 6 +++---
 cache.h              | 8 ++++----
 environment.c        | 6 ++----
 fast-import.c        | 4 +++-
 4 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index f4b87c6c9..a58bc6bee 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1402,10 +1402,10 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 		int keep_fd, keep_msg_len = strlen(keep_msg);
 
 		if (!keep_name)
-			keep_fd = odb_pack_keep(name, sizeof(name), sha1);
-		else
-			keep_fd = open(keep_name, O_RDWR|O_CREAT|O_EXCL, 0600);
+			snprintf(name, sizeof(name), "%s/pack/pack-%s.keep",
+				 get_object_directory(), sha1_to_hex(sha1));
 
+		keep_fd = odb_pack_keep(keep_name ? keep_name : name);
 		if (keep_fd < 0) {
 			if (errno != EEXIST)
 				die_errno(_("cannot write keep file '%s'"),
diff --git a/cache.h b/cache.h
index 97896e2e5..cb8f48f6f 100644
--- a/cache.h
+++ b/cache.h
@@ -1649,11 +1649,11 @@ extern int odb_mkstemp(char *template, size_t limit, const char *pattern);
 extern char *odb_pack_name(struct strbuf *buf, const unsigned char *sha1, const char *ext);
 
 /*
- * Create a pack .keep file in the object database's pack directory, for
- * a pack with checksum "sha1". The return value is a file descriptor opened
- * for writing, or -1 on error. The name of the keep file is written to "name".
+ * 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
+ * error.
  */
-extern int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1);
+extern int odb_pack_keep(const char *name);
 
 /*
  * mmap the index file for the specified packfile (if it is not
diff --git a/environment.c b/environment.c
index 42dc3106d..2fdba7622 100644
--- a/environment.c
+++ b/environment.c
@@ -296,18 +296,16 @@ int odb_mkstemp(char *template, size_t limit, const char *pattern)
 	return xmkstemp_mode(template, mode);
 }
 
-int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1)
+int odb_pack_keep(const char *name)
 {
 	int fd;
 
-	snprintf(name, namesz, "%s/pack/pack-%s.keep",
-		 get_object_directory(), sha1_to_hex(sha1));
 	fd = open(name, O_RDWR|O_CREAT|O_EXCL, 0600);
 	if (0 <= fd)
 		return fd;
 
 	/* slow path */
-	safe_create_leading_directories(name);
+	safe_create_leading_directories_const(name);
 	return open(name, O_RDWR|O_CREAT|O_EXCL, 0600);
 }
 
diff --git a/fast-import.c b/fast-import.c
index 6c13472c4..dad697653 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -944,7 +944,9 @@ static char *keep_pack(const char *curr_index_name)
 	static const char *keep_msg = "fast-import";
 	int keep_fd;
 
-	keep_fd = odb_pack_keep(name, sizeof(name), pack_data->sha1);
+	snprintf(name, sizeof(name), "%s/pack/pack-%s.keep",
+		 get_object_directory(), sha1_to_hex(pack_data->sha1));
+	keep_fd = odb_pack_keep(name);
 	if (keep_fd < 0)
 		die_errno("cannot create keep file");
 	write_or_die(keep_fd, keep_msg, strlen(keep_msg));
-- 
2.12.0.623.g86ec6c963

[PATCH v2 4/5] replace snprintf with odb_pack_name()

[Jeff King]

In several places we write the name of the pack filename
into a fixed-size buffer using snprintf(), but do not check
the return value.  As a result, a very long object directory
could cause us to quietly truncate the pack filename
(potentially leading to a corrupted repository, as a newly
written packfile could be missing its .pack extension).

We can use odb_pack_name() to do this with a strbuf (and
shorten the code, as well).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/index-pack.c | 27 +++++++++++----------------
 fast-import.c        | 28 +++++++++++++---------------
 2 files changed, 24 insertions(+), 31 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index a58bc6bee..dcb346ab7 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1386,7 +1386,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 		  unsigned char *sha1)
 {
 	const char *report = "pack";
-	char name[PATH_MAX];
+	struct strbuf name = STRBUF_INIT;
 	int err;
 
 	if (!from_stdin) {
@@ -1402,14 +1402,13 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 		int keep_fd, keep_msg_len = strlen(keep_msg);
 
 		if (!keep_name)
-			snprintf(name, sizeof(name), "%s/pack/pack-%s.keep",
-				 get_object_directory(), sha1_to_hex(sha1));
+			odb_pack_name(&name, sha1, "keep");
 
-		keep_fd = odb_pack_keep(keep_name ? keep_name : name);
+		keep_fd = odb_pack_keep(keep_name ? keep_name : name.buf);
 		if (keep_fd < 0) {
 			if (errno != EEXIST)
 				die_errno(_("cannot write keep file '%s'"),
-					  keep_name ? keep_name : name);
+					  keep_name ? keep_name : name.buf);
 		} else {
 			if (keep_msg_len > 0) {
 				write_or_die(keep_fd, keep_msg, keep_msg_len);
@@ -1417,28 +1416,22 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 			}
 			if (close(keep_fd) != 0)
 				die_errno(_("cannot close written keep file '%s'"),
-					  keep_name ? keep_name : name);
+					  keep_name ? keep_name : name.buf);
 			report = "keep";
 		}
 	}
 
 	if (final_pack_name != curr_pack_name) {
-		if (!final_pack_name) {
-			snprintf(name, sizeof(name), "%s/pack/pack-%s.pack",
-				 get_object_directory(), sha1_to_hex(sha1));
-			final_pack_name = name;
-		}
+		if (!final_pack_name)
+			final_pack_name = odb_pack_name(&name, sha1, "pack");
 		if (finalize_object_file(curr_pack_name, final_pack_name))
 			die(_("cannot store pack file"));
 	} else if (from_stdin)
 		chmod(final_pack_name, 0444);
 
 	if (final_index_name != curr_index_name) {
-		if (!final_index_name) {
-			snprintf(name, sizeof(name), "%s/pack/pack-%s.idx",
-				 get_object_directory(), sha1_to_hex(sha1));
-			final_index_name = name;
-		}
+		if (!final_index_name)
+			final_index_name = odb_pack_name(&name, sha1, "idx");
 		if (finalize_object_file(curr_index_name, final_index_name))
 			die(_("cannot store index file"));
 	} else
@@ -1464,6 +1457,8 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 			input_offset += err;
 		}
 	}
+
+	strbuf_release(&name);
 }
 
 static int git_index_pack_config(const char *k, const char *v, void *cb)
diff --git a/fast-import.c b/fast-import.c
index dad697653..41a539f97 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -940,43 +940,40 @@ static const char *create_index(void)
 
 static char *keep_pack(const char *curr_index_name)
 {
-	static char name[PATH_MAX];
 	static const char *keep_msg = "fast-import";
+	struct strbuf name = STRBUF_INIT;
 	int keep_fd;
 
-	snprintf(name, sizeof(name), "%s/pack/pack-%s.keep",
-		 get_object_directory(), sha1_to_hex(pack_data->sha1));
-	keep_fd = odb_pack_keep(name);
+	odb_pack_name(&name, pack_data->sha1, "keep");
+	keep_fd = odb_pack_keep(name.buf);
 	if (keep_fd < 0)
 		die_errno("cannot create keep file");
 	write_or_die(keep_fd, keep_msg, strlen(keep_msg));
 	if (close(keep_fd))
 		die_errno("failed to write keep file");
 
-	snprintf(name, sizeof(name), "%s/pack/pack-%s.pack",
-		 get_object_directory(), sha1_to_hex(pack_data->sha1));
-	if (finalize_object_file(pack_data->pack_name, name))
+	odb_pack_name(&name, pack_data->sha1, "pack");
+	if (finalize_object_file(pack_data->pack_name, name.buf))
 		die("cannot store pack file");
 
-	snprintf(name, sizeof(name), "%s/pack/pack-%s.idx",
-		 get_object_directory(), sha1_to_hex(pack_data->sha1));
-	if (finalize_object_file(curr_index_name, name))
+	odb_pack_name(&name, pack_data->sha1, "idx");
+	if (finalize_object_file(curr_index_name, name.buf))
 		die("cannot store index file");
 	free((void *)curr_index_name);
-	return name;
+	return strbuf_detach(&name, NULL);
 }
 
 static void unkeep_all_packs(void)
 {
-	static char name[PATH_MAX];
+	struct strbuf name = STRBUF_INIT;
 	int k;
 
 	for (k = 0; k < pack_id; k++) {
 		struct packed_git *p = all_packs[k];
-		snprintf(name, sizeof(name), "%s/pack/pack-%s.keep",
-			 get_object_directory(), sha1_to_hex(p->sha1));
-		unlink_or_warn(name);
+		odb_pack_name(&name, p->sha1, "keep");
+		unlink_or_warn(name.buf);
 	}
+	strbuf_release(&name);
 }
 
 static int loosen_small_pack(const struct packed_git *p)
@@ -1035,6 +1032,7 @@ static void end_packfile(void)
 			die("core git rejected index %s", idx_name);
 		all_packs[pack_id] = new_p;
 		install_packed_git(new_p);
+		free(idx_name);
 
 		/* Print the boundary */
 		if (pack_edges) {
-- 
2.12.0.623.g86ec6c963

[PATCH v2 5/5] index-pack: make pointer-alias fallbacks safer

[Jeff King]

The final() function accepts a NULL value for certain
parameters, and falls back to writing into a reusable "name"
buffer, and then either:

  1. For "keep_name", requiring all uses to do "keep_name ?
     keep_name : name.buf". This is awkward, and it's easy
     to accidentally look at the maybe-NULL keep_name.

  2. For "final_index_name" and "final_pack_name", aliasing
     those pointers to the "name" buffer. This is easier to
     use, but the aliased pointers become invalid after the
     buffer is reused (this isn't a bug now, but it's a
     potential pitfall).

One way to make this safer would be to introduce an extra
pointer to do the aliasing, and have its lifetime match the
validity of the "name" buffer. But it's still easy to
accidentally use the wrong name (i.e., to use
"final_pack_name" instead of the aliased pointer).

Instead, let's use three separate buffers that will remain
valid through the function. That makes it safe to alias the
pointers and use them consistently. The extra allocations
shouldn't matter, as this function is not performance
sensitive.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/index-pack.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index dcb346ab7..88d205f85 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1386,7 +1386,9 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 		  unsigned char *sha1)
 {
 	const char *report = "pack";
-	struct strbuf name = STRBUF_INIT;
+	struct strbuf pack_name = STRBUF_INIT;
+	struct strbuf index_name = STRBUF_INIT;
+	struct strbuf keep_name_buf = STRBUF_INIT;
 	int err;
 
 	if (!from_stdin) {
@@ -1402,13 +1404,13 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 		int keep_fd, keep_msg_len = strlen(keep_msg);
 
 		if (!keep_name)
-			odb_pack_name(&name, sha1, "keep");
+			keep_name = odb_pack_name(&keep_name_buf, sha1, "keep");
 
-		keep_fd = odb_pack_keep(keep_name ? keep_name : name.buf);
+		keep_fd = odb_pack_keep(keep_name);
 		if (keep_fd < 0) {
 			if (errno != EEXIST)
 				die_errno(_("cannot write keep file '%s'"),
-					  keep_name ? keep_name : name.buf);
+					  keep_name);
 		} else {
 			if (keep_msg_len > 0) {
 				write_or_die(keep_fd, keep_msg, keep_msg_len);
@@ -1416,14 +1418,14 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 			}
 			if (close(keep_fd) != 0)
 				die_errno(_("cannot close written keep file '%s'"),
-					  keep_name ? keep_name : name.buf);
+					  keep_name);
 			report = "keep";
 		}
 	}
 
 	if (final_pack_name != curr_pack_name) {
 		if (!final_pack_name)
-			final_pack_name = odb_pack_name(&name, sha1, "pack");
+			final_pack_name = odb_pack_name(&pack_name, sha1, "pack");
 		if (finalize_object_file(curr_pack_name, final_pack_name))
 			die(_("cannot store pack file"));
 	} else if (from_stdin)
@@ -1431,7 +1433,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 
 	if (final_index_name != curr_index_name) {
 		if (!final_index_name)
-			final_index_name = odb_pack_name(&name, sha1, "idx");
+			final_index_name = odb_pack_name(&index_name, sha1, "idx");
 		if (finalize_object_file(curr_index_name, final_index_name))
 			die(_("cannot store index file"));
 	} else
@@ -1458,7 +1460,9 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 		}
 	}
 
-	strbuf_release(&name);
+	strbuf_release(&index_name);
+	strbuf_release(&pack_name);
+	strbuf_release(&keep_name_buf);
 }
 
 static int git_index_pack_config(const char *k, const char *v, void *cb)
-- 
2.12.0.623.g86ec6c963

Re: [PATCH v2 2/5] sha1_file.c: make pack-name helper globally accessible

[Jeff King]

On Thu, Mar 16, 2017 at 10:27:06AM -0400, Jeff King wrote:

> -/*
> - * 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,
> -				struct strbuf *buf,
> -				const char *which)
> + 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), which);
> +		    sha1_to_hex(sha1), ext);
>  	return buf->buf;
>  }

Incidentally, this entire function could be implemented as:

  return git_path_buf(buf, "objects/pack/pack-%s.%s",
                      sha1_to_hex(sha1), ext);

as the git_path() functions are smart enough to replace "objects/" with
the true object directory when necessary. I don't know if people find
that more or less readable. Since it's buried in a helper function, I
doubt it matters much either way. The git_path functions do also do some
path normalization, which might be of value.

-Peff

Re: [PATCH v2 4/5] replace snprintf with odb_pack_name()

[Ramsay Jones]

On 16/03/17 14:27, Jeff King wrote:
> In several places we write the name of the pack filename
> into a fixed-size buffer using snprintf(), but do not check
> the return value.  As a result, a very long object directory
> could cause us to quietly truncate the pack filename
> (potentially leading to a corrupted repository, as a newly
> written packfile could be missing its .pack extension).
> 
> We can use odb_pack_name() to do this with a strbuf (and
> shorten the code, as well).
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/index-pack.c | 27 +++++++++++----------------
>  fast-import.c        | 28 +++++++++++++---------------
>  2 files changed, 24 insertions(+), 31 deletions(-)
> 
[snip]

> diff --git a/fast-import.c b/fast-import.c
> index dad697653..41a539f97 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -940,43 +940,40 @@ static const char *create_index(void)
>  
>  static char *keep_pack(const char *curr_index_name)
>  {
> -	static char name[PATH_MAX];
>  	static const char *keep_msg = "fast-import";
> +	struct strbuf name = STRBUF_INIT;
>  	int keep_fd;
>  
> -	snprintf(name, sizeof(name), "%s/pack/pack-%s.keep",
> -		 get_object_directory(), sha1_to_hex(pack_data->sha1));
> -	keep_fd = odb_pack_keep(name);
> +	odb_pack_name(&name, pack_data->sha1, "keep");
> +	keep_fd = odb_pack_keep(name.buf);
>  	if (keep_fd < 0)
>  		die_errno("cannot create keep file");
>  	write_or_die(keep_fd, keep_msg, strlen(keep_msg));
>  	if (close(keep_fd))
>  		die_errno("failed to write keep file");
>  
> -	snprintf(name, sizeof(name), "%s/pack/pack-%s.pack",
> -		 get_object_directory(), sha1_to_hex(pack_data->sha1));
> -	if (finalize_object_file(pack_data->pack_name, name))
> +	odb_pack_name(&name, pack_data->sha1, "pack");
> +	if (finalize_object_file(pack_data->pack_name, name.buf))
>  		die("cannot store pack file");
>  
> -	snprintf(name, sizeof(name), "%s/pack/pack-%s.idx",
> -		 get_object_directory(), sha1_to_hex(pack_data->sha1));
> -	if (finalize_object_file(curr_index_name, name))
> +	odb_pack_name(&name, pack_data->sha1, "idx");
> +	if (finalize_object_file(curr_index_name, name.buf))
>  		die("cannot store index file");
>  	free((void *)curr_index_name);
> -	return name;
> +	return strbuf_detach(&name, NULL);

Hmm, who deletes this, ...

>  }
>  
>  static void unkeep_all_packs(void)
>  {
> -	static char name[PATH_MAX];
> +	struct strbuf name = STRBUF_INIT;
>  	int k;
>  
>  	for (k = 0; k < pack_id; k++) {
>  		struct packed_git *p = all_packs[k];
> -		snprintf(name, sizeof(name), "%s/pack/pack-%s.keep",
> -			 get_object_directory(), sha1_to_hex(p->sha1));
> -		unlink_or_warn(name);
> +		odb_pack_name(&name, p->sha1, "keep");
> +		unlink_or_warn(name.buf);
>  	}
> +	strbuf_release(&name);
>  }
>  
>  static int loosen_small_pack(const struct packed_git *p)
> @@ -1035,6 +1032,7 @@ static void end_packfile(void)
>  			die("core git rejected index %s", idx_name);
>  		all_packs[pack_id] = new_p;
>  		install_packed_git(new_p);
> +		free(idx_name);

Ah, OK. Yep, no leaks here. ;-)

>  
>  		/* Print the boundary */
>  		if (pack_edges) {
> 

ATB,
Ramsay Jones

Re: [PATCH v2 5/5] index-pack: make pointer-alias fallbacks safer

[Ramsay Jones]

On 16/03/17 14:27, Jeff King wrote:
> The final() function accepts a NULL value for certain
> parameters, and falls back to writing into a reusable "name"
> buffer, and then either:
> 
>   1. For "keep_name", requiring all uses to do "keep_name ?
>      keep_name : name.buf". This is awkward, and it's easy
>      to accidentally look at the maybe-NULL keep_name.
> 
>   2. For "final_index_name" and "final_pack_name", aliasing
>      those pointers to the "name" buffer. This is easier to
>      use, but the aliased pointers become invalid after the
>      buffer is reused (this isn't a bug now, but it's a
>      potential pitfall).
> 
> One way to make this safer would be to introduce an extra
> pointer to do the aliasing, and have its lifetime match the
> validity of the "name" buffer. But it's still easy to
> accidentally use the wrong name (i.e., to use
> "final_pack_name" instead of the aliased pointer).
> 
> Instead, let's use three separate buffers that will remain
> valid through the function. That makes it safe to alias the
> pointers and use them consistently. The extra allocations
> shouldn't matter, as this function is not performance
> sensitive.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/index-pack.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index dcb346ab7..88d205f85 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -1386,7 +1386,9 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
>  		  unsigned char *sha1)
>  {
>  	const char *report = "pack";
> -	struct strbuf name = STRBUF_INIT;
> +	struct strbuf pack_name = STRBUF_INIT;
> +	struct strbuf index_name = STRBUF_INIT;
> +	struct strbuf keep_name_buf = STRBUF_INIT;
>  	int err;
>  
>  	if (!from_stdin) {
> @@ -1402,13 +1404,13 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
>  		int keep_fd, keep_msg_len = strlen(keep_msg);
>  
>  		if (!keep_name)
> -			odb_pack_name(&name, sha1, "keep");
> +			keep_name = odb_pack_name(&keep_name_buf, sha1, "keep");
>  
> -		keep_fd = odb_pack_keep(keep_name ? keep_name : name.buf);
> +		keep_fd = odb_pack_keep(keep_name);
>  		if (keep_fd < 0) {
>  			if (errno != EEXIST)
>  				die_errno(_("cannot write keep file '%s'"),
> -					  keep_name ? keep_name : name.buf);
> +					  keep_name);
>  		} else {
>  			if (keep_msg_len > 0) {
>  				write_or_die(keep_fd, keep_msg, keep_msg_len);
> @@ -1416,14 +1418,14 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
>  			}
>  			if (close(keep_fd) != 0)
>  				die_errno(_("cannot close written keep file '%s'"),
> -					  keep_name ? keep_name : name.buf);
> +					  keep_name);
>  			report = "keep";
>  		}
>  	}
>  
>  	if (final_pack_name != curr_pack_name) {
>  		if (!final_pack_name)
> -			final_pack_name = odb_pack_name(&name, sha1, "pack");
> +			final_pack_name = odb_pack_name(&pack_name, sha1, "pack");
>  		if (finalize_object_file(curr_pack_name, final_pack_name))
>  			die(_("cannot store pack file"));
>  	} else if (from_stdin)
> @@ -1431,7 +1433,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
>  
>  	if (final_index_name != curr_index_name) {
>  		if (!final_index_name)
> -			final_index_name = odb_pack_name(&name, sha1, "idx");
> +			final_index_name = odb_pack_name(&index_name, sha1, "idx");
>  		if (finalize_object_file(curr_index_name, final_index_name))
>  			die(_("cannot store index file"));
>  	} else
> @@ -1458,7 +1460,9 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
>  		}
>  	}
>  
> -	strbuf_release(&name);
> +	strbuf_release(&index_name);
> +	strbuf_release(&pack_name);
> +	strbuf_release(&keep_name_buf);
>  }
>  
>  static int git_index_pack_config(const char *k, const char *v, void *cb)
> 

Yep, much better.

ATB,
Ramsay Jones

Re: [PATCH v2 2/5] sha1_file.c: make pack-name helper globally accessible

[Ramsay Jones]

On 16/03/17 14:31, Jeff King wrote:
> On Thu, Mar 16, 2017 at 10:27:06AM -0400, Jeff King wrote:
> 
>> -/*
>> - * 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,
>> -				struct strbuf *buf,
>> -				const char *which)
>> + 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), which);
>> +		    sha1_to_hex(sha1), ext);
>>  	return buf->buf;
>>  }
> 
> Incidentally, this entire function could be implemented as:
> 
>   return git_path_buf(buf, "objects/pack/pack-%s.%s",
>                       sha1_to_hex(sha1), ext);
> 
> as the git_path() functions are smart enough to replace "objects/" with
> the true object directory when necessary. I don't know if people find
> that more or less readable. Since it's buried in a helper function, I
> doubt it matters much either way. The git_path functions do also do some
> path normalization, which might be of value.

Hmm, I don't have strong feelings either way.

However, I note that the only normalization going on (that I can see)
is to remove .//* from the beginning of the resulting string. I don't
know why, but I guess it is to cater to people using the various
GIT_ environment variables doing things like:

   $ GIT_OBJECT_DIRECTORY=./my-objects git ....

It has always puzzled me slightly, why the git_path functions do this
normalization, but (for example) setup_git_env(), git_path_from_env(),
get_common_dir(), ... don't! ;-)

ATB,
Ramsay Jones

Re: [PATCH v2 2/5] sha1_file.c: make pack-name helper globally accessible

[Jeff King]

On Thu, Mar 16, 2017 at 05:03:03PM +0000, Ramsay Jones wrote:

> > Incidentally, this entire function could be implemented as:
> > 
> >   return git_path_buf(buf, "objects/pack/pack-%s.%s",
> >                       sha1_to_hex(sha1), ext);
> > 
> > as the git_path() functions are smart enough to replace "objects/" with
> > the true object directory when necessary. I don't know if people find
> > that more or less readable. Since it's buried in a helper function, I
> > doubt it matters much either way. The git_path functions do also do some
> > path normalization, which might be of value.
> 
> Hmm, I don't have strong feelings either way.
> 
> However, I note that the only normalization going on (that I can see)
> is to remove .//* from the beginning of the resulting string. I don't
> know why, but I guess it is to cater to people using the various
> GIT_ environment variables doing things like:
> 
>    $ GIT_OBJECT_DIRECTORY=./my-objects git ....
> 
> It has always puzzled me slightly, why the git_path functions do this
> normalization, but (for example) setup_git_env(), git_path_from_env(),
> get_common_dir(), ... don't! ;-)

I think there's some double-slash normalization (though not consistent
or thorough). So yeah, I'm not really sure the normalization is of great
value. I am surprised that there isn't a call to "convert_slashes()" in
there on Windows, as it seems to be sprinkled other places (like
prefix_filename()).

<shrug> If nobody's complaining, I'm happy to leave it as-is for now,
and if somebody comes up with a case where git_path normalization makes
a difference, we can look at switching it.

-Peff

Re: [PATCH v2 4/5] replace snprintf with odb_pack_name()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> +	struct strbuf name = STRBUF_INIT;
>  	int err;
>  
>  	if (!from_stdin) {
> @@ -1402,14 +1402,13 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
>  		int keep_fd, keep_msg_len = strlen(keep_msg);
>  
>  		if (!keep_name)
> -			snprintf(name, sizeof(name), "%s/pack/pack-%s.keep",
> -				 get_object_directory(), sha1_to_hex(sha1));
> +			odb_pack_name(&name, sha1, "keep");
>  
> -		keep_fd = odb_pack_keep(keep_name ? keep_name : name);
> +		keep_fd = odb_pack_keep(keep_name ? keep_name : name.buf);
>  		if (keep_fd < 0) {
>  			if (errno != EEXIST)
>  				die_errno(_("cannot write keep file '%s'"),
> -					  keep_name ? keep_name : name);
> +					  keep_name ? keep_name : name.buf);
>  		} else {
>  			if (keep_msg_len > 0) {
>  				write_or_die(keep_fd, keep_msg, keep_msg_len);
> @@ -1417,28 +1416,22 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
>  			}
>  			if (close(keep_fd) != 0)
>  				die_errno(_("cannot close written keep file '%s'"),
> -					  keep_name ? keep_name : name);
> +					  keep_name ? keep_name : name.buf);
>  			report = "keep";
>  		}
>  	}


The patch as-posted is obviously with less damage to the current
code, but the above makes me wonder if it makes simpler to do

	if (keep_name)
		strbuf_addstr(&name, keep_name);
	else
		odb_pack_name(&name, sha1, "keep");

so that we can always use name.buf without having to do "?:" thing.

Re: [PATCH v2 4/5] replace snprintf with odb_pack_name()

[Jeff King]

On Thu, Mar 16, 2017 at 11:33:36AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > +	struct strbuf name = STRBUF_INIT;
> >  	int err;
> >  
> >  	if (!from_stdin) {
> > @@ -1402,14 +1402,13 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
> >  		int keep_fd, keep_msg_len = strlen(keep_msg);
> >  
> >  		if (!keep_name)
> > -			snprintf(name, sizeof(name), "%s/pack/pack-%s.keep",
> > -				 get_object_directory(), sha1_to_hex(sha1));
> > +			odb_pack_name(&name, sha1, "keep");
> >  
> > -		keep_fd = odb_pack_keep(keep_name ? keep_name : name);
> > +		keep_fd = odb_pack_keep(keep_name ? keep_name : name.buf);
> >  		if (keep_fd < 0) {
> >  			if (errno != EEXIST)
> >  				die_errno(_("cannot write keep file '%s'"),
> > -					  keep_name ? keep_name : name);
> > +					  keep_name ? keep_name : name.buf);
> >  		} else {
> >  			if (keep_msg_len > 0) {
> >  				write_or_die(keep_fd, keep_msg, keep_msg_len);
> > @@ -1417,28 +1416,22 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
> >  			}
> >  			if (close(keep_fd) != 0)
> >  				die_errno(_("cannot close written keep file '%s'"),
> > -					  keep_name ? keep_name : name);
> > +					  keep_name ? keep_name : name.buf);
> >  			report = "keep";
> >  		}
> >  	}
> 
> 
> The patch as-posted is obviously with less damage to the current
> code, but the above makes me wonder if it makes simpler to do
> 
> 	if (keep_name)
> 		strbuf_addstr(&name, keep_name);
> 	else
> 		odb_pack_name(&name, sha1, "keep");
> 
> so that we can always use name.buf without having to do "?:" thing.

See the next patch. :)

-Peff

Re: [PATCH v2 4/5] replace snprintf with odb_pack_name()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> so that we can always use name.buf without having to do "?:" thing.
>
> See the next patch. :)

Nice ;-)  Thanks, queued the whole thing.