On Wed, Dec 7, 2022 at 1:12 AM Junio C Hamano <gitster@xxxxxxxxx> 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 understand this concern, but I do think it would be really useful. I see that Taylor and Brian have also mentioned how this would be useful. As such, I think it's a good indication to take this forward. > 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. > I will be more cognizant of this approach and make these changes. > 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. > I must admit, I did take the path of least resistance here. So we finally need to parse the `revision:<pathname>` where the `<pathname>` is generated dynamically as we move through the check-attr stack. My question is, if I generate an `object_id` at the start (in builtin/check-attr.c) with only the `revision`, is there a way to traverse to find the blob for each of the different <pathnames>? I haven't looked at Git code for a while now, and I'm not sure what the best way to do this. Maybe I've missed some API which would make this simple, any help is appreciated. > 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. > The concern then is that we'll also have to duplicate some of the code in `git_all_attrs` and `git_check_attr` and have some logic to work correctly for with and without the `-a` option. All in all, this was much easier. I don't have enough knowledge to say if the other callers will someday want to extend to providing a revision too, so I'll leave that to you! > Thanks. Thanks for the review. -- - Karthik
