patch, 2 partscheck-attr: add support to work with revisions

> Git check-attr currently doesn't check the git worktree, it either
> checks the index or the files directly. This means we cannot check the
> attributes for a file against a certain revision.
>
> 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.
>
> We cannot use the `<rev>:<path>` syntax like the one used in `git show`
> because any non-flag parameter before `--` is treated as an attribute
> and any parameter after `--` is treated as a pathname.
>
> This involves creating a new function `read_attr_from_blob`, which given
> the path reads the blob for the path against the provided revision and
> parses the attributes line by line. This function is plugged into
> `read_attr()` function wherein we go through the different attributes.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> Co-authored-by: toon@iotcl.com
> ---
>  archive.c              |   2 +-
>  attr.c                 | 120 ++++++++++++++++++++++++++++-------------
>  attr.h                 |   7 ++-
>  builtin/check-attr.c   |  25 ++++-----
>  builtin/pack-objects.c |   2 +-
>  convert.c              |   2 +-
>  ll-merge.c             |   4 +-
>  pathspec.c             |   2 +-
>  t/t0003-attributes.sh  |  56 ++++++++++++++++++-
>  userdiff.c             |   2 +-
>  ws.c                   |   2 +-
>  11 files changed, 165 insertions(+), 59 deletions(-)
>
> diff --git a/archive.c b/archive.c
> index 941495f5d7..bf64dbdce1 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -120,7 +120,7 @@ static const struct attr_check *get_archive_attrs(struct index_state *istate,
>  	static struct attr_check *check;
>  	if (!check)
>  		check = attr_check_initl("export-ignore", "export-subst", NULL);
> -	git_check_attr(istate, path, check);
> +	git_check_attr(istate, path, check, NULL);
>  	return check;
>  }
>  
> diff --git a/attr.c b/attr.c
> index 42ad6de8c7..42b67a401f 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -11,8 +11,12 @@
>  #include "exec-cmd.h"
>  #include "attr.h"
>  #include "dir.h"
> +#include "git-compat-util.h"

> +	struct object_id oid;
> +	unsigned long sz;
> +	enum object_type type;
> +	void *buf;
> +	struct strbuf sb;
> +
> +	if (object_name == NULL)

> +		return NULL;
> +
> +	strbuf_init(&sb, strlen(path) + 1 + strlen(object_name));
> +	strbuf_addstr(&sb, object_name);
> +	strbuf_addstr(&sb, ":");
> +	strbuf_addstr(&sb, path);

>  	if (collect_all) {
> -		git_all_attrs(&the_index, full_path, check);
> +		git_all_attrs(&the_index, full_path, check, object_name);
>  	} else {
> -		git_check_attr(&the_index, full_path, check);
> +		git_check_attr(&the_index, full_path, check, object_name);
>  	}

> +test_expect_success 'bare repository: with --revision' '
> +	(
> +		cd bare.git &&
> +		(
> +			echo "f	test=f" &&
> +			echo "a/i test=a/i"

> >
> > diff --git a/attr.c b/attr.c
> > index 42ad6de8c7..42b67a401f 100644
> > --- a/attr.c
> > +++ b/attr.c
> > @@ -11,8 +11,12 @@
> >  #include "exec-cmd.h"
> >  #include "attr.h"
> >  #include "dir.h"
> > +#include "git-compat-util.h"
>
> As a rule in this project we include either cache.h at the top, or
> git-compat-util.h, and the former includes the latter.
>
> This file already has cache.h at the top, so it won't need this.
>

>
> > +             more = (*ep == '\n');
>
> Nit: parens not needed.
>

> > +     struct object_id oid;
> > +     unsigned long sz;
> > +     enum object_type type;
> > +     void *buf;
> > +     struct strbuf sb;
> > +
> > +     if (object_name == NULL)
>
> Ditto !object_name test.
>

> > +             return NULL;
> > +
> > +     strbuf_init(&sb, strlen(path) + 1 + strlen(object_name));
> > +     strbuf_addstr(&sb, object_name);
> > +     strbuf_addstr(&sb, ":");
> > +     strbuf_addstr(&sb, path);
>
> Is this really performance sensitive so we need to pre-size this, or
> would a simpler:
>
>         struct strbuf sb = STRBUF_INIT;
>         strbuf_addf(&sb, "%s:%s", path, object_name);
>
> Do?
>

>
> >  void git_check_attr(struct index_state *istate,
> > -                 const char *path, struct attr_check *check);
> > +                                     const char *path, struct attr_check *check,
> > +                                     const char *object_name);
>
> This (and possibly other places) seem funnily indented..
>

> >       if (collect_all) {
> > -             git_all_attrs(&the_index, full_path, check);
> > +             git_all_attrs(&the_index, full_path, check, object_name);
> >       } else {
> > -             git_check_attr(&the_index, full_path, check);
> > +             git_check_attr(&the_index, full_path, check, object_name);
> >       }
>
> Earlier you do a get_oid(), shouldn't that be a
> repo_get_oid(istate->repo, ...) to be future-proof? I.e. use the repo of
> the passed-in index.
>
> I think it'll always be "the_repository" for now, but for an API it
> makes sense to hardcode that assumption in fewer places.
>

> > +test_expect_success 'bare repository: with --revision' '
> > +     (
> > +             cd bare.git &&
> > +             (
> > +                     echo "f test=f" &&
> > +                     echo "a/i test=a/i"
>
> You don't need a subshell just to echo a string. You can use {}-braces,
> but in this case just:
>
>     printf "%s\n", "f test=f" "a/i test=a/i" | git hash-object .... &&
>
>

> > +             ) | git hash-object -w --stdin > id &&
> > +             git update-index --add --cacheinfo 100644 $(cat id) .gitattributes &&
>
> Split the "cat" into a varible, otherwise its failure will be hidden.
>

> > +             git write-tree > id &&
>
> We use ">id" style for redirection, not "> id".
>
> > +             git commit-tree $(cat id) -m "random commit message" > id &&
>
> Ditto..

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

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

>
> On Tue, Dec 6, 2022 at 7:15 PM Junio C Hamano <gitster@pobox.com> wrote:
> > 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.
>
> A few minor comments...
>
> We don't usually squat on short options, such as `-r`, right from the
> start but only add the short alias once shown that there is demand.
>
> Option `-r` has strong association with "recursive" elsewhere, so I'd
> worry about giving it such a completely different meaning here.
>
> Rather than calling the option `--revision`, perhaps pattern it after
> git-restore's `--source` option which allows specifying a particular
> commit, tree, tag, etc.?

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

> On Wed, Dec 7, 2022 at 1:12 AM Junio C Hamano <gitster@pobox.com> wrote:
> [...]
>> 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.

>
>
> On Wed, Dec 07 2022, Karthik Nayak wrote:
>
> > On Wed, Dec 7, 2022 at 1:12 AM Junio C Hamano <gitster@pobox.com> wrote:
> > [...]
> >> 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.
>
> The get_oid() that you're doing now happens in a loop, and should be
> pulled out of it.
>
> I suggested making that a feature in
> https://lore.kernel.org/git/221207.86lenja0zi.gmgdl@evledraar.gmail.com/;
> but if you keep the interface you've got where you only support a single
> <rev> it would make the most sense to do that get_oid() in the builtin/
> code at the start, and then pass the oid/path pair down.
>
> You'd still need to call read_object_file() or equivalent for each
> <rev>:<path> pair though.

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

>
>
> On Wed, Dec 07 2022, Junio C Hamano wrote:
>
> > 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 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.

> Given a pathname, git-check-attr(1) will list the attributes which apply to that
> pathname by reading all relevant gitattributes files. Currently there is no way
> to specify a revision to read the gitattributes from.
>
> This is specifically useful in bare repositories wherein the gitattributes are
> only present in the git working tree but not available directly on the
> filesystem.
>
> This series aims to add a new flag `-r|--revisions` to git-check-attr(1) which
> allows us to read gitattributes from the specified revision.

>
> Karthik Nayak (2):
>   t0003: move setup for `--all` into new block
>   attr: add flag `-r|--revisions` to work with revisions
>
>  archive.c              |   2 +-
>  attr.c                 | 120 ++++++++++++++++++++++++++++-------------
>  attr.h                 |   7 ++-
>  builtin/check-attr.c   |  25 ++++-----
>  builtin/pack-objects.c |   2 +-
>  convert.c              |   2 +-
>  ll-merge.c             |   4 +-
>  pathspec.c             |   2 +-
>  t/t0003-attributes.sh  |  63 ++++++++++++++++++++--
>  userdiff.c             |   2 +-
>  ws.c                   |   2 +-
>  11 files changed, 170 insertions(+), 61 deletions(-)
>