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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 4, 2018, 10:04 UTC
Message-ID: <CAPig+cSkQ9FM=o7icJY-eSoJ0Po_1v8sb0oRX1T6J42N0dOisA@mail.gmail.com>
In-Reply-To: <20180703235337.31770-1-jyn514@gmail.com>

On Tue, Jul 3, 2018 at 7:57 PM Joshua Nelson <jyn514@gmail.com> wrote:

Show 12 quoted lines

> 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/

Show 8 quoted lines

> 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.

Show 19 quoted lines

> @@ -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.

Show 10 quoted lines

> +                       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.

Show 6 quoted lines

> +       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.