{"thread":{"id":"23202","subject":"[PATCH] builtins: reset startup_info->have_run_setup_gitdir when unsetting up repository","startedAt":"2010-03-27T09:13:03Z","lastAt":"2010-03-28T07:11:21Z","messageCount":5,"participants":["Nguyễn Thái Ngọc Duy","Jonathan Nieder","Nguyen Thai Ngoc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"137953","messageId":"1269681184-1992-1-git-send-email-pclouds@gmail.com","threadId":"23202","inReplyTo":null,"subject":"[PATCH] builtins: setup repository before print unknown command error","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-03-27T09:13:03Z","receivedAt":"2010-03-27T09:13:03Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"help_unknown_cmd() will need to look into repository's config, in\norder to collect all possible commands/aliases and give a\nsuggestion. So, repository must be set up before this function is\ncalled.\n\nAs it is now, because\n - alias handling will always be done before help_unknown_cmd()\n - alias handling code will search and set up repository if found\n - alias handline code will not undo repository setup\n\nThese ensure that repository will always be set up (or attempted to\nset up) before help_unknown_cmd(), so there is no issue. But the setup\ndependency here is subtle. It may break some day if someone reorders\nthe loop, for example.\n\nMake a note about this.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n git.c |    7 +++++++\n 1 files changed, 7 insertions(+), 0 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex bd1d4bb..7ebfea5 100644\n--- a/git.c\n+++ b/git.c\n@@ -547,6 +547,13 @@ int main(int argc, const char **argv)\n \t\t\texit(1);\n \t\t}\n \t\tif (!done_help) {\n+\t\t\t/*\n+\t\t\t * help_unknown_cmd() requires that a repository has been\n+\t\t\t * searched for and set up if found.\n+\t\t\t * Luckily, the alias handling code already took care of this.\n+\t\t\t */\n+\t\t\tif (!startup_info->have_run_setup_gitdir)\n+\t\t\t\tdie(\"internal error: handling unknown command\");\n \t\t\tcmd = argv[0] = help_unknown_cmd(cmd);\n \t\t\tdone_help = 1;\n \t\t} else\n-- \n1.7.0.rc1.541.g2da82.dirty\n"},{"id":"137952","messageId":"1269681184-1992-2-git-send-email-pclouds@gmail.com","threadId":"23202","inReplyTo":"1269681184-1992-1-git-send-email-pclouds@gmail.com","subject":"[PATCH] builtins: reset startup_info->have_run_setup_gitdir when unsetting up repository","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-03-27T09:13:04Z","receivedAt":"2010-03-27T09:13:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"startup_info->have_run_setup_gitdir is used to guard unallowed access\nto repository. When a repository has been set up and the real command\ndoes not expect any setup, we should revert to the original \"fresh\"\nstate, including startup_info->have_run_setup_gitdir. Otherwise, the\nnext attempt to set up repository will fail.\n\nWhile at it, also reset repository_format_version to zero. When\nomitted, the format should be understood as the last supported\nversion, i.e. zero. This is probably only used by \"git init\" or \"git\nclone\".\n\nFault caught by Jonathan Nieder (and demonstrated with a beautiful test case)\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Junio, I can split this up and put it to the original patches where the faults\n are introduced if you want.\n\n git.c           |   13 ++++++++-----\n setup.c         |    2 +-\n t/t0001-init.sh |   14 ++++++++++++++\n 3 files changed, 23 insertions(+), 6 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 7ebfea5..5c249fd 100644\n--- a/git.c\n+++ b/git.c\n@@ -242,11 +242,14 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n \t\t\tint nongit_ok;\n \t\t\tsetup_git_directory_gently(&nongit_ok);\n \t\t}\n-\t\telse if (startup_info->have_repository) {\n-\t\t\tif (p->option & (RUN_SETUP_GENTLY | RUN_SETUP))\n-\t\t\t\t; /* done already */\n-\t\t\telse\n-\t\t\t\tunset_git_directory(startup_info->prefix);\n+\t\telse if (startup_info->have_run_setup_gitdir) {\n+\t\t\tif (startup_info->have_repository) {\n+\t\t\t\tif (p->option & (RUN_SETUP_GENTLY | RUN_SETUP))\n+\t\t\t\t\t; /* done already */\n+\t\t\t\telse\n+\t\t\t\t\tunset_git_directory(startup_info->prefix);\n+\t\t\t}\n+\t\t\tstartup_info->have_run_setup_gitdir = 0;\n \t\t}\n \n \t\tif (use_pager == -1 && p->option & RUN_SETUP)\ndiff --git a/setup.c b/setup.c\nindex d9bb616..43b529e 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -358,7 +358,7 @@ void unset_git_directory(const char *prefix)\n \tinside_git_dir = -1;\n \n \t/* Initialized in check_repository_format_version() */\n-\trepository_format_version = 0xFF;\n+\trepository_format_version = 0;\n \tshared_repository = PERM_UMASK;\n \tis_bare_repository_cfg = -1;\n \tgit_work_tree_cfg = NULL;\ndiff --git a/t/t0001-init.sh b/t/t0001-init.sh\nindex 5386504..f93856b 100755\n--- a/t/t0001-init.sh\n+++ b/t/t0001-init.sh\n@@ -33,6 +33,20 @@ test_expect_success 'plain' '\n \tcheck_config plain/.git false unset\n '\n \n+test_expect_success 'plain through aliased command' '\n+\t(\n+\t\tunset GIT_DIR GIT_WORK_TREE GIT_CONFIG_NOGLOBAL &&\n+\t\tHOME=$(pwd)/alias-config &&\n+\t\texport HOME &&\n+\t\tmkdir alias-config &&\n+\t\techo \"[alias] aliasedinit = init\" >alias-config/.gitconfig &&\n+\t\tmkdir plain-aliased &&\n+\t\tcd plain-aliased &&\n+\t\tgit aliasedinit\n+\t) &&\n+\tcheck_config plain-aliased/.git false unset\n+'\n+\n test_expect_success 'plain with GIT_WORK_TREE' '\n \tif (\n \t\tunset GIT_DIR\n-- \n1.7.0.rc1.541.g2da82.dirty\n"},{"id":"137979","messageId":"20100327223813.GA5809@progeny.tock","threadId":"23202","inReplyTo":"1269681184-1992-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] builtins: reset startup_info->have_run_setup_gitdir when unsetting up repository","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-03-27T22:38:13Z","receivedAt":"2010-03-27T22:38:13Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi again,\n\nNguyễn Thái Ngọc Duy wrote:\n\n> While at it, also reset repository_format_version to zero. When\n> omitted, the format should be understood as the last supported\n> version, i.e. zero. This is probably only used by \"git init\" or \"git\n> clone\".\n[...]\n>  \t/* Initialized in check_repository_format_version() */\n> -\trepository_format_version = 0xFF;\n> +\trepository_format_version = 0;\n\nGood change, but wrong justification in my opinion.\n\nv0.99.9l^2~54 (init-db: check template and repository format.,\n2005-11-25) taught ‘git init-db’ to make sure that\n\n  repository_format_version <= GIT_REPO_VERSION\n\nbefore initializing a new repository.  repository_format_version was\nbeing explicitly initialized globally to 0 at the time, presumably to\nensure tests like this always succeed when no repository format version\nis declared (or in other words, the repository format for repositories\ninitialized before git v0.99.9l^2~56 is zero by convention).  As a happy\nside effect, that default takes care of the “no pre-existing repository”\ncase here.\n\nThe explicit initialization was removed in commit v1.4.3-rc1~230 (remove\nunnecessary initializations, 2006-08-15), since according to ANSI C it\nis redundant.\n\nSo I think the convention is not “if in doubt, the repository has\nversion GIT_REPO_VERSION” but “if the repository lacks a\ncore.repositoryversion setting, it must be really old”.\n\nJonathan\n"},{"id":"137984","messageId":"20100328003416.GA8011@progeny.tock","threadId":"23202","inReplyTo":"1269681184-1992-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] builtins: reset startup_info->have_run_setup_gitdir when unsetting up repository","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-03-28T00:34:16Z","receivedAt":"2010-03-28T00:34:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Nguyễn Thái Ngọc Duy wrote:\n\n>  - alias handling will always be done before help_unknown_cmd()\n>  - alias handling code will search and set up repository if found\n>  - alias handline code will not undo repository setup\n> \n> These ensure that repository will always be set up (or attempted to\n> set up) before help_unknown_cmd(), so there is no issue. But the setup\n> dependency here is subtle. It may break some day if someone reorders\n> the loop, for example.\n> \n> Make a note about this.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n\nMakes sense, thanks.\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nNguyễn Thái Ngọc Duy wrote:\n\n> startup_info->have_run_setup_gitdir is used to guard unallowed access\n> to repository. When a repository has been set up and the real command\n> does not expect any setup, we should revert to the original \"fresh\"\n> state, including startup_info->have_run_setup_gitdir. Otherwise, the\n> next attempt to set up repository will fail.\n[...]\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n\nThe test this adds fails for current master, too.\n\nThe cause: core.worktree gets set to point to the trash directory.\nThis results from a facet of the test I was not paying attention to ---\nthe trash directory is a git repository itself, which poisons git with\nsome extra information it hadn’t learned to forget yet.\n\nThe third and fourth tests below fail in master for this reason.\n\nThe first test passes everywhere; it is just checking basic behavior.\n\nThe second test passes in master and fails in nd/setup without the\ngit.c hunk of your patch.  It is a more targeted test, meant to check\nonly for the problem your patch solves.\n\nOf course, with your patch applied, all four tests pass.  Thanks.\n\nReviewed-and-tested-by: Jonathan Nieder <jrnieder@gmail.com>\n\ndiff --git a/t/t0001-init.sh b/t/t0001-init.sh\nindex 6757734..6d6766e 100755\n--- a/t/t0001-init.sh\n+++ b/t/t0001-init.sh\n@@ -33,6 +33,59 @@ test_expect_success 'plain' '\n \tcheck_config plain/.git false unset\n '\n \n+test_expect_success 'plain nested in bare' '\n+\t(\n+\t\tunset GIT_DIR GIT_WORK_TREE &&\n+\t\tgit init --bare bare-ancestor.git &&\n+\t\tcd bare-ancestor.git &&\n+\t\tmkdir plain-nested &&\n+\t\tcd plain-nested &&\n+\t\tgit init\n+\t) &&\n+\tcheck_config bare-ancestor.git/plain-nested/.git false unset\n+'\n+\n+test_expect_success 'plain through aliased command' '\n+\tmkdir alias-config &&\n+\techo \"[alias] aliasedinit = init\" >alias-config/.gitconfig &&\n+\t(\n+\t\tunset GIT_DIR GIT_WORK_TREE GIT_CONFIG_NOGLOBAL &&\n+\t\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n+\t\tHOME=$(pwd)/alias-config &&\n+\t\texport GIT_CEILING_DIRECTORIES HOME &&\n+\t\tmkdir plain-aliased &&\n+\t\tcd plain-aliased &&\n+\t\tgit aliasedinit\n+\t) &&\n+\tcheck_config plain-aliased/.git false unset\n+'\n+\n+test_expect_success 'plain nested through aliased command' '\n+\t(\n+\t\tunset GIT_DIR GIT_WORK_TREE &&\n+\t\tgit init plain-ancestor-aliased &&\n+\t\tcd plain-ancestor-aliased &&\n+\t\techo \"[alias] aliasedinit = init\" >>.git/config &&\n+\t\tmkdir plain-nested &&\n+\t\tcd plain-nested &&\n+\t\tgit aliasedinit\n+\t) &&\n+\tcheck_config plain-ancestor-aliased/plain-nested/.git false unset\n+'\n+\n+test_expect_success 'plain nested in bare through aliased command' '\n+\t(\n+\t\tunset GIT_DIR GIT_WORK_TREE &&\n+\t\tgit init --bare bare-ancestor-aliased.git &&\n+\t\tcd bare-ancestor-aliased.git &&\n+\t\techo \"[alias] aliasedinit = init\" >>config &&\n+\t\tmkdir plain-nested &&\n+\t\tcd plain-nested &&\n+\t\tgit aliasedinit\n+\t) &&\n+\tcheck_config bare-ancestor-aliased.git/plain-nested/.git false unset\n+'\n+\n test_expect_success 'plain with GIT_WORK_TREE' '\n \tif (\n \t\tunset GIT_DIR\n"},{"id":"137996","messageId":"fcaeb9bf1003280011r4c25a076t22d8759de31c3e3f@mail.gmail.com","threadId":"23202","inReplyTo":"20100327223813.GA5809@progeny.tock","subject":"Re: [PATCH] builtins: reset startup_info->have_run_setup_gitdir when unsetting up repository","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-03-28T07:11:21Z","receivedAt":"2010-03-28T07:11:21Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2010/3/27 Jonathan Nieder <jrnieder@gmail.com>:\n> Hi again,\n>\n> Nguyễn Thái Ngọc Duy wrote:\n>\n>> While at it, also reset repository_format_version to zero. When\n>> omitted, the format should be understood as the last supported\n>> version, i.e. zero. This is probably only used by \"git init\" or \"git\n>> clone\".\n> [...]\n>>       /* Initialized in check_repository_format_version() */\n>> -     repository_format_version = 0xFF;\n>> +     repository_format_version = 0;\n>\n> Good change, but wrong justification in my opinion.\n>\n> v0.99.9l^2~54 (init-db: check template and repository format.,\n> 2005-11-25) taught ‘git init-db’ to make sure that\n>\n>  repository_format_version <= GIT_REPO_VERSION\n>\n> before initializing a new repository.  repository_format_version was\n> being explicitly initialized globally to 0 at the time, presumably to\n> ensure tests like this always succeed when no repository format version\n> is declared (or in other words, the repository format for repositories\n> initialized before git v0.99.9l^2~56 is zero by convention).  As a happy\n> side effect, that default takes care of the “no pre-existing repository”\n> case here.\n>\n> The explicit initialization was removed in commit v1.4.3-rc1~230 (remove\n> unnecessary initializations, 2006-08-15), since according to ANSI C it\n> is redundant.\n>\n> So I think the convention is not “if in doubt, the repository has\n> version GIT_REPO_VERSION” but “if the repository lacks a\n> core.repositoryversion setting, it must be really old”.\n\nMakes sense. Thanks. Will leave a note at the initialization to reduce\nhistory digging time for other people.\n-- \nDuy\n"}]}