[PATCH v2 0/4] rev-parse: adjust results when they should be relative

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 0/4] rev-parse: adjust results when they should be relative

Michael Rappazzo
Differences from v1[1]:
 - Simplify implementation based on review comments
 - Included similar changes in rev-parse for --git-path and --shared-index-path
 - Added tests and separated them into individual commits

[1] http://thread.gmane.org/gmane.comp.version-control.git/290669

Michael Rappazzo (4):
  rev-parse: fix some options when executed from subpath of main tree
  t1500-rev-parse: add tests executed from sub path of the main worktree
  t2027-worktree-list: add and adjust tests related to git-rev-parse
  t1700-split-index: add test for rev-parse --shared-index-path

 builtin/rev-parse.c      | 19 ++++++++++++++-----
 t/t1500-rev-parse.sh     | 37 +++++++++++++++++++++++++++++++++++++
 t/t1700-split-index.sh   | 17 +++++++++++++++++
 t/t2027-worktree-list.sh | 10 +++++++++-
 4 files changed, 77 insertions(+), 6 deletions(-)

--
2.8.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Michael Rappazzo
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'.  Change this to return the
proper relative path to the git directory.

Signed-off-by: Michael Rappazzo <[hidden email]>
---
 builtin/rev-parse.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index c961b74..1da2e10 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -564,10 +564,13 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
  }
 
  if (!strcmp(arg, "--git-path")) {
+ struct strbuf sb = STRBUF_INIT;
  if (!argv[i + 1])
  die("--git-path requires an argument");
- puts(git_path("%s", argv[i + 1]));
- i++;
+
+ puts(relative_path(xstrfmt("%s/%s", get_git_dir(), argv[++i]),
+ prefix, &sb));
+ strbuf_release(&sb);
  continue;
  }
  if (as_is) {
@@ -787,8 +790,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
  continue;
  }
  if (!strcmp(arg, "--git-common-dir")) {
- const char *pfx = prefix ? prefix : "";
- puts(prefix_filename(pfx, strlen(pfx), get_git_common_dir()));
+ struct strbuf sb = STRBUF_INIT;
+ puts(relative_path(get_git_common_dir(), prefix, &sb));
+ strbuf_release(&sb);
  continue;
  }
  if (!strcmp(arg, "--is-inside-git-dir")) {
@@ -811,7 +815,12 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
  die(_("Could not read the index"));
  if (the_index.split_index) {
  const unsigned char *sha1 = the_index.split_index->base_sha1;
- puts(git_path("sharedindex.%s", sha1_to_hex(sha1)));
+ struct strbuf sb = STRBUF_INIT;
+
+ puts(relative_path(
+ xstrfmt("%s/sharedindex.%s", get_git_dir(), sha1_to_hex(sha1)),
+ prefix, &sb));
+ strbuf_release(&sb);
  }
  continue;
  }
--
2.8.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 2/4] t1500-rev-parse: add tests executed from sub path of the main worktree

Michael Rappazzo
In reply to this post by Michael Rappazzo
Signed-off-by: Michael Rappazzo <[hidden email]>
---
 t/t1500-rev-parse.sh | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 48ee077..1e220f7 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -36,6 +36,7 @@ test_rev_parse() {
 # label is-bare is-inside-git is-inside-work prefix git-dir
 
 ROOT=$(pwd)
+original_core_bare=$(git config core.bare)
 
 test_rev_parse toplevel false false true '' .git
 
@@ -84,4 +85,40 @@ test_rev_parse 'GIT_DIR=../repo.git, core.bare = true' true false false ''
 git config --unset core.bare
 test_rev_parse 'GIT_DIR=../repo.git, core.bare undefined' false false true ''
 
+#cleanup from the above
+cd ..
+rm -r work
+mv repo.git .git || exit 1
+unset GIT_DIR
+unset GIT_CONFIG
+git config core.bare $original_core_bare
+
+test_expect_success 'git-common-dir from worktree root' '
+ echo .git >expect &&
+ git rev-parse --git-common-dir >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'git-common-dir inside sub-dir' '
+ mkdir -p path/to/child &&
+ test_when_finished "rm -rf path" &&
+ echo "$(git -C path/to/child rev-parse --show-cdup).git" >expect &&
+ git -C path/to/child rev-parse --git-common-dir >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'git-path from worktree root' '
+ echo .git/objects >expect &&
+ git rev-parse --git-path objects >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'git-path inside sub-dir' '
+ mkdir -p path/to/child &&
+ test_when_finished "rm -rf path" &&
+ echo "$(git -C path/to/child rev-parse --show-cdup).git/objects" >expect &&
+ git -C path/to/child rev-parse --git-path objects >actual &&
+ test_cmp expect actual
+'
+
 test_done
--
2.8.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 3/4] t2027-worktree-list: add and adjust tests related to git-rev-parse

Michael Rappazzo
In reply to this post by Michael Rappazzo
Adjust the incorrect expectation for `rev-parse --git-common-dir`.

Add a test for `git rev-parse --git-path` executed from a linked
worktree.

Signed-off-by: Michael Rappazzo <[hidden email]>
---
 t/t2027-worktree-list.sh | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
index 1b1b65a..16eec6e 100755
--- a/t/t2027-worktree-list.sh
+++ b/t/t2027-worktree-list.sh
@@ -14,10 +14,18 @@ test_expect_success 'rev-parse --git-common-dir on main worktree' '
  test_cmp expected actual &&
  mkdir sub &&
  git -C sub rev-parse --git-common-dir >actual2 &&
- echo sub/.git >expected2 &&
+ echo ../.git >expected2 &&
  test_cmp expected2 actual2
 '
 
+test_expect_success 'rev-parse --git-path objects linked worktree' '
+ echo "$(git rev-parse --show-toplevel)/.git/worktrees/linked-tree/objects" >expect &&
+ test_when_finished "rm -rf linked-tree && git worktree prune" &&
+ git worktree add --detach linked-tree master &&
+ git -C linked-tree rev-parse --git-path objects >actual &&
+ test_cmp expect actual
+'
+
 test_expect_success '"list" all worktrees from main' '
  echo "$(git rev-parse --show-toplevel) $(git rev-parse --short HEAD) [$(git symbolic-ref --short HEAD)]" >expect &&
  test_when_finished "rm -rf here && git worktree prune" &&
--
2.8.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 4/4] t1700-split-index: add test for rev-parse --shared-index-path

Michael Rappazzo
In reply to this post by Michael Rappazzo
Signed-off-by: Michael Rappazzo <[hidden email]>
---
 t/t1700-split-index.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index 8aef49f..d2d9e02 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -200,4 +200,21 @@ EOF
  test_cmp expect actual
 '
 
+test_expect_success 'rev-parse --shared-index-path' '
+ rm -rf .git &&
+ test_create_repo . &&
+ git update-index --split-index &&
+ ls -t .git/sharedindex* | tail -n 1 >expect &&
+ git rev-parse --shared-index-path >actual &&
+ test_cmp expect actual &&
+ mkdir work &&
+ test_when_finished "rm -rf work" &&
+ (
+ cd work &&
+ ls -t ../.git/sharedindex* | tail -n 1 >expect &&
+ git rev-parse --shared-index-path >actual &&
+ test_cmp expect actual
+ )
+'
+
 test_done
--
2.8.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

SZEDER Gábor
In reply to this post by Michael Rappazzo
[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 <[hidden email]>
> ---
>  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, ...


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 2/4] t1500-rev-parse: add tests executed from sub path of the main worktree

SZEDER Gábor
In reply to this post by Michael Rappazzo
[Resend to list, sorry for the duplicates.]

> Signed-off-by: Michael Rappazzo <[hidden email]>
> ---
>  t/t1500-rev-parse.sh | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>
> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
> index 48ee077..1e220f7 100755
> --- a/t/t1500-rev-parse.sh
> +++ b/t/t1500-rev-parse.sh
> @@ -36,6 +36,7 @@ test_rev_parse() {
>  # label is-bare is-inside-git is-inside-work prefix git-dir
>  
>  ROOT=$(pwd)
> +original_core_bare=$(git config core.bare)
>  
>  test_rev_parse toplevel false false true '' .git
>  
> @@ -84,4 +85,40 @@ test_rev_parse 'GIT_DIR=../repo.git, core.bare = true' true false false ''
>  git config --unset core.bare
>  test_rev_parse 'GIT_DIR=../repo.git, core.bare undefined' false false true ''
>  
> +#cleanup from the above
> +cd ..
> +rm -r work
> +mv repo.git .git || exit 1

You can't just 'exit 1' mid-script, because terminating the test script
abruptly makes the test harness unhappy.

> +unset GIT_DIR
> +unset GIT_CONFIG

Both variables are set at this point, so calling plain 'unset' is OK.
Still, I would suggest using 'sane_unset' instead, so the next person
looking at this test doesn't have to spend brain cycles on figuring
out whether plain 'unset' is indeed safe or not.

> +git config core.bare $original_core_bare

This whole '#cleanup from the above' block is just ugly.  Not your
fault, of course, but the consequence of how the preceeding tests were
written in the past.  I think it would be best if this series were
scheduled on top of the 't1500 cleanup & modernization' patch I saw a
few days ago, then this block wouldn't be necessary at all.

> +test_expect_success 'git-common-dir from worktree root' '
> + echo .git >expect &&
> + git rev-parse --git-common-dir >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'git-common-dir inside sub-dir' '
> + mkdir -p path/to/child &&
> + test_when_finished "rm -rf path" &&
> + echo "$(git -C path/to/child rev-parse --show-cdup).git" >expect &&
> + git -C path/to/child rev-parse --git-common-dir >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'git-path from worktree root' '
> + echo .git/objects >expect &&
> + git rev-parse --git-path objects >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'git-path inside sub-dir' '
> + mkdir -p path/to/child &&
> + test_when_finished "rm -rf path" &&
> + echo "$(git -C path/to/child rev-parse --show-cdup).git/objects" >expect &&
> + git -C path/to/child rev-parse --git-path objects >actual &&
> + test_cmp expect actual
> +'
> +
>  test_done
> --
> 2.8.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 3/4] t2027-worktree-list: add and adjust tests related to git-rev-parse

SZEDER Gábor
In reply to this post by Michael Rappazzo
[Resend to list, sorry for the duplicates...]

> Adjust the incorrect expectation for `rev-parse --git-common-dir`.
>
> Add a test for `git rev-parse --git-path` executed from a linked
> worktree.
>
> Signed-off-by: Michael Rappazzo <[hidden email]>
> ---
>  t/t2027-worktree-list.sh | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
> index 1b1b65a..16eec6e 100755
> --- a/t/t2027-worktree-list.sh
> +++ b/t/t2027-worktree-list.sh
> @@ -14,10 +14,18 @@ test_expect_success 'rev-parse --git-common-dir on main worktree' '
>   test_cmp expected actual &&
>   mkdir sub &&
>   git -C sub rev-parse --git-common-dir >actual2 &&
> - echo sub/.git >expected2 &&
> + echo ../.git >expected2 &&
>   test_cmp expected2 actual2
>  '

This hunk must go into patch 1/4.

The full test suite should pass after every single commit.  As patch
1/4 fixes things, the changing behavior makes this test case fail,
resulting in two commits in which 'make test' would fail.

> +test_expect_success 'rev-parse --git-path objects linked worktree' '
> + echo "$(git rev-parse --show-toplevel)/.git/worktrees/linked-tree/objects" >expect &&
> + test_when_finished "rm -rf linked-tree && git worktree prune" &&
> + git worktree add --detach linked-tree master &&
> + git -C linked-tree rev-parse --git-path objects >actual &&
> + test_cmp expect actual
> +'
> +
>  test_expect_success '"list" all worktrees from main' '
>   echo "$(git rev-parse --show-toplevel) $(git rev-parse --short HEAD) [$(git symbolic-ref --short HEAD)]" >expect &&
>   test_when_finished "rm -rf here && git worktree prune" &&
> --
> 2.8.0



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Michael Rappazzo
In reply to this post by Michael Rappazzo
On Fri, Apr 29, 2016 at 9:50 AM, SZEDER Gábor <[hidden email]> 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 <[hidden email]>
>> ---
>>  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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

SZEDER Gábor

Quoting Mike Rappazzo <[hidden email]>:

> On Fri, Apr 29, 2016 at 9:50 AM, SZEDER Gábor <[hidden email]> 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.

Not sure I understand your intention about rewording, in particular that
"isn't always incorrect" part.  Just to make sure there is no
misunderstanding let's have a look at the two broken cases:

    $ git -C t/ rev-parse --git-common-dir
    t/.git

Obviously wrong.
This is what you correctly described in the commit message as
"incorrectly returns a path which starts 'sub/path/.git'.

    $ git -C t/ rev-parse --git-path objects
    .git/objects

Wrong as well.  It would be correct if we were at the top of the working
tree, but we are not.  It must be relative to the directory where '-C t/'
brought us.
Your description in the commit message implies that '--git-path <path>'
is wrong in the same way as '--git-common-dir' is, i.e. in this case
would also return the relative path starting with the subdirectory.
That is apparently not the case.

My point in the previous email was that both are wrong when executed in
a subdir of the working tree, but they are wrong in different ways.  And
they are always wrong when executed from a subdir of the working tree.
I would have described the current wrong behavior as:

    When executed from a subdirectory of the working tree, however,
    '--git-common-dir' incorrectly returns a path which starts
    'sub/path/.git', while '--git-path <path>' incorrectly returns a path
    relative to the top of the working tree.

(Still hasn't looked at shared index...)

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

I wasn't aware of Junio's suggestion.

It shouldn't really matter in practice, because both the absolute and
relative paths will ultimately lead to the same place.  However, I
still think that for consistency's sake absolute paths would be better
when executed in a subdir of the working tree.


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano
SZEDER Gábor <[hidden email]> writes:

>> I agree with this, but at one point Junio suggested that it should
>> return the relative path[1],
>
> I wasn't aware of Junio's suggestion.

Mike justified his position to change everything to absolute, citing
that it is what "rev-parse --show-toplevel" does when run from a
subdirectory.  I just gave a counter-example that rev-parse does not
always talk in absolute with "rev-parse -C t --show-cdup".

It hinted that his justification may not be strong enough to choose
absolute over relative, but wasn't meant to be a suggestion to
choose relative at all.  At most, I was saying "either is equally
plausible".

> It shouldn't really matter in practice, because both the absolute and
> relative paths will ultimately lead to the same place.  However, I
> still think that for consistency's sake absolute paths would be better
> when executed in a subdir of the working tree.

Sure.

One thing I'd be worried about is that people may be using it to
compute paths and store them in $GIT_DIR/somewhere, and expecting
that they can later do a wholesale "mv" of directory hierarchy and
relative paths still work.  I wouldn't be one of those who does the
"mv" so I personally do not care, but I suspect some do.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Eric Sunshine
In reply to this post by SZEDER Gábor
On Fri, May 6, 2016 at 10:13 AM, SZEDER Gábor <[hidden email]> wrote:

> Quoting Mike Rappazzo <[hidden email]>:
>> I'll try to reword this to make it indicate that the value isn't
>> always incorrect.
>
> Not sure I understand your intention about rewording, in particular that
> "isn't always incorrect" part.  Just to make sure there is no
> misunderstanding let's have a look at the two broken cases:
>
>    $ git -C t/ rev-parse --git-common-dir
>    t/.git
>
> Obviously wrong.
> This is what you correctly described in the commit message as
> "incorrectly returns a path which starts 'sub/path/.git'.
>
>    $ git -C t/ rev-parse --git-path objects
>    .git/objects
>
> Wrong as well.  It would be correct if we were at the top of the working
> tree, but we are not.  It must be relative to the directory where '-C t/'
> brought us.
> Your description in the commit message implies that '--git-path <path>'
> is wrong in the same way as '--git-common-dir' is, i.e. in this case
> would also return the relative path starting with the subdirectory.
> That is apparently not the case.
>
> My point in the previous email was that both are wrong when executed in
> a subdir of the working tree, but they are wrong in different ways.  And
> they are always wrong when executed from a subdir of the working tree.

This explanation argues strongly in favor of placing each fix in a
separate patch[1] so that each commit message can be tailored to
precisely match the problem being fixed. Bundling the corresponding
tests with the fix would be a bonus; no need for a lead-in patch
introducing the tests en masse, and no need for test_expect_failure.

[1]: http://article.gmane.org/gmane.comp.version-control.git/293001/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html