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

From: Teng Long <dyroneteng@gmail.com>
Date: Dec 16, 2021, 12:16 UTC
Message-ID: <20211216121630.14553-1-dyroneteng@gmail.com>
In-Reply-To: <xmqqk0g5656r.fsf@gitster.g>

Show 7 quoted lines

> I notice that this changed from --oid to --object and I agree that
> it would probably be more friendly to end users.  In fact, this
>
>    $ sed -ne '/^SYNOPSIS/,/^DESCRIPTION/p' Documentation/git-*.txt |
>      grep -e -oid
>
> did not find any hits.

Thank you for comfirming that.

Show 12 quoted lines

> It usually is a good idea to enclose these in () so that they are
> safe to use in any context in a statement.
> 
> Luckily, bitwise-or and bitwise-and, which are the most likely
> candidates for these symbols to be used with, bind looser than
> left-shift, so something like 
> 
> 	if ((LS_TREE_ONLY | LS_SHOW_TREES) & opt)
> 		... do this ...
> 
> is safe either way, but (LS_TREE_ONLY + LS_SHOW_TREES) would have
> different value with and without () around (1 << N).

Make sense.

Will fix in patch v6.

> Style: we do not initialize statics explicitly to zero.
> ...
> Likewise.  It is a bit curious to see these listed in decreasing
> order, though.

Will fix.

Show 5 quoted lines

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

Very clear explanation.

Will fix.

Show 5 quoted lines

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

I found a word "interspace", it looks like a little better than the old one. I will rename to it in next patch, and If there's a better idea, will apply further.

> SP before ','.

Will fix.

Show 16 quoted lines

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

Let me try to represent to make sure I understand your suggestion sufficiently.

"write_name_quoted_relative" is used to compute the relative file name by "prefix" and output the name and a line termination to the given FD.

We do not want use "write_name_quoted_relative" in here because the function alway output a line termination after "name", this may bring some inconvenience because the "name" may not be the last field in the future.

So, instead:

We need to calculate the file name (relative path and quotes if need) without "write_name_quoted_relative" and then output the line termination before return.

Thanks.