{"thread":{"id":"47131","subject":"use of PWD","startedAt":"2017-11-07T19:32:23Z","lastAt":"2017-11-14T02:07:46Z","messageCount":6,"participants":["Joey Hess","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"332023","messageId":"20171107192239.6hinu235hfpwqpv6@kitenet.net","threadId":"47131","inReplyTo":null,"subject":"use of PWD","fromName":"Joey Hess","fromEmail":"id@joeyh.name","sentAt":"2017-11-07T19:22:39Z","receivedAt":"2017-11-07T19:32:23Z","isPatch":false,"sender":{"key":"id@joeyh.name","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"In strbuf_add_absolute_path, git uses PWD if set when making relative\npaths absolute, otherwise it falls back to getcwd(3). Using PWD may not\nbe a good idea. Here's one case where it confuses git badly:\n\njoey@darkstar:/>sudo ln -s /media/hd/repo hd\njoey@darkstar:/>cd /hd/repo\njoey@darkstar:/hd/repo>git --git-dir=../../../home/joey/tmp/repo/.git cat-file -t HEAD\nfatal: unable to normalize object directory: /hd/repo/../../../home/joey/tmp/repo/.git/objects\njoey@darkstar:/hd/repo>ls -d ../../../home/joey/tmp/repo/.git\n../../../home/joey/tmp/repo/.git/\n\nIn that situation where cd has followed a symlink to a different\ndepth, there seems to be no way to give git a relative path that works.\nOther numbers of ../ also don't work.\n\nWhat does work is to unset PWD:\n\njoey@darkstar:/hd/repo>PWD= git --git-dir=../../../home/joey/tmp/repo/.git cat-file -t HEAD\ncommit\n\nSo why does git use PWD at all? Some shell code used pwd earlier\n(leading to similar bugs like the one fixed in v1.5.1.5), but in\nthe C code, it was first introduced in commit\n1b9a9467f8b9a8da2fe58d10ae16779492aa7737, which speaks of the \"user's\nview of the current directory\", which is what PWD is. The use of PWD in\nthat commit may be ok.\n\nThen in commit 10c4c881c4d2cb0ece0508e7142e189e68445257, \nthe limited use of PWD broadened a lot, seemingly without\nintending to look at the \"user's view of the current directory\"\nanymore, due to reusing the code from the earlier commit.\n\n-- \nsee shy jo\n"},{"id":"332068","messageId":"20171108075336.is4awgyw53dohf7y@sigill.intra.peff.net","threadId":"47131","inReplyTo":"20171107192239.6hinu235hfpwqpv6@kitenet.net","subject":"Re: use of PWD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-08T07:53:36Z","receivedAt":"2017-11-08T07:53:44Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 07, 2017 at 03:22:39PM -0400, Joey Hess wrote:\n\n> In strbuf_add_absolute_path, git uses PWD if set when making relative\n> paths absolute, otherwise it falls back to getcwd(3). Using PWD may not\n> be a good idea. Here's one case where it confuses git badly:\n> \n> joey@darkstar:/>sudo ln -s /media/hd/repo hd\n> joey@darkstar:/>cd /hd/repo\n> joey@darkstar:/hd/repo>git --git-dir=../../../home/joey/tmp/repo/.git cat-file -t HEAD\n> fatal: unable to normalize object directory: /hd/repo/../../../home/joey/tmp/repo/.git/objects\n> joey@darkstar:/hd/repo>ls -d ../../../home/joey/tmp/repo/.git\n> ../../../home/joey/tmp/repo/.git/\n> \n> In that situation where cd has followed a symlink to a different\n> depth, there seems to be no way to give git a relative path that works.\n> Other numbers of ../ also don't work.\n\nI wondered if:\n\n  git --git-dir=../../home/joey/tmp/repo.git\n\nwould work. But interestingly we _do_ resolve the relative git-dir using\nthe physical path, so that fails with \"not a git repository\". IOW,\nthere's no relative path that could possibly work.\n\nAlso interestingly, your case \"worked\" until my 670c359da3\n(link_alt_odb_entry: handle normalize_path errors, 2016-10-03). But\nthat's only because we quietly generated a broken nonsense path, which\nturned out not to matter because there are no alternates to link.\n\nSo totally orthogonal to your bug, I wonder if we ought to be doing:\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 057262d46e..0b76233aa7 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -530,11 +530,11 @@ void prepare_alt_odb(void)\n \tif (alt_odb_tail)\n \t\treturn;\n \n-\talt = getenv(ALTERNATE_DB_ENVIRONMENT);\n-\tif (!alt) alt = \"\";\n-\n \talt_odb_tail = &alt_odb_list;\n-\tlink_alt_odb_entries(alt, strlen(alt), PATH_SEP, NULL, 0);\n+\n+\talt = getenv(ALTERNATE_DB_ENVIRONMENT);\n+\tif (alt)\n+\t\tlink_alt_odb_entries(alt, strlen(alt), PATH_SEP, NULL, 0);\n \n \tread_info_alternates(get_object_directory(), 0);\n }\n\nto avoid hitting link_alt_odb_entries() at all when there are no\nentries.\n\nAnyway, back on topic.\n\n> So why does git use PWD at all? Some shell code used pwd earlier\n> (leading to similar bugs like the one fixed in v1.5.1.5), but in\n> the C code, it was first introduced in commit\n> 1b9a9467f8b9a8da2fe58d10ae16779492aa7737, which speaks of the \"user's\n> view of the current directory\", which is what PWD is. The use of PWD in\n> that commit may be ok.\n> \n> Then in commit 10c4c881c4d2cb0ece0508e7142e189e68445257, \n> the limited use of PWD broadened a lot, seemingly without\n> intending to look at the \"user's view of the current directory\"\n> anymore, due to reusing the code from the earlier commit.\n\nI had trouble finding anything definite in the list archive. I suspect\nit was mostly about trying to make things look \"nice\" to the user in\nmessages, etc. But as you noticed, while it works some of the time, it\ndefinitely doesn't always.\n\nInterestingly, ripping it out and just using getcwd() causes t1305 to\nfail. The test does:\n\n  ln -s foo bar\n  cd bar\n  git config includeIf.gitdir:bar/.key value\n\nand expects that condition to trigger.\n\nThat's somewhat convenient, but it's also slightly crazy that the config\nmight or might not trigger for the same repo depending on how you\nhappened to \"cd\" there.\n\nThis was added by 0624c63ce6 (config: match both symlink & realpath\nversions in IncludeIf.gitdir:*, 2017-05-16) which makes it clear that\nthis behavior is very intentional.\n\nIdeally we would instead be canonicalizing both the cwd and the\ndirectory mentioned in the config file, and then comparing those\nresults.  But I suspect that may be tricky since what's in the config is\nactually a globbing pattern. I guess you'd have to expand the glob and\nthen `real_path` the results.\n\nOr we can leave the minor weirdness (which AFAIK nobody is complaining\nabout), and just let that matching code use its own custom\n$PWD-respecting implementation. And change strbuf_add_absolute_path() to\ndo the more robust physical-path matching.\n\n-Peff\n"},{"id":"332274","messageId":"xmqqd14pef5q.fsf@gitster.mtv.corp.google.com","threadId":"47131","inReplyTo":"20171108075336.is4awgyw53dohf7y@sigill.intra.peff.net","subject":"Re: use of PWD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-11T02:13:21Z","receivedAt":"2017-11-11T02:13:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> So totally orthogonal to your bug, I wonder if we ought to be doing:\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 057262d46e..0b76233aa7 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -530,11 +530,11 @@ void prepare_alt_odb(void)\n>  \tif (alt_odb_tail)\n>  \t\treturn;\n>  \n> -\talt = getenv(ALTERNATE_DB_ENVIRONMENT);\n> -\tif (!alt) alt = \"\";\n> -\n>  \talt_odb_tail = &alt_odb_list;\n> -\tlink_alt_odb_entries(alt, strlen(alt), PATH_SEP, NULL, 0);\n> +\n> +\talt = getenv(ALTERNATE_DB_ENVIRONMENT);\n> +\tif (alt)\n> +\t\tlink_alt_odb_entries(alt, strlen(alt), PATH_SEP, NULL, 0);\n>  \n>  \tread_info_alternates(get_object_directory(), 0);\n>  }\n>\n> to avoid hitting link_alt_odb_entries() at all when there are no\n> entries.\n\nSounds sane.\n"},{"id":"332318","messageId":"20171112102739.6xtnnsmtabhnhrm5@sigill.intra.peff.net","threadId":"47131","inReplyTo":"xmqqd14pef5q.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] link_alt_odb_entries: make empty input a noop","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-12T10:27:39Z","receivedAt":"2017-11-12T10:27:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 11, 2017 at 11:13:21AM +0900, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > So totally orthogonal to your bug, I wonder if we ought to be doing:\n> >\n> > diff --git a/sha1_file.c b/sha1_file.c\n> > index 057262d46e..0b76233aa7 100644\n> > --- a/sha1_file.c\n> > +++ b/sha1_file.c\n> > @@ -530,11 +530,11 @@ void prepare_alt_odb(void)\n> >  \tif (alt_odb_tail)\n> >  \t\treturn;\n> >  \n> > -\talt = getenv(ALTERNATE_DB_ENVIRONMENT);\n> > -\tif (!alt) alt = \"\";\n> > -\n> >  \talt_odb_tail = &alt_odb_list;\n> > -\tlink_alt_odb_entries(alt, strlen(alt), PATH_SEP, NULL, 0);\n> > +\n> > +\talt = getenv(ALTERNATE_DB_ENVIRONMENT);\n> > +\tif (alt)\n> > +\t\tlink_alt_odb_entries(alt, strlen(alt), PATH_SEP, NULL, 0);\n> >  \n> >  \tread_info_alternates(get_object_directory(), 0);\n> >  }\n> >\n> > to avoid hitting link_alt_odb_entries() at all when there are no\n> > entries.\n> \n> Sounds sane.\n\nHere it is as a real patch. I actually bumped the check into the\nfunction itself, since it keeps the logic all in one place. And as a\nbonus, we save work if you truly have an empty environment variable or\ninfo/alternates file, though I don't expect those are very common. :)\n\nI also rebased on top of dc732bd5cb (read_info_alternates: read contents\ninto strbuf, 2017-09-19), which had a trivial textual conflict.\n\nThis should make Joey's immediate pain go away, though only by papering\nit over. I tend to agree that we shouldn't be looking at $PWD at all\nhere.\n\n-- >8 --\nSubject: [PATCH] link_alt_odb_entries: make empty input a noop\n\nIf an empty string is passed to link_alt_odb_entries(), our\nloop finds no entries and we link nothing. But we still do\nsome preparatory work to normalize the object directory\npath, even though we'll never look at the result. This\ntriggers in basically every git process, since we feed the\nusually-empty ALTERNATE_DB_ENVIRONMENT to the function.\n\nLet's detect early that there's nothing to do and return.\nWhile we're at it, let's treat NULL the same as an empty\nstring as a favor to our callers. That saves\nprepare_alt_odb() from having to cover this case.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n sha1_file.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex d708981376..8a7c6b7eba 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -404,6 +404,9 @@ static void link_alt_odb_entries(const char *alt, int sep,\n \tstruct strbuf objdirbuf = STRBUF_INIT;\n \tstruct strbuf entry = STRBUF_INIT;\n \n+\tif (!alt || !*alt)\n+\t\treturn;\n+\n \tif (depth > 5) {\n \t\terror(\"%s: ignoring alternate object stores, nesting too deep.\",\n \t\t\t\trelative_base);\n@@ -604,7 +607,6 @@ void prepare_alt_odb(void)\n \t\treturn;\n \n \talt = getenv(ALTERNATE_DB_ENVIRONMENT);\n-\tif (!alt) alt = \"\";\n \n \talt_odb_tail = &alt_odb_list;\n \tlink_alt_odb_entries(alt, PATH_SEP, NULL, 0);\n-- \n2.15.0.413.g6cc52d366b\n\n"},{"id":"332422","messageId":"20171113171119.fjhufmbbuidr35ud@kitenet.net","threadId":"47131","inReplyTo":"20171112102739.6xtnnsmtabhnhrm5@sigill.intra.peff.net","subject":"Re: [PATCH] link_alt_odb_entries: make empty input a noop","fromName":"Joey Hess","fromEmail":"id@joeyh.name","sentAt":"2017-11-13T17:11:19Z","receivedAt":"2017-11-13T17:11:34Z","isPatch":true,"sender":{"key":"id@joeyh.name","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"Jeff King wrote:\n> This should make Joey's immediate pain go away, though only by papering\n> it over. I tend to agree that we shouldn't be looking at $PWD at all\n> here.\n\nI've confirmed that Jeff's patch fixes the case I was having trouble with.\n\n-- \nsee shy jo\n"},{"id":"332492","messageId":"xmqqined7gut.fsf@gitster.mtv.corp.google.com","threadId":"47131","inReplyTo":"20171113171119.fjhufmbbuidr35ud@kitenet.net","subject":"Re: [PATCH] link_alt_odb_entries: make empty input a noop","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-14T02:07:38Z","receivedAt":"2017-11-14T02:07:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joey Hess <id@joeyh.name> writes:\n\n> Jeff King wrote:\n>> This should make Joey's immediate pain go away, though only by papering\n>> it over. I tend to agree that we shouldn't be looking at $PWD at all\n>> here.\n>\n> I've confirmed that Jeff's patch fixes the case I was having trouble with.\n\nThanks, both.\n"}]}