[PATCH 0/2] fix ls-files parseopt regression

[PATCH 0/2] fix ls-files parseopt regression

[Jeff King]

These patches are on top of the mv/parseopt-ls-files topic in next. The
first is a related cleanup, the second is the fix.

 1/2: t3000: use test_cmp instead of diff
 2/2: ls-files: fix broken --no-empty-directory

-Peff

[PATCH 1/2] t3000: use test_cmp instead of diff

[Jeff King]

These ancient tests predate test_cmp.

While we're at it, let's switch to our usual "expected
before actual" order of arguments; this makes the diff
output "here's what is changed from expected" instead of the
reverse.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t3000-ls-files-others.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t3000-ls-files-others.sh b/t/t3000-ls-files-others.sh
index bc0a351..36eee0f 100755
--- a/t/t3000-ls-files-others.sh
+++ b/t/t3000-ls-files-others.sh
@@ -42,7 +42,7 @@ test_expect_success \
 
 test_expect_success \
     'git ls-files --others should pick up symlinks.' \
-    'diff output expected1'
+    'test_cmp expected1 output'
 
 test_expect_success \
     'git ls-files --others --directory to show output.' \
@@ -51,6 +51,6 @@ test_expect_success \
 
 test_expect_success \
     'git ls-files --others --directory should not get confused.' \
-    'diff output expected2'
+    'test_cmp expected2 output'
 
 test_done
-- 
1.6.2.198.ge2a58

[PATCH 2/2] ls-files: fix broken --no-empty-directory

[Jeff King]

Commit ce8e880 converted ls-files to use parseopt; the
--no-empty-directory option was converted as an
OPT_BIT for "empty-directory" to set the
DIR_HIDE_EMPTY_DIRECTORY flag. However, that makes it do the
opposite of what it should: --empty-directory would hide,
but --no-empty-directory would turn off hiding.

Signed-off-by: Jeff King <peff@peff.net>
---
Two comments on this patch:

  1. The original conversion gave us --empty-directory and
     --no-empty-directory. This one gives us --no-empty-directory and
     --no-no-empty-directory. If we really want to expose a negatable
     flag, then either:

       - the bit needs to change to DIR_SHOW_EMPTY_DIRECTORY; however,
         that changes the behavior for callers who zero the flag field

       - we need a custom option which implements the negation with
         respect to the bit, instead of a vanilla OPT_BIT

  2. I tried to follow the existing style of the t3000 test script
     rather than overhauling it. However, I think the result is a little
     hard to read. We set up expectations for many cases at the
     beginning, and then do all the tests. I think it might read better
     as "set up a case, create expectat, run test".

 builtin-ls-files.c         |    4 ++--
 t/t3000-ls-files-others.sh |   14 +++++++++++++-
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/builtin-ls-files.c b/builtin-ls-files.c
index 1742c0f..437c366 100644
--- a/builtin-ls-files.c
+++ b/builtin-ls-files.c
@@ -454,8 +454,8 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 		OPT_BIT(0, "directory", &dir.flags,
 			"show 'other' directories' name only",
 			DIR_SHOW_OTHER_DIRECTORIES),
-		OPT_BIT(0, "empty-directory", &dir.flags,
-			"list empty directories",
+		OPT_BIT(0, "no-empty-directory", &dir.flags,
+			"don't show empty directories",
 			DIR_HIDE_EMPTY_DIRECTORIES),
 		OPT_BOOLEAN('u', "unmerged", &show_unmerged,
 			"show unmerged files in the output"),
diff --git a/t/t3000-ls-files-others.sh b/t/t3000-ls-files-others.sh
index 36eee0f..379d963 100755
--- a/t/t3000-ls-files-others.sh
+++ b/t/t3000-ls-files-others.sh
@@ -13,12 +13,13 @@ filesystem.
     path2/file2 - a file in a directory
     path3-junk  - a file to confuse things
     path3/file3 - a file in a directory
+    path4       - an empty directory
 '
 . ./test-lib.sh
 
 date >path0
 ln -s xyzzy path1
-mkdir path2 path3
+mkdir path2 path3 path4
 date >path2/file2
 date >path2-junk
 date >path3/file3
@@ -28,6 +29,7 @@ git update-index --add path3-junk path3/file3
 cat >expected1 <<EOF
 expected1
 expected2
+expected3
 output
 path0
 path1
@@ -35,6 +37,8 @@ path2-junk
 path2/file2
 EOF
 sed -e 's|path2/file2|path2/|' <expected1 >expected2
+cat <expected2 >expected3
+echo path4/ >>expected2
 
 test_expect_success \
     'git ls-files --others to show output.' \
@@ -53,4 +57,12 @@ test_expect_success \
     'git ls-files --others --directory should not get confused.' \
     'test_cmp expected2 output'
 
+test_expect_success \
+    'git ls-files --others --directory --no-empty-directory to show output.' \
+    'git ls-files --others --directory --no-empty-directory >output'
+
+test_expect_success \
+    '--no-empty-directory hides empty directory' \
+    'test_cmp expected3 output'
+
 test_done
-- 
1.6.2.198.ge2a58

Re: [PATCH 2/2] ls-files: fix broken --no-empty-directory

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 831 bytes --]

On Sat, Mar 07, 2009 at 08:27:22PM -0500, Jeff King <peff@peff.net> wrote:
> diff --git a/builtin-ls-files.c b/builtin-ls-files.c
> index 1742c0f..437c366 100644
> --- a/builtin-ls-files.c
> +++ b/builtin-ls-files.c
> @@ -454,8 +454,8 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
>  		OPT_BIT(0, "directory", &dir.flags,
>  			"show 'other' directories' name only",
>  			DIR_SHOW_OTHER_DIRECTORIES),
> -		OPT_BIT(0, "empty-directory", &dir.flags,
> -			"list empty directories",
> +		OPT_BIT(0, "no-empty-directory", &dir.flags,
> +			"don't show empty directories",
>  			DIR_HIDE_EMPTY_DIRECTORIES),
>  		OPT_BOOLEAN('u', "unmerged", &show_unmerged,
>  			"show unmerged files in the output"),

Thanks for catching this. But then why not using PARSE_OPT_NONEG?

That would avoid --no-no-empty-directory.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 2/2] ls-files: fix broken --no-empty-directory

[Jeff King]

On Sun, Mar 08, 2009 at 10:13:12PM +0100, Miklos Vajna wrote:

> > +		OPT_BIT(0, "no-empty-directory", &dir.flags,
> > +			"don't show empty directories",
> >  			DIR_HIDE_EMPTY_DIRECTORIES),
> >  		OPT_BOOLEAN('u', "unmerged", &show_unmerged,
> >  			"show unmerged files in the output"),
> 
> Thanks for catching this. But then why not using PARSE_OPT_NONEG?
> 
> That would avoid --no-no-empty-directory.

I think either we don't care about negation, in which case it is not
hurting anybody to support --no-no-empty-directory, or we do, in which
case you actually want to do the negation properly. Which would be
something like:

diff --git a/builtin-ls-files.c b/builtin-ls-files.c
index 437c366..2c5f7a5 100644
--- a/builtin-ls-files.c
+++ b/builtin-ls-files.c
@@ -427,6 +427,7 @@ static int option_parse_exclude_standard(const struct option *opt,
 int cmd_ls_files(int argc, const char **argv, const char *prefix)
 {
 	int require_work_tree = 0, show_tag = 0;
+	int show_empty_directories = 1;
 	struct dir_struct dir;
 	struct option builtin_ls_files_options[] = {
 		{ OPTION_CALLBACK, 'z', NULL, NULL, NULL,
@@ -454,9 +455,8 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 		OPT_BIT(0, "directory", &dir.flags,
 			"show 'other' directories' name only",
 			DIR_SHOW_OTHER_DIRECTORIES),
-		OPT_BIT(0, "no-empty-directory", &dir.flags,
-			"don't show empty directories",
-			DIR_HIDE_EMPTY_DIRECTORIES),
+		OPT_BOOLEAN(0, "empty-directory", &show_empty_directories,
+			"show empty directories (on by default)"),
 		OPT_BOOLEAN('u', "unmerged", &show_unmerged,
 			"show unmerged files in the output"),
 		{ OPTION_CALLBACK, 'x', "exclude", &dir.exclude_list[EXC_CMDL], "pattern",
@@ -506,6 +506,8 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 		show_stage = 1;
 	if (dir.exclude_per_dir)
 		exc_given = 1;
+	if (!show_empty_directories)
+		dir.flags |= DIR_HIDE_EMPTY_DIRECTORIES;
 
 	if (require_work_tree && !is_inside_work_tree())
 		setup_work_tree();


which is even still a little confusing, as you get "--empty-directory"
in the usage message. But you would almost never want to use that, as it
is already the default.

-Peff

Re: [PATCH 2/2] ls-files: fix broken --no-empty-directory

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 487 bytes --]

On Tue, Mar 10, 2009 at 03:11:11PM -0400, Jeff King <peff@peff.net> wrote:
> which is even still a little confusing, as you get "--empty-directory"
> in the usage message. But you would almost never want to use that, as it
> is already the default.

Exactly, that's why I suggested the usage of PARSE_OPT_NONEG, which
would avoid a new no-op option. ;-)

But I'm fine with the above patch as well, in case having the "no-"
prefix in an option name is considered as an improper negation.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]