git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/3] support `--oid-only` in `ls-tree`
@ 2021-11-15 11:51 Teng Long
  2021-11-15 11:51 ` [PATCH 1/3] ls-tree.c: support `--oid-only` option for "git-ls-tree" Teng Long
                   ` (5 more replies)
  0 siblings, 6 replies; 38+ messages in thread
From: Teng Long @ 2021-11-15 11:51 UTC (permalink / raw)
  To: git; +Cc: gitster, peff, Teng Long

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


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH 1/3] ls-tree.c: support `--oid-only` option for "git-ls-tree"
  2021-11-15 11:51 [PATCH 0/3] support `--oid-only` in `ls-tree` Teng Long
@ 2021-11-15 11:51 ` Teng Long
  2021-11-15 15:12   ` Ævar Arnfjörð Bjarmason
  2021-11-15 19:16   ` Jeff King
  2021-11-15 11:51 ` [PATCH 2/3] t3104: add related tests for `--oid-only` option Teng Long
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 38+ messages in thread
From: Teng Long @ 2021-11-15 11:51 UTC (permalink / raw)
  To: git; +Cc: gitster, peff, Teng Long

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 <dyroneteng@gmail.com>
---
 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


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH 2/3] t3104: add related tests for `--oid-only` option
  2021-11-15 11:51 [PATCH 0/3] support `--oid-only` in `ls-tree` Teng Long
  2021-11-15 11:51 ` [PATCH 1/3] ls-tree.c: support `--oid-only` option for "git-ls-tree" Teng Long
@ 2021-11-15 11:51 ` Teng Long
  2021-11-15 15:54   ` Đoàn Trần Công Danh
  2021-11-15 11:51 ` [PATCH 3/3] git-ls-tree.txt: description of the 'oid-only' option Teng Long
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 38+ messages in thread
From: Teng Long @ 2021-11-15 11:51 UTC (permalink / raw)
  To: git; +Cc: gitster, peff, Teng Long

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 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


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH 3/3] git-ls-tree.txt: description of the 'oid-only' option
  2021-11-15 11:51 [PATCH 0/3] support `--oid-only` in `ls-tree` Teng Long
  2021-11-15 11:51 ` [PATCH 1/3] ls-tree.c: support `--oid-only` option for "git-ls-tree" Teng Long
  2021-11-15 11:51 ` [PATCH 2/3] t3104: add related tests for `--oid-only` option Teng Long
@ 2021-11-15 11:51 ` Teng Long
  2021-11-15 15:13 ` [PATCH 0/3] support `--oid-only` in `ls-tree` Ævar Arnfjörð Bjarmason
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 38+ messages in thread
From: Teng Long @ 2021-11-15 11:51 UTC (permalink / raw)
  To: git; +Cc: gitster, peff, Teng Long

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 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[=<n>]]
+	    [--name-only] [--name-status] [--oid-only]
+	    [--full-name] [--full-tree] [--abbrev[=<n>]]
 	    <tree-ish> [<path>...]
 
 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[=<n>]::
 	Instead of showing the full 40-byte hexadecimal object
 	lines, show the shortest prefix that is at least '<n>'
-- 
2.33.1.9.g5fbd2fc599.dirty


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 1/3] ls-tree.c: support `--oid-only` option for "git-ls-tree"
  2021-11-15 11:51 ` [PATCH 1/3] ls-tree.c: support `--oid-only` option for "git-ls-tree" Teng Long
@ 2021-11-15 15:12   ` Ævar Arnfjörð Bjarmason
  2021-11-18  9:28     ` Teng Long
  2021-11-15 19:16   ` Jeff King
  1 sibling, 1 reply; 38+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-15 15:12 UTC (permalink / raw)
  To: Teng Long; +Cc: git, gitster, peff


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 <dyroneteng@gmail.com>
> ---
>  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/

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 0/3] support `--oid-only` in `ls-tree`
  2021-11-15 11:51 [PATCH 0/3] support `--oid-only` in `ls-tree` Teng Long
                   ` (2 preceding siblings ...)
  2021-11-15 11:51 ` [PATCH 3/3] git-ls-tree.txt: description of the 'oid-only' option Teng Long
@ 2021-11-15 15:13 ` Ævar Arnfjörð Bjarmason
  2021-11-15 19:09   ` Jeff King
  2021-11-15 19:23 ` Jeff King
  2021-11-19 12:09 ` [PATCH v2 0/1] " Teng Long
  5 siblings, 1 reply; 38+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-15 15:13 UTC (permalink / raw)
  To: Teng Long; +Cc: git, gitster, peff


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:

    <mode> SP <type> SP <object> TAB <file>

Or, with -l:

    <mode> SP <type> SP <object> SP <object size> TAB <file>

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

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 2/3] t3104: add related tests for `--oid-only` option
  2021-11-15 11:51 ` [PATCH 2/3] t3104: add related tests for `--oid-only` option Teng Long
@ 2021-11-15 15:54   ` Đoàn Trần Công Danh
  2021-11-18  8:45     ` Teng Long
  0 siblings, 1 reply; 38+ messages in thread
From: Đoàn Trần Công Danh @ 2021-11-15 15:54 UTC (permalink / raw)
  To: Teng Long; +Cc: git, gitster, peff

On 2021-11-15 19:51:52+0800, Teng Long <dyroneteng@gmail.com> wrote:
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
>  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

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 0/3] support `--oid-only` in `ls-tree`
  2021-11-15 15:13 ` [PATCH 0/3] support `--oid-only` in `ls-tree` Ævar Arnfjörð Bjarmason
@ 2021-11-15 19:09   ` Jeff King
  2021-11-15 21:50     ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 38+ messages in thread
From: Jeff King @ 2021-11-15 19:09 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Teng Long, git, gitster

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

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 1/3] ls-tree.c: support `--oid-only` option for "git-ls-tree"
  2021-11-15 11:51 ` [PATCH 1/3] ls-tree.c: support `--oid-only` option for "git-ls-tree" Teng Long
  2021-11-15 15:12   ` Ævar Arnfjörð Bjarmason
@ 2021-11-15 19:16   ` Jeff King
  2021-11-15 19:25     ` Jeff King
  2021-11-18 11:23     ` Teng Long
  1 sibling, 2 replies; 38+ messages in thread
From: Jeff King @ 2021-11-15 19:16 UTC (permalink / raw)
  To: Teng Long; +Cc: git, gitster

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

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 0/3] support `--oid-only` in `ls-tree`
  2021-11-15 11:51 [PATCH 0/3] support `--oid-only` in `ls-tree` Teng Long
                   ` (3 preceding siblings ...)
  2021-11-15 15:13 ` [PATCH 0/3] support `--oid-only` in `ls-tree` Ævar Arnfjörð Bjarmason
@ 2021-11-15 19:23 ` Jeff King
  2021-11-19 12:09 ` [PATCH v2 0/1] " Teng Long
  5 siblings, 0 replies; 38+ messages in thread
From: Jeff King @ 2021-11-15 19:23 UTC (permalink / raw)
  To: Teng Long; +Cc: git, gitster

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

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 1/3] ls-tree.c: support `--oid-only` option for "git-ls-tree"
  2021-11-15 19:16   ` Jeff King
@ 2021-11-15 19:25     ` Jeff King
  2021-11-18 11:23     ` Teng Long
  1 sibling, 0 replies; 38+ messages in thread
From: Jeff King @ 2021-11-15 19:25 UTC (permalink / raw)
  To: Teng Long; +Cc: git, gitster

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

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 0/3] support `--oid-only` in `ls-tree`
  2021-11-15 19:09   ` Jeff King
@ 2021-11-15 21:50     ` Ævar Arnfjörð Bjarmason
  2021-11-19  2:57       ` Teng Long
  0 siblings, 1 reply; 38+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-15 21:50 UTC (permalink / raw)
  To: Jeff King; +Cc: Teng Long, git, gitster


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);
 }

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 2/3] t3104: add related tests for `--oid-only` option
  2021-11-15 15:54   ` Đoàn Trần Công Danh
@ 2021-11-18  8:45     ` Teng Long
  0 siblings, 0 replies; 38+ messages in thread
From: Teng Long @ 2021-11-18  8:45 UTC (permalink / raw)
  To: congdanhqx; +Cc: dyroneteng, git, gitster, peff


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.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 1/3] ls-tree.c: support `--oid-only` option for "git-ls-tree"
  2021-11-15 15:12   ` Ævar Arnfjörð Bjarmason
@ 2021-11-18  9:28     ` Teng Long
  2021-11-18 11:00       ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 38+ messages in thread
From: Teng Long @ 2021-11-18  9:28 UTC (permalink / raw)
  To: avarab; +Cc: dyroneteng, git, gitster, peff


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

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 1/3] ls-tree.c: support `--oid-only` option for "git-ls-tree"
  2021-11-18  9:28     ` Teng Long
@ 2021-11-18 11:00       ` Ævar Arnfjörð Bjarmason
  0 siblings, 0 replies; 38+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-18 11:00 UTC (permalink / raw)
  To: Teng Long; +Cc: git, gitster, peff


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

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 1/3] ls-tree.c: support `--oid-only` option for "git-ls-tree"
  2021-11-15 19:16   ` Jeff King
  2021-11-15 19:25     ` Jeff King
@ 2021-11-18 11:23     ` Teng Long
  1 sibling, 0 replies; 38+ messages in thread
From: Teng Long @ 2021-11-18 11:23 UTC (permalink / raw)
  To: peff; +Cc: dyroneteng, git, gitster

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.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 0/3] support `--oid-only` in `ls-tree`
  2021-11-15 21:50     ` Ævar Arnfjörð Bjarmason
@ 2021-11-19  2:57       ` Teng Long
  0 siblings, 0 replies; 38+ messages in thread
From: Teng Long @ 2021-11-19  2:57 UTC (permalink / raw)
  To: avarab; +Cc: dyroneteng, git, gitster, peff

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

^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH v2 0/1] support `--oid-only` in `ls-tree`
  2021-11-15 11:51 [PATCH 0/3] support `--oid-only` in `ls-tree` Teng Long
                   ` (4 preceding siblings ...)
  2021-11-15 19:23 ` Jeff King
@ 2021-11-19 12:09 ` Teng Long
  2021-11-19 12:09   ` [PATCH v2 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree" Teng Long
  2021-11-22  8:07   ` [PATCH v3 0/1] ls-tree.c: support `--oid-only` option Teng Long
  5 siblings, 2 replies; 38+ messages in thread
From: Teng Long @ 2021-11-19 12:09 UTC (permalink / raw)
  To: git; +Cc: gitster, peff, avarab, congdanhqx, Teng Long


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


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH v2 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree"
  2021-11-19 12:09 ` [PATCH v2 0/1] " Teng Long
@ 2021-11-19 12:09   ` Teng Long
  2021-11-19 13:30     ` Ævar Arnfjörð Bjarmason
  2021-11-22  8:07   ` [PATCH v3 0/1] ls-tree.c: support `--oid-only` option Teng Long
  1 sibling, 1 reply; 38+ messages in thread
From: Teng Long @ 2021-11-19 12:09 UTC (permalink / raw)
  To: git; +Cc: gitster, peff, avarab, congdanhqx, Teng Long

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 <peff@peff.net>
Reviewed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Reviewed-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 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[=<n>]]
+	    [--name-only] [--name-status] [--oid-only]
+	    [--full-name] [--full-tree] [--abbrev[=<n>]]
 	    <tree-ish> [<path>...]
 
 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[=<n>]::
 	Instead of showing the full 40-byte hexadecimal object
 	lines, show the shortest prefix that is at least '<n>'
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 [<options>] <tree-ish> [<path>...]"),
 	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


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree"
  2021-11-19 12:09   ` [PATCH v2 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree" Teng Long
@ 2021-11-19 13:30     ` Ævar Arnfjörð Bjarmason
  2021-11-19 17:32       ` Junio C Hamano
  2021-11-22  7:45       ` Teng Long
  0 siblings, 2 replies; 38+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-19 13:30 UTC (permalink / raw)
  To: Teng Long; +Cc: git, gitster, peff, congdanhqx


On Fri, Nov 19 2021, Teng Long wrote:

> Reviewed-by: Jeff King <peff@peff.net>
> Reviewed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Reviewed-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>

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

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree"
  2021-11-19 13:30     ` Ævar Arnfjörð Bjarmason
@ 2021-11-19 17:32       ` Junio C Hamano
  2021-11-22  7:45       ` Teng Long
  1 sibling, 0 replies; 38+ messages in thread
From: Junio C Hamano @ 2021-11-19 17:32 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Teng Long, git, peff, congdanhqx

Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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.


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree"
  2021-11-19 13:30     ` Ævar Arnfjörð Bjarmason
  2021-11-19 17:32       ` Junio C Hamano
@ 2021-11-22  7:45       ` Teng Long
  2021-11-22 11:14         ` Ævar Arnfjörð Bjarmason
  1 sibling, 1 reply; 38+ messages in thread
From: Teng Long @ 2021-11-22  7:45 UTC (permalink / raw)
  To: avarab; +Cc: congdanhqx, dyroneteng, git, gitster, peff


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.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH v3 0/1] ls-tree.c: support `--oid-only` option
  2021-11-19 12:09 ` [PATCH v2 0/1] " Teng Long
  2021-11-19 12:09   ` [PATCH v2 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree" Teng Long
@ 2021-11-22  8:07   ` Teng Long
  2021-11-22  8:07     ` [PATCH v3 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree" Teng Long
  2021-11-23  4:58     ` [PATCH v4 0/1] ls-tree.c: support `--oid-only` option Teng Long
  1 sibling, 2 replies; 38+ messages in thread
From: Teng Long @ 2021-11-22  8:07 UTC (permalink / raw)
  To: git; +Cc: avarab, congdanhqx, gitster, peff, Teng Long

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 <peff@peff.net>
    -    Reviewed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    -    Reviewed-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
         Signed-off-by: Teng Long <dyroneteng@gmail.com>
     
      ## Documentation/git-ls-tree.txt ##
-- 
2.33.1.10.g438dd9044d.dirty


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH v3 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree"
  2021-11-22  8:07   ` [PATCH v3 0/1] ls-tree.c: support `--oid-only` option Teng Long
@ 2021-11-22  8:07     ` Teng Long
  2021-11-22 18:11       ` Peter Baumann
                         ` (2 more replies)
  2021-11-23  4:58     ` [PATCH v4 0/1] ls-tree.c: support `--oid-only` option Teng Long
  1 sibling, 3 replies; 38+ messages in thread
From: Teng Long @ 2021-11-22  8:07 UTC (permalink / raw)
  To: git; +Cc: avarab, congdanhqx, gitster, peff, Teng Long

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 <dyroneteng@gmail.com>
---
 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[=<n>]]
+	    [--name-only] [--name-status] [--oid-only]
+	    [--full-name] [--full-tree] [--abbrev[=<n>]]
 	    <tree-ish> [<path>...]
 
 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[=<n>]::
 	Instead of showing the full 40-byte hexadecimal object
 	lines, show the shortest prefix that is at least '<n>'
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 [<options>] <tree-ish> [<path>...]"),
 	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


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree"
  2021-11-22  7:45       ` Teng Long
@ 2021-11-22 11:14         ` Ævar Arnfjörð Bjarmason
  0 siblings, 0 replies; 38+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-22 11:14 UTC (permalink / raw)
  To: Teng Long; +Cc: congdanhqx, git, gitster, peff


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




^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree"
  2021-11-22  8:07     ` [PATCH v3 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree" Teng Long
@ 2021-11-22 18:11       ` Peter Baumann
  2021-11-22 18:54       ` Junio C Hamano
  2021-11-23  0:14       ` Đoàn Trần Công Danh
  2 siblings, 0 replies; 38+ messages in thread
From: Peter Baumann @ 2021-11-22 18:11 UTC (permalink / raw)
  To: Teng Long
  Cc: git, Ævar Arnfjörð Bjarmason, congdanhqx,
	Junio C Hamano, Jeff King

[ 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 <dyroneteng@gmail.com> 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 <dyroneteng@gmail.com>
> ---
>  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[=<n>]]
> +           [--name-only] [--name-status] [--oid-only]
> +           [--full-name] [--full-tree] [--abbrev[=<n>]]
>             <tree-ish> [<path>...]
>

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[=<n>]::
>         Instead of showing the full 40-byte hexadecimal object
>         lines, show the shortest prefix that is at least '<n>'
> 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 [<options>] <tree-ish> [<path>...]"),
>         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
>

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree"
  2021-11-22  8:07     ` [PATCH v3 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree" Teng Long
  2021-11-22 18:11       ` Peter Baumann
@ 2021-11-22 18:54       ` Junio C Hamano
  2021-11-23  1:09         ` Ævar Arnfjörð Bjarmason
  2021-11-23  0:14       ` Đoàn Trần Công Danh
  2 siblings, 1 reply; 38+ messages in thread
From: Junio C Hamano @ 2021-11-22 18:54 UTC (permalink / raw)
  To: Teng Long; +Cc: git, avarab, congdanhqx, peff

Teng Long <dyroneteng@gmail.com> 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.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree"
  2021-11-22  8:07     ` [PATCH v3 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree" Teng Long
  2021-11-22 18:11       ` Peter Baumann
  2021-11-22 18:54       ` Junio C Hamano
@ 2021-11-23  0:14       ` Đoàn Trần Công Danh
  2 siblings, 0 replies; 38+ messages in thread
From: Đoàn Trần Công Danh @ 2021-11-23  0:14 UTC (permalink / raw)
  To: Teng Long; +Cc: git, avarab, gitster, peff

On 2021-11-22 16:07:28+0800, Teng Long <dyroneteng@gmail.com> 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 <dyroneteng@gmail.com>
> ---
>  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[=<n>]]
> +	    [--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[=<n>]]
>  	    <tree-ish> [<path>...]
>  
>  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[=<n>]::
>  	Instead of showing the full 40-byte hexadecimal object
>  	lines, show the shortest prefix that is at least '<n>'
> 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 [<options>] <tree-ish> [<path>...]"),
>  	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

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree"
  2021-11-22 18:54       ` Junio C Hamano
@ 2021-11-23  1:09         ` Ævar Arnfjörð Bjarmason
  2021-11-23  1:26           ` Junio C Hamano
  0 siblings, 1 reply; 38+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-23  1:09 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Teng Long, git, congdanhqx, peff


On Mon, Nov 22 2021, Junio C Hamano wrote:

> Teng Long <dyroneteng@gmail.com> 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.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree"
  2021-11-23  1:09         ` Ævar Arnfjörð Bjarmason
@ 2021-11-23  1:26           ` Junio C Hamano
  2021-11-23  2:28             ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 38+ messages in thread
From: Junio C Hamano @ 2021-11-23  1:26 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Teng Long, git, congdanhqx, peff

Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree"
  2021-11-23  1:26           ` Junio C Hamano
@ 2021-11-23  2:28             ` Ævar Arnfjörð Bjarmason
  2021-11-23  2:55               ` Junio C Hamano
  0 siblings, 1 reply; 38+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-23  2:28 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Teng Long, git, congdanhqx, peff


On Mon, Nov 22 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree"
  2021-11-23  2:28             ` Ævar Arnfjörð Bjarmason
@ 2021-11-23  2:55               ` Junio C Hamano
  2021-11-23  3:35                 ` Junio C Hamano
  0 siblings, 1 reply; 38+ messages in thread
From: Junio C Hamano @ 2021-11-23  2:55 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Teng Long, git, congdanhqx, peff

Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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.




^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree"
  2021-11-23  2:55               ` Junio C Hamano
@ 2021-11-23  3:35                 ` Junio C Hamano
  2021-11-23 11:04                   ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 38+ messages in thread
From: Junio C Hamano @ 2021-11-23  3:35 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Teng Long, git, congdanhqx, peff

Junio C Hamano <gitster@pobox.com> writes:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH v4 0/1] ls-tree.c: support `--oid-only` option
  2021-11-22  8:07   ` [PATCH v3 0/1] ls-tree.c: support `--oid-only` option Teng Long
  2021-11-22  8:07     ` [PATCH v3 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree" Teng Long
@ 2021-11-23  4:58     ` Teng Long
  2021-11-23  4:58       ` [PATCH v4 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree" Teng Long
  1 sibling, 1 reply; 38+ messages in thread
From: Teng Long @ 2021-11-23  4:58 UTC (permalink / raw)
  To: git; +Cc: avarab, congdanhqx, gitster, peff, Teng Long

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 <peff@peff.net>
    -    Reviewed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    -    Reviewed-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
         Signed-off-by: Teng Long <dyroneteng@gmail.com>
     
      ## 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[=<n>]]
    -+	    [--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[=<n>]]
      	    <tree-ish> [<path>...]
      
      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[=<n>]::
      	Instead of showing the full 40-byte hexadecimal object
    - 	lines, show the shortest prefix that is at least '<n>'
     
      ## 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


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH v4 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree"
  2021-11-23  4:58     ` [PATCH v4 0/1] ls-tree.c: support `--oid-only` option Teng Long
@ 2021-11-23  4:58       ` Teng Long
  2021-11-23 22:32         ` Junio C Hamano
  0 siblings, 1 reply; 38+ messages in thread
From: Teng Long @ 2021-11-23  4:58 UTC (permalink / raw)
  To: git; +Cc: avarab, congdanhqx, gitster, peff, Teng Long

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 <dyroneteng@gmail.com>
---
 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[=<n>]]
+'git ls-tree' [-d] [-r] [-t] [-l] [-z] [-n] [-s] [-o]
+	    [--name-only | --oid-only]
+	    [--name-status | --oid-only]
+	    [--full-name] [--full-tree] [--abbrev[=<n>]]
 	    <tree-ish> [<path>...]
 
 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[=<n>]::
 	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 [<options>] <tree-ish> [<path>...]"),
 	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


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v3 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree"
  2021-11-23  3:35                 ` Junio C Hamano
@ 2021-11-23 11:04                   ` Ævar Arnfjörð Bjarmason
  0 siblings, 0 replies; 38+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-23 11:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Teng Long, git, congdanhqx, peff


On Mon, Nov 22 2021, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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))

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree"
  2021-11-23  4:58       ` [PATCH v4 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree" Teng Long
@ 2021-11-23 22:32         ` Junio C Hamano
  2021-12-06  7:52           ` Teng Long
  0 siblings, 1 reply; 38+ messages in thread
From: Junio C Hamano @ 2021-11-23 22:32 UTC (permalink / raw)
  To: Teng Long; +Cc: git, avarab, congdanhqx, peff

Teng Long <dyroneteng@gmail.com> 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[=<n>]]
> +'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[=<n>]]
>  	    <tree-ish> [<path>...]
>  
>  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 [<options>] <tree-ish> [<path>...]"),
>  	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
<mode>, <type>, <object-name>, <size>, and <filename> 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.


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree"
  2021-11-23 22:32         ` Junio C Hamano
@ 2021-12-06  7:52           ` Teng Long
  0 siblings, 0 replies; 38+ messages in thread
From: Teng Long @ 2021-12-06  7:52 UTC (permalink / raw)
  To: gitster; +Cc: avarab, congdanhqx, dyroneteng, git, peff

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
> <mode>, <type>, <object-name>, <size>, and <filename> 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.

^ permalink raw reply	[flat|nested] 38+ messages in thread

end of thread, other threads:[~2021-12-06  7:55 UTC | newest]

Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-15 11:51 [PATCH 0/3] support `--oid-only` in `ls-tree` Teng Long
2021-11-15 11:51 ` [PATCH 1/3] ls-tree.c: support `--oid-only` option for "git-ls-tree" Teng Long
2021-11-15 15:12   ` Ævar Arnfjörð Bjarmason
2021-11-18  9:28     ` Teng Long
2021-11-18 11:00       ` Ævar Arnfjörð Bjarmason
2021-11-15 19:16   ` Jeff King
2021-11-15 19:25     ` Jeff King
2021-11-18 11:23     ` Teng Long
2021-11-15 11:51 ` [PATCH 2/3] t3104: add related tests for `--oid-only` option Teng Long
2021-11-15 15:54   ` Đoàn Trần Công Danh
2021-11-18  8:45     ` Teng Long
2021-11-15 11:51 ` [PATCH 3/3] git-ls-tree.txt: description of the 'oid-only' option Teng Long
2021-11-15 15:13 ` [PATCH 0/3] support `--oid-only` in `ls-tree` Ævar Arnfjörð Bjarmason
2021-11-15 19:09   ` Jeff King
2021-11-15 21:50     ` Ævar Arnfjörð Bjarmason
2021-11-19  2:57       ` Teng Long
2021-11-15 19:23 ` Jeff King
2021-11-19 12:09 ` [PATCH v2 0/1] " Teng Long
2021-11-19 12:09   ` [PATCH v2 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree" Teng Long
2021-11-19 13:30     ` Ævar Arnfjörð Bjarmason
2021-11-19 17:32       ` Junio C Hamano
2021-11-22  7:45       ` Teng Long
2021-11-22 11:14         ` Ævar Arnfjörð Bjarmason
2021-11-22  8:07   ` [PATCH v3 0/1] ls-tree.c: support `--oid-only` option Teng Long
2021-11-22  8:07     ` [PATCH v3 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree" Teng Long
2021-11-22 18:11       ` Peter Baumann
2021-11-22 18:54       ` Junio C Hamano
2021-11-23  1:09         ` Ævar Arnfjörð Bjarmason
2021-11-23  1:26           ` Junio C Hamano
2021-11-23  2:28             ` Ævar Arnfjörð Bjarmason
2021-11-23  2:55               ` Junio C Hamano
2021-11-23  3:35                 ` Junio C Hamano
2021-11-23 11:04                   ` Ævar Arnfjörð Bjarmason
2021-11-23  0:14       ` Đoàn Trần Công Danh
2021-11-23  4:58     ` [PATCH v4 0/1] ls-tree.c: support `--oid-only` option Teng Long
2021-11-23  4:58       ` [PATCH v4 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree" Teng Long
2021-11-23 22:32         ` Junio C Hamano
2021-12-06  7:52           ` Teng Long

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).