Re: [PATCH v7 4/5] dir_iterator: refactor state machine model

[Michael Haggerty <mhagger@alum.mit.edu>]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
Daniel,

> Although it does make tests harder to understand, if we were to
> specify how to iterate with human-readable flags we'd add the getopt()
> + flag configuration overhead to this helper program to be able to
> handle all cases properly. Additionally, new flags added to
> dir_iterator would have to edit the test program as well, generating
> extra work.

I think you're exaggerating the amount of extra work. I think all you
need to do is squash the attached patch into your patch 4/5, for the
gain of only 14 lines of simple code, four of which could be deleted
if you don't care about supporting "--". Supporting hypothetical
future new options would cost exactly two more lines for each option.
Plus, this also fixes the handling of more than two args.

It might be even shorter if you use `parse_options()`, but that seems
like overkill here.

On the plus side, anybody can now change the `DIR_ITERATOR_*`
constants without breaking things, or grep for them to find all of the
places that they are used. Plus the test code is more readable.

In my opinion that is a win.

Michael

 t/helper/test-dir-iterator.c | 28 +++++++++++++++++++++-------
 t/t0065-dir-iterator.sh      | 10 +++++-----
 2 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/t/helper/test-dir-iterator.c b/t/helper/test-dir-iterator.c
index a1b8c78434..c2eb2ca1f9 100644
--- a/t/helper/test-dir-iterator.c
+++ b/t/helper/test-dir-iterator.c
@@ -4,18 +4,32 @@
 #include "dir-iterator.h"
 
 int cmd_main(int argc, const char **argv) {
+	const char **myargv = argv;
+	int myargc = argc;
 	struct strbuf path = STRBUF_INIT;
 	struct dir_iterator *diter;
-	unsigned flag = DIR_ITERATOR_PRE_ORDER_TRAVERSAL;
-
-	if (argc < 2) {
-		return 1;
+	unsigned flag = 0;
+
+	while (--myargc && starts_with(*++myargv, "--")) {
+		if (!strcmp(*myargv, "--pre-order")) {
+			flag |= DIR_ITERATOR_PRE_ORDER_TRAVERSAL;
+		} else if (!strcmp(*myargv, "--post-order")) {
+			flag |= DIR_ITERATOR_POST_ORDER_TRAVERSAL;
+		} else if (!strcmp(*myargv, "--list-root-dir")) {
+			flag |= DIR_ITERATOR_LIST_ROOT_DIR;
+		} else if (!strcmp(*myargv, "--")) {
+			myargc--;
+			myargv++;
+			break;
+		} else {
+			die("Unrecognized option: %s", *myargv);
+		}
 	}
 
-	strbuf_add(&path, argv[1], strlen(argv[1]));
+	if (myargc != 1)
+		die("expected exactly one non-option argument");
 
-	if (argc == 3)
-		flag = atoi(argv[2]);
+	strbuf_addstr(&path, *myargv);
 
 	diter = dir_iterator_begin(path.buf, flag);
 
diff --git a/t/t0065-dir-iterator.sh b/t/t0065-dir-iterator.sh
index ade3ee0e7e..4819e6181d 100755
--- a/t/t0065-dir-iterator.sh
+++ b/t/t0065-dir-iterator.sh
@@ -28,7 +28,7 @@ test_expect_success 'dir-iterator should iterate through all files' '
 	>dir/a/e &&
 	>dir/d/e/d/a &&
 
-	test-dir-iterator ./dir 1 | sort >./actual-pre-order-sorted-output &&
+	test-dir-iterator --pre-order ./dir | sort >./actual-pre-order-sorted-output &&
 	rm -rf dir &&
 
 	test_cmp expect-sorted-output actual-pre-order-sorted-output
@@ -44,7 +44,7 @@ test_expect_success 'dir-iterator should iterate through all files on post-order
 	>dir/a/e &&
 	>dir/d/e/d/a &&
 
-	test-dir-iterator ./dir 2 | sort >actual-post-order-sorted-output &&
+	test-dir-iterator --post-order ./dir | sort >actual-post-order-sorted-output &&
 	rm -rf dir &&
 
 	test_cmp expect-sorted-output actual-post-order-sorted-output
@@ -61,7 +61,7 @@ test_expect_success 'dir-iterator should list files properly on pre-order mode'
 	mkdir -p dir/a/b/c/ &&
 	>dir/a/b/c/d &&
 
-	test-dir-iterator ./dir 1 >actual-pre-order-output &&
+	test-dir-iterator --pre-order ./dir >actual-pre-order-output &&
 	rm -rf dir &&
 
 	test_cmp expect-pre-order-output actual-pre-order-output
@@ -78,7 +78,7 @@ test_expect_success 'dir-iterator should list files properly on post-order mode'
 	mkdir -p dir/a/b/c/ &&
 	>dir/a/b/c/d &&
 
-	test-dir-iterator ./dir 2 >actual-post-order-output &&
+	test-dir-iterator --post-order ./dir >actual-post-order-output &&
 	rm -rf dir &&
 
 	test_cmp expect-post-order-output actual-post-order-output
@@ -100,7 +100,7 @@ test_expect_success 'dir-iterator should list files properly on pre-order + post
 	mkdir -p dir/a/b/c/ &&
 	>dir/a/b/c/d &&
 
-	test-dir-iterator ./dir 7 >actual-pre-order-post-order-root-dir-output &&
+	test-dir-iterator --pre-order --post-order --list-root-dir ./dir >actual-pre-order-post-order-root-dir-output &&
 	rm -rf dir &&
 
 	test_cmp expect-pre-order-post-order-root-dir-output actual-pre-order-post-order-root-dir-output
-- 
2.11.0