git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Mike Rappazzo <rappazzo@gmail.com>
Date
May 6, 2016, 13:02 UTC
Message-ID
<CANoM8SWBzFiHGc3zAwndyx+GUkWQGDoaewVVQtH-06jazAn8uQ@mail.gmail.com>
In-Reply-To
<20160429135051.15492-1-szeder@ira.uka.de>
On Fri, Apr 29, 2016 at 9:50 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:
Show 16 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.

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

Show 15 quoted lines
>
>> 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.

Show 24 quoted lines
>> 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
Previous: Michael RappazzoNext: SZEDER Gábor
Message 9 of 12 in “rev-parse: adjust results when they should be relative”
  1. 0/4 rev-parse: adjust results when they should be relativeMichael Rappazzo, Apr 22, 2016
  2. 1/4 rev-parse: fix some options when executed from subpath of main treeMichael Rappazzo, Apr 22, 2016
  3. SZEDER GáborApr 29, 2016
  4. 2/4 t1500-rev-parse: add tests executed from sub path of the main worktreeMichael Rappazzo, Apr 22, 2016
  5. SZEDER GáborApr 29, 2016
  6. 3/4 t2027-worktree-list: add and adjust tests related to git-rev-parseMichael Rappazzo, Apr 22, 2016
  7. SZEDER GáborApr 29, 2016
  8. 4/4 t1700-split-index: add test for rev-parse --shared-index-pathMichael Rappazzo, Apr 22, 2016
  9. Mike RappazzoMay 6, 2016
  10. SZEDER GáborMay 6, 2016
  11. Junio C HamanoMay 6, 2016
  12. Eric SunshineMay 10, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.