Re: [PATCH v2 1/4] rev-parse: fix some options when executed from subpath of main tree

From: SZEDER Gábor <szeder@ira.uka.de>
Date: Apr 29, 2016, 14:21 UTC
Message-ID: <20160429142131.16282-1-szeder@ira.uka.de>
In-Reply-To: <1461361992-91918-2-git-send-email-rappazzo@gmail.com>

[Resend to list, sorry for the duplicates.]

Show 6 quoted lines

> Executing `git-rev-parse` with `--git-common-dir`, `--git-path <path>`,
> or `--shared-index-path` from the root of the main worktree results in
> a relative path to the git dir.
> 
> When executed from a subdirectory of the main tree, however, it incorrectly
> returns a path which starts 'sub/path/.git'.

This is not completely true, because '--git-path ...' returns a relative path starting with '.git':

  $ git -C t/ rev-parse --git-dir --git-path objects --git-common-dir
  /home/szeder/src/git/.git
  .git/objects
  t/.git

It's still wrong, of course.

> Change this to return the
> proper relative path to the git directory.

I think returning absolute paths would be better. It is consistent with the already properly working '--git-dir' option, which returns an absolute path in this case. Furthermore, both '--git-path ...' and '--git-common-dir' already return absolute paths when run from a subdirectory of the .git directory:

  $ git -C .git/refs rev-parse --git-dir --git-path objects --git-common-dir
  /home/szeder/src/git/.git
  /home/szeder/src/git/.git/objects
  /home/szeder/src/git/.git

> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
> ---
>  builtin/rev-parse.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)

This patch doesn't add any new tests, while subsequent patches of the series do nothing but add more tests. Splitting up your changes this way doesn't add any value, it only increases the number of commits. I think either:

  - all those new tests could be added with this patch, or

  - if you want to add the test separately, then add them before
    this patch and mark them with 'test_expect_failure' to clearly
    demonstrate what the series is about to fix, and flip them to
    'test_expect_success' in this patch.

  - An alternative way to split this series, following the "Make
    separate commits for logically separate changes" guideline, would
    be to fix and test these options in separate patches, i.e. fix and
    test '--git-path ...' in one patch, then fix and test
    '--git-common-dir' in the next, ...