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

[John Cai <johncai86@gmail.com>]

On 23/03/14 10:43AM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:

Hi Junio,

> 
> > Since referring to in-tree attributes is often useful with any
> > command, not limited to "diff", I wonder if it is feasible to add
> > support for the --attr-source=<tree> option globally, instead of
> > implementing such an option piecemeal.  If your "git diff" in a bare
> > repository starts honoring attributes recorded in HEAD, don't your
> > users expect your "git log" and "git show" to also honor them?
> 
> Just for illustration, here is one way to do so.
> 
> The implementation goes in the opposite direction from the more
> recent trend, which is why I am not making it an official patch, but

Could you explain why this goes against the "more recent trend" for my
understanding?

> with this you can do things like:
> 
>   $ git --attr-source=e83c5163 check-attr whitespace cache.h
>   cache.h: whitespace: unspecified
>   $ git --attr-source=e2f6331a142^ check-attr whitespace cache.h
>   cache.h: whitespace: set
>   $ git --attr-source=HEAD check-attr whitespace cache.h
>   cache.h: whitespace: indent,trail,space

I like the idea of an option that is global. For git-check-attr however, we
already have a --source option. Not sure how big of a deal that is. If we wanted
to, we could put --source on a deprecation path by making it hidden for a
while, etc.

> 
> where e83c5163 is the very first version of Git from 2005 where
> .gitattributes did not exist, e2f6331a142^ is the last version
> before we set whitespace to indent,trail,space, and HEAD is a more
> recent version of our source tree.
> 
> In the following illustration patch, the attr-source tree object
> name is kept as a string as long as possible and at the very last
> minute when we call git_check_attr() for the first time we turn it
> into an object id.  This is because at the very early stage when we
> parse the global option we may not even know where our repository is
> (hence do not have enough information to parse HEAD).  We also figure
> out is_bare_repository() late in the process for the same reason.
> 
> 
> 
>  attr.c | 29 +++++++++++++++++++++++++++++
>  attr.h |  6 ++++++
>  git.c  |  3 +++
>  3 files changed, 38 insertions(+)
> 
> diff --git c/attr.c w/attr.c
> index 1053dfcd4b..2fd6e0eab2 100644
> --- c/attr.c
> +++ w/attr.c
> @@ -1165,12 +1165,41 @@ 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 struct object_id *default_attr_source(void)
> +{
> +	static struct object_id attr_source;
> +
> +	if (is_null_oid(&attr_source)) {
> +		if (!default_attr_source_tree_object_name &&
> +		    is_bare_repository())
> +			default_attr_source_tree_object_name = "HEAD";
> +
> +		if (default_attr_source_tree_object_name &&
> +		    is_null_oid(&attr_source))
> +			get_oid(default_attr_source_tree_object_name, &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,
>  		    struct attr_check *check)
>  {
>  	int i;
>  
> +	if (!tree_oid)
> +		tree_oid = default_attr_source();
> +
>  	collect_some_attrs(istate, tree_oid, path, check);
>  
>  	for (i = 0; i < check->nr; i++) {
> diff --git c/attr.h w/attr.h
> index 9884ea2bc6..a77e3713b2 100644
> --- c/attr.h
> +++ w/attr.h
> @@ -135,6 +135,12 @@ struct git_attr;
>  struct all_attrs_item;
>  struct attr_stack;
>  
> +/*
> + * The textual object name for the tree-ish used by git_check_attr()
> + * when NULL is given to tree_oid.
> + */
> +void set_git_attr_source(const char *);
> +
>  /*
>   * Given a string, return the gitattribute object that
>   * corresponds to it.
> 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);