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

[Eric Sunshine <sunshine@sunshineco.com>]

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.