Re: [PATCH 1/1] attr: add native file mode values support

["Torsten Bögershausen" <tboegi@web.de>]

On Sat, Nov 11, 2023 at 04:03:08AM +0000, Joanna Wang wrote:

Some thoughts and comments inline...

> Gives all paths inherent 'mode' attribute values based on the paths'
> modes (one of 100644, 100755, 120000, 040000, 160000). Users may use
> this feature to filter by file types. For example a pathspec such as
> ':(attr:mode=160000)' could filter for submodules without needing

My spontanous feeling is that filetype may be another choice:
> ':(attr:filetype=160000)' could filter for submodules without needing
And having written this, we can think using something borrowed from
`find . -type f`

:(attr:filetype=f)' or :(attr:filetype=x)' (for executable)

> `mode=160000` to actually be specified for each subdmoule path in
> .gitattributes. The native mode values are also reflected in
> `git check-attr` results.

But then I missed the point why we need an attribute here?
The mode is already defined by the the file system (and Git),
is there a special reason that the user can define or re-define the
value here ?
May be there is, when working with pathspec.
But then "pathspec=" could be a better construction.
Since "mode" could make a reader think that Git does somewhat with the file
when checking out.
My personal hope reading "mode=100755" in .gitattributes would
be that Git makes it executable when checking out, if if it is
recorded in Git as 100644, probably coming from a file-system that
doesn't support the executable bit in a Unix way.


> If there is any existing mode attribute for a path (e.g. there is
> !mode, -mode, mode, mode=<value> in .gitattributes) that setting will
> take precedence over the native mode value.
>
> ---
>
> I went with 'mode' because that is the word used in documentation
> (e.g. https://git-scm.com/book/sv/v2/Git-Internals-Git-Objects)
> and in the code (e.g. `ce_mode`). Please let me know what you think
> of this UX.
>
> The implementation happens within git_check_attr() method which is
> the common mathod called for getting a pathspec attribute value.
>
> The previous discussed idea did not work with `git check-attr`.
> (https://lore.kernel.org/all/CAMmZTi8swsSMcLUcW+YwUDg8GcrY_ks2+i35-nsHE3o9MNpsUQ@mail.gmail.com/).
>
> There are no tests for excluding based on pathspec attrs in subdirectories
> due to an existing bug. Since it is not specific to native mode, I thought
> it would be better to fix separately.
> https://lore.kernel.org/all/CAMmZTi89Un+bsLXdEdYs44oT8eLNp8y=Pm8ywaurcQ7ccRKGdQ@mail.gmail.com/

Reading "pathspec attrs" above it feels better to call the new attribute pathspec.
But why should a user be allowed to overwrite what we have in the index ?
That is somewhat missing in the motivation for this change,
it would be good to have this explained in the commit message.

>
>  Documentation/gitattributes.txt | 10 +++++++
>  attr.c                          | 42 ++++++++++++++++++++++++--
>  t/t0003-attributes.sh           | 40 +++++++++++++++++++++++++
>  t/t6135-pathspec-with-attrs.sh  | 53 +++++++++++++++++++++++++++++++++
>  4 files changed, 143 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
> index 8c1793c148..bb3c11f151 100644
> --- a/Documentation/gitattributes.txt
> +++ b/Documentation/gitattributes.txt
> @@ -156,6 +156,16 @@ Unspecified::
>  Any other value causes Git to act as if `text` has been left
>  unspecified.
>
> +`mode`
> +^^^^^
> +
> +This attribute is for filtering files by their file bit modes
> +(40000, 120000, 160000, 100755, 100644) and has native support in git,
> +meaning values for this attribute are natively set (e.g. mode=100644) by
> +git without having to specify them in .gitattributes. However, if
> +'mode' is set in .gitattributest for some path, that value takes precendence,
> +whether it is 'set', 'unset', 'unspecified', or some other value.
> +
>  `eol`
>  ^^^^^
>
> diff --git a/attr.c b/attr.c
> index e62876dfd3..95dc1cf695 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -1240,20 +1240,58 @@ static struct object_id *default_attr_source(void)
>  	return &attr_source;
>  }
>
> +
> +/*
> + * This implements native file mode attr support.
> + * We always return the current mode of the path, not the mode stored
> + * in the index, unless the path is a directory where we check the index
> + * to see if it is a GITLINK. It is ok to rely on the index for GITLINK
> + * modes because a path cannot become a GITLINK (which is a git concept only)
> + * without having it indexed with a GITLINK mode in git.
> + */
> +static unsigned int native_mode_attr(struct index_state *istate, const char *path)
> +{
> +	struct stat st;
> +	unsigned int mode;
> +	if (lstat(path, &st))
> +		die_errno(_("unable to stat '%s'"), path);
> +	mode = canon_mode(st.st_mode);
> +	if (S_ISDIR(mode)) {
> +		int pos = index_name_pos(istate, path, strlen(path));
> +		if (pos >= 0)
> +			if (S_ISGITLINK(istate->cache[pos]->ce_mode))
> +				return istate->cache[pos]->ce_mode;
> +	}
> +	return mode;
> +}
> +
> +
>  void git_check_attr(struct index_state *istate,
>  		    const char *path,
>  		    struct attr_check *check)
>  {
>  	int i;
>  	const struct object_id *tree_oid = default_attr_source();
> +	struct strbuf sb = STRBUF_INIT;
>
>  	collect_some_attrs(istate, tree_oid, path, check);
>
>  	for (i = 0; i < check->nr; i++) {
>  		unsigned int n = check->items[i].attr->attr_nr;
>  		const char *value = check->all_attrs[n].value;
> -		if (value == ATTR__UNKNOWN)
> -			value = ATTR__UNSET;
> +		if (value == ATTR__UNKNOWN){
> +			if (strcmp(check->all_attrs[n].attr->name, "mode") == 0) {
> +				/*
> +				 * If value is ATTR_UNKNOWN then it has not been set
> +				 * anywhere with -mode (ATTR_FALSE), !mode (ATTR_UNSET),
> +				 * mode (ATTR_TRUE), or an explicit value. We fill
> +				 * value with the native mode value.
> +				 */
> +				strbuf_addf(&sb, "%06o", native_mode_attr(istate, path));
> +				value = sb.buf;
> +			} else
> +				value = ATTR__UNSET;
> +		}
>  		check->items[i].value = value;
>  	}
>  }
> diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
> index aee2298f01..9c2603d8e2 100755
> --- a/t/t0003-attributes.sh
> +++ b/t/t0003-attributes.sh
> @@ -19,6 +19,15 @@ attr_check () {
>  	test_must_be_empty err
>  }
>
> +attr_check_mode () {
> +	path="$1" expect="$2" git_opts="$3" &&
It would be easier to read, if each assignment is on it's on line
> +
> +	git $git_opts check-attr mode -- "$path" >actual 2>err &&
> +	echo "$path: mode: $expect" >expect &&
> +	test_cmp expect actual &&
> +	test_must_be_empty err
> +}
> +
>  attr_check_quote () {
>  	path="$1" quoted_path="$2" expect="$3" &&
>
> @@ -558,4 +567,35 @@ test_expect_success EXPENSIVE 'large attributes file ignored in index' '
>  	test_cmp expect err
>  '
>
> +test_expect_success 'submodule with .git file' '
> +	mkdir sub &&
> +	(cd sub &&
> +	git init &&
> +	mv .git .real &&
> +	echo "gitdir: .real" >.git &&
> +	test_commit first) &&
> +	git update-index --add -- sub

Style and indentation (Please use TAB for indenting)
Using sub-shells is good. Somewhat easier to read would be this:

	mkdir sub &&
	(
	  cd sub &&
	  git init &&
	  mv .git .real &&
	  echo "gitdir: .real" >.git &&
	  test_commit first
	)  &&




> +'
> +
> +test_expect_success 'native mode attributes work' '
> +	>exec && chmod +x exec && attr_check_mode exec 100755 &&
> +	>normal && attr_check_mode normal 100644 &&
> +	mkdir dir && attr_check_mode dir 040000 &&
> +	ln -s normal normal_sym && attr_check_mode normal_sym 120000 &&
> +	attr_check_mode sub 160000
> +'

We need a precondition here:
test_expect_success SYMLINKS

> +
> +test_expect_success '.gitattributes mode values take precedence' '
> +	(
> +		echo "mode_value* mode=myownmode" &&
> +		echo "mode_set* mode" &&
> +		echo "mode_unset* -mode" &&
> +		echo "mode_unspecified* !mode"
> +	) >.gitattributes &&

Can this be written using a
	cat <<-\EOF >expect &&

	EOF
expression ?

> +	>mode_value && attr_check_mode mode_value myownmode &&
> +	>mode_unset && attr_check_mode mode_unset unset &&
> +	>mode_unspecified && attr_check_mode mode_unspecified unspecified &&
> +	>mode_set && attr_check_mode mode_set set
> +'
> +
>  test_done
> diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh
> index a9c1e4e0ec..fd9569d39b 100755
> --- a/t/t6135-pathspec-with-attrs.sh
> +++ b/t/t6135-pathspec-with-attrs.sh
> @@ -64,6 +64,9 @@ test_expect_success 'setup .gitattributes' '
>  	fileSetLabel label
>  	fileValue label=foo
>  	fileWrongLabel label☺
> +	mode_set* mode=1234
> +	mode_unset* -mode
> +	mode_unspecified* !mode
>  	EOF
>  	echo fileSetLabel label1 >sub/.gitattributes &&
>  	git add .gitattributes sub/.gitattributes &&
> @@ -295,4 +298,54 @@ test_expect_success 'reading from .gitattributes in a subdirectory (3)' '
>  	test_cmp expect actual
>  '
>
> +test_expect_success 'mode attr is handled correctly for overriden values' '
> +	>mode_set_1 &&
> +	>mode_unset_1 &&
> +	>mode_unspecified_1 &&
> +	>mode_regular_file_1 &&
> +
> +	git status -s ":(attr:mode=1234)mode*" >actual &&
> +	cat <<-\EOF >expect &&
> +	?? mode_set_1
> +	EOF
> +	test_cmp expect actual &&
> +
> +	git status -s ":(attr:-mode)mode*" >actual &&
> +	echo ?? mode_unset_1 >expect &&
> +	test_cmp expect actual &&
> +
> +	git status -s ":(attr:!mode)mode*" >actual &&
> +	echo ?? mode_unspecified_1 >expect &&
> +	test_cmp expect actual &&
> +
> +	git status -s ":(attr:mode=100644)mode*" >actual &&
> +	echo ?? mode_regular_file_1 >expect &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'mode attr values are current file modes, not indexed modes' '
> +	>mode_exec_file_1 &&
> +
> +	git status -s ":(attr:mode=100644)mode_exec_*" >actual &&
> +	echo ?? mode_exec_file_1 >expect &&
> +	test_cmp expect actual &&
> +
> +	git add mode_exec_file_1 && chmod +x mode_exec_file_1 &&
> +	git status -s ":(attr:mode=100755)mode_exec_*" >actual &&
> +	echo AM mode_exec_file_1 >expect &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'mode attr can be excluded' '
> +	>mode_1_regular &&
> +	>mode_1_exec  && chmod +x mode_1_exec &&
> +	git status -s ":(exclude,attr:mode=100644)" "mode_1_*" >actual &&
> +	echo ?? mode_1_exec >expect &&
> +	test_cmp expect actual &&
> +
> +	git status -s ":(exclude,attr:mode=100755)" "mode_1_*" >actual &&
> +	echo ?? mode_1_regular >expect &&
> +	test_cmp expect actual
> +'
> +
>  test_done
> --
> 2.42.0.869.gea05f2083d-goog
>
>