[PATCH 0/2] Migrate ls-tree to parse-opts

[PATCH 0/2] Migrate ls-tree to parse-opts

[Stephen Boyd]

This looked pretty straightforward. I guess --name-status is there
for backward compatibility. Should it be there anymore?

Stephen Boyd (2):
  t3101: test more ls-tree options
  ls-tree: migrate to parse-options

 builtin-ls-tree.c          |  100 +++++++++++++++++--------------------------
 t/t3101-ls-tree-dirname.sh |   89 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 127 insertions(+), 62 deletions(-)

[PATCH 1/2] t3101: test more ls-tree options

[Stephen Boyd]

Add tests for --full-name, --full-tree, --abbrev, and --name-only.

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---
 t/t3101-ls-tree-dirname.sh |   89 +++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 87 insertions(+), 2 deletions(-)

diff --git a/t/t3101-ls-tree-dirname.sh b/t/t3101-ls-tree-dirname.sh
index 51cb4a3..99458e4 100755
--- a/t/t3101-ls-tree-dirname.sh
+++ b/t/t3101-ls-tree-dirname.sh
@@ -39,8 +39,8 @@ test_expect_success \
      tree=`git write-tree` &&
      echo $tree'
 
-_x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
-_x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
+_x5='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
+_x40="$_x5$_x5$_x5$_x5$_x5$_x5$_x5$_x5"
 test_output () {
     sed -e "s/ $_x40	/ X	/" <current >check
     test_cmp expected check
@@ -141,4 +141,89 @@ test_expect_success 'ls-tree filter is leading path match' '
 	test_output
 '
 
+test_expect_success 'ls-tree --full-name' '
+	(
+		cd path0 &&
+		git ls-tree --full-name $tree a
+	) > current &&
+	cat >expected <<\EOF &&
+040000 tree X	path0/a
+EOF
+	test_output
+'
+
+test_expect_success 'ls-tree --full-tree' '
+	(
+		cd path1/b/c &&
+		git ls-tree --full-tree $tree
+	) > current &&
+	cat >expected <<\EOF &&
+100644 blob X	1.txt
+100644 blob X	2.txt
+040000 tree X	path0
+040000 tree X	path1
+040000 tree X	path2
+040000 tree X	path3
+EOF
+	test_output
+'
+
+test_expect_success 'ls-tree --full-tree -r' '
+	(
+		cd path3/ &&
+		git ls-tree --full-tree -r $tree
+	) > current &&
+	cat >expected <<\EOF &&
+100644 blob X	1.txt
+100644 blob X	2.txt
+100644 blob X	path0/a/b/c/1.txt
+100644 blob X	path1/b/c/1.txt
+100644 blob X	path2/1.txt
+100644 blob X	path3/1.txt
+100644 blob X	path3/2.txt
+EOF
+	test_output
+'
+
+test_expect_success 'ls-tree --abbrev=5' '
+	git ls-tree --abbrev=5 $tree > current.abbrev &&
+	sed -e "s/ $_x5	/ X	/"  < current.abbrev > current &&
+	cat >expected <<\EOF &&
+100644 blob X	1.txt
+100644 blob X	2.txt
+040000 tree X	path0
+040000 tree X	path1
+040000 tree X	path2
+040000 tree X	path3
+EOF
+	test_output
+'
+
+test_expect_success 'ls-tree --name-only' '
+	git ls-tree --name-only $tree > current
+	cat >expected <<\EOF &&
+1.txt
+2.txt
+path0
+path1
+path2
+path3
+EOF
+	test_output
+'
+
+test_expect_success 'ls-tree --name-only -r' '
+	git ls-tree --name-only -r $tree > current
+	cat >expected <<\EOF &&
+1.txt
+2.txt
+path0/a/b/c/1.txt
+path1/b/c/1.txt
+path2/1.txt
+path3/1.txt
+path3/2.txt
+EOF
+	test_output
+'
+
 test_done
-- 
1.6.5.2.155.gbb47

[PATCH 2/2] ls-tree: migrate to parse-options

[Stephen Boyd]

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---
 builtin-ls-tree.c |  100 +++++++++++++++++++++-------------------------------
 1 files changed, 40 insertions(+), 60 deletions(-)

diff --git a/builtin-ls-tree.c b/builtin-ls-tree.c
index 22008df..04408df 100644
--- a/builtin-ls-tree.c
+++ b/builtin-ls-tree.c
@@ -9,6 +9,7 @@
 #include "commit.h"
 #include "quote.h"
 #include "builtin.h"
+#include "parse-options.h"
 
 static int line_termination = '\n';
 #define LS_RECURSIVE 1
@@ -22,8 +23,10 @@ static const char **pathspec;
 static int chomp_prefix;
 static const char *ls_tree_prefix;
 
-static const char ls_tree_usage[] =
-	"git ls-tree [-d] [-r] [-t] [-l] [-z] [--name-only] [--name-status] [--full-name] [--full-tree] [--abbrev[=<n>]] <tree-ish> [path...]";
+static const  char * const ls_tree_usage[] = {
+	"git ls-tree [-d] [-r] [-t] [-l | --long] [-z] [--name-only] [--name-status] [--full-name] [--full-tree] [--abbrev[=<n>]] <tree-ish> [path...]",
+	NULL
+};
 
 static int show_recursive(const char *base, int baselen, const char *pathname)
 {
@@ -117,76 +120,53 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 {
 	unsigned char sha1[20];
 	struct tree *tree;
+	int full_tree = 0;
+	const struct option ls_tree_options[] = {
+		OPT_BIT('d', NULL, &ls_options, "only show trees",
+			LS_TREE_ONLY),
+		OPT_BIT('r', NULL, &ls_options, "recurse into subtrees",
+			LS_RECURSIVE),
+		OPT_BIT('t', NULL, &ls_options, "show trees when recursing",
+			LS_SHOW_TREES),
+		OPT_SET_INT('z', NULL, &line_termination,
+			    "terminate entries with NUL byte", 0),
+		OPT_BIT('l', "long", &ls_options, "include object size",
+			LS_SHOW_SIZE),
+		OPT_BIT(0, "name-only", &ls_options, "list only filenames",
+			LS_NAME_ONLY),
+		OPT_BIT(0, "name-status", &ls_options, "list only filenames",
+			LS_NAME_ONLY),
+		OPT_SET_INT(0, "full-name", &chomp_prefix,
+			    "use full path names", 0),
+		OPT_BOOLEAN(0, "full-tree", &full_tree,
+			    "list entire tree; not just current directory "
+			    "(implies --full-name)"),
+		OPT__ABBREV(&abbrev),
+		OPT_END()
+	};
 
 	git_config(git_default_config, NULL);
 	ls_tree_prefix = prefix;
 	if (prefix && *prefix)
 		chomp_prefix = strlen(prefix);
-	while (1 < argc && argv[1][0] == '-') {
-		switch (argv[1][1]) {
-		case 'z':
-			line_termination = 0;
-			break;
-		case 'r':
-			ls_options |= LS_RECURSIVE;
-			break;
-		case 'd':
-			ls_options |= LS_TREE_ONLY;
-			break;
-		case 't':
-			ls_options |= LS_SHOW_TREES;
-			break;
-		case 'l':
-			ls_options |= LS_SHOW_SIZE;
-			break;
-		case '-':
-			if (!strcmp(argv[1]+2, "name-only") ||
-			    !strcmp(argv[1]+2, "name-status")) {
-				ls_options |= LS_NAME_ONLY;
-				break;
-			}
-			if (!strcmp(argv[1]+2, "long")) {
-				ls_options |= LS_SHOW_SIZE;
-				break;
-			}
-			if (!strcmp(argv[1]+2, "full-name")) {
-				chomp_prefix = 0;
-				break;
-			}
-			if (!strcmp(argv[1]+2, "full-tree")) {
-				ls_tree_prefix = prefix = NULL;
-				chomp_prefix = 0;
-				break;
-			}
-			if (!prefixcmp(argv[1]+2, "abbrev=")) {
-				abbrev = strtoul(argv[1]+9, NULL, 10);
-				if (abbrev && abbrev < MINIMUM_ABBREV)
-					abbrev = MINIMUM_ABBREV;
-				else if (abbrev > 40)
-					abbrev = 40;
-				break;
-			}
-			if (!strcmp(argv[1]+2, "abbrev")) {
-				abbrev = DEFAULT_ABBREV;
-				break;
-			}
-			/* otherwise fallthru */
-		default:
-			usage(ls_tree_usage);
-		}
-		argc--; argv++;
+
+	argc = parse_options(argc, argv, prefix, ls_tree_options,
+			     ls_tree_usage, 0);
+	if (full_tree) {
+		ls_tree_prefix = prefix = NULL;
+		chomp_prefix = 0;
 	}
 	/* -d -r should imply -t, but -d by itself should not have to. */
 	if ( (LS_TREE_ONLY|LS_RECURSIVE) ==
 	    ((LS_TREE_ONLY|LS_RECURSIVE) & ls_options))
 		ls_options |= LS_SHOW_TREES;
 
-	if (argc < 2)
-		usage(ls_tree_usage);
-	if (get_sha1(argv[1], sha1))
-		die("Not a valid object name %s", argv[1]);
+	if (argc < 1)
+		usage_with_options(ls_tree_usage, ls_tree_options);
+	if (get_sha1(argv[0], sha1))
+		die("Not a valid object name %s", argv[0]);
 
-	pathspec = get_pathspec(prefix, argv + 2);
+	pathspec = get_pathspec(prefix, argv + 1);
 	tree = parse_tree_indirect(sha1);
 	if (!tree)
 		die("not a tree object");
-- 
1.6.5.2.155.gbb47

Re: [PATCH 1/2] t3101: test more ls-tree options

[Junio C Hamano]

Thanks.  I'd squash this patch in for the following reasons; Ok?

 - The --abbrev=n option only guarantees that at least n hexdigits are
   used but the implementation is allowed to use more to avoid
   ambiguities.

 - The point of test_output function is that it does its own filtering
   before comparison; it is misleading to feed it if the caller munges the
   input.

 - Redirection should be immediately followed by filename; this is just a
   matter of personal taste, but being consistent in the new code is
   better than resulting in blocks of lines with mixed styles (no, I won't
   welcome a patch to fix the existing "cmd > file", unless it is done as
   part of a patch aka "while at it" that fixes something of substance
   nearby at the same time).

 t/t3101-ls-tree-dirname.sh |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/t/t3101-ls-tree-dirname.sh b/t/t3101-ls-tree-dirname.sh
index 99458e4..6ef371a 100755
--- a/t/t3101-ls-tree-dirname.sh
+++ b/t/t3101-ls-tree-dirname.sh
@@ -39,8 +39,8 @@ test_expect_success \
      tree=`git write-tree` &&
      echo $tree'
 
-_x5='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
-_x40="$_x5$_x5$_x5$_x5$_x5$_x5$_x5$_x5"
+_x05='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
+_x40="$_x05$_x05$_x05$_x05$_x05$_x05$_x05$_x05"
 test_output () {
     sed -e "s/ $_x40	/ X	/" <current >check
     test_cmp expected check
@@ -145,7 +145,7 @@ test_expect_success 'ls-tree --full-name' '
 	(
 		cd path0 &&
 		git ls-tree --full-name $tree a
-	) > current &&
+	) >current &&
 	cat >expected <<\EOF &&
 040000 tree X	path0/a
 EOF
@@ -156,7 +156,7 @@ test_expect_success 'ls-tree --full-tree' '
 	(
 		cd path1/b/c &&
 		git ls-tree --full-tree $tree
-	) > current &&
+	) >current &&
 	cat >expected <<\EOF &&
 100644 blob X	1.txt
 100644 blob X	2.txt
@@ -172,7 +172,7 @@ test_expect_success 'ls-tree --full-tree -r' '
 	(
 		cd path3/ &&
 		git ls-tree --full-tree -r $tree
-	) > current &&
+	) >current &&
 	cat >expected <<\EOF &&
 100644 blob X	1.txt
 100644 blob X	2.txt
@@ -186,8 +186,8 @@ EOF
 '
 
 test_expect_success 'ls-tree --abbrev=5' '
-	git ls-tree --abbrev=5 $tree > current.abbrev &&
-	sed -e "s/ $_x5	/ X	/"  < current.abbrev > current &&
+	git ls-tree --abbrev=5 $tree >current &&
+	sed -e "s/ $_x05[0-9a-f]*	/ X	/" <current >check &&
 	cat >expected <<\EOF &&
 100644 blob X	1.txt
 100644 blob X	2.txt
@@ -196,11 +196,11 @@ test_expect_success 'ls-tree --abbrev=5' '
 040000 tree X	path2
 040000 tree X	path3
 EOF
-	test_output
+	test_cmp expected check
 '
 
 test_expect_success 'ls-tree --name-only' '
-	git ls-tree --name-only $tree > current
+	git ls-tree --name-only $tree >current
 	cat >expected <<\EOF &&
 1.txt
 2.txt
@@ -213,7 +213,7 @@ EOF
 '
 
 test_expect_success 'ls-tree --name-only -r' '
-	git ls-tree --name-only -r $tree > current
+	git ls-tree --name-only -r $tree >current
 	cat >expected <<\EOF &&
 1.txt
 2.txt
-- 
1.6.5.3.283.g4b054

Re: [PATCH 2/2] ls-tree: migrate to parse-options

[Junio C Hamano]

Thanks.

As parse-options infrastructure gives much better per-option help text,
there is not much point to keep the list of options that can go stale
in the usage text.

I'd squash this to yours.  Ok?

---
 builtin-ls-tree.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin-ls-tree.c b/builtin-ls-tree.c
index 04408df..14a913a 100644
--- a/builtin-ls-tree.c
+++ b/builtin-ls-tree.c
@@ -24,7 +24,7 @@ static int chomp_prefix;
 static const char *ls_tree_prefix;
 
 static const  char * const ls_tree_usage[] = {
-	"git ls-tree [-d] [-r] [-t] [-l | --long] [-z] [--name-only] [--name-status] [--full-name] [--full-tree] [--abbrev[=<n>]] <tree-ish> [path...]",
+	"git ls-tree <options> <tree-ish> [path...]",
 	NULL
 };
 
-- 
1.6.5.3.283.g4b054

Re: [PATCH 1/2] t3101: test more ls-tree options

[Stephen Boyd]

Junio C Hamano wrote:
> Thanks.  I'd squash this patch in for the following reasons; Ok?

Looks good. Thanks.

I figured reusing the test_output function would be a bad idea.

Re: [PATCH 2/2] ls-tree: migrate to parse-options

[Stephen Boyd]

Junio C Hamano wrote:
> Thanks.
>
> As parse-options infrastructure gives much better per-option help text,
> there is not much point to keep the list of options that can go stale
> in the usage text.
>
> I'd squash this to yours.  Ok?
>

I like this style too. Less usage update patches are better. Thanks.

Re: [PATCH 2/2] ls-tree: migrate to parse-options

[Stephen Boyd]

Junio C Hamano wrote:
>
> @@ -24,7 +24,7 @@ static int chomp_prefix;
>  static const char *ls_tree_prefix;
>  
>  static const  char * const ls_tree_usage[] = {
> -	"git ls-tree [-d] [-r] [-t] [-l | --long] [-z] [--name-only] [--name-status] [--full-name] [--full-tree] [--abbrev[=<n>]] <tree-ish> [path...]",
> +	"git ls-tree <options> <tree-ish> [path...]",
>  	NULL
>  };
>  

Is it [<options>] or [<options>...] or <options> or even <options>... 
though?

Re: [PATCH 2/2] ls-tree: migrate to parse-options

[Junio C Hamano]

Stephen Boyd <bebarino@gmail.com> writes:

> Junio C Hamano wrote:
>>
>> @@ -24,7 +24,7 @@ static int chomp_prefix;
>>  static const char *ls_tree_prefix;
>>   static const  char * const ls_tree_usage[] = {
>> -	"git ls-tree [-d] [-r] [-t] [-l | --long] [-z] [--name-only] [--name-status] [--full-name] [--full-tree] [--abbrev[=<n>]] <tree-ish> [path...]",
>> +	"git ls-tree <options> <tree-ish> [path...]",
>>  	NULL
>>  };
>>  
>
> Is it [<options>] or [<options>...] or <options> or even
> <options>... though?

Output from "git grep -e '<option' -- '*.c'" indicates that it
should be "[<options>]"; thanks for spotting.

Re: [PATCH 2/2] ls-tree: migrate to parse-options

[Thiago Farina]

On Wed, Nov 18, 2009 at 8:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stephen Boyd <bebarino@gmail.com> writes:
>
>> Junio C Hamano wrote:
>>>
>>> @@ -24,7 +24,7 @@ static int chomp_prefix;
>>>  static const char *ls_tree_prefix;
>>>   static const  char * const ls_tree_usage[] = {
>>> -    "git ls-tree [-d] [-r] [-t] [-l | --long] [-z] [--name-only] [--name-status] [--full-name] [--full-tree] [--abbrev[=<n>]] <tree-ish> [path...]",
>>> +    "git ls-tree <options> <tree-ish> [path...]",
>>>      NULL
>>>  };
>>>
>>
>> Is it [<options>] or [<options>...] or <options> or even
>> <options>... though?
>
> Output from "git grep -e '<option' -- '*.c'" indicates that it
> should be "[<options>]"; thanks for spotting.

$ git grep -e '<option' -- '*.c' | wc -l
4

$ git grep -e '\[options' -- '*.c' | wc -l
42

$ git grep -e '\[<options' -- '*.c' | wc -l
2

Shouldn't be just "[options]"?