{"thread":{"id":"48820","subject":"[PATCH 2/3] ls-tree: update usage info","startedAt":"2018-07-03T03:58:43Z","lastAt":"2018-07-06T21:32:38Z","messageCount":21,"participants":["Joshua Nelson","Elijah Newren","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"351573","messageId":"20180703035802.24060-2-jyn514@gmail.com","threadId":"48820","inReplyTo":"20180703035802.24060-1-jyn514@gmail.com","subject":"[PATCH 2/3] ls-tree: update usage info","fromName":"Joshua Nelson","fromEmail":"jyn514@gmail.com","sentAt":"2018-07-03T03:58:01Z","receivedAt":"2018-07-03T03:58:43Z","isPatch":true,"sender":{"key":"jyn514@gmail.com","avatar":"https://gravatar.com/avatar/1eb2e767b4f956ee324f49d8726ec382595377f83526b0b0b4aa5cd4dd0bd5b9?d=mp&s=160"},"body":"show [tree-ish] and [--] as optional\n---\n builtin/ls-tree.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git builtin/ls-tree.c builtin/ls-tree.c\nindex 14102b052..c5649b09c 100644\n--- builtin/ls-tree.c\n+++ builtin/ls-tree.c\n@@ -26,7 +26,7 @@ static int chomp_prefix;\n static const char *ls_tree_prefix;\n \n static const  char * const ls_tree_usage[] = {\n-\tN_(\"git ls-tree [<options>] <tree-ish> [<path>...]\"),\n+\tN_(\"git ls-tree [<options>] [tree-ish] [--] [<path>...]\"),\n \tNULL\n };\n \n-- \n2.18.GIT\n\n"},{"id":"351574","messageId":"20180703035802.24060-1-jyn514@gmail.com","threadId":"48820","inReplyTo":null,"subject":"[PATCH 1/3] ls-tree: make <tree-ish> optional","fromName":"Joshua Nelson","fromEmail":"jyn514@gmail.com","sentAt":"2018-07-03T03:58:00Z","receivedAt":"2018-07-03T03:58:45Z","isPatch":true,"sender":{"key":"jyn514@gmail.com","avatar":"https://gravatar.com/avatar/1eb2e767b4f956ee324f49d8726ec382595377f83526b0b0b4aa5cd4dd0bd5b9?d=mp&s=160"},"body":"use syntax similar to `git-checkout` to make <tree-ish> optional for\n`ls-tree`. if <tree-ish> is omitted, default to HEAD. infer arguments as\nfollows:\n\n1. if args start with --\n\tassume <tree-ish> to be HEAD\n2. if exactly one arg precedes --, treat the argument as <tree-ish>\n3. if more than one arg precedes --, exit with an error\n4. if -- is not in args\n\ta) if args[0] is a valid <tree-ish> object, treat is as such\n\tb) else, assume <tree-ish> to be HEAD\n\nin all cases, every argument besides <tree-ish> is treated as a <path>\n---\n builtin/ls-tree.c | 39 ++++++++++++++++++++++++++++++++++-----\n 1 file changed, 34 insertions(+), 5 deletions(-)\n\ndiff --git builtin/ls-tree.c builtin/ls-tree.c\nindex 409da4e83..14102b052 100644\n--- builtin/ls-tree.c\n+++ builtin/ls-tree.c\n@@ -153,7 +153,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n \t\tchomp_prefix = strlen(prefix);\n \n \targc = parse_options(argc, argv, prefix, ls_tree_options,\n-\t\t\t     ls_tree_usage, 0);\n+\t\t\t     ls_tree_usage, PARSE_OPT_KEEP_DASHDASH);\n \tif (full_tree) {\n \t\tls_tree_prefix = prefix = NULL;\n \t\tchomp_prefix = 0;\n@@ -163,10 +163,39 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n \t    ((LS_TREE_ONLY|LS_RECURSIVE) & ls_options))\n \t\tls_options |= LS_SHOW_TREES;\n \n+\tconst char *object;\n+\tshort initialized = 0;\n \tif (argc < 1)\n-\t\tusage_with_options(ls_tree_usage, ls_tree_options);\n-\tif (get_oid(argv[0], &oid))\n-\t\tdie(\"Not a valid object name %s\", argv[0]);\n+\t\tobject = \"HEAD\";\n+\telse {\n+\t\t/* taken from checkout.c;\n+\t\t * we have a simpler case because we never create a branch */\n+\t\tshort dash_dash_pos = -1, i = 0;\n+\t\tfor (; i < argc; i++) {\n+\t\t\tif (!strcmp(argv[i], \"--\")) {\n+\t\t\t\tdash_dash_pos = i;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t\tif (dash_dash_pos == 0) {\n+\t\t\tobject = \"HEAD\";\n+\t\t\targv++, argc++;\n+\t\t} else if (dash_dash_pos == 1) {\n+\t\t\tobject = argv[0];\n+\t\t\targv += 2, argc += 2;\n+\t\t} else if (dash_dash_pos >= 2)\n+\t\t\tdie(_(\"only one reference expected, %d given.\"), dash_dash_pos);\n+\t\telse if (get_oid(argv[0], &oid)) // not a valid object\n+\t\t\tobject = \"HEAD\";\n+\t\telse {\n+\t\t\targv++, argc++;\n+\t\t\tinitialized = 1;\n+\t\t}\n+\t}\n+\n+\tif (!initialized) // if we've already run get_oid, don't run it again\n+\t\tif (get_oid(object, &oid))\n+\t\t\tdie(\"Not a valid object name %s\", object);\n \n \t/*\n \t * show_recursive() rolls its own matching code and is\n@@ -177,7 +206,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n \tparse_pathspec(&pathspec, PATHSPEC_ALL_MAGIC &\n \t\t\t\t  ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),\n \t\t       PATHSPEC_PREFER_CWD,\n-\t\t       prefix, argv + 1);\n+\t\t       prefix, argv);\n \tfor (i = 0; i < pathspec.nr; i++)\n \t\tpathspec.items[i].nowildcard_len = pathspec.items[i].len;\n \tpathspec.has_wildcard = 0;\n-- \n2.18.GIT\n\n"},{"id":"351575","messageId":"20180703035802.24060-3-jyn514@gmail.com","threadId":"48820","inReplyTo":"20180703035802.24060-1-jyn514@gmail.com","subject":"[PATCH 3/3] ls-tree: add unit tests for arguments","fromName":"Joshua Nelson","fromEmail":"jyn514@gmail.com","sentAt":"2018-07-03T03:58:02Z","receivedAt":"2018-07-03T03:58:48Z","isPatch":true,"sender":{"key":"jyn514@gmail.com","avatar":"https://gravatar.com/avatar/1eb2e767b4f956ee324f49d8726ec382595377f83526b0b0b4aa5cd4dd0bd5b9?d=mp&s=160"},"body":"Signed-off-by: Joshua Nelson <jyn514@gmail.com>\n---\n t/t3104-ls-tree-optional-args.sh | 43 ++++++++++++++++++++++++++++++++\n 1 file changed, 43 insertions(+)\n create mode 100644 t/t3104-ls-tree-optional-args.sh\n\ndiff --git t/t3104-ls-tree-optional-args.sh t/t3104-ls-tree-optional-args.sh\nnew file mode 100644\nindex 000000000..5917563a7\n--- /dev/null\n+++ t/t3104-ls-tree-optional-args.sh\n@@ -0,0 +1,43 @@\n+#!/bin/sh\n+\n+test_description='ls-tree test (optional args)\n+\n+This test tries to run git-ls-tree with various combinations of options.'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'initial setup' '\n+echo hi > test && cp test test2 && git add test test2 && git commit -m initial'\n+\n+# cat appends newlines after every file\n+test_expect_success 'succeed when given no args' 'git ls-tree'\n+\n+test_expect_success 'succeed when given only --' 'git ls-tree'\n+\n+test_expect_success 'add second commit' '\n+echo hi > test3 && git add test3 && git commit -m \"commit 2\"'\n+\n+test_expect_success 'succeed when given revision' '\n+git ls-tree HEAD~1'\n+\n+test_expect_success 'succeed when given revision and --' '\n+git ls-tree HEAD~1 --'\n+\n+test_expect_success 'succeed when given -- and file' '\n+git ls-tree -- test3'\n+\n+test_expect_success 'do nothing when given bad files' '\n+git ls-tree -- bad_files'\n+\n+test_expect_success 'succeed when given file from past revision' '\n+git ls-tree HEAD~1 test'\n+\n+test_expect_success 'succeed when given only file' 'git ls-tree test'\n+\n+test_expect_success 'raise error when given bad args' '\n+test_must_fail  git ls-tree HEAD HEAD --'\n+\n+test_expect_success 'raise error when given bad revision' '\n+test_must_fail git ls-tree bad_revision --'\n+\n+test_done\n-- \n2.18.GIT\n\n"},{"id":"351577","messageId":"CABPp-BFu+m-7Fyf4kj8hUO33vhAxKuAu5JnyL5Tzc7c6Kq+Hjg@mail.gmail.com","threadId":"48820","inReplyTo":"20180703035802.24060-1-jyn514@gmail.com","subject":"Re: [PATCH 1/3] ls-tree: make <tree-ish> optional","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-03T07:12:43Z","receivedAt":"2018-07-03T07:12:48Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Jul 2, 2018 at 8:58 PM, Joshua Nelson <jyn514@gmail.com> wrote:\n> use syntax similar to `git-checkout` to make <tree-ish> optional for\n> `ls-tree`. if <tree-ish> is omitted, default to HEAD. infer arguments as\n> follows:\n>\n> 1. if args start with --\n>         assume <tree-ish> to be HEAD\n> 2. if exactly one arg precedes --, treat the argument as <tree-ish>\n> 3. if more than one arg precedes --, exit with an error\n> 4. if -- is not in args\n>         a) if args[0] is a valid <tree-ish> object, treat is as such\n>         b) else, assume <tree-ish> to be HEAD\n>\n> in all cases, every argument besides <tree-ish> is treated as a <path>\n\nCool, this is something I've wanted a few times.  Thanks for\nsubmitting.  A few minor issues:\n  1. Could you start sentences with capitals?  Thus, 'Use syntax...',\n'Infer arguments...', 'In all cases...', etc.\n  2. Missing Signed-off-by on this commit (and commit 2); I notice you\ncorrectly added it to commit 3.\n\nAlso, additional issues below...\n\n> ---\n>  builtin/ls-tree.c | 39 ++++++++++++++++++++++++++++++++++-----\n>  1 file changed, 34 insertions(+), 5 deletions(-)\n>\n> diff --git builtin/ls-tree.c builtin/ls-tree.c\n> index 409da4e83..14102b052 100644\n> --- builtin/ls-tree.c\n> +++ builtin/ls-tree.c\n\nDo you have diff.noprefix set?  It took me a little bit to realize\nthat this was the reason your patches weren't applying for me.\n\n> @@ -153,7 +153,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n>                 chomp_prefix = strlen(prefix);\n>\n>         argc = parse_options(argc, argv, prefix, ls_tree_options,\n> -                            ls_tree_usage, 0);\n> +                            ls_tree_usage, PARSE_OPT_KEEP_DASHDASH);\n>         if (full_tree) {\n>                 ls_tree_prefix = prefix = NULL;\n>                 chomp_prefix = 0;\n> @@ -163,10 +163,39 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n>             ((LS_TREE_ONLY|LS_RECURSIVE) & ls_options))\n>                 ls_options |= LS_SHOW_TREES;\n>\n> +       const char *object;\n> +       short initialized = 0;\n\nThis will fail to compile under 'make DEVELOPER=1' (ISO C90 forbids\nmixed declarations and code).\n\n>         if (argc < 1)\n> -               usage_with_options(ls_tree_usage, ls_tree_options);\n> -       if (get_oid(argv[0], &oid))\n> -               die(\"Not a valid object name %s\", argv[0]);\n> +               object = \"HEAD\";\n> +       else {\n> +               /* taken from checkout.c;\n> +                * we have a simpler case because we never create a branch */\n\n/*\n * multi-line comment style in git utilizes puts the first word on the\n * second line, and terminates on its own line, like this.\n */\n\nAlso, I was going to ask if this code should be put into a utility\nfunction or something, but it isn't taken verbatim from checkout.c,\njust adopted from there.\n\n> +               short dash_dash_pos = -1, i = 0;\n> +               for (; i < argc; i++) {\n\nAny reason you moved the i = 0 to the line before?  I thought 'i' was\nuninitialized and almost commented to that effect before I noticed\nwhere it was.\n\n> +                       if (!strcmp(argv[i], \"--\")) {\n> +                               dash_dash_pos = i;\n> +                               break;\n> +                       }\n> +               }\n> +               if (dash_dash_pos == 0) {\n> +                       object = \"HEAD\";\n> +                       argv++, argc++;\n> +               } else if (dash_dash_pos == 1) {\n> +                       object = argv[0];\n> +                       argv += 2, argc += 2;\n> +               } else if (dash_dash_pos >= 2)\n> +                       die(_(\"only one reference expected, %d given.\"), dash_dash_pos);\n> +               else if (get_oid(argv[0], &oid)) // not a valid object\n> +                       object = \"HEAD\";\n> +               else {\n> +                       argv++, argc++;\n> +                       initialized = 1;\n> +               }\n> +       }\n> +\n> +       if (!initialized) // if we've already run get_oid, don't run it again\n> +               if (get_oid(object, &oid))\n> +                       die(\"Not a valid object name %s\", object);\n>\n>         /*\n>          * show_recursive() rolls its own matching code and is\n> @@ -177,7 +206,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n>         parse_pathspec(&pathspec, PATHSPEC_ALL_MAGIC &\n>                                   ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),\n>                        PATHSPEC_PREFER_CWD,\n> -                      prefix, argv + 1);\n> +                      prefix, argv);\n>         for (i = 0; i < pathspec.nr; i++)\n>                 pathspec.items[i].nowildcard_len = pathspec.items[i].len;\n>         pathspec.has_wildcard = 0;\n> --\n> 2.18.GIT\n>\n"},{"id":"351578","messageId":"CABPp-BFLciH4H712LzwXyn22esRU6FN87SLbUqOJP3CKvx+DdA@mail.gmail.com","threadId":"48820","inReplyTo":"20180703035802.24060-2-jyn514@gmail.com","subject":"Re: [PATCH 2/3] ls-tree: update usage info","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-03T07:14:18Z","receivedAt":"2018-07-03T07:14:22Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Joshua,\n\nOn Mon, Jul 2, 2018 at 8:58 PM, Joshua Nelson <jyn514@gmail.com> wrote:\n> show [tree-ish] and [--] as optional\n\nYou're missing a Signed-off-by tag here.\n\n> ---\n>  builtin/ls-tree.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git builtin/ls-tree.c builtin/ls-tree.c\n\nNo corresponding update to Documentation/git-ls-tree.txt?\n\n> index 14102b052..c5649b09c 100644\n> --- builtin/ls-tree.c\n> +++ builtin/ls-tree.c\n> @@ -26,7 +26,7 @@ static int chomp_prefix;\n>  static const char *ls_tree_prefix;\n>\n>  static const  char * const ls_tree_usage[] = {\n> -       N_(\"git ls-tree [<options>] <tree-ish> [<path>...]\"),\n> +       N_(\"git ls-tree [<options>] [tree-ish] [--] [<path>...]\"),\n>         NULL\n>  };\n>\n> --\n> 2.18.GIT\n"},{"id":"351579","messageId":"CAPig+cT9XB-nJFm0rD9RckBzcbk9vh8Hz=3XOA-HA-JoXssG_A@mail.gmail.com","threadId":"48820","inReplyTo":"20180703035802.24060-1-jyn514@gmail.com","subject":"Re: [PATCH 1/3] ls-tree: make <tree-ish> optional","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-03T07:15:54Z","receivedAt":"2018-07-03T07:16:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Thanks for contributing to Git. As this seems to be your first\nsubmission to the project, don't be alarmed by the extent and nature\nof the review comments. They are intended to help you polish the\nsubmission, and are not meant with ill-intent.\n\nOn Mon, Jul 2, 2018 at 11:58 PM Joshua Nelson <jyn514@gmail.com> wrote:\n> use syntax similar to `git-checkout` to make <tree-ish> optional for\n> `ls-tree`. if <tree-ish> is omitted, default to HEAD. infer arguments as\n> follows:\n\nNit: Capitalize first word of each sentence.\n\nThis commit message explains what the patch changes (which is a good\nthing to do), but it's missing the really important explanation of\n_why_ this change is desirable. Without such justification, it's\ndifficult to judge if such a change is worthwhile. As this is a\nplumbing command, people may need more convincing (especially if other\nplumbing commands don't provide convenient defaults like this).\n\n> 1. if args start with --\n>         assume <tree-ish> to be HEAD\n> 2. if exactly one arg precedes --, treat the argument as <tree-ish>\n> 3. if more than one arg precedes --, exit with an error\n> 4. if -- is not in args\n>         a) if args[0] is a valid <tree-ish> object, treat is as such\n>         b) else, assume <tree-ish> to be HEAD\n>\n> in all cases, every argument besides <tree-ish> is treated as a <path>\n\nThis and the other patches are missing your Signed-off-by: (which\nshould be placed right here).\n\nThe three patches of this series are all directly related to this one\nchange. As such, they should be combined into a single patch. (At the\nvery least, 1/3 and 2/3 should be combined; one could argue that 3/3\nis lengthy enough to make it separate, but that's a judgment call.)\n\nNow, on to the actual code...\n\n> diff --git builtin/ls-tree.c builtin/ls-tree.c\n> @@ -163,10 +163,39 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n>             ((LS_TREE_ONLY|LS_RECURSIVE) & ls_options))\n>                 ls_options |= LS_SHOW_TREES;\n>\n> +       const char *object;\n> +       short initialized = 0;\n\nThis project frowns on declaration-after-statement. Move these to the\ntop of the {...} block where other variables are declared.\n\nWhy use 'short'? Unless there's a very good reason that this needs to\nbe a particular size, you shouldn't deviate from project norm, which\nis to use the natural word size 'int'.\n\n'initialized' is too generic, thus isn't a great name.\n\n>         if (argc < 1)\n> -               usage_with_options(ls_tree_usage, ls_tree_options);\n> -       if (get_oid(argv[0], &oid))\n> -               die(\"Not a valid object name %s\", argv[0]);\n> +               object = \"HEAD\";\n> +       else {\n> +               /* taken from checkout.c;\n> +                * we have a simpler case because we never create a branch */\n\nStyle: Multi-line comments are formatted like this:\n\n    /*\n     * Gobble\n     * wobble.\n     */\n\nHowever, this comment doesn't belong in the code, as it won't be\nparticularly helpful to anyone reading the code in the future. This\nsort of note would be more appropriate in the commit message or, even\nbetter, in the commentary section just below the \"---\" lines following\nyour Signed-off-by:.\n\n> +               short dash_dash_pos = -1, i = 0;\n\nSame question about why you used 'short' rather than 'int' (especially\nas 'dash_dash_pos' is declared 'int' in checkout.c).\n\nIs there a good reason why you initialize 'i' in the declaration\nrather than in the for-loop? A _good_ reason to do so in the for-loop\nis that doing so makes the for-loop more idiomatic, reduces cognitive\nload on readers (since they don't have to chase down the\ninitialization), and safeguards against someone adding new code\nbetween the declaration and the for-loop which might inadvertently\nalter the initial value.\n\n> +               for (; i < argc; i++) {\n> +                       if (!strcmp(argv[i], \"--\")) {\n> +                               dash_dash_pos = i;\n> +                               break;\n> +                       }\n> +               }\n> +               if (dash_dash_pos == 0) {\n> +                       object = \"HEAD\";\n> +                       argv++, argc++;\n\n'argc' is never accessed beyond this point, so changing its value is\npointless. Same observation for the 'else' arms. (And, even if there\nwas a valid reason to modify 'argc', I think you'd want to be\ndecrementing it, not incrementing it, to show that you've consumed an\nargument.)\n\n> +               } else if (dash_dash_pos == 1) {\n> +                       object = argv[0];\n> +                       argv += 2, argc += 2;\n> +               } else if (dash_dash_pos >= 2)\n> +                       die(_(\"only one reference expected, %d given.\"), dash_dash_pos);\n> +               else if (get_oid(argv[0], &oid)) // not a valid object\n\nStyle: Use /* comments */ in this codebase, not // comments.\n\n> +                       object = \"HEAD\";\n> +               else {\n> +                       argv++, argc++;\n> +                       initialized = 1;\n> +               }\n> +       }\n> +\n> +       if (!initialized) // if we've already run get_oid, don't run it again\n> +               if (get_oid(object, &oid))\n> +                       die(\"Not a valid object name %s\", object);\n\nCan't you accomplish the same without the 'initialized' variable by\ninstead initializing 'object' to NULL and then checking it here?\n\n    if (object && get_oid(object, &oid))\n        die(_(\"not a valid object name: %s\", object);\n"},{"id":"351580","messageId":"CAPig+cTf3iUSuopwKj_8OC9qf3kpD9=_Aika+_Xb-_kt1TxgPQ@mail.gmail.com","threadId":"48820","inReplyTo":"20180703035802.24060-2-jyn514@gmail.com","subject":"Re: [PATCH 2/3] ls-tree: update usage info","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-03T07:18:27Z","receivedAt":"2018-07-03T07:18:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jul 2, 2018 at 11:58 PM Joshua Nelson <jyn514@gmail.com> wrote:\n> show [tree-ish] and [--] as optional\n> ---\n> diff --git builtin/ls-tree.c builtin/ls-tree.c\n> @@ -26,7 +26,7 @@ static int chomp_prefix;\n>  static const  char * const ls_tree_usage[] = {\n> -       N_(\"git ls-tree [<options>] <tree-ish> [<path>...]\"),\n> +       N_(\"git ls-tree [<options>] [tree-ish] [--] [<path>...]\"),\n\nYou lost the '<' and '>'. This should be typeset as:\n\n    git ls-tree [<options>] [<tree-ish>] [--] [<path>...]\n\nAnd, as Elijah noted, Documentation/git-ls-tree.txt needs an update.\n"},{"id":"351581","messageId":"CABPp-BG95d4uWa_86qu5oVK9sYuVtDP3gv1JH4KXdzvMy0h_dA@mail.gmail.com","threadId":"48820","inReplyTo":"20180703035802.24060-3-jyn514@gmail.com","subject":"Re: [PATCH 3/3] ls-tree: add unit tests for arguments","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-03T07:30:54Z","receivedAt":"2018-07-03T07:30:59Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Joshua,\n\nOn Mon, Jul 2, 2018 at 8:58 PM, Joshua Nelson <jyn514@gmail.com> wrote:\n\nThe commit message could use some clarification; it currently makes\nthe reader think you're testing all arguments of ls-tree, when you're\nonly testing a few new ones.  Alternatively, you could squash this\nwith patch 1.\n\n> Signed-off-by: Joshua Nelson <jyn514@gmail.com>\n> ---\n>  t/t3104-ls-tree-optional-args.sh | 43 ++++++++++++++++++++++++++++++++\n>  1 file changed, 43 insertions(+)\n>  create mode 100644 t/t3104-ls-tree-optional-args.sh\n\nPlease note that test files should be executable, not regular files.\n\n>\n> diff --git t/t3104-ls-tree-optional-args.sh t/t3104-ls-tree-optional-args.sh\n> new file mode 100644\n> index 000000000..5917563a7\n> --- /dev/null\n> +++ t/t3104-ls-tree-optional-args.sh\n> @@ -0,0 +1,43 @@\n> +#!/bin/sh\n> +\n> +test_description='ls-tree test (optional args)\n> +\n> +This test tries to run git-ls-tree with various combinations of options.'\n\nThis description seems like it's true for all the t310*.sh tests.  How\ndoes t3104 differ from t310[0-3]*.sh?\n\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'initial setup' '\n> +echo hi > test && cp test test2 && git add test test2 && git commit -m initial'\n> +\n> +# cat appends newlines after every file\n\nWhat is this comment in reference to?\n\n> +test_expect_success 'succeed when given no args' 'git ls-tree'\n> +\n> +test_expect_success 'succeed when given only --' 'git ls-tree'\n\nI was a little surprised that these tests only check that the command\nruns to completion with a 0 exit status, and do not do any\nverification of the output.  I wasn't necessarily expected full output\nverification, but having a few different commits and searching for\nsomething that'd only be shown in the relevant commit would be nice.\n\n> +\n> +test_expect_success 'add second commit' '\n> +echo hi > test3 && git add test3 && git commit -m \"commit 2\"'\n> +\n> +test_expect_success 'succeed when given revision' '\n> +git ls-tree HEAD~1'\n> +\n> +test_expect_success 'succeed when given revision and --' '\n> +git ls-tree HEAD~1 --'\n> +\n> +test_expect_success 'succeed when given -- and file' '\n> +git ls-tree -- test3'\n> +\n> +test_expect_success 'do nothing when given bad files' '\n> +git ls-tree -- bad_files'\n\n...and to reiterate the point above about verifying the output is\ncorrect, how is 'doing nothing' here distinguishable from showing all\nthe files in the current commit if you're not checking any part of the\noutput?\n\n> +\n> +test_expect_success 'succeed when given file from past revision' '\n> +git ls-tree HEAD~1 test'\n> +\n> +test_expect_success 'succeed when given only file' 'git ls-tree test'\n> +\n> +test_expect_success 'raise error when given bad args' '\n> +test_must_fail  git ls-tree HEAD HEAD --'\n> +\n> +test_expect_success 'raise error when given bad revision' '\n> +test_must_fail git ls-tree bad_revision --'\n> +\n> +test_done\n> --\n> 2.18.GIT\n\nLots of little things to fix up, but you're off to a great start.  I'm\nexcited to be able to have ls-tree work without me having to specify\nHEAD all the time.  :-)\n"},{"id":"351582","messageId":"CAPig+cSZbGSsUvXmM1+q7ZhHmRg75tuM+=KyuzG5NFuzOdbYVA@mail.gmail.com","threadId":"48820","inReplyTo":"20180703035802.24060-3-jyn514@gmail.com","subject":"Re: [PATCH 3/3] ls-tree: add unit tests for arguments","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-03T07:33:08Z","receivedAt":"2018-07-03T07:33:24Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jul 2, 2018 at 11:58 PM Joshua Nelson <jyn514@gmail.com> wrote:\n> Signed-off-by: Joshua Nelson <jyn514@gmail.com>\n> ---\n> diff --git t/t3104-ls-tree-optional-args.sh t/t3104-ls-tree-optional-args.sh\n> @@ -0,0 +1,43 @@\n> +test_expect_success 'initial setup' '\n> +echo hi > test && cp test test2 && git add test test2 && git commit -m initial'\n\nAs this is a new test script, please use modern formatting style:\nindent test body with tabs, closing single-quote goes on its own line,\nbreak lines at &&, and no whitespace between > and the filename. Also,\nit's customary to simply to call the test \"setup\".\n\ntest_expect_success 'setup' '\n    echo hi >test &&\n    cp test test2 &&\n    git add test test2 &&\n    git commit -m initial\n'\n\n> +# cat appends newlines after every file\n\nWhy is this talking about a \"cat\"? Doesn't seem relevant.\n\n> +test_expect_success 'succeed when given no args' 'git ls-tree'\n\nThis test seems too minimal. As the intention of this patch series is\nto make git-ls-tree default to HEAD when no treeish is given, I would\nexpect the test with no arguments to verify that it did indeed list\nthe tree associated with HEAD. As implemented, this test tells us\nnothing other than that it didn't error out or crash.\n\n> +test_expect_success 'succeed when given only --' 'git ls-tree'\n\nUm, what's this supposed to be testing? Presently, it seems to\nduplicate the previous test. I'm guessing it should be running \"git\nls-tree --\" instead.\n\n> +test_expect_success 'add second commit' '\n> +echo hi > test3 && git add test3 && git commit -m \"commit 2\"'\n> +\n> +test_expect_success 'succeed when given revision' '\n> +git ls-tree HEAD~1'\n\nGiven how patch 1/3 makes some fundamental changes to how git-ls-tree\nprocesses its arguments, I would again expect this test to verify that\nit indeed lists the correct tree. As the test is currently\nimplemented, we have no way of knowing what tree (if any) it listed.\n\n> +test_expect_success 'succeed when given revision and --' '\n> +git ls-tree HEAD~1 --'\n> +\n> +test_expect_success 'succeed when given -- and file' '\n> +git ls-tree -- test3'\n\nAs above, given the fundamental changes to argument processing, I'd\nexpect this to verify that the output of this command is indeed what\nis expected.\n\n> +test_expect_success 'do nothing when given bad files' '\n> +git ls-tree -- bad_files'\n\nI wonder about this. Is it just an accident of implementation that\ngit-ls-tree doesn't error out in this case, or is it intended behavior\nthat it should return 0 even when the file is not in the tree? If the\n0 exit code is just an accident of implementation, then we shouldn't\nbe enforcing this by a test (instead, someone perhaps ought to fix\ngit-ls-tree).\n\n> +test_expect_success 'succeed when given file from past revision' '\n> +git ls-tree HEAD~1 test'\n\nSame comment as above about verifying gave expected output.\n\n> +test_expect_success 'succeed when given only file' 'git ls-tree test'\n> +\n> +test_expect_success 'raise error when given bad args' '\n> +test_must_fail  git ls-tree HEAD HEAD --'\n> +\n> +test_expect_success 'raise error when given bad revision' '\n> +test_must_fail git ls-tree bad_revision --'\n"},{"id":"351666","messageId":"xmqqbmbonff3.fsf@gitster-ct.c.googlers.com","threadId":"48820","inReplyTo":"CABPp-BFu+m-7Fyf4kj8hUO33vhAxKuAu5JnyL5Tzc7c6Kq+Hjg@mail.gmail.com","subject":"Re: [PATCH 1/3] ls-tree: make <tree-ish> optional","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-03T22:05:36Z","receivedAt":"2018-07-03T22:05:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> On Mon, Jul 2, 2018 at 8:58 PM, Joshua Nelson <jyn514@gmail.com> wrote:\n>> use syntax similar to `git-checkout` to make <tree-ish> optional for\n>> `ls-tree`. if <tree-ish> is omitted, default to HEAD. infer arguments as\n>> follows:\n>>\n>> 1. if args start with --\n>>         assume <tree-ish> to be HEAD\n>> 2. if exactly one arg precedes --, treat the argument as <tree-ish>\n>> 3. if more than one arg precedes --, exit with an error\n>> 4. if -- is not in args\n>>         a) if args[0] is a valid <tree-ish> object, treat is as such\n>>         b) else, assume <tree-ish> to be HEAD\n>>\n>> in all cases, every argument besides <tree-ish> is treated as a <path>\n>\n> Cool, this is something I've wanted a few times.\n\nHmph, is it, and why?  \n\nI'd prefer *not* to have such a DWIM in a command like ls-tree, aka\nplumbing commands, where predictability is worth 1000 times more\nthan ease of typing.\n"},{"id":"351671","messageId":"CABPp-BGJjJGhOEBiJJ5J5rnMXfLndKgwU-4_JVqumBB81DUHpQ@mail.gmail.com","threadId":"48820","inReplyTo":"xmqqbmbonff3.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/3] ls-tree: make <tree-ish> optional","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-03T22:55:16Z","receivedAt":"2018-07-03T22:55:20Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Jul 3, 2018 at 3:05 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Elijah Newren <newren@gmail.com> writes:\n>\n>> On Mon, Jul 2, 2018 at 8:58 PM, Joshua Nelson <jyn514@gmail.com> wrote:\n>>> use syntax similar to `git-checkout` to make <tree-ish> optional for\n>>> `ls-tree`. if <tree-ish> is omitted, default to HEAD. infer arguments as\n>>> follows:\n>>>\n>>> 1. if args start with --\n>>>         assume <tree-ish> to be HEAD\n>>> 2. if exactly one arg precedes --, treat the argument as <tree-ish>\n>>> 3. if more than one arg precedes --, exit with an error\n>>> 4. if -- is not in args\n>>>         a) if args[0] is a valid <tree-ish> object, treat is as such\n>>>         b) else, assume <tree-ish> to be HEAD\n>>>\n>>> in all cases, every argument besides <tree-ish> is treated as a <path>\n>>\n>> Cool, this is something I've wanted a few times.\n>\n> Hmph, is it, and why?\n\nDefault <tree-ish> of HEAD when nothing is specified is certainly\nsomething I wanted.  To be honest, I wanted it for rev-list too.\nDespite dozens if not hundreds of times of typing 'git ls-tree -r' or\n'git rev-list' expecting to see the results for HEAD (just as git log\ndoes), and getting git's error message reminding me that I need to\nspecify HEAD, I can't seem to get it through my head to remember that\nI need to specify it.\n\n> I'd prefer *not* to have such a DWIM in a command like ls-tree, aka\n> plumbing commands, where predictability is worth 1000 times more\n> than ease of typing.\n\nFair enough.  However, what if no <tree-ish> or <path> are specified,\nthough -- would you be okay with the HEAD being assumed instead of\nerroring out in that case?\n"},{"id":"351672","messageId":"808212de-91c1-eabf-cb69-e9899834f075@gmail.com","threadId":"48820","inReplyTo":"CABPp-BGJjJGhOEBiJJ5J5rnMXfLndKgwU-4_JVqumBB81DUHpQ@mail.gmail.com","subject":"Re: [PATCH 1/3] ls-tree: make <tree-ish> optional","fromName":"Joshua Nelson","fromEmail":"jyn514@gmail.com","sentAt":"2018-07-03T22:58:41Z","receivedAt":"2018-07-03T22:58:47Z","isPatch":true,"sender":{"key":"jyn514@gmail.com","avatar":"https://gravatar.com/avatar/1eb2e767b4f956ee324f49d8726ec382595377f83526b0b0b4aa5cd4dd0bd5b9?d=mp&s=160"},"body":"Agreed, ls-tree when called with no arguments was the main use case I\nwrote this for; the rest was mostly because other commands allow greater\nambiguity and I wanted to make the syntax consistent.\n\nI don't mind doing this for rev-list as well if that's a useful feature.\n\n\nOn 07/03/2018 06:55 PM, Elijah Newren wrote:\n> On Tue, Jul 3, 2018 at 3:05 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Elijah Newren <newren@gmail.com> writes:\n>>\n>>> On Mon, Jul 2, 2018 at 8:58 PM, Joshua Nelson <jyn514@gmail.com> wrote:\n>>>> use syntax similar to `git-checkout` to make <tree-ish> optional for\n>>>> `ls-tree`. if <tree-ish> is omitted, default to HEAD. infer arguments as\n>>>> follows:\n>>>>\n>>>> 1. if args start with --\n>>>>         assume <tree-ish> to be HEAD\n>>>> 2. if exactly one arg precedes --, treat the argument as <tree-ish>\n>>>> 3. if more than one arg precedes --, exit with an error\n>>>> 4. if -- is not in args\n>>>>         a) if args[0] is a valid <tree-ish> object, treat is as such\n>>>>         b) else, assume <tree-ish> to be HEAD\n>>>>\n>>>> in all cases, every argument besides <tree-ish> is treated as a <path>\n>>> Cool, this is something I've wanted a few times.\n>> Hmph, is it, and why?\n> Default <tree-ish> of HEAD when nothing is specified is certainly\n> something I wanted.  To be honest, I wanted it for rev-list too.\n> Despite dozens if not hundreds of times of typing 'git ls-tree -r' or\n> 'git rev-list' expecting to see the results for HEAD (just as git log\n> does), and getting git's error message reminding me that I need to\n> specify HEAD, I can't seem to get it through my head to remember that\n> I need to specify it.\n>\n>> I'd prefer *not* to have such a DWIM in a command like ls-tree, aka\n>> plumbing commands, where predictability is worth 1000 times more\n>> than ease of typing.\n> Fair enough.  However, what if no <tree-ish> or <path> are specified,\n> though -- would you be okay with the HEAD being assumed instead of\n> erroring out in that case?\n"},{"id":"351673","messageId":"26b538bd-df59-d9a6-460d-0b1042b35250@gmail.com","threadId":"48820","inReplyTo":"CAPig+cT9XB-nJFm0rD9RckBzcbk9vh8Hz=3XOA-HA-JoXssG_A@mail.gmail.com","subject":"Re: [PATCH 1/3] ls-tree: make <tree-ish> optional","fromName":"Joshua Nelson","fromEmail":"jyn514@gmail.com","sentAt":"2018-07-03T23:15:39Z","receivedAt":"2018-07-03T23:15:43Z","isPatch":true,"sender":{"key":"jyn514@gmail.com","avatar":"https://gravatar.com/avatar/1eb2e767b4f956ee324f49d8726ec382595377f83526b0b0b4aa5cd4dd0bd5b9?d=mp&s=160"},"body":"\nOn 07/03/2018 03:15 AM, Eric Sunshine wrote:\n> Thanks for contributing to Git. As this seems to be your first\n> submission to the project, don't be alarmed by the extent and nature\n> of the review comments. They are intended to help you polish the\n> submission, and are not meant with ill-intent.\n> \n> On Mon, Jul 2, 2018 at 11:58 PM Joshua Nelson <jyn514@gmail.com> wrote:\n>> use syntax similar to `git-checkout` to make <tree-ish> optional for\n>> `ls-tree`. if <tree-ish> is omitted, default to HEAD. infer arguments as\n>> follows:\n> \n> Nit: Capitalize first word of each sentence.\n> \n> This commit message explains what the patch changes (which is a good\n> thing to do), but it's missing the really important explanation of\n> _why_ this change is desirable. Without such justification, it's\n> difficult to judge if such a change is worthwhile. As this is a\n> plumbing command, people may need more convincing (especially if other\n> plumbing commands don't provide convenient defaults like this).\n> \n>> 1. if args start with --\n>>         assume <tree-ish> to be HEAD\n>> 2. if exactly one arg precedes --, treat the argument as <tree-ish>\n>> 3. if more than one arg precedes --, exit with an error\n>> 4. if -- is not in args\n>>         a) if args[0] is a valid <tree-ish> object, treat is as such\n>>         b) else, assume <tree-ish> to be HEAD\n>>\n>> in all cases, every argument besides <tree-ish> is treated as a <path>\n> \n> This and the other patches are missing your Signed-off-by: (which\n> should be placed right here).\n> \n> The three patches of this series are all directly related to this one\n> change. As such, they should be combined into a single patch. (At the\n> very least, 1/3 and 2/3 should be combined; one could argue that 3/3\n> is lengthy enough to make it separate, but that's a judgment call.)\n> \n> Now, on to the actual code...\n> \n>> diff --git builtin/ls-tree.c builtin/ls-tree.c\n>> @@ -163,10 +163,39 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n>>             ((LS_TREE_ONLY|LS_RECURSIVE) & ls_options))\n>>                 ls_options |= LS_SHOW_TREES;\n>>\n>> +       const char *object;\n>> +       short initialized = 0;\n> \n> This project frowns on declaration-after-statement. Move these to the\n> top of the {...} block where other variables are declared.\n> \n> Why use 'short'? Unless there's a very good reason that this needs to\n> be a particular size, you shouldn't deviate from project norm, which\n> is to use the natural word size 'int'.\n> \n> 'initialized' is too generic, thus isn't a great name.\n> \n>>         if (argc < 1)\n>> -               usage_with_options(ls_tree_usage, ls_tree_options);\n>> -       if (get_oid(argv[0], &oid))\n>> -               die(\"Not a valid object name %s\", argv[0]);\n>> +               object = \"HEAD\";\n>> +       else {\n>> +               /* taken from checkout.c;\n>> +                * we have a simpler case because we never create a branch */\n> \n> Style: Multi-line comments are formatted like this:\n> \n>     /*\n>      * Gobble\n>      * wobble.\n>      */\n> \n> However, this comment doesn't belong in the code, as it won't be\n> particularly helpful to anyone reading the code in the future. This\n> sort of note would be more appropriate in the commit message or, even\n> better, in the commentary section just below the \"---\" lines following\n> your Signed-off-by:.\n\nI wasn't aware I could put comments in email generated by\ngit-send-email, thanks for the tip :)\n\n> \n>> +               short dash_dash_pos = -1, i = 0;\n> \n> Same question about why you used 'short' rather than 'int' (especially\n> as 'dash_dash_pos' is declared 'int' in checkout.c).\n> \n> Is there a good reason why you initialize 'i' in the declaration\n> rather than in the for-loop? A _good_ reason to do so in the for-loop\n> is that doing so makes the for-loop more idiomatic, reduces cognitive\n> load on readers (since they don't have to chase down the\n> initialization), and safeguards against someone adding new code\n> between the declaration and the for-loop which might inadvertently\n> alter the initial value.\n\nNo good reason, it happened to end up that way after I finished\ndebugging. I've changed it to a more conventional declaration.\n\n> \n>> +               for (; i < argc; i++) {\n>> +                       if (!strcmp(argv[i], \"--\")) {\n>> +                               dash_dash_pos = i;\n>> +                               break;\n>> +                       }\n>> +               }\n>> +               if (dash_dash_pos == 0) {\n>> +                       object = \"HEAD\";\n>> +                       argv++, argc++;\n> \n> 'argc' is never accessed beyond this point, so changing its value is\n> pointless. Same observation for the 'else' arms. (And, even if there\n> was a valid reason to modify 'argc', I think you'd want to be\n> decrementing it, not incrementing it, to show that you've consumed an\n> argument.)\n> \n>> +               } else if (dash_dash_pos == 1) {\n>> +                       object = argv[0];\n>> +                       argv += 2, argc += 2;\n>> +               } else if (dash_dash_pos >= 2)\n>> +                       die(_(\"only one reference expected, %d given.\"), dash_dash_pos);\n>> +               else if (get_oid(argv[0], &oid)) // not a valid object\n> \n> Style: Use /* comments */ in this codebase, not // comments.\n> \n>> +                       object = \"HEAD\";\n>> +               else {\n>> +                       argv++, argc++;\n>> +                       initialized = 1;\n>> +               }\n>> +       }\n>> +\n>> +       if (!initialized) // if we've already run get_oid, don't run it again\n>> +               if (get_oid(object, &oid))\n>> +                       die(\"Not a valid object name %s\", object);\n> \n> Can't you accomplish the same without the 'initialized' variable by\n> instead initializing 'object' to NULL and then checking it here?\n\nI think my code wasn't very clear here - 'initialized' checks if 'oid'\nhas been initialized, not 'object'. AFAIK, structs can't be initialized\nto NULL, but my C is not very good so I'd be interested to learn otherwise.\n\nI'm writing a new patch with variable names that are more descriptive.\n\n> \n>     if (object && get_oid(object, &oid))\n>         die(_(\"not a valid object name: %s\", object);\n> \n"},{"id":"351674","messageId":"20180703235337.31770-1-jyn514@gmail.com","threadId":"48820","inReplyTo":"26b538bd-df59-d9a6-460d-0b1042b35250@gmail.com","subject":"[PATCH] ls-tree: make <tree-ish> optional","fromName":"Joshua Nelson","fromEmail":"jyn514@gmail.com","sentAt":"2018-07-03T23:53:38Z","receivedAt":"2018-07-03T23:56:58Z","isPatch":true,"sender":{"key":"jyn514@gmail.com","avatar":"https://gravatar.com/avatar/1eb2e767b4f956ee324f49d8726ec382595377f83526b0b0b4aa5cd4dd0bd5b9?d=mp&s=160"},"body":"Use syntax similar to `git-checkout` to make <tree-ish> optional for\n`ls-tree`. If <tree-ish> is omitted, default to HEAD. Infer arguments as\nfollows:\n\n1. If args start with '--', assume <tree-ish> to be HEAD\n2. If exactly one arg precedes '--', treat the argument as <tree-ish>\n3. If more than one arg precedes '--', exit with an error\n4. If '--' is not in args:\n\ta) If args[0] is a valid <tree-ish> object, treat it as such\n\tb) Else, assume <tree-ish> to be HEAD\n\nIn all cases, every argument besides <tree-ish> is treated as a <path>.\n\nSigned-off-by: Joshua Nelson <jyn514@gmail.com>\n---\n\n Documentation/git-ls-tree.txt    |  2 +-\n builtin/ls-tree.c                | 40 ++++++++++++++++----\n t/t3104-ls-tree-optional-args.sh | 63 ++++++++++++++++++++++++++++++++\n 3 files changed, 97 insertions(+), 8 deletions(-)\n create mode 100755 t/t3104-ls-tree-optional-args.sh\n\ndiff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt\nindex 9dee7bef3..290fd11c3 100644\n--- a/Documentation/git-ls-tree.txt\n+++ b/Documentation/git-ls-tree.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n [verse]\n 'git ls-tree' [-d] [-r] [-t] [-l] [-z]\n \t    [--name-only] [--name-status] [--full-name] [--full-tree] [--abbrev[=<n>]]\n-\t    <tree-ish> [<path>...]\n+\t    [<tree-ish>] [--] [<path>...]\n \n DESCRIPTION\n -----------\ndiff --git a/builtin/ls-tree.c b/builtin/ls-tree.c\nindex 409da4e83..64bfbae71 100644\n--- a/builtin/ls-tree.c\n+++ b/builtin/ls-tree.c\n@@ -26,7 +26,7 @@ static int chomp_prefix;\n static const char *ls_tree_prefix;\n \n static const  char * const ls_tree_usage[] = {\n-\tN_(\"git ls-tree [<options>] <tree-ish> [<path>...]\"),\n+\tN_(\"git ls-tree [<options>] [<tree-ish>] [--] [<path>...]\"),\n \tNULL\n };\n \n@@ -122,7 +122,9 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n {\n \tstruct object_id oid;\n \tstruct tree *tree;\n-\tint i, full_tree = 0;\n+\tconst char *tree_ish;\n+\tint i, full_tree = 0, oid_initialized = 0, dash_dash_pos = -1;\n+\n \tconst struct option ls_tree_options[] = {\n \t\tOPT_BIT('d', NULL, &ls_options, N_(\"only show trees\"),\n \t\t\tLS_TREE_ONLY),\n@@ -153,7 +155,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n \t\tchomp_prefix = strlen(prefix);\n \n \targc = parse_options(argc, argv, prefix, ls_tree_options,\n-\t\t\t     ls_tree_usage, 0);\n+\t\t\t     ls_tree_usage, PARSE_OPT_KEEP_DASHDASH);\n \tif (full_tree) {\n \t\tls_tree_prefix = prefix = NULL;\n \t\tchomp_prefix = 0;\n@@ -164,9 +166,33 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n \t\tls_options |= LS_SHOW_TREES;\n \n \tif (argc < 1)\n-\t\tusage_with_options(ls_tree_usage, ls_tree_options);\n-\tif (get_oid(argv[0], &oid))\n-\t\tdie(\"Not a valid object name %s\", argv[0]);\n+\t\ttree_ish = \"HEAD\";\n+\telse {\n+\t\tfor (i = 0; i < argc; i++) {\n+\t\t\tif (!strcmp(argv[i], \"--\")) {\n+\t\t\t\tdash_dash_pos = i;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t\tif (dash_dash_pos == 0) {\n+\t\t\ttree_ish = \"HEAD\";\n+\t\t\targv++;\n+\t\t} else if (dash_dash_pos == 1) {\n+\t\t\ttree_ish = argv[0];\n+\t\t\targv += 2;\n+\t\t} else if (dash_dash_pos >= 2)\n+\t\t\tdie(_(\"only one reference expected, %d given.\"), dash_dash_pos);\n+\t\telse if (get_oid(argv[0], &oid)) // not a valid object\n+\t\t\ttree_ish = \"HEAD\";\n+\t\telse {\n+\t\t\targv++;\n+\t\t\toid_initialized = 1;\n+\t\t}\n+\t}\n+\n+\tif (!oid_initialized) /* if we've already run get_oid, don't run it again */\n+\t\tif (get_oid(tree_ish, &oid))\n+\t\t\tdie(\"Not a valid object name %s\", tree_ish);\n \n \t/*\n \t * show_recursive() rolls its own matching code and is\n@@ -177,7 +203,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n \tparse_pathspec(&pathspec, PATHSPEC_ALL_MAGIC &\n \t\t\t\t  ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),\n \t\t       PATHSPEC_PREFER_CWD,\n-\t\t       prefix, argv + 1);\n+\t\t       prefix, argv);\n \tfor (i = 0; i < pathspec.nr; i++)\n \t\tpathspec.items[i].nowildcard_len = pathspec.items[i].len;\n \tpathspec.has_wildcard = 0;\ndiff --git a/t/t3104-ls-tree-optional-args.sh b/t/t3104-ls-tree-optional-args.sh\nnew file mode 100755\nindex 000000000..e9d8389bc\n--- /dev/null\n+++ b/t/t3104-ls-tree-optional-args.sh\n@@ -0,0 +1,63 @@\n+#!/bin/sh\n+\n+test_description='ls-tree test (optional args)\n+\n+This test runs git-ls-tree with ambiguous positional options.'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\techo hi > test &&\n+\tcp test test2 &&\n+\tgit add test test2 &&\n+\tgit commit -m initial &&\n+\tprintf \"100644 blob 45b983be36b73c0788dc9cbcb76cbb80fc7bb057\\ttest\\n\" > expected1 &&\n+\tprintf \"100644 blob 45b983be36b73c0788dc9cbcb76cbb80fc7bb057\\ttest2\\n\" > expected2\n+'\n+\n+# cat appends newlines after every file\n+test_expect_success 'show HEAD when given no args' '\n+\tif [ \"$(git ls-tree)\" != \"$(cat expected1 expected2)\" ]; then false; fi\n+'\n+\n+test_expect_success 'show HEAD when given only --' '\n+\tif [ \"$(git ls-tree --)\" != \"$(cat expected1 expected2)\" ]; then false; fi\n+'\n+\n+test_expect_success 'setup' '\n+\techo hi > test3 &&\n+\techo there >> test1 &&\n+\tgit add test3 test1 &&\n+\tgit commit -m \"commit 2\" &&\n+\tprintf \"100644 blob 45b983be36b73c0788dc9cbcb76cbb80fc7bb057\\ttest3\\n\" > expected3\n+'\n+\n+test_expect_success 'show all files when given only revision' '\n+\tif [ \"$(git ls-tree HEAD~1)\" != \"$(cat expected1 expected2)\" ]; then false; fi\n+'\n+\n+test_expect_success 'show all files when given revision and --' '\n+\tif [ \"$(git ls-tree HEAD~1 --)\" != \"$(cat expected1 expected2)\" ]; then false; fi\n+'\n+\n+test_expect_success 'show file when given -- and file' '\n+\tif [ \"$(git ls-tree -- test3)\" != \"$(cat expected3)\" ]; then false; fi\n+'\n+\n+test_expect_success 'show file when given revision and file' '\n+\tif [ \"$(git ls-tree HEAD~1 test)\" != \"$(cat expected1)\" ]; then false; fi\n+'\n+\n+test_expect_success 'show file when given only file' '\n+\tif [ \"$(git ls-tree test3)\" != \"$(cat expected3)\" ]; then false; fi\n+'\n+\n+test_expect_success 'raise error when given bad args' '\n+\ttest_must_fail  git ls-tree HEAD HEAD --\n+'\n+\n+test_expect_success 'raise error when given bad revision' '\n+\ttest_must_fail git ls-tree bad_revision --\n+'\n+\n+test_done\n-- \n2.18.GIT\n\n"},{"id":"351675","messageId":"368608dc-4351-541c-0236-9f5760ca5d97@gmail.com","threadId":"48820","inReplyTo":"20180703235337.31770-1-jyn514@gmail.com","subject":"Re: [PATCH] ls-tree: make <tree-ish> optional","fromName":"Joshua Nelson","fromEmail":"jyn514@gmail.com","sentAt":"2018-07-04T00:05:20Z","receivedAt":"2018-07-04T00:05:24Z","isPatch":true,"sender":{"key":"jyn514@gmail.com","avatar":"https://gravatar.com/avatar/1eb2e767b4f956ee324f49d8726ec382595377f83526b0b0b4aa5cd4dd0bd5b9?d=mp&s=160"},"body":"\nOn 07/03/2018 07:53 PM, Joshua Nelson wrote:\n> diff --git a/t/t3104-ls-tree-optional-args.sh b/t/t3104-ls-tree-optional-args.sh\n> new file mode 100755\n> index 000000000..e9d8389bc\n> --- /dev/null\n> +++ b/t/t3104-ls-tree-optional-args.sh\n> @@ -0,0 +1,63 @@\n> +#!/bin/sh\n> +\n> +test_description='ls-tree test (optional args)\n> +\n> +This test runs git-ls-tree with ambiguous positional options.'\n> +\n> +# cat appends newlines after every file\n> +test_expect_success 'show HEAD when given no args' '\n> +\tif [ \"$(git ls-tree)\" != \"$(cat expected1 expected2)\" ]; then false; fi\n> +'\n\nI forgot to take out the comment about cat; it's not actually true, I\njust happened to be using echo instead of printf when I wrote it.\n\nIs it customary to send a new patch or second patch that builds on the\nfirst? If I specify a commit range to git-send-email it sends both, not\njust the latest.\n"},{"id":"351684","messageId":"CAPig+cQu-e63NUUXAB_QA+M-rgPfqBLOOm5fdjsSVuWHJt7TJA@mail.gmail.com","threadId":"48820","inReplyTo":"26b538bd-df59-d9a6-460d-0b1042b35250@gmail.com","subject":"Re: [PATCH 1/3] ls-tree: make <tree-ish> optional","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-04T09:29:45Z","receivedAt":"2018-07-04T09:30:00Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 3, 2018 at 7:15 PM Joshua Nelson <jyn514@gmail.com> wrote:\n> On 07/03/2018 03:15 AM, Eric Sunshine wrote:\n> >> +               /* taken from checkout.c;\n> >> +                * we have a simpler case because we never create a branch */\n> >\n> > However, this comment doesn't belong in the code, as it won't be\n> > particularly helpful to anyone reading the code in the future. This\n> > sort of note would be more appropriate in the commit message or, even\n> > better, in the commentary section just below the \"---\" lines following\n> > your Signed-off-by:.\n>\n> I wasn't aware I could put comments in email generated by\n> git-send-email, thanks for the tip :)\n\nAn even more common workflow is to use git-format-patch to generate\nthe patches locally, then edit the patches to add commentary below the\n\"---\" line if needed, proof-read everything, and finally use\ngit-send-email to send out the already-generated patches.\n\n> >> +                       object = \"HEAD\";\n> >> +               else {\n> >> +                       argv++, argc++;\n> >> +                       initialized = 1;\n> >> +               }\n> >> +       }\n> >> +\n> >> +       if (!initialized) // if we've already run get_oid, don't run it again\n> >> +               if (get_oid(object, &oid))\n> >> +                       die(\"Not a valid object name %s\", object);\n> >\n> > Can't you accomplish the same without the 'initialized' variable by\n> > instead initializing 'object' to NULL and then checking it here?\n>\n> I think my code wasn't very clear here - 'initialized' checks if 'oid'\n> has been initialized, not 'object'. AFAIK, structs can't be initialized\n> to NULL, but my C is not very good so I'd be interested to learn otherwise.\n\nOh, the intention of 'initialized' was quite clear, but it seems\nunnecessary, which is why I was suggesting an alternative. Unless I'm\nmisunderstanding the code (certainly a possibility), I think\n'initialized' is redundant, thus you can get by without it\nSpecifically, the only time you set 'initialized' is when you don't\nset 'object', which means that you can infer whether 'oid' was\ninitialized based upon whether 'object' was set. So, my suggestion\nwas:\n\n    const char *object = NULL;\n    ...\n    ... conditionals possibly assigning to 'object' ...\n    ...\n    if (object && get_oid(object, &oid))\n        die(_(\"not a valid object name: %s\", object);\n\nIf you arrived at that final 'if' statement and object is still NULL,\nthen you know that 'oid' is already initialized; if 'object' is not\nNULL, then you use it to initialized 'oid'.\n"},{"id":"351685","messageId":"CAPig+cR4=0r5qRmKzV7Tzph49Ln0kJDAGtSMxF0BNxujwjeiWg@mail.gmail.com","threadId":"48820","inReplyTo":"368608dc-4351-541c-0236-9f5760ca5d97@gmail.com","subject":"Re: [PATCH] ls-tree: make <tree-ish> optional","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-04T09:38:32Z","receivedAt":"2018-07-04T09:38:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 3, 2018 at 8:05 PM Joshua Nelson <jyn514@gmail.com> wrote:\n> Is it customary to send a new patch or second patch that builds on the\n> first?\n\nIt depends what you mean. If there were problems with a version of a\npatch you sent, then you \"re-roll\", which means you re-send the patch\nin its entirety as if it's brand new (not building on the problematic\nprevious version). You'd also indicate the attempt number in the\nsubject; for instance \"[PATCH v2]\" for the second attempt.\n\nIf you're working on some functionality that is built up over a series\nsteps, then you would send all those patches as a \"patch series\", in\nwhich each patch in the series builds upon the patch or patches\npreceding it in the series.\n\n> If I specify a commit range to git-send-email it sends both, not\n> just the latest.\n\nI'm not quite sure what you mean.\n"},{"id":"351686","messageId":"CAPig+cSkQ9FM=o7icJY-eSoJ0Po_1v8sb0oRX1T6J42N0dOisA@mail.gmail.com","threadId":"48820","inReplyTo":"20180703235337.31770-1-jyn514@gmail.com","subject":"Re: [PATCH] ls-tree: make <tree-ish> optional","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-04T10:04:10Z","receivedAt":"2018-07-04T10:04:25Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 3, 2018 at 7:57 PM Joshua Nelson <jyn514@gmail.com> wrote:\n> Use syntax similar to `git-checkout` to make <tree-ish> optional for\n> `ls-tree`. If <tree-ish> is omitted, default to HEAD. Infer arguments as\n> follows:\n>\n> 1. If args start with '--', assume <tree-ish> to be HEAD\n> 2. If exactly one arg precedes '--', treat the argument as <tree-ish>\n> 3. If more than one arg precedes '--', exit with an error\n> 4. If '--' is not in args:\n>         a) If args[0] is a valid <tree-ish> object, treat it as such\n>         b) Else, assume <tree-ish> to be HEAD\n>\n> In all cases, every argument besides <tree-ish> is treated as a <path>.\n\nSee review comments below. However, it's not clear if you should be\nputting more effort into this submissions since Junio did not seem too\ninterested in such a behavior change[1]. Perhaps wait for word from\nhim before acting on any of the below comments.\n\n[1]: https://public-inbox.org/git/xmqqbmbonff3.fsf@gitster-ct.c.googlers.com/\n\n> Signed-off-by: Joshua Nelson <jyn514@gmail.com>\n> ---\n> diff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt\n> @@ -11,7 +11,7 @@ SYNOPSIS\n>  'git ls-tree' [-d] [-r] [-t] [-l] [-z]\n>             [--name-only] [--name-status] [--full-name] [--full-tree] [--abbrev[=<n>]]\n> -           <tree-ish> [<path>...]\n> +           [<tree-ish>] [--] [<path>...]\n\nThe documentation should also explain what happens when the\nnow-optional <tree-ish> is omitted. A bit later in this document, you\nfind:\n\n    <tree-ish>\n        Id of a tree-ish.\n\nYou probably want to amend this to add: \"When omitted, defaults to HEAD.\"\n\n> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c\n> @@ -122,7 +122,9 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n> +       const char *tree_ish;\n\nI think 'treeish' would be a more common way to spell this in this code base.\n\n> @@ -164,9 +166,33 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n>         if (argc < 1)\n> +               tree_ish = \"HEAD\";\n> +       else {\n> +               for (i = 0; i < argc; i++) {\n> +                       if (!strcmp(argv[i], \"--\")) {\n> +                               dash_dash_pos = i;\n> +                               break;\n> +                       }\n> +               }\n> +               if (dash_dash_pos == 0) {\n> +                       tree_ish = \"HEAD\";\n> +                       argv++;\n> +               } else if (dash_dash_pos == 1) {\n> +                       tree_ish = argv[0];\n> +                       argv += 2;\n> +               } else if (dash_dash_pos >= 2)\n> +                       die(_(\"only one reference expected, %d given.\"), dash_dash_pos);\n> +               else if (get_oid(argv[0], &oid)) // not a valid object\n\nMentioned previously: Use /* comments */ in this code-base, not // comments.\n\n> +                       tree_ish = \"HEAD\";\n> +               else {\n> +                       argv++;\n> +                       oid_initialized = 1;\n> +               }\n> +       }\n> +\n> +       if (!oid_initialized) /* if we've already run get_oid, don't run it again */\n> +               if (get_oid(tree_ish, &oid))\n> +                       die(\"Not a valid object name %s\", tree_ish);\n\nAs explained in [2], I think 'oid_initialized' is unneeded since it\ncan be inferred from 'tree_ish', thus this code could be simplified.\n\n[2]: https://public-inbox.org/git/CAPig+cQu-e63NUUXAB_QA+M-rgPfqBLOOm5fdjsSVuWHJt7TJA@mail.gmail.com/\n\n> diff --git a/t/t3104-ls-tree-optional-args.sh b/t/t3104-ls-tree-optional-args.sh\n> @@ -0,0 +1,63 @@\n> +test_expect_success 'setup' '\n> +       echo hi > test &&\n\nMentioned previously: Drop whitespace after \">\". Same comment applies\nto several other places in this script.\n\n> +       cp test test2 &&\n> +       git add test test2 &&\n> +       git commit -m initial &&\n> +       printf \"100644 blob 45b983be36b73c0788dc9cbcb76cbb80fc7bb057\\ttest\\n\" > expected1 &&\n> +       printf \"100644 blob 45b983be36b73c0788dc9cbcb76cbb80fc7bb057\\ttest2\\n\" > expected2\n> +'\n\nThere's work being done on Git to make it possible to swap in a new\nhash algorithm in place of SHA-1, which means that these hard-coded\nhash values won't fly. Instead, you'd want to determine them\ndynamically. For instance:\n\n    ...\n    printf \"100644 blob %s\\ttest\\n\" $(git rev-parse HEAD:test) >expected1 &&\n    printf \"100644 blob %s\\ttest2\\n\" $(git rev-parse HEAD:test2) >expected2\n\nor something.\n\n> +test_expect_success 'show HEAD when given no args' '\n> +       if [ \"$(git ls-tree)\" != \"$(cat expected1 expected2)\" ]; then false; fi\n> +'\n\nIn this code-base, we use 'test' rather than '[', and we wouldn't\nbother with stuffing the comparison in an 'if' statement since its\nvalue can be used directly as the test result. However, better is to\ndump the result of git-ls-tree to a file and then use test_cmp() to\ncompare the expected and actual output. If the expected and actual\noutput don't match, test_cmp() will show the differences to aid\ndebugging. So, you might write this test like this:\n\n    test_expect_success 'show HEAD when given no args' '\n        cat expected1 expected2 >expected &&\n        git ls-tree >actual &&\n        test_cmp expected actual\n    '\n\nSame comment regarding remaining tests.\n"},{"id":"351783","messageId":"xmqqh8lcmh7u.fsf@gitster-ct.c.googlers.com","threadId":"48820","inReplyTo":"CABPp-BGJjJGhOEBiJJ5J5rnMXfLndKgwU-4_JVqumBB81DUHpQ@mail.gmail.com","subject":"Re: [PATCH 1/3] ls-tree: make <tree-ish> optional","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-06T17:01:09Z","receivedAt":"2018-07-06T17:01:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n>> I'd prefer *not* to have such a DWIM in a command like ls-tree, aka\n>> plumbing commands, where predictability is worth 1000 times more\n>> than ease of typing.\n>\n> Fair enough.  However, what if no <tree-ish> or <path> are specified,\n> though -- would you be okay with the HEAD being assumed instead of\n> erroring out in that case?\n\nIf we wrote ls-tree to do so 12 years ago, then I wouldn't have\nopposed.  Changing the behaviour now?  Not so sure if it is worth\nhaving to worry about updating the code, docs and making sure we\nspot all the possible typoes.\n"},{"id":"351817","messageId":"6d7eca9f-bf3f-5724-42ed-bd579a4d604f@gmail.com","threadId":"48820","inReplyTo":"xmqqh8lcmh7u.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/3] ls-tree: make <tree-ish> optional","fromName":"Joshua Nelson","fromEmail":"jyn514@gmail.com","sentAt":"2018-07-06T21:26:33Z","receivedAt":"2018-07-06T21:26:40Z","isPatch":true,"sender":{"key":"jyn514@gmail.com","avatar":"https://gravatar.com/avatar/1eb2e767b4f956ee324f49d8726ec382595377f83526b0b0b4aa5cd4dd0bd5b9?d=mp&s=160"},"body":"\nOn 07/06/2018 01:01 PM, Junio C Hamano wrote:\n> Elijah Newren <newren@gmail.com> writes:\n> \n>>> I'd prefer *not* to have such a DWIM in a command like ls-tree, aka\n>>> plumbing commands, where predictability is worth 1000 times more\n>>> than ease of typing.\n>>\n>> Fair enough.  However, what if no <tree-ish> or <path> are specified,\n>> though -- would you be okay with the HEAD being assumed instead of\n>> erroring out in that case?\n> \n> If we wrote ls-tree to do so 12 years ago, then I wouldn't have\n> opposed.  Changing the behaviour now?  Not so sure if it is worth\n> having to worry about updating the code, docs and making sure we\n> spot all the possible typoes.\n> \n\nI have to say, as a first time contributor, reading this is extremely\ndiscouraging. I'm not being told the patch can be improved, or that I've\nmade some error that should be corrected. I'm being told that the entire\nidea of the patch is unwanted, that it doesn't have a place in a mature\nproject like git, that only bug fixes and security issues should be\naccepted.\n\nThat makes me not want to contribute.\n"},{"id":"351820","messageId":"xmqqo9fkhwy9.fsf@gitster-ct.c.googlers.com","threadId":"48820","inReplyTo":"6d7eca9f-bf3f-5724-42ed-bd579a4d604f@gmail.com","subject":"Re: [PATCH 1/3] ls-tree: make <tree-ish> optional","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-06T21:32:30Z","receivedAt":"2018-07-06T21:32:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joshua Nelson <jyn514@gmail.com> writes:\n\n> On 07/06/2018 01:01 PM, Junio C Hamano wrote:\n>> Elijah Newren <newren@gmail.com> writes:\n>> \n>>>> I'd prefer *not* to have such a DWIM in a command like ls-tree, aka\n>>>> plumbing commands, where predictability is worth 1000 times more\n>>>> than ease of typing.\n>>>\n>>> Fair enough.  However, what if no <tree-ish> or <path> are specified,\n>>> though -- would you be okay with the HEAD being assumed instead of\n>>> erroring out in that case?\n>> \n>> If we wrote ls-tree to do so 12 years ago, then I wouldn't have\n>> opposed.  Changing the behaviour now?  Not so sure if it is worth\n>> having to worry about updating the code, docs and making sure we\n>> spot all the possible typoes.\n>> \n>\n> I have to say, as a first time contributor, reading this is extremely\n> discouraging. I'm not being told the patch can be improved, or that I've\n> made some error that should be corrected. I'm being told that the entire\n> idea of the patch is unwanted, that it doesn't have a place in a mature\n> project like git, that only bug fixes and security issues should be\n> accepted.\n\n... on plumbing commands like ls-tree, where keeping the interface\nstable is much more important than making it easier to \"type\".\n\nRules for porcelain commands are entirely different.\n"}]}