Re: [PATCH v5 1/1] ls-tree.c: support `--object-only` option for "git-ls-tree"
From: Teng Long <hidden>
Date: 2021-12-16 12:16:38
I notice that this changed from --oid to --object and I agree that
it would probably be more friendly to end users. In fact, this
$ sed -ne '/^SYNOPSIS/,/^DESCRIPTION/p' Documentation/git-*.txt |
grep -e -oid
did not find any hits.Thank you for comfirming that.
It usually is a good idea to enclose these in () so that they are safe to use in any context in a statement. Luckily, bitwise-or and bitwise-and, which are the most likely candidates for these symbols to be used with, bind looser than left-shift, so something like if ((LS_TREE_ONLY | LS_SHOW_TREES) & opt) ... do this ... is safe either way, but (LS_TREE_ONLY + LS_SHOW_TREES) would have different value with and without () around (1 << N).
Make sense. Will fix in patch v6.
Style: we do not initialize statics explicitly to zero. ... Likewise. It is a bit curious to see these listed in decreasing order, though.
Will fix.
It is a good idea to leave a comma even after the last element, _unless_ there is a strong reason why the element that currently is at the last MUST stay to be last when new elements are added to the enum. That way, a future patch that adds a new element can add it to the list with a patch noise with fewer lines.
Very clear explanation. Will fix.
A better name, anybody? This bit is to keep track of the fact that we made _some_ output already so any further output needs an inter-field space before writing what it wants to write out.
I found a word "interspace", it looks like a little better than the old one. I will rename to it in next patch, and If there's a better idea, will apply further.
SP before ','.
Will fix.
Curious. I wonder if we can get rid of these two lines (and the line_termination bit in the SHOW_FILE_NAME part), and have an unconditional putchar(line_termination); at the end of the function. That way, we could in the future choose to introduce a feature to show only <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.