Re: [PATCH 1/2] diff: use HEAD for attributes when using bare repository

[John Cai <johncai86@gmail.com>]

Hi Junio,

On 16 Mar 2023, at 18:56, Junio C Hamano wrote:

> John Cai <johncai86@gmail.com> writes:
>
>> Ah I see--in that case, what would be a good object to put this state into? Mabe
>> repo_settings?
>
> I personally think a global is good enough for a case like this.  It
> is not like the code in the attribute subsystem is structured to
> work in multiple repositories at the same time in the first place, so
> in repo_settings, the_repository, or elsewhere is just as good as
> having it in a plain old file-scope static, and even after moving it
> into the_repository or something, the access will initially be done
> by referring to the_repository global (which may not even exist by
> the time "git --attr-source=<tree>" gets parsed), and not by plumbing
> a "struct repository *" parameter through the callchain.

Makes sense.

>
> Another thing to note is that there needs a mechanism to convey the
> tree object used to read the attributes from down to subprocesses,
> e.g. exporting the GIT_ATTR_SOURCE environment variable and make Git
> behave as if it saw the "git --attr-source=<tree>" option when such
> an environment variable exists, or something.  Otherwise, the custom
> attribute source would only take effect inside built-in commands.
>
> In any case, here is what I have now, stealing some tests from your
> patch.  I do not think I'll be working on it further at least for
> now, so feel free to run with it.  I am not adding the "in a bare
> repository, always read from HEAD unless told otherwise", as I do
> not see a good way to countermand it from a command line.

Thanks for the patch! Looks like you took it most of the way. I'll take a look
and make some additions if necessary.

thanks
John

>
> ----- >8 ----- cut here ----- >8 ----- cut here ----- >8 -----
> Subject: [PATCH] attr: teach "--attr-source=<tree>" global option to "git"
>
> Earlier, 47cfc9bd (attr: add flag `--source` to work with tree-ish,
> 2023-01-14) taught "git check-attr" the "--source=<tree>" option to
> allow it to read attribute files from a tree-ish, but did so only
> for the command.  Just like "check-attr" users wanted a way to use
> attributes from a tree-ish and not from the working tree files,
> users of other commands (like "git diff") would benefit from the
> same.
>
> Undo most of the UI change the commit made, while keeping the
> internal logic to read attributes from a given tree-ish.  Expose the
> internal logic via a new "--attr-source=<tree>" command line option
> given to "git", so that it can be used with any git command that
> runs internally.
>
> The tests were stolen and adjusted from John Cai's effort where only
> "git diff" learned the "--attr-source" option to read from a
> tree-ish.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  archive.c                 |  2 +-
>  attr.c                    | 34 ++++++++++++++++++++++++++++++++--
>  attr.h                    | 13 +++++++++----
>  builtin/check-attr.c      | 17 ++++++++---------
>  builtin/pack-objects.c    |  2 +-
>  convert.c                 |  2 +-
>  git.c                     |  3 +++
>  ll-merge.c                |  4 ++--
>  pathspec.c                |  2 +-
>  t/lib-diff-alternative.sh | 31 ++++++++++++++++++++++++++-----
>  t/t0003-attributes.sh     |  7 ++++++-
>  t/t4018-diff-funcname.sh  | 19 +++++++++++++++++++
>  userdiff.c                |  2 +-
>  ws.c                      |  2 +-
>  14 files changed, 111 insertions(+), 29 deletions(-)
>
> diff --git c/archive.c w/archive.c
> index 9aeaf2bd87..385aa5f248 100644
> --- c/archive.c
> +++ w/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, NULL, path, check);
> +	git_check_attr(istate, path, check);
>  	return check;
>  }
>
> diff --git c/attr.c w/attr.c
> index 1053dfcd4b..d34c7e9d54 100644
> --- c/attr.c
> +++ w/attr.c
> @@ -1165,11 +1165,40 @@ static void collect_some_attrs(struct index_state *istate,
>  	fill(path, pathlen, basename_offset, check->stack, check->all_attrs, rem);
>  }
>
> +static const char *default_attr_source_tree_object_name;
> +
> +void set_git_attr_source(const char *tree_object_name)
> +{
> +	default_attr_source_tree_object_name = xstrdup(tree_object_name);
> +}
> +
> +
> +static void compute_default_attr_source(struct object_id *attr_source)
> +{
> +	if (!default_attr_source_tree_object_name || !is_null_oid(attr_source))
> +		return;
> +
> +	if (get_oid_treeish(default_attr_source_tree_object_name, attr_source))
> +		die(_("bad --attr-source object"));
> +}
> +
> +static struct object_id *default_attr_source(void)
> +{
> +	static struct object_id attr_source;
> +
> +	if (is_null_oid(&attr_source))
> +		compute_default_attr_source(&attr_source);
> +	if (is_null_oid(&attr_source))
> +		return NULL;
> +	return &attr_source;
> +}
> +
>  void git_check_attr(struct index_state *istate,
> -		    const struct object_id *tree_oid, const char *path,
> +		    const char *path,
>  		    struct attr_check *check)
>  {
>  	int i;
> +	const struct object_id *tree_oid = default_attr_source();
>
>  	collect_some_attrs(istate, tree_oid, path, check);
>
> @@ -1182,10 +1211,11 @@ void git_check_attr(struct index_state *istate,
>  	}
>  }
>
> -void git_all_attrs(struct index_state *istate, const struct object_id *tree_oid,
> +void git_all_attrs(struct index_state *istate,
>  		   const char *path, struct attr_check *check)
>  {
>  	int i;
> +	const struct object_id *tree_oid = default_attr_source();
>
>  	attr_check_reset(check);
>  	collect_some_attrs(istate, tree_oid, path, check);
> diff --git c/attr.h w/attr.h
> index 9884ea2bc6..676bd17ce2 100644
> --- c/attr.h
> +++ w/attr.h
> @@ -45,7 +45,7 @@
>   * const char *path;
>   *
>   * setup_check();
> - * git_check_attr(&the_index, tree_oid, path, check);
> + * git_check_attr(&the_index, path, check);
>   * ------------
>   *
>   * - Act on `.value` member of the result, left in `check->items[]`:
> @@ -120,7 +120,6 @@
>  #define ATTR_MAX_FILE_SIZE (100 * 1024 * 1024)
>
>  struct index_state;
> -struct object_id;
>
>  /**
>   * An attribute is an opaque object that is identified by its name. Pass the
> @@ -135,6 +134,12 @@ struct git_attr;
>  struct all_attrs_item;
>  struct attr_stack;
>
> +/*
> + * The textual object name for the tree-ish used by git_check_attr()
> + * to read attributes from (instead of from the working tree).
> + */
> +void set_git_attr_source(const char *);
> +
>  /*
>   * Given a string, return the gitattribute object that
>   * corresponds to it.
> @@ -203,14 +208,14 @@ void attr_check_free(struct attr_check *check);
>  const char *git_attr_name(const struct git_attr *);
>
>  void git_check_attr(struct index_state *istate,
> -		    const struct object_id *tree_oid, const char *path,
> +		    const char *path,
>  		    struct attr_check *check);
>
>  /*
>   * Retrieve all attributes that apply to the specified path.
>   * check holds the attributes and their values.
>   */
> -void git_all_attrs(struct index_state *istate, const struct object_id *tree_oid,
> +void git_all_attrs(struct index_state *istate,
>  		   const char *path, struct attr_check *check);
>
>  enum git_attr_direction {
> diff --git c/builtin/check-attr.c w/builtin/check-attr.c
> index d7a40e674c..1a7929c980 100644
> --- c/builtin/check-attr.c
> +++ w/builtin/check-attr.c
> @@ -58,7 +58,7 @@ static void output_attr(struct attr_check *check, const char *file)
>  }
>
>  static void check_attr(const char *prefix, struct attr_check *check,
> -		       const struct object_id *tree_oid, int collect_all,
> +		       int collect_all,
>  		       const char *file)
>
>  {
> @@ -66,9 +66,9 @@ static void check_attr(const char *prefix, struct attr_check *check,
>  		prefix_path(prefix, prefix ? strlen(prefix) : 0, file);
>
>  	if (collect_all) {
> -		git_all_attrs(&the_index, tree_oid, full_path, check);
> +		git_all_attrs(&the_index, full_path, check);
>  	} else {
> -		git_check_attr(&the_index, tree_oid, full_path, check);
> +		git_check_attr(&the_index, full_path, check);
>  	}
>  	output_attr(check, file);
>
> @@ -76,7 +76,7 @@ static void check_attr(const char *prefix, struct attr_check *check,
>  }
>
>  static void check_attr_stdin_paths(const char *prefix, struct attr_check *check,
> -				   const struct object_id *tree_oid, int collect_all)
> +				   int collect_all)
>  {
>  	struct strbuf buf = STRBUF_INIT;
>  	struct strbuf unquoted = STRBUF_INIT;
> @@ -90,7 +90,7 @@ static void check_attr_stdin_paths(const char *prefix, struct attr_check *check,
>  				die("line is badly quoted");
>  			strbuf_swap(&buf, &unquoted);
>  		}
> -		check_attr(prefix, check, tree_oid, collect_all, buf.buf);
> +		check_attr(prefix, check, collect_all, buf.buf);
>  		maybe_flush_or_die(stdout, "attribute to stdout");
>  	}
>  	strbuf_release(&buf);
> @@ -106,7 +106,6 @@ static NORETURN void error_with_usage(const char *msg)
>  int cmd_check_attr(int argc, const char **argv, const char *prefix)
>  {
>  	struct attr_check *check;
> -	struct object_id *tree_oid = NULL;
>  	struct object_id initialized_oid;
>  	int cnt, i, doubledash, filei;
>
> @@ -182,14 +181,14 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)
>  	if (source) {
>  		if (repo_get_oid_tree(the_repository, source, &initialized_oid))
>  			die("%s: not a valid tree-ish source", source);
> -		tree_oid = &initialized_oid;
> +		set_git_attr_source(source);
>  	}
>
>  	if (stdin_paths)
> -		check_attr_stdin_paths(prefix, check, tree_oid, all_attrs);
> +		check_attr_stdin_paths(prefix, check, all_attrs);
>  	else {
>  		for (i = filei; i < argc; i++)
> -			check_attr(prefix, check, tree_oid, all_attrs, argv[i]);
> +			check_attr(prefix, check, all_attrs, argv[i]);
>  		maybe_flush_or_die(stdout, "attribute to stdout");
>  	}
>
> diff --git c/builtin/pack-objects.c w/builtin/pack-objects.c
> index 74a167a180..d561541b8c 100644
> --- c/builtin/pack-objects.c
> +++ w/builtin/pack-objects.c
> @@ -1320,7 +1320,7 @@ static int no_try_delta(const char *path)
>
>  	if (!check)
>  		check = attr_check_initl("delta", NULL);
> -	git_check_attr(the_repository->index, NULL, path, check);
> +	git_check_attr(the_repository->index, path, check);
>  	if (ATTR_FALSE(check->items[0].value))
>  		return 1;
>  	return 0;
> diff --git c/convert.c w/convert.c
> index a54d1690c0..9b67649032 100644
> --- c/convert.c
> +++ w/convert.c
> @@ -1308,7 +1308,7 @@ void convert_attrs(struct index_state *istate,
>  		git_config(read_convert_config, NULL);
>  	}
>
> -	git_check_attr(istate, NULL, path, check);
> +	git_check_attr(istate, path, check);
>  	ccheck = check->items;
>  	ca->crlf_action = git_path_check_crlf(ccheck + 4);
>  	if (ca->crlf_action == CRLF_UNDEFINED)
> diff --git c/git.c w/git.c
> index 6171fd6769..21bddc5718 100644
> --- c/git.c
> +++ w/git.c
> @@ -4,6 +4,7 @@
>  #include "help.h"
>  #include "run-command.h"
>  #include "alias.h"
> +#include "attr.h"
>  #include "shallow.h"
>
>  #define RUN_SETUP		(1<<0)
> @@ -307,6 +308,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>  			} else {
>  				exit(list_cmds(cmd));
>  			}
> +		} else if (skip_prefix(cmd, "--attr-source=", &cmd)) {
> +			set_git_attr_source(cmd);
>  		} else {
>  			fprintf(stderr, _("unknown option: %s\n"), cmd);
>  			usage(git_usage_string);
> diff --git c/ll-merge.c w/ll-merge.c
> index 130d26501c..22a603e8af 100644
> --- c/ll-merge.c
> +++ w/ll-merge.c
> @@ -391,7 +391,7 @@ enum ll_merge_result ll_merge(mmbuffer_t *result_buf,
>  		normalize_file(theirs, path, istate);
>  	}
>
> -	git_check_attr(istate, NULL, path, check);
> +	git_check_attr(istate, path, check);
>  	ll_driver_name = check->items[0].value;
>  	if (check->items[1].value) {
>  		marker_size = atoi(check->items[1].value);
> @@ -419,7 +419,7 @@ int ll_merge_marker_size(struct index_state *istate, const char *path)
>
>  	if (!check)
>  		check = attr_check_initl("conflict-marker-size", NULL);
> -	git_check_attr(istate, NULL, path, check);
> +	git_check_attr(istate, path, check);
>  	if (check->items[0].value) {
>  		marker_size = atoi(check->items[0].value);
>  		if (marker_size <= 0)
> diff --git c/pathspec.c w/pathspec.c
> index ab70fcbe61..74e02c75fc 100644
> --- c/pathspec.c
> +++ w/pathspec.c
> @@ -730,7 +730,7 @@ int match_pathspec_attrs(struct index_state *istate,
>  	if (name[namelen])
>  		name = to_free = xmemdupz(name, namelen);
>
> -	git_check_attr(istate, NULL, name, item->attr_check);
> +	git_check_attr(istate, name, item->attr_check);
>
>  	free(to_free);
>
> diff --git c/t/lib-diff-alternative.sh w/t/lib-diff-alternative.sh
> index a8f5d3274a..02381eb7f1 100644
> --- c/t/lib-diff-alternative.sh
> +++ w/t/lib-diff-alternative.sh
> @@ -112,15 +112,36 @@ EOF
>
>  	STRATEGY=$1
>
> -	test_expect_success "$STRATEGY diff from attributes" '
> +	test_expect_success "setup attributes files for tests with $STRATEGY" '
> +		git checkout -b master &&
>  		echo "file* diff=driver" >.gitattributes &&
> -		git config diff.driver.algorithm "$STRATEGY" &&
> -		test_must_fail git diff --no-index file1 file2 > output &&
> -		cat expect &&
> -		cat output &&
> +		git add file1 file2 .gitattributes &&
> +		git commit -m "adding files" &&
> +		git checkout -b branchA &&
> +		echo "file* diff=driverA" >.gitattributes &&
> +		git add .gitattributes &&
> +		git commit -m "adding driverA as diff driver" &&
> +		git checkout master &&
> +		git clone --bare --no-local . bare.git
> +	'
> +
> +	test_expect_success "$STRATEGY diff from attributes" '
> +		test_must_fail git -c diff.driver.algorithm=$STRATEGY diff --no-index file1 file2 > output &&
>  		test_cmp expect output
>  	'
>
> +	test_expect_success "diff from attributes with bare repo when branch different than HEAD" '
> +		git -C bare.git --attr-source=branchA -c diff.driver.algorithm=myers \
> +			-c diff.driverA.algorithm=$STRATEGY \
> +			diff HEAD:file1 HEAD:file2 >output &&
> +		test_cmp expect output
> +	'
> +
> +	test_expect_success "diff from attributes with bare repo with invalid source" '
> +		test_must_fail git -C bare.git --attr-source=invalid-branch diff \
> +			HEAD:file1 HEAD:file2
> +	'
> +
>  	test_expect_success "$STRATEGY diff from attributes has valid diffstat" '
>  		echo "file* diff=driver" >.gitattributes &&
>  		git config diff.driver.algorithm "$STRATEGY" &&
> diff --git c/t/t0003-attributes.sh w/t/t0003-attributes.sh
> index 89b306cb11..73db37a7f3 100755
> --- c/t/t0003-attributes.sh
> +++ w/t/t0003-attributes.sh
> @@ -30,8 +30,13 @@ attr_check_quote () {
>  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 &&
> +
> +	git $git_opts check-attr --source $source test -- "$path" >actual 2>err &&
> +	test_cmp expect actual &&
> +	test_must_be_empty err &&
> +
> +	git $git_opts --attr-source="$source" check-attr test -- "$path" >actual 2>err &&
>  	test_cmp expect actual &&
>  	test_must_be_empty err
>  }
> diff --git c/t/t4018-diff-funcname.sh w/t/t4018-diff-funcname.sh
> index 42a2b9a13b..c8d555771d 100755
> --- c/t/t4018-diff-funcname.sh
> +++ w/t/t4018-diff-funcname.sh
> @@ -63,6 +63,25 @@ do
>  		test_i18ngrep ! fatal msg &&
>  		test_i18ngrep ! error msg
>  	'
> +
> +	test_expect_success "builtin $p pattern compiles on bare repo with --attr-source" '
> +		test_when_finished "rm -rf bare.git" &&
> +		git checkout -B master &&
> +		git add . &&
> +		echo "*.java diff=notexist" >.gitattributes &&
> +		git add .gitattributes &&
> +		git commit -am "changing gitattributes" &&
> +		git checkout -B branchA &&
> +		echo "*.java diff=$p" >.gitattributes &&
> +		git add .gitattributes &&
> +		git commit -am "changing gitattributes" &&
> +		git clone --bare --no-local . bare.git &&
> +		git -C bare.git symbolic-ref HEAD refs/heads/master &&
> +		test_expect_code 1 git -C bare.git --attr-source=branchA \
> +			diff --exit-code HEAD:A.java HEAD:B.java 2>msg &&
> +		test_i18ngrep ! fatal msg &&
> +		test_i18ngrep ! error msg
> +	'
>  done
>
>  test_expect_success 'last regexp must not be negated' '
> diff --git c/userdiff.c w/userdiff.c
> index 58a3d59ef8..156660eaca 100644
> --- c/userdiff.c
> +++ w/userdiff.c
> @@ -415,7 +415,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, NULL, path, check);
> +	git_check_attr(istate, path, check);
>
>  	if (ATTR_TRUE(check->items[0].value))
>  		return &driver_true;
> diff --git c/ws.c w/ws.c
> index da3d0e28cb..903bfcd53e 100644
> --- c/ws.c
> +++ w/ws.c
> @@ -79,7 +79,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, NULL, pathname, attr_whitespace_rule);
> +	git_check_attr(istate, pathname, attr_whitespace_rule);
>  	value = attr_whitespace_rule->items[0].value;
>  	if (ATTR_TRUE(value)) {
>  		/* true (whitespace) */