{"thread":{"id":"14029","subject":"[PATCH v2] Make git_dir a path relative to work_tree in setup_work_tree()","startedAt":"2008-06-19T03:28:27Z","lastAt":"2008-06-19T19:34:06Z","messageCount":7,"participants":["Daniel Barkalow","Johannes Schindelin","Junio C Hamano","Linus Torvalds"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"80285","messageId":"alpine.LNX.1.00.0806182327090.19665@iabervon.org","threadId":"14029","inReplyTo":null,"subject":"[PATCH v2] Make git_dir a path relative to work_tree in setup_work_tree()","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-06-19T03:28:27Z","receivedAt":"2008-06-19T03:28:27Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"Once we find the absolute paths for git_dir and work_tree, we can make\ngit_dir a relative path since we know pwd will be work_tree. This should\nsave the kernel some time traversing the path to work_tree all the time\nif git_dir is inside work_tree.\n\nSigned-off-by: Daniel Barkalow <barkalow@iabervon.org>\n---\nNot only was work_tree potentially NULL, it was also already a copied \ncanonical path. So this simpler patch should be better.\n\n cache.h |    1 +\n path.c  |   17 +++++++++++++++++\n setup.c |    3 ++-\n 3 files changed, 20 insertions(+), 1 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex eab1a17..3465c94 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -524,6 +524,7 @@ static inline int is_absolute_path(const char *path)\n \treturn path[0] == '/';\n }\n const char *make_absolute_path(const char *path);\n+const char *make_relative_path(const char *abs, const char *base);\n \n /* Read and unpack a sha1 file into memory, write memory to a sha1 file */\n extern int sha1_object_info(const unsigned char *, unsigned long *);\ndiff --git a/path.c b/path.c\nindex b7c24a2..790d8d4 100644\n--- a/path.c\n+++ b/path.c\n@@ -294,6 +294,23 @@ int adjust_shared_perm(const char *path)\n /* We allow \"recursive\" symbolic links. Only within reason, though. */\n #define MAXDEPTH 5\n \n+const char *make_relative_path(const char *abs, const char *base)\n+{\n+\tstatic char buf[PATH_MAX + 1];\n+\tint baselen;\n+\tif (!base)\n+\t\treturn abs;\n+\tbaselen = strlen(base);\n+\tif (prefixcmp(abs, base))\n+\t\treturn abs;\n+\tif (abs[baselen] == '/')\n+\t\tbaselen++;\n+\telse if (base[baselen - 1] != '/')\n+\t\treturn abs;\n+\tstrcpy(buf, abs + baselen);\n+\treturn buf;\n+}\n+\n const char *make_absolute_path(const char *path)\n {\n \tstatic char bufs[2][PATH_MAX + 1], *buf = bufs[0], *next_buf = bufs[1];\ndiff --git a/setup.c b/setup.c\nindex d630e37..1643ee4 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -292,7 +292,8 @@ void setup_work_tree(void)\n \twork_tree = get_git_work_tree();\n \tgit_dir = get_git_dir();\n \tif (!is_absolute_path(git_dir))\n-\t\tset_git_dir(make_absolute_path(git_dir));\n+\t\tset_git_dir(make_relative_path(make_absolute_path(git_dir),\n+\t\t\t\t\t       work_tree));\n \tif (!work_tree || chdir(work_tree))\n \t\tdie(\"This operation must be run in a work tree\");\n \tinitialized = 1;\n-- \n1.5.4.5\n"},{"id":"80317","messageId":"alpine.DEB.1.00.0806191510060.6439@racer","threadId":"14029","inReplyTo":"alpine.LNX.1.00.0806182327090.19665@iabervon.org","subject":"Re: [PATCH v2] Make git_dir a path relative to work_tree in setup_work_tree()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-06-19T14:11:11Z","receivedAt":"2008-06-19T14:11:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 18 Jun 2008, Daniel Barkalow wrote:\n\n> diff --git a/setup.c b/setup.c\n> index d630e37..1643ee4 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -292,7 +292,8 @@ void setup_work_tree(void)\n>  \twork_tree = get_git_work_tree();\n>  \tgit_dir = get_git_dir();\n>  \tif (!is_absolute_path(git_dir))\n\nI suspect it needs \"work_tree &&\" here.\n\n> -\t\tset_git_dir(make_absolute_path(git_dir));\n> +\t\tset_git_dir(make_relative_path(make_absolute_path(git_dir),\n> +\t\t\t\t\t       work_tree));\n>  \tif (!work_tree || chdir(work_tree))\n>  \t\tdie(\"This operation must be run in a work tree\");\n>  \tinitialized = 1;\n\nAll in all I am pretty surprised how easy it was.  I tried yesterday, for \nhalf an hour, to come up with something sensible, and failed.\n\nThanks,\nDscho\n"},{"id":"80327","messageId":"alpine.LNX.1.00.0806191146210.19665@iabervon.org","threadId":"14029","inReplyTo":"alpine.DEB.1.00.0806191510060.6439@racer","subject":"Re: [PATCH v2] Make git_dir a path relative to work_tree in setup_work_tree()","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-06-19T16:12:19Z","receivedAt":"2008-06-19T16:12:19Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Thu, 19 Jun 2008, Johannes Schindelin wrote:\n\n> Hi,\n> \n> On Wed, 18 Jun 2008, Daniel Barkalow wrote:\n> \n> > diff --git a/setup.c b/setup.c\n> > index d630e37..1643ee4 100644\n> > --- a/setup.c\n> > +++ b/setup.c\n> > @@ -292,7 +292,8 @@ void setup_work_tree(void)\n> >  \twork_tree = get_git_work_tree();\n> >  \tgit_dir = get_git_dir();\n> >  \tif (!is_absolute_path(git_dir))\n> \n> I suspect it needs \"work_tree &&\" here.\n\nI'm not clear on the semantics of !get_git_work_tree(); is a non-absolute \npath for git_dir right then?\n\n> > -\t\tset_git_dir(make_absolute_path(git_dir));\n> > +\t\tset_git_dir(make_relative_path(make_absolute_path(git_dir),\n> > +\t\t\t\t\t       work_tree));\n> >  \tif (!work_tree || chdir(work_tree))\n> >  \t\tdie(\"This operation must be run in a work tree\");\n> >  \tinitialized = 1;\n> \n> All in all I am pretty surprised how easy it was.  I tried yesterday, for \n> half an hour, to come up with something sensible, and failed.\n\nI was sure you'd come up with just this solution, because you'd just \nrecently explained that make_absolute_path() means you can find when one \npath is in another path with a simple string compare. And, since we know \nwhat pwd is going to be...\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"80333","messageId":"alpine.DEB.1.00.0806191807120.6439@racer","threadId":"14029","inReplyTo":"alpine.LNX.1.00.0806191146210.19665@iabervon.org","subject":"Re: [PATCH v2] Make git_dir a path relative to work_tree in setup_work_tree()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-06-19T17:09:04Z","receivedAt":"2008-06-19T17:09:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 19 Jun 2008, Daniel Barkalow wrote:\n\n> On Thu, 19 Jun 2008, Johannes Schindelin wrote:\n> \n> > On Wed, 18 Jun 2008, Daniel Barkalow wrote:\n> > \n> > > diff --git a/setup.c b/setup.c\n> > > index d630e37..1643ee4 100644\n> > > --- a/setup.c\n> > > +++ b/setup.c\n> > > @@ -292,7 +292,8 @@ void setup_work_tree(void)\n> > >  \twork_tree = get_git_work_tree();\n> > >  \tgit_dir = get_git_dir();\n> > >  \tif (!is_absolute_path(git_dir))\n> > \n> > I suspect it needs \"work_tree &&\" here.\n> \n> I'm not clear on the semantics of !get_git_work_tree(); is a non-absolute \n> path for git_dir right then?\n\nMy reading was: if there is no work_tree, then a relative git_dir is just \nfine, since we are quite unlikely to jump around in the file system.\n\nAnd your implementation of make_relative_path() is nice enough to a \n(work_tree ==) base == NULL, but would return the absolute path in that \ncase.\n\nHaven't had time to test anything, though.\n\nCiao,\nDscho\n"},{"id":"80347","messageId":"7vod5xuvtj.fsf@gitster.siamese.dyndns.org","threadId":"14029","inReplyTo":"alpine.LNX.1.00.0806182327090.19665@iabervon.org","subject":"Re: [PATCH v2] Make git_dir a path relative to work_tree in setup_work_tree()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-19T18:24:40Z","receivedAt":"2008-06-19T18:24:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow <barkalow@iabervon.org> writes:\n\n> diff --git a/path.c b/path.c\n> index b7c24a2..790d8d4 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -294,6 +294,23 @@ int adjust_shared_perm(const char *path)\n>  /* We allow \"recursive\" symbolic links. Only within reason, though. */\n>  #define MAXDEPTH 5\n>  \n> +const char *make_relative_path(const char *abs, const char *base)\n> +{\n> +\tstatic char buf[PATH_MAX + 1];\n> +\tint baselen;\n> +\tif (!base)\n> +\t\treturn abs;\n\nThis special case may help the specific caller you have below, but doesn't\nit make the function do more than it advertises with its name?\n\nOther than that, I think the change is Ok, but as a \"performance tweak\",\nit should be backed by some numbers, please?\n\n> +\tbaselen = strlen(base);\n> +\tif (prefixcmp(abs, base))\n> +\t\treturn abs;\n> +\tif (abs[baselen] == '/')\n> +\t\tbaselen++;\n> +\telse if (base[baselen - 1] != '/')\n> +\t\treturn abs;\n> +\tstrcpy(buf, abs + baselen);\n> +\treturn buf;\n> +}\n> +\n>  const char *make_absolute_path(const char *path)\n>  {\n>  \tstatic char bufs[2][PATH_MAX + 1], *buf = bufs[0], *next_buf = bufs[1];\n> diff --git a/setup.c b/setup.c\n> index d630e37..1643ee4 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -292,7 +292,8 @@ void setup_work_tree(void)\n>  \twork_tree = get_git_work_tree();\n>  \tgit_dir = get_git_dir();\n>  \tif (!is_absolute_path(git_dir))\n> -\t\tset_git_dir(make_absolute_path(git_dir));\n> +\t\tset_git_dir(make_relative_path(make_absolute_path(git_dir),\n> +\t\t\t\t\t       work_tree));\n>  \tif (!work_tree || chdir(work_tree))\n>  \t\tdie(\"This operation must be run in a work tree\");\n>  \tinitialized = 1;\n> -- \n> 1.5.4.5\n"},{"id":"80350","messageId":"alpine.LNX.1.00.0806191427000.19665@iabervon.org","threadId":"14029","inReplyTo":"7vod5xuvtj.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2] Make git_dir a path relative to work_tree in setup_work_tree()","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-06-19T18:44:11Z","receivedAt":"2008-06-19T18:44:11Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Thu, 19 Jun 2008, Junio C Hamano wrote:\n\n> Daniel Barkalow <barkalow@iabervon.org> writes:\n> \n> > diff --git a/path.c b/path.c\n> > index b7c24a2..790d8d4 100644\n> > --- a/path.c\n> > +++ b/path.c\n> > @@ -294,6 +294,23 @@ int adjust_shared_perm(const char *path)\n> >  /* We allow \"recursive\" symbolic links. Only within reason, though. */\n> >  #define MAXDEPTH 5\n> >  \n> > +const char *make_relative_path(const char *abs, const char *base)\n> > +{\n> > +\tstatic char buf[PATH_MAX + 1];\n> > +\tint baselen;\n> > +\tif (!base)\n> > +\t\treturn abs;\n> \n> This special case may help the specific caller you have below, but doesn't\n> it make the function do more than it advertises with its name?\n\nI don't think so; the best path relative to nothing is the absolute path. \nThe idea is to return the easiest equivalent path if your pwd is known to \nbe \"base\" and you give it an absolute path. If you don't know what your \npwd is, the easiest equivalent path is the absolute path with no symlinks. \nSimilarly, you get an absolute path if the path you give it isn't inside \nbase. Maybe \"make_brief_path\" would be a better name?\n\n> Other than that, I think the change is Ok, but as a \"performance tweak\",\n> it should be backed by some numbers, please?\n\nI was hoping Linus would provide some, since he'd noticed the slowness in \nthe first place. I'm not sure I have the RAM to have the kernel spend \nnon-trivial time looking up path elements in an all-cached tree. I'll try \nto replicate Linus's test case when I get a chance if he hasn't said \nanything.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"80359","messageId":"alpine.LFD.1.10.0806191206350.2907@woody.linux-foundation.org","threadId":"14029","inReplyTo":"7vod5xuvtj.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2] Make git_dir a path relative to work_tree in setup_work_tree()","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-06-19T19:34:06Z","receivedAt":"2008-06-19T19:34:06Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 19 Jun 2008, Junio C Hamano wrote:\n> \n> Other than that, I think the change is Ok, but as a \"performance tweak\",\n> it should be backed by some numbers, please?\n\nDaniel's patch didn't apply for me as-is, so I recreated it with some \ndifferences (appended), and here are the numbers from ten runs each.\n\nThere is some IO for me - probably due to more-or-less random flushing of \nthe journal - so the variation is bigger than I'd like, but whatever:\n\n\tBefore:\n\t\treal    0m8.135s\n\t\treal    0m7.933s\n\t\treal    0m8.080s\n\t\treal    0m7.954s\n\t\treal    0m7.949s\n\t\treal    0m8.112s\n\t\treal    0m7.934s\n\t\treal    0m8.059s\n\t\treal    0m7.979s\n\t\treal    0m8.038s\n\n\n\tAfter:\n\t\treal    0m7.685s\n\t\treal    0m7.968s\n\t\treal    0m7.703s\n\t\treal    0m7.850s\n\t\treal    0m7.995s\n\t\treal    0m7.817s\n\t\treal    0m7.963s\n\t\treal    0m7.955s\n\t\treal    0m7.848s\n\t\treal    0m7.969s\n\nNow, going by \"best of ten\" (on the assumption that the longer numbers \nare all due to IO), I'm saying a 7.933s -> 7.685s reduction, and it does \nseem to be outside of the noise (ie the \"after\" case never broke 8s, while \nthe \"before\" case did so half the time).\n\nSo looks like about 3% to me. \n\nDoing it for a slightly smaller test-case (just the \"arch\" subdirectory) \ngets more stable numbers probably due to not filling the journal with \nmetadata updates, so we have:\n\n\tBefore:\n\t\treal    0m1.633s\n\t\treal    0m1.633s\n\t\treal    0m1.633s\n\t\treal    0m1.632s\n\t\treal    0m1.632s\n\t\treal    0m1.630s\n\t\treal    0m1.634s\n\t\treal    0m1.631s\n\t\treal    0m1.632s\n\t\treal    0m1.632s\n\n\tAfter:\n\t\treal    0m1.610s\n\t\treal    0m1.609s\n\t\treal    0m1.610s\n\t\treal    0m1.608s\n\t\treal    0m1.607s\n\t\treal    0m1.610s\n\t\treal    0m1.609s\n\t\treal    0m1.611s\n\t\treal    0m1.608s\n\t\treal    0m1.611s\n\nwhere I'ld just take the averages and say 1.632 vs 1.610, which is just \nover 1% peformance improvement.\n\nSo it's not in the noise, but it's not as big as I initially thought and \nmeasured.\n\n(That said, it obviously depends on how deep the working directory path is \ntoo, and whether it is behind NFS or something else that might need to \ncause more work to look up).\n\nModified/updated patch appended.\n\n\t\t\tLinus\n\n----\n cache.h |    1 +\n path.c  |   17 +++++++++++++++++\n setup.c |    3 ++-\n 3 files changed, 20 insertions(+), 1 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 01c8502..29aae0c 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -526,6 +526,7 @@ static inline int is_absolute_path(const char *path)\n }\n const char *make_absolute_path(const char *path);\n const char *make_nonrelative_path(const char *path);\n+const char *make_relative_path(const char *abs, const char *base);\n \n /* Read and unpack a sha1 file into memory, write memory to a sha1 file */\n extern int sha1_object_info(const unsigned char *, unsigned long *);\ndiff --git a/path.c b/path.c\nindex 7a35a26..6e3df18 100644\n--- a/path.c\n+++ b/path.c\n@@ -330,6 +330,23 @@ const char *make_nonrelative_path(const char *path)\n /* We allow \"recursive\" symbolic links. Only within reason, though. */\n #define MAXDEPTH 5\n \n+const char *make_relative_path(const char *abs, const char *base)\n+{\n+\tstatic char buf[PATH_MAX + 1];\n+\tint baselen;\n+\tif (!base)\n+\t\treturn abs;\n+\tbaselen = strlen(base);\n+\tif (prefixcmp(abs, base))\n+\t\treturn abs;\n+\tif (abs[baselen] == '/')\n+\t\tbaselen++;\n+\telse if (base[baselen - 1] != '/')\n+\t\treturn abs;\n+\tstrcpy(buf, abs + baselen);\n+\treturn buf;\n+}\n+\n const char *make_absolute_path(const char *path)\n {\n \tstatic char bufs[2][PATH_MAX + 1], *buf = bufs[0], *next_buf = bufs[1];\ndiff --git a/setup.c b/setup.c\nindex d630e37..3b111ea 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -292,9 +292,10 @@ void setup_work_tree(void)\n \twork_tree = get_git_work_tree();\n \tgit_dir = get_git_dir();\n \tif (!is_absolute_path(git_dir))\n-\t\tset_git_dir(make_absolute_path(git_dir));\n+\t\tgit_dir = make_absolute_path(git_dir);\n \tif (!work_tree || chdir(work_tree))\n \t\tdie(\"This operation must be run in a work tree\");\n+\tset_git_dir(make_relative_path(git_dir, work_tree));\n \tinitialized = 1;\n }\n \n"}]}