On Wed, Dec 7, 2022 at 12:48 PM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote: > > > On Wed, Dec 07 2022, Junio C Hamano wrote: > > > Karthik Nayak <karthik.188@xxxxxxxxx> writes: > > > >> Add a new flag `--revision`/`-r` which will allow it work with > >> revisions. This command will now, instead of checking the files/index, > >> try and receive the blob for the given attribute file against the > >> provided revision. The flag overrides checking against the index and > >> filesystem and also works with bare repositories. > > > > As "check-attr" was not invented as a user-facing subcommand but was > > a hack for debugging, I would have minded this change, but these > > days people seem to treat it as if it is just one of the proper > > plumbing commands, the new command line convention bothers me a > > bit. No other command uses --<anything> to signal that what comes > > after it is a rev. > > > > But I do not think of a better alternative without making the > > command line ambiguous, so I'll stop at raising a concern, so that > > others who may be better at UI can come up with one. > > I don't really like it either, but maybe we've backed ourselves into a > corner here. > > But let's look at that. So the command takes: > > git check-attr <attr>... -- <path>... > > Or: > > echo "<path>" | > git check-attr --stdin <attr>... > > So we'd want to specify a <revision>, without making the <attr> or > <path> ambiguous. > > Now, when we map the attributes we go through attr_name_valid(), which > checks that the attribute names are valid. A commentthere says: > > * Attribute name cannot begin with '-' and must consist of > * characters from [-A-Za-z0-9_.]. > > So can't we instead accept: > > git check-attr [<rev>:]<attr>... -- <path>... > > I.e.: > > git check-attr HEAD~:foo -- path > > And it wouldn't be ambiguous because attribute names can't contain ":"? > This would be consistent with e.g. "git show" and "git cat-file", just > with "<attr>" instead of the "<path>". > > This would also mean that we'd support: > > git check-attr HEAD:foo HEAD~:bar HEAD~2:baz > > etc., i.e. the ability to support multiple revision/attribute > pairs. Skimming the currently proposed code there seems to be no good > reason not to support that (we just need to look up other blobs), and it > would allow querying those without spawning N processes. Thanks for this walkthrough, quick question, this wouldn't work with the `-a` condition, right? Current patch series tends to work with/without `-a`. Also, my personal opinion is that when being consistent we need to be fully consistent, i.e. <revision>:<path>, tweaking this slightly to be <revision>:<attr> is worse than breaking consistency. -- - Karthik
