threads / patch / 48820

patch, 3 partsls-tree: update usage info

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

## tl;dr

21 messages between Jul 3, 2018 and Jul 6, 2018. Diffs are folded; open one to read it.

replies: 20people: 4as markdown or json

Joshua Nelson· Jul 3, 2018, 03:58 UTC · lore

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

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(-)
Show changes to diff +34 −5
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
Joshua Nelson· Jul 3, 2018, 03:58 UTC · re: Joshua Nelson · lore
show [tree-ish] and [--] as optional
---
 builtin/ls-tree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to diff +1 −1
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
Elijah Newren· Jul 3, 2018, 07:14 UTC · re: Joshua Nelson · lore

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

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.
Show 5 quoted lines
> ---
>  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?
Show 14 quoted lines
> 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
Eric Sunshine· Jul 3, 2018, 07:18 UTC · re: Joshua Nelson · lore

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

On Mon, Jul 2, 2018 at 11:58 PM Joshua Nelson <jyn514@gmail.com> wrote:
Show 7 quoted lines
> 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.
Joshua Nelson· Jul 3, 2018, 03:58 UTC · re: Joshua Nelson · lore

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

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
Show changes to diff +43 −0
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
Elijah Newren· Jul 3, 2018, 07:30 UTC · re: Joshua Nelson · lore

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

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.

Show 5 quoted lines
> 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.
Show 12 quoted lines
>
> 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?

Show 7 quoted lines
> +
> +. ./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.

Show 15 quoted lines
> +
> +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?

Show 15 quoted lines
> +
> +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. :-)

Eric Sunshine· Jul 3, 2018, 07:33 UTC · re: Joshua Nelson · lore

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

On Mon, Jul 2, 2018 at 11:58 PM Joshua Nelson <jyn514@gmail.com> wrote:
Show 6 quoted lines
> 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.

Show 5 quoted lines
> +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.

Show 5 quoted lines
> +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.
Show 7 quoted lines
> +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 --'
Elijah Newren· Jul 3, 2018, 07:12 UTC · re: Joshua Nelson · lore

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

On Mon, Jul 2, 2018 at 8:58 PM, Joshua Nelson <jyn514@gmail.com> wrote:
Show 13 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 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...
Show 8 quoted lines
> ---
>  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.

Show 15 quoted lines
> @@ -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).

Show 8 quoted lines
>         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.

Show 39 quoted lines
> +                       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
>
Junio C Hamano· Jul 3, 2018, 22:05 UTC · re: Elijah Newren · lore

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

Elijah Newren <newren@gmail.com> writes:
Show 16 quoted lines
> 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.

Elijah Newren· Jul 3, 2018, 22:55 UTC · re: Junio C Hamano · lore

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

On Tue, Jul 3, 2018 at 3:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 20 quoted lines
> 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?

Joshua Nelson· Jul 3, 2018, 22:58 UTC · re: Elijah Newren · lore

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

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:
Show 33 quoted lines
> 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?
Junio C Hamano· Jul 6, 2018, 17:01 UTC · re: Elijah Newren · lore

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

Elijah Newren <newren@gmail.com> writes:
Show 7 quoted lines
>> 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.

Joshua Nelson· Jul 6, 2018, 21:26 UTC · re: Junio C Hamano · lore

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

On 07/06/2018 01:01 PM, Junio C Hamano wrote:
Show 15 quoted lines
> 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.
Junio C Hamano· Jul 6, 2018, 21:32 UTC · re: Joshua Nelson · lore

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

Joshua Nelson <jyn514@gmail.com> writes:
Show 23 quoted lines
> 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.
Eric Sunshine· Jul 3, 2018, 07:15 UTC · re: Joshua Nelson · lore

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

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

Show 9 quoted lines
> 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...
Show 7 quoted lines
> 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.
Show 8 quoted lines
>         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.

Show 9 quoted lines
> +               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.)

Show 6 quoted lines
> +               } 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.
Show 10 quoted lines
> +                       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);
Joshua Nelson· Jul 3, 2018, 23:15 UTC · re: Eric Sunshine · lore

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

On 07/03/2018 03:15 AM, Eric Sunshine wrote:
Show 77 quoted lines
> 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 :)

Show 13 quoted lines
> 
>> +               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.

Show 39 quoted lines
> 
>> +               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);
>
Joshua Nelson· Jul 3, 2018, 23:53 UTC · re: Joshua Nelson · lore

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

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
Show changes to 3 files +97 −8

Documentation/git-ls-tree.txt, builtin/ls-tree.c, 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
Joshua Nelson· Jul 4, 2018, 00:05 UTC · re: Joshua Nelson · lore

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

On 07/03/2018 07:53 PM, Joshua Nelson wrote:
Show 16 quoted lines
> 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.

Eric Sunshine· Jul 4, 2018, 09:38 UTC · re: Joshua Nelson · lore

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

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.
Eric Sunshine· Jul 4, 2018, 10:04 UTC · re: Joshua Nelson · lore

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

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.
Eric Sunshine· Jul 4, 2018, 09:29 UTC · re: Joshua Nelson · lore

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

On Tue, Jul 3, 2018 at 7:15 PM Joshua Nelson <jyn514@gmail.com> wrote:
Show 12 quoted lines
> 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.

Show 17 quoted lines
> >> +                       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'.

← back to recent threads