git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "brian m. carlson" <sandals@crustytoothpaste.net>
To: "SZEDER Gábor" <szeder.dev@gmail.com>
Cc: git@vger.kernel.org, Johannes Schindelin <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH] rev-parse: add option for absolute or relative path formatting
Date: Wed, 9 Sep 2020 22:23:33 +0000	[thread overview]
Message-ID: <20200909222333.GH241078@camp.crustytoothpaste.net> (raw)
In-Reply-To: <20200909145114.GE6209@szeder.dev>

[-- Attachment #1: Type: text/plain, Size: 5388 bytes --]

On 2020-09-09 at 14:51:14, SZEDER Gábor wrote:
> On Tue, Sep 08, 2020 at 06:50:17PM +0000, brian m. carlson wrote:
> > diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
> > index 19b12b6d43..6b95292559 100644
> > --- a/Documentation/git-rev-parse.txt
> > +++ b/Documentation/git-rev-parse.txt
> > @@ -208,6 +208,15 @@ Options for Files
> >  	Only the names of the variables are listed, not their value,
> >  	even if they are set.
> >  
> > +--path-format=(absolute|relative)::
> > +	Controls the behavior of certain other options following it on the command
> > +	line.
> 
> Does it affect only one subsequent such option on the command line, or
> all such options?  IOW, while standing in the top directory of the
> worktree, would the following command
> 
>   git rev-parse --path-format=absolute --git-dir --git-path foo --show-toplevel
> 
> print three absolute paths, or one absolute paths and two relative
> paths?
> 
> The wording here is not clear on this, the commit message doesn't
> mention it, and the tests added in this patch only check one such
> option, but looking at the code and doing some testing of my own I
> found that it affects all subsequent such options.

It affects all subsequent options.  Moreover, I believe it's possible to
switch in the middle of the command line if you want some things
relative and some absolute.  That seemed to be both the easiest solution
and the most flexible, so I went with it.  I'll add more tests for this
case and improve the commit message.

> > diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
> > index 408b97d5af..6f3811d189 100755
> > --- a/t/t1500-rev-parse.sh
> > +++ b/t/t1500-rev-parse.sh
> > @@ -3,6 +3,16 @@
> >  test_description='test git rev-parse'
> >  . ./test-lib.sh
> >  
> > +test_one () {
> > +	dir="$1" &&
> > +	expect="$2" &&
> > +	shift &&
> > +	shift &&
> > +	echo "$expect" >expect &&
> > +	git -C "$dir" rev-parse "$@" >actual
> 
> Broken && chain.

Thanks, will fix.

> > +	test_cmp expect actual
> > +}
> > +
> >  # usage: [options] label is-bare is-inside-git is-inside-work prefix git-dir absolute-git-dir
> >  test_rev_parse () {
> >  	d=
> > @@ -60,7 +70,13 @@ ROOT=$(pwd)
> >  
> >  test_expect_success 'setup' '
> >  	mkdir -p sub/dir work &&
> > -	cp -R .git repo.git
> > +	cp -R .git repo.git &&
> > +	git checkout -b main &&
> > +	test_commit abc &&
> > +	git checkout -b side &&
> > +	test_commit def &&
> > +	git checkout main &&
> > +	git worktree add worktree side
> >  '
> >  
> >  test_rev_parse toplevel false false true '' .git "$ROOT/.git"
> > @@ -88,6 +104,24 @@ test_rev_parse -C work -g ../repo.git -b t 'GIT_DIR=../repo.git, core.bare = tru
> >  
> >  test_rev_parse -C work -g ../repo.git -b u 'GIT_DIR=../repo.git, core.bare undefined' false false true ''
> >  
> > +test_expect_success 'rev-parse --path-format=absolute' '
> > +	test_one "." "$ROOT/.git" --path-format=absolute --git-dir &&
> > +	test_one "." "$ROOT/.git" --path-format=absolute --git-common-dir &&
> > +	test_one "worktree" "$ROOT/.git/worktrees/worktree" --path-format=absolute --git-dir &&
> > +	test_one "worktree" "$ROOT/.git" --path-format=absolute --git-common-dir &&
> > +	test_one "." "$ROOT" --path-format=absolute --show-toplevel &&
> > +	test_one "." "$ROOT/.git/objects" --path-format=absolute --git-path objects
> > +'
> > +
> > +test_expect_success 'rev-parse --path-format=relative' '
> > +	test_one "." ".git" --path-format=relative --git-dir &&
> > +	test_one "." ".git" --path-format=relative --git-common-dir &&
> > +	test_one "worktree" "../.git/worktrees/worktree" --path-format=relative --git-dir &&
> > +	test_one "worktree" "../.git" --path-format=relative --git-common-dir &&
> > +	test_one "." "./" --path-format=relative --show-toplevel &&
> > +	test_one "." ".git/objects" --path-format=relative --git-path objects
> > +'
> 
> I would like to see a test that demonstrates that '--absolute-git-dir'
> is unaffected by '--path-format=relative', just to be sure.
> 
> There are some cases where this new option doesn't do what I would
> expect:
> 
>   $ ./git -C Documentation/ rev-parse --git-dir --show-toplevel
>   /home/szeder/src/git/.git
>   /home/szeder/src/git
>   $ ./git -C Documentation/ rev-parse --path-format=absolute --git-dir --show-toplevel
>   /home/szeder/src/git/.git
>   /home/szeder/src/git
>   # So far so good, but:
>   $ ./git -C Documentation/ rev-parse --path-format=relative --git-dir --show-toplevel
>   /home/szeder/src/git/.git
>   /home/szeder/src/git
> 
> 
>   $ ls -l .git/foo
>   ls: cannot access '.git/foo': No such file or directory
>   $ ./git rev-parse --git-path foo
>   .git/foo
>   $ ./git rev-parse --path-format=relative --git-path foo
>   .git/foo
>   $ ./git rev-parse --path-format=absolute --git-path foo
>   /home/szeder/src/git/.git/foo
>   $ ./git rev-parse --git-path foo/bar
>   .git/foo/bar
>   $ ./git rev-parse --path-format=relative --git-path foo/bar
>   .git/foo/bar
>   # So far so good, but:
>   $ ./git rev-parse --path-format=absolute --git-path foo/bar
>   fatal: Invalid path '/home/szeder/src/git/.git/foo': No such file or directory

That's going to be a little tricky to fix, but I'll look into it.
-- 
brian m. carlson: Houston, Texas, US

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

  reply	other threads:[~2020-09-09 22:23 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-08 18:50 [PATCH] rev-parse: add option for absolute or relative path formatting brian m. carlson
2020-09-09  3:23 ` Johannes Schindelin
2020-09-09 22:31   ` brian m. carlson
2020-09-09 14:51 ` SZEDER Gábor
2020-09-09 22:23   ` brian m. carlson [this message]
2020-09-10 15:19     ` SZEDER Gábor
2020-09-11  0:03       ` brian m. carlson
2020-11-04 22:16 ` Jonathan Nieder
2020-11-05  3:11   ` brian m. carlson
2020-11-06  0:51     ` Jonathan Nieder
2020-11-06  1:57       ` brian m. carlson

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=20200909222333.GH241078@camp.crustytoothpaste.net \
    --to=sandals@crustytoothpaste.net \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=szeder.dev@gmail.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).