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

[Mike Rappazzo <rappazzo@gmail.com>]

On Fri, Apr 29, 2016 at 9:50 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>> 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.

I'll try to reword this to make it indicate that the value isn't
always incorrect.

>
>> 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
>

I agree with this, but at one point Junio suggested that it should
return the relative path[1],
so I went with that.  Maybe I could RFC a separate patch that changes
all of these to
absolute.

>> 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, ...
>
>

Thanks, have a re-roll ready to go based on you input.  I went with
option 2 - add
tests with expect failure and fix them with the next.

[1] http://article.gmane.org/gmane.comp.version-control.git/290061