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

From: Karthik Nayak <karthik.188@gmail.com>
Date: Dec 7, 2022, 11:40 UTC
Message-ID: <CAOLa=ZS5k=s98Bo9GE+RRa3jtanehL35y-hhLhy1DoM7GyO0cQ@mail.gmail.com>
In-Reply-To: <xmqqedtc842m.fsf@gitster.g>

On Wed, Dec 7, 2022 at 1:12 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 20 quoted lines

>
> 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.
>

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.

Show 15 quoted lines

> 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.

Show 5 quoted lines

> 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.

Show 8 quoted lines

> 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