[v2,2/3] multi-pack-index: use --object-dir real path

Commit Message

Derrick Stolee April 25, 2022, 6:27 p.m. UTC

From: Derrick Stolee <[e-mail]>

The --object-dir argument to 'git multi-pack-index' allows a user to
specify an alternate to use instead of the local $GITDIR. This is used
by third-party tools like VFS for Git to maintain the pack-files in a
"shared object cache" used by multiple clones.

On Windows, the user can specify a path using a Windows-style file path
with backslashes such as "C:\Path\To\ObjectDir". This same path style is
used in the .git/objects/info/alternates file, so it already matches the
path of that alternate. However, find_odb() converts these paths to
real-paths for the comparison, which use forward slashes. As of the
previous change, lookup_multi_pack_index() uses real-paths, so it
correctly finds the target multi-pack-index when given these paths.

Some commands such as 'git multi-pack-index repack' call child processes
using the object_dir value, so it can be helpful to convert the path to
the real-path before sending it to those locations.

Add a callback to convert the real path immediately upon parsing the
argument. We need to be careful that we don't store the exact value out
of get_object_directory() and free it, or we could corrupt a later use
of the_repository->objects->odb->path.

We don't use get_object_directory() for the initial instantiation in
cmd_multi_pack_index() because we need 'git multi-pack-index -h' to work
without a Git repository.

Signed-off-by: Derrick Stolee <[e-mail]>
---
 builtin/multi-pack-index.c | 45 ++++++++++++++++++++++++++++----------
 1 file changed, 34 insertions(+), 11 deletions(-)

Comments

Junio C Hamano April 25, 2022, 6:58 p.m. UTC | #1

"Derrick Stolee via GitGitGadget" <[e-mail]> writes:

> From: Derrick Stolee <[e-mail]>
>
> The --object-dir argument to 'git multi-pack-index' allows a user to
> specify an alternate to use instead of the local $GITDIR. This is used
> by third-party tools like VFS for Git to maintain the pack-files in a
> "shared object cache" used by multiple clones.
>
> On Windows, the user can specify a path using a Windows-style file path
> with backslashes such as "C:\Path\To\ObjectDir". This same path style is
> used in the .git/objects/info/alternates file, so it already matches the
> path of that alternate. However, find_odb() converts these paths to
> real-paths for the comparison, which use forward slashes. As of the
> previous change, lookup_multi_pack_index() uses real-paths, so it
> correctly finds the target multi-pack-index when given these paths.
>
> Some commands such as 'git multi-pack-index repack' call child processes
> using the object_dir value, so it can be helpful to convert the path to
> the real-path before sending it to those locations.
>
> Add a callback to convert the real path immediately upon parsing the
> argument. We need to be careful that we don't store the exact value out
> of get_object_directory() and free it, or we could corrupt a later use
> of the_repository->objects->odb->path.
>
> We don't use get_object_directory() for the initial instantiation in
> cmd_multi_pack_index() because we need 'git multi-pack-index -h' to work
> without a Git repository.
>
> Signed-off-by: Derrick Stolee <[e-mail]>
> ---
>  builtin/multi-pack-index.c | 45 ++++++++++++++++++++++++++++----------
>  1 file changed, 34 insertions(+), 11 deletions(-)

Much nicer compared to the previous round, by doing the
normalization early.

Will queue.  thanks.

Patch

diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
index 4480ba39827..5edbb7fe86e 100644
--- a/builtin/multi-pack-index.c
+++ b/builtin/multi-pack-index.c
@@ -44,7 +44,7 @@  static char const * const builtin_multi_pack_index_usage[] = {
 };
 
 static struct opts_multi_pack_index {
-	const char *object_dir;
+	char *object_dir;
 	const char *preferred_pack;
 	const char *refs_snapshot;
 	unsigned long batch_size;
@@ -52,9 +52,23 @@  static struct opts_multi_pack_index {
 	int stdin_packs;
 } opts;
 
+
+static int parse_object_dir(const struct option *opt, const char *arg,
+			    int unset)
+{
+	free(opts.object_dir);
+	if (unset)
+		opts.object_dir = xstrdup(get_object_directory());
+	else
+		opts.object_dir = real_pathdup(arg, 1);
+	return 0;
+}
+
 static struct option common_opts[] = {
-	OPT_FILENAME(0, "object-dir", &opts.object_dir,
-	  N_("object directory containing set of packfile and pack-index pairs")),
+	OPT_CALLBACK(0, "object-dir", &opts.object_dir,
+	  N_("directory"),
+	  N_("object directory containing set of packfile and pack-index pairs"),
+	  parse_object_dir),
 	OPT_END(),
 };
 
@@ -232,31 +246,40 @@  static int cmd_multi_pack_index_repack(int argc, const char **argv)
 int cmd_multi_pack_index(int argc, const char **argv,
 			 const char *prefix)
 {
+	int res;
 	struct option *builtin_multi_pack_index_options = common_opts;
 
 	git_config(git_default_config, NULL);
 
+	if (the_repository &&
+	    the_repository->objects &&
+	    the_repository->objects->odb)
+		opts.object_dir = xstrdup(the_repository->objects->odb->path);
+
 	argc = parse_options(argc, argv, prefix,
 			     builtin_multi_pack_index_options,
 			     builtin_multi_pack_index_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
 
-	if (!opts.object_dir)
-		opts.object_dir = get_object_directory();
-
 	if (!argc)
 		goto usage;
 
 	if (!strcmp(argv[0], "repack"))
-		return cmd_multi_pack_index_repack(argc, argv);
+		res = cmd_multi_pack_index_repack(argc, argv);
 	else if (!strcmp(argv[0], "write"))
-		return cmd_multi_pack_index_write(argc, argv);
+		res =  cmd_multi_pack_index_write(argc, argv);
 	else if (!strcmp(argv[0], "verify"))
-		return cmd_multi_pack_index_verify(argc, argv);
+		res =  cmd_multi_pack_index_verify(argc, argv);
 	else if (!strcmp(argv[0], "expire"))
-		return cmd_multi_pack_index_expire(argc, argv);
+		res =  cmd_multi_pack_index_expire(argc, argv);
+	else {
+		error(_("unrecognized subcommand: %s"), argv[0]);
+		goto usage;
+	}
+
+	free(opts.object_dir);
+	return res;
 
-	error(_("unrecognized subcommand: %s"), argv[0]);
 usage:
 	usage_with_options(builtin_multi_pack_index_usage,
 			   builtin_multi_pack_index_options);