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.
