{"thread":{"id":"57774","subject":"[PATCH 0/2] multi-pack-index: use real paths for --object-dir","startedAt":"2022-04-21T14:57:58Z","lastAt":"2022-04-25T18:58:44Z","messageCount":11,"participants":["Derrick Stolee via GitGitGadget","Victoria Dye","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"454080","messageId":"pull.1221.git.1650553069.gitgitgadget@gmail.com","threadId":"57774","inReplyTo":null,"subject":"[PATCH 0/2] multi-pack-index: use real paths for --object-dir","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-21T14:57:47Z","receivedAt":"2022-04-21T14:57:58Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"A VFS for Git user reported [1] that the background maintenance did not seem\nto be doing anything. At least, the 'git multi-pack-index expire' and 'git\nmulti-pack-index repack' steps did nothing. The 'write' subcommand worked to\ncollect all pack-files into a single multi-pack-index file, but the packs\nwere only growing in number instead of being maintained carefully.\n\n[1] https://github.com/microsoft/git/issues/497\n\nThe root issue is about path comparisons between Windows and Unix path\nstyles.\n\nThe fix is two-fold:\n\n * Patch 1 performs real-path normalization at a low-level where it is\n required.\n * Patch 2 performs real-path normalization at a high-level to be extra\n safe.\n\nI'm not exactly sure how to test this properly in the Git codebase, but I'm\nlooking to add some tests into VFS for Git to ensure this doesn't regress in\nthe future.\n\nThanks, -Stolee\n\nDerrick Stolee (2):\n midx: use real paths in lookup_multi_pack_index()\n multi-pack-index: use --object-dir real path\n\n builtin/multi-pack-index.c | 19 ++++++++++++++++---\n midx.c | 17 +++++++++++++----\n 2 files changed, 29 insertions(+), 7 deletions(-)\n\n\nbase-commit: 6cd33dceed60949e2dbc32e3f0f5e67c4c882e1e\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1221%2Fderrickstolee%2Fmidx-normalize-object-dir-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1221/derrickstolee/midx-normalize-object-dir-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1221\n-- \ngitgitgadget\n"},{"id":"454081","messageId":"34785a0c7cc6c3547951fb29fc2b3c1cf8be6f54.1650553069.git.gitgitgadget@gmail.com","threadId":"57774","inReplyTo":"pull.1221.git.1650553069.gitgitgadget@gmail.com","subject":"[PATCH 1/2] midx: use real paths in lookup_multi_pack_index()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-21T14:57:48Z","receivedAt":"2022-04-21T14:58:00Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThis helper looks for a parsed multi-pack-index whose object directory\nmatches the given object_dir. Before going into the loop over the parsed\nmulti-pack-indexes, it calls find_odb() to ensure that the given\nobject_dir is actually a known object directory.\n\nHowever, find_odb() uses real-path manipulations to compare the input to\nthe alternate directories. This same real-path comparison is not used in\nthe loop, leading to potential issues with the strcmp().\n\nUpdate the method to use the real-path values instead.\n\nSigned-off-by: Derrick Stolee \n---\n midx.c | 17 +++++++++++++----\n 1 file changed, 13 insertions(+), 4 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 107365d2114..3db0e47735f 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1132,17 +1132,26 @@ cleanup:\n static struct multi_pack_index *lookup_multi_pack_index(struct repository *r,\n \t\t\t\t\t\t\tconst char *object_dir)\n {\n+\tstruct multi_pack_index *result = NULL;\n \tstruct multi_pack_index *cur;\n+\tchar *obj_dir_real = real_pathdup(object_dir, 1);\n+\tstruct strbuf cur_path_real = STRBUF_INIT;\n \n \t/* Ensure the given object_dir is local, or a known alternate. */\n-\tfind_odb(r, object_dir);\n+\tfind_odb(r, obj_dir_real);\n \n \tfor (cur = get_multi_pack_index(r); cur; cur = cur->next) {\n-\t\tif (!strcmp(object_dir, cur->object_dir))\n-\t\t\treturn cur;\n+\t\tstrbuf_realpath(&cur_path_real, cur->object_dir, 1);\n+\t\tif (!strcmp(obj_dir_real, cur_path_real.buf)) {\n+\t\t\tresult = cur;\n+\t\t\tgoto cleanup;\n+\t\t}\n \t}\n \n-\treturn NULL;\n+cleanup:\n+\tfree(obj_dir_real);\n+\tstrbuf_release(&cur_path_real);\n+\treturn result;\n }\n \n static int write_midx_internal(const char *object_dir,\n-- \ngitgitgadget\n\n"},{"id":"454082","messageId":"0435406e2db6c5977928a2b0b0b79e66c0a078ee.1650553069.git.gitgitgadget@gmail.com","threadId":"57774","inReplyTo":"pull.1221.git.1650553069.gitgitgadget@gmail.com","subject":"[PATCH 2/2] multi-pack-index: use --object-dir real path","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-21T14:57:49Z","receivedAt":"2022-04-21T14:58:06Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe --object-dir argument to 'git multi-pack-index' allows a user to\nspecify an alternate to use instead of the local $GITDIR. This is used\nby third-party tools like VFS for Git to maintain the pack-files in a\n\"shared object cache\" used by multiple clones.\n\nOn Windows, the user can specify a path using a Windows-style file path\nwith backslashes such as \"C:\\Path\\To\\ObjectDir\". This same path style is\nused in the .git/objects/info/alternates file, so it already matches the\npath of that alternate. However, find_odb() converts these paths to\nreal-paths for the comparison, which use forward slashes. As of the\nprevious change, lookup_multi_pack_index() uses real-paths, so it\ncorrectly finds the target multi-pack-index when given these paths.\n\nSome commands such as 'git multi-pack-index repack' call child processes\nusing the object_dir value, so it can be helpful to convert the path to\nthe real-path before sending it to those locations.\n\nAdding the normalization in builtin/multi-pack-index.c is a little\ncomplicated because of how the sub-commands were split in 60ca94769\n(builtin/multi-pack-index.c: split sub-commands, 2021-03-30). The\n--object-dir argument could be parsed before the sub-command name _or_\nafter it. Thus, create a normalize_object_dir() helper to call after all\narguments are parsed, but before any logic is run on that object dir.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/multi-pack-index.c | 19 ++++++++++++++++---\n 1 file changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c\nindex 4480ba39827..3853960f9ba 100644\n--- a/builtin/multi-pack-index.c\n+++ b/builtin/multi-pack-index.c\n@@ -90,6 +90,14 @@ static void read_packs_from_stdin(struct string_list *to)\n \tstrbuf_release(&buf);\n }\n \n+static void normalize_object_dir(void)\n+{\n+\tif (!opts.object_dir)\n+\t\topts.object_dir = get_object_directory();\n+\telse\n+\t\topts.object_dir = real_pathdup(opts.object_dir, 1);\n+}\n+\n static int cmd_multi_pack_index_write(int argc, const char **argv)\n {\n \tstruct option *options;\n@@ -127,6 +135,8 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)\n \n \tFREE_AND_NULL(options);\n \n+\tnormalize_object_dir();\n+\n \tif (opts.stdin_packs) {\n \t\tstruct string_list packs = STRING_LIST_INIT_DUP;\n \t\tint ret;\n@@ -169,6 +179,8 @@ static int cmd_multi_pack_index_verify(int argc, const char **argv)\n \n \tFREE_AND_NULL(options);\n \n+\tnormalize_object_dir();\n+\n \treturn verify_midx_file(the_repository, opts.object_dir, opts.flags);\n }\n \n@@ -195,6 +207,8 @@ static int cmd_multi_pack_index_expire(int argc, const char **argv)\n \n \tFREE_AND_NULL(options);\n \n+\tnormalize_object_dir();\n+\n \treturn expire_midx_packs(the_repository, opts.object_dir, opts.flags);\n }\n \n@@ -225,6 +239,8 @@ static int cmd_multi_pack_index_repack(int argc, const char **argv)\n \n \tFREE_AND_NULL(options);\n \n+\tnormalize_object_dir();\n+\n \treturn midx_repack(the_repository, opts.object_dir,\n \t\t\t (size_t)opts.batch_size, opts.flags);\n }\n@@ -241,9 +257,6 @@ int cmd_multi_pack_index(int argc, const char **argv,\n \t\t\t builtin_multi_pack_index_usage,\n \t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n \n-\tif (!opts.object_dir)\n-\t\topts.object_dir = get_object_directory();\n-\n \tif (!argc)\n \t\tgoto usage;\n \n-- \ngitgitgadget\n"},{"id":"454157","messageId":"936e7f12-362d-3303-ddf4-5b40f17a0b9e@github.com","threadId":"57774","inReplyTo":"0435406e2db6c5977928a2b0b0b79e66c0a078ee.1650553069.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] multi-pack-index: use --object-dir real path","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-04-21T19:50:51Z","receivedAt":"2022-04-21T19:50:55Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n> \n> The --object-dir argument to 'git multi-pack-index' allows a user to\n> specify an alternate to use instead of the local $GITDIR. This is used\n> by third-party tools like VFS for Git to maintain the pack-files in a\n> \"shared object cache\" used by multiple clones.\n> \n> On Windows, the user can specify a path using a Windows-style file path\n> with backslashes such as \"C:\\Path\\To\\ObjectDir\". This same path style is\n> used in the .git/objects/info/alternates file, so it already matches the\n> path of that alternate. However, find_odb() converts these paths to\n> real-paths for the comparison, which use forward slashes. As of the\n> previous change, lookup_multi_pack_index() uses real-paths, so it\n> correctly finds the target multi-pack-index when given these paths.\n> \n> Some commands such as 'git multi-pack-index repack' call child processes\n> using the object_dir value, so it can be helpful to convert the path to\n> the real-path before sending it to those locations.\n> \n> Adding the normalization in builtin/multi-pack-index.c is a little\n> complicated because of how the sub-commands were split in 60ca94769\n> (builtin/multi-pack-index.c: split sub-commands, 2021-03-30). The\n> --object-dir argument could be parsed before the sub-command name _or_\n> after it. Thus, create a normalize_object_dir() helper to call after all\n> arguments are parsed, but before any logic is run on that object dir.\n> \n> Signed-off-by: Derrick Stolee \n> ---\n> builtin/multi-pack-index.c | 19 ++++++++++++++++---\n> 1 file changed, 16 insertions(+), 3 deletions(-)\n> \n> diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c\n> index 4480ba39827..3853960f9ba 100644\n> --- a/builtin/multi-pack-index.c\n> +++ b/builtin/multi-pack-index.c\n> @@ -90,6 +90,14 @@ static void read_packs_from_stdin(struct string_list *to)\n> \tstrbuf_release(&buf);\n> }\n> \n> +static void normalize_object_dir(void)\n> +{\n> +\tif (!opts.object_dir)\n> +\t\topts.object_dir = get_object_directory();\n> +\telse\n> +\t\topts.object_dir = real_pathdup(opts.object_dir, 1);\n> +}\n> +\n\nRather than copy the 'normalize_object_dir()' calls to every subcommand, you\ncould \"centralize\" this by making the 'object_dir' option an 'OPT_CALLBACK'\noption, something like:\n\nstatic struct option common_opts[] = {\n\tOPT_CALLBACK(0, \"object-dir\", &opts.object_dir, N_(\"file\"),\n\t\t N_(\"object directory containing set of packfile and pack-index pairs\"),\n\t\t normalize_object_dir),\n\tOPT_END(),\n};\n\nIt would require changing the function signature of 'normalize_object_dir'\nto match what's shown in 'Documentation/technical/api-parse-options.txt',\nand it potentially needs prefix handling similar to what's done in\nparse-options.c:get_value() (which internally calls 'fix_filename()' for\nfilename opts), but I think it's probably worth reducing duplication here\nand avoiding the need to add 'normalize_object_dir()' to any new subcommand\nin the future.\n\n> static int cmd_multi_pack_index_write(int argc, const char **argv)\n> {\n> \tstruct option *options;\n> @@ -127,6 +135,8 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)\n> \n> \tFREE_AND_NULL(options);\n> \n> +\tnormalize_object_dir();\n> +\n> \tif (opts.stdin_packs) {\n> \t\tstruct string_list packs = STRING_LIST_INIT_DUP;\n> \t\tint ret;\n> @@ -169,6 +179,8 @@ static int cmd_multi_pack_index_verify(int argc, const char **argv)\n> \n> \tFREE_AND_NULL(options);\n> \n> +\tnormalize_object_dir();\n> +\n> \treturn verify_midx_file(the_repository, opts.object_dir, opts.flags);\n> }\n> \n> @@ -195,6 +207,8 @@ static int cmd_multi_pack_index_expire(int argc, const char **argv)\n> \n> \tFREE_AND_NULL(options);\n> \n> +\tnormalize_object_dir();\n> +\n> \treturn expire_midx_packs(the_repository, opts.object_dir, opts.flags);\n> }\n> \n> @@ -225,6 +239,8 @@ static int cmd_multi_pack_index_repack(int argc, const char **argv)\n> \n> \tFREE_AND_NULL(options);\n> \n> +\tnormalize_object_dir();\n> +\n> \treturn midx_repack(the_repository, opts.object_dir,\n> \t\t\t (size_t)opts.batch_size, opts.flags);\n> }\n> @@ -241,9 +257,6 @@ int cmd_multi_pack_index(int argc, const char **argv,\n> \t\t\t builtin_multi_pack_index_usage,\n> \t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n> \n> -\tif (!opts.object_dir)\n> -\t\topts.object_dir = get_object_directory();\n> -\n> \tif (!argc)\n> \t\tgoto usage;\n> \n\n"},{"id":"454162","messageId":"be6aed1c-6237-3090-983f-42ef7e043d72@github.com","threadId":"57774","inReplyTo":"936e7f12-362d-3303-ddf4-5b40f17a0b9e@github.com","subject":"Re: [PATCH 2/2] multi-pack-index: use --object-dir real path","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-04-21T19:55:39Z","receivedAt":"2022-04-21T19:55:45Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/21/2022 3:50 PM, Victoria Dye wrote:\n> Derrick Stolee via GitGitGadget wrote:\n>> +static void normalize_object_dir(void)\n>> +{\n>> +\tif (!opts.object_dir)\n>> +\t\topts.object_dir = get_object_directory();\n>> +\telse\n>> +\t\topts.object_dir = real_pathdup(opts.object_dir, 1);\n>> +}\n>> +\n> \n> Rather than copy the 'normalize_object_dir()' calls to every subcommand, you\n> could \"centralize\" this by making the 'object_dir' option an 'OPT_CALLBACK'\n> option, something like:\n> \n> static struct option common_opts[] = {\n> \tOPT_CALLBACK(0, \"object-dir\", &opts.object_dir, N_(\"file\"),\n> \t\t N_(\"object directory containing set of packfile and pack-index pairs\"),\n> \t\t normalize_object_dir),\n> \tOPT_END(),\n> };\n> \n> It would require changing the function signature of 'normalize_object_dir'\n> to match what's shown in 'Documentation/technical/api-parse-options.txt',\n> and it potentially needs prefix handling similar to what's done in\n> parse-options.c:get_value() (which internally calls 'fix_filename()' for\n> filename opts), but I think it's probably worth reducing duplication here\n> and avoiding the need to add 'normalize_object_dir()' to any new subcommand\n> in the future.\n\nThanks! I agree that that would be a cleaner approach, especially if\na new subcommand is added in the future.\n\nThanks,\n-Stolee\n"},{"id":"454176","messageId":"xmqqy1zyfaev.fsf@gitster.g","threadId":"57774","inReplyTo":"936e7f12-362d-3303-ddf4-5b40f17a0b9e@github.com","subject":"Re: [PATCH 2/2] multi-pack-index: use --object-dir real path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-21T20:28:56Z","receivedAt":"2022-04-21T20:29:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Victoria Dye writes:\n\n>> +static void normalize_object_dir(void)\n>> +{\n>> +\tif (!opts.object_dir)\n>> +\t\topts.object_dir = get_object_directory();\n>> +\telse\n>> +\t\topts.object_dir = real_pathdup(opts.object_dir, 1);\n>> +}\n>> +\n>\n> Rather than copy the 'normalize_object_dir()' calls to every subcommand, you\n> could \"centralize\" this by making the 'object_dir' option an 'OPT_CALLBACK'\n> option, something like:\n>\n> static struct option common_opts[] = {\n> \tOPT_CALLBACK(0, \"object-dir\", &opts.object_dir, N_(\"file\"),\n> \t\t N_(\"object directory containing set of packfile and pack-index pairs\"),\n> \t\t normalize_object_dir),\n> \tOPT_END(),\n> };\n>\n> It would require changing the function signature of 'normalize_object_dir'\n> to match what's shown in 'Documentation/technical/api-parse-options.txt',\n> and it potentially needs prefix handling similar to what's done in\n> parse-options.c:get_value() (which internally calls 'fix_filename()' for\n> filename opts), but I think it's probably worth reducing duplication here\n> and avoiding the need to add 'normalize_object_dir()' to any new subcommand\n> in the future.\n\nGood suggestion. Thanks, both, for taking care of this.\n\nAre there other places that we take end-user input and treat it as a\npathname without necessary normalization, I wonder. The codepath\nfixed by this series is relatively new, and I am not surprised such\na bug was still there and hopefully it was an isolated remaining bug.\n"},{"id":"454400","messageId":"pull.1221.v2.git.1650911234.gitgitgadget@gmail.com","threadId":"57774","inReplyTo":"pull.1221.git.1650553069.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] multi-pack-index: use real paths for --object-dir","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-25T18:27:11Z","receivedAt":"2022-04-25T18:27:25Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"A VFS for Git user reported [1] that the background maintenance did not seem\nto be doing anything. At least, the 'git multi-pack-index expire' and 'git\nmulti-pack-index repack' steps did nothing. The 'write' subcommand worked to\ncollect all pack-files into a single multi-pack-index file, but the packs\nwere only growing in number instead of being maintained carefully.\n\n[1] https://github.com/microsoft/git/issues/497\n\nThe root issue is about path comparisons between Windows and Unix path\nstyles.\n\nThe fix is two-fold:\n\n * Patch 1 performs real-path normalization at a low-level where it is\n required.\n * Patch 2 performs real-path normalization at a high-level to be extra\n safe.\n * Patch 3 adds an extra API hardening that I noticed while making Patch 2\n for v2.\n\nI'm not exactly sure how to test this properly in the Git codebase, but I'm\nlooking to add some tests into VFS for Git to ensure this doesn't regress in\nthe future.\n\n\nUpdates in v2\n=============\n\n * Patch 2 is rewritten to use an option callback, as recommended by\n Victoria. This is more robust to future changes to the CLI.\n * Patch 3 is new, and something I found while working on patch 2.\n\nThanks, -Stolee\n\nDerrick Stolee (3):\n midx: use real paths in lookup_multi_pack_index()\n multi-pack-index: use --object-dir real path\n cache: use const char * for get_object_directory()\n\n builtin/multi-pack-index.c | 45 ++++++++++++++++++++++++++++----------\n cache.h | 2 +-\n environment.c | 2 +-\n midx.c | 17 ++++++++++----\n 4 files changed, 49 insertions(+), 17 deletions(-)\n\n\nbase-commit: 6cd33dceed60949e2dbc32e3f0f5e67c4c882e1e\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1221%2Fderrickstolee%2Fmidx-normalize-object-dir-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1221/derrickstolee/midx-normalize-object-dir-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1221\n\nRange-diff vs v1:\n\n 1: 34785a0c7cc = 1: 34785a0c7cc midx: use real paths in lookup_multi_pack_index()\n 2: 0435406e2db ! 2: fd580e79477 multi-pack-index: use --object-dir real path\n @@ Commit message\n using the object_dir value, so it can be helpful to convert the path to\n the real-path before sending it to those locations.\n \n - Adding the normalization in builtin/multi-pack-index.c is a little\n - complicated because of how the sub-commands were split in 60ca94769\n - (builtin/multi-pack-index.c: split sub-commands, 2021-03-30). The\n - --object-dir argument could be parsed before the sub-command name _or_\n - after it. Thus, create a normalize_object_dir() helper to call after all\n - arguments are parsed, but before any logic is run on that object dir.\n + Add a callback to convert the real path immediately upon parsing the\n + argument. We need to be careful that we don't store the exact value out\n + of get_object_directory() and free it, or we could corrupt a later use\n + of the_repository->objects->odb->path.\n +\n + We don't use get_object_directory() for the initial instantiation in\n + cmd_multi_pack_index() because we need 'git multi-pack-index -h' to work\n + without a Git repository.\n \n Signed-off-by: Derrick Stolee \n \n ## builtin/multi-pack-index.c ##\n -@@ builtin/multi-pack-index.c: static void read_packs_from_stdin(struct string_list *to)\n - \tstrbuf_release(&buf);\n - }\n +@@ builtin/multi-pack-index.c: static char const * const builtin_multi_pack_index_usage[] = {\n + };\n + \n + static struct opts_multi_pack_index {\n +-\tconst char *object_dir;\n ++\tchar *object_dir;\n + \tconst char *preferred_pack;\n + \tconst char *refs_snapshot;\n + \tunsigned long batch_size;\n +@@ builtin/multi-pack-index.c: static struct opts_multi_pack_index {\n + \tint stdin_packs;\n + } opts;\n \n -+static void normalize_object_dir(void)\n ++\n ++static int parse_object_dir(const struct option *opt, const char *arg,\n ++\t\t\t int unset)\n +{\n -+\tif (!opts.object_dir)\n -+\t\topts.object_dir = get_object_directory();\n ++\tfree(opts.object_dir);\n ++\tif (unset)\n ++\t\topts.object_dir = xstrdup(get_object_directory());\n +\telse\n -+\t\topts.object_dir = real_pathdup(opts.object_dir, 1);\n ++\t\topts.object_dir = real_pathdup(arg, 1);\n ++\treturn 0;\n +}\n +\n - static int cmd_multi_pack_index_write(int argc, const char **argv)\n - {\n - \tstruct option *options;\n -@@ builtin/multi-pack-index.c: static int cmd_multi_pack_index_write(int argc, const char **argv)\n - \n - \tFREE_AND_NULL(options);\n - \n -+\tnormalize_object_dir();\n -+\n - \tif (opts.stdin_packs) {\n - \t\tstruct string_list packs = STRING_LIST_INIT_DUP;\n - \t\tint ret;\n -@@ builtin/multi-pack-index.c: static int cmd_multi_pack_index_verify(int argc, const char **argv)\n - \n - \tFREE_AND_NULL(options);\n - \n -+\tnormalize_object_dir();\n -+\n - \treturn verify_midx_file(the_repository, opts.object_dir, opts.flags);\n - }\n - \n -@@ builtin/multi-pack-index.c: static int cmd_multi_pack_index_expire(int argc, const char **argv)\n - \n - \tFREE_AND_NULL(options);\n - \n -+\tnormalize_object_dir();\n -+\n - \treturn expire_midx_packs(the_repository, opts.object_dir, opts.flags);\n - }\n + static struct option common_opts[] = {\n +-\tOPT_FILENAME(0, \"object-dir\", &opts.object_dir,\n +-\t N_(\"object directory containing set of packfile and pack-index pairs\")),\n ++\tOPT_CALLBACK(0, \"object-dir\", &opts.object_dir,\n ++\t N_(\"directory\"),\n ++\t N_(\"object directory containing set of packfile and pack-index pairs\"),\n ++\t parse_object_dir),\n + \tOPT_END(),\n + };\n \n @@ builtin/multi-pack-index.c: static int cmd_multi_pack_index_repack(int argc, const char **argv)\n + int cmd_multi_pack_index(int argc, const char **argv,\n + \t\t\t const char *prefix)\n + {\n ++\tint res;\n + \tstruct option *builtin_multi_pack_index_options = common_opts;\n \n - \tFREE_AND_NULL(options);\n + \tgit_config(git_default_config, NULL);\n \n -+\tnormalize_object_dir();\n ++\tif (the_repository &&\n ++\t the_repository->objects &&\n ++\t the_repository->objects->odb)\n ++\t\topts.object_dir = xstrdup(the_repository->objects->odb->path);\n +\n - \treturn midx_repack(the_repository, opts.object_dir,\n - \t\t\t (size_t)opts.batch_size, opts.flags);\n - }\n -@@ builtin/multi-pack-index.c: int cmd_multi_pack_index(int argc, const char **argv,\n + \targc = parse_options(argc, argv, prefix,\n + \t\t\t builtin_multi_pack_index_options,\n \t\t\t builtin_multi_pack_index_usage,\n \t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n \n @@ builtin/multi-pack-index.c: int cmd_multi_pack_index(int argc, const char **argv\n \tif (!argc)\n \t\tgoto usage;\n \n + \tif (!strcmp(argv[0], \"repack\"))\n +-\t\treturn cmd_multi_pack_index_repack(argc, argv);\n ++\t\tres = cmd_multi_pack_index_repack(argc, argv);\n + \telse if (!strcmp(argv[0], \"write\"))\n +-\t\treturn cmd_multi_pack_index_write(argc, argv);\n ++\t\tres = cmd_multi_pack_index_write(argc, argv);\n + \telse if (!strcmp(argv[0], \"verify\"))\n +-\t\treturn cmd_multi_pack_index_verify(argc, argv);\n ++\t\tres = cmd_multi_pack_index_verify(argc, argv);\n + \telse if (!strcmp(argv[0], \"expire\"))\n +-\t\treturn cmd_multi_pack_index_expire(argc, argv);\n ++\t\tres = cmd_multi_pack_index_expire(argc, argv);\n ++\telse {\n ++\t\terror(_(\"unrecognized subcommand: %s\"), argv[0]);\n ++\t\tgoto usage;\n ++\t}\n ++\n ++\tfree(opts.object_dir);\n ++\treturn res;\n + \n +-\terror(_(\"unrecognized subcommand: %s\"), argv[0]);\n + usage:\n + \tusage_with_options(builtin_multi_pack_index_usage,\n + \t\t\t builtin_multi_pack_index_options);\n -: ----------- > 3: c9b13f0e146 cache: use const char * for get_object_directory()\n\n-- \ngitgitgadget\n"},{"id":"454401","messageId":"34785a0c7cc6c3547951fb29fc2b3c1cf8be6f54.1650911234.git.gitgitgadget@gmail.com","threadId":"57774","inReplyTo":"pull.1221.v2.git.1650911234.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] midx: use real paths in lookup_multi_pack_index()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-25T18:27:12Z","receivedAt":"2022-04-25T18:27:27Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThis helper looks for a parsed multi-pack-index whose object directory\nmatches the given object_dir. Before going into the loop over the parsed\nmulti-pack-indexes, it calls find_odb() to ensure that the given\nobject_dir is actually a known object directory.\n\nHowever, find_odb() uses real-path manipulations to compare the input to\nthe alternate directories. This same real-path comparison is not used in\nthe loop, leading to potential issues with the strcmp().\n\nUpdate the method to use the real-path values instead.\n\nSigned-off-by: Derrick Stolee \n---\n midx.c | 17 +++++++++++++----\n 1 file changed, 13 insertions(+), 4 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 107365d2114..3db0e47735f 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1132,17 +1132,26 @@ cleanup:\n static struct multi_pack_index *lookup_multi_pack_index(struct repository *r,\n \t\t\t\t\t\t\tconst char *object_dir)\n {\n+\tstruct multi_pack_index *result = NULL;\n \tstruct multi_pack_index *cur;\n+\tchar *obj_dir_real = real_pathdup(object_dir, 1);\n+\tstruct strbuf cur_path_real = STRBUF_INIT;\n \n \t/* Ensure the given object_dir is local, or a known alternate. */\n-\tfind_odb(r, object_dir);\n+\tfind_odb(r, obj_dir_real);\n \n \tfor (cur = get_multi_pack_index(r); cur; cur = cur->next) {\n-\t\tif (!strcmp(object_dir, cur->object_dir))\n-\t\t\treturn cur;\n+\t\tstrbuf_realpath(&cur_path_real, cur->object_dir, 1);\n+\t\tif (!strcmp(obj_dir_real, cur_path_real.buf)) {\n+\t\t\tresult = cur;\n+\t\t\tgoto cleanup;\n+\t\t}\n \t}\n \n-\treturn NULL;\n+cleanup:\n+\tfree(obj_dir_real);\n+\tstrbuf_release(&cur_path_real);\n+\treturn result;\n }\n \n static int write_midx_internal(const char *object_dir,\n-- \ngitgitgadget\n\n"},{"id":"454402","messageId":"fd580e7947743db545a839e81f027984e70b7717.1650911234.git.gitgitgadget@gmail.com","threadId":"57774","inReplyTo":"pull.1221.v2.git.1650911234.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] multi-pack-index: use --object-dir real path","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-25T18:27:13Z","receivedAt":"2022-04-25T18:27:28Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe --object-dir argument to 'git multi-pack-index' allows a user to\nspecify an alternate to use instead of the local $GITDIR. This is used\nby third-party tools like VFS for Git to maintain the pack-files in a\n\"shared object cache\" used by multiple clones.\n\nOn Windows, the user can specify a path using a Windows-style file path\nwith backslashes such as \"C:\\Path\\To\\ObjectDir\". This same path style is\nused in the .git/objects/info/alternates file, so it already matches the\npath of that alternate. However, find_odb() converts these paths to\nreal-paths for the comparison, which use forward slashes. As of the\nprevious change, lookup_multi_pack_index() uses real-paths, so it\ncorrectly finds the target multi-pack-index when given these paths.\n\nSome commands such as 'git multi-pack-index repack' call child processes\nusing the object_dir value, so it can be helpful to convert the path to\nthe real-path before sending it to those locations.\n\nAdd a callback to convert the real path immediately upon parsing the\nargument. We need to be careful that we don't store the exact value out\nof get_object_directory() and free it, or we could corrupt a later use\nof the_repository->objects->odb->path.\n\nWe don't use get_object_directory() for the initial instantiation in\ncmd_multi_pack_index() because we need 'git multi-pack-index -h' to work\nwithout a Git repository.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/multi-pack-index.c | 45 ++++++++++++++++++++++++++++----------\n 1 file changed, 34 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c\nindex 4480ba39827..5edbb7fe86e 100644\n--- a/builtin/multi-pack-index.c\n+++ b/builtin/multi-pack-index.c\n@@ -44,7 +44,7 @@ static char const * const builtin_multi_pack_index_usage[] = {\n };\n \n static struct opts_multi_pack_index {\n-\tconst char *object_dir;\n+\tchar *object_dir;\n \tconst char *preferred_pack;\n \tconst char *refs_snapshot;\n \tunsigned long batch_size;\n@@ -52,9 +52,23 @@ static struct opts_multi_pack_index {\n \tint stdin_packs;\n } opts;\n \n+\n+static int parse_object_dir(const struct option *opt, const char *arg,\n+\t\t\t int unset)\n+{\n+\tfree(opts.object_dir);\n+\tif (unset)\n+\t\topts.object_dir = xstrdup(get_object_directory());\n+\telse\n+\t\topts.object_dir = real_pathdup(arg, 1);\n+\treturn 0;\n+}\n+\n static struct option common_opts[] = {\n-\tOPT_FILENAME(0, \"object-dir\", &opts.object_dir,\n-\t N_(\"object directory containing set of packfile and pack-index pairs\")),\n+\tOPT_CALLBACK(0, \"object-dir\", &opts.object_dir,\n+\t N_(\"directory\"),\n+\t N_(\"object directory containing set of packfile and pack-index pairs\"),\n+\t parse_object_dir),\n \tOPT_END(),\n };\n \n@@ -232,31 +246,40 @@ static int cmd_multi_pack_index_repack(int argc, const char **argv)\n int cmd_multi_pack_index(int argc, const char **argv,\n \t\t\t const char *prefix)\n {\n+\tint res;\n \tstruct option *builtin_multi_pack_index_options = common_opts;\n \n \tgit_config(git_default_config, NULL);\n \n+\tif (the_repository &&\n+\t the_repository->objects &&\n+\t the_repository->objects->odb)\n+\t\topts.object_dir = xstrdup(the_repository->objects->odb->path);\n+\n \targc = parse_options(argc, argv, prefix,\n \t\t\t builtin_multi_pack_index_options,\n \t\t\t builtin_multi_pack_index_usage,\n \t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n \n-\tif (!opts.object_dir)\n-\t\topts.object_dir = get_object_directory();\n-\n \tif (!argc)\n \t\tgoto usage;\n \n \tif (!strcmp(argv[0], \"repack\"))\n-\t\treturn cmd_multi_pack_index_repack(argc, argv);\n+\t\tres = cmd_multi_pack_index_repack(argc, argv);\n \telse if (!strcmp(argv[0], \"write\"))\n-\t\treturn cmd_multi_pack_index_write(argc, argv);\n+\t\tres = cmd_multi_pack_index_write(argc, argv);\n \telse if (!strcmp(argv[0], \"verify\"))\n-\t\treturn cmd_multi_pack_index_verify(argc, argv);\n+\t\tres = cmd_multi_pack_index_verify(argc, argv);\n \telse if (!strcmp(argv[0], \"expire\"))\n-\t\treturn cmd_multi_pack_index_expire(argc, argv);\n+\t\tres = cmd_multi_pack_index_expire(argc, argv);\n+\telse {\n+\t\terror(_(\"unrecognized subcommand: %s\"), argv[0]);\n+\t\tgoto usage;\n+\t}\n+\n+\tfree(opts.object_dir);\n+\treturn res;\n \n-\terror(_(\"unrecognized subcommand: %s\"), argv[0]);\n usage:\n \tusage_with_options(builtin_multi_pack_index_usage,\n \t\t\t builtin_multi_pack_index_options);\n-- \ngitgitgadget\n\n"},{"id":"454403","messageId":"c9b13f0e1465f8dd41399bba533b3499fc8c8f5d.1650911234.git.gitgitgadget@gmail.com","threadId":"57774","inReplyTo":"pull.1221.v2.git.1650911234.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] cache: use const char * for get_object_directory()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-25T18:27:14Z","receivedAt":"2022-04-25T18:27:30Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe get_object_directory() method returns the exact string stored at\nthe_repository->objects->odb->path. The return type of \"char *\" implies\nthat the caller must keep track of the buffer and free() it when\ncomplete. This causes significant problems later when the ODB is\naccessed.\n\nUse \"const char *\" as the return type to avoid this confusion. There are\nno current callers that care about the non-const definition.\n\nSigned-off-by: Derrick Stolee \n---\n cache.h | 2 +-\n environment.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 6226f6a8a53..595582becc8 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -566,7 +566,7 @@ extern char *git_work_tree_cfg;\n int is_inside_work_tree(void);\n const char *get_git_dir(void);\n const char *get_git_common_dir(void);\n-char *get_object_directory(void);\n+const char *get_object_directory(void);\n char *get_index_file(void);\n char *get_graft_file(struct repository *r);\n void set_git_dir(const char *path, int make_realpath);\ndiff --git a/environment.c b/environment.c\nindex 5bff1b386fd..b3296ce7d15 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -273,7 +273,7 @@ const char *get_git_work_tree(void)\n \treturn the_repository->worktree;\n }\n \n-char *get_object_directory(void)\n+const char *get_object_directory(void)\n {\n \tif (!the_repository->objects->odb)\n \t\tBUG(\"git environment hasn't been setup\");\n-- \ngitgitgadget\n"},{"id":"454404","messageId":"xmqqy1ztuh0h.fsf@gitster.g","threadId":"57774","inReplyTo":"fd580e7947743db545a839e81f027984e70b7717.1650911234.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/3] multi-pack-index: use --object-dir real path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-25T18:58:38Z","receivedAt":"2022-04-25T18:58:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n>\n> The --object-dir argument to 'git multi-pack-index' allows a user to\n> specify an alternate to use instead of the local $GITDIR. This is used\n> by third-party tools like VFS for Git to maintain the pack-files in a\n> \"shared object cache\" used by multiple clones.\n>\n> On Windows, the user can specify a path using a Windows-style file path\n> with backslashes such as \"C:\\Path\\To\\ObjectDir\". This same path style is\n> used in the .git/objects/info/alternates file, so it already matches the\n> path of that alternate. However, find_odb() converts these paths to\n> real-paths for the comparison, which use forward slashes. As of the\n> previous change, lookup_multi_pack_index() uses real-paths, so it\n> correctly finds the target multi-pack-index when given these paths.\n>\n> Some commands such as 'git multi-pack-index repack' call child processes\n> using the object_dir value, so it can be helpful to convert the path to\n> the real-path before sending it to those locations.\n>\n> Add a callback to convert the real path immediately upon parsing the\n> argument. We need to be careful that we don't store the exact value out\n> of get_object_directory() and free it, or we could corrupt a later use\n> of the_repository->objects->odb->path.\n>\n> We don't use get_object_directory() for the initial instantiation in\n> cmd_multi_pack_index() because we need 'git multi-pack-index -h' to work\n> without a Git repository.\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> builtin/multi-pack-index.c | 45 ++++++++++++++++++++++++++++----------\n> 1 file changed, 34 insertions(+), 11 deletions(-)\n\nMuch nicer compared to the previous round, by doing the\nnormalization early.\n\nWill queue. thanks.\n"}]}