On 23/06/2022 16:57, Junio C Hamano wrote:
Phillip Wood <phillip.wood123@xxxxxxxxx> writes:
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.
Thanks for raising these issues. I agree with you on many of them.
In addition to what you covered ....
+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"
I think we would call this "name". The name of the existing option
that controls how they are shown is "--full-name", not "--full-path",
for example.
That's a good point, also I've just noticed that this is another case
where there is a separator character is printed automatically when the
format string is expanded. I think it is probably right to format the
name based on whether or not -z was passed but we should leave it up to
the user to supply a delimiter in the format string.
+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.
+mtime::
+ The modified time of file which is in the index.
These are only the low-bits of the full timestamp, not ctime/mtime
themselves.
But stepping back a bit, why do we need to include them in the
output? What workflow and use case are we trying to help? Dump
output from "stat <path>" equivalent from ls-files and compare with
"stat ." output to see which ones are stale? Or is there any value
to see the value of, say, ctime as an individual data item?
+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.
Again, why do we need to include these in the output?
Wouldn't it be sufficient, as well as a lot more useful, to show a
single bit "the cached stat info matches what is in the working tree
(yes/no)"?
That does sound useful
+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
I am guessing that most of the above are only useful for curious
geeks and those who are debugging their new tweak to the code that
touches the index, i.e. a debugging feature. But these folks can
run "git" under a debugger, and they probably have to do so when
they are seeing an unexpected value in the flags member of a cache
entry anyway. So I am not sure whom this field is intended to help.
I wondered about that as well, but thought there might be a plausible
use if someone wants to check if an entry is marked intent-to-add, or
has the skip-worktree/spare-index bits set (are there other ways to
inspect those?)
Best Wishes
Phillip
