git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "SZEDER Gábor" <szeder@ira.uka.de>
To: Michael Rappazzo <rappazzo@gmail.com>
Cc: "SZEDER Gábor" <szeder@ira.uka.de>,
	gitster@pobox.com, sunshine@sunshineco.com, pclouds@gmail.com,
	git@vger.kernel.org
Subject: Re: [PATCH v2 1/4] rev-parse: fix some options when executed from subpath of main tree
Date: Fri, 29 Apr 2016 16:21:31 +0200	[thread overview]
Message-ID: <20160429142131.16282-1-szeder@ira.uka.de> (raw)
In-Reply-To: <1461361992-91918-2-git-send-email-rappazzo@gmail.com>

[Resend to list, sorry for the duplicates.]

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

  reply	other threads:[~2016-04-29 14:21 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-04-22 21:53 [PATCH v2 0/4] rev-parse: adjust results when they should be relative Michael Rappazzo
2016-04-22 21:53 ` [PATCH v2 1/4] rev-parse: fix some options when executed from subpath of main tree Michael Rappazzo
2016-04-29 14:21   ` SZEDER Gábor [this message]
     [not found]   ` <20160429135051.15492-1-szeder@ira.uka.de>
2016-05-06 13:02     ` Mike Rappazzo
2016-05-06 14:13       ` SZEDER Gábor
2016-05-06 17:08         ` Junio C Hamano
2016-05-10  7:49         ` Eric Sunshine
2016-04-22 21:53 ` [PATCH v2 2/4] t1500-rev-parse: add tests executed from sub path of the main worktree Michael Rappazzo
2016-04-29 14:22   ` SZEDER Gábor
2016-04-22 21:53 ` [PATCH v2 3/4] t2027-worktree-list: add and adjust tests related to git-rev-parse Michael Rappazzo
2016-04-29 14:22   ` SZEDER Gábor
2016-04-22 21:53 ` [PATCH v2 4/4] t1700-split-index: add test for rev-parse --shared-index-path Michael Rappazzo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160429142131.16282-1-szeder@ira.uka.de \
    --to=szeder@ira.uka.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=pclouds@gmail.com \
    --cc=rappazzo@gmail.com \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).