* [PATCH 0/2] fix ls-files parseopt regression @ 2009-03-08 1:20 Jeff King 2009-03-08 1:22 ` [PATCH 1/2] t3000: use test_cmp instead of diff Jeff King 2009-03-08 1:27 ` [PATCH 2/2] ls-files: fix broken --no-empty-directory Jeff King 0 siblings, 2 replies; 6+ messages in thread From: Jeff King @ 2009-03-08 1:20 UTC (permalink / raw) To: Junio C Hamano; +Cc: Miklos Vajna, git 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] t3000: use test_cmp instead of diff 2009-03-08 1:20 [PATCH 0/2] fix ls-files parseopt regression Jeff King @ 2009-03-08 1:22 ` Jeff King 2009-03-08 1:27 ` [PATCH 2/2] ls-files: fix broken --no-empty-directory Jeff King 1 sibling, 0 replies; 6+ messages in thread From: Jeff King @ 2009-03-08 1:22 UTC (permalink / raw) To: Junio C Hamano; +Cc: Miklos Vajna, git 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] ls-files: fix broken --no-empty-directory 2009-03-08 1:20 [PATCH 0/2] fix ls-files parseopt regression Jeff King 2009-03-08 1:22 ` [PATCH 1/2] t3000: use test_cmp instead of diff Jeff King @ 2009-03-08 1:27 ` Jeff King 2009-03-08 21:13 ` Miklos Vajna 1 sibling, 1 reply; 6+ messages in thread From: Jeff King @ 2009-03-08 1:27 UTC (permalink / raw) To: Junio C Hamano; +Cc: Miklos Vajna, git 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ls-files: fix broken --no-empty-directory 2009-03-08 1:27 ` [PATCH 2/2] ls-files: fix broken --no-empty-directory Jeff King @ 2009-03-08 21:13 ` Miklos Vajna 2009-03-10 19:11 ` Jeff King 0 siblings, 1 reply; 6+ messages in thread From: Miklos Vajna @ 2009-03-08 21:13 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, git [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ls-files: fix broken --no-empty-directory 2009-03-08 21:13 ` Miklos Vajna @ 2009-03-10 19:11 ` Jeff King 2009-03-10 22:16 ` Miklos Vajna 0 siblings, 1 reply; 6+ messages in thread From: Jeff King @ 2009-03-10 19:11 UTC (permalink / raw) To: Miklos Vajna; +Cc: Junio C Hamano, git 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ls-files: fix broken --no-empty-directory 2009-03-10 19:11 ` Jeff King @ 2009-03-10 22:16 ` Miklos Vajna 0 siblings, 0 replies; 6+ messages in thread From: Miklos Vajna @ 2009-03-10 22:16 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, git [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-03-10 22:18 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-03-08 1:20 [PATCH 0/2] fix ls-files parseopt regression Jeff King 2009-03-08 1:22 ` [PATCH 1/2] t3000: use test_cmp instead of diff Jeff King 2009-03-08 1:27 ` [PATCH 2/2] ls-files: fix broken --no-empty-directory Jeff King 2009-03-08 21:13 ` Miklos Vajna 2009-03-10 19:11 ` Jeff King 2009-03-10 22:16 ` Miklos Vajna
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).