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

Karthik Nayak <karthik.188@gmail.com> writes:

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.