{"thread":{"id":"23178","subject":"[PATCH 1/2] builtins: do not commit pager choice early","startedAt":"2010-03-25T13:38:14Z","lastAt":"2010-03-26T20:58:48Z","messageCount":4,"participants":["Nguyễn Thái Ngọc Duy","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"137811","messageId":"1269524295-24569-1-git-send-email-pclouds@gmail.com","threadId":"23178","inReplyTo":null,"subject":"[PATCH 1/2] builtins: do not commit pager choice early","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-03-25T13:38:14Z","receivedAt":"2010-03-25T13:38:14Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"From: Duy Nguyen \n\nCommitting pager choice may require setting up the pager, which\nwill need access to repository.\n\nAt the time after handle_options() is called, the repository has not\nbeen found yet. As a result, unallowed access to repository may\nhappen.\n\nThere are several possible code path after\nhandle_options()/commit_pager_choice() is called:\n\n1. list_common_cmds_help()\n2. run_argv()\n3. help_unknown_cmd()\n\nCase 2. will have commit_pager_choice() called inside run_builtin() if\na command is found. Case 1. and 3. won't need a pager, it's short\nprintout and should be fitted within a screen. So, removing\ncommit_pager_choice() call after handle_options() is safe.\n\nSigned-off-by: Duy Nguyen \n---\n On top of nd/setup. This may help fix the breakage in t7006. And forget t9100\n breakage I mentioned elsewhere. My system is broken, git-svn dies when it \"exit 0;\"\n and even git one year ago does not fix it.\n\n git.c | 1 -\n 1 files changed, 0 insertions(+), 1 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 3798791..bd1d4bb 100644\n--- a/git.c\n+++ b/git.c\n@@ -514,7 +514,6 @@ int main(int argc, const char **argv)\n \targv++;\n \targc--;\n \thandle_options(&argv, &argc, NULL);\n-\tcommit_pager_choice();\n \tif (argc > 0) {\n \t\tif (!prefixcmp(argv[0], \"--\"))\n \t\t\targv[0] += 2;\n-- \n1.7.0.rc1.541.g2da82.dirty\n"},{"id":"137812","messageId":"1269524295-24569-2-git-send-email-pclouds@gmail.com","threadId":"23178","inReplyTo":"1269524295-24569-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/2] builtins: setup repository before print unknown command error","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-03-25T13:38:15Z","receivedAt":"2010-03-25T13:38:15Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"From: Duy Nguyen \n\nhelp_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 the repository setup explicit, to express it clearer, although\nthis code will never be run (It does not mean the code is not tested,\nI removed alias handling code to reproduce the problem, and this code\nhelped fix it)\n\nSigned-off-by: Duy Nguyen \n---\n git.c | 4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex bd1d4bb..d84996c 100644\n--- a/git.c\n+++ b/git.c\n@@ -547,6 +547,10 @@ int main(int argc, const char **argv)\n \t\t\texit(1);\n \t\t}\n \t\tif (!done_help) {\n+\t\t\tif (!startup_info->have_run_setup_gitdir) {\n+\t\t\t\tint nongit_ok;\n+\t\t\t\tsetup_git_directory_gently(&nongit_ok);\n+\t\t\t}\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":"137891","messageId":"20100326200347.GA11395@progeny.tock","threadId":"23178","inReplyTo":"1269524295-24569-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 1/2] builtins: do not commit pager choice early","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-03-26T20:03:47Z","receivedAt":"2010-03-26T20:03:47Z","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> Committing pager choice may require setting up the pager, which\n> will need access to repository.\n[...]\n> Signed-off-by: Duy Nguyen \n\nTested-by: Jonathan Nieder \n\nWithout this patch, I get\n\n| * expecting success: \n| test_terminal git --paginate rev-list HEAD &&\n| test -e paginated.out\n| \n| fatal: internal error: access to .git/config without repo setup\n| * FAIL 7: git --paginate rev-list uses a pager\n\nand with it, t7006 passes again. I assume on your system that\ntest was being skipped when running tests without -v (confusing,\nyes) because that test needs a tty but IO::Pty from CPAN is not\npresent.\n\n> There are several possible code path after\n> handle_options()/commit_pager_choice() is called:\n> \n> 1. list_common_cmds_help()\n> 2. run_argv()\n> 3. help_unknown_cmd()\n> \n> Case 2. will have commit_pager_choice() called inside run_builtin() if\n> a command is found. Case 1. and 3. won't need a pager, it's short\n> printout and should be fitted within a screen. So, removing\n> commit_pager_choice() call after handle_options() is safe.\n\nMakes sense.\n\nThanks,\nJonathan\n"},{"id":"137901","messageId":"20100326205848.GB11395@progeny.tock","threadId":"23178","inReplyTo":"1269524295-24569-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 2/2] builtins: setup repository before print unknown command error","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-03-26T20:58:48Z","receivedAt":"2010-03-26T20:58:48Z","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> As 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> \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\nIt took me a while to figure out what was going on here. I think it is\ntoo complicated. Could it make sense to use\n\n\t/*\n\t * help_unknown_cmd() requires that the repository has searched\n\t * for and set up if found.\n\t * Luckily, the alias handling code already took care of this.\n\t */\n\tif (!startup_info->have_run_setup_gitdir)\n\t\tdie(\"internal error handling unknown command\");\n\ninstead, to document the current assumption?\n\nAlternatively, one might add a new ensure_git_directory_is_set_up() function\nthat only runs setup_git_repository_gently if it has not already been run.\nI do not like that so much --- too easy to hide bugs.\n\nBetter to run setup_git_repository_gently once unconditionally, before\nhandle_argv. Unfortunately, that breaks git-init. Actually, the current\nseries breaks git-init when run through an alias.\n\nThoughts?\n\ndiff --git a/t/t0001-init.sh b/t/t0001-init.sh\nindex 6757734..792abe2 100755\n--- a/t/t0001-init.sh\n+++ b/t/t0001-init.sh\n@@ -33,6 +33,21 @@ test_expect_success 'plain' '\n \tcheck_config plain/.git false unset\n '\n \n+test_expect_failure '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"}]}