git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* use of PWD
@ 2017-11-07 19:22 Joey Hess
  2017-11-08  7:53 ` Jeff King
  0 siblings, 1 reply; 6+ messages in thread
From: Joey Hess @ 2017-11-07 19:22 UTC (permalink / raw)
  To: Git Mailing List

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

In strbuf_add_absolute_path, git uses PWD if set when making relative
paths absolute, otherwise it falls back to getcwd(3). Using PWD may not
be a good idea. Here's one case where it confuses git badly:

joey@darkstar:/>sudo ln -s /media/hd/repo hd
joey@darkstar:/>cd /hd/repo
joey@darkstar:/hd/repo>git --git-dir=../../../home/joey/tmp/repo/.git cat-file -t HEAD
fatal: unable to normalize object directory: /hd/repo/../../../home/joey/tmp/repo/.git/objects
joey@darkstar:/hd/repo>ls -d ../../../home/joey/tmp/repo/.git
../../../home/joey/tmp/repo/.git/

In that situation where cd has followed a symlink to a different
depth, there seems to be no way to give git a relative path that works.
Other numbers of ../ also don't work.

What does work is to unset PWD:

joey@darkstar:/hd/repo>PWD= git --git-dir=../../../home/joey/tmp/repo/.git cat-file -t HEAD
commit

So why does git use PWD at all? Some shell code used pwd earlier
(leading to similar bugs like the one fixed in v1.5.1.5), but in
the C code, it was first introduced in commit
1b9a9467f8b9a8da2fe58d10ae16779492aa7737, which speaks of the "user's
view of the current directory", which is what PWD is. The use of PWD in
that commit may be ok.

Then in commit 10c4c881c4d2cb0ece0508e7142e189e68445257, 
the limited use of PWD broadened a lot, seemingly without
intending to look at the "user's view of the current directory"
anymore, due to reusing the code from the earlier commit.

-- 
see shy jo

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: use of PWD
  2017-11-07 19:22 use of PWD Joey Hess
@ 2017-11-08  7:53 ` Jeff King
  2017-11-11  2:13   ` Junio C Hamano
  0 siblings, 1 reply; 6+ messages in thread
From: Jeff King @ 2017-11-08  7:53 UTC (permalink / raw)
  To: Joey Hess; +Cc: Git Mailing List

On Tue, Nov 07, 2017 at 03:22:39PM -0400, Joey Hess wrote:

> In strbuf_add_absolute_path, git uses PWD if set when making relative
> paths absolute, otherwise it falls back to getcwd(3). Using PWD may not
> be a good idea. Here's one case where it confuses git badly:
> 
> joey@darkstar:/>sudo ln -s /media/hd/repo hd
> joey@darkstar:/>cd /hd/repo
> joey@darkstar:/hd/repo>git --git-dir=../../../home/joey/tmp/repo/.git cat-file -t HEAD
> fatal: unable to normalize object directory: /hd/repo/../../../home/joey/tmp/repo/.git/objects
> joey@darkstar:/hd/repo>ls -d ../../../home/joey/tmp/repo/.git
> ../../../home/joey/tmp/repo/.git/
> 
> In that situation where cd has followed a symlink to a different
> depth, there seems to be no way to give git a relative path that works.
> Other numbers of ../ also don't work.

I wondered if:

  git --git-dir=../../home/joey/tmp/repo.git

would work. But interestingly we _do_ resolve the relative git-dir using
the physical path, so that fails with "not a git repository". IOW,
there's no relative path that could possibly work.

Also interestingly, your case "worked" until my 670c359da3
(link_alt_odb_entry: handle normalize_path errors, 2016-10-03). But
that's only because we quietly generated a broken nonsense path, which
turned out not to matter because there are no alternates to link.

So totally orthogonal to your bug, I wonder if we ought to be doing:

diff --git a/sha1_file.c b/sha1_file.c
index 057262d46e..0b76233aa7 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -530,11 +530,11 @@ void prepare_alt_odb(void)
 	if (alt_odb_tail)
 		return;
 
-	alt = getenv(ALTERNATE_DB_ENVIRONMENT);
-	if (!alt) alt = "";
-
 	alt_odb_tail = &alt_odb_list;
-	link_alt_odb_entries(alt, strlen(alt), PATH_SEP, NULL, 0);
+
+	alt = getenv(ALTERNATE_DB_ENVIRONMENT);
+	if (alt)
+		link_alt_odb_entries(alt, strlen(alt), PATH_SEP, NULL, 0);
 
 	read_info_alternates(get_object_directory(), 0);
 }

to avoid hitting link_alt_odb_entries() at all when there are no
entries.

Anyway, back on topic.

> So why does git use PWD at all? Some shell code used pwd earlier
> (leading to similar bugs like the one fixed in v1.5.1.5), but in
> the C code, it was first introduced in commit
> 1b9a9467f8b9a8da2fe58d10ae16779492aa7737, which speaks of the "user's
> view of the current directory", which is what PWD is. The use of PWD in
> that commit may be ok.
> 
> Then in commit 10c4c881c4d2cb0ece0508e7142e189e68445257, 
> the limited use of PWD broadened a lot, seemingly without
> intending to look at the "user's view of the current directory"
> anymore, due to reusing the code from the earlier commit.

I had trouble finding anything definite in the list archive. I suspect
it was mostly about trying to make things look "nice" to the user in
messages, etc. But as you noticed, while it works some of the time, it
definitely doesn't always.

Interestingly, ripping it out and just using getcwd() causes t1305 to
fail. The test does:

  ln -s foo bar
  cd bar
  git config includeIf.gitdir:bar/.key value

and expects that condition to trigger.

That's somewhat convenient, but it's also slightly crazy that the config
might or might not trigger for the same repo depending on how you
happened to "cd" there.

This was added by 0624c63ce6 (config: match both symlink & realpath
versions in IncludeIf.gitdir:*, 2017-05-16) which makes it clear that
this behavior is very intentional.

Ideally we would instead be canonicalizing both the cwd and the
directory mentioned in the config file, and then comparing those
results.  But I suspect that may be tricky since what's in the config is
actually a globbing pattern. I guess you'd have to expand the glob and
then `real_path` the results.

Or we can leave the minor weirdness (which AFAIK nobody is complaining
about), and just let that matching code use its own custom
$PWD-respecting implementation. And change strbuf_add_absolute_path() to
do the more robust physical-path matching.

-Peff

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: use of PWD
  2017-11-08  7:53 ` Jeff King
@ 2017-11-11  2:13   ` Junio C Hamano
  2017-11-12 10:27     ` [PATCH] link_alt_odb_entries: make empty input a noop Jeff King
  0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2017-11-11  2:13 UTC (permalink / raw)
  To: Jeff King; +Cc: Joey Hess, Git Mailing List

Jeff King <peff@peff.net> writes:

> So totally orthogonal to your bug, I wonder if we ought to be doing:
>
> diff --git a/sha1_file.c b/sha1_file.c
> index 057262d46e..0b76233aa7 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -530,11 +530,11 @@ void prepare_alt_odb(void)
>  	if (alt_odb_tail)
>  		return;
>  
> -	alt = getenv(ALTERNATE_DB_ENVIRONMENT);
> -	if (!alt) alt = "";
> -
>  	alt_odb_tail = &alt_odb_list;
> -	link_alt_odb_entries(alt, strlen(alt), PATH_SEP, NULL, 0);
> +
> +	alt = getenv(ALTERNATE_DB_ENVIRONMENT);
> +	if (alt)
> +		link_alt_odb_entries(alt, strlen(alt), PATH_SEP, NULL, 0);
>  
>  	read_info_alternates(get_object_directory(), 0);
>  }
>
> to avoid hitting link_alt_odb_entries() at all when there are no
> entries.

Sounds sane.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH] link_alt_odb_entries: make empty input a noop
  2017-11-11  2:13   ` Junio C Hamano
@ 2017-11-12 10:27     ` Jeff King
  2017-11-13 17:11       ` Joey Hess
  0 siblings, 1 reply; 6+ messages in thread
From: Jeff King @ 2017-11-12 10:27 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Joey Hess, Git Mailing List

On Sat, Nov 11, 2017 at 11:13:21AM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > So totally orthogonal to your bug, I wonder if we ought to be doing:
> >
> > diff --git a/sha1_file.c b/sha1_file.c
> > index 057262d46e..0b76233aa7 100644
> > --- a/sha1_file.c
> > +++ b/sha1_file.c
> > @@ -530,11 +530,11 @@ void prepare_alt_odb(void)
> >  	if (alt_odb_tail)
> >  		return;
> >  
> > -	alt = getenv(ALTERNATE_DB_ENVIRONMENT);
> > -	if (!alt) alt = "";
> > -
> >  	alt_odb_tail = &alt_odb_list;
> > -	link_alt_odb_entries(alt, strlen(alt), PATH_SEP, NULL, 0);
> > +
> > +	alt = getenv(ALTERNATE_DB_ENVIRONMENT);
> > +	if (alt)
> > +		link_alt_odb_entries(alt, strlen(alt), PATH_SEP, NULL, 0);
> >  
> >  	read_info_alternates(get_object_directory(), 0);
> >  }
> >
> > to avoid hitting link_alt_odb_entries() at all when there are no
> > entries.
> 
> Sounds sane.

Here it is as a real patch. I actually bumped the check into the
function itself, since it keeps the logic all in one place. And as a
bonus, we save work if you truly have an empty environment variable or
info/alternates file, though I don't expect those are very common. :)

I also rebased on top of dc732bd5cb (read_info_alternates: read contents
into strbuf, 2017-09-19), which had a trivial textual conflict.

This should make Joey's immediate pain go away, though only by papering
it over. I tend to agree that we shouldn't be looking at $PWD at all
here.

-- >8 --
Subject: [PATCH] link_alt_odb_entries: make empty input a noop

If an empty string is passed to link_alt_odb_entries(), our
loop finds no entries and we link nothing. But we still do
some preparatory work to normalize the object directory
path, even though we'll never look at the result. This
triggers in basically every git process, since we feed the
usually-empty ALTERNATE_DB_ENVIRONMENT to the function.

Let's detect early that there's nothing to do and return.
While we're at it, let's treat NULL the same as an empty
string as a favor to our callers. That saves
prepare_alt_odb() from having to cover this case.

Signed-off-by: Jeff King <peff@peff.net>
---
 sha1_file.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sha1_file.c b/sha1_file.c
index d708981376..8a7c6b7eba 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -404,6 +404,9 @@ static void link_alt_odb_entries(const char *alt, int sep,
 	struct strbuf objdirbuf = STRBUF_INIT;
 	struct strbuf entry = STRBUF_INIT;
 
+	if (!alt || !*alt)
+		return;
+
 	if (depth > 5) {
 		error("%s: ignoring alternate object stores, nesting too deep.",
 				relative_base);
@@ -604,7 +607,6 @@ void prepare_alt_odb(void)
 		return;
 
 	alt = getenv(ALTERNATE_DB_ENVIRONMENT);
-	if (!alt) alt = "";
 
 	alt_odb_tail = &alt_odb_list;
 	link_alt_odb_entries(alt, PATH_SEP, NULL, 0);
-- 
2.15.0.413.g6cc52d366b


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] link_alt_odb_entries: make empty input a noop
  2017-11-12 10:27     ` [PATCH] link_alt_odb_entries: make empty input a noop Jeff King
@ 2017-11-13 17:11       ` Joey Hess
  2017-11-14  2:07         ` Junio C Hamano
  0 siblings, 1 reply; 6+ messages in thread
From: Joey Hess @ 2017-11-13 17:11 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, Git Mailing List

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

Jeff King wrote:
> This should make Joey's immediate pain go away, though only by papering
> it over. I tend to agree that we shouldn't be looking at $PWD at all
> here.

I've confirmed that Jeff's patch fixes the case I was having trouble with.

-- 
see shy jo

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] link_alt_odb_entries: make empty input a noop
  2017-11-13 17:11       ` Joey Hess
@ 2017-11-14  2:07         ` Junio C Hamano
  0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2017-11-14  2:07 UTC (permalink / raw)
  To: Joey Hess; +Cc: Jeff King, Git Mailing List

Joey Hess <id@joeyh.name> writes:

> Jeff King wrote:
>> This should make Joey's immediate pain go away, though only by papering
>> it over. I tend to agree that we shouldn't be looking at $PWD at all
>> here.
>
> I've confirmed that Jeff's patch fixes the case I was having trouble with.

Thanks, both.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2017-11-14  2:07 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-07 19:22 use of PWD Joey Hess
2017-11-08  7:53 ` Jeff King
2017-11-11  2:13   ` Junio C Hamano
2017-11-12 10:27     ` [PATCH] link_alt_odb_entries: make empty input a noop Jeff King
2017-11-13 17:11       ` Joey Hess
2017-11-14  2:07         ` Junio C Hamano

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

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

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git