[PATCH 1/3] ls-tree: make <tree-ish> optional

[PATCH 1/3] ls-tree: make <tree-ish> optional

[Joshua Nelson]

use syntax similar to `git-checkout` to make <tree-ish> optional for
`ls-tree`. if <tree-ish> is omitted, default to HEAD. infer arguments as
follows:

1. if args start with --
	assume <tree-ish> to be HEAD
2. if exactly one arg precedes --, treat the argument as <tree-ish>
3. if more than one arg precedes --, exit with an error
4. if -- is not in args
	a) if args[0] is a valid <tree-ish> object, treat is as such
	b) else, assume <tree-ish> to be HEAD

in all cases, every argument besides <tree-ish> is treated as a <path>
---
 builtin/ls-tree.c | 39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git builtin/ls-tree.c builtin/ls-tree.c
index 409da4e83..14102b052 100644
--- builtin/ls-tree.c
+++ builtin/ls-tree.c
@@ -153,7 +153,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 		chomp_prefix = strlen(prefix);
 
 	argc = parse_options(argc, argv, prefix, ls_tree_options,
-			     ls_tree_usage, 0);
+			     ls_tree_usage, PARSE_OPT_KEEP_DASHDASH);
 	if (full_tree) {
 		ls_tree_prefix = prefix = NULL;
 		chomp_prefix = 0;
@@ -163,10 +163,39 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 	    ((LS_TREE_ONLY|LS_RECURSIVE) & ls_options))
 		ls_options |= LS_SHOW_TREES;
 
+	const char *object;
+	short initialized = 0;
 	if (argc < 1)
-		usage_with_options(ls_tree_usage, ls_tree_options);
-	if (get_oid(argv[0], &oid))
-		die("Not a valid object name %s", argv[0]);
+		object = "HEAD";
+	else {
+		/* taken from checkout.c;
+		 * we have a simpler case because we never create a branch */
+		short dash_dash_pos = -1, i = 0;
+		for (; i < argc; i++) {
+			if (!strcmp(argv[i], "--")) {
+				dash_dash_pos = i;
+				break;
+			}
+		}
+		if (dash_dash_pos == 0) {
+			object = "HEAD";
+			argv++, argc++;
+		} else if (dash_dash_pos == 1) {
+			object = argv[0];
+			argv += 2, argc += 2;
+		} else if (dash_dash_pos >= 2)
+			die(_("only one reference expected, %d given."), dash_dash_pos);
+		else if (get_oid(argv[0], &oid)) // not a valid object
+			object = "HEAD";
+		else {
+			argv++, argc++;
+			initialized = 1;
+		}
+	}
+
+	if (!initialized) // if we've already run get_oid, don't run it again
+		if (get_oid(object, &oid))
+			die("Not a valid object name %s", object);
 
 	/*
 	 * show_recursive() rolls its own matching code and is
@@ -177,7 +206,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 	parse_pathspec(&pathspec, PATHSPEC_ALL_MAGIC &
 				  ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
 		       PATHSPEC_PREFER_CWD,
-		       prefix, argv + 1);
+		       prefix, argv);
 	for (i = 0; i < pathspec.nr; i++)
 		pathspec.items[i].nowildcard_len = pathspec.items[i].len;
 	pathspec.has_wildcard = 0;
-- 
2.18.GIT

[PATCH 2/3] ls-tree: update usage info

[Joshua Nelson]

show [tree-ish] and [--] as optional
---
 builtin/ls-tree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git builtin/ls-tree.c builtin/ls-tree.c
index 14102b052..c5649b09c 100644
--- builtin/ls-tree.c
+++ builtin/ls-tree.c
@@ -26,7 +26,7 @@ static int chomp_prefix;
 static const char *ls_tree_prefix;
 
 static const  char * const ls_tree_usage[] = {
-	N_("git ls-tree [<options>] <tree-ish> [<path>...]"),
+	N_("git ls-tree [<options>] [tree-ish] [--] [<path>...]"),
 	NULL
 };
 
-- 
2.18.GIT

Re: [PATCH 2/3] ls-tree: update usage info

[Elijah Newren]

Hi Joshua,

On Mon, Jul 2, 2018 at 8:58 PM, Joshua Nelson <jyn514@gmail.com> wrote:
> show [tree-ish] and [--] as optional

You're missing a Signed-off-by tag here.

> ---
>  builtin/ls-tree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git builtin/ls-tree.c builtin/ls-tree.c

No corresponding update to Documentation/git-ls-tree.txt?

> index 14102b052..c5649b09c 100644
> --- builtin/ls-tree.c
> +++ builtin/ls-tree.c
> @@ -26,7 +26,7 @@ static int chomp_prefix;
>  static const char *ls_tree_prefix;
>
>  static const  char * const ls_tree_usage[] = {
> -       N_("git ls-tree [<options>] <tree-ish> [<path>...]"),
> +       N_("git ls-tree [<options>] [tree-ish] [--] [<path>...]"),
>         NULL
>  };
>
> --
> 2.18.GIT

Re: [PATCH 2/3] ls-tree: update usage info

[Eric Sunshine]

On Mon, Jul 2, 2018 at 11:58 PM Joshua Nelson <jyn514@gmail.com> wrote:
> show [tree-ish] and [--] as optional
> ---
> diff --git builtin/ls-tree.c builtin/ls-tree.c
> @@ -26,7 +26,7 @@ static int chomp_prefix;
>  static const  char * const ls_tree_usage[] = {
> -       N_("git ls-tree [<options>] <tree-ish> [<path>...]"),
> +       N_("git ls-tree [<options>] [tree-ish] [--] [<path>...]"),

You lost the '<' and '>'. This should be typeset as:

    git ls-tree [<options>] [<tree-ish>] [--] [<path>...]

And, as Elijah noted, Documentation/git-ls-tree.txt needs an update.

[PATCH 3/3] ls-tree: add unit tests for arguments

[Joshua Nelson]

Signed-off-by: Joshua Nelson <jyn514@gmail.com>
---
 t/t3104-ls-tree-optional-args.sh | 43 ++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)
 create mode 100644 t/t3104-ls-tree-optional-args.sh

diff --git t/t3104-ls-tree-optional-args.sh t/t3104-ls-tree-optional-args.sh
new file mode 100644
index 000000000..5917563a7
--- /dev/null
+++ t/t3104-ls-tree-optional-args.sh
@@ -0,0 +1,43 @@
+#!/bin/sh
+
+test_description='ls-tree test (optional args)
+
+This test tries to run git-ls-tree with various combinations of options.'
+
+. ./test-lib.sh
+
+test_expect_success 'initial setup' '
+echo hi > test && cp test test2 && git add test test2 && git commit -m initial'
+
+# cat appends newlines after every file
+test_expect_success 'succeed when given no args' 'git ls-tree'
+
+test_expect_success 'succeed when given only --' 'git ls-tree'
+
+test_expect_success 'add second commit' '
+echo hi > test3 && git add test3 && git commit -m "commit 2"'
+
+test_expect_success 'succeed when given revision' '
+git ls-tree HEAD~1'
+
+test_expect_success 'succeed when given revision and --' '
+git ls-tree HEAD~1 --'
+
+test_expect_success 'succeed when given -- and file' '
+git ls-tree -- test3'
+
+test_expect_success 'do nothing when given bad files' '
+git ls-tree -- bad_files'
+
+test_expect_success 'succeed when given file from past revision' '
+git ls-tree HEAD~1 test'
+
+test_expect_success 'succeed when given only file' 'git ls-tree test'
+
+test_expect_success 'raise error when given bad args' '
+test_must_fail  git ls-tree HEAD HEAD --'
+
+test_expect_success 'raise error when given bad revision' '
+test_must_fail git ls-tree bad_revision --'
+
+test_done
-- 
2.18.GIT

Re: [PATCH 3/3] ls-tree: add unit tests for arguments

[Elijah Newren]

Hi Joshua,

On Mon, Jul 2, 2018 at 8:58 PM, Joshua Nelson <jyn514@gmail.com> wrote:

The commit message could use some clarification; it currently makes
the reader think you're testing all arguments of ls-tree, when you're
only testing a few new ones.  Alternatively, you could squash this
with patch 1.

> Signed-off-by: Joshua Nelson <jyn514@gmail.com>
> ---
>  t/t3104-ls-tree-optional-args.sh | 43 ++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
>  create mode 100644 t/t3104-ls-tree-optional-args.sh

Please note that test files should be executable, not regular files.

>
> diff --git t/t3104-ls-tree-optional-args.sh t/t3104-ls-tree-optional-args.sh
> new file mode 100644
> index 000000000..5917563a7
> --- /dev/null
> +++ t/t3104-ls-tree-optional-args.sh
> @@ -0,0 +1,43 @@
> +#!/bin/sh
> +
> +test_description='ls-tree test (optional args)
> +
> +This test tries to run git-ls-tree with various combinations of options.'

This description seems like it's true for all the t310*.sh tests.  How
does t3104 differ from t310[0-3]*.sh?

> +
> +. ./test-lib.sh
> +
> +test_expect_success 'initial setup' '
> +echo hi > test && cp test test2 && git add test test2 && git commit -m initial'
> +
> +# cat appends newlines after every file

What is this comment in reference to?

> +test_expect_success 'succeed when given no args' 'git ls-tree'
> +
> +test_expect_success 'succeed when given only --' 'git ls-tree'

I was a little surprised that these tests only check that the command
runs to completion with a 0 exit status, and do not do any
verification of the output.  I wasn't necessarily expected full output
verification, but having a few different commits and searching for
something that'd only be shown in the relevant commit would be nice.

> +
> +test_expect_success 'add second commit' '
> +echo hi > test3 && git add test3 && git commit -m "commit 2"'
> +
> +test_expect_success 'succeed when given revision' '
> +git ls-tree HEAD~1'
> +
> +test_expect_success 'succeed when given revision and --' '
> +git ls-tree HEAD~1 --'
> +
> +test_expect_success 'succeed when given -- and file' '
> +git ls-tree -- test3'
> +
> +test_expect_success 'do nothing when given bad files' '
> +git ls-tree -- bad_files'

...and to reiterate the point above about verifying the output is
correct, how is 'doing nothing' here distinguishable from showing all
the files in the current commit if you're not checking any part of the
output?

> +
> +test_expect_success 'succeed when given file from past revision' '
> +git ls-tree HEAD~1 test'
> +
> +test_expect_success 'succeed when given only file' 'git ls-tree test'
> +
> +test_expect_success 'raise error when given bad args' '
> +test_must_fail  git ls-tree HEAD HEAD --'
> +
> +test_expect_success 'raise error when given bad revision' '
> +test_must_fail git ls-tree bad_revision --'
> +
> +test_done
> --
> 2.18.GIT

Lots of little things to fix up, but you're off to a great start.  I'm
excited to be able to have ls-tree work without me having to specify
HEAD all the time.  :-)

Re: [PATCH 3/3] ls-tree: add unit tests for arguments

[Eric Sunshine]

On Mon, Jul 2, 2018 at 11:58 PM Joshua Nelson <jyn514@gmail.com> wrote:
> Signed-off-by: Joshua Nelson <jyn514@gmail.com>
> ---
> diff --git t/t3104-ls-tree-optional-args.sh t/t3104-ls-tree-optional-args.sh
> @@ -0,0 +1,43 @@
> +test_expect_success 'initial setup' '
> +echo hi > test && cp test test2 && git add test test2 && git commit -m initial'

As this is a new test script, please use modern formatting style:
indent test body with tabs, closing single-quote goes on its own line,
break lines at &&, and no whitespace between > and the filename. Also,
it's customary to simply to call the test "setup".

test_expect_success 'setup' '
    echo hi >test &&
    cp test test2 &&
    git add test test2 &&
    git commit -m initial
'

> +# cat appends newlines after every file

Why is this talking about a "cat"? Doesn't seem relevant.

> +test_expect_success 'succeed when given no args' 'git ls-tree'

This test seems too minimal. As the intention of this patch series is
to make git-ls-tree default to HEAD when no treeish is given, I would
expect the test with no arguments to verify that it did indeed list
the tree associated with HEAD. As implemented, this test tells us
nothing other than that it didn't error out or crash.

> +test_expect_success 'succeed when given only --' 'git ls-tree'

Um, what's this supposed to be testing? Presently, it seems to
duplicate the previous test. I'm guessing it should be running "git
ls-tree --" instead.

> +test_expect_success 'add second commit' '
> +echo hi > test3 && git add test3 && git commit -m "commit 2"'
> +
> +test_expect_success 'succeed when given revision' '
> +git ls-tree HEAD~1'

Given how patch 1/3 makes some fundamental changes to how git-ls-tree
processes its arguments, I would again expect this test to verify that
it indeed lists the correct tree. As the test is currently
implemented, we have no way of knowing what tree (if any) it listed.

> +test_expect_success 'succeed when given revision and --' '
> +git ls-tree HEAD~1 --'
> +
> +test_expect_success 'succeed when given -- and file' '
> +git ls-tree -- test3'

As above, given the fundamental changes to argument processing, I'd
expect this to verify that the output of this command is indeed what
is expected.

> +test_expect_success 'do nothing when given bad files' '
> +git ls-tree -- bad_files'

I wonder about this. Is it just an accident of implementation that
git-ls-tree doesn't error out in this case, or is it intended behavior
that it should return 0 even when the file is not in the tree? If the
0 exit code is just an accident of implementation, then we shouldn't
be enforcing this by a test (instead, someone perhaps ought to fix
git-ls-tree).

> +test_expect_success 'succeed when given file from past revision' '
> +git ls-tree HEAD~1 test'

Same comment as above about verifying gave expected output.

> +test_expect_success 'succeed when given only file' 'git ls-tree test'
> +
> +test_expect_success 'raise error when given bad args' '
> +test_must_fail  git ls-tree HEAD HEAD --'
> +
> +test_expect_success 'raise error when given bad revision' '
> +test_must_fail git ls-tree bad_revision --'

Re: [PATCH 1/3] ls-tree: make <tree-ish> optional

[Elijah Newren]

On Mon, Jul 2, 2018 at 8:58 PM, Joshua Nelson <jyn514@gmail.com> wrote:
> use syntax similar to `git-checkout` to make <tree-ish> optional for
> `ls-tree`. if <tree-ish> is omitted, default to HEAD. infer arguments as
> follows:
>
> 1. if args start with --
>         assume <tree-ish> to be HEAD
> 2. if exactly one arg precedes --, treat the argument as <tree-ish>
> 3. if more than one arg precedes --, exit with an error
> 4. if -- is not in args
>         a) if args[0] is a valid <tree-ish> object, treat is as such
>         b) else, assume <tree-ish> to be HEAD
>
> in all cases, every argument besides <tree-ish> is treated as a <path>

Cool, this is something I've wanted a few times.  Thanks for
submitting.  A few minor issues:
  1. Could you start sentences with capitals?  Thus, 'Use syntax...',
'Infer arguments...', 'In all cases...', etc.
  2. Missing Signed-off-by on this commit (and commit 2); I notice you
correctly added it to commit 3.

Also, additional issues below...

> ---
>  builtin/ls-tree.c | 39 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 34 insertions(+), 5 deletions(-)
>
> diff --git builtin/ls-tree.c builtin/ls-tree.c
> index 409da4e83..14102b052 100644
> --- builtin/ls-tree.c
> +++ builtin/ls-tree.c

Do you have diff.noprefix set?  It took me a little bit to realize
that this was the reason your patches weren't applying for me.

> @@ -153,7 +153,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
>                 chomp_prefix = strlen(prefix);
>
>         argc = parse_options(argc, argv, prefix, ls_tree_options,
> -                            ls_tree_usage, 0);
> +                            ls_tree_usage, PARSE_OPT_KEEP_DASHDASH);
>         if (full_tree) {
>                 ls_tree_prefix = prefix = NULL;
>                 chomp_prefix = 0;
> @@ -163,10 +163,39 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
>             ((LS_TREE_ONLY|LS_RECURSIVE) & ls_options))
>                 ls_options |= LS_SHOW_TREES;
>
> +       const char *object;
> +       short initialized = 0;

This will fail to compile under 'make DEVELOPER=1' (ISO C90 forbids
mixed declarations and code).

>         if (argc < 1)
> -               usage_with_options(ls_tree_usage, ls_tree_options);
> -       if (get_oid(argv[0], &oid))
> -               die("Not a valid object name %s", argv[0]);
> +               object = "HEAD";
> +       else {
> +               /* taken from checkout.c;
> +                * we have a simpler case because we never create a branch */

/*
 * multi-line comment style in git utilizes puts the first word on the
 * second line, and terminates on its own line, like this.
 */

Also, I was going to ask if this code should be put into a utility
function or something, but it isn't taken verbatim from checkout.c,
just adopted from there.

> +               short dash_dash_pos = -1, i = 0;
> +               for (; i < argc; i++) {

Any reason you moved the i = 0 to the line before?  I thought 'i' was
uninitialized and almost commented to that effect before I noticed
where it was.

> +                       if (!strcmp(argv[i], "--")) {
> +                               dash_dash_pos = i;
> +                               break;
> +                       }
> +               }
> +               if (dash_dash_pos == 0) {
> +                       object = "HEAD";
> +                       argv++, argc++;
> +               } else if (dash_dash_pos == 1) {
> +                       object = argv[0];
> +                       argv += 2, argc += 2;
> +               } else if (dash_dash_pos >= 2)
> +                       die(_("only one reference expected, %d given."), dash_dash_pos);
> +               else if (get_oid(argv[0], &oid)) // not a valid object
> +                       object = "HEAD";
> +               else {
> +                       argv++, argc++;
> +                       initialized = 1;
> +               }
> +       }
> +
> +       if (!initialized) // if we've already run get_oid, don't run it again
> +               if (get_oid(object, &oid))
> +                       die("Not a valid object name %s", object);
>
>         /*
>          * show_recursive() rolls its own matching code and is
> @@ -177,7 +206,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
>         parse_pathspec(&pathspec, PATHSPEC_ALL_MAGIC &
>                                   ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
>                        PATHSPEC_PREFER_CWD,
> -                      prefix, argv + 1);
> +                      prefix, argv);
>         for (i = 0; i < pathspec.nr; i++)
>                 pathspec.items[i].nowildcard_len = pathspec.items[i].len;
>         pathspec.has_wildcard = 0;
> --
> 2.18.GIT
>

Re: [PATCH 1/3] ls-tree: make <tree-ish> optional

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> On Mon, Jul 2, 2018 at 8:58 PM, Joshua Nelson <jyn514@gmail.com> wrote:
>> use syntax similar to `git-checkout` to make <tree-ish> optional for
>> `ls-tree`. if <tree-ish> is omitted, default to HEAD. infer arguments as
>> follows:
>>
>> 1. if args start with --
>>         assume <tree-ish> to be HEAD
>> 2. if exactly one arg precedes --, treat the argument as <tree-ish>
>> 3. if more than one arg precedes --, exit with an error
>> 4. if -- is not in args
>>         a) if args[0] is a valid <tree-ish> object, treat is as such
>>         b) else, assume <tree-ish> to be HEAD
>>
>> in all cases, every argument besides <tree-ish> is treated as a <path>
>
> Cool, this is something I've wanted a few times.

Hmph, is it, and why?  

I'd prefer *not* to have such a DWIM in a command like ls-tree, aka
plumbing commands, where predictability is worth 1000 times more
than ease of typing.

Re: [PATCH 1/3] ls-tree: make <tree-ish> optional

[Elijah Newren]

On Tue, Jul 3, 2018 at 3:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Elijah Newren <newren@gmail.com> writes:
>
>> On Mon, Jul 2, 2018 at 8:58 PM, Joshua Nelson <jyn514@gmail.com> wrote:
>>> use syntax similar to `git-checkout` to make <tree-ish> optional for
>>> `ls-tree`. if <tree-ish> is omitted, default to HEAD. infer arguments as
>>> follows:
>>>
>>> 1. if args start with --
>>>         assume <tree-ish> to be HEAD
>>> 2. if exactly one arg precedes --, treat the argument as <tree-ish>
>>> 3. if more than one arg precedes --, exit with an error
>>> 4. if -- is not in args
>>>         a) if args[0] is a valid <tree-ish> object, treat is as such
>>>         b) else, assume <tree-ish> to be HEAD
>>>
>>> in all cases, every argument besides <tree-ish> is treated as a <path>
>>
>> Cool, this is something I've wanted a few times.
>
> Hmph, is it, and why?

Default <tree-ish> of HEAD when nothing is specified is certainly
something I wanted.  To be honest, I wanted it for rev-list too.
Despite dozens if not hundreds of times of typing 'git ls-tree -r' or
'git rev-list' expecting to see the results for HEAD (just as git log
does), and getting git's error message reminding me that I need to
specify HEAD, I can't seem to get it through my head to remember that
I need to specify it.

> I'd prefer *not* to have such a DWIM in a command like ls-tree, aka
> plumbing commands, where predictability is worth 1000 times more
> than ease of typing.

Fair enough.  However, what if no <tree-ish> or <path> are specified,
though -- would you be okay with the HEAD being assumed instead of
erroring out in that case?

Re: [PATCH 1/3] ls-tree: make <tree-ish> optional

[Joshua Nelson]

Agreed, ls-tree when called with no arguments was the main use case I
wrote this for; the rest was mostly because other commands allow greater
ambiguity and I wanted to make the syntax consistent.

I don't mind doing this for rev-list as well if that's a useful feature.


On 07/03/2018 06:55 PM, Elijah Newren wrote:
> On Tue, Jul 3, 2018 at 3:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Elijah Newren <newren@gmail.com> writes:
>>
>>> On Mon, Jul 2, 2018 at 8:58 PM, Joshua Nelson <jyn514@gmail.com> wrote:
>>>> use syntax similar to `git-checkout` to make <tree-ish> optional for
>>>> `ls-tree`. if <tree-ish> is omitted, default to HEAD. infer arguments as
>>>> follows:
>>>>
>>>> 1. if args start with --
>>>>         assume <tree-ish> to be HEAD
>>>> 2. if exactly one arg precedes --, treat the argument as <tree-ish>
>>>> 3. if more than one arg precedes --, exit with an error
>>>> 4. if -- is not in args
>>>>         a) if args[0] is a valid <tree-ish> object, treat is as such
>>>>         b) else, assume <tree-ish> to be HEAD
>>>>
>>>> in all cases, every argument besides <tree-ish> is treated as a <path>
>>> Cool, this is something I've wanted a few times.
>> Hmph, is it, and why?
> Default <tree-ish> of HEAD when nothing is specified is certainly
> something I wanted.  To be honest, I wanted it for rev-list too.
> Despite dozens if not hundreds of times of typing 'git ls-tree -r' or
> 'git rev-list' expecting to see the results for HEAD (just as git log
> does), and getting git's error message reminding me that I need to
> specify HEAD, I can't seem to get it through my head to remember that
> I need to specify it.
>
>> I'd prefer *not* to have such a DWIM in a command like ls-tree, aka
>> plumbing commands, where predictability is worth 1000 times more
>> than ease of typing.
> Fair enough.  However, what if no <tree-ish> or <path> are specified,
> though -- would you be okay with the HEAD being assumed instead of
> erroring out in that case?

Re: [PATCH 1/3] ls-tree: make <tree-ish> optional

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

>> I'd prefer *not* to have such a DWIM in a command like ls-tree, aka
>> plumbing commands, where predictability is worth 1000 times more
>> than ease of typing.
>
> Fair enough.  However, what if no <tree-ish> or <path> are specified,
> though -- would you be okay with the HEAD being assumed instead of
> erroring out in that case?

If we wrote ls-tree to do so 12 years ago, then I wouldn't have
opposed.  Changing the behaviour now?  Not so sure if it is worth
having to worry about updating the code, docs and making sure we
spot all the possible typoes.

Re: [PATCH 1/3] ls-tree: make <tree-ish> optional

[Joshua Nelson]

On 07/06/2018 01:01 PM, Junio C Hamano wrote:
> Elijah Newren <newren@gmail.com> writes:
> 
>>> I'd prefer *not* to have such a DWIM in a command like ls-tree, aka
>>> plumbing commands, where predictability is worth 1000 times more
>>> than ease of typing.
>>
>> Fair enough.  However, what if no <tree-ish> or <path> are specified,
>> though -- would you be okay with the HEAD being assumed instead of
>> erroring out in that case?
> 
> If we wrote ls-tree to do so 12 years ago, then I wouldn't have
> opposed.  Changing the behaviour now?  Not so sure if it is worth
> having to worry about updating the code, docs and making sure we
> spot all the possible typoes.
> 

I have to say, as a first time contributor, reading this is extremely
discouraging. I'm not being told the patch can be improved, or that I've
made some error that should be corrected. I'm being told that the entire
idea of the patch is unwanted, that it doesn't have a place in a mature
project like git, that only bug fixes and security issues should be
accepted.

That makes me not want to contribute.

Re: [PATCH 1/3] ls-tree: make <tree-ish> optional

[Junio C Hamano]

Joshua Nelson <jyn514@gmail.com> writes:

> On 07/06/2018 01:01 PM, Junio C Hamano wrote:
>> Elijah Newren <newren@gmail.com> writes:
>> 
>>>> I'd prefer *not* to have such a DWIM in a command like ls-tree, aka
>>>> plumbing commands, where predictability is worth 1000 times more
>>>> than ease of typing.
>>>
>>> Fair enough.  However, what if no <tree-ish> or <path> are specified,
>>> though -- would you be okay with the HEAD being assumed instead of
>>> erroring out in that case?
>> 
>> If we wrote ls-tree to do so 12 years ago, then I wouldn't have
>> opposed.  Changing the behaviour now?  Not so sure if it is worth
>> having to worry about updating the code, docs and making sure we
>> spot all the possible typoes.
>> 
>
> I have to say, as a first time contributor, reading this is extremely
> discouraging. I'm not being told the patch can be improved, or that I've
> made some error that should be corrected. I'm being told that the entire
> idea of the patch is unwanted, that it doesn't have a place in a mature
> project like git, that only bug fixes and security issues should be
> accepted.

... on plumbing commands like ls-tree, where keeping the interface
stable is much more important than making it easier to "type".

Rules for porcelain commands are entirely different.

Re: [PATCH 1/3] ls-tree: make <tree-ish> optional

[Eric Sunshine]

Thanks for contributing to Git. As this seems to be your first
submission to the project, don't be alarmed by the extent and nature
of the review comments. They are intended to help you polish the
submission, and are not meant with ill-intent.

On Mon, Jul 2, 2018 at 11:58 PM Joshua Nelson <jyn514@gmail.com> wrote:
> use syntax similar to `git-checkout` to make <tree-ish> optional for
> `ls-tree`. if <tree-ish> is omitted, default to HEAD. infer arguments as
> follows:

Nit: Capitalize first word of each sentence.

This commit message explains what the patch changes (which is a good
thing to do), but it's missing the really important explanation of
_why_ this change is desirable. Without such justification, it's
difficult to judge if such a change is worthwhile. As this is a
plumbing command, people may need more convincing (especially if other
plumbing commands don't provide convenient defaults like this).

> 1. if args start with --
>         assume <tree-ish> to be HEAD
> 2. if exactly one arg precedes --, treat the argument as <tree-ish>
> 3. if more than one arg precedes --, exit with an error
> 4. if -- is not in args
>         a) if args[0] is a valid <tree-ish> object, treat is as such
>         b) else, assume <tree-ish> to be HEAD
>
> in all cases, every argument besides <tree-ish> is treated as a <path>

This and the other patches are missing your Signed-off-by: (which
should be placed right here).

The three patches of this series are all directly related to this one
change. As such, they should be combined into a single patch. (At the
very least, 1/3 and 2/3 should be combined; one could argue that 3/3
is lengthy enough to make it separate, but that's a judgment call.)

Now, on to the actual code...

> diff --git builtin/ls-tree.c builtin/ls-tree.c
> @@ -163,10 +163,39 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
>             ((LS_TREE_ONLY|LS_RECURSIVE) & ls_options))
>                 ls_options |= LS_SHOW_TREES;
>
> +       const char *object;
> +       short initialized = 0;

This project frowns on declaration-after-statement. Move these to the
top of the {...} block where other variables are declared.

Why use 'short'? Unless there's a very good reason that this needs to
be a particular size, you shouldn't deviate from project norm, which
is to use the natural word size 'int'.

'initialized' is too generic, thus isn't a great name.

>         if (argc < 1)
> -               usage_with_options(ls_tree_usage, ls_tree_options);
> -       if (get_oid(argv[0], &oid))
> -               die("Not a valid object name %s", argv[0]);
> +               object = "HEAD";
> +       else {
> +               /* taken from checkout.c;
> +                * we have a simpler case because we never create a branch */

Style: Multi-line comments are formatted like this:

    /*
     * Gobble
     * wobble.
     */

However, this comment doesn't belong in the code, as it won't be
particularly helpful to anyone reading the code in the future. This
sort of note would be more appropriate in the commit message or, even
better, in the commentary section just below the "---" lines following
your Signed-off-by:.

> +               short dash_dash_pos = -1, i = 0;

Same question about why you used 'short' rather than 'int' (especially
as 'dash_dash_pos' is declared 'int' in checkout.c).

Is there a good reason why you initialize 'i' in the declaration
rather than in the for-loop? A _good_ reason to do so in the for-loop
is that doing so makes the for-loop more idiomatic, reduces cognitive
load on readers (since they don't have to chase down the
initialization), and safeguards against someone adding new code
between the declaration and the for-loop which might inadvertently
alter the initial value.

> +               for (; i < argc; i++) {
> +                       if (!strcmp(argv[i], "--")) {
> +                               dash_dash_pos = i;
> +                               break;
> +                       }
> +               }
> +               if (dash_dash_pos == 0) {
> +                       object = "HEAD";
> +                       argv++, argc++;

'argc' is never accessed beyond this point, so changing its value is
pointless. Same observation for the 'else' arms. (And, even if there
was a valid reason to modify 'argc', I think you'd want to be
decrementing it, not incrementing it, to show that you've consumed an
argument.)

> +               } else if (dash_dash_pos == 1) {
> +                       object = argv[0];
> +                       argv += 2, argc += 2;
> +               } else if (dash_dash_pos >= 2)
> +                       die(_("only one reference expected, %d given."), dash_dash_pos);
> +               else if (get_oid(argv[0], &oid)) // not a valid object

Style: Use /* comments */ in this codebase, not // comments.

> +                       object = "HEAD";
> +               else {
> +                       argv++, argc++;
> +                       initialized = 1;
> +               }
> +       }
> +
> +       if (!initialized) // if we've already run get_oid, don't run it again
> +               if (get_oid(object, &oid))
> +                       die("Not a valid object name %s", object);

Can't you accomplish the same without the 'initialized' variable by
instead initializing 'object' to NULL and then checking it here?

    if (object && get_oid(object, &oid))
        die(_("not a valid object name: %s", object);

Re: [PATCH 1/3] ls-tree: make <tree-ish> optional

[Joshua Nelson]

On 07/03/2018 03:15 AM, Eric Sunshine wrote:
> Thanks for contributing to Git. As this seems to be your first
> submission to the project, don't be alarmed by the extent and nature
> of the review comments. They are intended to help you polish the
> submission, and are not meant with ill-intent.
> 
> On Mon, Jul 2, 2018 at 11:58 PM Joshua Nelson <jyn514@gmail.com> wrote:
>> use syntax similar to `git-checkout` to make <tree-ish> optional for
>> `ls-tree`. if <tree-ish> is omitted, default to HEAD. infer arguments as
>> follows:
> 
> Nit: Capitalize first word of each sentence.
> 
> This commit message explains what the patch changes (which is a good
> thing to do), but it's missing the really important explanation of
> _why_ this change is desirable. Without such justification, it's
> difficult to judge if such a change is worthwhile. As this is a
> plumbing command, people may need more convincing (especially if other
> plumbing commands don't provide convenient defaults like this).
> 
>> 1. if args start with --
>>         assume <tree-ish> to be HEAD
>> 2. if exactly one arg precedes --, treat the argument as <tree-ish>
>> 3. if more than one arg precedes --, exit with an error
>> 4. if -- is not in args
>>         a) if args[0] is a valid <tree-ish> object, treat is as such
>>         b) else, assume <tree-ish> to be HEAD
>>
>> in all cases, every argument besides <tree-ish> is treated as a <path>
> 
> This and the other patches are missing your Signed-off-by: (which
> should be placed right here).
> 
> The three patches of this series are all directly related to this one
> change. As such, they should be combined into a single patch. (At the
> very least, 1/3 and 2/3 should be combined; one could argue that 3/3
> is lengthy enough to make it separate, but that's a judgment call.)
> 
> Now, on to the actual code...
> 
>> diff --git builtin/ls-tree.c builtin/ls-tree.c
>> @@ -163,10 +163,39 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
>>             ((LS_TREE_ONLY|LS_RECURSIVE) & ls_options))
>>                 ls_options |= LS_SHOW_TREES;
>>
>> +       const char *object;
>> +       short initialized = 0;
> 
> This project frowns on declaration-after-statement. Move these to the
> top of the {...} block where other variables are declared.
> 
> Why use 'short'? Unless there's a very good reason that this needs to
> be a particular size, you shouldn't deviate from project norm, which
> is to use the natural word size 'int'.
> 
> 'initialized' is too generic, thus isn't a great name.
> 
>>         if (argc < 1)
>> -               usage_with_options(ls_tree_usage, ls_tree_options);
>> -       if (get_oid(argv[0], &oid))
>> -               die("Not a valid object name %s", argv[0]);
>> +               object = "HEAD";
>> +       else {
>> +               /* taken from checkout.c;
>> +                * we have a simpler case because we never create a branch */
> 
> Style: Multi-line comments are formatted like this:
> 
>     /*
>      * Gobble
>      * wobble.
>      */
> 
> However, this comment doesn't belong in the code, as it won't be
> particularly helpful to anyone reading the code in the future. This
> sort of note would be more appropriate in the commit message or, even
> better, in the commentary section just below the "---" lines following
> your Signed-off-by:.

I wasn't aware I could put comments in email generated by
git-send-email, thanks for the tip :)

> 
>> +               short dash_dash_pos = -1, i = 0;
> 
> Same question about why you used 'short' rather than 'int' (especially
> as 'dash_dash_pos' is declared 'int' in checkout.c).
> 
> Is there a good reason why you initialize 'i' in the declaration
> rather than in the for-loop? A _good_ reason to do so in the for-loop
> is that doing so makes the for-loop more idiomatic, reduces cognitive
> load on readers (since they don't have to chase down the
> initialization), and safeguards against someone adding new code
> between the declaration and the for-loop which might inadvertently
> alter the initial value.

No good reason, it happened to end up that way after I finished
debugging. I've changed it to a more conventional declaration.

> 
>> +               for (; i < argc; i++) {
>> +                       if (!strcmp(argv[i], "--")) {
>> +                               dash_dash_pos = i;
>> +                               break;
>> +                       }
>> +               }
>> +               if (dash_dash_pos == 0) {
>> +                       object = "HEAD";
>> +                       argv++, argc++;
> 
> 'argc' is never accessed beyond this point, so changing its value is
> pointless. Same observation for the 'else' arms. (And, even if there
> was a valid reason to modify 'argc', I think you'd want to be
> decrementing it, not incrementing it, to show that you've consumed an
> argument.)
> 
>> +               } else if (dash_dash_pos == 1) {
>> +                       object = argv[0];
>> +                       argv += 2, argc += 2;
>> +               } else if (dash_dash_pos >= 2)
>> +                       die(_("only one reference expected, %d given."), dash_dash_pos);
>> +               else if (get_oid(argv[0], &oid)) // not a valid object
> 
> Style: Use /* comments */ in this codebase, not // comments.
> 
>> +                       object = "HEAD";
>> +               else {
>> +                       argv++, argc++;
>> +                       initialized = 1;
>> +               }
>> +       }
>> +
>> +       if (!initialized) // if we've already run get_oid, don't run it again
>> +               if (get_oid(object, &oid))
>> +                       die("Not a valid object name %s", object);
> 
> Can't you accomplish the same without the 'initialized' variable by
> instead initializing 'object' to NULL and then checking it here?

I think my code wasn't very clear here - 'initialized' checks if 'oid'
has been initialized, not 'object'. AFAIK, structs can't be initialized
to NULL, but my C is not very good so I'd be interested to learn otherwise.

I'm writing a new patch with variable names that are more descriptive.

> 
>     if (object && get_oid(object, &oid))
>         die(_("not a valid object name: %s", object);
> 

[PATCH] ls-tree: make <tree-ish> optional

[Joshua Nelson]

Use syntax similar to `git-checkout` to make <tree-ish> optional for
`ls-tree`. If <tree-ish> is omitted, default to HEAD. Infer arguments as
follows:

1. If args start with '--', assume <tree-ish> to be HEAD
2. If exactly one arg precedes '--', treat the argument as <tree-ish>
3. If more than one arg precedes '--', exit with an error
4. If '--' is not in args:
	a) If args[0] is a valid <tree-ish> object, treat it as such
	b) Else, assume <tree-ish> to be HEAD

In all cases, every argument besides <tree-ish> is treated as a <path>.

Signed-off-by: Joshua Nelson <jyn514@gmail.com>
---

 Documentation/git-ls-tree.txt    |  2 +-
 builtin/ls-tree.c                | 40 ++++++++++++++++----
 t/t3104-ls-tree-optional-args.sh | 63 ++++++++++++++++++++++++++++++++
 3 files changed, 97 insertions(+), 8 deletions(-)
 create mode 100755 t/t3104-ls-tree-optional-args.sh

diff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt
index 9dee7bef3..290fd11c3 100644
--- a/Documentation/git-ls-tree.txt
+++ b/Documentation/git-ls-tree.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 [verse]
 'git ls-tree' [-d] [-r] [-t] [-l] [-z]
 	    [--name-only] [--name-status] [--full-name] [--full-tree] [--abbrev[=<n>]]
-	    <tree-ish> [<path>...]
+	    [<tree-ish>] [--] [<path>...]
 
 DESCRIPTION
 -----------
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 409da4e83..64bfbae71 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -26,7 +26,7 @@ static int chomp_prefix;
 static const char *ls_tree_prefix;
 
 static const  char * const ls_tree_usage[] = {
-	N_("git ls-tree [<options>] <tree-ish> [<path>...]"),
+	N_("git ls-tree [<options>] [<tree-ish>] [--] [<path>...]"),
 	NULL
 };
 
@@ -122,7 +122,9 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 {
 	struct object_id oid;
 	struct tree *tree;
-	int i, full_tree = 0;
+	const char *tree_ish;
+	int i, full_tree = 0, oid_initialized = 0, dash_dash_pos = -1;
+
 	const struct option ls_tree_options[] = {
 		OPT_BIT('d', NULL, &ls_options, N_("only show trees"),
 			LS_TREE_ONLY),
@@ -153,7 +155,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 		chomp_prefix = strlen(prefix);
 
 	argc = parse_options(argc, argv, prefix, ls_tree_options,
-			     ls_tree_usage, 0);
+			     ls_tree_usage, PARSE_OPT_KEEP_DASHDASH);
 	if (full_tree) {
 		ls_tree_prefix = prefix = NULL;
 		chomp_prefix = 0;
@@ -164,9 +166,33 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 		ls_options |= LS_SHOW_TREES;
 
 	if (argc < 1)
-		usage_with_options(ls_tree_usage, ls_tree_options);
-	if (get_oid(argv[0], &oid))
-		die("Not a valid object name %s", argv[0]);
+		tree_ish = "HEAD";
+	else {
+		for (i = 0; i < argc; i++) {
+			if (!strcmp(argv[i], "--")) {
+				dash_dash_pos = i;
+				break;
+			}
+		}
+		if (dash_dash_pos == 0) {
+			tree_ish = "HEAD";
+			argv++;
+		} else if (dash_dash_pos == 1) {
+			tree_ish = argv[0];
+			argv += 2;
+		} else if (dash_dash_pos >= 2)
+			die(_("only one reference expected, %d given."), dash_dash_pos);
+		else if (get_oid(argv[0], &oid)) // not a valid object
+			tree_ish = "HEAD";
+		else {
+			argv++;
+			oid_initialized = 1;
+		}
+	}
+
+	if (!oid_initialized) /* if we've already run get_oid, don't run it again */
+		if (get_oid(tree_ish, &oid))
+			die("Not a valid object name %s", tree_ish);
 
 	/*
 	 * show_recursive() rolls its own matching code and is
@@ -177,7 +203,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 	parse_pathspec(&pathspec, PATHSPEC_ALL_MAGIC &
 				  ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
 		       PATHSPEC_PREFER_CWD,
-		       prefix, argv + 1);
+		       prefix, argv);
 	for (i = 0; i < pathspec.nr; i++)
 		pathspec.items[i].nowildcard_len = pathspec.items[i].len;
 	pathspec.has_wildcard = 0;
diff --git a/t/t3104-ls-tree-optional-args.sh b/t/t3104-ls-tree-optional-args.sh
new file mode 100755
index 000000000..e9d8389bc
--- /dev/null
+++ b/t/t3104-ls-tree-optional-args.sh
@@ -0,0 +1,63 @@
+#!/bin/sh
+
+test_description='ls-tree test (optional args)
+
+This test runs git-ls-tree with ambiguous positional options.'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	echo hi > test &&
+	cp test test2 &&
+	git add test test2 &&
+	git commit -m initial &&
+	printf "100644 blob 45b983be36b73c0788dc9cbcb76cbb80fc7bb057\ttest\n" > expected1 &&
+	printf "100644 blob 45b983be36b73c0788dc9cbcb76cbb80fc7bb057\ttest2\n" > expected2
+'
+
+# cat appends newlines after every file
+test_expect_success 'show HEAD when given no args' '
+	if [ "$(git ls-tree)" != "$(cat expected1 expected2)" ]; then false; fi
+'
+
+test_expect_success 'show HEAD when given only --' '
+	if [ "$(git ls-tree --)" != "$(cat expected1 expected2)" ]; then false; fi
+'
+
+test_expect_success 'setup' '
+	echo hi > test3 &&
+	echo there >> test1 &&
+	git add test3 test1 &&
+	git commit -m "commit 2" &&
+	printf "100644 blob 45b983be36b73c0788dc9cbcb76cbb80fc7bb057\ttest3\n" > expected3
+'
+
+test_expect_success 'show all files when given only revision' '
+	if [ "$(git ls-tree HEAD~1)" != "$(cat expected1 expected2)" ]; then false; fi
+'
+
+test_expect_success 'show all files when given revision and --' '
+	if [ "$(git ls-tree HEAD~1 --)" != "$(cat expected1 expected2)" ]; then false; fi
+'
+
+test_expect_success 'show file when given -- and file' '
+	if [ "$(git ls-tree -- test3)" != "$(cat expected3)" ]; then false; fi
+'
+
+test_expect_success 'show file when given revision and file' '
+	if [ "$(git ls-tree HEAD~1 test)" != "$(cat expected1)" ]; then false; fi
+'
+
+test_expect_success 'show file when given only file' '
+	if [ "$(git ls-tree test3)" != "$(cat expected3)" ]; then false; fi
+'
+
+test_expect_success 'raise error when given bad args' '
+	test_must_fail  git ls-tree HEAD HEAD --
+'
+
+test_expect_success 'raise error when given bad revision' '
+	test_must_fail git ls-tree bad_revision --
+'
+
+test_done
-- 
2.18.GIT

Re: [PATCH] ls-tree: make <tree-ish> optional

[Joshua Nelson]

On 07/03/2018 07:53 PM, Joshua Nelson wrote:
> diff --git a/t/t3104-ls-tree-optional-args.sh b/t/t3104-ls-tree-optional-args.sh
> new file mode 100755
> index 000000000..e9d8389bc
> --- /dev/null
> +++ b/t/t3104-ls-tree-optional-args.sh
> @@ -0,0 +1,63 @@
> +#!/bin/sh
> +
> +test_description='ls-tree test (optional args)
> +
> +This test runs git-ls-tree with ambiguous positional options.'
> +
> +# cat appends newlines after every file
> +test_expect_success 'show HEAD when given no args' '
> +	if [ "$(git ls-tree)" != "$(cat expected1 expected2)" ]; then false; fi
> +'

I forgot to take out the comment about cat; it's not actually true, I
just happened to be using echo instead of printf when I wrote it.

Is it customary to send a new patch or second patch that builds on the
first? If I specify a commit range to git-send-email it sends both, not
just the latest.

Re: [PATCH] ls-tree: make <tree-ish> optional

[Eric Sunshine]

On Tue, Jul 3, 2018 at 8:05 PM Joshua Nelson <jyn514@gmail.com> wrote:
> Is it customary to send a new patch or second patch that builds on the
> first?

It depends what you mean. If there were problems with a version of a
patch you sent, then you "re-roll", which means you re-send the patch
in its entirety as if it's brand new (not building on the problematic
previous version). You'd also indicate the attempt number in the
subject; for instance "[PATCH v2]" for the second attempt.

If you're working on some functionality that is built up over a series
steps, then you would send all those patches as a "patch series", in
which each patch in the series builds upon the patch or patches
preceding it in the series.

> If I specify a commit range to git-send-email it sends both, not
> just the latest.

I'm not quite sure what you mean.

Re: [PATCH] ls-tree: make <tree-ish> optional

[Eric Sunshine]

On Tue, Jul 3, 2018 at 7:57 PM Joshua Nelson <jyn514@gmail.com> wrote:
> Use syntax similar to `git-checkout` to make <tree-ish> optional for
> `ls-tree`. If <tree-ish> is omitted, default to HEAD. Infer arguments as
> follows:
>
> 1. If args start with '--', assume <tree-ish> to be HEAD
> 2. If exactly one arg precedes '--', treat the argument as <tree-ish>
> 3. If more than one arg precedes '--', exit with an error
> 4. If '--' is not in args:
>         a) If args[0] is a valid <tree-ish> object, treat it as such
>         b) Else, assume <tree-ish> to be HEAD
>
> In all cases, every argument besides <tree-ish> is treated as a <path>.

See review comments below. However, it's not clear if you should be
putting more effort into this submissions since Junio did not seem too
interested in such a behavior change[1]. Perhaps wait for word from
him before acting on any of the below comments.

[1]: https://public-inbox.org/git/xmqqbmbonff3.fsf@gitster-ct.c.googlers.com/

> Signed-off-by: Joshua Nelson <jyn514@gmail.com>
> ---
> diff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt
> @@ -11,7 +11,7 @@ SYNOPSIS
>  'git ls-tree' [-d] [-r] [-t] [-l] [-z]
>             [--name-only] [--name-status] [--full-name] [--full-tree] [--abbrev[=<n>]]
> -           <tree-ish> [<path>...]
> +           [<tree-ish>] [--] [<path>...]

The documentation should also explain what happens when the
now-optional <tree-ish> is omitted. A bit later in this document, you
find:

    <tree-ish>
        Id of a tree-ish.

You probably want to amend this to add: "When omitted, defaults to HEAD."

> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
> @@ -122,7 +122,9 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
> +       const char *tree_ish;

I think 'treeish' would be a more common way to spell this in this code base.

> @@ -164,9 +166,33 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
>         if (argc < 1)
> +               tree_ish = "HEAD";
> +       else {
> +               for (i = 0; i < argc; i++) {
> +                       if (!strcmp(argv[i], "--")) {
> +                               dash_dash_pos = i;
> +                               break;
> +                       }
> +               }
> +               if (dash_dash_pos == 0) {
> +                       tree_ish = "HEAD";
> +                       argv++;
> +               } else if (dash_dash_pos == 1) {
> +                       tree_ish = argv[0];
> +                       argv += 2;
> +               } else if (dash_dash_pos >= 2)
> +                       die(_("only one reference expected, %d given."), dash_dash_pos);
> +               else if (get_oid(argv[0], &oid)) // not a valid object

Mentioned previously: Use /* comments */ in this code-base, not // comments.

> +                       tree_ish = "HEAD";
> +               else {
> +                       argv++;
> +                       oid_initialized = 1;
> +               }
> +       }
> +
> +       if (!oid_initialized) /* if we've already run get_oid, don't run it again */
> +               if (get_oid(tree_ish, &oid))
> +                       die("Not a valid object name %s", tree_ish);

As explained in [2], I think 'oid_initialized' is unneeded since it
can be inferred from 'tree_ish', thus this code could be simplified.

[2]: https://public-inbox.org/git/CAPig+cQu-e63NUUXAB_QA+M-rgPfqBLOOm5fdjsSVuWHJt7TJA@mail.gmail.com/

> diff --git a/t/t3104-ls-tree-optional-args.sh b/t/t3104-ls-tree-optional-args.sh
> @@ -0,0 +1,63 @@
> +test_expect_success 'setup' '
> +       echo hi > test &&

Mentioned previously: Drop whitespace after ">". Same comment applies
to several other places in this script.

> +       cp test test2 &&
> +       git add test test2 &&
> +       git commit -m initial &&
> +       printf "100644 blob 45b983be36b73c0788dc9cbcb76cbb80fc7bb057\ttest\n" > expected1 &&
> +       printf "100644 blob 45b983be36b73c0788dc9cbcb76cbb80fc7bb057\ttest2\n" > expected2
> +'

There's work being done on Git to make it possible to swap in a new
hash algorithm in place of SHA-1, which means that these hard-coded
hash values won't fly. Instead, you'd want to determine them
dynamically. For instance:

    ...
    printf "100644 blob %s\ttest\n" $(git rev-parse HEAD:test) >expected1 &&
    printf "100644 blob %s\ttest2\n" $(git rev-parse HEAD:test2) >expected2

or something.

> +test_expect_success 'show HEAD when given no args' '
> +       if [ "$(git ls-tree)" != "$(cat expected1 expected2)" ]; then false; fi
> +'

In this code-base, we use 'test' rather than '[', and we wouldn't
bother with stuffing the comparison in an 'if' statement since its
value can be used directly as the test result. However, better is to
dump the result of git-ls-tree to a file and then use test_cmp() to
compare the expected and actual output. If the expected and actual
output don't match, test_cmp() will show the differences to aid
debugging. So, you might write this test like this:

    test_expect_success 'show HEAD when given no args' '
        cat expected1 expected2 >expected &&
        git ls-tree >actual &&
        test_cmp expected actual
    '

Same comment regarding remaining tests.

Re: [PATCH 1/3] ls-tree: make <tree-ish> optional

[Eric Sunshine]

On Tue, Jul 3, 2018 at 7:15 PM Joshua Nelson <jyn514@gmail.com> wrote:
> On 07/03/2018 03:15 AM, Eric Sunshine wrote:
> >> +               /* taken from checkout.c;
> >> +                * we have a simpler case because we never create a branch */
> >
> > However, this comment doesn't belong in the code, as it won't be
> > particularly helpful to anyone reading the code in the future. This
> > sort of note would be more appropriate in the commit message or, even
> > better, in the commentary section just below the "---" lines following
> > your Signed-off-by:.
>
> I wasn't aware I could put comments in email generated by
> git-send-email, thanks for the tip :)

An even more common workflow is to use git-format-patch to generate
the patches locally, then edit the patches to add commentary below the
"---" line if needed, proof-read everything, and finally use
git-send-email to send out the already-generated patches.

> >> +                       object = "HEAD";
> >> +               else {
> >> +                       argv++, argc++;
> >> +                       initialized = 1;
> >> +               }
> >> +       }
> >> +
> >> +       if (!initialized) // if we've already run get_oid, don't run it again
> >> +               if (get_oid(object, &oid))
> >> +                       die("Not a valid object name %s", object);
> >
> > Can't you accomplish the same without the 'initialized' variable by
> > instead initializing 'object' to NULL and then checking it here?
>
> I think my code wasn't very clear here - 'initialized' checks if 'oid'
> has been initialized, not 'object'. AFAIK, structs can't be initialized
> to NULL, but my C is not very good so I'd be interested to learn otherwise.

Oh, the intention of 'initialized' was quite clear, but it seems
unnecessary, which is why I was suggesting an alternative. Unless I'm
misunderstanding the code (certainly a possibility), I think
'initialized' is redundant, thus you can get by without it
Specifically, the only time you set 'initialized' is when you don't
set 'object', which means that you can infer whether 'oid' was
initialized based upon whether 'object' was set. So, my suggestion
was:

    const char *object = NULL;
    ...
    ... conditionals possibly assigning to 'object' ...
    ...
    if (object && get_oid(object, &oid))
        die(_("not a valid object name: %s", object);

If you arrived at that final 'if' statement and object is still NULL,
then you know that 'oid' is already initialized; if 'object' is not
NULL, then you use it to initialized 'oid'.