Re: [PATCH v5 1/1] ls-tree.c: support `--object-only` option for "git-ls-tree"

[Junio C Hamano <gitster@pobox.com>]

Teng Long <dyroneteng@gmail.com> writes:

> We usually pipe the output from `git ls-trees` to tools like
> `sed` or `cut` when we only want to extract some fields.
>
> When we want only the pathname component, we can pass
> `--name-only` option to omit such a pipeline, but there are no
> options for extracting other fields.
>
> Teach the "--object-only" option to the command to only show the
> object name. This option cannot be used together with
> "--name-only" or "--long" (mutually exclusive).

I notice that this changed from --oid to --object and I agree that
it would probably be more friendly to end users.  In fact, this

    $ sed -ne '/^SYNOPSIS/,/^DESCRIPTION/p' Documentation/git-*.txt |
      grep -e -oid

did not find any hits.

> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
> index 3a442631c7..beaa8bf13b 100644
> --- a/builtin/ls-tree.c
> +++ b/builtin/ls-tree.c
> @@ -16,21 +16,38 @@
>  
>  static int line_termination = '\n';
>  #define LS_RECURSIVE 1
> -#define LS_TREE_ONLY 2
> -#define LS_SHOW_TREES 4
> -#define LS_NAME_ONLY 8
> -#define LS_SHOW_SIZE 16
> +#define LS_TREE_ONLY 1 << 1
> +#define LS_SHOW_TREES 1 << 2
> +#define LS_NAME_ONLY 1 << 3
> +#define LS_SHOW_SIZE 1 << 4
> +#define LS_OBJECT_ONLY 1 << 5

It usually is a good idea to enclose these in () so that they are
safe to use in any context in a statement.

Luckily, bitwise-or and bitwise-and, which are the most likely
candidates for these symbols to be used with, bind looser than
left-shift, so something like 

	if ((LS_TREE_ONLY | LS_SHOW_TREES) & opt)
		... do this ...

is safe either way, but (LS_TREE_ONLY + LS_SHOW_TREES) would have
different value with and without () around (1 << N).

> +static unsigned int shown_bits = 0;

Style: we do not initialize statics explicitly to zero.

> +#define SHOW_DEFAULT 29 /* 11101 size is not shown to output by default */
> +#define SHOW_MODE 1 << 4
> +#define SHOW_TYPE 1 << 3
> +#define SHOW_OBJECT_NAME 1 << 2
> +#define SHOW_SIZE 1 << 1
> +#define SHOW_FILE_NAME 1

Likewise.  It is a bit curious to see these listed in decreasing
order, though.

>  static const  char * const ls_tree_usage[] = {
>  	N_("git ls-tree [<options>] <tree-ish> [<path>...]"),
>  	NULL
>  };
>  
> +enum {
> +	MODE_UNSPECIFIED = 0,
> +	MODE_NAME_ONLY,
> +	MODE_OBJECT_ONLY,
> +	MODE_LONG

It is a good idea to leave a comma even after the last element,
_unless_ there is a strong reason why the element that currently is
at the last MUST stay to be last when new elements are added to the
enum.  That way, a future patch that adds a new element can add it
to the list with a patch noise with fewer lines.

> +};
> +
> +static int cmdmode = MODE_UNSPECIFIED;

This is also initializing a static variable to zero, and arguments
can be made either way: (A) unspecified is set to zero in enum
definition exactly so that we can use zero to signal the variable is
unspecified, so an explicit zero initialization here goes against
the spirit of choosing 0 as MODE_UNSPECIFIED; or (B) enum definition
can be scrambled in future changes to use something other than zero
for MODE_UNSPECIFIED, and explicitly writing it like this is more
future-proof.

I am OK with the way it is written (i.e. (B)).

> @@ -66,6 +83,7 @@ static int show_tree(const struct object_id *oid, struct strbuf *base,
>  {
>  	int retval = 0;
>  	int baselen;
> +	int follow = 0;

A better name, anybody?

This bit is to keep track of the fact that we made _some_ output
already so any further output needs an inter-field space before
writing what it wants to write out.

>  	const char *type = blob_type;
>  
>  	if (S_ISGITLINK(mode)) {
> @@ -74,8 +92,8 @@ static int show_tree(const struct object_id *oid, struct strbuf *base,
>  		 *
>  		 * Something similar to this incomplete example:
>  		 *
> -		if (show_subprojects(base, baselen, pathname))
> -			retval = READ_TREE_RECURSIVE;
> +		 * if (show_subprojects(base, baselen, pathname))
> +		 *	retval = READ_TREE_RECURSIVE;
>  		 *
>  		 */

Nice ;-)

>  		type = commit_type;
> @@ -90,35 +108,67 @@ static int show_tree(const struct object_id *oid, struct strbuf *base,
>  	else if (ls_options & LS_TREE_ONLY)
>  		return 0;
>  
> -	if (!(ls_options & LS_NAME_ONLY)) {
> -		if (ls_options & LS_SHOW_SIZE) {
> -			char size_text[24];
> -			if (!strcmp(type, blob_type)) {
> -				unsigned long size;
> -				if (oid_object_info(the_repository, oid, &size) == OBJ_BAD)
> -					xsnprintf(size_text, sizeof(size_text),
> -						  "BAD");
> -				else
> -					xsnprintf(size_text, sizeof(size_text),
> -						  "%"PRIuMAX, (uintmax_t)size);
> -			} else
> -				xsnprintf(size_text, sizeof(size_text), "-");
> -			printf("%06o %s %s %7s\t", mode, type,
> -			       find_unique_abbrev(oid, abbrev),
> -			       size_text);
> +	if (shown_bits & SHOW_MODE) {
> +		printf("%06o",mode);

SP before ','.

> +		follow = 1;
> +	}
> +	if (shown_bits & SHOW_TYPE) {
> +		printf("%s%s", follow == 1 ? " " : "", type);
> +		follow = 1;
> +	}
> +	if (shown_bits & SHOW_OBJECT_NAME) {
> +		printf("%s%s", follow == 1 ? " " : "",
> +		       find_unique_abbrev(oid, abbrev));
> +		if (!(shown_bits ^ SHOW_OBJECT_NAME))
> +			printf("%c", line_termination);

Curious.  I wonder if we can get rid of these two lines (and the
line_termination bit in the SHOW_FILE_NAME part), and have an
unconditional 

	putchar(line_termination);

at the end of the function.

That way, we could in the future choose to introduce a feature to
show only <mode, type, size> and nothing else, which may be useful
for taking per-type stats.

We need to stop using write_name_quoted_relative() in SHOW_FILE_NAME
part, because the helper insists that the name written by it must be
at the end of the entry, if we go that route, but it may be a good
change in the longer term.

> +		follow = 1;
> +	}
> +	if (shown_bits & SHOW_SIZE) {
> +		char size_text[24];
> +		if (!strcmp(type, blob_type)) {
> +			unsigned long size;
> +			if (oid_object_info(the_repository, oid, &size) == OBJ_BAD)
> +				xsnprintf(size_text, sizeof(size_text), "BAD");
> +			else
> +				xsnprintf(size_text, sizeof(size_text),
> +					  "%"PRIuMAX, (uintmax_t)size);
>  		} else
> -			printf("%06o %s %s\t", mode, type,
> -			       find_unique_abbrev(oid, abbrev));
> +			xsnprintf(size_text, sizeof(size_text), "-");
> +		printf("%s%7s", follow == 1 ? " " : "", size_text);
> +		follow = 1;
> +	}
> +	if (shown_bits & SHOW_FILE_NAME) {
> +		if (follow)
> +			printf("\t");
> +		baselen = base->len;
> +		strbuf_addstr(base, pathname);
> +		write_name_quoted_relative(base->buf,
> +					   chomp_prefix ? ls_tree_prefix : NULL,
> +					   stdout, line_termination);
> +		strbuf_setlen(base, baselen);
>  	}

But the above nits aside, the updated organization of this function
looks much cleaner than the original.  Nicely reorganized.

> -	baselen = base->len;
> -	strbuf_addstr(base, pathname);
> -	write_name_quoted_relative(base->buf,
> -				   chomp_prefix ? ls_tree_prefix : NULL,
> -				   stdout, line_termination);
> -	strbuf_setlen(base, baselen);
>  	return retval;
>  }

Thanks.