# [PATCH 0/3] support `--oid-only` in `ls-tree` 55 messages from 2021-11-15 to 2021-12-17. Participants: Teng Long, Ævar Arnfjörð Bjarmason, Đoàn Trần Công Danh, Jeff King, Junio C Hamano, Peter Baumann. Thread: https://gitlist.dev/t/56905 ## Teng Long, 2021-11-15 11:51 Subject: [PATCH 0/3] support `--oid-only` in `ls-tree` Message-ID: <20211115115153.48307-1-dyroneteng@gmail.com> URL: https://gitlist.dev/e/20211115115153.48307-1-dyroneteng%40gmail.com ``` Sometimes, we only want to get the objects from output of `ls-tree` and commands like `sed` or `cut` is usually used to intercept the origin output to achieve this purpose in practical. The patch contains three commits 1. Implementation of the option. 2. Add new tests in "t3104". 3. Documentation modifications. I'm appreciate if someone help to review the patch. Thanks. Teng Long (3): ls-tree.c: support `--oid-only` option for "git-ls-tree" t3104: add related tests for `--oid-only` option git-ls-tree.txt: description of the 'oid-only' option Documentation/git-ls-tree.txt | 8 +++-- builtin/ls-tree.c | 11 +++++++ t/t3104-ls-tree-oid.sh | 55 +++++++++++++++++++++++++++++++++++ 3 files changed, 72 insertions(+), 2 deletions(-) create mode 100755 t/t3104-ls-tree-oid.sh -- 2.33.1.9.g5fbd2fc599.dirty ``` ## Teng Long, 2021-11-15 11:51 Subject: [PATCH 1/3] ls-tree.c: support `--oid-only` option for "git-ls-tree" Message-ID: <20211115115153.48307-2-dyroneteng@gmail.com> URL: https://gitlist.dev/e/20211115115153.48307-2-dyroneteng%40gmail.com In-Reply-To: <20211115115153.48307-1-dyroneteng@gmail.com> ``` This commit supply an option names `--oid-only` to let `git ls-tree` only print out the OID of the object. `--oid-only` and `--name-only` are mutually exclusive in use. Signed-off-by: Teng Long --- builtin/ls-tree.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index 3a442631c7..1f82229649 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c @@ -20,6 +20,7 @@ static int line_termination = '\n'; #define LS_SHOW_TREES 4 #define LS_NAME_ONLY 8 #define LS_SHOW_SIZE 16 +#define LS_OID_ONLY 32 static int abbrev; static int ls_options; static struct pathspec pathspec; @@ -90,6 +91,14 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, else if (ls_options & LS_TREE_ONLY) return 0; + if ((ls_options & LS_NAME_ONLY) && (ls_options & LS_OID_ONLY)) + die(_("cannot specify --oid-only and --name-only at the same time")); + + if (ls_options & LS_OID_ONLY) { + printf("%s\n", find_unique_abbrev(oid, abbrev)); + return 0; + } + if (!(ls_options & LS_NAME_ONLY)) { if (ls_options & LS_SHOW_SIZE) { char size_text[24]; @@ -139,6 +148,8 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) LS_NAME_ONLY), OPT_BIT(0, "name-status", &ls_options, N_("list only filenames"), LS_NAME_ONLY), + OPT_BIT(0, "oid-only", &ls_options, N_("list only oids"), + LS_OID_ONLY), OPT_SET_INT(0, "full-name", &chomp_prefix, N_("use full path names"), 0), OPT_BOOL(0, "full-tree", &full_tree, -- 2.33.1.9.g5fbd2fc599.dirty ``` ## Teng Long, 2021-11-15 11:51 Subject: [PATCH 2/3] t3104: add related tests for `--oid-only` option Message-ID: <20211115115153.48307-3-dyroneteng@gmail.com> URL: https://gitlist.dev/e/20211115115153.48307-3-dyroneteng%40gmail.com In-Reply-To: <20211115115153.48307-1-dyroneteng@gmail.com> ``` Signed-off-by: Teng Long --- t/t3104-ls-tree-oid.sh | 55 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100755 t/t3104-ls-tree-oid.sh diff --git a/t/t3104-ls-tree-oid.sh b/t/t3104-ls-tree-oid.sh new file mode 100755 index 0000000000..78ab9127c7 --- /dev/null +++ b/t/t3104-ls-tree-oid.sh @@ -0,0 +1,55 @@ +#!/bin/sh + +test_description='git ls-tree oids handling.' + +. ./test-lib.sh + +test_expect_success 'setup' ' + echo 111 >1.txt && + echo 222 >2.txt && + mkdir -p path0/a/b/c && + echo 333 >path0/a/b/c/3.txt && + find *.txt path* \( -type f -o -type l \) -print | + xargs git update-index --add && + tree=$(git write-tree) && + echo $tree +' + + +test_expect_success 'specify with --oid-only' ' + git ls-tree --oid-only $tree >current && + cat >expected <<\EOF && +58c9bdf9d017fcd178dc8c073cbfcbb7ff240d6c +c200906efd24ec5e783bee7f23b5d7c941b0c12c +4e3849a078083863912298a25db30997cb8ca6d6 +EOF + test_cmp current expected +' + +test_expect_success 'specify with --oid-only and -r' ' + git ls-tree --oid-only -r $tree >current && + cat >expected <<\EOF && +58c9bdf9d017fcd178dc8c073cbfcbb7ff240d6c +c200906efd24ec5e783bee7f23b5d7c941b0c12c +55bd0ac4c42e46cd751eb7405e12a35e61425550 +EOF + test_cmp current expected +' + +test_expect_success 'specify with --oid-only and --abbrev' ' + git ls-tree --oid-only --abbrev=6 $tree >current && + cat >expected <<\EOF && +58c9bd +c20090 +4e3849 +EOF + test_cmp current expected +' + +test_expect_success 'cannot specify --name-only and --oid-only as the same time' ' + test_must_fail git ls-tree --oid-only --name-only $tree >current 2>&1 >/dev/null && + echo "fatal: cannot specify --oid-only and --name-only at the same time" > expected && + test_cmp current expected +' + +test_done -- 2.33.1.9.g5fbd2fc599.dirty ``` ## Teng Long, 2021-11-15 11:51 Subject: [PATCH 3/3] git-ls-tree.txt: description of the 'oid-only' option Message-ID: <20211115115153.48307-4-dyroneteng@gmail.com> URL: https://gitlist.dev/e/20211115115153.48307-4-dyroneteng%40gmail.com In-Reply-To: <20211115115153.48307-1-dyroneteng@gmail.com> ``` Signed-off-by: Teng Long --- Documentation/git-ls-tree.txt | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt index db02d6d79a..bc711dc00a 100644 --- a/Documentation/git-ls-tree.txt +++ b/Documentation/git-ls-tree.txt @@ -10,7 +10,8 @@ SYNOPSIS -------- [verse] 'git ls-tree' [-d] [-r] [-t] [-l] [-z] - [--name-only] [--name-status] [--full-name] [--full-tree] [--abbrev[=]] + [--name-only] [--name-status] [--oid-only] + [--full-name] [--full-tree] [--abbrev[=]] [...] DESCRIPTION @@ -59,7 +60,10 @@ OPTIONS --name-only:: --name-status:: List only filenames (instead of the "long" output), one per line. - + Cannot be used with `--oid-only` together. +--oid-only:: + List only OIDs of the objects, one per line. Cannot be used with + `--name-only` or `--name-status` together. --abbrev[=]:: Instead of showing the full 40-byte hexadecimal object lines, show the shortest prefix that is at least '' -- 2.33.1.9.g5fbd2fc599.dirty ``` ## Ævar Arnfjörð Bjarmason, 2021-11-15 15:12 Subject: Re: [PATCH 1/3] ls-tree.c: support `--oid-only` option for "git-ls-tree" Message-ID: <211115.86r1bhsb8x.gmgdl@evledraar.gmail.com> URL: https://gitlist.dev/e/211115.86r1bhsb8x.gmgdl%40evledraar.gmail.com In-Reply-To: <20211115115153.48307-2-dyroneteng@gmail.com> ``` lorn Mon, Nov 15 2021, Teng Long wrote: > This commit supply an option names `--oid-only` to let `git ls-tree` > only print out the OID of the object. `--oid-only` and `--name-only` > are mutually exclusive in use. > > Signed-off-by: Teng Long > --- > builtin/ls-tree.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c > index 3a442631c7..1f82229649 100644 > --- a/builtin/ls-tree.c > +++ b/builtin/ls-tree.c > @@ -20,6 +20,7 @@ static int line_termination = '\n'; > #define LS_SHOW_TREES 4 > #define LS_NAME_ONLY 8 > #define LS_SHOW_SIZE 16 > +#define LS_OID_ONLY 32 > static int abbrev; > static int ls_options; > static struct pathspec pathspec; > @@ -90,6 +91,14 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, > else if (ls_options & LS_TREE_ONLY) > return 0; > > + if ((ls_options & LS_NAME_ONLY) && (ls_options & LS_OID_ONLY)) > + die(_("cannot specify --oid-only and --name-only at the same time")); If you make these an OPT_CMDMODE you get this behavior for free. See e.g. my https://lore.kernel.org/git/patch-v2-06.10-d945fc94774-20211112T221506Z-avarab@gmail.com/ ``` ## Ævar Arnfjörð Bjarmason, 2021-11-15 15:13 Subject: Re: [PATCH 0/3] support `--oid-only` in `ls-tree` Message-ID: <211115.86mtm5saz7.gmgdl@evledraar.gmail.com> URL: https://gitlist.dev/e/211115.86mtm5saz7.gmgdl%40evledraar.gmail.com In-Reply-To: <20211115115153.48307-1-dyroneteng@gmail.com> ``` On Mon, Nov 15 2021, Teng Long wrote: > Sometimes, we only want to get the objects from output of `ls-tree` > and commands like `sed` or `cut` is usually used to intercept the > origin output to achieve this purpose in practical. > > The patch contains three commits > > 1. Implementation of the option. > 2. Add new tests in "t3104". > 3. Documentation modifications. > > I'm appreciate if someone help to review the patch. I've looked it over, they look correct mostly, the test code in 2/3 looks a bit too complex (using find?). But I'd much rather see this be done with adding strbuf_expand() to ls-tree. I.e. its docs say that it can emit: SP SP TAB Or, with -l: SP SP SP TAB If you use strbuf_expand() you can just define a default format of: %(objectmode) SP %(objecttype) SP %(objectname) TAB %(path) Then make the existing -l option a shorthand for tweaking that to: %(objectmode) SP %(objecttype) SP %(objectsize) SP %(objectname) TAB %(path) Then you can get what you want out of this with a simple: git ls-tree --format="%(objectname)" See e.g. git-cat-file for an existing use of strbuf_expand(). ``` ## Đoàn Trần Công Danh, 2021-11-15 15:54 Subject: Re: [PATCH 2/3] t3104: add related tests for `--oid-only` option Message-ID: URL: https://gitlist.dev/e/YZKCmPJzOwLeFhIJ%40danh.dev In-Reply-To: <20211115115153.48307-3-dyroneteng@gmail.com> ``` On 2021-11-15 19:51:52+0800, Teng Long wrote: > Signed-off-by: Teng Long > --- > t/t3104-ls-tree-oid.sh | 55 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 55 insertions(+) > create mode 100755 t/t3104-ls-tree-oid.sh > > diff --git a/t/t3104-ls-tree-oid.sh b/t/t3104-ls-tree-oid.sh > new file mode 100755 > index 0000000000..78ab9127c7 > --- /dev/null > +++ b/t/t3104-ls-tree-oid.sh > @@ -0,0 +1,55 @@ > +#!/bin/sh > + > +test_description='git ls-tree oids handling.' > + > +. ./test-lib.sh > + > +test_expect_success 'setup' ' > + echo 111 >1.txt && > + echo 222 >2.txt && > + mkdir -p path0/a/b/c && > + echo 333 >path0/a/b/c/3.txt && > + find *.txt path* \( -type f -o -type l \) -print | > + xargs git update-index --add && > + tree=$(git write-tree) && > + echo $tree > +' > + > + > +test_expect_success 'specify with --oid-only' ' > + git ls-tree --oid-only $tree >current && > + cat >expected <<\EOF && > +58c9bdf9d017fcd178dc8c073cbfcbb7ff240d6c > +c200906efd24ec5e783bee7f23b5d7c941b0c12c > +4e3849a078083863912298a25db30997cb8ca6d6 > +EOF Failed with: GIT_TEST_DEFAULT_HASH=sha256 ./t3104-ls-tree-oid.sh I think we can use: git ls-tree $tree | awk '{print $3}' >expected > + test_cmp current expected > +' > + > +test_expect_success 'specify with --oid-only and -r' ' > + git ls-tree --oid-only -r $tree >current && > + cat >expected <<\EOF && > +58c9bdf9d017fcd178dc8c073cbfcbb7ff240d6c > +c200906efd24ec5e783bee7f23b5d7c941b0c12c > +55bd0ac4c42e46cd751eb7405e12a35e61425550 > +EOF > + test_cmp current expected > +' Ditto for this test and below tests. > + > +test_expect_success 'specify with --oid-only and --abbrev' ' > + git ls-tree --oid-only --abbrev=6 $tree >current && > + cat >expected <<\EOF && > +58c9bd > +c20090 > +4e3849 > +EOF > + test_cmp current expected > +' > + > +test_expect_success 'cannot specify --name-only and --oid-only as the same time' ' > + test_must_fail git ls-tree --oid-only --name-only $tree >current 2>&1 >/dev/null && The last redirection '>/dev/null' does nothing, me think. > + echo "fatal: cannot specify --oid-only and --name-only at the same time" > expected && Style nit: use '>expected' instead of '> expected' > + test_cmp current expected > +' > + > +test_done > -- > 2.33.1.9.g5fbd2fc599.dirty > -- Danh ``` ## Jeff King, 2021-11-15 19:16 Subject: Re: [PATCH 1/3] ls-tree.c: support `--oid-only` option for "git-ls-tree" Message-ID: URL: https://gitlist.dev/e/YZKyC7c8b9PnyXG8%40coredump.intra.peff.net In-Reply-To: <20211115115153.48307-2-dyroneteng@gmail.com> ``` On Mon, Nov 15, 2021 at 07:51:51PM +0800, Teng Long wrote: > diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c > index 3a442631c7..1f82229649 100644 > --- a/builtin/ls-tree.c > +++ b/builtin/ls-tree.c > @@ -20,6 +20,7 @@ static int line_termination = '\n'; > #define LS_SHOW_TREES 4 > #define LS_NAME_ONLY 8 > #define LS_SHOW_SIZE 16 > +#define LS_OID_ONLY 32 > static int abbrev; > static int ls_options; > static struct pathspec pathspec; > @@ -90,6 +91,14 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, > else if (ls_options & LS_TREE_ONLY) > return 0; > > + if ((ls_options & LS_NAME_ONLY) && (ls_options & LS_OID_ONLY)) > + die(_("cannot specify --oid-only and --name-only at the same time")); This seems reasonable to me. Letting them overwrite each other (i.e., "last one wins") would also be fine, but we can always loosen to that behavior later if we choose. This is a somewhat funny place to put the check, though. It will be run for every entry in the tree (so is a tiny bit less efficient, but also would not trigger for an empty tree). It probably should go in cmd_ls_tree(), perhaps here: diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index 1f82229649..3c9ea00489 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c @@ -91,9 +91,6 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, else if (ls_options & LS_TREE_ONLY) return 0; - if ((ls_options & LS_NAME_ONLY) && (ls_options & LS_OID_ONLY)) - die(_("cannot specify --oid-only and --name-only at the same time")); - if (ls_options & LS_OID_ONLY) { printf("%s\n", find_unique_abbrev(oid, abbrev)); return 0; @@ -175,6 +172,9 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) ((LS_TREE_ONLY|LS_RECURSIVE) & ls_options)) ls_options |= LS_SHOW_TREES; + if ((ls_options & LS_NAME_ONLY) && (ls_options & LS_OID_ONLY)) + die(_("cannot specify --oid-only and --name-only at the same time")); + if (argc < 1) usage_with_options(ls_tree_usage, ls_tree_options); if (get_oid(argv[0], &oid)) Ævar also mentioned using OPT_CMDMODE(), which I think would naturally move the logic in a similar way. -Peff ``` ## Jeff King, 2021-11-15 19:23 Subject: Re: [PATCH 0/3] support `--oid-only` in `ls-tree` Message-ID: URL: https://gitlist.dev/e/YZKzpv%2Bn6bHJ2WOs%40coredump.intra.peff.net In-Reply-To: <20211115115153.48307-1-dyroneteng@gmail.com> ``` On Mon, Nov 15, 2021 at 07:51:50PM +0800, Teng Long wrote: > Sometimes, we only want to get the objects from output of `ls-tree` > and commands like `sed` or `cut` is usually used to intercept the > origin output to achieve this purpose in practical. > > The patch contains three commits > > 1. Implementation of the option. > 2. Add new tests in "t3104". > 3. Documentation modifications. > > I'm appreciate if someone help to review the patch. This seems like a good feature to have. I think it would make sense to squash the three patches into a single one. The documentation and test patches do not stand on their own, which is why there was nothing useful to say in their commit messages. The implementation looks generally sensible (modulo the comments already given). I was surprised that there was not an existing ls-tree script that these would fit into. But there really isn't; t3101 covers --name-only and other output, but is really focused on the pathnames (though I think it would be OK to refactor it to cover output more generally). -Peff ``` ## Ævar Arnfjörð Bjarmason, 2021-11-15 21:50 Subject: Re: [PATCH 0/3] support `--oid-only` in `ls-tree` Message-ID: <211115.86o86lqe3c.gmgdl@evledraar.gmail.com> URL: https://gitlist.dev/e/211115.86o86lqe3c.gmgdl%40evledraar.gmail.com In-Reply-To: ``` On Mon, Nov 15 2021, Jeff King wrote: > On Mon, Nov 15, 2021 at 04:13:24PM +0100, Ævar Arnfjörð Bjarmason wrote: > >> But I'd much rather see this be done with adding strbuf_expand() to >> ls-tree. I.e. its docs say that it can emit: > > I had a similar thought, but that's a much bigger task. I think it would > be reasonable to add --oid-only to match the existing --name-only, etc. > If we later add a custom --format option, then it can easily be folded > in and explained as "this is an alias for --format=%(objectname)", just > like --name-only would become "this is an alias for --format=%(path)". A quick patch to do it below, seems to work, passes all tests, but I don't know how much I'd trust it. It's also quite an add use of strbuf_expa(). We print to stdout directly since write_name_quoted_relative() really wants to write to stdout, and not give you a buffer. But I guess it makes sense in a way. The hardcoded %7s for %(objectsize) is a bit nasty, but I don't know if we've got anything existing that handles format specifiers with strbuf_expand() that we could steal. I really wouldn't trust this code much, I found it when writing it that our tests for ls-tree are really lacking, e.g. we may not have a single test for "-l" anywhere (or maybe I didn't look enough, I was just running t/*ls*tree* while hacking it. I do thin that we should consider just going with --format in either case if we agree that this is a good direction. I.e. could just support 3-4 hardcoded formats now and die if anything else is specified. Then we'd be future-proof with the same interface expanding later, and wouldn't need to support options that we're only carrying because we didn't implement the more generic format support. (Assume my Signed-off-by, if there's any interest...) diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index 3a442631c71..e89daad4229 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c @@ -31,6 +31,20 @@ static const char * const ls_tree_usage[] = { NULL }; +static const char *ls_tree_format_d = "%(objectmode) %(objecttype) %(objectname) %(path)"; +static const char *ls_tree_format_l = "%(objectmode) %(objecttype) %(objectname) %(objectsize) %(path)"; +static const char *ls_tree_format_n = "%(path)"; + +struct expand_ls_tree_data { + const char *format; + unsigned mode; + const char *type; + const struct object_id *oid; + int abbrev; + const char *pathname; + const char *basebuf; +}; + static int show_recursive(const char *base, int baselen, const char *pathname) { int i; @@ -61,9 +75,69 @@ static int show_recursive(const char *base, int baselen, const char *pathname) return 0; } +static size_t expand_show_tree(struct strbuf *sb, + const char *start, + void *context) +{ + struct expand_ls_tree_data *data = context; + const char *end; + const char *p; + size_t len; + const char *type = blob_type; + + if (sb->len) { + fputs(sb->buf, stdout); + strbuf_reset(sb); + } + + if (*start != '(') + die(_("bad format as of '%s'"), start); + end = strchr(start + 1, ')'); + if (!end) + die(_("ls-tree format element '%s' does not end in ')'"), + start); + len = end - start + 1; + + if (skip_prefix(start, "(objectmode)", &p)) { + printf("%06o", data->mode); + } else if (skip_prefix(start, "(objecttype)", &p)) { + fputs(data->type, stdout); + } else if (skip_prefix(start, "(objectsize)", &p)) { + char size_text[24]; + const struct object_id *oid = data->oid; + + if (!strcmp(type, blob_type)) { + unsigned long size; + if (oid_object_info(the_repository, oid, &size) == OBJ_BAD) + xsnprintf(size_text, sizeof(size_text), + "BAD"); + else + xsnprintf(size_text, sizeof(size_text), + "%"PRIuMAX, (uintmax_t)size); + } else { + xsnprintf(size_text, sizeof(size_text), "-"); + } + printf("%7s", size_text); + } else if (skip_prefix(start, "(objectname)", &p)) { + fputs(find_unique_abbrev(data->oid, data->abbrev), stdout); + } else if (skip_prefix(start, "(path)", &p)) { + write_name_quoted_relative(data->basebuf, + chomp_prefix ? ls_tree_prefix : NULL, + stdout, line_termination); + + } else { + unsigned int errlen = (unsigned long)len; + die(_("bad ls-tree format specifiec %%%.*s"), errlen, start); + } + + return len; +} + static int show_tree(const struct object_id *oid, struct strbuf *base, const char *pathname, unsigned mode, void *context) { + struct expand_ls_tree_data *data = context; + struct strbuf sb = STRBUF_INIT; int retval = 0; int baselen; const char *type = blob_type; @@ -90,31 +164,18 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, else if (ls_options & LS_TREE_ONLY) return 0; - if (!(ls_options & LS_NAME_ONLY)) { - if (ls_options & LS_SHOW_SIZE) { - char size_text[24]; - if (!strcmp(type, blob_type)) { - unsigned long size; - if (oid_object_info(the_repository, oid, &size) == OBJ_BAD) - xsnprintf(size_text, sizeof(size_text), - "BAD"); - else - xsnprintf(size_text, sizeof(size_text), - "%"PRIuMAX, (uintmax_t)size); - } else - xsnprintf(size_text, sizeof(size_text), "-"); - printf("%06o %s %s %7s\t", mode, type, - find_unique_abbrev(oid, abbrev), - size_text); - } else - printf("%06o %s %s\t", mode, type, - find_unique_abbrev(oid, abbrev)); - } baselen = base->len; strbuf_addstr(base, pathname); - write_name_quoted_relative(base->buf, - chomp_prefix ? ls_tree_prefix : NULL, - stdout, line_termination); + + strbuf_reset(&sb); + data->mode = mode; + data->type = type; + data->oid = oid; + data->abbrev = abbrev; + data->pathname = pathname; + data->basebuf = base->buf; + strbuf_expand(&sb, data->format, expand_show_tree, data); + strbuf_setlen(base, baselen); return retval; } @@ -147,6 +208,9 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) OPT__ABBREV(&abbrev), OPT_END() }; + struct expand_ls_tree_data ls_tree_cb_data = { + .format = ls_tree_format_d, + }; git_config(git_default_config, NULL); ls_tree_prefix = prefix; @@ -161,8 +225,14 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) } /* -d -r should imply -t, but -d by itself should not have to. */ if ( (LS_TREE_ONLY|LS_RECURSIVE) == - ((LS_TREE_ONLY|LS_RECURSIVE) & ls_options)) + ((LS_TREE_ONLY|LS_RECURSIVE) & ls_options)) { ls_options |= LS_SHOW_TREES; + } + if (ls_options & LS_NAME_ONLY) + ls_tree_cb_data.format = ls_tree_format_n; + + if (ls_options & LS_SHOW_SIZE) + ls_tree_cb_data.format = ls_tree_format_l; if (argc < 1) usage_with_options(ls_tree_usage, ls_tree_options); @@ -185,6 +255,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) tree = parse_tree_indirect(&oid); if (!tree) die("not a tree object"); + return !!read_tree(the_repository, tree, - &pathspec, show_tree, NULL); + &pathspec, show_tree, &ls_tree_cb_data); } ``` ## Jeff King, 2021-11-15 19:25 Subject: Re: [PATCH 1/3] ls-tree.c: support `--oid-only` option for "git-ls-tree" Message-ID: URL: https://gitlist.dev/e/YZK0MKCYAJmG%2BpSU%40coredump.intra.peff.net In-Reply-To: ``` On Mon, Nov 15, 2021 at 02:16:27PM -0500, Jeff King wrote: > On Mon, Nov 15, 2021 at 07:51:51PM +0800, Teng Long wrote: > > > diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c > > index 3a442631c7..1f82229649 100644 > > --- a/builtin/ls-tree.c > > +++ b/builtin/ls-tree.c > > @@ -20,6 +20,7 @@ static int line_termination = '\n'; > > #define LS_SHOW_TREES 4 > > #define LS_NAME_ONLY 8 > > #define LS_SHOW_SIZE 16 > > +#define LS_OID_ONLY 32 > > static int abbrev; > > static int ls_options; > > static struct pathspec pathspec; > > @@ -90,6 +91,14 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, > > else if (ls_options & LS_TREE_ONLY) > > return 0; > > > > + if ((ls_options & LS_NAME_ONLY) && (ls_options & LS_OID_ONLY)) > > + die(_("cannot specify --oid-only and --name-only at the same time")); > > This seems reasonable to me. Letting them overwrite each other (i.e., > "last one wins") would also be fine, but we can always loosen to that > behavior later if we choose. Oh, and whichever direction we go, it would probably make sense for --long to be handled in the same way. I.e.: git ls-tree --long --oid-only does not really make sense. Though we currently just ignore --long for: git ls-tree --long --name-only which is arguably a bug. -Peff ``` ## Jeff King, 2021-11-15 19:09 Subject: Re: [PATCH 0/3] support `--oid-only` in `ls-tree` Message-ID: URL: https://gitlist.dev/e/YZKwduZgH4BEpFzd%40coredump.intra.peff.net In-Reply-To: <211115.86mtm5saz7.gmgdl@evledraar.gmail.com> ``` On Mon, Nov 15, 2021 at 04:13:24PM +0100, Ævar Arnfjörð Bjarmason wrote: > But I'd much rather see this be done with adding strbuf_expand() to > ls-tree. I.e. its docs say that it can emit: I had a similar thought, but that's a much bigger task. I think it would be reasonable to add --oid-only to match the existing --name-only, etc. If we later add a custom --format option, then it can easily be folded in and explained as "this is an alias for --format=%(objectname)", just like --name-only would become "this is an alias for --format=%(path)". -Peff ``` ## Teng Long, 2021-11-18 08:45 Subject: Re: [PATCH 2/3] t3104: add related tests for `--oid-only` option Message-ID: <20211118084557.76846-1-dyroneteng@gmail.com> URL: https://gitlist.dev/e/20211118084557.76846-1-dyroneteng%40gmail.com In-Reply-To: ``` on Mon, 15 Nov 2021 22:54:00 +0700, Đoàn Trần Công Danh wrote: Thanks for helping to review this patch. > Failed with: > > GIT_TEST_DEFAULT_HASH=sha256 ./t3104-ls-tree-oid.sh You totally right and the tests should pass both sha1 and sha256. > I think we can use: > > git ls-tree $tree | awk '{print $3}' >expected > ... > ... > > Ditto for this test and below tests. Yes, correct and better. But should also escape the dollar character to work. > The last redirection '>/dev/null' does nothing, me think. > Style nit: > > use '>expected' instead of '> expected' Yeah, that's my bad and will fix. ``` ## Teng Long, 2021-11-18 09:28 Subject: Re: [PATCH 1/3] ls-tree.c: support `--oid-only` option for "git-ls-tree" Message-ID: <20211118092803.78395-1-dyroneteng@gmail.com> URL: https://gitlist.dev/e/20211118092803.78395-1-dyroneteng%40gmail.com In-Reply-To: <211115.86r1bhsb8x.gmgdl@evledraar.gmail.com> ``` > If you make these an OPT_CMDMODE you get this behavior for free. See > e.g. my > https://lore.kernel.org/git/patch-v2-06.10-d945fc94774-20211112T221506Z-avarab@gmail.com/ Thank you very much for providing this input. So I try to read this patch your mentioned and try to repeat the idea in my understanding. First, OPT_CMDMODE() can be used for: 1. Easy for checking the combined command options, such as "mutually exclusive" conditions. 2. Die and output the error message consistently when the incompatible options are found. 3. Brings better extensibilites, no need to change a lot of if/elses. Then, you suggest to consider about to use OPT_CMDMODE instead of the current implementations. Did I understand your suggestion right and comprehensive? ``` ## Ævar Arnfjörð Bjarmason, 2021-11-18 11:00 Subject: Re: [PATCH 1/3] ls-tree.c: support `--oid-only` option for "git-ls-tree" Message-ID: <211118.86czmx67yi.gmgdl@evledraar.gmail.com> URL: https://gitlist.dev/e/211118.86czmx67yi.gmgdl%40evledraar.gmail.com In-Reply-To: <20211118092803.78395-1-dyroneteng@gmail.com> ``` On Thu, Nov 18 2021, Teng Long wrote: >> If you make these an OPT_CMDMODE you get this behavior for free. See >> e.g. my >> https://lore.kernel.org/git/patch-v2-06.10-d945fc94774-20211112T221506Z-avarab@gmail.com/ > > Thank you very much for providing this input. > > So I try to read this patch your mentioned and try to repeat the idea in my understanding. > > First, OPT_CMDMODE() can be used for: > > 1. Easy for checking the combined command options, such as "mutually exclusive" conditions. > > 2. Die and output the error message consistently when the incompatible options are found. > > 3. Brings better extensibilites, no need to change a lot of if/elses. > > Then, you suggest to consider about to use OPT_CMDMODE instead of the current implementations. > > Did I understand your suggestion right and comprehensive? Yes, all of that is correct. It's a way of defining N options, --foo, --bar, --baz, where combining any of them is an error. We usually use it for a "command mode" (hence the name), but it can be used when the command has flags that are mutually exclusive. I think (but am not sure, and didn't check) that you can even use it for --foo AND --bar that are exclusive, and --other --flags that are also mutually exclusive (but could be combined with one of --foo or --bar), you just need to provide another variable for it to set. But I haven't tested that or used it like that, maybe it doesn't work for some reason I'm forgetting... ``` ## Teng Long, 2021-11-18 11:23 Subject: Re: [PATCH 1/3] ls-tree.c: support `--oid-only` option for "git-ls-tree" Message-ID: <20211118112348.85948-1-dyroneteng@gmail.com> URL: https://gitlist.dev/e/20211118112348.85948-1-dyroneteng%40gmail.com In-Reply-To: ``` On Mon, 15 Nov 2021 14:16:27 -0500, Jeff King wrote: > This is a somewhat funny place to put the check, though. It will be run > for every entry in the tree (so is a tiny bit less efficient, but also > would not trigger for an empty tree). It probably should go in > cmd_ls_tree(), perhaps here: Yes, it's better here as a fail-fast case. According to the suggestion of the new location I think why not put the logic further head, after the parse_options() return, like: diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index 1f82229649..003a9ade54 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c @@ -166,6 +166,10 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, ls_tree_options, ls_tree_usage, 0); + + if ((ls_options & LS_NAME_ONLY) && (ls_options & LS_OID_ONLY)) + die(_("cannot specify --oid-only and --name-only at the same time")); + if (full_tree) { ls_tree_prefix = prefix = NULL; chomp_prefix = 0; Will it bring other new problems? Thank you. ``` ## Teng Long, 2021-11-19 02:57 Subject: Re: [PATCH 0/3] support `--oid-only` in `ls-tree` Message-ID: <20211119025749.91014-1-dyroneteng@gmail.com> URL: https://gitlist.dev/e/20211119025749.91014-1-dyroneteng%40gmail.com In-Reply-To: <211115.86o86lqe3c.gmgdl@evledraar.gmail.com> ``` > A quick patch to do it below, seems to work, passes all tests, but I > don't know how much I'd trust it. It's also quite an add use of > strbuf_expa(). We print to stdout directly since > write_name_quoted_relative() really wants to write to stdout, and not > give you a buffer. But I guess it makes sense in a way. Thanks for the patch and the inputs about "strbuf_expa()". > Then we'd be future-proof with the same interface expanding later, and > wouldn't need to support options that we're only carrying because we > didn't implement the more generic format support. I agree but like Peff said it maybe another bigger task. I think I will firstly solve the existing problems in next patch. I will consider about the generic format support but not sure whether it will continue to iterate in this patchset. > (Assume my Signed-off-by, if there's any interest...) Of course I will. Thank you very much for your advice and guidance again. ``` ## Teng Long, 2021-11-19 12:09 Subject: [PATCH v2 0/1] support `--oid-only` in `ls-tree` Message-ID: URL: https://gitlist.dev/e/cover.1637321601.git.dyroneteng%40gmail.com In-Reply-To: <20211115115153.48307-1-dyroneteng@gmail.com> ``` This patch series supports for only outputing the "objects" (OID) with a new option names `--oid-only`. Changes with the first patch are : 1. Three commits are squashed to 1 commit (Peff's advice) 2. The tests issues (Đoàn Trần Công Danh's advice) 3. Use `OPT_CMDMODE()` for mutually exclusive control (Ævar Arnfjörð Bjarmason's advice) Some discussions are not included in Patch 2 : 1. `git ls-tree --long --name-only` and `git ls-tree --long --oid-only` which is arguably a bug (Peff's advice) 2. Support `--format` for `git-ls-tree` (Ævar Arnfjörð Bjarmason's advice) The reason why these 2 discussions not included is I'm not sure whether I should continue on the current patchset or start a new one. And for the second, I think current implementation is clear and simple to use, meeting the needs of the moment. Maybe I will to support `--format` option, but before that, I'm appreciate if there are more suggestions appear. Thanks. Teng Long (1): ls-tree.c: support `--oid-only` option for "git-ls-tree" Documentation/git-ls-tree.txt | 8 +++++-- builtin/ls-tree.c | 27 ++++++++++++++++------- t/t3104-ls-tree-oid.sh | 40 +++++++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 10 deletions(-) create mode 100755 t/t3104-ls-tree-oid.sh Range-diff against v1: 1: c4479178d7 < -: ---------- ls-tree.c: support `--oid-only` option for "git-ls-tree" 2: 853ebbcf88 < -: ---------- t3104: add related tests for `--oid-only` option 3: 33c68c1f11 < -: ---------- git-ls-tree.txt: description of the 'oid-only' option -: ---------- > 1: 8b68568d6c ls-tree.c: support `--oid-only` option for "git-ls-tree" -- 2.33.1.10.g1f74a882e4 ``` ## Teng Long, 2021-11-19 12:09 Subject: [PATCH v2 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree" Message-ID: <8b68568d6cbe379d40c61c48bf446eaa88221df5.1637321601.git.dyroneteng@gmail.com> URL: https://gitlist.dev/e/8b68568d6cbe379d40c61c48bf446eaa88221df5.1637321601.git.dyroneteng%40gmail.com In-Reply-To: ``` Sometimes, we only want to get the objects from output of `ls-tree` and commands like `sed` or `cut` is usually used to intercept the origin output to achieve this purpose in practical. This commit supply an option names `--oid-only` to let `git ls-tree` only print out the OID of the object. `--oid-only` and `--name-only` are mutually exclusive in use. Reviewed-by: Jeff King Reviewed-by: Ævar Arnfjörð Bjarmason Reviewed-by: Đoàn Trần Công Danh Signed-off-by: Teng Long --- Documentation/git-ls-tree.txt | 8 +++++-- builtin/ls-tree.c | 27 ++++++++++++++++------- t/t3104-ls-tree-oid.sh | 40 +++++++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 10 deletions(-) create mode 100755 t/t3104-ls-tree-oid.sh diff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt index db02d6d79a..bc711dc00a 100644 --- a/Documentation/git-ls-tree.txt +++ b/Documentation/git-ls-tree.txt @@ -10,7 +10,8 @@ SYNOPSIS -------- [verse] 'git ls-tree' [-d] [-r] [-t] [-l] [-z] - [--name-only] [--name-status] [--full-name] [--full-tree] [--abbrev[=]] + [--name-only] [--name-status] [--oid-only] + [--full-name] [--full-tree] [--abbrev[=]] [...] DESCRIPTION @@ -59,7 +60,10 @@ OPTIONS --name-only:: --name-status:: List only filenames (instead of the "long" output), one per line. - + Cannot be used with `--oid-only` together. +--oid-only:: + List only OIDs of the objects, one per line. Cannot be used with + `--name-only` or `--name-status` together. --abbrev[=]:: Instead of showing the full 40-byte hexadecimal object lines, show the shortest prefix that is at least '' diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index 3a442631c7..1e4a82e669 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c @@ -18,19 +18,26 @@ static int line_termination = '\n'; #define LS_RECURSIVE 1 #define LS_TREE_ONLY 2 #define LS_SHOW_TREES 4 -#define LS_NAME_ONLY 8 -#define LS_SHOW_SIZE 16 +#define LS_SHOW_SIZE 8 static int abbrev; static int ls_options; static struct pathspec pathspec; static int chomp_prefix; static const char *ls_tree_prefix; -static const char * const ls_tree_usage[] = { +static const char * const ls_tree_usage[] = { N_("git ls-tree [] [...]"), NULL }; +enum { + MODE_UNSPECIFIED = 0, + MODE_NAME_ONLY, + MODE_OID_ONLY +}; + +static int cmdmode = MODE_UNSPECIFIED; + static int show_recursive(const char *base, int baselen, const char *pathname) { int i; @@ -90,7 +97,12 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, else if (ls_options & LS_TREE_ONLY) return 0; - if (!(ls_options & LS_NAME_ONLY)) { + if (cmdmode == 2) { + printf("%s\n", find_unique_abbrev(oid, abbrev)); + return 0; + } + + if (cmdmode == 0) { if (ls_options & LS_SHOW_SIZE) { char size_text[24]; if (!strcmp(type, blob_type)) { @@ -135,10 +147,9 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) N_("terminate entries with NUL byte"), 0), OPT_BIT('l', "long", &ls_options, N_("include object size"), LS_SHOW_SIZE), - OPT_BIT(0, "name-only", &ls_options, N_("list only filenames"), - LS_NAME_ONLY), - OPT_BIT(0, "name-status", &ls_options, N_("list only filenames"), - LS_NAME_ONLY), + OPT_CMDMODE('n', "name-only", &cmdmode, N_("list only filenames"), MODE_NAME_ONLY), + OPT_CMDMODE('s', "name-status", &cmdmode, N_("list only filenames"), MODE_NAME_ONLY), + OPT_CMDMODE('o', "oid-only", &cmdmode, N_("list only oids"), MODE_OID_ONLY), OPT_SET_INT(0, "full-name", &chomp_prefix, N_("use full path names"), 0), OPT_BOOL(0, "full-tree", &full_tree, diff --git a/t/t3104-ls-tree-oid.sh b/t/t3104-ls-tree-oid.sh new file mode 100755 index 0000000000..4c02cdd3c3 --- /dev/null +++ b/t/t3104-ls-tree-oid.sh @@ -0,0 +1,40 @@ +#!/bin/sh + +test_description='git ls-tree oids handling.' + +. ./test-lib.sh + +test_expect_success 'setup' ' + echo 111 >1.txt && + echo 222 >2.txt && + mkdir -p path0/a/b/c && + echo 333 >path0/a/b/c/3.txt && + find *.txt path* \( -type f -o -type l \) -print | + xargs git update-index --add && + tree=$(git write-tree) && + echo $tree +' + +test_expect_success 'usage: --oid-only' ' + git ls-tree --oid-only $tree >current && + git ls-tree $tree | awk "{print \$3}" >expected && + test_cmp current expected +' + +test_expect_success 'usage: --oid-only with -r' ' + git ls-tree --oid-only -r $tree >current && + git ls-tree -r $tree | awk "{print \$3}" >expected && + test_cmp current expected +' + +test_expect_success 'usage: --oid-only with --abbrev' ' + git ls-tree --oid-only --abbrev=6 $tree >current && + git ls-tree --abbrev=6 $tree | awk "{print \$3}" > expected && + test_cmp current expected +' + +test_expect_failure 'usage: incompatible options: --name-only with --oid-only' ' + test_incompatible_usage git ls-tree --oid-only --name-only +' + +test_done -- 2.33.1.10.g1f74a882e4 ``` ## Ævar Arnfjörð Bjarmason, 2021-11-19 13:30 Subject: Re: [PATCH v2 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree" Message-ID: <211119.86wnl42ri2.gmgdl@evledraar.gmail.com> URL: https://gitlist.dev/e/211119.86wnl42ri2.gmgdl%40evledraar.gmail.com In-Reply-To: <8b68568d6cbe379d40c61c48bf446eaa88221df5.1637321601.git.dyroneteng@gmail.com> ``` On Fri, Nov 19 2021, Teng Long wrote: > Reviewed-by: Jeff King > Reviewed-by: Ævar Arnfjörð Bjarmason > Reviewed-by: Đoàn Trần Công Danh Please don't add the Reviewed-by headers yourself, either Junio accumulates them, or if someone explicitly mentions that you can add it with their name it's OK. It doesn't just mean this person reviewed this series in some ML thread, but "this person is 100% OK with this in its current form". > List only filenames (instead of the "long" output), one per line. > - > + Cannot be used with `--oid-only` together. Better: "Cannot be combined with OPT." > +--oid-only:: > + List only OIDs of the objects, one per line. Cannot be used with > + `--name-only` or `--name-status` together. Better: "Cannot be combined with OPT or OPT2." > +enum { > + MODE_UNSPECIFIED = 0, > + MODE_NAME_ONLY, > + MODE_OID_ONLY > +}; > + > +static int cmdmode = MODE_UNSPECIFIED; Better: static enum { MODE_NAME_ONLY = 1, ... } cmdmode = MODE_NAME_ONLY; I.e. no need for the MODE_UNSPECIFIED just to skip past "0". > @@ -135,10 +147,9 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) > N_("terminate entries with NUL byte"), 0), > OPT_BIT('l', "long", &ls_options, N_("include object size"), > LS_SHOW_SIZE), > - OPT_BIT(0, "name-only", &ls_options, N_("list only filenames"), > - LS_NAME_ONLY), > - OPT_BIT(0, "name-status", &ls_options, N_("list only filenames"), > - LS_NAME_ONLY), > + OPT_CMDMODE('n', "name-only", &cmdmode, N_("list only filenames"), MODE_NAME_ONLY), > + OPT_CMDMODE('s', "name-status", &cmdmode, N_("list only filenames"), MODE_NAME_ONLY), > + OPT_CMDMODE('o', "oid-only", &cmdmode, N_("list only oids"), MODE_OID_ONLY), Better to preserve the wrapping here, to stay within 79 columns. > +test_expect_success 'setup' ' > + echo 111 >1.txt && > + echo 222 >2.txt && Just use: test_commit A && test_commit B etc? > + mkdir -p path0/a/b/c && > + echo 333 >path0/a/b/c/3.txt && > + find *.txt path* \( -type f -o -type l \) -print | > + xargs git update-index --add && > + tree=$(git write-tree) && > + echo $tree Stray echo? Unclear why this test setup is so complex, shouldn't this just be (continued from above): mkdir -p C && test_commit C/D.txt To test nested dirs? > +' > + > +test_expect_success 'usage: --oid-only' ' > + git ls-tree --oid-only $tree >current && > + git ls-tree $tree | awk "{print \$3}" >expected && just cut -f1 instead of awk? Also don't put "git" on the LHS of a pipe, it might hide segfaults. Also applies to the below. > + test_cmp current expected > +' > + > +test_expect_success 'usage: --oid-only with -r' ' > + git ls-tree --oid-only -r $tree >current && > + git ls-tree -r $tree | awk "{print \$3}" >expected && > + test_cmp current expected > +' > + > +test_expect_success 'usage: --oid-only with --abbrev' ' > + git ls-tree --oid-only --abbrev=6 $tree >current && > + git ls-tree --abbrev=6 $tree | awk "{print \$3}" > expected && > + test_cmp current expected > +' > + > +test_expect_failure 'usage: incompatible options: --name-only with --oid-only' ' > + test_incompatible_usage git ls-tree --oid-only --name-only > +' Hrm, did you copy this use of test_incompatible_usage from t1006-cat-file.sh without providing the function? More data for: https://lore.kernel.org/git/87tuhmk19c.fsf@evledraar.gmail.com/ :) Better to use: test_expect_code 128 ... # (or was it 129?) ``` ## Junio C Hamano, 2021-11-19 17:32 Subject: Re: [PATCH v2 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree" Message-ID: URL: https://gitlist.dev/e/xmqqy25koxua.fsf%40gitster.g In-Reply-To: <211119.86wnl42ri2.gmgdl@evledraar.gmail.com> ``` Ævar Arnfjörð Bjarmason writes: I think that many points you raised in your message are valid, but there is one thing that is not. >> +enum { >> + MODE_UNSPECIFIED = 0, >> + MODE_NAME_ONLY, >> + MODE_OID_ONLY >> +}; >> + >> +static int cmdmode = MODE_UNSPECIFIED; > > Better: > > static enum { > MODE_NAME_ONLY = 1, > ... > } cmdmode = MODE_NAME_ONLY; > > I.e. no need for the MODE_UNSPECIFIED just to skip past "0". If the original wanted to make the default to be "unspecified", your suggestion changes the semantics. "enum" is not necessarily an "int", and because the pointer of "cmdmode" is given to OPT_CMDMODE(), which expects a pointer to "int", your suggestion breaks the code there, too. I wonder if cmdmode cannot be a on-stack variable in cmd_ls_tree() that is passed as the context pointer to show_tree() via read_tree(), though. The enum definition still need to be visible throughout the file, but such a structure would let us lose a "global" variable. Thanks. ``` ## Teng Long, 2021-11-22 07:45 Subject: Re: [PATCH v2 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree" Message-ID: <20211122074538.87255-1-dyroneteng@gmail.com> URL: https://gitlist.dev/e/20211122074538.87255-1-dyroneteng%40gmail.com In-Reply-To: <211119.86wnl42ri2.gmgdl@evledraar.gmail.com> ``` On Fri, 19 Nov 2021 14:30:52 +0100, Ævar Arnfjörð Bjarmason wrote > Please don't add the Reviewed-by headers yourself, either Junio > accumulates them, or if someone explicitly mentions that you can add it > with their name it's OK. I think I misunderstood the meanings of the header before. Thanks for the important tips. > Better: "Cannot be combined with OPT." > Better: "Cannot be combined with OPT or OPT2." > ... > Better to preserve the wrapping here, to stay within 79 columns. Will apply. > Just use: > > test_commit A && > test_commit B > > etc? > ... > Stray echo? Unclear why this test setup is so complex, shouldn't this just be (continued from above): > > mkdir -p C && > test_commit C/D.txt > To test nested dirs? Will apply. > just cut -f1 instead of awk? Also don't put "git" on the LHS of a pipe, > it might hide segfaults. Also applies to the below. > Will apply, and could you please describe the problem with more details? (appreciate if there is an executable example) Thank you. ``` ## Teng Long, 2021-11-22 08:07 Subject: [PATCH v3 0/1] ls-tree.c: support `--oid-only` option Message-ID: URL: https://gitlist.dev/e/cover.1637567328.git.dyroneteng%40gmail.com In-Reply-To: ``` Diffs from previous patch: 1. Remove "Reviewed-by" headers in commit message. 2. Optimize option descriptions in Doc. (Ævar Arnfjörð Bjarmason' advice) 3. Optimize and bugfix in "t3104". (Ævar Arnfjörð Bjarmason' advice) 4. The formatting problems of line wrappers (over 79 col) All the advices are from Ævar Arnfjörð Bjarmason and Junio C Hamano, thank you very much. Althought some advices are apply in this path, but some questions remains, they are in link [1]. [1] https://public-inbox.org/git/20211122074538.87255-1-dyroneteng@gmail.com/ Teng Long (1): ls-tree.c: support `--oid-only` option for "git-ls-tree" Documentation/git-ls-tree.txt | 8 +++++-- builtin/ls-tree.c | 27 ++++++++++++++++------- t/t3104-ls-tree-oid.sh | 40 +++++++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 10 deletions(-) create mode 100755 t/t3104-ls-tree-oid.sh Range-diff against v2: 1: 8b68568d6c ! 1: 6c15b4c176 ls-tree.c: support `--oid-only` option for "git-ls-tree" @@ Commit message only print out the OID of the object. `--oid-only` and `--name-only` are mutually exclusive in use. - Reviewed-by: Jeff King - Reviewed-by: Ævar Arnfjörð Bjarmason - Reviewed-by: Đoàn Trần Công Danh Signed-off-by: Teng Long ## Documentation/git-ls-tree.txt ## -- 2.33.1.10.g438dd9044d.dirty ``` ## Teng Long, 2021-11-22 08:07 Subject: [PATCH v3 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree" Message-ID: <6c15b4c176b7c03072fa59a4efd9f6fea7d62eae.1637567328.git.dyroneteng@gmail.com> URL: https://gitlist.dev/e/6c15b4c176b7c03072fa59a4efd9f6fea7d62eae.1637567328.git.dyroneteng%40gmail.com In-Reply-To: ``` Sometimes, we only want to get the objects from output of `ls-tree` and commands like `sed` or `cut` is usually used to intercept the origin output to achieve this purpose in practical. This commit supply an option names `--oid-only` to let `git ls-tree` only print out the OID of the object. `--oid-only` and `--name-only` are mutually exclusive in use. Signed-off-by: Teng Long --- Documentation/git-ls-tree.txt | 8 +++++-- builtin/ls-tree.c | 27 ++++++++++++++++------- t/t3104-ls-tree-oid.sh | 40 +++++++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 10 deletions(-) create mode 100755 t/t3104-ls-tree-oid.sh diff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt index db02d6d79a..bc711dc00a 100644 --- a/Documentation/git-ls-tree.txt +++ b/Documentation/git-ls-tree.txt @@ -10,7 +10,8 @@ SYNOPSIS -------- [verse] 'git ls-tree' [-d] [-r] [-t] [-l] [-z] - [--name-only] [--name-status] [--full-name] [--full-tree] [--abbrev[=]] + [--name-only] [--name-status] [--oid-only] + [--full-name] [--full-tree] [--abbrev[=]] [...] DESCRIPTION @@ -59,7 +60,10 @@ OPTIONS --name-only:: --name-status:: List only filenames (instead of the "long" output), one per line. - + Cannot be used with `--oid-only` together. +--oid-only:: + List only OIDs of the objects, one per line. Cannot be used with + `--name-only` or `--name-status` together. --abbrev[=]:: Instead of showing the full 40-byte hexadecimal object lines, show the shortest prefix that is at least '' diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index 3a442631c7..1e4a82e669 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c @@ -18,19 +18,26 @@ static int line_termination = '\n'; #define LS_RECURSIVE 1 #define LS_TREE_ONLY 2 #define LS_SHOW_TREES 4 -#define LS_NAME_ONLY 8 -#define LS_SHOW_SIZE 16 +#define LS_SHOW_SIZE 8 static int abbrev; static int ls_options; static struct pathspec pathspec; static int chomp_prefix; static const char *ls_tree_prefix; -static const char * const ls_tree_usage[] = { +static const char * const ls_tree_usage[] = { N_("git ls-tree [] [...]"), NULL }; +enum { + MODE_UNSPECIFIED = 0, + MODE_NAME_ONLY, + MODE_OID_ONLY +}; + +static int cmdmode = MODE_UNSPECIFIED; + static int show_recursive(const char *base, int baselen, const char *pathname) { int i; @@ -90,7 +97,12 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, else if (ls_options & LS_TREE_ONLY) return 0; - if (!(ls_options & LS_NAME_ONLY)) { + if (cmdmode == 2) { + printf("%s\n", find_unique_abbrev(oid, abbrev)); + return 0; + } + + if (cmdmode == 0) { if (ls_options & LS_SHOW_SIZE) { char size_text[24]; if (!strcmp(type, blob_type)) { @@ -135,10 +147,9 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) N_("terminate entries with NUL byte"), 0), OPT_BIT('l', "long", &ls_options, N_("include object size"), LS_SHOW_SIZE), - OPT_BIT(0, "name-only", &ls_options, N_("list only filenames"), - LS_NAME_ONLY), - OPT_BIT(0, "name-status", &ls_options, N_("list only filenames"), - LS_NAME_ONLY), + OPT_CMDMODE('n', "name-only", &cmdmode, N_("list only filenames"), MODE_NAME_ONLY), + OPT_CMDMODE('s', "name-status", &cmdmode, N_("list only filenames"), MODE_NAME_ONLY), + OPT_CMDMODE('o', "oid-only", &cmdmode, N_("list only oids"), MODE_OID_ONLY), OPT_SET_INT(0, "full-name", &chomp_prefix, N_("use full path names"), 0), OPT_BOOL(0, "full-tree", &full_tree, diff --git a/t/t3104-ls-tree-oid.sh b/t/t3104-ls-tree-oid.sh new file mode 100755 index 0000000000..4c02cdd3c3 --- /dev/null +++ b/t/t3104-ls-tree-oid.sh @@ -0,0 +1,40 @@ +#!/bin/sh + +test_description='git ls-tree oids handling.' + +. ./test-lib.sh + +test_expect_success 'setup' ' + echo 111 >1.txt && + echo 222 >2.txt && + mkdir -p path0/a/b/c && + echo 333 >path0/a/b/c/3.txt && + find *.txt path* \( -type f -o -type l \) -print | + xargs git update-index --add && + tree=$(git write-tree) && + echo $tree +' + +test_expect_success 'usage: --oid-only' ' + git ls-tree --oid-only $tree >current && + git ls-tree $tree | awk "{print \$3}" >expected && + test_cmp current expected +' + +test_expect_success 'usage: --oid-only with -r' ' + git ls-tree --oid-only -r $tree >current && + git ls-tree -r $tree | awk "{print \$3}" >expected && + test_cmp current expected +' + +test_expect_success 'usage: --oid-only with --abbrev' ' + git ls-tree --oid-only --abbrev=6 $tree >current && + git ls-tree --abbrev=6 $tree | awk "{print \$3}" > expected && + test_cmp current expected +' + +test_expect_failure 'usage: incompatible options: --name-only with --oid-only' ' + test_incompatible_usage git ls-tree --oid-only --name-only +' + +test_done -- 2.33.1.10.g438dd9044d.dirty ``` ## Ævar Arnfjörð Bjarmason, 2021-11-22 11:14 Subject: Re: [PATCH v2 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree" Message-ID: <211122.8635no1lwp.gmgdl@evledraar.gmail.com> URL: https://gitlist.dev/e/211122.8635no1lwp.gmgdl%40evledraar.gmail.com In-Reply-To: <20211122074538.87255-1-dyroneteng@gmail.com> ``` On Mon, Nov 22 2021, Teng Long wrote: > On Fri, 19 Nov 2021 14:30:52 +0100, Ævar Arnfjörð Bjarmason wrote >> just cut -f1 instead of awk? Also don't put "git" on the LHS of a pipe, >> it might hide segfaults. Also applies to the below. >> > > Will apply, and could you please describe the problem with more details? > (appreciate if there is an executable example) Run this in a terminal: git stawtus | cat; echo $?; The LHS of the pipe fails, but the exit code of that command is hidden. So we prefer: git stawtus >out && # fails [...] ``` ## Peter Baumann, 2021-11-22 18:11 Subject: Re: [PATCH v3 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree" Message-ID: URL: https://gitlist.dev/e/CAJm9OHd_MDjFqXok663O8bQmvdPCjrQeEHumPLDpxqVC4i4%3D1Q%40mail.gmail.com In-Reply-To: <6c15b4c176b7c03072fa59a4efd9f6fea7d62eae.1637567328.git.dyroneteng@gmail.com> ``` [ Sorry if you receive this mail twice, it seems like it didn't get through the first time. ] On Mon, Nov 22, 2021 at 9:50 AM Teng Long wrote: > > Sometimes, we only want to get the objects from output of `ls-tree` > and commands like `sed` or `cut` is usually used to intercept the > origin output to achieve this purpose in practical. > > This commit supply an option names `--oid-only` to let `git ls-tree` > only print out the OID of the object. `--oid-only` and `--name-only` > are mutually exclusive in use. > > Signed-off-by: Teng Long > --- > Documentation/git-ls-tree.txt | 8 +++++-- > builtin/ls-tree.c | 27 ++++++++++++++++------- > t/t3104-ls-tree-oid.sh | 40 +++++++++++++++++++++++++++++++++++ > 3 files changed, 65 insertions(+), 10 deletions(-) > create mode 100755 t/t3104-ls-tree-oid.sh > > diff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt > index db02d6d79a..bc711dc00a 100644 > --- a/Documentation/git-ls-tree.txt > +++ b/Documentation/git-ls-tree.txt > @@ -10,7 +10,8 @@ SYNOPSIS > -------- > [verse] > 'git ls-tree' [-d] [-r] [-t] [-l] [-z] > - [--name-only] [--name-status] [--full-name] [--full-tree] [--abbrev[=]] > + [--name-only] [--name-status] [--oid-only] > + [--full-name] [--full-tree] [--abbrev[=]] > [...] > Shouldn't the synopsis also indicate that the options are exclusive, e.g. [--name-only | --oid-only] ? Besides adding the new --oid-only mode, you also add one letter acronyms for [-n | --name-only] [-s | --name-status ] and one letter abbreviation [-o | --oid-only ] which are all undocumented in the help page. If we want the short one letter version, they should be documented. For me, it is at least questionable why we introduce them and more so in a commit adding --oid-only. > DESCRIPTION > @@ -59,7 +60,10 @@ OPTIONS > --name-only:: > --name-status:: > List only filenames (instead of the "long" output), one per line. > - > + Cannot be used with `--oid-only` together. > +--oid-only:: > + List only OIDs of the objects, one per line. Cannot be used with > + `--name-only` or `--name-status` together. > --abbrev[=]:: > Instead of showing the full 40-byte hexadecimal object > lines, show the shortest prefix that is at least '' > diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c > index 3a442631c7..1e4a82e669 100644 > --- a/builtin/ls-tree.c > +++ b/builtin/ls-tree.c > @@ -18,19 +18,26 @@ static int line_termination = '\n'; > #define LS_RECURSIVE 1 > #define LS_TREE_ONLY 2 > #define LS_SHOW_TREES 4 > -#define LS_NAME_ONLY 8 > -#define LS_SHOW_SIZE 16 > +#define LS_SHOW_SIZE 8 > static int abbrev; > static int ls_options; > static struct pathspec pathspec; > static int chomp_prefix; > static const char *ls_tree_prefix; > > -static const char * const ls_tree_usage[] = { > +static const char * const ls_tree_usage[] = { > N_("git ls-tree [] [...]"), > NULL > }; > > +enum { > + MODE_UNSPECIFIED = 0, > + MODE_NAME_ONLY, > + MODE_OID_ONLY > +}; > + > +static int cmdmode = MODE_UNSPECIFIED; > + > static int show_recursive(const char *base, int baselen, const char *pathname) > { > int i; > @@ -90,7 +97,12 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, > else if (ls_options & LS_TREE_ONLY) > return 0; > > - if (!(ls_options & LS_NAME_ONLY)) { > + if (cmdmode == 2) { > + printf("%s\n", find_unique_abbrev(oid, abbrev)); > + return 0; > + } > + > + if (cmdmode == 0) { > if (ls_options & LS_SHOW_SIZE) { > char size_text[24]; > if (!strcmp(type, blob_type)) { > @@ -135,10 +147,9 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) > N_("terminate entries with NUL byte"), 0), > OPT_BIT('l', "long", &ls_options, N_("include object size"), > LS_SHOW_SIZE), > - OPT_BIT(0, "name-only", &ls_options, N_("list only filenames"), > - LS_NAME_ONLY), > - OPT_BIT(0, "name-status", &ls_options, N_("list only filenames"), > - LS_NAME_ONLY), > + OPT_CMDMODE('n', "name-only", &cmdmode, N_("list only filenames"), MODE_NAME_ONLY), > + OPT_CMDMODE('s', "name-status", &cmdmode, N_("list only filenames"), MODE_NAME_ONLY), > + OPT_CMDMODE('o', "oid-only", &cmdmode, N_("list only oids"), MODE_OID_ONLY), > OPT_SET_INT(0, "full-name", &chomp_prefix, > N_("use full path names"), 0), > OPT_BOOL(0, "full-tree", &full_tree, > diff --git a/t/t3104-ls-tree-oid.sh b/t/t3104-ls-tree-oid.sh > new file mode 100755 > index 0000000000..4c02cdd3c3 > --- /dev/null > +++ b/t/t3104-ls-tree-oid.sh > @@ -0,0 +1,40 @@ > +#!/bin/sh > + > +test_description='git ls-tree oids handling.' > + > +. ./test-lib.sh > + > +test_expect_success 'setup' ' > + echo 111 >1.txt && > + echo 222 >2.txt && > + mkdir -p path0/a/b/c && > + echo 333 >path0/a/b/c/3.txt && > + find *.txt path* \( -type f -o -type l \) -print | I don't see the test using any symbolic links. Why are we searching for symbolic links with "-type l" here? -Peter > + xargs git update-index --add && > + tree=$(git write-tree) && > + echo $tree > +' > + > +test_expect_success 'usage: --oid-only' ' > + git ls-tree --oid-only $tree >current && > + git ls-tree $tree | awk "{print \$3}" >expected && > + test_cmp current expected > +' > + > +test_expect_success 'usage: --oid-only with -r' ' > + git ls-tree --oid-only -r $tree >current && > + git ls-tree -r $tree | awk "{print \$3}" >expected && > + test_cmp current expected > +' > + > +test_expect_success 'usage: --oid-only with --abbrev' ' > + git ls-tree --oid-only --abbrev=6 $tree >current && > + git ls-tree --abbrev=6 $tree | awk "{print \$3}" > expected && > + test_cmp current expected > +' > + > +test_expect_failure 'usage: incompatible options: --name-only with --oid-only' ' > + test_incompatible_usage git ls-tree --oid-only --name-only > +' > + > +test_done > -- > 2.33.1.10.g438dd9044d.dirty > ``` ## Junio C Hamano, 2021-11-22 18:54 Subject: Re: [PATCH v3 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree" Message-ID: URL: https://gitlist.dev/e/xmqqczmsf2d2.fsf%40gitster.g In-Reply-To: <6c15b4c176b7c03072fa59a4efd9f6fea7d62eae.1637567328.git.dyroneteng@gmail.com> ``` Teng Long writes: > Sometimes, we only want to get the objects from output of `ls-tree` > and commands like `sed` or `cut` is usually used to intercept the > origin output to achieve this purpose in practical. "in practical" -> "in practice". That's true and that is exactly this plumbing command was designed to be used. > This commit supply an option names `--oid-only` to let `git ls-tree` > only print out the OID of the object. `--oid-only` and `--name-only` > are mutually exclusive in use. Teach the "--oid-only" option to tell the command to only show the object name, just like "--name-only" option tells the command to only show the path component, for each entry. These two options are mutually exclusive. perhaps? The above leaves "mode-only" and "type-only". I wonder if it is a better design to add just one new option, --hide-fields, and make the existing --name-only into a synonym to git ls-tree --hide-fields=mode,type,object $T which would mean we do not need to end up with four mutually exclusive commands, and anybody who wants to only see object names can do git ls-tree --hide-fields=mode,type,file $T Note: the above uses the terminology in the OUTPUT FORMAT section; if we want to use "name" instead of "file", I am perfectly OK with it, but then we should update the documentation to match. Come to think of it, I think "--show-fields" may work even better than "--hide-fields". We can use it to get rid of the "--long" option: git ls-tree --show-fields=mode,type,object,size,file $T would be equivelent to git ls-tree --long $T The field order may need to be thought through, especially when "-z" output is not being used. We may need a rule to require "file" to be at the end, if exists, or even simpler rule "you can choose which fields are shown but the order they come out is not affected" (i.e. "--show-fields=mode,type" and "--show-fields=type,mode" give the same output). I am OK if we started with "only a single field allowed" and extend it to support multiple fields later (until that happens, we cannot emulate the "--long" output, though). Then we do not have to answer two tricky questions, what to do with the output order, and what field separators are used in the output. ``` ## Đoàn Trần Công Danh, 2021-11-23 00:14 Subject: Re: [PATCH v3 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree" Message-ID: URL: https://gitlist.dev/e/YZwyXAn%2BgxUZ%2BaD9%40danh.dev In-Reply-To: <6c15b4c176b7c03072fa59a4efd9f6fea7d62eae.1637567328.git.dyroneteng@gmail.com> ``` On 2021-11-22 16:07:28+0800, Teng Long wrote: > Sometimes, we only want to get the objects from output of `ls-tree` > and commands like `sed` or `cut` is usually used to intercept the > origin output to achieve this purpose in practical. > > This commit supply an option names `--oid-only` to let `git ls-tree` > only print out the OID of the object. `--oid-only` and `--name-only` > are mutually exclusive in use. > > Signed-off-by: Teng Long > --- > Documentation/git-ls-tree.txt | 8 +++++-- > builtin/ls-tree.c | 27 ++++++++++++++++------- > t/t3104-ls-tree-oid.sh | 40 +++++++++++++++++++++++++++++++++++ > 3 files changed, 65 insertions(+), 10 deletions(-) > create mode 100755 t/t3104-ls-tree-oid.sh > > diff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt > index db02d6d79a..bc711dc00a 100644 > --- a/Documentation/git-ls-tree.txt > +++ b/Documentation/git-ls-tree.txt > @@ -10,7 +10,8 @@ SYNOPSIS > -------- > [verse] > 'git ls-tree' [-d] [-r] [-t] [-l] [-z] > - [--name-only] [--name-status] [--full-name] [--full-tree] [--abbrev[=]] > + [--name-only] [--name-status] [--oid-only] Please indicate those options are incompatible (as someone else said): [--name-only | --name-status | --oid-only] > + [--full-name] [--full-tree] [--abbrev[=]] > [...] > > DESCRIPTION > @@ -59,7 +60,10 @@ OPTIONS > --name-only:: > --name-status:: > List only filenames (instead of the "long" output), one per line. > - > + Cannot be used with `--oid-only` together. > +--oid-only:: > + List only OIDs of the objects, one per line. Cannot be used with > + `--name-only` or `--name-status` together. > --abbrev[=]:: > Instead of showing the full 40-byte hexadecimal object > lines, show the shortest prefix that is at least '' > diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c > index 3a442631c7..1e4a82e669 100644 > --- a/builtin/ls-tree.c > +++ b/builtin/ls-tree.c > @@ -18,19 +18,26 @@ static int line_termination = '\n'; > #define LS_RECURSIVE 1 > #define LS_TREE_ONLY 2 > #define LS_SHOW_TREES 4 > -#define LS_NAME_ONLY 8 > -#define LS_SHOW_SIZE 16 > +#define LS_SHOW_SIZE 8 > static int abbrev; > static int ls_options; > static struct pathspec pathspec; > static int chomp_prefix; > static const char *ls_tree_prefix; > > -static const char * const ls_tree_usage[] = { > +static const char * const ls_tree_usage[] = { > N_("git ls-tree [] [...]"), > NULL > }; > > +enum { > + MODE_UNSPECIFIED = 0, > + MODE_NAME_ONLY, > + MODE_OID_ONLY > +}; > + > +static int cmdmode = MODE_UNSPECIFIED; > + > static int show_recursive(const char *base, int baselen, const char *pathname) > { > int i; > @@ -90,7 +97,12 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, > else if (ls_options & LS_TREE_ONLY) > return 0; > > - if (!(ls_options & LS_NAME_ONLY)) { > + if (cmdmode == 2) { I think it's better to use the enum name: if (cmdmode == MODE_OID_ONLY) { > + printf("%s\n", find_unique_abbrev(oid, abbrev)); > + return 0; > + } > + > + if (cmdmode == 0) { Ditto: if (cmdmode == MODE_UNSPECIFIED) { Speaking about this, where will MODE_NAME_ONLY be used? > if (ls_options & LS_SHOW_SIZE) { > char size_text[24]; > if (!strcmp(type, blob_type)) { > @@ -135,10 +147,9 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) > N_("terminate entries with NUL byte"), 0), > OPT_BIT('l', "long", &ls_options, N_("include object size"), > LS_SHOW_SIZE), > - OPT_BIT(0, "name-only", &ls_options, N_("list only filenames"), > - LS_NAME_ONLY), > - OPT_BIT(0, "name-status", &ls_options, N_("list only filenames"), > - LS_NAME_ONLY), > + OPT_CMDMODE('n', "name-only", &cmdmode, N_("list only filenames"), MODE_NAME_ONLY), > + OPT_CMDMODE('s', "name-status", &cmdmode, N_("list only filenames"), MODE_NAME_ONLY), > + OPT_CMDMODE('o', "oid-only", &cmdmode, N_("list only oids"), MODE_OID_ONLY), > OPT_SET_INT(0, "full-name", &chomp_prefix, > N_("use full path names"), 0), > OPT_BOOL(0, "full-tree", &full_tree, > diff --git a/t/t3104-ls-tree-oid.sh b/t/t3104-ls-tree-oid.sh > new file mode 100755 > index 0000000000..4c02cdd3c3 > --- /dev/null > +++ b/t/t3104-ls-tree-oid.sh > @@ -0,0 +1,40 @@ > +#!/bin/sh > + > +test_description='git ls-tree oids handling.' > + > +. ./test-lib.sh > + > +test_expect_success 'setup' ' > + echo 111 >1.txt && > + echo 222 >2.txt && > + mkdir -p path0/a/b/c && > + echo 333 >path0/a/b/c/3.txt && > + find *.txt path* \( -type f -o -type l \) -print | > + xargs git update-index --add && > + tree=$(git write-tree) && > + echo $tree > +' > + > +test_expect_success 'usage: --oid-only' ' > + git ls-tree --oid-only $tree >current && > + git ls-tree $tree | awk "{print \$3}" >expected && > + test_cmp current expected > +' > + > +test_expect_success 'usage: --oid-only with -r' ' > + git ls-tree --oid-only -r $tree >current && > + git ls-tree -r $tree | awk "{print \$3}" >expected && > + test_cmp current expected > +' > + > +test_expect_success 'usage: --oid-only with --abbrev' ' > + git ls-tree --oid-only --abbrev=6 $tree >current && > + git ls-tree --abbrev=6 $tree | awk "{print \$3}" > expected && > + test_cmp current expected > +' > + > +test_expect_failure 'usage: incompatible options: --name-only with --oid-only' ' > + test_incompatible_usage git ls-tree --oid-only --name-only > +' > + > +test_done It seems like you haven't updated the test code from v2 -- Danh ``` ## Ævar Arnfjörð Bjarmason, 2021-11-23 01:09 Subject: Re: [PATCH v3 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree" Message-ID: <211123.86tug3wu8v.gmgdl@evledraar.gmail.com> URL: https://gitlist.dev/e/211123.86tug3wu8v.gmgdl%40evledraar.gmail.com In-Reply-To: ``` On Mon, Nov 22 2021, Junio C Hamano wrote: > Teng Long writes: > >> Sometimes, we only want to get the objects from output of `ls-tree` >> and commands like `sed` or `cut` is usually used to intercept the >> origin output to achieve this purpose in practical. > > "in practical" -> "in practice". > > That's true and that is exactly this plumbing command was designed > to be used. > >> This commit supply an option names `--oid-only` to let `git ls-tree` >> only print out the OID of the object. `--oid-only` and `--name-only` >> are mutually exclusive in use. > > Teach the "--oid-only" option to tell the command to only show > the object name, just like "--name-only" option tells the > command to only show the path component, for each entry. These > two options are mutually exclusive. > > perhaps? > > The above leaves "mode-only" and "type-only". I wonder if it is a > better design to add just one new option, --hide-fields, and make > the existing --name-only into a synonym to > > git ls-tree --hide-fields=mode,type,object $T > > which would mean we do not need to end up with four mutually > exclusive commands, and anybody who wants to only see object names > can do > > git ls-tree --hide-fields=mode,type,file $T > > Note: the above uses the terminology in the OUTPUT FORMAT section; > if we want to use "name" instead of "file", I am perfectly OK with > it, but then we should update the documentation to match. > > Come to think of it, I think "--show-fields" may work even better > than "--hide-fields". We can use it to get rid of the "--long" > option: > > git ls-tree --show-fields=mode,type,object,size,file $T > > would be equivelent to > > git ls-tree --long $T > > The field order may need to be thought through, especially when "-z" > output is not being used. We may need a rule to require "file" to > be at the end, if exists, or even simpler rule "you can choose which > fields are shown but the order they come out is not affected" (i.e. > "--show-fields=mode,type" and "--show-fields=type,mode" give the > same output). > > I am OK if we started with "only a single field allowed" and extend > it to support multiple fields later (until that happens, we cannot > emulate the "--long" output, though). Then we do not have to answer > two tricky questions, what to do with the output order, and what > field separators are used in the output. All of which (and more) would also be addressed in an obvious way by just supporting --format as I suggested in https://lore.kernel.org/git/211115.86o86lqe3c.gmgdl@evledraar.gmail.com/; don't you think that's a better approach? As noted in https://lore.kernel.org/git/211115.86mtm5saz7.gmgdl@evledraar.gmail.com/ we could start by simply dying if the format is not on a small list of formats we handle, i.e. not implement the strbuf_expand() change to start with. A --show-fields and --hide-fields seems like a rather elaborate interface in lieu of just having a --format. You'd presumably then want a --field-seperator and --name-field-separator (we use SP and TAB for the two, currently), so we've got N option now just to emulate what a --format would do for us. ``` ## Junio C Hamano, 2021-11-23 01:26 Subject: Re: [PATCH v3 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree" Message-ID: URL: https://gitlist.dev/e/xmqqpmqrachl.fsf%40gitster.g In-Reply-To: <211123.86tug3wu8v.gmgdl@evledraar.gmail.com> ``` Ævar Arnfjörð Bjarmason writes: > All of which (and more) would also be addressed in an obvious way by > just supporting --format as I suggested in > https://lore.kernel.org/git/211115.86o86lqe3c.gmgdl@evledraar.gmail.com/; > don't you think that's a better approach? That is what I would call over-engineering that I would rather not to have in low level plumbing. I am all for making _parsing_ the output from the tool easier by scripts; I am not interested in eliminating the _output_ by scripts. They should capture and format the pieces we output in any way they want. So, no, I do not think it is a better approach at all. ``` ## Ævar Arnfjörð Bjarmason, 2021-11-23 02:28 Subject: Re: [PATCH v3 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree" Message-ID: <211123.868rxfwqdw.gmgdl@evledraar.gmail.com> URL: https://gitlist.dev/e/211123.868rxfwqdw.gmgdl%40evledraar.gmail.com In-Reply-To: ``` On Mon, Nov 22 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason writes: > >> All of which (and more) would also be addressed in an obvious way by >> just supporting --format as I suggested in >> https://lore.kernel.org/git/211115.86o86lqe3c.gmgdl@evledraar.gmail.com/; >> don't you think that's a better approach? > > That is what I would call over-engineering that I would rather not > to have in low level plumbing. > > I am all for making _parsing_ the output from the tool easier by > scripts; I am not interested in eliminating the _output_ by scripts. > They should capture and format the pieces we output in any way they > want. > > So, no, I do not think it is a better approach at all. We've got --format for for-each-ref and family (also branch etc.), and for the "log" family. I'm not sure I understand what you're saying, do you think if we could go back and change it that the "FIELD NAMES" in git-for-each-ref (which is plumbing) would have been better done as --field-name=refname,objecttype,... etc? Having used it extensively it's been very hard to have the flexibility of formatting, e.g. to specify arbitrary delimiters. It also leaves the door open to teaching ls-tree etc. the %(if) syntax in the ref-filter, e.g. if you'd like to only print out certain data for certain object types or whatever. ``` ## Junio C Hamano, 2021-11-23 02:55 Subject: Re: [PATCH v3 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree" Message-ID: URL: https://gitlist.dev/e/xmqqlf1fa8cj.fsf%40gitster.g In-Reply-To: <211123.868rxfwqdw.gmgdl@evledraar.gmail.com> ``` Ævar Arnfjörð Bjarmason writes: >> That is what I would call over-engineering that I would rather not >> to have in low level plumbing. >> ... > We've got --format for for-each-ref and family (also branch etc.), and > for the "log" family. But I didn't comment on them. ls-tree is a lot lower-level plumbing where --format does not belong in my mind. ``` ## Junio C Hamano, 2021-11-23 03:35 Subject: Re: [PATCH v3 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree" Message-ID: URL: https://gitlist.dev/e/xmqqh7c3a6hs.fsf%40gitster.g In-Reply-To: ``` Junio C Hamano writes: > Ævar Arnfjörð Bjarmason writes: > >>> That is what I would call over-engineering that I would rather not >>> to have in low level plumbing. >>> ... >> We've got --format for for-each-ref and family (also branch etc.), and >> for the "log" family. > > But I didn't comment on them. ls-tree is a lot lower-level plumbing > where --format does not belong in my mind. There is a lot more practical reason why I'd prefer a less flexible and good enough interface. I can see, without coding it myself but from mere memory of how the code looked like, how such a "we allow you to choose which field to include, but you do not get to choose the order of fields or any other string in the output" can be done with minimum disruption to the existing code and without introducing a bug. On the other hand, I am fairly certain that anything more flexible than that will risk new bugs involved in any large shuffling of the code, which I am getting tired of. So there. ``` ## Teng Long, 2021-11-23 04:58 Subject: [PATCH v4 0/1] ls-tree.c: support `--oid-only` option Message-ID: URL: https://gitlist.dev/e/cover.1637642029.git.dyroneteng%40gmail.com In-Reply-To: ``` Thanks for the discussions on v3 (even I send a patch with wrong contents and the right cover). So I looked at them, and I think I have to send a new patch first, so this includes: 1. Commit message modifications (Junio C Hamano's advice) 2. Documentation modifications (Peter Baumann's advice) 3. To use the MODE enum name instead (Đoàn Trần Công Danh's advice) The other discussions I will reply today. Teng Long (1): ls-tree.c: support `--oid-only` option for "git-ls-tree" Documentation/git-ls-tree.txt | 18 ++++++++++++--- builtin/ls-tree.c | 30 +++++++++++++++++------- t/t3104-ls-tree-oid.sh | 43 +++++++++++++++++++++++++++++++++++ 3 files changed, 80 insertions(+), 11 deletions(-) create mode 100755 t/t3104-ls-tree-oid.sh Range-diff against v3: 1: 8b68568d6c ! 1: 63876dbeb7 ls-tree.c: support `--oid-only` option for "git-ls-tree" @@ Commit message Sometimes, we only want to get the objects from output of `ls-tree` and commands like `sed` or `cut` is usually used to intercept the - origin output to achieve this purpose in practical. + origin output to achieve this purpose in practice. - This commit supply an option names `--oid-only` to let `git ls-tree` - only print out the OID of the object. `--oid-only` and `--name-only` - are mutually exclusive in use. + This commit teach the "--oid-only" option to tell the command to + only show the object name, just like "--name-only" option tells the + command to only show the path component, for each entry. These two + options are mutually exclusive. - Reviewed-by: Jeff King - Reviewed-by: Ævar Arnfjörð Bjarmason - Reviewed-by: Đoàn Trần Công Danh Signed-off-by: Teng Long ## Documentation/git-ls-tree.txt ## -@@ Documentation/git-ls-tree.txt: SYNOPSIS +@@ Documentation/git-ls-tree.txt: git-ls-tree - List the contents of a tree object + SYNOPSIS -------- [verse] - 'git ls-tree' [-d] [-r] [-t] [-l] [-z] +-'git ls-tree' [-d] [-r] [-t] [-l] [-z] - [--name-only] [--name-status] [--full-name] [--full-tree] [--abbrev[=]] -+ [--name-only] [--name-status] [--oid-only] ++'git ls-tree' [-d] [-r] [-t] [-l] [-z] [-n] [-s] [-o] ++ [--name-only | --oid-only] ++ [--name-status | --oid-only] + [--full-name] [--full-tree] [--abbrev[=]] [...] DESCRIPTION @@ Documentation/git-ls-tree.txt: OPTIONS + \0 line termination on output and do not quote filenames. + See OUTPUT FORMAT below for more information. + ++-n:: --name-only:: - --name-status:: +---name-status:: List only filenames (instead of the "long" output), one per line. -- -+ Cannot be used with `--oid-only` together. ++ Cannot be combined with `--oid-only`. ++ ++-s:: ++--name-status:: ++ Consistent behavior with `--name-only`. ++ ++-o:: +--oid-only:: -+ List only OIDs of the objects, one per line. Cannot be used with -+ `--name-only` or `--name-status` together. ++ List only names of the objects, one per line. Cannot be combined ++ with `--name-only` or `--name-status`. + --abbrev[=]:: Instead of showing the full 40-byte hexadecimal object - lines, show the shortest prefix that is at least '' ## builtin/ls-tree.c ## @@ builtin/ls-tree.c: static int line_termination = '\n'; @@ builtin/ls-tree.c: static int show_tree(const struct object_id *oid, struct strb return 0; - if (!(ls_options & LS_NAME_ONLY)) { -+ if (cmdmode == 2) { ++ if (cmdmode == MODE_OID_ONLY) { + printf("%s\n", find_unique_abbrev(oid, abbrev)); + return 0; + } + -+ if (cmdmode == 0) { ++ if (cmdmode == MODE_UNSPECIFIED) { if (ls_options & LS_SHOW_SIZE) { char size_text[24]; if (!strcmp(type, blob_type)) { @@ builtin/ls-tree.c: int cmd_ls_tree(int argc, const char **argv, const char *pref - LS_NAME_ONLY), - OPT_BIT(0, "name-status", &ls_options, N_("list only filenames"), - LS_NAME_ONLY), -+ OPT_CMDMODE('n', "name-only", &cmdmode, N_("list only filenames"), MODE_NAME_ONLY), -+ OPT_CMDMODE('s', "name-status", &cmdmode, N_("list only filenames"), MODE_NAME_ONLY), -+ OPT_CMDMODE('o', "oid-only", &cmdmode, N_("list only oids"), MODE_OID_ONLY), ++ OPT_CMDMODE('n', "name-only", &cmdmode, ++ N_("list only filenames"), MODE_NAME_ONLY), ++ OPT_CMDMODE('s', "name-status", &cmdmode, ++ N_("list only filenames"), MODE_NAME_ONLY), ++ OPT_CMDMODE('o', "oid-only", &cmdmode, ++ N_("list only oids"), MODE_OID_ONLY), OPT_SET_INT(0, "full-name", &chomp_prefix, N_("use full path names"), 0), OPT_BOOL(0, "full-tree", &full_tree, @@ t/t3104-ls-tree-oid.sh (new) +. ./test-lib.sh + +test_expect_success 'setup' ' -+ echo 111 >1.txt && -+ echo 222 >2.txt && -+ mkdir -p path0/a/b/c && -+ echo 333 >path0/a/b/c/3.txt && ++ test_commit A && ++ test_commit B && ++ mkdir -p C && ++ test_commit C/D.txt && + find *.txt path* \( -type f -o -type l \) -print | + xargs git update-index --add && + tree=$(git write-tree) && @@ t/t3104-ls-tree-oid.sh (new) + +test_expect_success 'usage: --oid-only' ' + git ls-tree --oid-only $tree >current && -+ git ls-tree $tree | awk "{print \$3}" >expected && ++ git ls-tree $tree >result && ++ cut -f1 result | cut -d " " -f3 >expected && + test_cmp current expected +' + +test_expect_success 'usage: --oid-only with -r' ' + git ls-tree --oid-only -r $tree >current && -+ git ls-tree -r $tree | awk "{print \$3}" >expected && ++ git ls-tree -r $tree >result && ++ cut -f1 result | cut -d " " -f3 >expected && + test_cmp current expected +' + +test_expect_success 'usage: --oid-only with --abbrev' ' + git ls-tree --oid-only --abbrev=6 $tree >current && -+ git ls-tree --abbrev=6 $tree | awk "{print \$3}" > expected && ++ git ls-tree --abbrev=6 $tree >result && ++ cut -f1 result | cut -d " " -f3 >expected && + test_cmp current expected +' + -+test_expect_failure 'usage: incompatible options: --name-only with --oid-only' ' -+ test_incompatible_usage git ls-tree --oid-only --name-only ++test_expect_success 'usage: incompatible options: --name-only with --oid-only' ' ++ test_expect_code 129 git ls-tree --oid-only --name-only +' + +test_done -- 2.33.1.10.g75523f744f.dirty ``` ## Teng Long, 2021-11-23 04:58 Subject: [PATCH v4 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree" Message-ID: <63876dbeb7f880c0001649b49d5265b744c42765.1637642029.git.dyroneteng@gmail.com> URL: https://gitlist.dev/e/63876dbeb7f880c0001649b49d5265b744c42765.1637642029.git.dyroneteng%40gmail.com In-Reply-To: ``` Sometimes, we only want to get the objects from output of `ls-tree` and commands like `sed` or `cut` is usually used to intercept the origin output to achieve this purpose in practice. This commit teach the "--oid-only" option to tell the command to only show the object name, just like "--name-only" option tells the command to only show the path component, for each entry. These two options are mutually exclusive. Signed-off-by: Teng Long --- Documentation/git-ls-tree.txt | 18 ++++++++++++--- builtin/ls-tree.c | 30 +++++++++++++++++------- t/t3104-ls-tree-oid.sh | 43 +++++++++++++++++++++++++++++++++++ 3 files changed, 80 insertions(+), 11 deletions(-) create mode 100755 t/t3104-ls-tree-oid.sh diff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt index db02d6d79a..fd2a871ca5 100644 --- a/Documentation/git-ls-tree.txt +++ b/Documentation/git-ls-tree.txt @@ -9,8 +9,10 @@ git-ls-tree - List the contents of a tree object SYNOPSIS -------- [verse] -'git ls-tree' [-d] [-r] [-t] [-l] [-z] - [--name-only] [--name-status] [--full-name] [--full-tree] [--abbrev[=]] +'git ls-tree' [-d] [-r] [-t] [-l] [-z] [-n] [-s] [-o] + [--name-only | --oid-only] + [--name-status | --oid-only] + [--full-name] [--full-tree] [--abbrev[=]] [...] DESCRIPTION @@ -56,9 +58,19 @@ OPTIONS \0 line termination on output and do not quote filenames. See OUTPUT FORMAT below for more information. +-n:: --name-only:: ---name-status:: List only filenames (instead of the "long" output), one per line. + Cannot be combined with `--oid-only`. + +-s:: +--name-status:: + Consistent behavior with `--name-only`. + +-o:: +--oid-only:: + List only names of the objects, one per line. Cannot be combined + with `--name-only` or `--name-status`. --abbrev[=]:: Instead of showing the full 40-byte hexadecimal object diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index 3a442631c7..0c2153a5ad 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c @@ -18,19 +18,26 @@ static int line_termination = '\n'; #define LS_RECURSIVE 1 #define LS_TREE_ONLY 2 #define LS_SHOW_TREES 4 -#define LS_NAME_ONLY 8 -#define LS_SHOW_SIZE 16 +#define LS_SHOW_SIZE 8 static int abbrev; static int ls_options; static struct pathspec pathspec; static int chomp_prefix; static const char *ls_tree_prefix; -static const char * const ls_tree_usage[] = { +static const char * const ls_tree_usage[] = { N_("git ls-tree [] [...]"), NULL }; +enum { + MODE_UNSPECIFIED = 0, + MODE_NAME_ONLY, + MODE_OID_ONLY +}; + +static int cmdmode = MODE_UNSPECIFIED; + static int show_recursive(const char *base, int baselen, const char *pathname) { int i; @@ -90,7 +97,12 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, else if (ls_options & LS_TREE_ONLY) return 0; - if (!(ls_options & LS_NAME_ONLY)) { + if (cmdmode == MODE_OID_ONLY) { + printf("%s\n", find_unique_abbrev(oid, abbrev)); + return 0; + } + + if (cmdmode == MODE_UNSPECIFIED) { if (ls_options & LS_SHOW_SIZE) { char size_text[24]; if (!strcmp(type, blob_type)) { @@ -135,10 +147,12 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) N_("terminate entries with NUL byte"), 0), OPT_BIT('l', "long", &ls_options, N_("include object size"), LS_SHOW_SIZE), - OPT_BIT(0, "name-only", &ls_options, N_("list only filenames"), - LS_NAME_ONLY), - OPT_BIT(0, "name-status", &ls_options, N_("list only filenames"), - LS_NAME_ONLY), + OPT_CMDMODE('n', "name-only", &cmdmode, + N_("list only filenames"), MODE_NAME_ONLY), + OPT_CMDMODE('s', "name-status", &cmdmode, + N_("list only filenames"), MODE_NAME_ONLY), + OPT_CMDMODE('o', "oid-only", &cmdmode, + N_("list only oids"), MODE_OID_ONLY), OPT_SET_INT(0, "full-name", &chomp_prefix, N_("use full path names"), 0), OPT_BOOL(0, "full-tree", &full_tree, diff --git a/t/t3104-ls-tree-oid.sh b/t/t3104-ls-tree-oid.sh new file mode 100755 index 0000000000..2d349f6e46 --- /dev/null +++ b/t/t3104-ls-tree-oid.sh @@ -0,0 +1,43 @@ +#!/bin/sh + +test_description='git ls-tree oids handling.' + +. ./test-lib.sh + +test_expect_success 'setup' ' + test_commit A && + test_commit B && + mkdir -p C && + test_commit C/D.txt && + find *.txt path* \( -type f -o -type l \) -print | + xargs git update-index --add && + tree=$(git write-tree) && + echo $tree +' + +test_expect_success 'usage: --oid-only' ' + git ls-tree --oid-only $tree >current && + git ls-tree $tree >result && + cut -f1 result | cut -d " " -f3 >expected && + test_cmp current expected +' + +test_expect_success 'usage: --oid-only with -r' ' + git ls-tree --oid-only -r $tree >current && + git ls-tree -r $tree >result && + cut -f1 result | cut -d " " -f3 >expected && + test_cmp current expected +' + +test_expect_success 'usage: --oid-only with --abbrev' ' + git ls-tree --oid-only --abbrev=6 $tree >current && + git ls-tree --abbrev=6 $tree >result && + cut -f1 result | cut -d " " -f3 >expected && + test_cmp current expected +' + +test_expect_success 'usage: incompatible options: --name-only with --oid-only' ' + test_expect_code 129 git ls-tree --oid-only --name-only +' + +test_done -- 2.33.1.10.g75523f744f.dirty ``` ## Ævar Arnfjörð Bjarmason, 2021-11-23 11:04 Subject: Re: [PATCH v3 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree" Message-ID: <211123.86mtlvumps.gmgdl@evledraar.gmail.com> URL: https://gitlist.dev/e/211123.86mtlvumps.gmgdl%40evledraar.gmail.com In-Reply-To: ``` On Mon, Nov 22 2021, Junio C Hamano wrote: > Junio C Hamano writes: > >> Ævar Arnfjörð Bjarmason writes: >> >>>> That is what I would call over-engineering that I would rather not >>>> to have in low level plumbing. >>>> ... >>> We've got --format for for-each-ref and family (also branch etc.), and >>> for the "log" family. >> >> But I didn't comment on them. ls-tree is a lot lower-level plumbing >> where --format does not belong in my mind. Yes, you're just talking about ls-tree here. I'm just trying to understand what you meant with: I am not interested in eliminating the _output_ by scripts. They should capture and format the pieces we output in any way they want. Which reads to me like "we provide the data, you pretty-fy it". In this case the proposed feature doesn't need a patch to git, but can also be done as: git ls-tree HEAD | cut -d$'\t' -f1 | cut -d ' ' -f3 I think it's useful. I'm just trying to understand what you think about such plumbing output. > There is a lot more practical reason why I'd prefer a less flexible > and good enough interface. > > I can see, without coding it myself but from mere memory of how the > code looked like, how such a "we allow you to choose which field to > include, but you do not get to choose the order of fields or any > other string in the output" can be done with minimum disruption to > the existing code and without introducing a bug. On the other hand, > I am fairly certain that anything more flexible than that will risk > new bugs involved in any large shuffling of the code, which I am > getting tired of. To be clear, I wasn't talking about running with the WIP patch I had in <211115.86o86lqe3c.gmgdl@evledraar.gmail.com> here, but that the interface wolud leave the door open to it. So something like the below. This works to do what --oid-only does without adding that switch, instead we add it tou our list of 4 supported hardcoded formats, all of which map to one of the MODE_* flags. We could just document that we support that limited list for now, and that we might add more in the future. So it's just a way of adding a new MODE_* without supporting an ever growing list of flags, --oid-only, --objectmode-only, --objectsize-only etc. Then if we'd ever want to generalize this in the future we can pick up someting like my WIP patch and we'd have support for any arbitrary format. diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index 1e4a82e669a..e1e746ae02a 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c @@ -30,10 +30,11 @@ static const char * const ls_tree_usage[] = { NULL }; -enum { +enum ls_tree_cmdmode { MODE_UNSPECIFIED = 0, MODE_NAME_ONLY, - MODE_OID_ONLY + MODE_OID_ONLY, + MODE_LONG, }; static int cmdmode = MODE_UNSPECIFIED; @@ -131,11 +132,22 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, return retval; } +static struct { + enum ls_tree_cmdmode cmdmode; + const char *fmt; +} allowed_formats[] = { + { MODE_UNSPECIFIED, "%(objectmode) %(objecttype) %(objectname)%09%(path)" }, + { MODE_NAME_ONLY, "%(path)" }, + { MODE_OID_ONLY, "%(objectname)" }, + { MODE_LONG, "%(objectmode) %(objecttype) %(objectsize) %(objectname)%09%(path)" }, +}; + 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 *format = NULL; const struct option ls_tree_options[] = { OPT_BIT('d', NULL, &ls_options, N_("only show trees"), LS_TREE_ONLY), @@ -149,7 +161,8 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) LS_SHOW_SIZE), OPT_CMDMODE('n', "name-only", &cmdmode, N_("list only filenames"), MODE_NAME_ONLY), OPT_CMDMODE('s', "name-status", &cmdmode, N_("list only filenames"), MODE_NAME_ONLY), - OPT_CMDMODE('o', "oid-only", &cmdmode, N_("list only oids"), MODE_OID_ONLY), + OPT_STRING(0 , "format", &format, N_("format"), + N_("(limited) format to use for the output")), OPT_SET_INT(0, "full-name", &chomp_prefix, N_("use full path names"), 0), OPT_BOOL(0, "full-tree", &full_tree, @@ -170,6 +183,22 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) ls_tree_prefix = prefix = NULL; chomp_prefix = 0; } + + if (format && cmdmode) + die(_("--format and --name-only, --long etc. are incompatible")); + if (format) { + size_t i; + + for (i = 0; i <= ARRAY_SIZE(allowed_formats); i++) { + if (i == ARRAY_SIZE(allowed_formats)) + die(_("your --format=%s is not on the whitelist of supported formats"), format); + if (!strcmp(format, allowed_formats[i].fmt)) { + cmdmode = allowed_formats[i].cmdmode; + break; + } + } + } + /* -d -r should imply -t, but -d by itself should not have to. */ if ( (LS_TREE_ONLY|LS_RECURSIVE) == ((LS_TREE_ONLY|LS_RECURSIVE) & ls_options)) ``` ## Junio C Hamano, 2021-11-23 22:32 Subject: Re: [PATCH v4 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree" Message-ID: URL: https://gitlist.dev/e/xmqqmtlu7bb0.fsf%40gitster.g In-Reply-To: <63876dbeb7f880c0001649b49d5265b744c42765.1637642029.git.dyroneteng@gmail.com> ``` Teng Long writes: > Sometimes, we only want to get the objects from output of `ls-tree` > and commands like `sed` or `cut` is usually used to intercept the > origin output to achieve this purpose in practice. I can guess what "intercept the origin" wants to say, but it does not roll off the tongue very well. > This commit teach the "--oid-only" option to tell the command to > only show the object name, just like "--name-only" option tells the > command to only show the path component, for each entry. These two > options are mutually exclusive. cf. Documentation/SubmittingPatches[[imperative-mood]] Perhaps like We usually pipe the output from `git ls-files` to tools like `sed` or `cut` when we only want to extract some fields. When we want only the pathname component, we can pass `--name-only` option to omit such a pipeline, but there are no options for extracting other fields. Teach the "--oid-only" option to the command to only show the object name. This option cannot be used together with "--name-only" or "--long". This does not work well with "--long", right? > -'git ls-tree' [-d] [-r] [-t] [-l] [-z] > - [--name-only] [--name-status] [--full-name] [--full-tree] [--abbrev[=]] > +'git ls-tree' [-d] [-r] [-t] [-l] [-z] [-n] [-s] [-o] Where does the addition of -n/-s/-o fit in the theme of the patch? This being a plumbing command, an existing option with long name does not deserve a shorthand (and --oid-only should start with long name only, too). If we were to introduce -n as yet another synonym for --name-only and --name-status (which we would not), the right way to spell it in the synopsis section would be: [-n | --name-only | --name-status] I would think, but this advise is only for your next topic. We are not going to add such a synonym. > + [--name-only | --oid-only] > + [--name-status | --oid-only] This looks very iffy. Has this been proof-read before sending out? > + [--full-name] [--full-tree] [--abbrev[=]] > [...] > > DESCRIPTION > @@ -56,9 +58,19 @@ OPTIONS > \0 line termination on output and do not quote filenames. > See OUTPUT FORMAT below for more information. > > +-n:: > --name-only:: > ---name-status:: > List only filenames (instead of the "long" output), one per line. > + Cannot be combined with `--oid-only`. > + > +-s:: > +--name-status:: > + Consistent behavior with `--name-only`. Usually we say A is "Consistent" with B when A and B are different but are moral equivalent in their respective contexts. These are identical, there is no difference. Lose all of the above change, except for "Cannot be combined with". The original is just fine. > +-o:: > +--oid-only:: > + List only names of the objects, one per line. Cannot be combined > + with `--name-only` or `--name-status`. Or "--long"? Does this work with it? ALso, lose "-o". > diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c > index 3a442631c7..0c2153a5ad 100644 > --- a/builtin/ls-tree.c > +++ b/builtin/ls-tree.c > @@ -18,19 +18,26 @@ static int line_termination = '\n'; > #define LS_RECURSIVE 1 > #define LS_TREE_ONLY 2 > #define LS_SHOW_TREES 4 > -#define LS_NAME_ONLY 8 > -#define LS_SHOW_SIZE 16 > +#define LS_SHOW_SIZE 8 > static int abbrev; > static int ls_options; > static struct pathspec pathspec; > static int chomp_prefix; > static const char *ls_tree_prefix; > > -static const char * const ls_tree_usage[] = { > +static const char * const ls_tree_usage[] = { > N_("git ls-tree [] [...]"), > NULL > }; > > +enum { > + MODE_UNSPECIFIED = 0, > + MODE_NAME_ONLY, > + MODE_OID_ONLY > +}; I suspect that "--long" would be part of this, if we were to go this route. OPT_CMDMODE() is a handy way to ensure "--name-only", "--oid-only", and "--long" are not given together, but it may be overkill to make only two or three options mutually exclusive. In any case, once we pass the parsing part, the code should translate the option into a bitmask that specifies which among , , , , and fields are shown. It will result in cleaner code in show_tree() if it uses that set of fields to decide what is shown and how without looking at the cmdmode enum. ``` ## Teng Long, 2021-12-06 07:52 Subject: Re: [PATCH v4 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree" Message-ID: <20211206075223.56031-1-dyroneteng@gmail.com> URL: https://gitlist.dev/e/20211206075223.56031-1-dyroneteng%40gmail.com In-Reply-To: ``` On Tue, 23 Nov 2021 14:32:35 -0800, Junio C Hamano wrote: > I can guess what "intercept the origin" wants to say, but it does >not roll off the tongue very well. > > cf. Documentation/SubmittingPatches[[imperative-mood]] Will learn and borrow your sentences and be shown in next patch :) > This does not work well with "--long", right? I think so. Peff pointed out this is arguably a bug before because `git ls-tree --long --name-only` do not really make sense (column of the object size is not shown in result). > Usually we say A is "Consistent" with B when A and B are different > but are moral equivalent in their respective contexts. These are > identical, there is no difference. Clearly understood now. > Lose all of the above change, except for "Cannot be combined with". > The original is just fine. Will. > Or "--long"? Does this work with it? As I understand the disscussed context so far, the "--long", "--name-only" and "--oid-only" they should be mutually exclusive with each other. > I suspect that "--long" would be part of this, if we were to go this > route. Agree. > OPT_CMDMODE() is a handy way to ensure "--name-only", "--oid-only", > and "--long" are not given together, but it may be overkill to make > only two or three options mutually exclusive. > > In any case, once we pass the parsing part, the code should > translate the option into a bitmask that specifies which among > , , , , and fields are > shown. It will result in cleaner code in show_tree() if it uses > that set of fields to decide what is shown and how without looking > at the cmdmode enum. Make sense. Thanks. ``` ## Teng Long, 2021-12-08 02:08 Subject: [PATCH v5 0/1] support `--object-only` option for "git-ls-tree" Message-ID: URL: https://gitlist.dev/e/cover.1638891420.git.dyroneteng%40gmail.com In-Reply-To: ``` Diffs from patch v4: * Change `--oid-only` to `--object-only`. Word "oid" may not be easily understood for users. * The commit message was modified in terms of Junio's advice. * Use "OPT_CMDMODE()" to make `--name-only`, `--object-only` and `--long` mutually exclusive with each other. * After options been parsed, translate options to bitmask, then use cleaner bitwise to determine which fields will be shown. * Add tests for mutually exclusive options. * Documentation modifications about the change of option name. Thanks. Teng Long (1): ls-tree.c: support `--object-only` option for "git-ls-tree" Documentation/git-ls-tree.txt | 7 +- builtin/ls-tree.c | 125 ++++++++++++++++++++++++---------- t/t3103-ls-tree-misc.sh | 8 +++ t/t3104-ls-tree-oid.sh | 51 ++++++++++++++ 4 files changed, 154 insertions(+), 37 deletions(-) create mode 100755 t/t3104-ls-tree-oid.sh Range-diff against v4: -: ---------- > 1: 38d55a878c ls-tree.c: support `--object-only` option for "git-ls-tree" -- 2.33.1.10.gd2a07a0ec5.dirty ``` ## Teng Long, 2021-12-08 02:08 Subject: [PATCH v5 1/1] ls-tree.c: support `--object-only` option for "git-ls-tree" Message-ID: <38d55a878c51104f0a5337481fdcd5c8818660ac.1638891420.git.dyroneteng@gmail.com> URL: https://gitlist.dev/e/38d55a878c51104f0a5337481fdcd5c8818660ac.1638891420.git.dyroneteng%40gmail.com In-Reply-To: ``` We usually pipe the output from `git ls-trees` to tools like `sed` or `cut` when we only want to extract some fields. When we want only the pathname component, we can pass `--name-only` option to omit such a pipeline, but there are no options for extracting other fields. Teach the "--object-only" option to the command to only show the object name. This option cannot be used together with "--name-only" or "--long" (mutually exclusive). Signed-off-by: Teng Long --- Documentation/git-ls-tree.txt | 7 +- builtin/ls-tree.c | 125 ++++++++++++++++++++++++---------- t/t3103-ls-tree-misc.sh | 8 +++ t/t3104-ls-tree-oid.sh | 51 ++++++++++++++ 4 files changed, 154 insertions(+), 37 deletions(-) create mode 100755 t/t3104-ls-tree-oid.sh diff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt index db02d6d79a..729370f235 100644 --- a/Documentation/git-ls-tree.txt +++ b/Documentation/git-ls-tree.txt @@ -10,7 +10,7 @@ SYNOPSIS -------- [verse] 'git ls-tree' [-d] [-r] [-t] [-l] [-z] - [--name-only] [--name-status] [--full-name] [--full-tree] [--abbrev[=]] + [--name-only] [--name-status] [--object-only] [--full-name] [--full-tree] [--abbrev[=]] [...] DESCRIPTION @@ -59,6 +59,11 @@ OPTIONS --name-only:: --name-status:: List only filenames (instead of the "long" output), one per line. + Cannot be combined with `--object-only`. + +--object-only:: + List only names of the objects, one per line. Cannot be combined + with `--name-only` or `--name-status`. --abbrev[=]:: Instead of showing the full 40-byte hexadecimal object diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index 3a442631c7..beaa8bf13b 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c @@ -16,21 +16,38 @@ static int line_termination = '\n'; #define LS_RECURSIVE 1 -#define LS_TREE_ONLY 2 -#define LS_SHOW_TREES 4 -#define LS_NAME_ONLY 8 -#define LS_SHOW_SIZE 16 +#define LS_TREE_ONLY 1 << 1 +#define LS_SHOW_TREES 1 << 2 +#define LS_NAME_ONLY 1 << 3 +#define LS_SHOW_SIZE 1 << 4 +#define LS_OBJECT_ONLY 1 << 5 static int abbrev; static int ls_options; static struct pathspec pathspec; static int chomp_prefix; static const char *ls_tree_prefix; +static unsigned int shown_bits = 0; +#define SHOW_DEFAULT 29 /* 11101 size is not shown to output by default */ +#define SHOW_MODE 1 << 4 +#define SHOW_TYPE 1 << 3 +#define SHOW_OBJECT_NAME 1 << 2 +#define SHOW_SIZE 1 << 1 +#define SHOW_FILE_NAME 1 static const char * const ls_tree_usage[] = { N_("git ls-tree [] [...]"), NULL }; +enum { + MODE_UNSPECIFIED = 0, + MODE_NAME_ONLY, + MODE_OBJECT_ONLY, + MODE_LONG +}; + +static int cmdmode = MODE_UNSPECIFIED; + static int show_recursive(const char *base, int baselen, const char *pathname) { int i; @@ -66,6 +83,7 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, { int retval = 0; int baselen; + int follow = 0; const char *type = blob_type; if (S_ISGITLINK(mode)) { @@ -74,8 +92,8 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, * * Something similar to this incomplete example: * - if (show_subprojects(base, baselen, pathname)) - retval = READ_TREE_RECURSIVE; + * if (show_subprojects(base, baselen, pathname)) + * retval = READ_TREE_RECURSIVE; * */ type = commit_type; @@ -90,35 +108,67 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, else if (ls_options & LS_TREE_ONLY) return 0; - if (!(ls_options & LS_NAME_ONLY)) { - if (ls_options & LS_SHOW_SIZE) { - char size_text[24]; - if (!strcmp(type, blob_type)) { - unsigned long size; - if (oid_object_info(the_repository, oid, &size) == OBJ_BAD) - xsnprintf(size_text, sizeof(size_text), - "BAD"); - else - xsnprintf(size_text, sizeof(size_text), - "%"PRIuMAX, (uintmax_t)size); - } else - xsnprintf(size_text, sizeof(size_text), "-"); - printf("%06o %s %s %7s\t", mode, type, - find_unique_abbrev(oid, abbrev), - size_text); + if (shown_bits & SHOW_MODE) { + printf("%06o",mode); + follow = 1; + } + if (shown_bits & SHOW_TYPE) { + printf("%s%s", follow == 1 ? " " : "", type); + follow = 1; + } + if (shown_bits & SHOW_OBJECT_NAME) { + printf("%s%s", follow == 1 ? " " : "", + find_unique_abbrev(oid, abbrev)); + if (!(shown_bits ^ SHOW_OBJECT_NAME)) + printf("%c", line_termination); + follow = 1; + } + if (shown_bits & SHOW_SIZE) { + char size_text[24]; + if (!strcmp(type, blob_type)) { + unsigned long size; + if (oid_object_info(the_repository, oid, &size) == OBJ_BAD) + xsnprintf(size_text, sizeof(size_text), "BAD"); + else + xsnprintf(size_text, sizeof(size_text), + "%"PRIuMAX, (uintmax_t)size); } else - printf("%06o %s %s\t", mode, type, - find_unique_abbrev(oid, abbrev)); + xsnprintf(size_text, sizeof(size_text), "-"); + printf("%s%7s", follow == 1 ? " " : "", size_text); + follow = 1; + } + if (shown_bits & SHOW_FILE_NAME) { + if (follow) + printf("\t"); + baselen = base->len; + strbuf_addstr(base, pathname); + write_name_quoted_relative(base->buf, + chomp_prefix ? ls_tree_prefix : NULL, + stdout, line_termination); + strbuf_setlen(base, baselen); } - baselen = base->len; - strbuf_addstr(base, pathname); - write_name_quoted_relative(base->buf, - chomp_prefix ? ls_tree_prefix : NULL, - stdout, line_termination); - strbuf_setlen(base, baselen); return retval; } +static int parse_shown_fields(void) +{ + if (cmdmode == MODE_NAME_ONLY) { + shown_bits = SHOW_FILE_NAME; + return 0; + } + if (cmdmode == MODE_OBJECT_ONLY) { + shown_bits = SHOW_OBJECT_NAME; + return 0; + } + if (!ls_options || (ls_options & LS_RECURSIVE) + || (ls_options & LS_SHOW_TREES) + || (ls_options & LS_TREE_ONLY)) + shown_bits = SHOW_DEFAULT; + if (cmdmode == MODE_LONG) + shown_bits = SHOW_DEFAULT | SHOW_SIZE; + return 1; +} + int cmd_ls_tree(int argc, const char **argv, const char *prefix) { struct object_id oid; @@ -133,12 +183,14 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) LS_SHOW_TREES), OPT_SET_INT('z', NULL, &line_termination, N_("terminate entries with NUL byte"), 0), - OPT_BIT('l', "long", &ls_options, N_("include object size"), - LS_SHOW_SIZE), - OPT_BIT(0, "name-only", &ls_options, N_("list only filenames"), - LS_NAME_ONLY), - OPT_BIT(0, "name-status", &ls_options, N_("list only filenames"), - LS_NAME_ONLY), + OPT_CMDMODE('l', "long", &cmdmode, N_("include object size"), + MODE_LONG), + OPT_CMDMODE(0, "name-only", &cmdmode, N_("list only filenames"), + MODE_NAME_ONLY), + OPT_CMDMODE(0, "name-status", &cmdmode, N_("list only filenames"), + MODE_NAME_ONLY), + OPT_CMDMODE(0, "object-only", &cmdmode, N_("list only objects"), + MODE_OBJECT_ONLY), OPT_SET_INT(0, "full-name", &chomp_prefix, N_("use full path names"), 0), OPT_BOOL(0, "full-tree", &full_tree, @@ -169,6 +221,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) if (get_oid(argv[0], &oid)) die("Not a valid object name %s", argv[0]); + parse_shown_fields(); /* * show_recursive() rolls its own matching code and is * generally ignorant of 'struct pathspec'. The magic mask diff --git a/t/t3103-ls-tree-misc.sh b/t/t3103-ls-tree-misc.sh index 14520913af..75e38b0a51 100755 --- a/t/t3103-ls-tree-misc.sh +++ b/t/t3103-ls-tree-misc.sh @@ -22,4 +22,12 @@ test_expect_success 'ls-tree fails with non-zero exit code on broken tree' ' test_must_fail git ls-tree -r HEAD ' +test_expect_success 'usage: incompatible options: --name-status with --long' ' + test_expect_code 129 git ls-tree --long --name-status +' + +test_expect_success 'usage: incompatible options: --name-only with --long' ' + test_expect_code 129 git ls-tree --long --name-only +' + test_done diff --git a/t/t3104-ls-tree-oid.sh b/t/t3104-ls-tree-oid.sh new file mode 100755 index 0000000000..81304e7b13 --- /dev/null +++ b/t/t3104-ls-tree-oid.sh @@ -0,0 +1,51 @@ +#!/bin/sh + +test_description='git ls-tree objects handling.' + +. ./test-lib.sh + +test_expect_success 'setup' ' + test_commit A && + test_commit B && + mkdir -p C && + test_commit C/D.txt && + find *.txt path* \( -type f -o -type l \) -print | + xargs git update-index --add && + tree=$(git write-tree) && + echo $tree +' + +test_expect_success 'usage: --object-only' ' + git ls-tree --object-only $tree >current && + git ls-tree $tree >result && + cut -f1 result | cut -d " " -f3 >expected && + test_cmp current expected +' + +test_expect_success 'usage: --object-only with -r' ' + git ls-tree --object-only -r $tree >current && + git ls-tree -r $tree >result && + cut -f1 result | cut -d " " -f3 >expected && + test_cmp current expected +' + +test_expect_success 'usage: --object-only with --abbrev' ' + git ls-tree --object-only --abbrev=6 $tree >current && + git ls-tree --abbrev=6 $tree >result && + cut -f1 result | cut -d " " -f3 >expected && + test_cmp current expected +' + +test_expect_success 'usage: incompatible options: --name-only with --object-only' ' + test_expect_code 129 git ls-tree --object-only --name-only +' + +test_expect_success 'usage: incompatible options: --name-status with --object-only' ' + test_expect_code 129 git ls-tree --object-only --name-status +' + +test_expect_success 'usage: incompatible options: --long with --object-only' ' + test_expect_code 129 git ls-tree --object-only --long +' + +test_done -- 2.33.1.10.gd2a07a0ec5.dirty ``` ## Junio C Hamano, 2021-12-15 19:25 Subject: Re: [PATCH v5 1/1] ls-tree.c: support `--object-only` option for "git-ls-tree" Message-ID: URL: https://gitlist.dev/e/xmqqk0g5656r.fsf%40gitster.g In-Reply-To: <38d55a878c51104f0a5337481fdcd5c8818660ac.1638891420.git.dyroneteng@gmail.com> ``` Teng Long writes: > We usually pipe the output from `git ls-trees` to tools like > `sed` or `cut` when we only want to extract some fields. > > When we want only the pathname component, we can pass > `--name-only` option to omit such a pipeline, but there are no > options for extracting other fields. > > Teach the "--object-only" option to the command to only show the > object name. This option cannot be used together with > "--name-only" or "--long" (mutually exclusive). I notice that this changed from --oid to --object and I agree that it would probably be more friendly to end users. In fact, this $ sed -ne '/^SYNOPSIS/,/^DESCRIPTION/p' Documentation/git-*.txt | grep -e -oid did not find any hits. > diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c > index 3a442631c7..beaa8bf13b 100644 > --- a/builtin/ls-tree.c > +++ b/builtin/ls-tree.c > @@ -16,21 +16,38 @@ > > static int line_termination = '\n'; > #define LS_RECURSIVE 1 > -#define LS_TREE_ONLY 2 > -#define LS_SHOW_TREES 4 > -#define LS_NAME_ONLY 8 > -#define LS_SHOW_SIZE 16 > +#define LS_TREE_ONLY 1 << 1 > +#define LS_SHOW_TREES 1 << 2 > +#define LS_NAME_ONLY 1 << 3 > +#define LS_SHOW_SIZE 1 << 4 > +#define LS_OBJECT_ONLY 1 << 5 It usually is a good idea to enclose these in () so that they are safe to use in any context in a statement. Luckily, bitwise-or and bitwise-and, which are the most likely candidates for these symbols to be used with, bind looser than left-shift, so something like if ((LS_TREE_ONLY | LS_SHOW_TREES) & opt) ... do this ... is safe either way, but (LS_TREE_ONLY + LS_SHOW_TREES) would have different value with and without () around (1 << N). > +static unsigned int shown_bits = 0; Style: we do not initialize statics explicitly to zero. > +#define SHOW_DEFAULT 29 /* 11101 size is not shown to output by default */ > +#define SHOW_MODE 1 << 4 > +#define SHOW_TYPE 1 << 3 > +#define SHOW_OBJECT_NAME 1 << 2 > +#define SHOW_SIZE 1 << 1 > +#define SHOW_FILE_NAME 1 Likewise. It is a bit curious to see these listed in decreasing order, though. > static const char * const ls_tree_usage[] = { > N_("git ls-tree [] [...]"), > NULL > }; > > +enum { > + MODE_UNSPECIFIED = 0, > + MODE_NAME_ONLY, > + MODE_OBJECT_ONLY, > + MODE_LONG It is a good idea to leave a comma even after the last element, _unless_ there is a strong reason why the element that currently is at the last MUST stay to be last when new elements are added to the enum. That way, a future patch that adds a new element can add it to the list with a patch noise with fewer lines. > +}; > + > +static int cmdmode = MODE_UNSPECIFIED; This is also initializing a static variable to zero, and arguments can be made either way: (A) unspecified is set to zero in enum definition exactly so that we can use zero to signal the variable is unspecified, so an explicit zero initialization here goes against the spirit of choosing 0 as MODE_UNSPECIFIED; or (B) enum definition can be scrambled in future changes to use something other than zero for MODE_UNSPECIFIED, and explicitly writing it like this is more future-proof. I am OK with the way it is written (i.e. (B)). > @@ -66,6 +83,7 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, > { > int retval = 0; > int baselen; > + int follow = 0; A better name, anybody? This bit is to keep track of the fact that we made _some_ output already so any further output needs an inter-field space before writing what it wants to write out. > const char *type = blob_type; > > if (S_ISGITLINK(mode)) { > @@ -74,8 +92,8 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, > * > * Something similar to this incomplete example: > * > - if (show_subprojects(base, baselen, pathname)) > - retval = READ_TREE_RECURSIVE; > + * if (show_subprojects(base, baselen, pathname)) > + * retval = READ_TREE_RECURSIVE; > * > */ Nice ;-) > type = commit_type; > @@ -90,35 +108,67 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, > else if (ls_options & LS_TREE_ONLY) > return 0; > > - if (!(ls_options & LS_NAME_ONLY)) { > - if (ls_options & LS_SHOW_SIZE) { > - char size_text[24]; > - if (!strcmp(type, blob_type)) { > - unsigned long size; > - if (oid_object_info(the_repository, oid, &size) == OBJ_BAD) > - xsnprintf(size_text, sizeof(size_text), > - "BAD"); > - else > - xsnprintf(size_text, sizeof(size_text), > - "%"PRIuMAX, (uintmax_t)size); > - } else > - xsnprintf(size_text, sizeof(size_text), "-"); > - printf("%06o %s %s %7s\t", mode, type, > - find_unique_abbrev(oid, abbrev), > - size_text); > + if (shown_bits & SHOW_MODE) { > + printf("%06o",mode); SP before ','. > + follow = 1; > + } > + if (shown_bits & SHOW_TYPE) { > + printf("%s%s", follow == 1 ? " " : "", type); > + follow = 1; > + } > + if (shown_bits & SHOW_OBJECT_NAME) { > + printf("%s%s", follow == 1 ? " " : "", > + find_unique_abbrev(oid, abbrev)); > + if (!(shown_bits ^ SHOW_OBJECT_NAME)) > + printf("%c", line_termination); Curious. I wonder if we can get rid of these two lines (and the line_termination bit in the SHOW_FILE_NAME part), and have an unconditional putchar(line_termination); at the end of the function. That way, we could in the future choose to introduce a feature to show only and nothing else, which may be useful for taking per-type stats. We need to stop using write_name_quoted_relative() in SHOW_FILE_NAME part, because the helper insists that the name written by it must be at the end of the entry, if we go that route, but it may be a good change in the longer term. > + follow = 1; > + } > + if (shown_bits & SHOW_SIZE) { > + char size_text[24]; > + if (!strcmp(type, blob_type)) { > + unsigned long size; > + if (oid_object_info(the_repository, oid, &size) == OBJ_BAD) > + xsnprintf(size_text, sizeof(size_text), "BAD"); > + else > + xsnprintf(size_text, sizeof(size_text), > + "%"PRIuMAX, (uintmax_t)size); > } else > - printf("%06o %s %s\t", mode, type, > - find_unique_abbrev(oid, abbrev)); > + xsnprintf(size_text, sizeof(size_text), "-"); > + printf("%s%7s", follow == 1 ? " " : "", size_text); > + follow = 1; > + } > + if (shown_bits & SHOW_FILE_NAME) { > + if (follow) > + printf("\t"); > + baselen = base->len; > + strbuf_addstr(base, pathname); > + write_name_quoted_relative(base->buf, > + chomp_prefix ? ls_tree_prefix : NULL, > + stdout, line_termination); > + strbuf_setlen(base, baselen); > } But the above nits aside, the updated organization of this function looks much cleaner than the original. Nicely reorganized. > - baselen = base->len; > - strbuf_addstr(base, pathname); > - write_name_quoted_relative(base->buf, > - chomp_prefix ? ls_tree_prefix : NULL, > - stdout, line_termination); > - strbuf_setlen(base, baselen); > return retval; > } Thanks. ``` ## Teng Long, 2021-12-16 12:16 Subject: Re: [PATCH v5 1/1] ls-tree.c: support `--object-only` option for "git-ls-tree" Message-ID: <20211216121630.14553-1-dyroneteng@gmail.com> URL: https://gitlist.dev/e/20211216121630.14553-1-dyroneteng%40gmail.com In-Reply-To: ``` > I notice that this changed from --oid to --object and I agree that > it would probably be more friendly to end users. In fact, this > > $ sed -ne '/^SYNOPSIS/,/^DESCRIPTION/p' Documentation/git-*.txt | > grep -e -oid > > did not find any hits. Thank you for comfirming that. > It usually is a good idea to enclose these in () so that they are > safe to use in any context in a statement. > > Luckily, bitwise-or and bitwise-and, which are the most likely > candidates for these symbols to be used with, bind looser than > left-shift, so something like > > if ((LS_TREE_ONLY | LS_SHOW_TREES) & opt) > ... do this ... > > is safe either way, but (LS_TREE_ONLY + LS_SHOW_TREES) would have > different value with and without () around (1 << N). Make sense. Will fix in patch v6. > Style: we do not initialize statics explicitly to zero. > ... > Likewise. It is a bit curious to see these listed in decreasing > order, though. Will fix. > It is a good idea to leave a comma even after the last element, > _unless_ there is a strong reason why the element that currently is > at the last MUST stay to be last when new elements are added to the > enum. That way, a future patch that adds a new element can add it > to the list with a patch noise with fewer lines. Very clear explanation. Will fix. > A better name, anybody? > > This bit is to keep track of the fact that we made _some_ output > already so any further output needs an inter-field space before > writing what it wants to write out. I found a word "interspace", it looks like a little better than the old one. I will rename to it in next patch, and If there's a better idea, will apply further. > SP before ','. Will fix. > Curious. I wonder if we can get rid of these two lines (and the > line_termination bit in the SHOW_FILE_NAME part), and have an > unconditional > > putchar(line_termination); > > at the end of the function. > > That way, we could in the future choose to introduce a feature to > show only and nothing else, which may be useful > for taking per-type stats. > > We need to stop using write_name_quoted_relative() in SHOW_FILE_NAME > part, because the helper insists that the name written by it must be > at the end of the entry, if we go that route, but it may be a good > change in the longer term. Let me try to represent to make sure I understand your suggestion sufficiently. "write_name_quoted_relative" is used to compute the relative file name by "prefix" and output the name and a line termination to the given FD. We do not want use "write_name_quoted_relative" in here because the function alway output a line termination after "name", this may bring some inconvenience because the "name" may not be the last field in the future. So, instead: We need to calculate the file name (relative path and quotes if need) without "write_name_quoted_relative" and then output the line termination before return. Thanks. ``` ## Junio C Hamano, 2021-12-16 21:26 Subject: Re: [PATCH v5 1/1] ls-tree.c: support `--object-only` option for "git-ls-tree" Message-ID: URL: https://gitlist.dev/e/xmqq7dc49r7j.fsf%40gitster.g In-Reply-To: <20211216121630.14553-1-dyroneteng@gmail.com> ``` Teng Long writes: >> A better name, anybody? >> >> This bit is to keep track of the fact that we made _some_ output >> already so any further output needs an inter-field space before >> writing what it wants to write out. > > I found a word "interspace", it looks like a little better than the old > one. I will rename to it in next patch, and If there's a better idea, > will apply further. Yeah, it really is needs_inter_field_space but that is way too long. >> We need to stop using write_name_quoted_relative() in SHOW_FILE_NAME >> part, because the helper insists that the name written by it must be >> at the end of the entry, if we go that route, but it may be a good >> change in the longer term. > > Let me try to represent to make sure I understand your suggestion > sufficiently. > > "write_name_quoted_relative" is used to compute the relative file name > by "prefix" and output the name and a line termination to the given FD. > > We do not want use "write_name_quoted_relative" in here because the > function alway output a line termination after "name", this may bring > some inconvenience because the "name" may not be the last field in the > future. > > So, instead: > > We need to calculate the file name (relative path and quotes if need) > without "write_name_quoted_relative" and then output the line > termination before return. I think we are on the same page. We can work backwards, I think. We have a repetitive if (mode should be shown) { show mode; record that we have already shown something; } if (type should be shown) { give inter-field-space if we have shown something; show type; record that we have already shown something; } ... that ends with if (name should be shown) { give inter-field-space if we have shown something; show name PLUS line termination; } But if we can make the last step to if (name should be shown) { give inter-field-space if we have shown something; show name; } give line termination; it gets easier to support a combination that does not show name, and we can have inter-record separator. But write_name_quoted_relative() does not give the caller a choice to have no terminator, so we need to do something like this: if (shown_bits & SHOW_FILE_NAME) { const char *name; struct strbuf name_buf = STRBUF_INIT; if (follow) printf("\t"); baselen = base->len; strbuf_addstr(base, pathname); name = relative_path(base->buf, chomp_prefix ? ls_tree_prefix : NULL, &name_buf); if (line_termination) quote_c_style(name, NULL, stdout, 0); else fputs(name, stdout); strbuf_release(&name_buf); strbuf_setlen(base, baselen); } I initially thought that extending write_name_quoted() and write_name_quoted_relative() to accept a special value or two for terminator to tell it not to add terminator would be sufficient (see below). I however think it is way too ugly to have the "add no terminator and do not quote" option at write_name_quoted() level, simply because the caller that chooses as-is can simply do fputs() itself without bothering to use write_name_quoted(). So I am not convinced that it will a good idea. If we were to go that "ugly helper" route, the above can become even simpler and closer to what you originally wrote, e.g. if (shown_bits & SHOW_FILE_NAME) { if (follow) printf("\t"); baselen = base->len; strbuf_addstr(base, pathname); write_name_quoted_relative(base->buf, chomp_prefix ? ls_tree_prefix : NULL, stdout, line_termination ? CQ_NO_TERMINATOR_C_QUOTED : CQ_NO_TERMINATOR_AS_IS); strbuf_setlen(base, baselen); } quote.c | 5 +++-- quote.h | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git c/quote.c w/quote.c index 26719d21d1..cbbcd8563f 100644 --- c/quote.c +++ w/quote.c @@ -340,12 +340,13 @@ void quote_two_c_style(struct strbuf *sb, const char *prefix, const char *path, void write_name_quoted(const char *name, FILE *fp, int terminator) { - if (terminator) { + if (0 < terminator || terminator == CQ_NO_TERMINATOR_C_QUOTED) { quote_c_style(name, NULL, fp, 0); } else { fputs(name, fp); } - fputc(terminator, fp); + if (0 <= terminator) + fputc(terminator, fp); } void write_name_quoted_relative(const char *name, const char *prefix, diff --git c/quote.h w/quote.h index 87ff458b06..5c8c7cf952 100644 --- c/quote.h +++ w/quote.h @@ -85,7 +85,26 @@ int unquote_c_style(struct strbuf *, const char *quoted, const char **endp); size_t quote_c_style(const char *name, struct strbuf *, FILE *, unsigned); void quote_two_c_style(struct strbuf *, const char *, const char *, unsigned); +/* + * Write a name, typically a filename, followed by a terminator that + * separates it from what comes next. + * When terminator is NUL, the name is given as-is. Otherwise, the + * name is c-quoted, suitable for text output. HT and LF are typical + * values used for the terminator, but other positive values are possible. + * + * In addition to non-negative values two special values in terminator + * are possible. + * -1: show the name c-quoted, without adding any terminator. + * -2: show the name as-is, without adding any terminator. + */ +#define CQ_NO_TERMINATOR_C_QUOTED (-1) +#define CQ_NO_TERMINATOR_AS_IS (-2) void write_name_quoted(const char *name, FILE *, int terminator); + +/* + * Similar to the above, but the name is first made relative to the prefix + * before being shown. + */ void write_name_quoted_relative(const char *name, const char *prefix, FILE *fp, int terminator); ``` ## Ævar Arnfjörð Bjarmason, 2021-12-16 21:29 Subject: Re: [PATCH v5 1/1] ls-tree.c: support `--object-only` option for "git-ls-tree" Message-ID: <211216.86mtl09qwl.gmgdl@evledraar.gmail.com> URL: https://gitlist.dev/e/211216.86mtl09qwl.gmgdl%40evledraar.gmail.com In-Reply-To: ``` On Thu, Dec 16 2021, Junio C Hamano wrote: > Teng Long writes: > >>> A better name, anybody? >>> >>> This bit is to keep track of the fact that we made _some_ output >>> already so any further output needs an inter-field space before >>> writing what it wants to write out. >> >> I found a word "interspace", it looks like a little better than the old >> one. I will rename to it in next patch, and If there's a better idea, >> will apply further. > > Yeah, it really is needs_inter_field_space but that is way too long. > >>> We need to stop using write_name_quoted_relative() in SHOW_FILE_NAME >>> part, because the helper insists that the name written by it must be >>> at the end of the entry, if we go that route, but it may be a good >>> change in the longer term. >> >> Let me try to represent to make sure I understand your suggestion >> sufficiently. >> >> "write_name_quoted_relative" is used to compute the relative file name >> by "prefix" and output the name and a line termination to the given FD. >> >> We do not want use "write_name_quoted_relative" in here because the >> function alway output a line termination after "name", this may bring >> some inconvenience because the "name" may not be the last field in the >> future. >> >> So, instead: >> >> We need to calculate the file name (relative path and quotes if need) >> without "write_name_quoted_relative" and then output the line >> termination before return. > > I think we are on the same page. We can work backwards, I think. > > We have a repetitive > > if (mode should be shown) { > show mode; > record that we have already shown something; > } > if (type should be shown) { > give inter-field-space if we have shown something; > show type; > record that we have already shown something; > } > ... > > that ends with > > if (name should be shown) { > give inter-field-space if we have shown something; > show name PLUS line termination; > } > > But if we can make the last step to > > if (name should be shown) { > give inter-field-space if we have shown something; > show name; > } > > give line termination; > > it gets easier to support a combination that does not show name, and > we can have inter-record separator. > > But write_name_quoted_relative() does not give the caller a choice > to have no terminator, so we need to do something like this: > > if (shown_bits & SHOW_FILE_NAME) { > const char *name; > struct strbuf name_buf = STRBUF_INIT; > > if (follow) > printf("\t"); > baselen = base->len; > strbuf_addstr(base, pathname); > > name = relative_path(base->buf, > chomp_prefix ? ls_tree_prefix : NULL, > &name_buf); > if (line_termination) > quote_c_style(name, NULL, stdout, 0); > else > fputs(name, stdout); > strbuf_release(&name_buf); > strbuf_setlen(base, baselen); > } > > I initially thought that extending write_name_quoted() and > write_name_quoted_relative() to accept a special value or two for > terminator to tell it not to add terminator would be sufficient (see > below). I however think it is way too ugly to have the "add no > terminator and do not quote" option at write_name_quoted() level, > simply because the caller that chooses as-is can simply do fputs() > itself without bothering to use write_name_quoted(). So I am not > convinced that it will a good idea. > > If we were to go that "ugly helper" route, the above can become even > simpler and closer to what you originally wrote, e.g. > > if (shown_bits & SHOW_FILE_NAME) { > if (follow) > printf("\t"); > baselen = base->len; > strbuf_addstr(base, pathname); > write_name_quoted_relative(base->buf, > chomp_prefix ? ls_tree_prefix : NULL, > stdout, > line_termination > ? CQ_NO_TERMINATOR_C_QUOTED > : CQ_NO_TERMINATOR_AS_IS); > strbuf_setlen(base, baselen); > } > > > quote.c | 5 +++-- > quote.h | 19 +++++++++++++++++++ > 2 files changed, 22 insertions(+), 2 deletions(-) > > diff --git c/quote.c w/quote.c > index 26719d21d1..cbbcd8563f 100644 > --- c/quote.c > +++ w/quote.c > @@ -340,12 +340,13 @@ void quote_two_c_style(struct strbuf *sb, const char *prefix, const char *path, > > void write_name_quoted(const char *name, FILE *fp, int terminator) > { > - if (terminator) { > + if (0 < terminator || terminator == CQ_NO_TERMINATOR_C_QUOTED) { > quote_c_style(name, NULL, fp, 0); > } else { > fputs(name, fp); > } > - fputc(terminator, fp); > + if (0 <= terminator) > + fputc(terminator, fp); > } > > void write_name_quoted_relative(const char *name, const char *prefix, > diff --git c/quote.h w/quote.h > index 87ff458b06..5c8c7cf952 100644 > --- c/quote.h > +++ w/quote.h > @@ -85,7 +85,26 @@ int unquote_c_style(struct strbuf *, const char *quoted, const char **endp); > size_t quote_c_style(const char *name, struct strbuf *, FILE *, unsigned); > void quote_two_c_style(struct strbuf *, const char *, const char *, unsigned); > > +/* > + * Write a name, typically a filename, followed by a terminator that > + * separates it from what comes next. > + * When terminator is NUL, the name is given as-is. Otherwise, the > + * name is c-quoted, suitable for text output. HT and LF are typical > + * values used for the terminator, but other positive values are possible. > + * > + * In addition to non-negative values two special values in terminator > + * are possible. > + * -1: show the name c-quoted, without adding any terminator. > + * -2: show the name as-is, without adding any terminator. > + */ > +#define CQ_NO_TERMINATOR_C_QUOTED (-1) > +#define CQ_NO_TERMINATOR_AS_IS (-2) > void write_name_quoted(const char *name, FILE *, int terminator); > + > +/* > + * Similar to the above, but the name is first made relative to the prefix > + * before being shown. > + */ > void write_name_quoted_relative(const char *name, const char *prefix, > FILE *fp, int terminator); > In my "just make ls-tree support --format" this whole thing is just: + if (prefix) + name = relative_path(name, prefix, &scratch); + quote_c_style(name, sb, NULL, 0); + strbuf_addch(sb, line_termination); But of course that's an implementation that's moved away from the "write stuff to a FH for me" API in quote.c. So I haven't looked carefully here, but you need this API just because of some constraint in the write_name_quoted()? Perhaps just running with that approach is better? Whether it's stealing that approach, or doing away with --object-only for a --format... ``` ## Teng Long, 2021-12-17 06:57 Subject: [PATCH v6 0/1] support `--object-only` option for "git-ls-tree" Message-ID: URL: https://gitlist.dev/e/cover.1639721750.git.dyroneteng%40gmail.com In-Reply-To: ``` Diffs from patch v5: * Let "write_name_quoted_relative" support two terminations, they are "CQ_NO_TERMINATOR_C_QUOTED" and "CQ_NO_TERMINATOR_AS_IS" (They are used to let the function write "name" at a 'c_quoted' way or just 'as-is', but without print a terminator). * Rename "follow" to "interspace" (Representing whether any further output needs an inter-field space). * Nits fixed. Many thanks to Junio and Ævar for your help and patient explanation. I noticed Ævar suggest the solution with using `--format`, but in this patch, the current approach continues. If this part of code needs to be improved or we want to support "--format" in "ls-tree" in the future, I'm more than glad to continue to contribute. Thanks. Teng Long (1): ls-tree.c: support `--object-only` option for "git-ls-tree" Documentation/git-ls-tree.txt | 7 +- builtin/ls-tree.c | 131 ++++++++++++++++++++++++---------- quote.c | 8 +-- quote.h | 19 +++++ t/t3103-ls-tree-misc.sh | 8 +++ t/t3104-ls-tree-oid.sh | 51 +++++++++++++ 6 files changed, 183 insertions(+), 41 deletions(-) create mode 100755 t/t3104-ls-tree-oid.sh Range-diff against v5: 1: 38d55a878c ! 1: 2e449d1c79 ls-tree.c: support `--object-only` option for "git-ls-tree" @@ builtin/ls-tree.c -#define LS_SHOW_TREES 4 -#define LS_NAME_ONLY 8 -#define LS_SHOW_SIZE 16 -+#define LS_TREE_ONLY 1 << 1 -+#define LS_SHOW_TREES 1 << 2 -+#define LS_NAME_ONLY 1 << 3 -+#define LS_SHOW_SIZE 1 << 4 -+#define LS_OBJECT_ONLY 1 << 5 ++#define LS_TREE_ONLY (1 << 1) ++#define LS_SHOW_TREES (1 << 2) ++#define LS_NAME_ONLY (1 << 3) ++#define LS_SHOW_SIZE (1 << 4) ++#define LS_OBJECT_ONLY (1 << 5) static int abbrev; static int ls_options; static struct pathspec pathspec; static int chomp_prefix; static const char *ls_tree_prefix; -+static unsigned int shown_bits = 0; -+#define SHOW_DEFAULT 29 /* 11101 size is not shown to output by default */ -+#define SHOW_MODE 1 << 4 -+#define SHOW_TYPE 1 << 3 -+#define SHOW_OBJECT_NAME 1 << 2 -+#define SHOW_SIZE 1 << 1 ++static unsigned int shown_bits; +#define SHOW_FILE_NAME 1 ++#define SHOW_SIZE (1 << 1) ++#define SHOW_OBJECT_NAME (1 << 2) ++#define SHOW_TYPE (1 << 3) ++#define SHOW_MODE (1 << 4) ++#define SHOW_DEFAULT 29 /* 11101 size is not shown to output by default */ static const char * const ls_tree_usage[] = { N_("git ls-tree [] [...]"), @@ builtin/ls-tree.c + MODE_UNSPECIFIED = 0, + MODE_NAME_ONLY, + MODE_OBJECT_ONLY, -+ MODE_LONG ++ MODE_LONG, +}; + +static int cmdmode = MODE_UNSPECIFIED; @@ builtin/ls-tree.c: static int show_tree(const struct object_id *oid, struct strb { int retval = 0; int baselen; -+ int follow = 0; ++ int interspace = 0; const char *type = blob_type; if (S_ISGITLINK(mode)) { @@ builtin/ls-tree.c: static int show_tree(const struct object_id *oid, struct strb - find_unique_abbrev(oid, abbrev), - size_text); + if (shown_bits & SHOW_MODE) { -+ printf("%06o",mode); -+ follow = 1; ++ printf("%06o", mode); ++ interspace = 1; + } + if (shown_bits & SHOW_TYPE) { -+ printf("%s%s", follow == 1 ? " " : "", type); -+ follow = 1; ++ printf("%s%s", interspace ? " " : "", type); ++ interspace = 1; + } + if (shown_bits & SHOW_OBJECT_NAME) { -+ printf("%s%s", follow == 1 ? " " : "", ++ printf("%s%s", interspace ? " " : "", + find_unique_abbrev(oid, abbrev)); + if (!(shown_bits ^ SHOW_OBJECT_NAME)) -+ printf("%c", line_termination); -+ follow = 1; ++ goto LINE_FINISH; ++ interspace = 1; + } + if (shown_bits & SHOW_SIZE) { + char size_text[24]; @@ builtin/ls-tree.c: static int show_tree(const struct object_id *oid, struct strb - printf("%06o %s %s\t", mode, type, - find_unique_abbrev(oid, abbrev)); + xsnprintf(size_text, sizeof(size_text), "-"); -+ printf("%s%7s", follow == 1 ? " " : "", size_text); -+ follow = 1; ++ printf("%s%7s", interspace ? " " : "", size_text); ++ interspace = 1; + } + if (shown_bits & SHOW_FILE_NAME) { -+ if (follow) ++ if (interspace) + printf("\t"); + baselen = base->len; + strbuf_addstr(base, pathname); + write_name_quoted_relative(base->buf, + chomp_prefix ? ls_tree_prefix : NULL, -+ stdout, line_termination); ++ stdout, ++ line_termination ++ ? CQ_NO_TERMINATOR_C_QUOTED ++ : CQ_NO_TERMINATOR_AS_IS); + strbuf_setlen(base, baselen); } - baselen = base->len; @@ builtin/ls-tree.c: static int show_tree(const struct object_id *oid, struct strb - chomp_prefix ? ls_tree_prefix : NULL, - stdout, line_termination); - strbuf_setlen(base, baselen); ++ ++LINE_FINISH: ++ putchar(line_termination); return retval; } @@ builtin/ls-tree.c: int cmd_ls_tree(int argc, const char **argv, const char *pref * show_recursive() rolls its own matching code and is * generally ignorant of 'struct pathspec'. The magic mask + ## quote.c ## +@@ quote.c: void quote_two_c_style(struct strbuf *sb, const char *prefix, const char *path, + + void write_name_quoted(const char *name, FILE *fp, int terminator) + { +- if (terminator) { ++ if (0 < terminator || terminator == CQ_NO_TERMINATOR_C_QUOTED) + quote_c_style(name, NULL, fp, 0); +- } else { ++ else + fputs(name, fp); +- } +- fputc(terminator, fp); ++ if (0 <= terminator) ++ fputc(terminator, fp); + } + + void write_name_quoted_relative(const char *name, const char *prefix, + + ## quote.h ## +@@ quote.h: int unquote_c_style(struct strbuf *, const char *quoted, const char **endp); + #define CQUOTE_NODQ 01 + size_t quote_c_style(const char *name, struct strbuf *, FILE *, unsigned); + void quote_two_c_style(struct strbuf *, const char *, const char *, unsigned); ++/* ++ * Write a name, typically a filename, followed by a terminator that ++ * separates it from what comes next. ++ * When terminator is NUL, the name is given as-is. Otherwise, the ++ * name is c-quoted, suitable for text output. HT and LF are typical ++ * values used for the terminator, but other positive values are possible. ++ * ++ * In addition to non-negative values two special values in terminator ++ * are possible. ++ * ++ * -1: show the name c-quoted, without adding any terminator. ++ * -2: show the name as-is, without adding any terminator. ++ */ ++#define CQ_NO_TERMINATOR_C_QUOTED (-1) ++#define CQ_NO_TERMINATOR_AS_IS (-2) + + void write_name_quoted(const char *name, FILE *, int terminator); ++/* ++ * Similar to the above, but the name is first made relative to the prefix ++ * before being shown. ++ */ + void write_name_quoted_relative(const char *name, const char *prefix, + FILE *fp, int terminator); + + ## t/t3103-ls-tree-misc.sh ## @@ t/t3103-ls-tree-misc.sh: test_expect_success 'ls-tree fails with non-zero exit code on broken tree' ' test_must_fail git ls-tree -r HEAD -- 2.33.1.10.g5f17c1a2c1.dirty ``` ## Teng Long, 2021-12-17 06:57 Subject: [PATCH v6 1/1] ls-tree.c: support `--object-only` option for "git-ls-tree" Message-ID: <2e449d1c792ff81da5f22c8bf65ed33c393d62f8.1639721750.git.dyroneteng@gmail.com> URL: https://gitlist.dev/e/2e449d1c792ff81da5f22c8bf65ed33c393d62f8.1639721750.git.dyroneteng%40gmail.com In-Reply-To: ``` We usually pipe the output from `git ls-trees` to tools like `sed` or `cut` when we only want to extract some fields. When we want only the pathname component, we can pass `--name-only` option to omit such a pipeline, but there are no options for extracting other fields. Teach the "--object-only" option to the command to only show the object name. This option cannot be used together with "--name-only" or "--long" (mutually exclusive). Signed-off-by: Teng Long --- Documentation/git-ls-tree.txt | 7 +- builtin/ls-tree.c | 131 ++++++++++++++++++++++++---------- quote.c | 8 +-- quote.h | 19 +++++ t/t3103-ls-tree-misc.sh | 8 +++ t/t3104-ls-tree-oid.sh | 51 +++++++++++++ 6 files changed, 183 insertions(+), 41 deletions(-) create mode 100755 t/t3104-ls-tree-oid.sh diff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt index db02d6d79a..729370f235 100644 --- a/Documentation/git-ls-tree.txt +++ b/Documentation/git-ls-tree.txt @@ -10,7 +10,7 @@ SYNOPSIS -------- [verse] 'git ls-tree' [-d] [-r] [-t] [-l] [-z] - [--name-only] [--name-status] [--full-name] [--full-tree] [--abbrev[=]] + [--name-only] [--name-status] [--object-only] [--full-name] [--full-tree] [--abbrev[=]] [...] DESCRIPTION @@ -59,6 +59,11 @@ OPTIONS --name-only:: --name-status:: List only filenames (instead of the "long" output), one per line. + Cannot be combined with `--object-only`. + +--object-only:: + List only names of the objects, one per line. Cannot be combined + with `--name-only` or `--name-status`. --abbrev[=]:: Instead of showing the full 40-byte hexadecimal object diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index 3a442631c7..e76c6e43e8 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c @@ -16,21 +16,38 @@ static int line_termination = '\n'; #define LS_RECURSIVE 1 -#define LS_TREE_ONLY 2 -#define LS_SHOW_TREES 4 -#define LS_NAME_ONLY 8 -#define LS_SHOW_SIZE 16 +#define LS_TREE_ONLY (1 << 1) +#define LS_SHOW_TREES (1 << 2) +#define LS_NAME_ONLY (1 << 3) +#define LS_SHOW_SIZE (1 << 4) +#define LS_OBJECT_ONLY (1 << 5) static int abbrev; static int ls_options; static struct pathspec pathspec; static int chomp_prefix; static const char *ls_tree_prefix; +static unsigned int shown_bits; +#define SHOW_FILE_NAME 1 +#define SHOW_SIZE (1 << 1) +#define SHOW_OBJECT_NAME (1 << 2) +#define SHOW_TYPE (1 << 3) +#define SHOW_MODE (1 << 4) +#define SHOW_DEFAULT 29 /* 11101 size is not shown to output by default */ static const char * const ls_tree_usage[] = { N_("git ls-tree [] [...]"), NULL }; +enum { + MODE_UNSPECIFIED = 0, + MODE_NAME_ONLY, + MODE_OBJECT_ONLY, + MODE_LONG, +}; + +static int cmdmode = MODE_UNSPECIFIED; + static int show_recursive(const char *base, int baselen, const char *pathname) { int i; @@ -66,6 +83,7 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, { int retval = 0; int baselen; + int interspace = 0; const char *type = blob_type; if (S_ISGITLINK(mode)) { @@ -74,8 +92,8 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, * * Something similar to this incomplete example: * - if (show_subprojects(base, baselen, pathname)) - retval = READ_TREE_RECURSIVE; + * if (show_subprojects(base, baselen, pathname)) + * retval = READ_TREE_RECURSIVE; * */ type = commit_type; @@ -90,35 +108,73 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, else if (ls_options & LS_TREE_ONLY) return 0; - if (!(ls_options & LS_NAME_ONLY)) { - if (ls_options & LS_SHOW_SIZE) { - char size_text[24]; - if (!strcmp(type, blob_type)) { - unsigned long size; - if (oid_object_info(the_repository, oid, &size) == OBJ_BAD) - xsnprintf(size_text, sizeof(size_text), - "BAD"); - else - xsnprintf(size_text, sizeof(size_text), - "%"PRIuMAX, (uintmax_t)size); - } else - xsnprintf(size_text, sizeof(size_text), "-"); - printf("%06o %s %s %7s\t", mode, type, - find_unique_abbrev(oid, abbrev), - size_text); + if (shown_bits & SHOW_MODE) { + printf("%06o", mode); + interspace = 1; + } + if (shown_bits & SHOW_TYPE) { + printf("%s%s", interspace ? " " : "", type); + interspace = 1; + } + if (shown_bits & SHOW_OBJECT_NAME) { + printf("%s%s", interspace ? " " : "", + find_unique_abbrev(oid, abbrev)); + if (!(shown_bits ^ SHOW_OBJECT_NAME)) + goto LINE_FINISH; + interspace = 1; + } + if (shown_bits & SHOW_SIZE) { + char size_text[24]; + if (!strcmp(type, blob_type)) { + unsigned long size; + if (oid_object_info(the_repository, oid, &size) == OBJ_BAD) + xsnprintf(size_text, sizeof(size_text), "BAD"); + else + xsnprintf(size_text, sizeof(size_text), + "%"PRIuMAX, (uintmax_t)size); } else - printf("%06o %s %s\t", mode, type, - find_unique_abbrev(oid, abbrev)); + xsnprintf(size_text, sizeof(size_text), "-"); + printf("%s%7s", interspace ? " " : "", size_text); + interspace = 1; + } + if (shown_bits & SHOW_FILE_NAME) { + if (interspace) + printf("\t"); + baselen = base->len; + strbuf_addstr(base, pathname); + write_name_quoted_relative(base->buf, + chomp_prefix ? ls_tree_prefix : NULL, + stdout, + line_termination + ? CQ_NO_TERMINATOR_C_QUOTED + : CQ_NO_TERMINATOR_AS_IS); + strbuf_setlen(base, baselen); } - baselen = base->len; - strbuf_addstr(base, pathname); - write_name_quoted_relative(base->buf, - chomp_prefix ? ls_tree_prefix : NULL, - stdout, line_termination); - strbuf_setlen(base, baselen); + +LINE_FINISH: + putchar(line_termination); return retval; } +static int parse_shown_fields(void) +{ + if (cmdmode == MODE_NAME_ONLY) { + shown_bits = SHOW_FILE_NAME; + return 0; + } + if (cmdmode == MODE_OBJECT_ONLY) { + shown_bits = SHOW_OBJECT_NAME; + return 0; + } + if (!ls_options || (ls_options & LS_RECURSIVE) + || (ls_options & LS_SHOW_TREES) + || (ls_options & LS_TREE_ONLY)) + shown_bits = SHOW_DEFAULT; + if (cmdmode == MODE_LONG) + shown_bits = SHOW_DEFAULT | SHOW_SIZE; + return 1; +} + int cmd_ls_tree(int argc, const char **argv, const char *prefix) { struct object_id oid; @@ -133,12 +189,14 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) LS_SHOW_TREES), OPT_SET_INT('z', NULL, &line_termination, N_("terminate entries with NUL byte"), 0), - OPT_BIT('l', "long", &ls_options, N_("include object size"), - LS_SHOW_SIZE), - OPT_BIT(0, "name-only", &ls_options, N_("list only filenames"), - LS_NAME_ONLY), - OPT_BIT(0, "name-status", &ls_options, N_("list only filenames"), - LS_NAME_ONLY), + OPT_CMDMODE('l', "long", &cmdmode, N_("include object size"), + MODE_LONG), + OPT_CMDMODE(0, "name-only", &cmdmode, N_("list only filenames"), + MODE_NAME_ONLY), + OPT_CMDMODE(0, "name-status", &cmdmode, N_("list only filenames"), + MODE_NAME_ONLY), + OPT_CMDMODE(0, "object-only", &cmdmode, N_("list only objects"), + MODE_OBJECT_ONLY), OPT_SET_INT(0, "full-name", &chomp_prefix, N_("use full path names"), 0), OPT_BOOL(0, "full-tree", &full_tree, @@ -169,6 +227,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) if (get_oid(argv[0], &oid)) die("Not a valid object name %s", argv[0]); + parse_shown_fields(); /* * show_recursive() rolls its own matching code and is * generally ignorant of 'struct pathspec'. The magic mask diff --git a/quote.c b/quote.c index 8a3a5e39eb..9f9da49fa2 100644 --- a/quote.c +++ b/quote.c @@ -340,12 +340,12 @@ void quote_two_c_style(struct strbuf *sb, const char *prefix, const char *path, void write_name_quoted(const char *name, FILE *fp, int terminator) { - if (terminator) { + if (0 < terminator || terminator == CQ_NO_TERMINATOR_C_QUOTED) quote_c_style(name, NULL, fp, 0); - } else { + else fputs(name, fp); - } - fputc(terminator, fp); + if (0 <= terminator) + fputc(terminator, fp); } void write_name_quoted_relative(const char *name, const char *prefix, diff --git a/quote.h b/quote.h index 049d8dd0b3..ec3f862545 100644 --- a/quote.h +++ b/quote.h @@ -84,8 +84,27 @@ int unquote_c_style(struct strbuf *, const char *quoted, const char **endp); #define CQUOTE_NODQ 01 size_t quote_c_style(const char *name, struct strbuf *, FILE *, unsigned); void quote_two_c_style(struct strbuf *, const char *, const char *, unsigned); +/* + * Write a name, typically a filename, followed by a terminator that + * separates it from what comes next. + * When terminator is NUL, the name is given as-is. Otherwise, the + * name is c-quoted, suitable for text output. HT and LF are typical + * values used for the terminator, but other positive values are possible. + * + * In addition to non-negative values two special values in terminator + * are possible. + * + * -1: show the name c-quoted, without adding any terminator. + * -2: show the name as-is, without adding any terminator. + */ +#define CQ_NO_TERMINATOR_C_QUOTED (-1) +#define CQ_NO_TERMINATOR_AS_IS (-2) void write_name_quoted(const char *name, FILE *, int terminator); +/* + * Similar to the above, but the name is first made relative to the prefix + * before being shown. + */ void write_name_quoted_relative(const char *name, const char *prefix, FILE *fp, int terminator); diff --git a/t/t3103-ls-tree-misc.sh b/t/t3103-ls-tree-misc.sh index 14520913af..75e38b0a51 100755 --- a/t/t3103-ls-tree-misc.sh +++ b/t/t3103-ls-tree-misc.sh @@ -22,4 +22,12 @@ test_expect_success 'ls-tree fails with non-zero exit code on broken tree' ' test_must_fail git ls-tree -r HEAD ' +test_expect_success 'usage: incompatible options: --name-status with --long' ' + test_expect_code 129 git ls-tree --long --name-status +' + +test_expect_success 'usage: incompatible options: --name-only with --long' ' + test_expect_code 129 git ls-tree --long --name-only +' + test_done diff --git a/t/t3104-ls-tree-oid.sh b/t/t3104-ls-tree-oid.sh new file mode 100755 index 0000000000..81304e7b13 --- /dev/null +++ b/t/t3104-ls-tree-oid.sh @@ -0,0 +1,51 @@ +#!/bin/sh + +test_description='git ls-tree objects handling.' + +. ./test-lib.sh + +test_expect_success 'setup' ' + test_commit A && + test_commit B && + mkdir -p C && + test_commit C/D.txt && + find *.txt path* \( -type f -o -type l \) -print | + xargs git update-index --add && + tree=$(git write-tree) && + echo $tree +' + +test_expect_success 'usage: --object-only' ' + git ls-tree --object-only $tree >current && + git ls-tree $tree >result && + cut -f1 result | cut -d " " -f3 >expected && + test_cmp current expected +' + +test_expect_success 'usage: --object-only with -r' ' + git ls-tree --object-only -r $tree >current && + git ls-tree -r $tree >result && + cut -f1 result | cut -d " " -f3 >expected && + test_cmp current expected +' + +test_expect_success 'usage: --object-only with --abbrev' ' + git ls-tree --object-only --abbrev=6 $tree >current && + git ls-tree --abbrev=6 $tree >result && + cut -f1 result | cut -d " " -f3 >expected && + test_cmp current expected +' + +test_expect_success 'usage: incompatible options: --name-only with --object-only' ' + test_expect_code 129 git ls-tree --object-only --name-only +' + +test_expect_success 'usage: incompatible options: --name-status with --object-only' ' + test_expect_code 129 git ls-tree --object-only --name-status +' + +test_expect_success 'usage: incompatible options: --long with --object-only' ' + test_expect_code 129 git ls-tree --object-only --long +' + +test_done -- 2.33.1.10.g5f17c1a2c1.dirty ``` ## Ævar Arnfjörð Bjarmason, 2021-12-17 13:09 Subject: Re: [PATCH v6 1/1] ls-tree.c: support `--object-only` option for "git-ls-tree" Message-ID: <211217.86o85f8jey.gmgdl@evledraar.gmail.com> URL: https://gitlist.dev/e/211217.86o85f8jey.gmgdl%40evledraar.gmail.com In-Reply-To: <2e449d1c792ff81da5f22c8bf65ed33c393d62f8.1639721750.git.dyroneteng@gmail.com> ``` On Fri, Dec 17 2021, Teng Long wrote: > [...] > int cmd_ls_tree(int argc, const char **argv, const char *prefix) > { > struct object_id oid; > @@ -133,12 +189,14 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) > LS_SHOW_TREES), > OPT_SET_INT('z', NULL, &line_termination, > N_("terminate entries with NUL byte"), 0), > - OPT_BIT('l', "long", &ls_options, N_("include object size"), > - LS_SHOW_SIZE), > - OPT_BIT(0, "name-only", &ls_options, N_("list only filenames"), > - LS_NAME_ONLY), > - OPT_BIT(0, "name-status", &ls_options, N_("list only filenames"), > - LS_NAME_ONLY), > + OPT_CMDMODE('l', "long", &cmdmode, N_("include object size"), > + MODE_LONG), > + OPT_CMDMODE(0, "name-only", &cmdmode, N_("list only filenames"), > + MODE_NAME_ONLY), > + OPT_CMDMODE(0, "name-status", &cmdmode, N_("list only filenames"), > + MODE_NAME_ONLY), > + OPT_CMDMODE(0, "object-only", &cmdmode, N_("list only objects"), > + MODE_OBJECT_ONLY), > OPT_SET_INT(0, "full-name", &chomp_prefix, > N_("use full path names"), 0), > OPT_BOOL(0, "full-tree", &full_tree, Very nice to have the OPT_CMDMODE for asserting the usage, but this would be even better if it were done as a separate commit. I.e. let's first do prep cleanups, then the new --object-name mode. > +test_expect_success 'usage: incompatible options: --name-status with --long' ' > + test_expect_code 129 git ls-tree --long --name-status > +' > + > +test_expect_success 'usage: incompatible options: --name-only with --long' ' > + test_expect_code 129 git ls-tree --long --name-only > +' > + > test_done > [...] > +test_expect_success 'usage: incompatible options: --name-only with --object-only' ' > + test_expect_code 129 git ls-tree --object-only --name-only > +' > + > +test_expect_success 'usage: incompatible options: --name-status with --object-only' ' > + test_expect_code 129 git ls-tree --object-only --name-status > +' > + > +test_expect_success 'usage: incompatible options: --long with --object-only' ' > + test_expect_code 129 git ls-tree --object-only --long > +' These tests don't check for what you think they check, because you don't supply a . So they're really just dying for the same reason a: git ls-tree Would. ``` ## Ævar Arnfjörð Bjarmason, 2021-12-17 13:30 Subject: [RFC PATCH 0/7] ls-tree --format Message-ID: URL: https://gitlist.dev/e/RFC-cover-0.7-00000000000-20211217T131635Z-avarab%40gmail.com In-Reply-To: ``` On Fri, Dec 17 2021, Teng Long wrote: > Many thanks to Junio and Ævar for your help and patient explanation. > I noticed Ævar suggest the solution with using `--format`, but in > this patch, the current approach continues. If this part of code needs > to be improved or we want to support "--format" in "ls-tree" in the > future, I'm more than glad to continue to contribute. FWIW here's the changes I had locally & cleaned up now that did the alternate --format approach. I think you'll probably want to steal some of this, e.g. you're patching the dead comment I removed in 1/6, 2-4/6 can be skipped, but I thought they were nice. Back when I last looked at this series, your --object-name patch was much shorter, but now it's about the same size as the generic --format support. So maybe it's worth considering implementing the more generic path. One reason I didn't submit this before is that I couldn't get past the performance regression this would inttroduce, i.e. if moved entirely to strbuf_expand(). Here though I'm keeping the old code, so it's no slower than "master", unlike your patch. But I haven't dug into why yours is slower: $ git hyperfine -L rev origin/master,tl/object-name,avar/ls-tree-format -s 'make CFLAGS=-O3' './git -C /run/user/1001/linux ls-tree -r HEAD' --warmup 10 -r 10 Benchmark 1: ./git -C /run/user/1001/linux ls-tree -r HEAD' in 'origin/master Time (mean ± σ): 67.8 ms ± 0.3 ms [User: 48.8 ms, System: 18.9 ms] Range (min … max): 67.4 ms … 68.4 ms 10 runs Benchmark 2: ./git -C /run/user/1001/linux ls-tree -r HEAD' in 'tl/object-name Time (mean ± σ): 72.8 ms ± 0.4 ms [User: 50.6 ms, System: 22.1 ms] Range (min … max): 72.0 ms … 73.2 ms 10 runs Benchmark 3: ./git -C /run/user/1001/linux ls-tree -r HEAD' in 'avar/ls-tree-format Time (mean ± σ): 67.6 ms ± 0.4 ms [User: 50.5 ms, System: 17.0 ms] Range (min … max): 67.1 ms … 68.4 ms 10 runs Summary './git -C /run/user/1001/linux ls-tree -r HEAD' in 'avar/ls-tree-format' ran 1.00 ± 0.01 times faster than './git -C /run/user/1001/linux ls-tree -r HEAD' in 'origin/master' 1.08 ± 0.01 times faster than './git -C /run/user/1001/linux ls-tree -r HEAD' in 'tl/object-name' I then tacket a 6/6 at the end here to implement your --object-name in terms of --format (but didn't update the comimt message etc.). That's slower as expected: $ git hyperfine -L rev tl/object-name,avar/ls-tree-format -s 'make CFLAGS=-O3' './git -C /run/user/1001/linux ls-tree --object-only -r HEAD' --warmup 10 -r 10 Benchmark 1: ./git -C /run/user/1001/linux ls-tree --object-only -r HEAD' in 'tl/object-name Time (mean ± σ): 58.7 ms ± 0.4 ms [User: 43.0 ms, System: 15.6 ms] Range (min … max): 58.4 ms … 59.6 ms 10 runs Benchmark 2: ./git -C /run/user/1001/linux ls-tree --object-only -r HEAD' in 'avar/ls-tree-format Time (mean ± σ): 65.6 ms ± 0.2 ms [User: 42.4 ms, System: 23.0 ms] Range (min … max): 65.1 ms … 65.9 ms 10 runs Summary './git -C /run/user/1001/linux ls-tree --object-only -r HEAD' in 'tl/object-name' ran 1.12 ± 0.01 times faster than './git -C /run/user/1001/linux ls-tree --object-only -r HEAD' in 'avar/ls-tree-format' But it's not too bad, so maybe it's fine & worth making it more generic? Anyway. Just food for thought and and FYI in case you're interested. Junio noted already that he'd like the --object-name approach first, so if you still want to pursue your current implementation I don't mind. I do think you should be making performance testing a part of your testing & cover letter writing though. A 8-10% slowdown isn't nothing, especially for exactly the sort of plumbing command that'll likely to be used to e.g. slurp up all paths in a very large repo. These patches really aren't "ready". There's no docs, and as I noted in some earlier thread the tests for ls-tree are really lacking. E.g. I seem to have a rather obvious bug in how -t and the --format interact here, but no test catches it. Well, that one's me not having added a test, but I'm fairly sure there might also be hidden bugs here due to lack of testing. Teng Long (1): ls-tree.c: support `--object-only` option for "git-ls-tree" Ævar Arnfjörð Bjarmason (6): ls-tree: remove commented-out code ls-tree: add missing braces to "else" arms ls-tree: use "enum object_type", not {blob,tree,commit}_type ls-tree: use "size_t", not "int" for "struct strbuf"'s "len" ls-tree: split up the "init" part of show_tree() ls-tree: add a --format= option Documentation/git-ls-tree.txt | 7 +- builtin/ls-tree.c | 226 ++++++++++++++++++++++++++++++---- t/t3103-ls-tree-misc.sh | 8 ++ t/t3104-ls-tree-oid.sh | 51 ++++++++ t/t3105-ls-tree-format.sh | 49 ++++++++ 5 files changed, 313 insertions(+), 28 deletions(-) create mode 100755 t/t3104-ls-tree-oid.sh create mode 100755 t/t3105-ls-tree-format.sh -- 2.34.1.1119.g7a3fc8778ee ``` ## Ævar Arnfjörð Bjarmason, 2021-12-17 13:30 Subject: [RFC PATCH 1/7] ls-tree: remove commented-out code Message-ID: URL: https://gitlist.dev/e/RFC-patch-1.7-a4778c87c74-20211217T131635Z-avarab%40gmail.com In-Reply-To: ``` Remove code added in f35a6d3bce7 (Teach core object handling functions about gitlinks, 2007-04-09), later patched in 7d0b18a4da1 (Add output flushing before fork(), 2008-08-04), and then finally ending up in its current form in d3bee161fef (tree.c: allow read_tree_recursive() to traverse gitlink entries, 2009-01-25). All while being commented-out! Signed-off-by: Ævar Arnfjörð Bjarmason --- builtin/ls-tree.c | 9 --------- 1 file changed, 9 deletions(-) diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index 3a442631c71..5f7c84950ce 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c @@ -69,15 +69,6 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, const char *type = blob_type; if (S_ISGITLINK(mode)) { - /* - * Maybe we want to have some recursive version here? - * - * Something similar to this incomplete example: - * - if (show_subprojects(base, baselen, pathname)) - retval = READ_TREE_RECURSIVE; - * - */ type = commit_type; } else if (S_ISDIR(mode)) { if (show_recursive(base->buf, base->len, pathname)) { -- 2.34.1.1119.g7a3fc8778ee ``` ## Ævar Arnfjörð Bjarmason, 2021-12-17 13:30 Subject: [RFC PATCH 2/7] ls-tree: add missing braces to "else" arms Message-ID: URL: https://gitlist.dev/e/RFC-patch-2.7-8b34ee653ff-20211217T131635Z-avarab%40gmail.com In-Reply-To: ``` Add missing {} to the "else" arms in show_tree() per the CodingGuidelines. Signed-off-by: Ævar Arnfjörð Bjarmason --- builtin/ls-tree.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index 5f7c84950ce..0a28f32ccb9 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c @@ -92,14 +92,16 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, else xsnprintf(size_text, sizeof(size_text), "%"PRIuMAX, (uintmax_t)size); - } else + } else { xsnprintf(size_text, sizeof(size_text), "-"); + } printf("%06o %s %s %7s\t", mode, type, find_unique_abbrev(oid, abbrev), size_text); - } else + } else { printf("%06o %s %s\t", mode, type, find_unique_abbrev(oid, abbrev)); + } } baselen = base->len; strbuf_addstr(base, pathname); -- 2.34.1.1119.g7a3fc8778ee ``` ## Ævar Arnfjörð Bjarmason, 2021-12-17 13:30 Subject: [RFC PATCH 4/7] ls-tree: use "size_t", not "int" for "struct strbuf"'s "len" Message-ID: URL: https://gitlist.dev/e/RFC-patch-4.7-721f56ecdac-20211217T131635Z-avarab%40gmail.com In-Reply-To: ``` The "struct strbuf"'s "len" member is a "size_t", not an "int", so let's change our corresponding types accordingly. This also changes the "len" and "speclen" variables, which are likewise used to store the return value of strlen(), which returns "size_t", not "int". Signed-off-by: Ævar Arnfjörð Bjarmason --- builtin/ls-tree.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index 3f0225b097f..eecc7482d54 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c @@ -31,7 +31,7 @@ static const char * const ls_tree_usage[] = { NULL }; -static int show_recursive(const char *base, int baselen, const char *pathname) +static int show_recursive(const char *base, size_t baselen, const char *pathname) { int i; @@ -43,7 +43,7 @@ static int show_recursive(const char *base, int baselen, const char *pathname) for (i = 0; i < pathspec.nr; i++) { const char *spec = pathspec.items[i].match; - int len, speclen; + size_t len, speclen; if (strncmp(base, spec, baselen)) continue; @@ -65,7 +65,7 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, const char *pathname, unsigned mode, void *context) { int retval = 0; - int baselen; + size_t baselen; enum object_type type = OBJ_BLOB; if (S_ISGITLINK(mode)) { -- 2.34.1.1119.g7a3fc8778ee ``` ## Ævar Arnfjörð Bjarmason, 2021-12-17 13:30 Subject: [RFC PATCH 3/7] ls-tree: use "enum object_type", not {blob,tree,commit}_type Message-ID: URL: https://gitlist.dev/e/RFC-patch-3.7-6219756cdd0-20211217T131635Z-avarab%40gmail.com In-Reply-To: ``` Change the ls-tree.c code to use type_name() on the enum instead of using the string constants. This doesn't matter either way for performance, but makes this a bit easier to read as we'll no longer need a strcmp() here. Signed-off-by: Ævar Arnfjörð Bjarmason --- builtin/ls-tree.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index 0a28f32ccb9..3f0225b097f 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c @@ -66,17 +66,17 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, { int retval = 0; int baselen; - const char *type = blob_type; + enum object_type type = OBJ_BLOB; if (S_ISGITLINK(mode)) { - type = commit_type; + type = OBJ_COMMIT; } else if (S_ISDIR(mode)) { if (show_recursive(base->buf, base->len, pathname)) { retval = READ_TREE_RECURSIVE; if (!(ls_options & LS_SHOW_TREES)) return retval; } - type = tree_type; + type = OBJ_TREE; } else if (ls_options & LS_TREE_ONLY) return 0; @@ -84,7 +84,7 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, if (!(ls_options & LS_NAME_ONLY)) { if (ls_options & LS_SHOW_SIZE) { char size_text[24]; - if (!strcmp(type, blob_type)) { + if (type == OBJ_BLOB) { unsigned long size; if (oid_object_info(the_repository, oid, &size) == OBJ_BAD) xsnprintf(size_text, sizeof(size_text), @@ -95,11 +95,11 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, } else { xsnprintf(size_text, sizeof(size_text), "-"); } - printf("%06o %s %s %7s\t", mode, type, + printf("%06o %s %s %7s\t", mode, type_name(type), find_unique_abbrev(oid, abbrev), size_text); } else { - printf("%06o %s %s\t", mode, type, + printf("%06o %s %s\t", mode, type_name(type), find_unique_abbrev(oid, abbrev)); } } -- 2.34.1.1119.g7a3fc8778ee ``` ## Ævar Arnfjörð Bjarmason, 2021-12-17 13:30 Subject: [RFC PATCH 6/7] ls-tree: add a --format= option Message-ID: URL: https://gitlist.dev/e/RFC-patch-6.7-eac299f06ff-20211217T131635Z-avarab%40gmail.com In-Reply-To: ``` Add a --format option to ls-tree. It has an existing default output, and then --long and --name-only options to emit the default output along with the objectsize and, or to only emit object paths. Rather than add --type-only, --object-only etc. we can just support a --format using a strbuf_expand() similar to "for-each-ref --format". We might still add such options in the future for convenience. The --format implementation is slower than the existing code, but this change does not cause any performance regressions. We'll leave the existing show_tree() unchanged, and only run show_tree_format() in if a --format different than the hardcoded built-in ones corresponding to the existing modes is provided. "Slower" here can bee seen via the the following "hyperfine" command. This uses GIT_TEST_LS_TREE_FORMAT_BACKEND= to force the use of the new backend: $ hyperfine -L env false,true -L f "-r,-r -l,-r --name-only,-r --format='%(objectname)'" 'GIT_TEST_LS_TREE_FORMAT_BACKEND={env} ./git -C ~/g/linux ls-tree {f} HEAD' -r 10 Benchmark 1: GIT_TEST_LS_TREE_FORMAT_BACKEND=false ./git -C ~/g/linux ls-tree -r HEAD Time (mean ± σ): 86.1 ms ± 0.6 ms [User: 65.2 ms, System: 20.9 ms] Range (min … max): 85.2 ms … 87.5 ms 10 runs Benchmark 2: GIT_TEST_LS_TREE_FORMAT_BACKEND=true ./git -C ~/g/linux ls-tree -r HEAD Time (mean ± σ): 122.5 ms ± 0.6 ms [User: 101.3 ms, System: 21.1 ms] Range (min … max): 121.8 ms … 123.4 ms 10 runs Benchmark 3: GIT_TEST_LS_TREE_FORMAT_BACKEND=false ./git -C ~/g/linux ls-tree -r -l HEAD Time (mean ± σ): 277.7 ms ± 1.3 ms [User: 234.6 ms, System: 43.0 ms] Range (min … max): 275.9 ms … 279.7 ms 10 runs Benchmark 4: GIT_TEST_LS_TREE_FORMAT_BACKEND=true ./git -C ~/g/linux ls-tree -r -l HEAD Time (mean ± σ): 332.8 ms ± 2.6 ms [User: 282.0 ms, System: 50.7 ms] Range (min … max): 329.6 ms … 338.2 ms 10 runs Benchmark 5: GIT_TEST_LS_TREE_FORMAT_BACKEND=false ./git -C ~/g/linux ls-tree -r --name-only HEAD Time (mean ± σ): 71.8 ms ± 0.4 ms [User: 54.1 ms, System: 17.6 ms] Range (min … max): 71.2 ms … 72.5 ms 10 runs Benchmark 6: GIT_TEST_LS_TREE_FORMAT_BACKEND=true ./git -C ~/g/linux ls-tree -r --name-only HEAD Time (mean ± σ): 86.6 ms ± 0.5 ms [User: 65.7 ms, System: 20.7 ms] Range (min … max): 85.9 ms … 87.4 ms 10 runs Benchmark 7: GIT_TEST_LS_TREE_FORMAT_BACKEND=false ./git -C ~/g/linux ls-tree -r --format='%(objectname)' HEAD Time (mean ± σ): 85.8 ms ± 0.6 ms [User: 66.2 ms, System: 19.5 ms] Range (min … max): 85.0 ms … 86.9 ms 10 runs Benchmark 8: GIT_TEST_LS_TREE_FORMAT_BACKEND=true ./git -C ~/g/linux ls-tree -r --format='%(objectname)' HEAD Time (mean ± σ): 85.3 ms ± 0.2 ms [User: 66.6 ms, System: 18.7 ms] Range (min … max): 85.0 ms … 85.7 ms 10 runs Summary 'GIT_TEST_LS_TREE_FORMAT_BACKEND=false ./git -C ~/g/linux ls-tree -r --name-only HEAD' ran 1.19 ± 0.01 times faster than 'GIT_TEST_LS_TREE_FORMAT_BACKEND=true ./git -C ~/g/linux ls-tree -r --format='%(objectname)' HEAD' 1.19 ± 0.01 times faster than 'GIT_TEST_LS_TREE_FORMAT_BACKEND=false ./git -C ~/g/linux ls-tree -r --format='%(objectname)' HEAD' 1.20 ± 0.01 times faster than 'GIT_TEST_LS_TREE_FORMAT_BACKEND=false ./git -C ~/g/linux ls-tree -r HEAD' 1.21 ± 0.01 times faster than 'GIT_TEST_LS_TREE_FORMAT_BACKEND=true ./git -C ~/g/linux ls-tree -r --name-only HEAD' 1.71 ± 0.01 times faster than 'GIT_TEST_LS_TREE_FORMAT_BACKEND=true ./git -C ~/g/linux ls-tree -r HEAD' 3.87 ± 0.03 times faster than 'GIT_TEST_LS_TREE_FORMAT_BACKEND=false ./git -C ~/g/linux ls-tree -r -l HEAD' 4.64 ± 0.05 times faster than 'GIT_TEST_LS_TREE_FORMAT_BACKEND=true ./git -C ~/g/linux ls-tree -r -l HEAD' I.e. something like the "--long" output would be much slower with this, mainly due to how we need to allocate various things to do with quote.c instead of spewing the output directly to stdout. But even a --format='%(objectname)' is fast with the new backend, so this is viable as a replacement for adding new formats, and we'll pay for this added complexity as a one-off, and not again every time a new format needs to be added. See [1] for an example of what it would otherwise take to add an --object-name flag. 1. https://lore.kernel.org/git/2e449d1c792ff81da5f22c8bf65ed33c393d62f8.1639721750.git.dyroneteng@gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason --- builtin/ls-tree.c | 167 +++++++++++++++++++++++++++++++++++++- t/t3105-ls-tree-format.sh | 49 +++++++++++ 2 files changed, 215 insertions(+), 1 deletion(-) create mode 100755 t/t3105-ls-tree-format.sh diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index df8312408da..efd85cab088 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c @@ -26,11 +26,34 @@ static struct pathspec pathspec; static int chomp_prefix; static const char *ls_tree_prefix; +/* + * The format equivalents that show_tree() is prepared to handle. + */ +static const char *ls_tree_format_d = "%(objectmode) %(objecttype) %(objectname)%x09%(path)"; +static const char *ls_tree_format_l = "%(objectmode) %(objecttype) %(objectname) %(objectsize:padded)%x09%(path)"; +static const char *ls_tree_format_n = "%(path)"; + static const char * const ls_tree_usage[] = { N_("git ls-tree [] [...]"), NULL }; +struct read_tree_ls_tree_data { + const char *format; + struct strbuf sb_scratch; + struct strbuf sb_tmp; +}; + +struct expand_ls_tree_data { + unsigned mode; + enum object_type type; + const struct object_id *oid; + const char *pathname; + const char *basebuf; + struct strbuf *sb_scratch; + struct strbuf *sb_tmp; +}; + static int show_recursive(const char *base, size_t baselen, const char *pathname) { int i; @@ -61,6 +84,76 @@ static int show_recursive(const char *base, size_t baselen, const char *pathname return 0; } +static void expand_objectsize(struct strbuf *sb, + const struct object_id *oid, + const enum object_type type, + unsigned int padded) +{ + if (type == OBJ_BLOB) { + unsigned long size; + if (oid_object_info(the_repository, oid, &size) < 0) + die(_("could not get object info about '%s'"), oid_to_hex(oid)); + if (padded) + strbuf_addf(sb, "%7"PRIuMAX, (uintmax_t)size); + else + strbuf_addf(sb, "%"PRIuMAX, (uintmax_t)size); + } else if (padded) { + strbuf_addf(sb, "%7s", "-"); + } else { + strbuf_addstr(sb, "-"); + } +} + +static size_t expand_show_tree(struct strbuf *sb, + const char *start, + void *context) +{ + struct expand_ls_tree_data *data = context; + const char *end; + const char *p; + size_t len; + + len = strbuf_expand_literal_cb(sb, start, NULL); + if (len) + return len; + + if (*start != '(') + die(_("bad format as of '%s'"), start); + end = strchr(start + 1, ')'); + if (!end) + die(_("ls-tree format element '%s' does not end in ')'"), + start); + len = end - start + 1; + + if (skip_prefix(start, "(objectmode)", &p)) { + strbuf_addf(sb, "%06o", data->mode); + } else if (skip_prefix(start, "(objecttype)", &p)) { + strbuf_addstr(sb, type_name(data->type)); + } else if (skip_prefix(start, "(objectsize:padded)", &p)) { + expand_objectsize(sb, data->oid, data->type, 1); + } else if (skip_prefix(start, "(objectsize)", &p)) { + expand_objectsize(sb, data->oid, data->type, 0); + } else if (skip_prefix(start, "(objectname)", &p)) { + strbuf_addstr(sb, find_unique_abbrev(data->oid, abbrev)); + } else if (skip_prefix(start, "(path)", &p)) { + const char *name = data->basebuf; + const char *prefix = chomp_prefix ? ls_tree_prefix : NULL; + + if (prefix) + name = relative_path(name, prefix, data->sb_scratch); + quote_c_style(name, data->sb_tmp, NULL, 0); + strbuf_add(sb, data->sb_tmp->buf, data->sb_tmp->len); + + strbuf_reset(data->sb_tmp); + /* The relative_path() function resets "scratch" */ + } else { + unsigned int errlen = (unsigned long)len; + die(_("bad ls-tree format specifiec %%%.*s"), errlen, start); + } + + return len; +} + static int show_tree_init(enum object_type *type, struct strbuf *base, const char *pathname, unsigned mode, int *retval) { @@ -79,6 +172,38 @@ static int show_tree_init(enum object_type *type, struct strbuf *base, return 0; } +static int show_tree_fmt(const struct object_id *oid, struct strbuf *base, + const char *pathname, unsigned mode, void *context) +{ + struct read_tree_ls_tree_data *data = context; + struct expand_ls_tree_data my_data = { + .mode = mode, + .type = OBJ_BLOB, + .oid = oid, + .pathname = pathname, + .sb_scratch = &data->sb_scratch, + .sb_tmp = &data->sb_tmp, + }; + struct strbuf sb = STRBUF_INIT; + int retval = 0; + size_t baselen; + + if (show_tree_init(&my_data.type, base, pathname, mode, &retval)) + return retval; + + baselen = base->len; + strbuf_addstr(base, pathname); + strbuf_reset(&sb); + my_data.basebuf = base->buf; + + strbuf_expand(&sb, data->format, expand_show_tree, &my_data); + strbuf_addch(&sb, line_termination); + fwrite(sb.buf, sb.len, 1, stdout); + strbuf_setlen(base, baselen); + + return retval; +} + static int show_tree(const struct object_id *oid, struct strbuf *base, const char *pathname, unsigned mode, void *context) { @@ -125,6 +250,12 @@ 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 *implicit_format = NULL; + const char *format = NULL; + struct read_tree_ls_tree_data read_tree_cb_data = { + .sb_scratch = STRBUF_INIT, + .sb_tmp = STRBUF_INIT, + }; const struct option ls_tree_options[] = { OPT_BIT('d', NULL, &ls_options, N_("only show trees"), LS_TREE_ONLY), @@ -145,9 +276,12 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) OPT_BOOL(0, "full-tree", &full_tree, N_("list entire tree; not just current directory " "(implies --full-name)")), + OPT_STRING_F(0 , "format", &format, N_("format"), + N_("format to use for the output"), PARSE_OPT_NONEG), OPT__ABBREV(&abbrev), OPT_END() }; + read_tree_fn_t fn = show_tree; git_config(git_default_config, NULL); ls_tree_prefix = prefix; @@ -164,6 +298,18 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) if ( (LS_TREE_ONLY|LS_RECURSIVE) == ((LS_TREE_ONLY|LS_RECURSIVE) & ls_options)) ls_options |= LS_SHOW_TREES; + if (ls_options & LS_NAME_ONLY) + implicit_format = ls_tree_format_n; + if (ls_options & LS_SHOW_SIZE) + implicit_format = ls_tree_format_l; + + if (format && implicit_format) + usage_msg_opt(_("providing --format cannot be combined with other format-altering options"), + ls_tree_usage, ls_tree_options); + if (implicit_format) + format = implicit_format; + if (!format) + format = ls_tree_format_d; if (argc < 1) usage_with_options(ls_tree_usage, ls_tree_options); @@ -186,6 +332,25 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) tree = parse_tree_indirect(&oid); if (!tree) die("not a tree object"); + + /* + * The generic show_tree_fmt() is slower than show_tree(), so + * take the fast path if possible. + */ + if (format && (!strcmp(format, ls_tree_format_d) || + !strcmp(format, ls_tree_format_l) || + !strcmp(format, ls_tree_format_n))) + fn = show_tree; + else if (format) + fn = show_tree_fmt; + /* + * Allow forcing the show_tree_fmt(), to test that it can + * handle the test suite. + */ + if (git_env_bool("GIT_TEST_LS_TREE_FORMAT_BACKEND", 0)) + fn = show_tree_fmt; + + read_tree_cb_data.format = format; return !!read_tree(the_repository, tree, - &pathspec, show_tree, NULL); + &pathspec, fn, &read_tree_cb_data); } diff --git a/t/t3105-ls-tree-format.sh b/t/t3105-ls-tree-format.sh new file mode 100755 index 00000000000..79817260ce8 --- /dev/null +++ b/t/t3105-ls-tree-format.sh @@ -0,0 +1,49 @@ +#!/bin/sh + +test_description='ls-tree --format' + +TEST_PASSES_SANITIZE_LEAK=true +. ./test-lib.sh + +test_expect_success 'ls-tree --format usage' ' + test_expect_code 129 git ls-tree --format=fmt -l && + test_expect_code 129 git ls-tree --format=fmt --name-only && + test_expect_code 129 git ls-tree --format=fmt --name-status +' + +test_expect_success 'setup' ' + mkdir dir && + test_commit dir/sub-file && + test_commit top-file +' + +test_ls_tree_format () { + format=$1 && + opts=$2 && + shift 2 && + git ls-tree $opts -r HEAD >expect.raw && + sed "s/^/> /" >expect actual && + test_cmp expect actual +} + +test_expect_success 'ls-tree --format=' ' + test_ls_tree_format \ + "%(objectmode) %(objecttype) %(objectname)%x09%(path)" \ + "" +' + +test_expect_success 'ls-tree --format=' ' + test_ls_tree_format \ + "%(objectmode) %(objecttype) %(objectname) %(objectsize:padded)%x09%(path)" \ + "--long" +' + +test_expect_success 'ls-tree --format=' ' + test_ls_tree_format \ + "%(path)" \ + "--name-only" + +' + +test_done -- 2.34.1.1119.g7a3fc8778ee ``` ## Ævar Arnfjörð Bjarmason, 2021-12-17 13:30 Subject: [RFC PATCH 5/7] ls-tree: split up the "init" part of show_tree() Message-ID: URL: https://gitlist.dev/e/RFC-patch-5.7-8c9d8a8221d-20211217T131635Z-avarab%40gmail.com In-Reply-To: ``` Split up the "init" part of the show_tree() function where we decide what the "type" is, and whether we'll return early. This makes things a bit less readable for now, but we'll soon re-use this in a sibling function, and avoiding the duplication will be worth it. Signed-off-by: Ævar Arnfjörð Bjarmason --- builtin/ls-tree.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index eecc7482d54..df8312408da 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c @@ -61,25 +61,33 @@ static int show_recursive(const char *base, size_t baselen, const char *pathname return 0; } -static int show_tree(const struct object_id *oid, struct strbuf *base, - const char *pathname, unsigned mode, void *context) +static int show_tree_init(enum object_type *type, struct strbuf *base, + const char *pathname, unsigned mode, int *retval) { - int retval = 0; - size_t baselen; - enum object_type type = OBJ_BLOB; - if (S_ISGITLINK(mode)) { - type = OBJ_COMMIT; + *type = OBJ_COMMIT; } else if (S_ISDIR(mode)) { if (show_recursive(base->buf, base->len, pathname)) { - retval = READ_TREE_RECURSIVE; + *retval = READ_TREE_RECURSIVE; if (!(ls_options & LS_SHOW_TREES)) - return retval; + return 1; } - type = OBJ_TREE; + *type = OBJ_TREE; } else if (ls_options & LS_TREE_ONLY) - return 0; + return 1; + return 0; +} + +static int show_tree(const struct object_id *oid, struct strbuf *base, + const char *pathname, unsigned mode, void *context) +{ + int retval = 0; + size_t baselen; + enum object_type type = OBJ_BLOB; + + if (show_tree_init(&type, base, pathname, mode, &retval)) + return retval; if (!(ls_options & LS_NAME_ONLY)) { if (ls_options & LS_SHOW_SIZE) { -- 2.34.1.1119.g7a3fc8778ee ``` ## Ævar Arnfjörð Bjarmason, 2021-12-17 13:30 Subject: [RFC PATCH 7/7] ls-tree.c: support `--object-only` option for "git-ls-tree" Message-ID: URL: https://gitlist.dev/e/RFC-patch-7.7-5e34df4f8dd-20211217T131635Z-avarab%40gmail.com In-Reply-To: ``` From: Teng Long We usually pipe the output from `git ls-trees` to tools like `sed` or `cut` when we only want to extract some fields. When we want only the pathname component, we can pass `--name-only` option to omit such a pipeline, but there are no options for extracting other fields. Teach the "--object-only" option to the command to only show the object name. This option cannot be used together with "--name-only" or "--long" (mutually exclusive). Signed-off-by: Teng Long Signed-off-by: Ævar Arnfjörð Bjarmason --- Documentation/git-ls-tree.txt | 7 ++++- builtin/ls-tree.c | 6 +++++ t/t3103-ls-tree-misc.sh | 8 ++++++ t/t3104-ls-tree-oid.sh | 51 +++++++++++++++++++++++++++++++++++ 4 files changed, 71 insertions(+), 1 deletion(-) create mode 100755 t/t3104-ls-tree-oid.sh diff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt index db02d6d79a9..729370f2357 100644 --- a/Documentation/git-ls-tree.txt +++ b/Documentation/git-ls-tree.txt @@ -10,7 +10,7 @@ SYNOPSIS -------- [verse] 'git ls-tree' [-d] [-r] [-t] [-l] [-z] - [--name-only] [--name-status] [--full-name] [--full-tree] [--abbrev[=]] + [--name-only] [--name-status] [--object-only] [--full-name] [--full-tree] [--abbrev[=]] [...] DESCRIPTION @@ -59,6 +59,11 @@ OPTIONS --name-only:: --name-status:: List only filenames (instead of the "long" output), one per line. + Cannot be combined with `--object-only`. + +--object-only:: + List only names of the objects, one per line. Cannot be combined + with `--name-only` or `--name-status`. --abbrev[=]:: Instead of showing the full 40-byte hexadecimal object diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index efd85cab088..f19b0138362 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c @@ -20,6 +20,7 @@ static int line_termination = '\n'; #define LS_SHOW_TREES 4 #define LS_NAME_ONLY 8 #define LS_SHOW_SIZE 16 +#define LS_OBJECT_ONLY 32 static int abbrev; static int ls_options; static struct pathspec pathspec; @@ -31,6 +32,7 @@ static const char *ls_tree_prefix; */ static const char *ls_tree_format_d = "%(objectmode) %(objecttype) %(objectname)%x09%(path)"; static const char *ls_tree_format_l = "%(objectmode) %(objecttype) %(objectname) %(objectsize:padded)%x09%(path)"; +static const char *ls_tree_format_o = "%(objectname)"; static const char *ls_tree_format_n = "%(path)"; static const char * const ls_tree_usage[] = { @@ -271,6 +273,8 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) LS_NAME_ONLY), OPT_BIT(0, "name-status", &ls_options, N_("list only filenames"), LS_NAME_ONLY), + OPT_BIT(0, "object-only", &ls_options, N_("list only objects"), + LS_OBJECT_ONLY), OPT_SET_INT(0, "full-name", &chomp_prefix, N_("use full path names"), 0), OPT_BOOL(0, "full-tree", &full_tree, @@ -302,6 +306,8 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) implicit_format = ls_tree_format_n; if (ls_options & LS_SHOW_SIZE) implicit_format = ls_tree_format_l; + if (ls_options & LS_OBJECT_ONLY) + implicit_format = ls_tree_format_o; if (format && implicit_format) usage_msg_opt(_("providing --format cannot be combined with other format-altering options"), diff --git a/t/t3103-ls-tree-misc.sh b/t/t3103-ls-tree-misc.sh index d18ba1bd84b..a8641706a6e 100755 --- a/t/t3103-ls-tree-misc.sh +++ b/t/t3103-ls-tree-misc.sh @@ -23,4 +23,12 @@ test_expect_success 'ls-tree fails with non-zero exit code on broken tree' ' test_must_fail git ls-tree -r HEAD ' +test_expect_success 'usage: incompatible options: --name-status with --long' ' + test_expect_code 129 git ls-tree --long --name-status +' + +test_expect_success 'usage: incompatible options: --name-only with --long' ' + test_expect_code 129 git ls-tree --long --name-only +' + test_done diff --git a/t/t3104-ls-tree-oid.sh b/t/t3104-ls-tree-oid.sh new file mode 100755 index 00000000000..81304e7b13a --- /dev/null +++ b/t/t3104-ls-tree-oid.sh @@ -0,0 +1,51 @@ +#!/bin/sh + +test_description='git ls-tree objects handling.' + +. ./test-lib.sh + +test_expect_success 'setup' ' + test_commit A && + test_commit B && + mkdir -p C && + test_commit C/D.txt && + find *.txt path* \( -type f -o -type l \) -print | + xargs git update-index --add && + tree=$(git write-tree) && + echo $tree +' + +test_expect_success 'usage: --object-only' ' + git ls-tree --object-only $tree >current && + git ls-tree $tree >result && + cut -f1 result | cut -d " " -f3 >expected && + test_cmp current expected +' + +test_expect_success 'usage: --object-only with -r' ' + git ls-tree --object-only -r $tree >current && + git ls-tree -r $tree >result && + cut -f1 result | cut -d " " -f3 >expected && + test_cmp current expected +' + +test_expect_success 'usage: --object-only with --abbrev' ' + git ls-tree --object-only --abbrev=6 $tree >current && + git ls-tree --abbrev=6 $tree >result && + cut -f1 result | cut -d " " -f3 >expected && + test_cmp current expected +' + +test_expect_success 'usage: incompatible options: --name-only with --object-only' ' + test_expect_code 129 git ls-tree --object-only --name-only +' + +test_expect_success 'usage: incompatible options: --name-status with --object-only' ' + test_expect_code 129 git ls-tree --object-only --name-status +' + +test_expect_success 'usage: incompatible options: --long with --object-only' ' + test_expect_code 129 git ls-tree --object-only --long +' + +test_done -- 2.34.1.1119.g7a3fc8778ee ```