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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Dec 06 2022, Karthik Nayak wrote:

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

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.

> +	if (buf == NULL)

if (!buf)

> +		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?

> +	} else if (object_name != NULL) {

else if (object_name)

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