{"thread":{"id":"54011","subject":"[PATCH] init: disallow --separate-git-dir with bare repository","startedAt":"2020-08-09T22:54:20Z","lastAt":"2020-08-11T09:02:47Z","messageCount":4,"participants":["Eric Sunshine","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"403241","messageId":"20200809225316.19503-1-sunshine@sunshineco.com","threadId":"54011","inReplyTo":null,"subject":"[PATCH] init: disallow --separate-git-dir with bare repository","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-08-09T22:53:16Z","receivedAt":"2020-08-09T22:54:20Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"The purpose of \"git init --separate-git-dir\" is to separate the\nrepository from the worktree. This is true even when --separate-git-dir\nis used on an existing worktree, in which case, it moves the .git/\nsubdirectory to a new location outside the worktree.\n\nHowever, an outright bare repository (such as one created by \"git init\n--bare\"), has no worktree, so using --separate-git-dir to separate it\nfrom its non-existent worktree is nonsensical. Therefore, make it an\nerror to use --separate-git-dir on a bare repository.\n\nImplementation note: \"git init\" considers a repository bare if told so\nexplicitly via --bare or if it guesses it to be so based upon\nheuristics. In the explicit --bare case, a conflict with\n--separate-git-dir is easy to detect early. In the guessed case,\nhowever, the conflict can only be detected once \"bareness\" is guessed,\nwhich happens after \"git init\" has begun creating the repository.\nTechnically, we can get by with a single late check which would cover\nboth cases, however, erroring out early, when possible, without leaving\ndetritus provides a better user experience.\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n\nNotes:\n    I ran across this while working on a worktree-related topic dealing\n    with --separate-git-dir. Closing this loophole eliminates some\n    potentially strange and unworkable cases that the other topic might\n    otherwise encounter. Even though this change is tangentially related\n    to the other topic, it's also sufficiently standalone to post\n    separately; plus, I want to get feedback on it early since I'm not\n    100% happy with checking for the conflict between --separate-git-dir\n    and a bare repository in two separate places (though, I can live\n    with it).\n\n builtin/init-db.c |  5 +++++\n t/t0001-init.sh   | 13 +++++++++++++\n 2 files changed, 18 insertions(+)\n\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex cee64823cb..60e5c14169 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -568,6 +568,9 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n \n \targc = parse_options(argc, argv, prefix, init_db_options, init_db_usage, 0);\n \n+\tif (real_git_dir && is_bare_repository_cfg == 1)\n+\t\tdie(_(\"--separate-git-dir and --bare are mutually exclusive\"));\n+\n \tif (real_git_dir && !is_absolute_path(real_git_dir))\n \t\treal_git_dir = real_pathdup(real_git_dir, 1);\n \n@@ -663,6 +666,8 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n \t\t\t\t   get_git_work_tree());\n \t}\n \telse {\n+\t\tif (real_git_dir)\n+\t\t\tdie(_(\"--separate-git-dir incompatible with bare repository\"));\n \t\tif (work_tree)\n \t\t\tset_git_work_tree(work_tree);\n \t}\ndiff --git a/t/t0001-init.sh b/t/t0001-init.sh\nindex 6d2467995e..5c585f7fcb 100755\n--- a/t/t0001-init.sh\n+++ b/t/t0001-init.sh\n@@ -316,6 +316,19 @@ test_expect_success 'init with separate gitdir' '\n \ttest_path_is_dir realgitdir/refs\n '\n \n+test_expect_success 'explicit bare & --separate-git-dir incompatible' '\n+\ttest_must_fail git init --bare --separate-git-dir goop.git bare.git 2>err &&\n+\ttest_i18ngrep \"mutually exclusive\" err\n+'\n+\n+test_expect_success 'implicit bare & --separate-git-dir incompatible' '\n+\ttest_when_finished \"rm -rf bare.git\" &&\n+\tmkdir -p bare.git &&\n+\ttest_must_fail env GIT_DIR=. \\\n+\t\tgit -C bare.git init --separate-git-dir goop.git 2>err &&\n+\ttest_i18ngrep \"incompatible\" err\n+'\n+\n test_lazy_prereq GETCWD_IGNORES_PERMS '\n \tbase=GETCWD_TEST_BASE_DIR &&\n \tmkdir -p $base/dir &&\n-- \n2.28.0.220.ged08abb693\n\n"},{"id":"403252","messageId":"20200810095648.GA37030@coredump.intra.peff.net","threadId":"54011","inReplyTo":"20200809225316.19503-1-sunshine@sunshineco.com","subject":"Re: [PATCH] init: disallow --separate-git-dir with bare repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-10T09:56:48Z","receivedAt":"2020-08-10T09:56:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 09, 2020 at 06:53:16PM -0400, Eric Sunshine wrote:\n\n> The purpose of \"git init --separate-git-dir\" is to separate the\n> repository from the worktree. This is true even when --separate-git-dir\n> is used on an existing worktree, in which case, it moves the .git/\n> subdirectory to a new location outside the worktree.\n> \n> However, an outright bare repository (such as one created by \"git init\n> --bare\"), has no worktree, so using --separate-git-dir to separate it\n> from its non-existent worktree is nonsensical. Therefore, make it an\n> error to use --separate-git-dir on a bare repository.\n\nI agree that it seems like nonsense. I'm a little curious what it\nhappens to do today, just because I'd wonder if it could possibly be of\nany use to somebody.\n\n> Implementation note: \"git init\" considers a repository bare if told so\n> explicitly via --bare or if it guesses it to be so based upon\n> heuristics. In the explicit --bare case, a conflict with\n> --separate-git-dir is easy to detect early. In the guessed case,\n> however, the conflict can only be detected once \"bareness\" is guessed,\n> which happens after \"git init\" has begun creating the repository.\n> Technically, we can get by with a single late check which would cover\n> both cases, however, erroring out early, when possible, without leaving\n> detritus provides a better user experience.\n\nI think we'd clean up that detritus with our atexit handler, but I like\nthe extra check here. It lets us give a slightly more specific message\nwhen we can catch it early (\"these two options are incompatible\").\n\n>  builtin/init-db.c |  5 +++++\n>  t/t0001-init.sh   | 13 +++++++++++++\n>  2 files changed, 18 insertions(+)\n\nThe patch itself looks good, assuming my \"I'd wonder...\" line of inquiry\nabove produces nothing of value. :)\n\n-Peff\n"},{"id":"403333","messageId":"CAPig+cSMzEuMYNnHhMGwqGFgjWfX0+j1KoOSK8V7Wav97p+UVw@mail.gmail.com","threadId":"54011","inReplyTo":"20200810095648.GA37030@coredump.intra.peff.net","subject":"Re: [PATCH] init: disallow --separate-git-dir with bare repository","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-08-11T05:44:00Z","receivedAt":"2020-08-11T05:44:16Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 10, 2020 at 5:56 AM Jeff King <peff@peff.net> wrote:\n> On Sun, Aug 09, 2020 at 06:53:16PM -0400, Eric Sunshine wrote:\n> > However, an outright bare repository (such as one created by \"git init\n> > --bare\"), has no worktree, so using --separate-git-dir to separate it\n> > from its non-existent worktree is nonsensical. Therefore, make it an\n> > error to use --separate-git-dir on a bare repository.\n>\n> I agree that it seems like nonsense. I'm a little curious what it\n> happens to do today, just because I'd wonder if it could possibly be of\n> any use to somebody.\n\nThe current behavior does some goofy stuff which I can't imagine being\nuseful to anyone. For instance:\n\n    % git init --bare --separate-git-dir bar.git foo.git\n    % ls -1\n    bar.git\n    foo.git\n    % cat foo.git\n    gitdir: /.../bar.git\n    %\n\nSo, I just initialized a bare repository \"foo.git\" which isn't\nactually a directory at all but instead is just a 'gitlink' file. What\ncan I do with file \"foo.git\"? Not a whole lot. \"bar.git\" is the actual\nrepository.\n\nThe case of re-initializing a bare repository with --separate-git-dir\nis even weirder:\n\n    % rm -rf foo.git bar.git\n    % git init --bare foo.git\n    % # ... add some commits ...\n    % git -C foo.git rev-parse master\n    86e28bed5ac8f5ea774690b4fc0eddb434800e9e\n    % git -C foo.git init --separate-git-dir ../bar.git\n    % ls -FA bar.git\n    HEAD         config       hooks/       objects/\n    branches/    description  info/        refs/\n    % ls -FA foo.git\n    .git         branches/    description  info/        refs/\n    HEAD         config       hooks/       objects/\n    % cat foo.git/.git\n    gitdir: /.../bar.git\n    % git -C bar.git rev-parse master --\n    fatal: bad revision 'master'\n    % git -C foo.git rev-parse master --\n    fatal: bad revision 'master'\n    % rm foo.git/.git\n    % git -C foo.git rev-parse master\n    86e28bed5ac8f5ea774690b4fc0eddb434800e9e\n    %\n\nSo, the repository doesn't actually move at all; it stays at\n\"foo.git\". And \"bar.git\", even though it looks like a repository, is\nactually empty; all the objects are still in \"foo.git\". Plus \"foo.git\"\nis _broken_ by the \".git\" file which --separate-git-dir placed there.\n\nSo, it's meaningless and perhaps destructive behavior using\n--separate-git-dir with a bare repository,\n\n> > Implementation note: \"git init\" considers a repository bare if told so\n> > explicitly via --bare or if it guesses it to be so based upon\n> > heuristics. In the explicit --bare case, a conflict with\n> > --separate-git-dir is easy to detect early. In the guessed case,\n> > however, the conflict can only be detected once \"bareness\" is guessed,\n> > which happens after \"git init\" has begun creating the repository.\n> > Technically, we can get by with a single late check which would cover\n> > both cases, however, erroring out early, when possible, without leaving\n> > detritus provides a better user experience.\n>\n> I think we'd clean up that detritus with our atexit handler, but I like\n> the extra check here. It lets us give a slightly more specific message\n> when we can catch it early (\"these two options are incompatible\").\n\nWith only the latter (after-the-fact) check:\n\n    % git init --bare --separate-git-dir bar.git foo.git\n    fatal: --separate-git-dir incompatible with bare repository\n    % ls -1\n    foo.git\n    % ls -A foo.git/\n    %\n\nIt leaves the directory \"foo.git\" around, though the directory is\nempty. With the earlier check in place, it avoids leaving that empty\ndirectory.\n\n> The patch itself looks good, assuming my \"I'd wonder...\" line of inquiry\n> above produces nothing of value. :)\n\nI can't find any value in the current behavior, so I'm pretty well\nconvinced that this patch is desirable (even if I don't quite like\nrepeating the check).\n\nI didn't add any of the above explanation-by-examples to the commit\nmessage because it seemed too much detail.\n"},{"id":"403340","messageId":"20200811090245.GA2050353@coredump.intra.peff.net","threadId":"54011","inReplyTo":"CAPig+cSMzEuMYNnHhMGwqGFgjWfX0+j1KoOSK8V7Wav97p+UVw@mail.gmail.com","subject":"Re: [PATCH] init: disallow --separate-git-dir with bare repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-11T09:02:45Z","receivedAt":"2020-08-11T09:02:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 11, 2020 at 01:44:00AM -0400, Eric Sunshine wrote:\n\n> > I agree that it seems like nonsense. I'm a little curious what it\n> > happens to do today, just because I'd wonder if it could possibly be of\n> > any use to somebody.\n> \n> The current behavior does some goofy stuff which I can't imagine being\n> useful to anyone. For instance:\n> [...]\n\nOK, that's sufficiently brain-dead that I agree nobody will be sad to\nsee it go. Thanks for indulging me. :)\n\n> > I think we'd clean up that detritus with our atexit handler, but I like\n> > the extra check here. It lets us give a slightly more specific message\n> > when we can catch it early (\"these two options are incompatible\").\n> \n> With only the latter (after-the-fact) check:\n> \n>     % git init --bare --separate-git-dir bar.git foo.git\n>     fatal: --separate-git-dir incompatible with bare repository\n>     % ls -1\n>     foo.git\n>     % ls -A foo.git/\n>     %\n> \n> It leaves the directory \"foo.git\" around, though the directory is\n> empty. With the earlier check in place, it avoids leaving that empty\n> directory.\n\nAh, right. git-clone has some magic auto-cleanup for signals/atexit, but\ngit-init does not.  It might be worth adding that, but it's definitely\noutside the scope of your patch.\n\n-Peff\n"}]}