Re: [PATCH v5 2/2] attr: add flag `--source` to work with tree-ish

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jan 11, 2023, 11:30 UTC
Message-ID: <4fc57003-3f29-2f1c-fe18-94b5f9f1bf2b@dunelm.org.uk>
In-Reply-To: <23813496fc73b7e5cb9f09b166e05c9a02bac43c.1671793109.git.karthik.188@gmail.com>

Hi Karthik

On 02/01/2023 11:04, Karthik Nayak wrote:

Show 15 quoted lines

> The contents of the .gitattributes files may evolve over time, but "git
> check-attr" always checks attributes against them in the working tree
> and/or in the index. It may be beneficial to optionally allow the users
> to check attributes taken from a commit other than HEAD against paths.
> 
> Add a new flag `--source` which will allow users to check the
> attributes against a commit (actually any tree-ish would do). When the
> user uses this flag, we go through the stack of .gitattributes files but
> instead of checking the current working tree and/or in the index, we
> check the blobs from the provided tree-ish object. This allows the
> command to also be used in bare repositories.
> 
> Since we use a tree-ish object, the user can pass "--source
> HEAD:subdirectory" and all the attributes will be looked up as if
> subdirectory was the root directory of the repository.

I think changing to --source is a good idea. I've left a few comments below - the tests are broken at the moment. I didn't look very closely at the implementation beyond scanning the range-diff as it looks like there are not any significant changes there.

Show 19 quoted lines

> We cannot simply use the `<rev>:<path>` syntax without the `--source`
> flag, similar to how it is used in `git show` because any non-flag
> parameter before `--` is treated as an attribute and any parameter after
> `--` is treated as a pathname.
> 
> The change involves creating a new function `read_attr_from_blob`, which
> given the path reads the blob for the path against the provided source and
> parses the attributes line by line. This function is plugged into
> `read_attr()` function wherein we go through the stack of attributes
> files.
> 
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> Signed-off-by: Toon Claes <toon@iotcl.com>
> Co-authored-by: toon@iotcl.com
> ---
> -'git check-attr' [-a | --all | <attr>...] [--] <pathname>...
> -'git check-attr' --stdin [-z] [-a | --all | <attr>...]
> +'git check-attr' [--source <tree>] [-a | --all | <attr>...] [--] <pathname>...
> +'git check-attr' --stdin [-z] [--source <tree>] [-a | --all | <attr>...]

I think "<tree>" would be better as "<tree-ish>" (see my comments on the --source option implementation below)

Show 12 quoted lines

>   
>   DESCRIPTION
>   -----------
> @@ -36,6 +36,12 @@ OPTIONS
>   	If `--stdin` is also given, input paths are separated
>   	with a NUL character instead of a linefeed character.
>   
> +--source=<tree>::
> +	Check attributes against the specified tree-ish. Paths provided as part
> +	of the revision will be treated as the root directory. It is common to
> +	specify the source tree by naming a commit, branch or tag associated
> +	with it.

I think it is confusing to keep the reference to "revision" here, we could just drop that sentence.

> -N_("git check-attr [-a | --all | <attr>...] [--] <pathname>..."),
> -N_("git check-attr --stdin [-z] [-a | --all | <attr>...]"),
> +N_("git check-attr [--source <tree>] [-a | --all | <attr>...] [--] <pathname>..."),
> +N_("git check-attr --stdin [-z] [--source <tree>] [-a | --all | <attr>...]"),

I think we should use "<tree-ish>" rather than "<tree>" so it is clear one can specify a commit or tag. That's what "git restore" does.

Show 16 quoted lines

> diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
> index b3aabb8aa3..78e9f47dbf 100755
> --- a/t/t0003-attributes.sh
> +++ b/t/t0003-attributes.sh
> @@ -25,7 +25,15 @@ attr_check_quote () {
>   	git check-attr test -- "$path" >actual &&
>   	echo "\"$quoted_path\": test: $expect" >expect &&
>   	test_cmp expect actual
> +}
> +
> +attr_check_source () {
> +	path="$1" expect="$2" source="$3" git_opts="$4" &&
>   
> +	git $git_opts check-attr --source $source test -- "$path" >actual 2>err &&
> +	echo "$path: test: $expect" >expect &&
> +	test_cmp expect actual

This is missing && which means we miss some test failures later

> +	test_must_be_empty err
>   }
>

> +test_expect_success 'setup branches' '
> +	mkdir -p foo/bar &&
> +	test_commit --printf "add .gitattributes" foo/bar/.gitattribute \

The file should be foo/bar/.gitattributes (not .gitattribute). We're missing failures due to this because of the missing && above

> +		"f test=f\na/i test=n\n" tag-1 &&
> +
> +	mkdir -p foo/bar &&

We don't need to make the directory again here

> +	test_commit --printf "add .gitattributes" foo/bar/.gitattribute \
> +		"g test=g\na/i test=m\n" tag-2

I think it would be worth either removing foo/bar/.gitattributes or donig test_write_lines to change it. That way we can be sure all the --source tests are actually using the tree-ish we give it and not just reading from the filesystem.

Best Wishes

Phillip

Show 74 quoted lines

> +'
> +
>   test_expect_success 'command line checks' '
>   	test_must_fail git check-attr &&
>   	test_must_fail git check-attr -- &&
>   	test_must_fail git check-attr test &&
>   	test_must_fail git check-attr test -- &&
>   	test_must_fail git check-attr -- f &&
> +	test_must_fail git check-attr --source &&
> +	test_must_fail git check-attr --source not-a-valid-ref &&
>   	echo "f" | test_must_fail git check-attr --stdin &&
>   	echo "f" | test_must_fail git check-attr --stdin -- f &&
>   	echo "f" | test_must_fail git check-attr --stdin test -- f &&
> @@ -287,6 +306,15 @@ test_expect_success 'using --git-dir and --work-tree' '
>   	)
>   '
>   
> +test_expect_success 'using --source' '
> +	attr_check_source foo/bar/f f tag-1 &&
> +	attr_check_source foo/bar/a/i n tag-1 &&
> +	attr_check_source foo/bar/f unspecified tag-2 &&
> +	attr_check_source foo/bar/a/i m tag-2 &&
> +	attr_check_source foo/bar/g g tag-2 &&
> +	attr_check_source foo/bar/g unspecified tag-1
> +'
> +
>   test_expect_success 'setup bare' '
>   	git clone --template= --bare . bare.git
>   '
> @@ -306,6 +334,18 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' '
>   	)
>   '
>   
> +test_expect_success 'bare repository: with --source' '
> +	(
> +		cd bare.git &&
> +		attr_check_source foo/bar/f f tag-1 &&
> +		attr_check_source foo/bar/a/i n tag-1 &&
> +		attr_check_source foo/bar/f unspecified tag-2 &&
> +		attr_check_source foo/bar/a/i m tag-2 &&
> +		attr_check_source foo/bar/g g tag-2 &&
> +		attr_check_source foo/bar/g unspecified tag-1
> +	)
> +'
> +
>   test_expect_success 'bare repository: check that --cached honors index' '
>   	(
>   		cd bare.git &&
> diff --git a/userdiff.c b/userdiff.c
> index 151d9a5278..b66f090a0b 100644
> --- a/userdiff.c
> +++ b/userdiff.c
> @@ -412,7 +412,7 @@ struct userdiff_driver *userdiff_find_by_path(struct index_state *istate,
>   		check = attr_check_initl("diff", NULL);
>   	if (!path)
>   		return NULL;
> -	git_check_attr(istate, path, check);
> +	git_check_attr(istate, NULL, path, check);
>   
>   	if (ATTR_TRUE(check->items[0].value))
>   		return &driver_true;
> diff --git a/ws.c b/ws.c
> index 6e69877f25..eadbbe5667 100644
> --- a/ws.c
> +++ b/ws.c
> @@ -78,7 +78,7 @@ unsigned whitespace_rule(struct index_state *istate, const char *pathname)
>   	if (!attr_whitespace_rule)
>   		attr_whitespace_rule = attr_check_initl("whitespace", NULL);
>   
> -	git_check_attr(istate, pathname, attr_whitespace_rule);
> +	git_check_attr(istate, NULL, pathname, attr_whitespace_rule);
>   	value = attr_whitespace_rule->items[0].value;
>   	if (ATTR_TRUE(value)) {
>   		/* true (whitespace) */