Re: [PATCH v3] ls-files: introduce "--format" option
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Jun 23, 2022, 14:06 UTC
- Message-ID
- <080f65b3-91f5-7b68-4235-4bfb956c8321@gmail.com>
- In-Reply-To
- <pull.1262.v3.git.1655777140231.gitgitgadget@gmail.com>
Hi ZheNing
On 21/06/2022 03:05, ZheNing Hu via GitGitGadget wrote:
Show 18 quoted lines
> From: ZheNing Hu <adlternative@gmail.com> > > Add a new option --format that output index enties > informations with custom format, taking inspiration > from the option with the same name in the `git ls-tree` > command. > > --format cannot used with -s, -o, -k, --resolve-undo, > --deduplicate and --debug. > > Signed-off-by: ZheNing Hu <adlternative@gmail.com> > --- > ls-files: introduce "--format" options > > v2->v3: > > 1. remove %(tag) because -t is deprecated, suggested by Phillip. > 2. fix some description of atoms in document, suggested by Phillip..
Thanks for re-rolling, having taken a look a closer look at the tests I'm concerned about the output format for some of the specifiers, see below.
Show 24 quoted lines
> [...] > +It is possible to print in a custom format by using the `--format` > +option, which is able to interpolate different fields using > +a `%(fieldname)` notation. For example, if you only care about the > +"objectname" and "path" fields, you can execute with a specific > +"--format" like > + > + git ls-files --format='%(objectname) %(path)' > + > +FIELD NAMES > +----------- > +Various values from structured fields can be used to interpolate > +into the resulting output. For each outputting line, the following > +names can be used: > + > +objectmode:: > + The mode of the file which is in the index. > +objectname:: > + The name of the file which is in the index. > +stage:: > + The stage of the file which is in the index. > +eol:: > + The <eolinfo> and <eolattr> of files both in the > + index and the work-tree.
Looking at the test for this option I think it needs more work, why should --format arbitrarily append a tab to the end of the output? - the user should be able to specify a separator if they want one as part of the format string. Also I'm not sure why there is so much whitespace in the output.
> +path:: > + The pathname of the file which is in the index.
I think that for all these it might be clearer to say "recorded in the index" rather than "of the file which is in the index"
> +ctime:: > + The create time of file which is in the index.
This is printed with a prefix 'ctime:' (the same applies to the format specifiers below) I think we should omit that and just print the data so the user can choose the format they want.
Show 15 quoted lines
> +mtime:: > + The modified time of file which is in the index. > +dev:: > + The ID of device containing file which is in the index. > +ino:: > + The inode number of file which is in the index. > +uid:: > + The user id of file owner which is in the index. > +gid:: > + The group id of file owner which is in the index. > +size:: > + The size of the file which is in the index. > +flags:: > + The flags of the file in the index which include > + in-memory only flags and some extended on-disk flags.
If %(flags) is going to be useful then I think we need to think about how they are printed and document that. At the moment they are printed as a hexadecimal number which is fine for debugging but probably not going to be useful for something like --format. I think printing documented symbolic names with some kind of separator (a comma maybe) between them is probably more useful
> [...]
> +test_expect_success 'git ls-files --format eol' ' > + printf "i/lf w/lf attr/ \t\n" >expect && > + printf "i/lf w/lf attr/ \t\n" >>expect && > + git ls-files --format="%(eol)" --eol >actual &&
I'm not sure why this is passing --eol as well as --format='%(eol)' - shouldn't that combination of flags be an error?
Best Wishes
Phillip