{"thread":{"id":"21156","subject":"[PATCH] ls-files: make option parser keep argv[0]","startedAt":"2009-10-08T14:20:28Z","lastAt":"2009-10-09T06:53:12Z","messageCount":4,"participants":["Ben Walton","Stephen Boyd","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"124380","messageId":"1255011628-31841-1-git-send-email-bwalton@artsci.utoronto.ca","threadId":"21156","inReplyTo":null,"subject":"[PATCH] ls-files: make option parser keep argv[0]","fromName":"Ben Walton","fromEmail":"bwalton@artsci.utoronto.ca","sentAt":"2009-10-08T14:20:28Z","receivedAt":"2009-10-08T14:20:28Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"The ls-files built-in was not asking the option parser to maintain\nargv[0]. This led to the possibility of fprintf(stderr, \"...\", NULL).\nOn Solaris, this was causing a segfault. On glibc systems, printed\nerror messages didn't contain proper strings, but rather, \"(null)\":...\n\nA trigger for this bug was: `git ls-files -i`\n\nSigned-off-by: Ben Walton \n---\n builtin-ls-files.c | 4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-ls-files.c b/builtin-ls-files.c\nindex f473220..9a3705a 100644\n--- a/builtin-ls-files.c\n+++ b/builtin-ls-files.c\n@@ -482,7 +482,7 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)\n \tgit_config(git_default_config, NULL);\n \n \targc = parse_options(argc, argv, prefix, builtin_ls_files_options,\n-\t\t\tls_files_usage, 0);\n+\t\t\tls_files_usage, PARSE_OPT_KEEP_ARGV0);\n \tif (show_tag || show_valid_bit) {\n \t\ttag_cached = \"H \";\n \t\ttag_unmerged = \"M \";\n@@ -505,7 +505,7 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)\n \tif (require_work_tree && !is_inside_work_tree())\n \t\tsetup_work_tree();\n \n-\tpathspec = get_pathspec(prefix, argv);\n+\tpathspec = get_pathspec(prefix, argv + 1);\n \n \t/* be nice with submodule paths ending in a slash */\n \tread_cache();\n-- \n1.6.4.4\n"},{"id":"124403","messageId":"4ACE4C72.4050400@gmail.com","threadId":"21156","inReplyTo":"1255011628-31841-1-git-send-email-bwalton@artsci.utoronto.ca","subject":"Re: [PATCH] ls-files: make option parser keep argv[0]","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2009-10-08T20:32:50Z","receivedAt":"2009-10-08T20:32:50Z","isPatch":true,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"Ben Walton wrote:\n> The ls-files built-in was not asking the option parser to maintain\n> argv[0]. This led to the possibility of fprintf(stderr, \"...\", NULL).\n> On Solaris, this was causing a segfault. On glibc systems, printed\n> error messages didn't contain proper strings, but rather, \"(null)\":...\n>\n> A trigger for this bug was: `git ls-files -i`\n>\n> Signed-off-by: Ben Walton \n>\n\nPatch looks good.\n\nJust a thought, maybe we should change the fprintf(stderr,...) and\nexit(1) call to a die() and replace the argv[0] with \"ls-files\" similar\nto the die() on line 546. Then your diffstat becomes -1 instead of 0.\n"},{"id":"124416","messageId":"1255053215-14059-1-git-send-email-bwalton@artsci.utoronto.ca","threadId":"21156","inReplyTo":"4ACE4C72.4050400@gmail.com","subject":"[PATCH] ls-files: die instead of fprintf/exit in -i error","fromName":"Ben Walton","fromEmail":"bwalton@artsci.utoronto.ca","sentAt":"2009-10-09T01:53:35Z","receivedAt":"2009-10-09T01:53:35Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"When ls-files was called with -i but no exclude pattern, it was\ncalling fprintf(stderr, \"...\", NULL) and then exiting. On Solaris,\npassing NULL into fprintf was causing a segfault. On glibc systems,\nit was simply producing incorrect output (eg: \"(null)\": ...). The\nNULL pointer was a result of argv[0] not being preserved by the option\nparser. Instead of requesting that the option parser preserve\nargv[0], use die() with a constant string.\n\nA trigger for this bug was: `git ls-files -i`\n\nSigned-off-by: Ben Walton \n---\nThis is the alternate solution to this bug as proposed earlier today.\nI don't have a preference either way for which solution is better or\nmore inline with the 'git way,' so please choose the most appropriate.\n\nI've run the test suite with both patches on Linux and Solaris\nand everything passes.\n\n builtin-ls-files.c | 7 ++-----\n 1 files changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin-ls-files.c b/builtin-ls-files.c\nindex f473220..2c95ca6 100644\n--- a/builtin-ls-files.c\n+++ b/builtin-ls-files.c\n@@ -524,11 +524,8 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)\n \t\tps_matched = xcalloc(1, num);\n \t}\n \n-\tif ((dir.flags & DIR_SHOW_IGNORED) && !exc_given) {\n-\t\tfprintf(stderr, \"%s: --ignored needs some exclude pattern\\n\",\n-\t\t\targv[0]);\n-\t\texit(1);\n-\t}\n+\tif ((dir.flags & DIR_SHOW_IGNORED) && !exc_given)\n+\t\tdie(\"ls-files --ignored needs some exclude pattern\");\n \n \t/* With no flags, we default to showing the cached files */\n \tif (!(show_stage | show_deleted | show_others | show_unmerged |\n-- \n1.6.4.4\n"},{"id":"124434","messageId":"7v1vld5a93.fsf@alter.siamese.dyndns.org","threadId":"21156","inReplyTo":"1255053215-14059-1-git-send-email-bwalton@artsci.utoronto.ca","subject":"Re: [PATCH] ls-files: die instead of fprintf/exit in -i error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-09T06:53:12Z","receivedAt":"2009-10-09T06:53:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Walton writes:\n\n> This is the alternate solution to this bug as proposed earlier today.\n> I don't have a preference either way for which solution is better or\n> more inline with the 'git way,' so please choose the most appropriate.\n\nI think die() is better for consistency reasons if nothing else.\n\nThanks.\n"}]}