{"thread":{"id":"33104","subject":"[BUG] bare repository detection does not work with aliases","startedAt":"2013-03-07T22:47:45Z","lastAt":"2013-03-08T23:03:28Z","messageCount":16,"participants":["Mark Lodato","Jeff King","Junio C Hamano","Johannes Sixt","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"210795","messageId":"CAHREChhuX82ibNEDQnQUeS9TEeyMFGpuNhyXzt1Pn-Tt2BVOQA@mail.gmail.com","threadId":"33104","inReplyTo":null,"subject":"[BUG] bare repository detection does not work with aliases","fromName":"Mark Lodato","fromEmail":"lodatom@gmail.com","sentAt":"2013-03-07T22:47:45Z","receivedAt":"2013-03-07T22:47:45Z","isPatch":false,"sender":{"key":"lodatom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58860?v=4"},"body":"It seems that the fallback bare repository detection in the absence of\ncore.bare fails for aliases.\n\n$ git init --bare foo\n$ cd foo\n$ git config alias.s 'status -sb'\n$ git s\nfatal: This operation must be run in a work tree\n$ sed -i -e '/bare =/d' config\n$ git s\n## Initial commit on master\n?? HEAD\n?? config\n?? description\n?? hooks\n?? info/\n$ git status -sb\nfatal: This operation must be run in a work tree\n\nThe reason I am using the fallback is to use a single bare repository\nwith multiple working directories (via git-new-workdir) as suggested\nin 8fa0ee3b [1].\n\n[1] https://github.com/git/git/commit/8fa0ee3b50736eb869a3e13375bb041c1bf5aa12\n"},{"id":"210808","messageId":"20130308054824.GA24429@sigill.intra.peff.net","threadId":"33104","inReplyTo":"CAHREChhuX82ibNEDQnQUeS9TEeyMFGpuNhyXzt1Pn-Tt2BVOQA@mail.gmail.com","subject":"Re: [BUG] bare repository detection does not work with aliases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-08T05:48:25Z","receivedAt":"2013-03-08T05:48:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 07, 2013 at 05:47:45PM -0500, Mark Lodato wrote:\n\n> It seems that the fallback bare repository detection in the absence of\n> core.bare fails for aliases.\n\nThis triggered some deja vu for me, so I went digging. And indeed, this\nhas been a bug since at least 2008. This patch (which never got applied)\nfixed it:\n\n  http://thread.gmane.org/gmane.comp.version-control.git/72792\n\nThe issue is that we treat:\n\n  GIT_DIR=/some/path git ...\n\nas if the current directory is the work tree, unless core.bare is\nexplicitly set, or unless an explicit work tree is given (via\nGIT_WORK_TREE, \"git --work-tree\", or in the config).  This is handy, and\nbackwards compatible.\n\nInside setup_git_directory, when we find the directory we put it in\n$GIT_DIR for later reference by ourselves or sub-programs (since we are\ntypically moving to the top of the working tree next, we need to record\nthe original path, and can't rely on discovery finding the same path\nagain). But we don't set $GIT_WORK_TREE. So if you don't have core.bare\nset, the above rule will kick in for sub-programs, or for aliases (which\nwill call setup_git_directory again).\n\nThe solution is that when we set $GIT_DIR like this, we need to also say\n\"no, there is no working tree; we are bare\". And that's what that patch\ndoes. It's 5 years old now, so not surprisingly, it does not apply\ncleanly. The moral equivalent in today's code base would be something\nlike:\n\ndiff --git a/environment.c b/environment.c\nindex 89d6c70..8edaedd 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -200,7 +200,8 @@ void set_git_work_tree(const char *new_work_tree)\n \t\treturn;\n \t}\n \tgit_work_tree_initialized = 1;\n-\twork_tree = xstrdup(real_path(new_work_tree));\n+\tif (*new_work_tree)\n+\t\twork_tree = xstrdup(real_path(new_work_tree));\n }\n \n const char *get_git_work_tree(void)\ndiff --git a/setup.c b/setup.c\nindex e1cfa48..f0e1251 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -544,7 +544,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,\n \tworktree = get_git_work_tree();\n \n \t/* both get_git_work_tree() and cwd are already normalized */\n-\tif (!strcmp(cwd, worktree)) { /* cwd == worktree */\n+\tif (!worktree || !strcmp(cwd, worktree)) { /* cwd == worktree */\n \t\tset_git_dir(gitdirenv);\n \t\tfree(gitfile);\n \t\treturn NULL;\n@@ -636,6 +636,8 @@ static const char *setup_bare_git_dir(char *cwd, int offset, int len, int *nongi\n \t}\n \telse\n \t\tset_git_dir(\".\");\n+\n+\tsetenv(GIT_WORK_TREE_ENVIRONMENT, \"\", 1);\n \treturn NULL;\n }\n \n\nwhich passes your test. Unfortunately, this patch runs afoul of the same\ncomplaints that prevented the original from being acceptable (weirdness\non Windows with empty environment variables).\n\nHaving read the discussion again, I _think_ the more sane thing is to\nactually just have a new variable, $GIT_BARE, which overrides any\ncore.bare config (just as $GIT_WORK_TREE override core.worktree). And\nthen we set that explicitly when we are in a bare $GIT_DIR, propagating\nour auto-detection to sub-processes.\n\n-Peff\n"},{"id":"210809","messageId":"94c531c1-57a0-4464-9f30-3c63f0c1a056@email.android.com","threadId":"33104","inReplyTo":"20130308054824.GA24429@sigill.intra.peff.net","subject":"Re: [BUG] bare repository detection does not work with aliases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-08T06:27:04Z","receivedAt":"2013-03-08T06:27:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The $GIT_BARE idea sounds very sensible to me.\n\n\n\nJeff King <peff@peff.net> wrote:\n\n>On Thu, Mar 07, 2013 at 05:47:45PM -0500, Mark Lodato wrote:\n>\n>> It seems that the fallback bare repository detection in the absence\n>of\n>> core.bare fails for aliases.\n>\n>This triggered some deja vu for me, so I went digging. And indeed, this\n>has been a bug since at least 2008. This patch (which never got\n>applied)\n>fixed it:\n>\n>  http://thread.gmane.org/gmane.comp.version-control.git/72792\n>\n>The issue is that we treat:\n>\n>  GIT_DIR=/some/path git ...\n>\n>as if the current directory is the work tree, unless core.bare is\n>explicitly set, or unless an explicit work tree is given (via\n>GIT_WORK_TREE, \"git --work-tree\", or in the config).  This is handy,\n>and\n>backwards compatible.\n>\n>Inside setup_git_directory, when we find the directory we put it in\n>$GIT_DIR for later reference by ourselves or sub-programs (since we are\n>typically moving to the top of the working tree next, we need to record\n>the original path, and can't rely on discovery finding the same path\n>again). But we don't set $GIT_WORK_TREE. So if you don't have core.bare\n>set, the above rule will kick in for sub-programs, or for aliases\n>(which\n>will call setup_git_directory again).\n>\n>The solution is that when we set $GIT_DIR like this, we need to also\n>say\n>\"no, there is no working tree; we are bare\". And that's what that patch\n>does. It's 5 years old now, so not surprisingly, it does not apply\n>cleanly. The moral equivalent in today's code base would be something\n>like:\n>\n>diff --git a/environment.c b/environment.c\n>index 89d6c70..8edaedd 100644\n>--- a/environment.c\n>+++ b/environment.c\n>@@ -200,7 +200,8 @@ void set_git_work_tree(const char *new_work_tree)\n> \t\treturn;\n> \t}\n> \tgit_work_tree_initialized = 1;\n>-\twork_tree = xstrdup(real_path(new_work_tree));\n>+\tif (*new_work_tree)\n>+\t\twork_tree = xstrdup(real_path(new_work_tree));\n> }\n> \n> const char *get_git_work_tree(void)\n>diff --git a/setup.c b/setup.c\n>index e1cfa48..f0e1251 100644\n>--- a/setup.c\n>+++ b/setup.c\n>@@ -544,7 +544,7 @@ static const char *setup_explicit_git_dir(const\n>char *gitdirenv,\n> \tworktree = get_git_work_tree();\n> \n> \t/* both get_git_work_tree() and cwd are already normalized */\n>-\tif (!strcmp(cwd, worktree)) { /* cwd == worktree */\n>+\tif (!worktree || !strcmp(cwd, worktree)) { /* cwd == worktree */\n> \t\tset_git_dir(gitdirenv);\n> \t\tfree(gitfile);\n> \t\treturn NULL;\n>@@ -636,6 +636,8 @@ static const char *setup_bare_git_dir(char *cwd,\n>int offset, int len, int *nongi\n> \t}\n> \telse\n> \t\tset_git_dir(\".\");\n>+\n>+\tsetenv(GIT_WORK_TREE_ENVIRONMENT, \"\", 1);\n> \treturn NULL;\n> }\n> \n>\n>which passes your test. Unfortunately, this patch runs afoul of the\n>same\n>complaints that prevented the original from being acceptable (weirdness\n>on Windows with empty environment variables).\n>\n>Having read the discussion again, I _think_ the more sane thing is to\n>actually just have a new variable, $GIT_BARE, which overrides any\n>core.bare config (just as $GIT_WORK_TREE override core.worktree). And\n>then we set that explicitly when we are in a bare $GIT_DIR, propagating\n>our auto-detection to sub-processes.\n>\n>-Peff\n>--\n>To unsubscribe from this list: send the line \"unsubscribe git\" in\n>the body of a message to majordomo@vger.kernel.org\n>More majordomo info at  http://vger.kernel.org/majordomo-info.html\n\n-- \nPardon terseness, typo and HTML from a tablet.\n"},{"id":"210810","messageId":"20130308063756.GA29242@sigill.intra.peff.net","threadId":"33104","inReplyTo":"94c531c1-57a0-4464-9f30-3c63f0c1a056@email.android.com","subject":"Re: [BUG] bare repository detection does not work with aliases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-08T06:37:56Z","receivedAt":"2013-03-08T06:37:56Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 07, 2013 at 10:27:04PM -0800, Junio C Hamano wrote:\n\n> The $GIT_BARE idea sounds very sensible to me.\n\nUnfortunately, it is not quite as simple as that. I just wrote up the\npatch, and it turns out that we are foiled by how core.bare is treated.\nIf it is true, the repo is definitely bare. If it is false, that is only\na hint for us.\n\nSo we cannot just look at is_bare_repository() after setup_git_directory\nruns. Because we are not \"definitely bare\", only \"maybe bare\", it\nreturns false. We just happen not to have a work tree. We could do\nsomething like:\n\n  if (is_bare_repository_cfg || !work_tree)\n          setenv(\"GIT_BARE\", \"1\", 1);\n\nwhich I think would work, but feels kind of wrong. We are bare in this\ninstance, but somebody setting GIT_WORK_TREE in a sub-process would\nwant to become unbare, presumably, but our variable would override them.\n\nJust looking through all of the code paths, I am getting a little\nnervous that I would not cover all the bases for such a $GIT_BARE to\nwork (e.g., doing GIT_BARE=0 would not do I would expect as a user,\nbecause of the historical way we treat core.bare=false).\n\nSo rather than introduce something like $GIT_BARE which is going to\nbring about all new kinds of corner cases, I think I'd rather just pass\nalong a $GIT_NO_IMPLICIT_WORK_TREE variable, which is much more direct\nfor solving this problem, and is less likely to end up having bugs of\nits own.\n\n-Peff\n"},{"id":"210813","messageId":"20130308071554.GB24429@sigill.intra.peff.net","threadId":"33104","inReplyTo":"20130308054824.GA24429@sigill.intra.peff.net","subject":"[PATCH] setup: suppress implicit \".\" work-tree for bare repos","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-08T07:15:54Z","receivedAt":"2013-03-08T07:15:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If an explicit GIT_DIR is given without a working tree, we\nimplicitly assume that the current working directory should\nbe used as the working tree. E.g.,:\n\n  GIT_DIR=/some/repo.git git status\n\nwould compare against the cwd.\n\nUnfortunately, we fool this rule for sub-invocations of git\nby setting GIT_DIR internally ourselves. For example:\n\n  git init foo\n  cd foo/.git\n  git status ;# fails, as we expect\n  git config alias.st status\n  git status ;# does not fail, but should\n\nWhat happens is that we run setup_git_directory when doing\nalias lookup (since we need to see the config), set GIT_DIR\nas a result, and then leave GIT_WORK_TREE blank (because we\ndo not have one). Then when we actually run the status\ncommand, we do setup_git_directory again, which sees our\nexplicit GIT_DIR and uses the cwd as an implicit worktree.\n\nIt's tempting to argue that we should be suppressing that\nsecond invocation of setup_git_directory, as it could use\nthe values we already found in memory. However, the problem\nstill exists for sub-processes (e.g., if \"git status\" were\nan external command).\n\nYou can see another example with the \"--bare\" option, which\nsets GIT_DIR explicitly. For example:\n\n  git init foo\n  cd foo/.git\n  git status ;# fails\n  git --bare status ;# does NOT fail\n\nWe need some way of telling sub-processes \"even though\nGIT_DIR is set, do not use cwd as an implicit working tree\".\nWe could do it by putting a special token into\nGIT_WORK_TREE, but the obvious choice (an empty string) has\nsome portability problems, and could potentially be\ntriggered accidentally by a user.\n\nInstead, we add a new boolean variable, GIT_IMPLICIT_WORK_TREE,\nwhich suppresses the use of cwd as a working tree when\nGIT_DIR is set. We trigger the new variable when we know we\nare in a bare setting.\n\nThe variable is left intentionally undocumented, as this is\nan internal detail (for now, anyway). If somebody comes up\nwith a good alternate use for it, and once we are confident\nwe have shaken any bugs out of it, we can consider promoting\nit further.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nSo I think this just ends up being a cleaner and smaller change than\ntrying to support $GIT_BARE. I think $GIT_BARE could allow more\nflexibility, but it's flexibility nobody is particularly asking for, and\nthere are lots of nasty corner cases around it. I'm pretty sure this is\ndoing the right thing.\n\nHaving written this, I'm still tempted to signal the same thing by\nputting /dev/null into GIT_WORK_TREE (Junio's suggestion from the old\nthread). This one works OK because we only check GIT_WORK_TREE_IMPLICIT\n_after_ exhausting all of the other working tree options, so it is\nalways subordinate to a later setting of GIT_WORK_TREE. But it seems a\nlittle cleaner for somebody setting GIT_WORK_TREE To clear this\n\"implicit\" flag automatically.\n\nAt the same time, I would wonder how other git implementations would\nreact to GIT_WORK_TREE=/dev/null. Would they try to chdir() there and\nbarf, when they could happily exist without a working tree? Doing it\nthis way seems a bit safer from regressions (those other implementations\ndo not get the _benefit_ of this patch unless they support\nGIT_WORK_TREE_IMPLICIT, of course, but at least we are not breaking\nthem).\n\n cache.h               |  1 +\n git.c                 |  1 +\n setup.c               |  8 ++++++++\n t/t1510-repo-setup.sh | 19 +++++++++++++++++++\n 4 files changed, 29 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex e493563..070169a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -344,6 +344,7 @@ static inline enum object_type object_type(unsigned int mode)\n #define GIT_DIR_ENVIRONMENT \"GIT_DIR\"\n #define GIT_NAMESPACE_ENVIRONMENT \"GIT_NAMESPACE\"\n #define GIT_WORK_TREE_ENVIRONMENT \"GIT_WORK_TREE\"\n+#define GIT_IMPLICIT_WORK_TREE_ENVIRONMENT \"GIT_IMPLICIT_WORK_TREE\"\n #define DEFAULT_GIT_DIR_ENVIRONMENT \".git\"\n #define DB_ENVIRONMENT \"GIT_OBJECT_DIRECTORY\"\n #define INDEX_ENVIRONMENT \"GIT_INDEX_FILE\"\ndiff --git a/git.c b/git.c\nindex b10c18b..24b7984 100644\n--- a/git.c\n+++ b/git.c\n@@ -125,6 +125,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\tstatic char git_dir[PATH_MAX+1];\n \t\t\tis_bare_repository_cfg = 1;\n \t\t\tsetenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir, sizeof(git_dir)), 0);\n+\t\t\tsetenv(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, \"0\", 1);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t} else if (!strcmp(cmd, \"-c\")) {\ndiff --git a/setup.c b/setup.c\nindex 1dee47e..6c87660 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -523,6 +523,12 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,\n \t\t\tset_git_work_tree(core_worktree);\n \t\t}\n \t}\n+\telse if (!git_env_bool(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, 1)) {\n+\t\t/* #16d */\n+\t\tset_git_dir(gitdirenv);\n+\t\tfree(gitfile);\n+\t\treturn NULL;\n+\t}\n \telse /* #2, #10 */\n \t\tset_git_work_tree(\".\");\n \n@@ -601,6 +607,8 @@ static const char *setup_bare_git_dir(char *cwd, int offset, int len, int *nongi\n \tif (check_repository_format_gently(\".\", nongit_ok))\n \t\treturn NULL;\n \n+\tsetenv(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, \"0\", 1);\n+\n \t/* --work-tree is set without --git-dir; use discovered one */\n \tif (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {\n \t\tconst char *gitdir;\ndiff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh\nindex 80aedfc..cf2ee78 100755\n--- a/t/t1510-repo-setup.sh\n+++ b/t/t1510-repo-setup.sh\n@@ -517,6 +517,25 @@ test_expect_success '#16c: bare .git has no worktree' '\n \t\t\"$here/16c/.git\" \"(null)\" \"$here/16c/sub\" \"(null)\"\n '\n \n+test_expect_success '#16d: bareness preserved across alias' '\n+\tsetup_repo 16d unset \"\" unset &&\n+\t(\n+\t\tcd 16d/.git &&\n+\t\ttest_must_fail git status &&\n+\t\tgit config alias.st status &&\n+\t\ttest_must_fail git st\n+\t)\n+'\n+\n+test_expect_success '#16e: bareness preserved by --bare' '\n+\tsetup_repo 16e unset \"\" unset &&\n+\t(\n+\t\tcd 16e/.git &&\n+\t\ttest_must_fail git status &&\n+\t\ttest_must_fail git --bare status\n+\t)\n+'\n+\n test_expect_success '#17: GIT_WORK_TREE without explicit GIT_DIR is accepted (bare case)' '\n \t# Just like #16.\n \tsetup_repo 17a unset \"\" true &&\n-- \n1.8.2.rc2.4.g3e774bb\n"},{"id":"210814","messageId":"513996D4.6060009@viscovery.net","threadId":"33104","inReplyTo":"20130308071554.GB24429@sigill.intra.peff.net","subject":"Re: [PATCH] setup: suppress implicit \".\" work-tree for bare repos","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2013-03-08T07:44:20Z","receivedAt":"2013-03-08T07:44:20Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 3/8/2013 8:15, schrieb Jeff King:\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -344,6 +344,7 @@ static inline enum object_type object_type(unsigned int mode)\n>  #define GIT_DIR_ENVIRONMENT \"GIT_DIR\"\n>  #define GIT_NAMESPACE_ENVIRONMENT \"GIT_NAMESPACE\"\n>  #define GIT_WORK_TREE_ENVIRONMENT \"GIT_WORK_TREE\"\n> +#define GIT_IMPLICIT_WORK_TREE_ENVIRONMENT \"GIT_IMPLICIT_WORK_TREE\"\n>  #define DEFAULT_GIT_DIR_ENVIRONMENT \".git\"\n>  #define DB_ENVIRONMENT \"GIT_OBJECT_DIRECTORY\"\n>  #define INDEX_ENVIRONMENT \"GIT_INDEX_FILE\"\n\nThis new variable needs to be added to environment.c:local_repo_env, right?\n\n-- Hannes\n"},{"id":"210815","messageId":"7vboaujphx.fsf@alter.siamese.dyndns.org","threadId":"33104","inReplyTo":"20130308071554.GB24429@sigill.intra.peff.net","subject":"Re: [PATCH] setup: suppress implicit \".\" work-tree for bare repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-08T07:54:18Z","receivedAt":"2013-03-08T07:54:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> diff --git a/cache.h b/cache.h\n> index e493563..070169a 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -344,6 +344,7 @@ static inline enum object_type object_type(unsigned int mode)\n>  #define GIT_DIR_ENVIRONMENT \"GIT_DIR\"\n>  #define GIT_NAMESPACE_ENVIRONMENT \"GIT_NAMESPACE\"\n>  #define GIT_WORK_TREE_ENVIRONMENT \"GIT_WORK_TREE\"\n> +#define GIT_IMPLICIT_WORK_TREE_ENVIRONMENT \"GIT_IMPLICIT_WORK_TREE\"\n>  #define DEFAULT_GIT_DIR_ENVIRONMENT \".git\"\n>  #define DB_ENVIRONMENT \"GIT_OBJECT_DIRECTORY\"\n>  #define INDEX_ENVIRONMENT \"GIT_INDEX_FILE\"\n\nNot adding any user documentation is fine (you explained why in the\nlog message), but I would really prefer to have some in-code comment\nto clarify its meaning.  Is it \"Please do use implicit work tree\"\nboolean?  Is it \"This is the path to the work tree we have already\nfigured out\" string?  Is it something else?  What is it used for,\nwho sets it, what other codepath that will be invented in the future\nneed to be careful to set it (or unset it) and how does one who\nwrites that new codepath decides that he needs to do so (or\nshouldn't)?\n\nI would know *today* that it is a bool to affect us, after having\ndiscovered that we are in bare and we have set GIT_DIR (so if the\nend user already had GIT_DIR, we shouldn't set it ourselves), and\nalso our child processes, but I am not confident that I will\nremember this thread 6 months down the road.\n"},{"id":"210817","messageId":"20130308084225.GA10963@sigill.intra.peff.net","threadId":"33104","inReplyTo":"513996D4.6060009@viscovery.net","subject":"Re: [PATCH] setup: suppress implicit \".\" work-tree for bare repos","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-08T08:42:25Z","receivedAt":"2013-03-08T08:42:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 08, 2013 at 08:44:20AM +0100, Johannes Sixt wrote:\n\n> Am 3/8/2013 8:15, schrieb Jeff King:\n> > --- a/cache.h\n> > +++ b/cache.h\n> > @@ -344,6 +344,7 @@ static inline enum object_type object_type(unsigned int mode)\n> >  #define GIT_DIR_ENVIRONMENT \"GIT_DIR\"\n> >  #define GIT_NAMESPACE_ENVIRONMENT \"GIT_NAMESPACE\"\n> >  #define GIT_WORK_TREE_ENVIRONMENT \"GIT_WORK_TREE\"\n> > +#define GIT_IMPLICIT_WORK_TREE_ENVIRONMENT \"GIT_IMPLICIT_WORK_TREE\"\n> >  #define DEFAULT_GIT_DIR_ENVIRONMENT \".git\"\n> >  #define DB_ENVIRONMENT \"GIT_OBJECT_DIRECTORY\"\n> >  #define INDEX_ENVIRONMENT \"GIT_INDEX_FILE\"\n> \n> This new variable needs to be added to environment.c:local_repo_env, right?\n\nIt does; I had no idea local_repo_env existed. We should add a comment\nto cache.h to that effect, too.\n\n-Peff\n"},{"id":"210818","messageId":"20130308084343.GB10963@sigill.intra.peff.net","threadId":"33104","inReplyTo":"7vboaujphx.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] setup: suppress implicit \".\" work-tree for bare repos","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-08T08:43:43Z","receivedAt":"2013-03-08T08:43:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 07, 2013 at 11:54:18PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > diff --git a/cache.h b/cache.h\n> > index e493563..070169a 100644\n> > --- a/cache.h\n> > +++ b/cache.h\n> > @@ -344,6 +344,7 @@ static inline enum object_type object_type(unsigned int mode)\n> >  #define GIT_DIR_ENVIRONMENT \"GIT_DIR\"\n> >  #define GIT_NAMESPACE_ENVIRONMENT \"GIT_NAMESPACE\"\n> >  #define GIT_WORK_TREE_ENVIRONMENT \"GIT_WORK_TREE\"\n> > +#define GIT_IMPLICIT_WORK_TREE_ENVIRONMENT \"GIT_IMPLICIT_WORK_TREE\"\n> >  #define DEFAULT_GIT_DIR_ENVIRONMENT \".git\"\n> >  #define DB_ENVIRONMENT \"GIT_OBJECT_DIRECTORY\"\n> >  #define INDEX_ENVIRONMENT \"GIT_INDEX_FILE\"\n> \n> Not adding any user documentation is fine (you explained why in the\n> log message), but I would really prefer to have some in-code comment\n> to clarify its meaning.  Is it \"Please do use implicit work tree\"\n> boolean?  Is it \"This is the path to the work tree we have already\n> figured out\" string?  Is it something else?  What is it used for,\n> who sets it, what other codepath that will be invented in the future\n> need to be careful to set it (or unset it) and how does one who\n> writes that new codepath decides that he needs to do so (or\n> shouldn't)?\n\nMy intent was that the commit message would be enough to explain it, but\nit is a pain for a later reader to have to blame the line back to that\ncommit to read it. I'll re-roll with a comment.\n\n-Peff\n"},{"id":"210820","messageId":"20130308092824.GA9127@sigill.intra.peff.net","threadId":"33104","inReplyTo":"20130308084343.GB10963@sigill.intra.peff.net","subject":"[PATCHv2] setup and GIT_IMPLICIT_WORK_TREE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-08T09:28:25Z","receivedAt":"2013-03-08T09:28:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here's a re-roll of the GIT_IMPLICIT_WORK_TREE patch which should\naddress the comments in the last round. I've added an explanatory\ncomment near the variable definition, and added it to local_repo_env.\n\nWhile doing that, I noticed some cleanup opportunities around\nlocal_repo_env, which resulted in the first two patches.\n\n  [1/3]: cache.h: drop LOCAL_REPO_ENV_SIZE\n  [2/3]: environment: add GIT_PREFIX to local_repo_env\n  [3/3]: setup: suppress implicit \".\" work-tree for bare repos\n\n-Peff\n"},{"id":"210821","messageId":"20130308092907.GA1923@sigill.intra.peff.net","threadId":"33104","inReplyTo":"20130308092824.GA9127@sigill.intra.peff.net","subject":"[PATCH v2 1/3] cache.h: drop LOCAL_REPO_ENV_SIZE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-08T09:29:08Z","receivedAt":"2013-03-08T09:29:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We keep a static array of variables that should be cleared\nwhen invoking a sub-process on another repo. We statically\nsize the array with the LOCAL_REPO_ENV_SIZE macro so that\nany readers do not have to count it themselves.\n\nAs it turns out, no readers actually use the macro, and it\ncreates a maintenance headache, as modifications to the\narray need to happen in two places (one to add the new\nelement, and another to bump the size).\n\nSince it's NULL-terminated, we can just drop the size macro\nentirely. While we're at it, we'll clean up some comments\naround it, and add a new mention of it at the top of the\nlist of environment variable macros. Even though\nlocal_repo_env is right below that list, it's easy to miss,\nand additions to that list should consider local_repo_env.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n cache.h       | 12 ++++++------\n environment.c |  6 ++----\n 2 files changed, 8 insertions(+), 10 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex e493563..b90044a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -341,6 +341,7 @@ static inline enum object_type object_type(unsigned int mode)\n \t\tOBJ_BLOB;\n }\n \n+/* Double-check local_repo_env below if you add to this list. */\n #define GIT_DIR_ENVIRONMENT \"GIT_DIR\"\n #define GIT_NAMESPACE_ENVIRONMENT \"GIT_NAMESPACE\"\n #define GIT_WORK_TREE_ENVIRONMENT \"GIT_WORK_TREE\"\n@@ -365,13 +366,12 @@ static inline enum object_type object_type(unsigned int mode)\n #define GIT_LITERAL_PATHSPECS_ENVIRONMENT \"GIT_LITERAL_PATHSPECS\"\n \n /*\n- * Repository-local GIT_* environment variables\n- * The array is NULL-terminated to simplify its usage in contexts such\n- * environment creation or simple walk of the list.\n- * The number of non-NULL entries is available as a macro.\n+ * Repository-local GIT_* environment variables; these will be cleared\n+ * when git spawns a sub-process that runs inside another repository.\n+ * The array is NULL-terminated, which makes it easy to pass in the \"env\"\n+ * parameter of a run-command invocation, or to do a simple walk.\n  */\n-#define LOCAL_REPO_ENV_SIZE 9\n-extern const char *const local_repo_env[LOCAL_REPO_ENV_SIZE + 1];\n+extern const char * const local_repo_env[];\n \n extern int is_bare_repository_cfg;\n extern int is_bare_repository(void);\ndiff --git a/environment.c b/environment.c\nindex 89d6c70..dc73927 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -83,11 +83,9 @@ static char *git_object_dir, *git_index_file, *git_graft_file;\n static char *git_object_dir, *git_index_file, *git_graft_file;\n \n /*\n- * Repository-local GIT_* environment variables\n- * Remember to update local_repo_env_size in cache.h when\n- * the size of the list changes\n+ * Repository-local GIT_* environment variables; see cache.h for details.\n  */\n-const char * const local_repo_env[LOCAL_REPO_ENV_SIZE + 1] = {\n+const char * const local_repo_env[] = {\n \tALTERNATE_DB_ENVIRONMENT,\n \tCONFIG_ENVIRONMENT,\n \tCONFIG_DATA_ENVIRONMENT,\n-- \n1.8.2.rc2.4.g3e774bb\n"},{"id":"210822","messageId":"20130308093025.GB1923@sigill.intra.peff.net","threadId":"33104","inReplyTo":"20130308092824.GA9127@sigill.intra.peff.net","subject":"[PATCH v2 2/3] environment: add GIT_PREFIX to local_repo_env","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-08T09:30:25Z","receivedAt":"2013-03-08T09:30:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The GIT_PREFIX variable is set based on our location within\nthe working tree. It should therefore be cleared whenever\nGIT_WORK_TREE is cleared.\n\nIn practice, this doesn't cause any bugs, because none of\nthe sub-programs we invoke with local_repo_env cleared\nactually care about GIT_PREFIX. But this is the right thing\nto do, and future proofs us again that assumption changing.\n\nWhile we're at it, let's define a GIT_PREFIX_ENVIRONMENT\nmacro; this avoids repetition of the string literal, which\ncan help catch any spelling mistakes in the code.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI noticed this one because it was near code I was touching in an earlier\niteration of patch 3. I gave a quick skim and did not notice any other\nvariables which would want to receive the same treatment.\n\n cache.h       | 1 +\n environment.c | 1 +\n setup.c       | 4 ++--\n 3 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex b90044a..23e6e62 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -345,6 +345,7 @@ static inline enum object_type object_type(unsigned int mode)\n #define GIT_DIR_ENVIRONMENT \"GIT_DIR\"\n #define GIT_NAMESPACE_ENVIRONMENT \"GIT_NAMESPACE\"\n #define GIT_WORK_TREE_ENVIRONMENT \"GIT_WORK_TREE\"\n+#define GIT_PREFIX_ENVIRONMENT \"GIT_PREFIX\"\n #define DEFAULT_GIT_DIR_ENVIRONMENT \".git\"\n #define DB_ENVIRONMENT \"GIT_OBJECT_DIRECTORY\"\n #define INDEX_ENVIRONMENT \"GIT_INDEX_FILE\"\ndiff --git a/environment.c b/environment.c\nindex dc73927..2bd1c37 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -95,6 +95,7 @@ const char * const local_repo_env[] = {\n \tGRAFT_ENVIRONMENT,\n \tINDEX_ENVIRONMENT,\n \tNO_REPLACE_OBJECTS_ENVIRONMENT,\n+\tGIT_PREFIX_ENVIRONMENT,\n \tNULL\n };\n \ndiff --git a/setup.c b/setup.c\nindex 1dee47e..1996295 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -794,9 +794,9 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \n \tprefix = setup_git_directory_gently_1(nongit_ok);\n \tif (prefix)\n-\t\tsetenv(\"GIT_PREFIX\", prefix, 1);\n+\t\tsetenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);\n \telse\n-\t\tsetenv(\"GIT_PREFIX\", \"\", 1);\n+\t\tsetenv(GIT_PREFIX_ENVIRONMENT, \"\", 1);\n \n \tif (startup_info) {\n \t\tstartup_info->have_repository = !nongit_ok || !*nongit_ok;\n-- \n1.8.2.rc2.4.g3e774bb\n"},{"id":"210823","messageId":"20130308093222.GA2086@sigill.intra.peff.net","threadId":"33104","inReplyTo":"20130308092824.GA9127@sigill.intra.peff.net","subject":"[PATCH v2 3/3] setup: suppress implicit \".\" work-tree for bare repos","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-08T09:32:22Z","receivedAt":"2013-03-08T09:32:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If an explicit GIT_DIR is given without a working tree, we\nimplicitly assume that the current working directory should\nbe used as the working tree. E.g.,:\n\n  GIT_DIR=/some/repo.git git status\n\nwould compare against the cwd.\n\nUnfortunately, we fool this rule for sub-invocations of git\nby setting GIT_DIR internally ourselves. For example:\n\n  git init foo\n  cd foo/.git\n  git status ;# fails, as we expect\n  git config alias.st status\n  git status ;# does not fail, but should\n\nWhat happens is that we run setup_git_directory when doing\nalias lookup (since we need to see the config), set GIT_DIR\nas a result, and then leave GIT_WORK_TREE blank (because we\ndo not have one). Then when we actually run the status\ncommand, we do setup_git_directory again, which sees our\nexplicit GIT_DIR and uses the cwd as an implicit worktree.\n\nIt's tempting to argue that we should be suppressing that\nsecond invocation of setup_git_directory, as it could use\nthe values we already found in memory. However, the problem\nstill exists for sub-processes (e.g., if \"git status\" were\nan external command).\n\nYou can see another example with the \"--bare\" option, which\nsets GIT_DIR explicitly. For example:\n\n  git init foo\n  cd foo/.git\n  git status ;# fails\n  git --bare status ;# does NOT fail\n\nWe need some way of telling sub-processes \"even though\nGIT_DIR is set, do not use cwd as an implicit working tree\".\nWe could do it by putting a special token into\nGIT_WORK_TREE, but the obvious choice (an empty string) has\nsome portability problems.\n\nInstead, we add a new boolean variable, GIT_IMPLICIT_WORK_TREE,\nwhich suppresses the use of cwd as a working tree when\nGIT_DIR is set. We trigger the new variable when we know we\nare in a bare setting.\n\nThe variable is left intentionally undocumented, as this is\nan internal detail (for now, anyway). If somebody comes up\nwith a good alternate use for it, and once we are confident\nwe have shaken any bugs out of it, we can consider promoting\nit further.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n cache.h               | 12 ++++++++++++\n environment.c         |  1 +\n git.c                 |  1 +\n setup.c               |  8 ++++++++\n t/t1510-repo-setup.sh | 19 +++++++++++++++++++\n 5 files changed, 41 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 23e6e62..635f2e9 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -367,6 +367,18 @@ static inline enum object_type object_type(unsigned int mode)\n #define GIT_LITERAL_PATHSPECS_ENVIRONMENT \"GIT_LITERAL_PATHSPECS\"\n \n /*\n+ * This environment variable is expected to contain a boolean indicating\n+ * whether we should or should not treat:\n+ *\n+ *   GIT_DIR=foo.git git ...\n+ *\n+ * as if GIT_WORK_TREE=. was given. It's not expected that users will make use\n+ * of this, but we use it internally to communicate to sub-processes that we\n+ * are in a bare repo. If not set, defaults to true.\n+ */\n+#define GIT_IMPLICIT_WORK_TREE_ENVIRONMENT \"GIT_IMPLICIT_WORK_TREE\"\n+\n+/*\n  * Repository-local GIT_* environment variables; these will be cleared\n  * when git spawns a sub-process that runs inside another repository.\n  * The array is NULL-terminated, which makes it easy to pass in the \"env\"\ndiff --git a/environment.c b/environment.c\nindex 2bd1c37..e2e75c1 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -92,6 +92,7 @@ const char * const local_repo_env[] = {\n \tDB_ENVIRONMENT,\n \tGIT_DIR_ENVIRONMENT,\n \tGIT_WORK_TREE_ENVIRONMENT,\n+\tGIT_IMPLICIT_WORK_TREE_ENVIRONMENT,\n \tGRAFT_ENVIRONMENT,\n \tINDEX_ENVIRONMENT,\n \tNO_REPLACE_OBJECTS_ENVIRONMENT,\ndiff --git a/git.c b/git.c\nindex b10c18b..24b7984 100644\n--- a/git.c\n+++ b/git.c\n@@ -125,6 +125,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\tstatic char git_dir[PATH_MAX+1];\n \t\t\tis_bare_repository_cfg = 1;\n \t\t\tsetenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir, sizeof(git_dir)), 0);\n+\t\t\tsetenv(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, \"0\", 1);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t} else if (!strcmp(cmd, \"-c\")) {\ndiff --git a/setup.c b/setup.c\nindex 1996295..9107f54 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -523,6 +523,12 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,\n \t\t\tset_git_work_tree(core_worktree);\n \t\t}\n \t}\n+\telse if (!git_env_bool(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, 1)) {\n+\t\t/* #16d */\n+\t\tset_git_dir(gitdirenv);\n+\t\tfree(gitfile);\n+\t\treturn NULL;\n+\t}\n \telse /* #2, #10 */\n \t\tset_git_work_tree(\".\");\n \n@@ -601,6 +607,8 @@ static const char *setup_bare_git_dir(char *cwd, int offset, int len, int *nongi\n \tif (check_repository_format_gently(\".\", nongit_ok))\n \t\treturn NULL;\n \n+\tsetenv(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, \"0\", 1);\n+\n \t/* --work-tree is set without --git-dir; use discovered one */\n \tif (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {\n \t\tconst char *gitdir;\ndiff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh\nindex 80aedfc..cf2ee78 100755\n--- a/t/t1510-repo-setup.sh\n+++ b/t/t1510-repo-setup.sh\n@@ -517,6 +517,25 @@ test_expect_success '#16c: bare .git has no worktree' '\n \t\t\"$here/16c/.git\" \"(null)\" \"$here/16c/sub\" \"(null)\"\n '\n \n+test_expect_success '#16d: bareness preserved across alias' '\n+\tsetup_repo 16d unset \"\" unset &&\n+\t(\n+\t\tcd 16d/.git &&\n+\t\ttest_must_fail git status &&\n+\t\tgit config alias.st status &&\n+\t\ttest_must_fail git st\n+\t)\n+'\n+\n+test_expect_success '#16e: bareness preserved by --bare' '\n+\tsetup_repo 16e unset \"\" unset &&\n+\t(\n+\t\tcd 16e/.git &&\n+\t\ttest_must_fail git status &&\n+\t\ttest_must_fail git --bare status\n+\t)\n+'\n+\n test_expect_success '#17: GIT_WORK_TREE without explicit GIT_DIR is accepted (bare case)' '\n \t# Just like #16.\n \tsetup_repo 17a unset \"\" true &&\n-- \n1.8.2.rc2.4.g3e774bb\n"},{"id":"210853","messageId":"CAPig+cRUCnWJLeuXL=LLk7kUkwPnHqaL_KGcSdq3yO+YZ345tQ@mail.gmail.com","threadId":"33104","inReplyTo":"20130308093025.GB1923@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/3] environment: add GIT_PREFIX to local_repo_env","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-03-08T21:39:02Z","receivedAt":"2013-03-08T21:39:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 8, 2013 at 4:30 AM, Jeff King <peff@peff.net> wrote:\n> The GIT_PREFIX variable is set based on our location within\n> the working tree. It should therefore be cleared whenever\n> GIT_WORK_TREE is cleared.\n>\n> In practice, this doesn't cause any bugs, because none of\n> the sub-programs we invoke with local_repo_env cleared\n> actually care about GIT_PREFIX. But this is the right thing\n> to do, and future proofs us again that assumption changing.\n\ns/again/against/\n\n-- ES\n"},{"id":"210854","messageId":"20130308214404.GA9723@sigill.intra.peff.net","threadId":"33104","inReplyTo":"CAPig+cRUCnWJLeuXL=LLk7kUkwPnHqaL_KGcSdq3yO+YZ345tQ@mail.gmail.com","subject":"Re: [PATCH v2 2/3] environment: add GIT_PREFIX to local_repo_env","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-08T21:44:04Z","receivedAt":"2013-03-08T21:44:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 08, 2013 at 04:39:02PM -0500, Eric Sunshine wrote:\n\n> On Fri, Mar 8, 2013 at 4:30 AM, Jeff King <peff@peff.net> wrote:\n> > The GIT_PREFIX variable is set based on our location within\n> > the working tree. It should therefore be cleared whenever\n> > GIT_WORK_TREE is cleared.\n> >\n> > In practice, this doesn't cause any bugs, because none of\n> > the sub-programs we invoke with local_repo_env cleared\n> > actually care about GIT_PREFIX. But this is the right thing\n> > to do, and future proofs us again that assumption changing.\n> \n> s/again/against/\n\nYep, thanks.\n\n-Peff\n"},{"id":"210860","messageId":"7vmwudfq9r.fsf@alter.siamese.dyndns.org","threadId":"33104","inReplyTo":"20130308214404.GA9723@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/3] environment: add GIT_PREFIX to local_repo_env","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-08T23:03:28Z","receivedAt":"2013-03-08T23:03:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Mar 08, 2013 at 04:39:02PM -0500, Eric Sunshine wrote:\n>\n>> On Fri, Mar 8, 2013 at 4:30 AM, Jeff King <peff@peff.net> wrote:\n>> > The GIT_PREFIX variable is set based on our location within\n>> > the working tree. It should therefore be cleared whenever\n>> > GIT_WORK_TREE is cleared.\n>> >\n>> > In practice, this doesn't cause any bugs, because none of\n>> > the sub-programs we invoke with local_repo_env cleared\n>> > actually care about GIT_PREFIX. But this is the right thing\n>> > to do, and future proofs us again that assumption changing.\n>> \n>> s/again/against/\n>\n> Yep, thanks.\n\nThanks; squashed-in.\n"}]}