{"thread":{"id":"18210","subject":"[PATCH 0/2] fix ls-files parseopt regression","startedAt":"2009-03-08T01:20:50Z","lastAt":"2009-03-10T22:16:48Z","messageCount":6,"participants":["Jeff King","Miklos Vajna"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"107364","messageId":"20090308012049.GA18616@coredump.intra.peff.net","threadId":"18210","inReplyTo":null,"subject":"[PATCH 0/2] fix ls-files parseopt regression","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-03-08T01:20:50Z","receivedAt":"2009-03-08T01:20:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These patches are on top of the mv/parseopt-ls-files topic in next. The\nfirst is a related cleanup, the second is the fix.\n\n 1/2: t3000: use test_cmp instead of diff\n 2/2: ls-files: fix broken --no-empty-directory\n\n-Peff\n"},{"id":"107366","messageId":"20090308012206.GA18714@coredump.intra.peff.net","threadId":"18210","inReplyTo":"20090308012049.GA18616@coredump.intra.peff.net","subject":"[PATCH 1/2] t3000: use test_cmp instead of diff","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-03-08T01:22:07Z","receivedAt":"2009-03-08T01:22:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These ancient tests predate test_cmp.\n\nWhile we're at it, let's switch to our usual \"expected\nbefore actual\" order of arguments; this makes the diff\noutput \"here's what is changed from expected\" instead of the\nreverse.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t3000-ls-files-others.sh |    4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t3000-ls-files-others.sh b/t/t3000-ls-files-others.sh\nindex bc0a351..36eee0f 100755\n--- a/t/t3000-ls-files-others.sh\n+++ b/t/t3000-ls-files-others.sh\n@@ -42,7 +42,7 @@ test_expect_success \\\n \n test_expect_success \\\n     'git ls-files --others should pick up symlinks.' \\\n-    'diff output expected1'\n+    'test_cmp expected1 output'\n \n test_expect_success \\\n     'git ls-files --others --directory to show output.' \\\n@@ -51,6 +51,6 @@ test_expect_success \\\n \n test_expect_success \\\n     'git ls-files --others --directory should not get confused.' \\\n-    'diff output expected2'\n+    'test_cmp expected2 output'\n \n test_done\n-- \n1.6.2.198.ge2a58\n"},{"id":"107368","messageId":"20090308012722.GB18714@coredump.intra.peff.net","threadId":"18210","inReplyTo":"20090308012049.GA18616@coredump.intra.peff.net","subject":"[PATCH 2/2] ls-files: fix broken --no-empty-directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-03-08T01:27:22Z","receivedAt":"2009-03-08T01:27:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit ce8e880 converted ls-files to use parseopt; the\n--no-empty-directory option was converted as an\nOPT_BIT for \"empty-directory\" to set the\nDIR_HIDE_EMPTY_DIRECTORY flag. However, that makes it do the\nopposite of what it should: --empty-directory would hide,\nbut --no-empty-directory would turn off hiding.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nTwo comments on this patch:\n\n  1. The original conversion gave us --empty-directory and\n     --no-empty-directory. This one gives us --no-empty-directory and\n     --no-no-empty-directory. If we really want to expose a negatable\n     flag, then either:\n\n       - the bit needs to change to DIR_SHOW_EMPTY_DIRECTORY; however,\n         that changes the behavior for callers who zero the flag field\n\n       - we need a custom option which implements the negation with\n         respect to the bit, instead of a vanilla OPT_BIT\n\n  2. I tried to follow the existing style of the t3000 test script\n     rather than overhauling it. However, I think the result is a little\n     hard to read. We set up expectations for many cases at the\n     beginning, and then do all the tests. I think it might read better\n     as \"set up a case, create expectat, run test\".\n\n builtin-ls-files.c         |    4 ++--\n t/t3000-ls-files-others.sh |   14 +++++++++++++-\n 2 files changed, 15 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-ls-files.c b/builtin-ls-files.c\nindex 1742c0f..437c366 100644\n--- a/builtin-ls-files.c\n+++ b/builtin-ls-files.c\n@@ -454,8 +454,8 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)\n \t\tOPT_BIT(0, \"directory\", &dir.flags,\n \t\t\t\"show 'other' directories' name only\",\n \t\t\tDIR_SHOW_OTHER_DIRECTORIES),\n-\t\tOPT_BIT(0, \"empty-directory\", &dir.flags,\n-\t\t\t\"list empty directories\",\n+\t\tOPT_BIT(0, \"no-empty-directory\", &dir.flags,\n+\t\t\t\"don't show empty directories\",\n \t\t\tDIR_HIDE_EMPTY_DIRECTORIES),\n \t\tOPT_BOOLEAN('u', \"unmerged\", &show_unmerged,\n \t\t\t\"show unmerged files in the output\"),\ndiff --git a/t/t3000-ls-files-others.sh b/t/t3000-ls-files-others.sh\nindex 36eee0f..379d963 100755\n--- a/t/t3000-ls-files-others.sh\n+++ b/t/t3000-ls-files-others.sh\n@@ -13,12 +13,13 @@ filesystem.\n     path2/file2 - a file in a directory\n     path3-junk  - a file to confuse things\n     path3/file3 - a file in a directory\n+    path4       - an empty directory\n '\n . ./test-lib.sh\n \n date >path0\n ln -s xyzzy path1\n-mkdir path2 path3\n+mkdir path2 path3 path4\n date >path2/file2\n date >path2-junk\n date >path3/file3\n@@ -28,6 +29,7 @@ git update-index --add path3-junk path3/file3\n cat >expected1 <<EOF\n expected1\n expected2\n+expected3\n output\n path0\n path1\n@@ -35,6 +37,8 @@ path2-junk\n path2/file2\n EOF\n sed -e 's|path2/file2|path2/|' <expected1 >expected2\n+cat <expected2 >expected3\n+echo path4/ >>expected2\n \n test_expect_success \\\n     'git ls-files --others to show output.' \\\n@@ -53,4 +57,12 @@ test_expect_success \\\n     'git ls-files --others --directory should not get confused.' \\\n     'test_cmp expected2 output'\n \n+test_expect_success \\\n+    'git ls-files --others --directory --no-empty-directory to show output.' \\\n+    'git ls-files --others --directory --no-empty-directory >output'\n+\n+test_expect_success \\\n+    '--no-empty-directory hides empty directory' \\\n+    'test_cmp expected3 output'\n+\n test_done\n-- \n1.6.2.198.ge2a58\n"},{"id":"107405","messageId":"20090308211312.GE4371@genesis.frugalware.org","threadId":"18210","inReplyTo":"20090308012722.GB18714@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] ls-files: fix broken --no-empty-directory","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2009-03-08T21:13:12Z","receivedAt":"2009-03-08T21:13:12Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Sat, Mar 07, 2009 at 08:27:22PM -0500, Jeff King <peff@peff.net> wrote:\n> diff --git a/builtin-ls-files.c b/builtin-ls-files.c\n> index 1742c0f..437c366 100644\n> --- a/builtin-ls-files.c\n> +++ b/builtin-ls-files.c\n> @@ -454,8 +454,8 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)\n>  \t\tOPT_BIT(0, \"directory\", &dir.flags,\n>  \t\t\t\"show 'other' directories' name only\",\n>  \t\t\tDIR_SHOW_OTHER_DIRECTORIES),\n> -\t\tOPT_BIT(0, \"empty-directory\", &dir.flags,\n> -\t\t\t\"list empty directories\",\n> +\t\tOPT_BIT(0, \"no-empty-directory\", &dir.flags,\n> +\t\t\t\"don't show empty directories\",\n>  \t\t\tDIR_HIDE_EMPTY_DIRECTORIES),\n>  \t\tOPT_BOOLEAN('u', \"unmerged\", &show_unmerged,\n>  \t\t\t\"show unmerged files in the output\"),\n\nThanks for catching this. But then why not using PARSE_OPT_NONEG?\n\nThat would avoid --no-no-empty-directory.\n"},{"id":"107601","messageId":"20090310191111.GA27662@sigill.intra.peff.net","threadId":"18210","inReplyTo":"20090308211312.GE4371@genesis.frugalware.org","subject":"Re: [PATCH 2/2] ls-files: fix broken --no-empty-directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-03-10T19:11:11Z","receivedAt":"2009-03-10T19:11:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 08, 2009 at 10:13:12PM +0100, Miklos Vajna wrote:\n\n> > +\t\tOPT_BIT(0, \"no-empty-directory\", &dir.flags,\n> > +\t\t\t\"don't show empty directories\",\n> >  \t\t\tDIR_HIDE_EMPTY_DIRECTORIES),\n> >  \t\tOPT_BOOLEAN('u', \"unmerged\", &show_unmerged,\n> >  \t\t\t\"show unmerged files in the output\"),\n> \n> Thanks for catching this. But then why not using PARSE_OPT_NONEG?\n> \n> That would avoid --no-no-empty-directory.\n\nI think either we don't care about negation, in which case it is not\nhurting anybody to support --no-no-empty-directory, or we do, in which\ncase you actually want to do the negation properly. Which would be\nsomething like:\n\ndiff --git a/builtin-ls-files.c b/builtin-ls-files.c\nindex 437c366..2c5f7a5 100644\n--- a/builtin-ls-files.c\n+++ b/builtin-ls-files.c\n@@ -427,6 +427,7 @@ static int option_parse_exclude_standard(const struct option *opt,\n int cmd_ls_files(int argc, const char **argv, const char *prefix)\n {\n \tint require_work_tree = 0, show_tag = 0;\n+\tint show_empty_directories = 1;\n \tstruct dir_struct dir;\n \tstruct option builtin_ls_files_options[] = {\n \t\t{ OPTION_CALLBACK, 'z', NULL, NULL, NULL,\n@@ -454,9 +455,8 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)\n \t\tOPT_BIT(0, \"directory\", &dir.flags,\n \t\t\t\"show 'other' directories' name only\",\n \t\t\tDIR_SHOW_OTHER_DIRECTORIES),\n-\t\tOPT_BIT(0, \"no-empty-directory\", &dir.flags,\n-\t\t\t\"don't show empty directories\",\n-\t\t\tDIR_HIDE_EMPTY_DIRECTORIES),\n+\t\tOPT_BOOLEAN(0, \"empty-directory\", &show_empty_directories,\n+\t\t\t\"show empty directories (on by default)\"),\n \t\tOPT_BOOLEAN('u', \"unmerged\", &show_unmerged,\n \t\t\t\"show unmerged files in the output\"),\n \t\t{ OPTION_CALLBACK, 'x', \"exclude\", &dir.exclude_list[EXC_CMDL], \"pattern\",\n@@ -506,6 +506,8 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)\n \t\tshow_stage = 1;\n \tif (dir.exclude_per_dir)\n \t\texc_given = 1;\n+\tif (!show_empty_directories)\n+\t\tdir.flags |= DIR_HIDE_EMPTY_DIRECTORIES;\n \n \tif (require_work_tree && !is_inside_work_tree())\n \t\tsetup_work_tree();\n\n\nwhich is even still a little confusing, as you get \"--empty-directory\"\nin the usage message. But you would almost never want to use that, as it\nis already the default.\n\n-Peff\n"},{"id":"107616","messageId":"20090310221648.GB4371@genesis.frugalware.org","threadId":"18210","inReplyTo":"20090310191111.GA27662@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] ls-files: fix broken --no-empty-directory","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2009-03-10T22:16:48Z","receivedAt":"2009-03-10T22:16:48Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Tue, Mar 10, 2009 at 03:11:11PM -0400, Jeff King <peff@peff.net> wrote:\n> which is even still a little confusing, as you get \"--empty-directory\"\n> in the usage message. But you would almost never want to use that, as it\n> is already the default.\n\nExactly, that's why I suggested the usage of PARSE_OPT_NONEG, which\nwould avoid a new no-op option. ;-)\n\nBut I'm fine with the above patch as well, in case having the \"no-\"\nprefix in an option name is considered as an improper negation.\n"}]}