Re: [PATCH] rev-parse: fix --git-common-dir when executed from subpath of main tree

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Apr 4, 2016, 06:01 UTC
Message-ID: <CAPig+cSDMD_MjRVh534YfWeY0E1VykWW08oML9oe7QVuxRC72g@mail.gmail.com>
In-Reply-To: <1459734143-95832-1-git-send-email-rappazzo@gmail.com>

On Sun, Apr 3, 2016 at 9:42 PM, Michael Rappazzo <rappazzo@gmail.com> wrote:

> Executing `git-rev-parse --git-common-dir` from the root of the main
> worktree results in '.git', which is the relative path to the git dir.
> When executed from a subpath of the main tree it returned somthing like:

s/returned/returns/ s/somthing/something/

It would be clearer if you stated explicitly that the subpath result is incorrect. For instance:

    When executed from a subdirectory of the main tree,
    however, it incorrectly returns:

More below...

Show 25 quoted lines

> 'sub/path/.git'.  Change this to return the proper relative path to the
> git directory (similar to `--show-cdup`).
>
> Add as test to t1500-rev-parse.sh for this case and adjust another test
> in t2027-worktree-list.sh to use this expectation.
>
> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
> ---
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> @@ -787,8 +787,18 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>                         if (!strcmp(arg, "--git-common-dir")) {
> -                               const char *pfx = prefix ? prefix : "";
> -                               puts(prefix_filename(pfx, strlen(pfx), get_git_common_dir()));
> +                               const char *git_common_dir = get_git_common_dir();
> +                               if (prefix && !is_absolute_path(git_common_dir)) {
> +                                       const char *pfx = prefix;
> +                                       while (pfx) {
> +                                               pfx = strchr(pfx, '/');
> +                                               if (pfx) {
> +                                                       pfx++;
> +                                                       printf("../");
> +                                               }
> +                                       }
> +                               }
> +                               printf("%s\n", git_common_dir);

How about simplifying this entire chunk of code to?

    struct strbuf sb = STRBUF_INIT;
    puts(relative_path(get_git_common_dir(), prefix, &sb));
    strbuf_release(&sb);

No need to check NULL prefix or absolute common dir.

Show 9 quoted lines

>                                 continue;
>                         }
> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
> @@ -3,6 +3,16 @@
>  test_description='test git rev-parse'
>  . ./test-lib.sh
>
> +test_expect_success 'git-common-dir inside sub-dir' '
> +   (

Strange indentation. Use TAB rather than spaces.

> +               mkdir -p path/to/child &&

Nit: Although functionally the same, typically 'mkdir' is outside the subshell.

Show 6 quoted lines

> +               cd path/to/child &&
> +               echo "$(git rev-parse --show-cdup).git" >expect &&
> +               git rev-parse --git-common-dir >actual &&
> +               test_cmp expect actual
> +       )
> +'

I guess you added this new test to the top of the script rather than the bottom (as is more customary) because this script is ancient and cd's all around the place with wild abandon and leaks environment variables; thus you avoided having to prefix this new test with:

    cd .. || exit 1
    sane_unset GIT_DIR GIT_CONFIG

which would have been needed had it been added to at the bottom of the script.

It probably wouldn't hurt to add tests to also verify correct behavior at the root of the main tree, as well as at the root and within a subdirectory of a linked worktree (though the latter two tests would probably go in a worktree-related test script).

Thanks.