{"thread":{"id":"45450","subject":"[PATCH 0/6] prefix_filename cleanups","startedAt":"2017-03-21T01:18:59Z","lastAt":"2017-03-21T23:32:22Z","messageCount":11,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"314758","messageId":"20170321011838.rdhnbfwbigm4s4e3@sigill.intra.peff.net","threadId":"45450","inReplyTo":null,"subject":"[PATCH 0/6] prefix_filename cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-21T01:18:38Z","receivedAt":"2017-03-21T01:18:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I noticed a spot in builtin/bundle.c that would benefit from using\nprefix_filename(). But when I tried to use it, I noticed its interface\nwas a little error-prone (because it returns a static buffer). And\nindeed, a little digging found a bug in hash-object related to this.\n\nSo here's the fix for the hash-object bug, some cleanups to make such\nbugs less likely, and then finally the bundle conversion. The bundle\nthing does fix some minor bugs. It _could_ come before the cleanups if\nwe wanted to float the fixes to the top, but the function is much more\npleasant to call after the cleanups. :)\n\n  [1/6]: hash-object: fix buffer reuse with --path in a subdirectory\n  [2/6]: prefix_filename: move docstring to header file\n  [3/6]: prefix_filename: drop length parameter\n  [4/6]: prefix_filename: return newly allocated string\n  [5/6]: prefix_filename: simplify windows #ifdef\n  [6/6]: bundle: use prefix_filename with bundle path\n\n abspath.c              | 30 +++++++++++-------------------\n apply.c                | 11 ++++++-----\n builtin/bundle.c       |  8 +-------\n builtin/config.c       |  4 +---\n builtin/hash-object.c  | 10 +++++-----\n builtin/log.c          |  3 +--\n builtin/mailinfo.c     | 11 ++---------\n builtin/merge-file.c   | 18 +++++++++++-------\n builtin/rev-parse.c    |  6 +++---\n builtin/worktree.c     |  5 +++--\n cache.h                | 14 +++++++++++++-\n diff-no-index.c        |  7 +++----\n diff.c                 |  6 +++---\n parse-options.c        |  2 +-\n setup.c                | 11 ++++++++---\n t/t1007-hash-object.sh | 10 ++++++++++\n worktree.c             |  5 ++++-\n 17 files changed, 86 insertions(+), 75 deletions(-)\n\n-Peff\n"},{"id":"314759","messageId":"20170321012042.jb5biz2min7a34gu@sigill.intra.peff.net","threadId":"45450","inReplyTo":"20170321011838.rdhnbfwbigm4s4e3@sigill.intra.peff.net","subject":"[PATCH 1/6] hash-object: fix buffer reuse with --path in a subdirectory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-21T01:20:42Z","receivedAt":"2017-03-21T01:22:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The hash-object command uses prefix_filename() without\nduplicating its return value. Since that function returns a\nstatic buffer, the value is overwritten by subsequent calls.\n\nThis can cause incorrect results when we use --path along\nwith hashing a file by its relative path, both of which need\nto call prefix_filename(). We overwrite the filename\ncomputed for --path, effectively ignoring it.\n\nWe can fix this by calling xstrdup on the return value. Note\nthat we don't bother freeing the \"vpath\" instance, as it\nremains valid until the program exit.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/hash-object.c  |  7 +++++--\n t/t1007-hash-object.sh | 10 ++++++++++\n 2 files changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/hash-object.c b/builtin/hash-object.c\nindex 9028e1fdc..56df77b0c 100644\n--- a/builtin/hash-object.c\n+++ b/builtin/hash-object.c\n@@ -115,7 +115,7 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)\n \n \tprefix_length = prefix ? strlen(prefix) : 0;\n \tif (vpath && prefix)\n-\t\tvpath = prefix_filename(prefix, prefix_length, vpath);\n+\t\tvpath = xstrdup(prefix_filename(prefix, prefix_length, vpath));\n \n \tgit_config(git_default_config, NULL);\n \n@@ -144,11 +144,14 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)\n \n \tfor (i = 0 ; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n+\t\tchar *to_free = NULL;\n \n \t\tif (0 <= prefix_length)\n-\t\t\targ = prefix_filename(prefix, prefix_length, arg);\n+\t\t\targ = to_free =\n+\t\t\t\txstrdup(prefix_filename(prefix, prefix_length, arg));\n \t\thash_object(arg, type, no_filters ? NULL : vpath ? vpath : arg,\n \t\t\t    flags, literally);\n+\t\tfree(to_free);\n \t}\n \n \tif (stdin_paths)\ndiff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh\nindex c5245c5cb..532682f51 100755\n--- a/t/t1007-hash-object.sh\n+++ b/t/t1007-hash-object.sh\n@@ -134,6 +134,16 @@ test_expect_success 'gitattributes also work in a subdirectory' '\n \t)\n '\n \n+test_expect_success '--path works in a subdirectory' '\n+\t(\n+\t\tcd subdir &&\n+\t\tpath1_sha=$(git hash-object --path=../file1 ../file0) &&\n+\t\tpath0_sha=$(git hash-object --path=../file0 ../file1) &&\n+\t\ttest \"$file0_sha\" = \"$path0_sha\" &&\n+\t\ttest \"$file1_sha\" = \"$path1_sha\"\n+\t)\n+'\n+\n test_expect_success 'check that --no-filters option works' '\n \tnofilters_file1=$(git hash-object --no-filters file1) &&\n \ttest \"$file0_sha\" = \"$nofilters_file1\" &&\n-- \n2.12.1.683.gcd02edfec\n\n"},{"id":"314760","messageId":"20170321012127.qp4lp4zcc3icprzw@sigill.intra.peff.net","threadId":"45450","inReplyTo":"20170321011838.rdhnbfwbigm4s4e3@sigill.intra.peff.net","subject":"[PATCH 2/6] prefix_filename: move docstring to header file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-21T01:21:27Z","receivedAt":"2017-03-21T01:22:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is a public function, so we should make its\ndocumentation available near the declaration.\n\nWhile we're at it, we can give a few details about how it\nworks.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n abspath.c |  5 -----\n cache.h   | 12 ++++++++++++\n 2 files changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/abspath.c b/abspath.c\nindex b02e068aa..fd30aff08 100644\n--- a/abspath.c\n+++ b/abspath.c\n@@ -246,11 +246,6 @@ char *absolute_pathdup(const char *path)\n \treturn strbuf_detach(&sb, NULL);\n }\n \n-/*\n- * Unlike prefix_path, this should be used if the named file does\n- * not have to interact with index entry; i.e. name of a random file\n- * on the filesystem.\n- */\n const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)\n {\n \tstatic struct strbuf path = STRBUF_INIT;\ndiff --git a/cache.h b/cache.h\nindex 9b2157f59..a01668fc4 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -529,7 +529,19 @@ extern const char *setup_git_directory_gently(int *);\n extern const char *setup_git_directory(void);\n extern char *prefix_path(const char *prefix, int len, const char *path);\n extern char *prefix_path_gently(const char *prefix, int len, int *remaining, const char *path);\n+\n+/*\n+ * Concatenate \"prefix\" (if len is non-zero) and \"path\", with no\n+ * connecting characters (so \"prefix\" should end with a \"/\").\n+ * Unlike prefix_path, this should be used if the named file does\n+ * not have to interact with index entry; i.e. name of a random file\n+ * on the filesystem.\n+ *\n+ * The return value may point to static storage which will be overwritten by\n+ * further calls.\n+ */\n extern const char *prefix_filename(const char *prefix, int len, const char *path);\n+\n extern int check_filename(const char *prefix, const char *name);\n extern void verify_filename(const char *prefix,\n \t\t\t    const char *name,\n-- \n2.12.1.683.gcd02edfec\n\n"},{"id":"314761","messageId":"20170321012228.ondlwe4hoaf3yks4@sigill.intra.peff.net","threadId":"45450","inReplyTo":"20170321011838.rdhnbfwbigm4s4e3@sigill.intra.peff.net","subject":"[PATCH 3/6] prefix_filename: drop length parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-21T01:22:28Z","receivedAt":"2017-03-21T01:22:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This function takes the prefix as a ptr/len pair, but in\nevery caller the length is exactly strlen(ptr). Let's\nsimplify the interface and just take the string. This saves\ncallers specifying it (and in some cases handling a NULL\nprefix).\n\nIn a handful of cases we had the length already without\ncalling strlen, so this is technically slower. But it's not\nlikely to matter (after all, if the prefix is non-empty\nwe'll allocate and copy it into a buffer anyway).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n abspath.c             | 4 +++-\n apply.c               | 6 ++----\n builtin/config.c      | 1 -\n builtin/hash-object.c | 9 +++------\n builtin/log.c         | 3 +--\n builtin/mailinfo.c    | 2 +-\n builtin/merge-file.c  | 8 ++------\n builtin/rev-parse.c   | 4 +---\n builtin/worktree.c    | 2 +-\n cache.h               | 2 +-\n diff-no-index.c       | 7 +++----\n diff.c                | 4 ++--\n parse-options.c       | 2 +-\n setup.c               | 2 +-\n worktree.c            | 2 +-\n 15 files changed, 23 insertions(+), 35 deletions(-)\n\ndiff --git a/abspath.c b/abspath.c\nindex fd30aff08..c6f480993 100644\n--- a/abspath.c\n+++ b/abspath.c\n@@ -246,9 +246,11 @@ char *absolute_pathdup(const char *path)\n \treturn strbuf_detach(&sb, NULL);\n }\n \n-const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)\n+const char *prefix_filename(const char *pfx, const char *arg)\n {\n \tstatic struct strbuf path = STRBUF_INIT;\n+\tsize_t pfx_len = pfx ? strlen(pfx) : 0;\n+\n #ifndef GIT_WINDOWS_NATIVE\n \tif (!pfx_len || is_absolute_path(arg))\n \t\treturn arg;\ndiff --git a/apply.c b/apply.c\nindex 0e2caeab9..b8bd5a4be 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -2046,7 +2046,7 @@ static void prefix_one(struct apply_state *state, char **name)\n \tchar *old_name = *name;\n \tif (!old_name)\n \t\treturn;\n-\t*name = xstrdup(prefix_filename(state->prefix, state->prefix_length, *name));\n+\t*name = xstrdup(prefix_filename(state->prefix, *name));\n \tfree(old_name);\n }\n \n@@ -4815,9 +4815,7 @@ int apply_all_patches(struct apply_state *state,\n \t\t\tread_stdin = 0;\n \t\t\tcontinue;\n \t\t} else if (0 < state->prefix_length)\n-\t\t\targ = prefix_filename(state->prefix,\n-\t\t\t\t\t      state->prefix_length,\n-\t\t\t\t\t      arg);\n+\t\t\targ = prefix_filename(state->prefix, arg);\n \n \t\tfd = open(arg, O_RDONLY);\n \t\tif (fd < 0) {\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 05843a0f9..74f6c34d1 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -528,7 +528,6 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tif (!is_absolute_path(given_config_source.file) && prefix)\n \t\t\tgiven_config_source.file =\n \t\t\t\txstrdup(prefix_filename(prefix,\n-\t\t\t\t\t\t\tstrlen(prefix),\n \t\t\t\t\t\t\tgiven_config_source.file));\n \t}\n \ndiff --git a/builtin/hash-object.c b/builtin/hash-object.c\nindex 56df77b0c..2ea36909d 100644\n--- a/builtin/hash-object.c\n+++ b/builtin/hash-object.c\n@@ -102,7 +102,6 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \tint i;\n-\tint prefix_length = -1;\n \tconst char *errstr = NULL;\n \n \targc = parse_options(argc, argv, NULL, hash_object_options,\n@@ -113,9 +112,8 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)\n \telse\n \t\tprefix = setup_git_directory_gently(&nongit);\n \n-\tprefix_length = prefix ? strlen(prefix) : 0;\n \tif (vpath && prefix)\n-\t\tvpath = xstrdup(prefix_filename(prefix, prefix_length, vpath));\n+\t\tvpath = xstrdup(prefix_filename(prefix, vpath));\n \n \tgit_config(git_default_config, NULL);\n \n@@ -146,9 +144,8 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)\n \t\tconst char *arg = argv[i];\n \t\tchar *to_free = NULL;\n \n-\t\tif (0 <= prefix_length)\n-\t\t\targ = to_free =\n-\t\t\t\txstrdup(prefix_filename(prefix, prefix_length, arg));\n+\t\tif (prefix)\n+\t\t\targ = to_free = xstrdup(prefix_filename(prefix, arg));\n \t\thash_object(arg, type, no_filters ? NULL : vpath ? vpath : arg,\n \t\t\t    flags, literally);\n \t\tfree(to_free);\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 281af8c1e..bfdc7a23d 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1084,8 +1084,7 @@ static const char *set_outdir(const char *prefix, const char *output_directory)\n \tif (!output_directory)\n \t\treturn prefix;\n \n-\treturn xstrdup(prefix_filename(prefix, outdir_offset,\n-\t\t\t\t       output_directory));\n+\treturn xstrdup(prefix_filename(prefix, output_directory));\n }\n \n static const char * const builtin_format_patch_usage[] = {\ndiff --git a/builtin/mailinfo.c b/builtin/mailinfo.c\nindex e3b62f2fc..681f07f54 100644\n--- a/builtin/mailinfo.c\n+++ b/builtin/mailinfo.c\n@@ -15,7 +15,7 @@ static char *prefix_copy(const char *prefix, const char *filename)\n {\n \tif (!prefix || is_absolute_path(filename))\n \t\treturn xstrdup(filename);\n-\treturn xstrdup(prefix_filename(prefix, strlen(prefix), filename));\n+\treturn xstrdup(prefix_filename(prefix, filename));\n }\n \n int cmd_mailinfo(int argc, const char **argv, const char *prefix)\ndiff --git a/builtin/merge-file.c b/builtin/merge-file.c\nindex 13e22a2f0..63cd94358 100644\n--- a/builtin/merge-file.c\n+++ b/builtin/merge-file.c\n@@ -28,7 +28,6 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)\n \txmparam_t xmp = {{0}};\n \tint ret = 0, i = 0, to_stdout = 0;\n \tint quiet = 0;\n-\tint prefixlen = 0;\n \tstruct option options[] = {\n \t\tOPT_BOOL('p', \"stdout\", &to_stdout, N_(\"send results to standard output\")),\n \t\tOPT_SET_INT(0, \"diff3\", &xmp.style, N_(\"use a diff3 based merge\"), XDL_MERGE_DIFF3),\n@@ -65,11 +64,8 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)\n \t\t\treturn error_errno(\"failed to redirect stderr to /dev/null\");\n \t}\n \n-\tif (prefix)\n-\t\tprefixlen = strlen(prefix);\n-\n \tfor (i = 0; i < 3; i++) {\n-\t\tconst char *fname = prefix_filename(prefix, prefixlen, argv[i]);\n+\t\tconst char *fname = prefix_filename(prefix, argv[i]);\n \t\tif (!names[i])\n \t\t\tnames[i] = argv[i];\n \t\tif (read_mmfile(mmfs + i, fname))\n@@ -90,7 +86,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)\n \n \tif (ret >= 0) {\n \t\tconst char *filename = argv[0];\n-\t\tconst char *fpath = prefix_filename(prefix, prefixlen, argv[0]);\n+\t\tconst char *fpath = prefix_filename(prefix, argv[0]);\n \t\tFILE *f = to_stdout ? stdout : fopen(fpath, \"wb\");\n \n \t\tif (!f)\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 254964326..c8035331e 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -228,9 +228,7 @@ static int show_file(const char *arg, int output_prefix)\n \tif ((filter & (DO_NONFLAGS|DO_NOREV)) == (DO_NONFLAGS|DO_NOREV)) {\n \t\tif (output_prefix) {\n \t\t\tconst char *prefix = startup_info->prefix;\n-\t\t\tshow(prefix_filename(prefix,\n-\t\t\t\t\t     prefix ? strlen(prefix) : 0,\n-\t\t\t\t\t     arg));\n+\t\t\tshow(prefix_filename(prefix, arg));\n \t\t} else\n \t\t\tshow(arg);\n \t\treturn 1;\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 831fe058a..e38325e44 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -338,7 +338,7 @@ static int add(int ac, const char **av, const char *prefix)\n \tif (ac < 1 || ac > 2)\n \t\tusage_with_options(worktree_usage, options);\n \n-\tpath = prefix_filename(prefix, strlen(prefix), av[0]);\n+\tpath = prefix_filename(prefix, av[0]);\n \tbranch = ac < 2 ? \"HEAD\" : av[1];\n \n \tif (!strcmp(branch, \"-\"))\ndiff --git a/cache.h b/cache.h\nindex a01668fc4..0b53aef0e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -540,7 +540,7 @@ extern char *prefix_path_gently(const char *prefix, int len, int *remaining, con\n  * The return value may point to static storage which will be overwritten by\n  * further calls.\n  */\n-extern const char *prefix_filename(const char *prefix, int len, const char *path);\n+extern const char *prefix_filename(const char *prefix, const char *path);\n \n extern int check_filename(const char *prefix, const char *name);\n extern void verify_filename(const char *prefix,\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex df762fd0f..5f7317ced 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -236,7 +236,7 @@ static void fixup_paths(const char **path, struct strbuf *replacement)\n void diff_no_index(struct rev_info *revs,\n \t\t   int argc, const char **argv)\n {\n-\tint i, prefixlen;\n+\tint i;\n \tconst char *paths[2];\n \tstruct strbuf replacement = STRBUF_INIT;\n \tconst char *prefix = revs->prefix;\n@@ -257,7 +257,6 @@ void diff_no_index(struct rev_info *revs,\n \t\t}\n \t}\n \n-\tprefixlen = prefix ? strlen(prefix) : 0;\n \tfor (i = 0; i < 2; i++) {\n \t\tconst char *p = argv[argc - 2 + i];\n \t\tif (!strcmp(p, \"-\"))\n@@ -266,8 +265,8 @@ void diff_no_index(struct rev_info *revs,\n \t\t\t * path that is \"-\", spell it as \"./-\".\n \t\t\t */\n \t\t\tp = file_from_standard_input;\n-\t\telse if (prefixlen)\n-\t\t\tp = xstrdup(prefix_filename(prefix, prefixlen, p));\n+\t\telse if (prefix)\n+\t\t\tp = xstrdup(prefix_filename(prefix, p));\n \t\tpaths[i] = p;\n \t}\n \ndiff --git a/diff.c b/diff.c\nindex a628ac3a9..70870b4b6 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4023,7 +4023,7 @@ int diff_opt_parse(struct diff_options *options,\n \telse if (!strcmp(arg, \"--pickaxe-regex\"))\n \t\toptions->pickaxe_opts |= DIFF_PICKAXE_REGEX;\n \telse if ((argcount = short_opt('O', av, &optarg))) {\n-\t\tconst char *path = prefix_filename(prefix, strlen(prefix), optarg);\n+\t\tconst char *path = prefix_filename(prefix, optarg);\n \t\toptions->orderfile = xstrdup(path);\n \t\treturn argcount;\n \t}\n@@ -4071,7 +4071,7 @@ int diff_opt_parse(struct diff_options *options,\n \telse if (!strcmp(arg, \"--no-function-context\"))\n \t\tDIFF_OPT_CLR(options, FUNCCONTEXT);\n \telse if ((argcount = parse_long_opt(\"output\", av, &optarg))) {\n-\t\tconst char *path = prefix_filename(prefix, strlen(prefix), optarg);\n+\t\tconst char *path = prefix_filename(prefix, optarg);\n \t\toptions->file = fopen(path, \"w\");\n \t\tif (!options->file)\n \t\t\tdie_errno(\"Could not open '%s'\", path);\ndiff --git a/parse-options.c b/parse-options.c\nindex 4fbe924a5..ba6cc30b2 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -40,7 +40,7 @@ static void fix_filename(const char *prefix, const char **file)\n \tif (!file || !*file || !prefix || is_absolute_path(*file)\n \t    || !strcmp(\"-\", *file))\n \t\treturn;\n-\t*file = xstrdup(prefix_filename(prefix, strlen(prefix), *file));\n+\t*file = xstrdup(prefix_filename(prefix, *file));\n }\n \n static int opt_command_mode_error(const struct option *opt,\ndiff --git a/setup.c b/setup.c\nindex 64f922a93..a76379e0c 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -142,7 +142,7 @@ int check_filename(const char *prefix, const char *arg)\n \t\t\treturn 1;\n \t\tname = arg + 2;\n \t} else if (prefix)\n-\t\tname = prefix_filename(prefix, strlen(prefix), arg);\n+\t\tname = prefix_filename(prefix, arg);\n \telse\n \t\tname = arg;\n \tif (!lstat(name, &st))\ndiff --git a/worktree.c b/worktree.c\nindex fa7bc67a5..42dd3d52b 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -254,7 +254,7 @@ struct worktree *find_worktree(struct worktree **list,\n \tif ((wt = find_worktree_by_suffix(list, arg)))\n \t\treturn wt;\n \n-\targ = prefix_filename(prefix, strlen(prefix), arg);\n+\targ = prefix_filename(prefix, arg);\n \tpath = real_pathdup(arg, 1);\n \tfor (; *list; list++)\n \t\tif (!fspathcmp(path, real_path((*list)->path)))\n-- \n2.12.1.683.gcd02edfec\n\n"},{"id":"314763","messageId":"20170321012847.yebhpdmk5zrizgmj@sigill.intra.peff.net","threadId":"45450","inReplyTo":"20170321011838.rdhnbfwbigm4s4e3@sigill.intra.peff.net","subject":"[PATCH 4/6] prefix_filename: return newly allocated string","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-21T01:28:49Z","receivedAt":"2017-03-21T01:29:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The prefix_filename() function returns a pointer to static\nstorage, which makes it easy to use dangerously. We already\nfixed one buggy caller in hash-object recently, and the\ncalls in apply.c are suspicious (I didn't dig in enough to\nconfirm that there is a bug, but we call the function once\nin apply_all_patches() and then again indirectly from\nparse_chunk()).\n\nLet's make it harder to get wrong by allocating the return\nvalue. For simplicity, we'll do this even when the prefix is\nempty (and we could just return the original file pointer).\nThat will cause us to allocate sometimes when we wouldn't\notherwise need to, but this function isn't called in\nperformance critical code-paths (and it already _might_\nallocate on any given call, so a caller that cares about\nperformance is questionable anyway).\n\nThe downside is that the callers need to remember to free()\nthe result to avoid leaking. Most of them already used\nxstrdup() on the result, so we know they are OK. The\nremainder have been converted to use free() as appropriate.\n\nI considered retaining a prefix_filename_unsafe() for cases\nwhere we know the static lifetime is OK (and handling the\ncleanup is awkward). This is only a handful of cases,\nthough, and it's not worth the mental energy in worrying\nabout whether the \"unsafe\" variant is OK to use in any\nsituation.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n abspath.c             | 10 ++++------\n apply.c               |  9 ++++++---\n builtin/config.c      |  3 +--\n builtin/hash-object.c |  2 +-\n builtin/log.c         |  2 +-\n builtin/mailinfo.c    | 11 ++---------\n builtin/merge-file.c  | 14 +++++++++++---\n builtin/rev-parse.c   |  4 +++-\n builtin/worktree.c    |  3 ++-\n cache.h               |  6 +++---\n diff-no-index.c       |  2 +-\n diff.c                |  6 +++---\n parse-options.c       |  2 +-\n setup.c               | 11 ++++++++---\n worktree.c            |  5 ++++-\n 15 files changed, 51 insertions(+), 39 deletions(-)\n\ndiff --git a/abspath.c b/abspath.c\nindex c6f480993..4addd1fde 100644\n--- a/abspath.c\n+++ b/abspath.c\n@@ -246,20 +246,18 @@ char *absolute_pathdup(const char *path)\n \treturn strbuf_detach(&sb, NULL);\n }\n \n-const char *prefix_filename(const char *pfx, const char *arg)\n+char *prefix_filename(const char *pfx, const char *arg)\n {\n-\tstatic struct strbuf path = STRBUF_INIT;\n+\tstruct strbuf path = STRBUF_INIT;\n \tsize_t pfx_len = pfx ? strlen(pfx) : 0;\n \n #ifndef GIT_WINDOWS_NATIVE\n \tif (!pfx_len || is_absolute_path(arg))\n-\t\treturn arg;\n-\tstrbuf_reset(&path);\n+\t\treturn xstrdup(arg);\n \tstrbuf_add(&path, pfx, pfx_len);\n \tstrbuf_addstr(&path, arg);\n #else\n \t/* don't add prefix to absolute paths, but still replace '\\' by '/' */\n-\tstrbuf_reset(&path);\n \tif (is_absolute_path(arg))\n \t\tpfx_len = 0;\n \telse if (pfx_len)\n@@ -267,5 +265,5 @@ const char *prefix_filename(const char *pfx, const char *arg)\n \tstrbuf_addstr(&path, arg);\n \tconvert_slashes(path.buf + pfx_len);\n #endif\n-\treturn path.buf;\n+\treturn strbuf_detach(&path, NULL);\n }\ndiff --git a/apply.c b/apply.c\nindex b8bd5a4be..e6dbab26a 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -2046,7 +2046,7 @@ static void prefix_one(struct apply_state *state, char **name)\n \tchar *old_name = *name;\n \tif (!old_name)\n \t\treturn;\n-\t*name = xstrdup(prefix_filename(state->prefix, *name));\n+\t*name = prefix_filename(state->prefix, *name);\n \tfree(old_name);\n }\n \n@@ -4805,6 +4805,7 @@ int apply_all_patches(struct apply_state *state,\n \n \tfor (i = 0; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n+\t\tchar *to_free = NULL;\n \t\tint fd;\n \n \t\tif (!strcmp(arg, \"-\")) {\n@@ -4814,19 +4815,21 @@ int apply_all_patches(struct apply_state *state,\n \t\t\terrs |= res;\n \t\t\tread_stdin = 0;\n \t\t\tcontinue;\n-\t\t} else if (0 < state->prefix_length)\n-\t\t\targ = prefix_filename(state->prefix, arg);\n+\t\t} else\n+\t\t\targ = to_free = prefix_filename(state->prefix, arg);\n \n \t\tfd = open(arg, O_RDONLY);\n \t\tif (fd < 0) {\n \t\t\terror(_(\"can't open patch '%s': %s\"), arg, strerror(errno));\n \t\t\tres = -128;\n+\t\t\tfree(to_free);\n \t\t\tgoto end;\n \t\t}\n \t\tread_stdin = 0;\n \t\tset_default_whitespace_mode(state);\n \t\tres = apply_patch(state, fd, arg, options);\n \t\tclose(fd);\n+\t\tfree(to_free);\n \t\tif (res < 0)\n \t\t\tgoto end;\n \t\terrs |= res;\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 74f6c34d1..4f49a0edb 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -527,8 +527,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \telse if (given_config_source.file) {\n \t\tif (!is_absolute_path(given_config_source.file) && prefix)\n \t\t\tgiven_config_source.file =\n-\t\t\t\txstrdup(prefix_filename(prefix,\n-\t\t\t\t\t\t\tgiven_config_source.file));\n+\t\t\t\tprefix_filename(prefix, given_config_source.file);\n \t}\n \n \tif (respect_includes == -1)\ndiff --git a/builtin/hash-object.c b/builtin/hash-object.c\nindex 2ea36909d..bbeaf20bc 100644\n--- a/builtin/hash-object.c\n+++ b/builtin/hash-object.c\n@@ -145,7 +145,7 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)\n \t\tchar *to_free = NULL;\n \n \t\tif (prefix)\n-\t\t\targ = to_free = xstrdup(prefix_filename(prefix, arg));\n+\t\t\targ = to_free = prefix_filename(prefix, arg);\n \t\thash_object(arg, type, no_filters ? NULL : vpath ? vpath : arg,\n \t\t\t    flags, literally);\n \t\tfree(to_free);\ndiff --git a/builtin/log.c b/builtin/log.c\nindex bfdc7a23d..670229cbb 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1084,7 +1084,7 @@ static const char *set_outdir(const char *prefix, const char *output_directory)\n \tif (!output_directory)\n \t\treturn prefix;\n \n-\treturn xstrdup(prefix_filename(prefix, output_directory));\n+\treturn prefix_filename(prefix, output_directory);\n }\n \n static const char * const builtin_format_patch_usage[] = {\ndiff --git a/builtin/mailinfo.c b/builtin/mailinfo.c\nindex 681f07f54..cfb667a59 100644\n--- a/builtin/mailinfo.c\n+++ b/builtin/mailinfo.c\n@@ -11,13 +11,6 @@\n static const char mailinfo_usage[] =\n \t\"git mailinfo [-k | -b] [-m | --message-id] [-u | --encoding=<encoding> | -n] [--scissors | --no-scissors] <msg> <patch> < mail >info\";\n \n-static char *prefix_copy(const char *prefix, const char *filename)\n-{\n-\tif (!prefix || is_absolute_path(filename))\n-\t\treturn xstrdup(filename);\n-\treturn xstrdup(prefix_filename(prefix, filename));\n-}\n-\n int cmd_mailinfo(int argc, const char **argv, const char *prefix)\n {\n \tconst char *def_charset;\n@@ -60,8 +53,8 @@ int cmd_mailinfo(int argc, const char **argv, const char *prefix)\n \tmi.input = stdin;\n \tmi.output = stdout;\n \n-\tmsgfile = prefix_copy(prefix, argv[1]);\n-\tpatchfile = prefix_copy(prefix, argv[2]);\n+\tmsgfile = prefix_filename(prefix, argv[1]);\n+\tpatchfile = prefix_filename(prefix, argv[2]);\n \n \tstatus = !!mailinfo(&mi, msgfile, patchfile);\n \tclear_mailinfo(&mi);\ndiff --git a/builtin/merge-file.c b/builtin/merge-file.c\nindex 63cd94358..47dde7c39 100644\n--- a/builtin/merge-file.c\n+++ b/builtin/merge-file.c\n@@ -65,11 +65,18 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)\n \t}\n \n \tfor (i = 0; i < 3; i++) {\n-\t\tconst char *fname = prefix_filename(prefix, argv[i]);\n+\t\tchar *fname;\n+\t\tint ret;\n+\n \t\tif (!names[i])\n \t\t\tnames[i] = argv[i];\n-\t\tif (read_mmfile(mmfs + i, fname))\n+\n+\t\tfname = prefix_filename(prefix, argv[i]);\n+\t\tret = read_mmfile(mmfs + i, fname);\n+\t\tfree(fname);\n+\t\tif (ret)\n \t\t\treturn -1;\n+\n \t\tif (mmfs[i].size > MAX_XDIFF_SIZE ||\n \t\t    buffer_is_binary(mmfs[i].ptr, mmfs[i].size))\n \t\t\treturn error(\"Cannot merge binary files: %s\",\n@@ -86,7 +93,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)\n \n \tif (ret >= 0) {\n \t\tconst char *filename = argv[0];\n-\t\tconst char *fpath = prefix_filename(prefix, argv[0]);\n+\t\tchar *fpath = prefix_filename(prefix, argv[0]);\n \t\tFILE *f = to_stdout ? stdout : fopen(fpath, \"wb\");\n \n \t\tif (!f)\n@@ -98,6 +105,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)\n \t\telse if (fclose(f))\n \t\t\tret = error_errno(\"Could not close %s\", filename);\n \t\tfree(result.ptr);\n+\t\tfree(fpath);\n \t}\n \n \tif (ret > 127)\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex c8035331e..7cd01c281 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -228,7 +228,9 @@ static int show_file(const char *arg, int output_prefix)\n \tif ((filter & (DO_NONFLAGS|DO_NOREV)) == (DO_NONFLAGS|DO_NOREV)) {\n \t\tif (output_prefix) {\n \t\t\tconst char *prefix = startup_info->prefix;\n-\t\t\tshow(prefix_filename(prefix, arg));\n+\t\t\tchar *fname = prefix_filename(prefix, arg);\n+\t\t\tshow(fname);\n+\t\t\tfree(fname);\n \t\t} else\n \t\t\tshow(arg);\n \t\treturn 1;\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex e38325e44..9993ded41 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -318,7 +318,8 @@ static int add(int ac, const char **av, const char *prefix)\n {\n \tstruct add_opts opts;\n \tconst char *new_branch_force = NULL;\n-\tconst char *path, *branch;\n+\tchar *path;\n+\tconst char *branch;\n \tstruct option options[] = {\n \t\tOPT__FORCE(&opts.force, N_(\"checkout <branch> even if already checked out in other worktree\")),\n \t\tOPT_STRING('b', NULL, &opts.new_branch, N_(\"branch\"),\ndiff --git a/cache.h b/cache.h\nindex 0b53aef0e..aa6a0fb91 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -537,10 +537,10 @@ extern char *prefix_path_gently(const char *prefix, int len, int *remaining, con\n  * not have to interact with index entry; i.e. name of a random file\n  * on the filesystem.\n  *\n- * The return value may point to static storage which will be overwritten by\n- * further calls.\n+ * The return value is always a newly allocated string (even if the\n+ * prefix was empty).\n  */\n-extern const char *prefix_filename(const char *prefix, const char *path);\n+extern char *prefix_filename(const char *prefix, const char *path);\n \n extern int check_filename(const char *prefix, const char *name);\n extern void verify_filename(const char *prefix,\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 5f7317ced..79229382b 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -266,7 +266,7 @@ void diff_no_index(struct rev_info *revs,\n \t\t\t */\n \t\t\tp = file_from_standard_input;\n \t\telse if (prefix)\n-\t\t\tp = xstrdup(prefix_filename(prefix, p));\n+\t\t\tp = prefix_filename(prefix, p);\n \t\tpaths[i] = p;\n \t}\n \ndiff --git a/diff.c b/diff.c\nindex 70870b4b6..58cb72d7e 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4023,8 +4023,7 @@ int diff_opt_parse(struct diff_options *options,\n \telse if (!strcmp(arg, \"--pickaxe-regex\"))\n \t\toptions->pickaxe_opts |= DIFF_PICKAXE_REGEX;\n \telse if ((argcount = short_opt('O', av, &optarg))) {\n-\t\tconst char *path = prefix_filename(prefix, optarg);\n-\t\toptions->orderfile = xstrdup(path);\n+\t\toptions->orderfile = prefix_filename(prefix, optarg);\n \t\treturn argcount;\n \t}\n \telse if ((argcount = parse_long_opt(\"diff-filter\", av, &optarg))) {\n@@ -4071,13 +4070,14 @@ int diff_opt_parse(struct diff_options *options,\n \telse if (!strcmp(arg, \"--no-function-context\"))\n \t\tDIFF_OPT_CLR(options, FUNCCONTEXT);\n \telse if ((argcount = parse_long_opt(\"output\", av, &optarg))) {\n-\t\tconst char *path = prefix_filename(prefix, optarg);\n+\t\tchar *path = prefix_filename(prefix, optarg);\n \t\toptions->file = fopen(path, \"w\");\n \t\tif (!options->file)\n \t\t\tdie_errno(\"Could not open '%s'\", path);\n \t\toptions->close_file = 1;\n \t\tif (options->use_color != GIT_COLOR_ALWAYS)\n \t\t\toptions->use_color = GIT_COLOR_NEVER;\n+\t\tfree(path);\n \t\treturn argcount;\n \t} else\n \t\treturn 0;\ndiff --git a/parse-options.c b/parse-options.c\nindex ba6cc30b2..a23a1e67f 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -40,7 +40,7 @@ static void fix_filename(const char *prefix, const char **file)\n \tif (!file || !*file || !prefix || is_absolute_path(*file)\n \t    || !strcmp(\"-\", *file))\n \t\treturn;\n-\t*file = xstrdup(prefix_filename(prefix, *file));\n+\t*file = prefix_filename(prefix, *file);\n }\n \n static int opt_command_mode_error(const struct option *opt,\ndiff --git a/setup.c b/setup.c\nindex a76379e0c..5c7946d2b 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -135,6 +135,7 @@ int path_inside_repo(const char *prefix, const char *path)\n int check_filename(const char *prefix, const char *arg)\n {\n \tconst char *name;\n+\tchar *to_free = NULL;\n \tstruct stat st;\n \n \tif (starts_with(arg, \":/\")) {\n@@ -142,13 +143,17 @@ int check_filename(const char *prefix, const char *arg)\n \t\t\treturn 1;\n \t\tname = arg + 2;\n \t} else if (prefix)\n-\t\tname = prefix_filename(prefix, arg);\n+\t\tname = to_free = prefix_filename(prefix, arg);\n \telse\n \t\tname = arg;\n-\tif (!lstat(name, &st))\n+\tif (!lstat(name, &st)) {\n+\t\tfree(to_free);\n \t\treturn 1; /* file exists */\n-\tif (errno == ENOENT || errno == ENOTDIR)\n+\t}\n+\tif (errno == ENOENT || errno == ENOTDIR) {\n+\t\tfree(to_free);\n \t\treturn 0; /* file does not exist */\n+\t}\n \tdie_errno(\"failed to stat '%s'\", arg);\n }\n \ndiff --git a/worktree.c b/worktree.c\nindex 42dd3d52b..2520fc65c 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -250,16 +250,19 @@ struct worktree *find_worktree(struct worktree **list,\n {\n \tstruct worktree *wt;\n \tchar *path;\n+\tchar *to_free;\n \n \tif ((wt = find_worktree_by_suffix(list, arg)))\n \t\treturn wt;\n \n-\targ = prefix_filename(prefix, arg);\n+\tif (prefix)\n+\t\targ = to_free = prefix_filename(prefix, arg);\n \tpath = real_pathdup(arg, 1);\n \tfor (; *list; list++)\n \t\tif (!fspathcmp(path, real_path((*list)->path)))\n \t\t\tbreak;\n \tfree(path);\n+\tfree(to_free);\n \treturn *list;\n }\n \n-- \n2.12.1.683.gcd02edfec\n\n"},{"id":"314764","messageId":"20170321013040.cl42hlrtwix6qa23@sigill.intra.peff.net","threadId":"45450","inReplyTo":"20170321011838.rdhnbfwbigm4s4e3@sigill.intra.peff.net","subject":"[PATCH 5/6] prefix_filename: simplify windows #ifdef","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-21T01:30:41Z","receivedAt":"2017-03-21T01:32:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The prefix_filename function used to do an early return when\nthere was no prefix on non-Windows platforms, but always\nallocated on Windows so that it could call convert_slashes().\n\nNow that the function always allocates, we can unify the\nlogic and make convert_slashes() the only conditional part.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI wondered here if a compiler might complain about the dead assignment\nto pfx_len in the is_absolute_path() conditional. On Windows, that's\nused as an offset for calling into convert_slashes(), but on other\nplatforms we never look at it again.\n\nHowever, neither gcc nor clang complained, so I'm inclined to go with\nwhat I have here.\n\n abspath.c | 15 ++++++---------\n 1 file changed, 6 insertions(+), 9 deletions(-)\n\ndiff --git a/abspath.c b/abspath.c\nindex 4addd1fde..7f1cfe979 100644\n--- a/abspath.c\n+++ b/abspath.c\n@@ -251,18 +251,15 @@ char *prefix_filename(const char *pfx, const char *arg)\n \tstruct strbuf path = STRBUF_INIT;\n \tsize_t pfx_len = pfx ? strlen(pfx) : 0;\n \n-#ifndef GIT_WINDOWS_NATIVE\n-\tif (!pfx_len || is_absolute_path(arg))\n-\t\treturn xstrdup(arg);\n-\tstrbuf_add(&path, pfx, pfx_len);\n-\tstrbuf_addstr(&path, arg);\n-#else\n-\t/* don't add prefix to absolute paths, but still replace '\\' by '/' */\n-\tif (is_absolute_path(arg))\n+\tif (!pfx_len)\n+\t\t; /* nothing to prefix */\n+\telse if (is_absolute_path(arg))\n \t\tpfx_len = 0;\n-\telse if (pfx_len)\n+\telse\n \t\tstrbuf_add(&path, pfx, pfx_len);\n+\n \tstrbuf_addstr(&path, arg);\n+#ifdef GIT_WINDOWS_NATIVE\n \tconvert_slashes(path.buf + pfx_len);\n #endif\n \treturn strbuf_detach(&path, NULL);\n-- \n2.12.1.683.gcd02edfec\n\n"},{"id":"314765","messageId":"20170321013126.kcupajrxl7mrqq7i@sigill.intra.peff.net","threadId":"45450","inReplyTo":"20170321011838.rdhnbfwbigm4s4e3@sigill.intra.peff.net","subject":"[PATCH 6/6] bundle: use prefix_filename with bundle path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-21T01:31:27Z","receivedAt":"2017-03-21T01:32:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We may take the path to a bundle file as an argument, and\nneed to adjust the filename based on the prefix we\ndiscovered while setting up the git directory. We do so\nmanually into a fixed-size buffer, but using\nprefix_filename() is the normal way.\n\nBesides being more concise, there are two subtle\nimprovements:\n\n  1. The original inserted a \"/\" between the two paths, even\n     though the \"prefix\" argument always has the \"/\"\n     appended. That means that:\n\n       cd subdir && git bundle verify ../foo.bundle\n\n     was looking at (and reporting) subdir//../foo.bundle.\n     Harmless, but ugly.  Using prefix_filename() gets this\n     right.\n\n  2. The original checked for an absolute path by looking\n     for a leading '/'. It should have been using\n     is_absolute_path(), which also covers more cases on\n     Windows (backslashes and dos drive prefixes).\n\n     But it's easier still to just pass the name to\n     prefix_filename(), which handles this case\n     automatically.\n\nNote that we'll just leak the resulting buffer in the name\nof simplicity, since it needs to last through the duration\nof the program anyway.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/bundle.c | 8 +-------\n 1 file changed, 1 insertion(+), 7 deletions(-)\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex 4883a435a..d0de59b94 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -20,21 +20,15 @@ int cmd_bundle(int argc, const char **argv, const char *prefix)\n \tstruct bundle_header header;\n \tconst char *cmd, *bundle_file;\n \tint bundle_fd = -1;\n-\tchar buffer[PATH_MAX];\n \n \tif (argc < 3)\n \t\tusage(builtin_bundle_usage);\n \n \tcmd = argv[1];\n-\tbundle_file = argv[2];\n+\tbundle_file = prefix_filename(prefix, argv[2]);\n \targc -= 2;\n \targv += 2;\n \n-\tif (prefix && bundle_file[0] != '/') {\n-\t\tsnprintf(buffer, sizeof(buffer), \"%s/%s\", prefix, bundle_file);\n-\t\tbundle_file = buffer;\n-\t}\n-\n \tmemset(&header, 0, sizeof(header));\n \tif (strcmp(cmd, \"create\") && (bundle_fd =\n \t\t\t\tread_bundle_header(bundle_file, &header)) < 0)\n-- \n2.12.1.683.gcd02edfec\n"},{"id":"314826","messageId":"xmqq4lymsehu.fsf@gitster.mtv.corp.google.com","threadId":"45450","inReplyTo":"20170321011838.rdhnbfwbigm4s4e3@sigill.intra.peff.net","subject":"Re: [PATCH 0/6] prefix_filename cleanups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-21T17:53:33Z","receivedAt":"2017-03-21T17:54:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I noticed a spot in builtin/bundle.c that would benefit from using\n> prefix_filename(). But when I tried to use it, I noticed its interface\n> was a little error-prone (because it returns a static buffer). And\n> indeed, a little digging found a bug in hash-object related to this.\n>\n> So here's the fix for the hash-object bug, some cleanups to make such\n> bugs less likely, and then finally the bundle conversion. The bundle\n> thing does fix some minor bugs. It _could_ come before the cleanups if\n> we wanted to float the fixes to the top, but the function is much more\n> pleasant to call after the cleanups. :)\n\nThanks.  They all looked sensible.\n"},{"id":"314830","messageId":"xmqqinn2qyyo.fsf@gitster.mtv.corp.google.com","threadId":"45450","inReplyTo":"20170321012847.yebhpdmk5zrizgmj@sigill.intra.peff.net","subject":"Re: [PATCH 4/6] prefix_filename: return newly allocated string","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-21T18:14:23Z","receivedAt":"2017-03-21T18:20:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> diff --git a/worktree.c b/worktree.c\n> index 42dd3d52b..2520fc65c 100644\n> --- a/worktree.c\n> +++ b/worktree.c\n> @@ -250,16 +250,19 @@ struct worktree *find_worktree(struct worktree **list,\n>  {\n>  \tstruct worktree *wt;\n>  \tchar *path;\n> +\tchar *to_free;\n>  \n>  \tif ((wt = find_worktree_by_suffix(list, arg)))\n>  \t\treturn wt;\n>  \n> -\targ = prefix_filename(prefix, arg);\n> +\tif (prefix)\n> +\t\targ = to_free = prefix_filename(prefix, arg);\n>  \tpath = real_pathdup(arg, 1);\n>  \tfor (; *list; list++)\n>  \t\tif (!fspathcmp(path, real_path((*list)->path)))\n>  \t\t\tbreak;\n>  \tfree(path);\n> +\tfree(to_free);\n>  \treturn *list;\n>  }\n\nworktree.c:265:6: error: to_free may be used uninitialized in this function\n---\n worktree.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/worktree.c b/worktree.c\nindex 2520fc65cc..bae787cf8d 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -250,7 +250,7 @@ struct worktree *find_worktree(struct worktree **list,\n {\n \tstruct worktree *wt;\n \tchar *path;\n-\tchar *to_free;\n+\tchar *to_free = NULL;\n \n \tif ((wt = find_worktree_by_suffix(list, arg)))\n \t\treturn wt;\n-- \n2.12.1-382-gc0f9c70589\n\n"},{"id":"314833","messageId":"20170321182335.3yrjkhk4mxhso73j@sigill.intra.peff.net","threadId":"45450","inReplyTo":"xmqqinn2qyyo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/6] prefix_filename: return newly allocated string","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-21T18:23:35Z","receivedAt":"2017-03-21T18:25:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 21, 2017 at 11:14:23AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > diff --git a/worktree.c b/worktree.c\n> > index 42dd3d52b..2520fc65c 100644\n> > --- a/worktree.c\n> > +++ b/worktree.c\n> > @@ -250,16 +250,19 @@ struct worktree *find_worktree(struct worktree **list,\n> >  {\n> >  \tstruct worktree *wt;\n> >  \tchar *path;\n> > +\tchar *to_free;\n> >  \n> >  \tif ((wt = find_worktree_by_suffix(list, arg)))\n> >  \t\treturn wt;\n> >  \n> > -\targ = prefix_filename(prefix, arg);\n> > +\tif (prefix)\n> > +\t\targ = to_free = prefix_filename(prefix, arg);\n> >  \tpath = real_pathdup(arg, 1);\n> >  \tfor (; *list; list++)\n> >  \t\tif (!fspathcmp(path, real_path((*list)->path)))\n> >  \t\t\tbreak;\n> >  \tfree(path);\n> > +\tfree(to_free);\n> >  \treturn *list;\n> >  }\n> \n> worktree.c:265:6: error: to_free may be used uninitialized in this function\n\nDoh. I had originally written it without the \"if (prefix)\" and added it\nas a micro-optimization at the end.\n\nStill, the whole thing compiles fine for me. I find it odd that neither\ngcc nor clang notices the problem on my system. It's quite obviously\nwrong.\n\n> diff --git a/worktree.c b/worktree.c\n> index 2520fc65cc..bae787cf8d 100644\n> --- a/worktree.c\n> +++ b/worktree.c\n> @@ -250,7 +250,7 @@ struct worktree *find_worktree(struct worktree **list,\n>  {\n>  \tstruct worktree *wt;\n>  \tchar *path;\n> -\tchar *to_free;\n> +\tchar *to_free = NULL;\n\nYep, this is the right fix. Thanks.\n\n-Peff\n"},{"id":"314885","messageId":"20170321233159.usottyq56ygo26kf@sigill.intra.peff.net","threadId":"45450","inReplyTo":"20170321182335.3yrjkhk4mxhso73j@sigill.intra.peff.net","subject":"Re: [PATCH 4/6] prefix_filename: return newly allocated string","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-21T23:32:00Z","receivedAt":"2017-03-21T23:32:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 21, 2017 at 02:23:35PM -0400, Jeff King wrote:\n\n> > worktree.c:265:6: error: to_free may be used uninitialized in this function\n> \n> Doh. I had originally written it without the \"if (prefix)\" and added it\n> as a micro-optimization at the end.\n> \n> Still, the whole thing compiles fine for me. I find it odd that neither\n> gcc nor clang notices the problem on my system. It's quite obviously\n> wrong.\n\nAh, I found it; it is only triggered with -O2. I usually compile with\n-O0 during my edit/compile/test cycles.\n\n-Peff\n"}]}