diff mbox series

[v2,1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree"

Message ID 8b68568d6cbe379d40c61c48bf446eaa88221df5.1637321601.git.dyroneteng@gmail.com (mailing list archive)
State New, archived
Headers show
Series support `--oid-only` in `ls-tree` | expand

Commit Message

Teng Long Nov. 19, 2021, 12:09 p.m. UTC 
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

Comments

Ævar Arnfjörð Bjarmason Nov. 19, 2021, 1:30 p.m. UTC | #1 
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?)
Junio C Hamano Nov. 19, 2021, 5:32 p.m. UTC | #2 
Æ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.
Teng Long Nov. 22, 2021, 7:45 a.m. UTC | #3 
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.
Ævar Arnfjörð Bjarmason Nov. 22, 2021, 11:14 a.m. UTC | #4 
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
    [...]
diff mbox series

Patch

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