Re: [PATCH 2/2] attr: add flag `-r|--revisions` to work with revisions

[Junio C Hamano <gitster@pobox.com>]

Karthik Nayak <karthik.188@gmail.com> 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.

As to the C API, please do not append the new parameter at the end.
When there are no logical ordering among the things listed, be it in
the members of a struct or the parameters to a function, we encourage
to append, but in this case

    void git_check_attr(struct index_state *istate,
                        const char *path,
                        struct attr_check *check)

we are saying "pick <path>, referring to .gitattributes files from
the index as needed, and apply the checks in check[]", and the new
behaviour is "pick <path>, referring to .gitattributes files from
the tree-ish as needed, and do the same", so istate and the tree-ish
object should sit together.

Also, at the C API level, I suspect that we strongly prefer to pass
around either the "struct object_id *" or "struct tree *", not working
with end-user supplied character strings without any sanity-checking
or parsing.

Another concern, among many existing callers of git_check_attr(),
is there any that will conceivably benefit in the future if they
could read the attributes from a tree-ish?  If not, it may make a
lot more sense if you did not butcher them, and instead introduce a
new API function git_check_attr_from_tree() and use it in the only
place you handle the "-r tree-ish" command line option in the
updated part of the "git check-attr" command.

Thanks.