{"thread":{"id":"47429","subject":"[PATCH] clone: support 'clone --shared' from a worktree","startedAt":"2017-12-11T23:16:37Z","lastAt":"2017-12-20T08:58:51Z","messageCount":8,"participants":["Eric Sunshine","Junio C Hamano","Brandon Williams"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"334638","messageId":"20171211231612.29275-1-sunshine@sunshineco.com","threadId":"47429","inReplyTo":null,"subject":"[PATCH] clone: support 'clone --shared' from a worktree","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-12-11T23:16:12Z","receivedAt":"2017-12-11T23:16:37Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"When worktree functionality was originally implemented, the possibility\nof 'clone --local' from within a worktree was overlooked, with the\nresult that the location of the \"objects\" directory of the source\nrepository was computed incorrectly, thus the objects could not be\ncopied or hard-linked by the clone. This shortcoming was addressed by\n744e469755 (clone: allow --local from a linked checkout, 2015-09-28).\n\nHowever, the related case of 'clone --shared' (despite being handled\nonly a few lines away from the 'clone --local' case) was not fixed by\n744e469755, with a similar result of the \"objects\" directory location\nbeing incorrectly computed for insertion into the 'alternates' file.\nFix this.\n\nReported-by: Marc-André Lureau <marcandre.lureau@gmail.com>\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n builtin/clone.c         | 3 ++-\n t/t2025-worktree-add.sh | 6 ++++++\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex b22845738a..6ad0ab3fa4 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -452,7 +452,8 @@ static void clone_local(const char *src_repo, const char *dest_repo)\n {\n \tif (option_shared) {\n \t\tstruct strbuf alt = STRBUF_INIT;\n-\t\tstrbuf_addf(&alt, \"%s/objects\", src_repo);\n+\t\tget_common_dir(&alt, src_repo);\n+\t\tstrbuf_addstr(&alt, \"/objects\");\n \t\tadd_to_alternates_file(alt.buf);\n \t\tstrbuf_release(&alt);\n \t} else {\ndiff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh\nindex b5c47ac602..7395973318 100755\n--- a/t/t2025-worktree-add.sh\n+++ b/t/t2025-worktree-add.sh\n@@ -245,6 +245,12 @@ test_expect_success 'local clone from linked checkout' '\n \t( cd here-clone && git fsck )\n '\n \n+test_expect_success 'local clone --shared from linked checkout' '\n+\tgit -C bare worktree add --detach ../baretree &&\n+\tgit clone --local --shared baretree bare-clone &&\n+\tgrep /bare/ bare-clone/.git/objects/info/alternates\n+'\n+\n test_expect_success '\"add\" worktree with --no-checkout' '\n \tgit worktree add --no-checkout -b swamp swamp &&\n \t! test -e swamp/init.t &&\n-- \n2.15.1.502.gccaef8de57\n\n"},{"id":"334652","messageId":"xmqq4low4xw6.fsf@gitster.mtv.corp.google.com","threadId":"47429","inReplyTo":"20171211231612.29275-1-sunshine@sunshineco.com","subject":"Re: [PATCH] clone: support 'clone --shared' from a worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-12T00:01:45Z","receivedAt":"2017-12-12T00:01:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> When worktree functionality was originally implemented, the possibility\n> of 'clone --local' from within a worktree was overlooked, with the\n> result that the location of the \"objects\" directory of the source\n> repository was computed incorrectly, thus the objects could not be\n> copied or hard-linked by the clone. This shortcoming was addressed by\n> 744e469755 (clone: allow --local from a linked checkout, 2015-09-28).\n>\n> However, the related case of 'clone --shared' (despite being handled\n> only a few lines away from the 'clone --local' case) was not fixed by\n> 744e469755, with a similar result of the \"objects\" directory location\n> being incorrectly computed for insertion into the 'alternates' file.\n> Fix this.\n>\n> Reported-by: Marc-André Lureau <marcandre.lureau@gmail.com>\n> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>\n> ---\n>  builtin/clone.c         | 3 ++-\n>  t/t2025-worktree-add.sh | 6 ++++++\n>  2 files changed, 8 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index b22845738a..6ad0ab3fa4 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -452,7 +452,8 @@ static void clone_local(const char *src_repo, const char *dest_repo)\n>  {\n>  \tif (option_shared) {\n>  \t\tstruct strbuf alt = STRBUF_INIT;\n> -\t\tstrbuf_addf(&alt, \"%s/objects\", src_repo);\n> +\t\tget_common_dir(&alt, src_repo);\n> +\t\tstrbuf_addstr(&alt, \"/objects\");\n>  \t\tadd_to_alternates_file(alt.buf);\n>  \t\tstrbuf_release(&alt);\n>  \t} else {\n\nMakes sense.  Will queue.\n\nThanks.\n"},{"id":"334653","messageId":"20171212001802.GC177995@google.com","threadId":"47429","inReplyTo":"20171211231612.29275-1-sunshine@sunshineco.com","subject":"Re: [PATCH] clone: support 'clone --shared' from a worktree","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-12T00:18:02Z","receivedAt":"2017-12-12T00:18:12Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 12/11, Eric Sunshine wrote:\n> When worktree functionality was originally implemented, the possibility\n> of 'clone --local' from within a worktree was overlooked, with the\n> result that the location of the \"objects\" directory of the source\n> repository was computed incorrectly, thus the objects could not be\n> copied or hard-linked by the clone. This shortcoming was addressed by\n> 744e469755 (clone: allow --local from a linked checkout, 2015-09-28).\n> \n> However, the related case of 'clone --shared' (despite being handled\n> only a few lines away from the 'clone --local' case) was not fixed by\n> 744e469755, with a similar result of the \"objects\" directory location\n> being incorrectly computed for insertion into the 'alternates' file.\n> Fix this.\n> \n> Reported-by: Marc-André Lureau <marcandre.lureau@gmail.com>\n> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>\n> ---\n>  builtin/clone.c         | 3 ++-\n>  t/t2025-worktree-add.sh | 6 ++++++\n>  2 files changed, 8 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index b22845738a..6ad0ab3fa4 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -452,7 +452,8 @@ static void clone_local(const char *src_repo, const char *dest_repo)\n>  {\n>  \tif (option_shared) {\n>  \t\tstruct strbuf alt = STRBUF_INIT;\n> -\t\tstrbuf_addf(&alt, \"%s/objects\", src_repo);\n> +\t\tget_common_dir(&alt, src_repo);\n> +\t\tstrbuf_addstr(&alt, \"/objects\");\n\nIf you wanted to do this in one function call you could either use\n'strbuf_git_common_path()' or either 'strbuf_git_path()' or\n'strbuf_repo_git_path()' which will do the proper path adjustments when\nworking on a path which should be shared between worktrees (i.e. part of\nthe common git dir).\n\n>  \t\tadd_to_alternates_file(alt.buf);\n>  \t\tstrbuf_release(&alt);\n>  \t} else {\n> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh\n> index b5c47ac602..7395973318 100755\n> --- a/t/t2025-worktree-add.sh\n> +++ b/t/t2025-worktree-add.sh\n> @@ -245,6 +245,12 @@ test_expect_success 'local clone from linked checkout' '\n>  \t( cd here-clone && git fsck )\n>  '\n>  \n> +test_expect_success 'local clone --shared from linked checkout' '\n> +\tgit -C bare worktree add --detach ../baretree &&\n> +\tgit clone --local --shared baretree bare-clone &&\n> +\tgrep /bare/ bare-clone/.git/objects/info/alternates\n> +'\n> +\n>  test_expect_success '\"add\" worktree with --no-checkout' '\n>  \tgit worktree add --no-checkout -b swamp swamp &&\n>  \t! test -e swamp/init.t &&\n> -- \n> 2.15.1.502.gccaef8de57\n> \n\n-- \nBrandon Williams\n"},{"id":"334656","messageId":"CAPig+cSTsuHdAqdBMvO80ybTSzfxncX8yQODSKd1bmaoNRQOjw@mail.gmail.com","threadId":"47429","inReplyTo":"20171212001802.GC177995@google.com","subject":"Re: [PATCH] clone: support 'clone --shared' from a worktree","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-12-12T00:40:56Z","receivedAt":"2017-12-12T00:41:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Dec 11, 2017 at 7:18 PM, Brandon Williams <bmwill@google.com> wrote:\n> On 12/11, Eric Sunshine wrote:\n>>               struct strbuf alt = STRBUF_INIT;\n>> -             strbuf_addf(&alt, \"%s/objects\", src_repo);\n>> +             get_common_dir(&alt, src_repo);\n>> +             strbuf_addstr(&alt, \"/objects\");\n>\n> If you wanted to do this in one function call you could either use\n> 'strbuf_git_common_path()' or either 'strbuf_git_path()' or\n> 'strbuf_repo_git_path()' which will do the proper path adjustments when\n> working on a path which should be shared between worktrees (i.e. part of\n> the common git dir).\n\nThanks for the pointers, however, the above fix mirrors the fix made\nby 744e469755 (clone: allow --local from a linked checkout,\n2015-09-28) to code immediately below it in the 'else' arm:\n\n    get_common_dir(&src, src_repo);\n    get_common_dir(&dest, dest_repo);\n    strbuf_addstr(&src, \"/objects\");\n    strbuf_addstr(&dest, \"/objects\");\n\nIt would be poor form and confusing to use one of the mechanisms you\nsuggest while leaving the 'else' arm untouched.\n\nRe-working both arms of the 'if' to use one of the suggested functions\nwould make a fine follow-on or preparatory patch, however, I'd rather\nnot hold up this fix merely to re-roll for such a minor cleanup. (I\nalso considered a follow-on patch to reduce the duplication between\nthe two cases but decided against it, for the present, since such a\npatch would almost be noise without much gain.)\n\nBy the way, is there any documentation explaining the differences\nbetween all these similar functions and when one should be used over\nthe others?\n"},{"id":"334658","messageId":"20171212005221.GD177995@google.com","threadId":"47429","inReplyTo":"CAPig+cSTsuHdAqdBMvO80ybTSzfxncX8yQODSKd1bmaoNRQOjw@mail.gmail.com","subject":"Re: [PATCH] clone: support 'clone --shared' from a worktree","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-12T00:52:21Z","receivedAt":"2017-12-12T00:52:28Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 12/11, Eric Sunshine wrote:\n> On Mon, Dec 11, 2017 at 7:18 PM, Brandon Williams <bmwill@google.com> wrote:\n> > On 12/11, Eric Sunshine wrote:\n> >>               struct strbuf alt = STRBUF_INIT;\n> >> -             strbuf_addf(&alt, \"%s/objects\", src_repo);\n> >> +             get_common_dir(&alt, src_repo);\n> >> +             strbuf_addstr(&alt, \"/objects\");\n> >\n> > If you wanted to do this in one function call you could either use\n> > 'strbuf_git_common_path()' or either 'strbuf_git_path()' or\n> > 'strbuf_repo_git_path()' which will do the proper path adjustments when\n> > working on a path which should be shared between worktrees (i.e. part of\n> > the common git dir).\n> \n> Thanks for the pointers, however, the above fix mirrors the fix made\n> by 744e469755 (clone: allow --local from a linked checkout,\n> 2015-09-28) to code immediately below it in the 'else' arm:\n> \n>     get_common_dir(&src, src_repo);\n>     get_common_dir(&dest, dest_repo);\n>     strbuf_addstr(&src, \"/objects\");\n>     strbuf_addstr(&dest, \"/objects\");\n> \n> It would be poor form and confusing to use one of the mechanisms you\n> suggest while leaving the 'else' arm untouched.\n> \n> Re-working both arms of the 'if' to use one of the suggested functions\n> would make a fine follow-on or preparatory patch, however, I'd rather\n> not hold up this fix merely to re-roll for such a minor cleanup. (I\n> also considered a follow-on patch to reduce the duplication between\n> the two cases but decided against it, for the present, since such a\n> patch would almost be noise without much gain.)\n\nI didn't look close enough at what you were trying to fix, you're right\nI think what you have here is good as the alternative would require a\nlot more reworking I think (at least to change the above part too).\n\nEither way though, I'm a little worried about what happens if you have\nGIT_COMMON_DIR set because then both the src and dest repo would share a\ncommon dir, I don't know if that is expected or not.  Maybe something\nelse to consider later.\n\n> \n> By the way, is there any documentation explaining the differences\n> between all these similar functions and when one should be used over\n> the others?\n\nI wish, I probably should have done a better job documenting it all in\npath.h when I added the repo_* flavor of functions.  I'll add that to my\nlist of things to do though :)\n\n-- \nBrandon Williams\n"},{"id":"334770","messageId":"20171213182802.114615-1-bmwill@google.com","threadId":"47429","inReplyTo":"20171212005221.GD177995@google.com","subject":"[PATCH] path: document path functions","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-13T18:28:02Z","receivedAt":"2017-12-13T18:28:13Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Signed-off-by: Brandon Williams <bmwill@google.com>\n---\n\nAs promised here is an update to the documentation for the path generating\nfunctions.\n\n path.h | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------\n 1 file changed, 112 insertions(+), 21 deletions(-)\n\ndiff --git a/path.h b/path.h\nindex 9541620c7..1ccd0373c 100644\n--- a/path.h\n+++ b/path.h\n@@ -4,53 +4,144 @@\n struct repository;\n \n /*\n- * Return a statically allocated filename, either generically (mkpath), in\n- * the repository directory (git_path), or in a submodule's repository\n- * directory (git_path_submodule). In all cases, note that the result\n- * may be overwritten by another call to _any_ of the functions. Consider\n- * using the safer \"dup\" or \"strbuf\" formats below (in some cases, the\n- * unsafe versions have already been removed).\n+ * The result to all functions which return statically allocated memory may be\n+ * overwritten by another call to _any_ one of these functions. Consider using\n+ * the safer variants which operate on strbufs or return allocated memory.\n  */\n-extern const char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2)));\n-extern const char *git_path(const char *fmt, ...) __attribute__((format (printf, 1, 2)));\n-extern const char *git_common_path(const char *fmt, ...) __attribute__((format (printf, 1, 2)));\n \n+/*\n+ * Return a statically allocated path.\n+ */\n+extern const char *mkpath(const char *fmt, ...)\n+\t__attribute__((format (printf, 1, 2)));\n+\n+/*\n+ * Return a path.\n+ */\n+extern char *mkpathdup(const char *fmt, ...)\n+\t__attribute__((format (printf, 1, 2)));\n+\n+/*\n+ * Construct a path and place the result in the provided buffer `buf`.\n+ */\n extern char *mksnpath(char *buf, size_t n, const char *fmt, ...)\n \t__attribute__((format (printf, 3, 4)));\n-extern void strbuf_git_path(struct strbuf *sb, const char *fmt, ...)\n-\t__attribute__((format (printf, 2, 3)));\n+\n+/*\n+ * The `git_common_path` family of functions will construct a path into a\n+ * repository's common git directory, which is shared by all worktrees.\n+ */\n+\n+/*\n+ * Constructs a path into the common git directory of repository `repo` and\n+ * append it in the provided buffer `sb`.\n+ */\n extern void strbuf_git_common_path(struct strbuf *sb,\n \t\t\t\t   const struct repository *repo,\n \t\t\t\t   const char *fmt, ...)\n \t__attribute__((format (printf, 3, 4)));\n-extern char *git_path_buf(struct strbuf *buf, const char *fmt, ...)\n-\t__attribute__((format (printf, 2, 3)));\n-extern int strbuf_git_path_submodule(struct strbuf *sb, const char *path,\n-\t\t\t\t     const char *fmt, ...)\n-\t__attribute__((format (printf, 3, 4)));\n-extern char *git_pathdup(const char *fmt, ...)\n-\t__attribute__((format (printf, 1, 2)));\n-extern char *mkpathdup(const char *fmt, ...)\n+\n+/*\n+ * Return a statically allocated path into the main repository's\n+ * (the_repository) common git directory.\n+ */\n+extern const char *git_common_path(const char *fmt, ...)\n \t__attribute__((format (printf, 1, 2)));\n-extern char *git_pathdup_submodule(const char *path, const char *fmt, ...)\n-\t__attribute__((format (printf, 2, 3)));\n \n+\n+/*\n+ * The `git_path` family of functions will construct a path into a repository's\n+ * git directory.\n+ *\n+ * These functions will perform adjustments to the resultant path to account\n+ * for special paths which are either considered common among worktrees (e.g.\n+ * paths into the object directory) or have been explicitly set via an\n+ * environment variable or config (e.g. path to the index file).\n+ *\n+ * For an exhaustive list of the adjustments made look at `common_list` and\n+ * `adjust_git_path` in path.c.\n+ */\n+\n+/*\n+ * Return a path into the git directory of repository `repo`.\n+ */\n extern char *repo_git_path(const struct repository *repo,\n \t\t\t   const char *fmt, ...)\n \t__attribute__((format (printf, 2, 3)));\n+\n+/*\n+ * Construct a path into the git directory of repository `repo` and append it\n+ * to the provided buffer `sb`.\n+ */\n extern void strbuf_repo_git_path(struct strbuf *sb,\n \t\t\t\t const struct repository *repo,\n \t\t\t\t const char *fmt, ...)\n \t__attribute__((format (printf, 3, 4)));\n \n+/*\n+ * Return a statically allocated path into the main repository's\n+ * (the_repository) git directory.\n+ */\n+extern const char *git_path(const char *fmt, ...)\n+\t__attribute__((format (printf, 1, 2)));\n+\n+/*\n+ * Return a path into the main repository's (the_repository) git directory.\n+ */\n+extern char *git_pathdup(const char *fmt, ...)\n+\t__attribute__((format (printf, 1, 2)));\n+\n+/*\n+ * Construct a path into the main repository's (the_repository) git directory\n+ * and place it in the provided buffer `buf`, the contents of the buffer will\n+ * be overridden.\n+ */\n+extern char *git_path_buf(struct strbuf *buf, const char *fmt, ...)\n+\t__attribute__((format (printf, 2, 3)));\n+\n+/*\n+ * Construct a path into the main repository's (the_repository) git directory\n+ * and append it to the provided buffer `sb`.\n+ */\n+extern void strbuf_git_path(struct strbuf *sb, const char *fmt, ...)\n+\t__attribute__((format (printf, 2, 3)));\n+\n+/*\n+ * Return a path into the worktree of repository `repo`.\n+ *\n+ * If the repository doesn't have a worktree NULL is returned.\n+ */\n extern char *repo_worktree_path(const struct repository *repo,\n \t\t\t\tconst char *fmt, ...)\n \t__attribute__((format (printf, 2, 3)));\n+\n+/*\n+ * Construct a path into the worktree of repository `repo` and append it\n+ * to the provided buffer `sb`.\n+ *\n+ * If the repository doesn't have a worktree nothing will be appended to `sb`.\n+ */\n extern void strbuf_repo_worktree_path(struct strbuf *sb,\n \t\t\t\t      const struct repository *repo,\n \t\t\t\t      const char *fmt, ...)\n \t__attribute__((format (printf, 3, 4)));\n \n+/*\n+ * Return a path into a submodule's git directory located at `path`.  `path`\n+ * must only reference a submodule of the main repository (the_repository).\n+ */\n+extern char *git_pathdup_submodule(const char *path, const char *fmt, ...)\n+\t__attribute__((format (printf, 2, 3)));\n+\n+/*\n+ * Construct a path into a submodule's git directory located at `path` and\n+ * append it to the provided buffer `sb`.  `path` must only reference a\n+ * submodule of the main repository (the_repository).\n+ */\n+extern int strbuf_git_path_submodule(struct strbuf *sb, const char *path,\n+\t\t\t\t     const char *fmt, ...)\n+\t__attribute__((format (printf, 3, 4)));\n+\n extern void report_linked_checkout_garbage(void);\n \n /*\n-- \n2.15.1.504.g5279b80103-goog\n\n"},{"id":"334774","messageId":"xmqqr2rywid7.fsf@gitster.mtv.corp.google.com","threadId":"47429","inReplyTo":"20171213182802.114615-1-bmwill@google.com","subject":"Re: [PATCH] path: document path functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-13T19:14:12Z","receivedAt":"2017-12-13T19:14:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams <bmwill@google.com> writes:\n\n> Signed-off-by: Brandon Williams <bmwill@google.com>\n> ---\n>\n> As promised here is an update to the documentation for the path generating\n> functions.\n>\n>  path.h | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------\n>  1 file changed, 112 insertions(+), 21 deletions(-)\n>\n> diff --git a/path.h b/path.h\n> index 9541620c7..1ccd0373c 100644\n> --- a/path.h\n> +++ b/path.h\n> @@ -4,53 +4,144 @@\n>  struct repository;\n>  \n>  /*\n> - * Return a statically allocated filename, either generically (mkpath), in\n> - * the repository directory (git_path), or in a submodule's repository\n> - * directory (git_path_submodule). In all cases, note that the result\n> - * may be overwritten by another call to _any_ of the functions. Consider\n> - * using the safer \"dup\" or \"strbuf\" formats below (in some cases, the\n> - * unsafe versions have already been removed).\n> + * The result to all functions which return statically allocated memory may be\n> + * overwritten by another call to _any_ one of these functions. Consider using\n> + * the safer variants which operate on strbufs or return allocated memory.\n>   */\n\nThe result of the update definitely is better, but stepping back a\nbit, it still does not answer some questions the original did not:\n\n - is the resulting path absolute?  if relative, relative to what (cwd)?\n\n - what should a caller write in fmt?  can we have a couple of\n   typical calling example?\n\n - does a caller need to know which hierarchies under $GIT_DIR are\n   common and call git_common_path() for them, or does git_path()\n   do the 'right' thing for the caller?  If latter, what's the use\n   of git_common_path() for callers (outside the implementation of\n   path.c API)?\n\nWill queue.  Thanks.\n"},{"id":"335067","messageId":"CAPig+cTG8Sp7G+A9sbV9u4HbtgSu9-X7FiamvM4Ncu1_VhgVXA@mail.gmail.com","threadId":"47429","inReplyTo":"20171213182802.114615-1-bmwill@google.com","subject":"Re: [PATCH] path: document path functions","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-12-20T08:58:44Z","receivedAt":"2017-12-20T08:58:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 13, 2017 at 1:28 PM, Brandon Williams <bmwill@google.com> wrote:\n> As promised here is an update to the documentation for the path generating\n> functions.\n\nThanks for working on this. Aside from the review comments by\nJunio[1], see a few more below. Although I had some familiarity with\nthe non-repo versions of these functions a couple year ago when\nworking on git-worktree, I've since forgotten much of it, so take my\ncomments as if from someone trying to learn this API from scratch\n(which is effectively what I'm doing again). In particular, as I read\nthis patch, I find that I still have to consult the source code\n(path.c) to figure out what some of these functions are for or what\nthey do.\n\n[1]: https://public-inbox.org/git/xmqqr2rywid7.fsf@gitster.mtv.corp.google.com/\n\n> diff --git a/path.h b/path.h\n> @@ -4,53 +4,144 @@\n>  /*\n> + * The result to all functions which return statically allocated memory may be\n> + * overwritten by another call to _any_ one of these functions. Consider using\n> + * the safer variants which operate on strbufs or return allocated memory.\n>   */\n>\n> +/*\n> + * Return a statically allocated path.\n> + */\n> +extern const char *mkpath(const char *fmt, ...)\n> +       __attribute__((format (printf, 1, 2)));\n\nIt's not, at first glance, clear what benefit this function provides\nover simply constructing a path manually with, say, sprintf() or\nstrbuf. Should this mention that a certain amount of normalization is\nperformed on the path?\n\nFurthermore, perhaps the comment block above which talks about\n\"statically allocated memory\" could do a better job of conveying that\nthese functions rid the caller of the responsibility of managing\nstring storage itself. I wonder if it also might make sense to mention\nthat the returned path won't necessarily be overwritten by the very\nnext call (or is that an implementation detail we'd rather people not\nrely upon?).\n\n> +/*\n> + * Return a path.\n> + */\n> +extern char *mkpathdup(const char *fmt, ...)\n> +       __attribute__((format (printf, 1, 2)));\n\nI realize that it's implied by \"dup\", but perhaps the documentation\nshould state explicitly that the caller is responsible for freeing the\nresult?\n\n> +/*\n> + * The `git_common_path` family of functions will construct a path into a\n> + * repository's common git directory, which is shared by all worktrees.\n> + */\n\nMy first question when reading this was \"What is a common git\ndirectory? (Is that where a bunch of repositories live or what?)\". I\nsuppose if it had said \"common .git/ directory\", it would have been\nclearer. Perhaps mentioning GIT_DIR might help clarify it, as well.\n\n    ...repository's common .git/ directory (or $GIT_DIR), which\n    is shared by all worktrees.\n\n> +/*\n> + * Constructs a path into the common git directory of repository `repo` and\n> + * append it in the provided buffer `sb`.\n> + */\n>  extern void strbuf_git_common_path(struct strbuf *sb,\n>                                    const struct repository *repo,\n>                                    const char *fmt, ...)\n>         __attribute__((format (printf, 3, 4)));\n\nIt's nice that this states explicitly that it _appends_ rather than\noverwrites. So often, one has to consult the implementation to\ndetermine the actual behavior.\n\nPerhaps I'm simple-minded, but at this point in the read, the concept\nof \"common git directory\" still feels nebulous. If the documentation\npresented even a simple example of how this is used (such as, for\ninstance, calling this function to obtain \".git/HEAD\"), it would help\nmake the concept more concrete. Or, perhaps the example could be\npresented in the comment block just above which talks about this\nfamily of functions.\n\n> +/*\n> + * Return a statically allocated path into the main repository's\n> + * (the_repository) common git directory.\n> + */\n> +extern const char *git_common_path(const char *fmt, ...)\n>         __attribute__((format (printf, 1, 2)));\n\nI suppose that other repositories would be submodules? Would it make\nsense to mention something about that to clue in readers?\n\n    Return a statically allocated path into the common .git/ directory\n    of the main repository (not a submodule repository).\n\nOr something.\n\nAside: After reading \"main repository\" here and seeing the 'repo'\nvariants below, one wonders why the corresponding\nrepo_git_common_path() doesn't exist. (Just an idle question; not\nactionable, and outside the scope of this patch.)\n\n> +/*\n> + * The `git_path` family of functions will construct a path into a repository's\n> + * git directory.\n> + *\n> + * These functions will perform adjustments to the resultant path to account\n> + * for special paths which are either considered common among worktrees (e.g.\n> + * paths into the object directory) or have been explicitly set via an\n> + * environment variable or config (e.g. path to the index file).\n> + *\n> + * For an exhaustive list of the adjustments made look at `common_list` and\n> + * `adjust_git_path` in path.c.\n> + */\n\nI feel somewhat clueless after reading this. It doesn't necessarily\ngive enough information for the reader to understand when or why these\nfunctions should be used as opposed to one of the functions described\nearlier. This may be due to it not stating early enough or forcefully\nenough that the location of certain files is not fixed, that they\nreside elsewhere when worktrees are involved. Perhaps something along\nthese lines would help make it more concrete:\n\n    Administrative files within .git/ may reside at different\n    locations depending upon whether worktrees are involved. Some\n    files and directories are common to all worktrees (for instance,\n    \".git/hooks\", \".git/objects\") but others are localized to the main\n    directory or to each worktree (for instance, \".git/HEAD\" vs\n    \".git/worktrees/<id>/HEAD). Other factors, such as environment\n    variables or configuration settings may also impact locations of\n    administrative files.\n\n    The 'git_path' family of functions employs specialized knowledge\n    of these issues when constructing a path to an administrative file\n    or directory, thus relieving the caller of the burden of manually\n    figuring out where resources reside.\n\n> +/*\n> + * Return a path into the git directory of repository `repo`.\n> + */\n>  extern char *repo_git_path(const struct repository *repo,\n>                            const char *fmt, ...)\n>         __attribute__((format (printf, 2, 3)));\n\nI presume by the return type that the caller is responsible for\nfreeing the result. Perhaps the documentation can state that\nexplicitly.\n\n> +/*\n> + * Return a path into the main repository's (the_repository) git directory.\n> + */\n> +extern char *git_pathdup(const char *fmt, ...)\n> +       __attribute__((format (printf, 1, 2)));\n\nPerhaps mention that the caller is responsible for freeing the result.\n\n> +/*\n> + * Construct a path into the main repository's (the_repository) git directory\n> + * and place it in the provided buffer `buf`, the contents of the buffer will\n> + * be overridden.\n\ns/overridden/overwritten/\n\n> + */\n> +extern char *git_path_buf(struct strbuf *buf, const char *fmt, ...)\n> +       __attribute__((format (printf, 2, 3)));\n\nNice to see that this explains that the buffer will be overwritten (as\nopposed to appended). This function is a less flexible special case of\nstrbuf_git_path() below. Does this mean it is deprecated? Should the\ndocumentation say something about preferring strbuf_git_path()\ninstead?\n\n> +/*\n> + * Construct a path into the main repository's (the_repository) git directory\n> + * and append it to the provided buffer `sb`.\n> + */\n> +extern void strbuf_git_path(struct strbuf *sb, const char *fmt, ...)\n> +       __attribute__((format (printf, 2, 3)));\n> +\n> +/*\n> + * Return a path into the worktree of repository `repo`.\n> + *\n> + * If the repository doesn't have a worktree NULL is returned.\n> + */\n>  extern char *repo_worktree_path(const struct repository *repo,\n>                                 const char *fmt, ...)\n>         __attribute__((format (printf, 2, 3)));\n\nNow this is getting confusing. The documentation for the 'git_path'\nfamily of functions said that it takes worktrees into consideration.\nSo what precisely do the 'worktree_path' family of functions do? When\ndoes one use them over the 'git_path' family?\n\nAlso, caller is responsible for freeing result.\n\n> +/*\n> + * Return a path into a submodule's git directory located at `path`.  `path`\n> + * must only reference a submodule of the main repository (the_repository).\n> + */\n\nOuch, ambiguity hurts my brain. Does this mean that the submodule is\nlocated at 'path' or that the .git/ directory is located at 'path'?\nPerhaps:\n\n    Return a path into the .git/ directory of a submodule located at\n    'path'.\n"}]}