Re: [PATCH v6 0/2] check-attr: add support to work with tree-ish

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Karthik

On 12/01/2023 11:00, Karthik Nayak wrote:
> v1: https://lore.kernel.org/git/20221206103736.53909-1-karthik.188@gmail.com/
> v2: https://lore.kernel.org/git/CAOLa=ZSsFGBw3ta1jWN8cmUch2ca=zTEjp1xMA6Linafx9W53g@mail.gmail.com/T/#t
> v3: https://lore.kernel.org/git/20221216093552.3171319-1-karthik.188@gmail.com/
> v4: https://lore.kernel.org/git/cover.1671630304.git.karthik.188@gmail.com
> v5: https://lore.kernel.org/git/cover.1671793109.git.karthik.188@gmail.com/
> 
> Given a pathname, git-check-attr(1) will list the attributes which apply to that
> pathname by reading all relevant gitattributes files. Currently there is no way
> to specify a tree-ish to read the gitattributes from.
> 
> This is specifically useful in bare repositories wherein the gitattributes are
> only present in the git working tree but not available directly on the
> filesystem.
> 
> This series aims to add a new flag `--source` to git-check-attr(1) which
> allows us to read gitattributes from the specified tree-ish.
> 
> Changes since v5:
> - Changed the documentation and helper code to say 'tree-ish' instead of 'tree'
> - Fixed broken tests because of missing `&&`
> 
> Range-diff against v5:
> 
> 1:  6224754179 = 1:  6224754179 t0003: move setup for `--all` into new block
> 2:  d835d989ad ! 2:  57f5957127 attr: add flag `--source` to work with tree-ish
>      @@ Documentation/git-check-attr.txt: git-check-attr - Display gitattributes informa
>        [verse]
>       -'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>...]
>      ++'git check-attr' [--source <tree-ish>] [-a | --all | <attr>...] [--] <pathname>...
>      ++'git check-attr' --stdin [-z] [--source <tree-ish>] [-a | --all | <attr>...]
>        
>        DESCRIPTION
>        -----------
>      @@ Documentation/git-check-attr.txt: 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
>      ++--source=<tree-ish>::
>      ++	Check attributes against the specified tree-ish. It is common to
>       +	specify the source tree by naming a commit, branch or tag associated
>       +	with it.
>       +
>      @@ attr.c: void git_check_attr(struct index_state *istate,
>        		const char *name = check->all_attrs[i].attr->name;
>       
>        ## attr.h ##
>      +@@
>      + #ifndef ATTR_H
>      + #define ATTR_H
>      +
>      ++#include "hash.h"
>      ++
>      + /**
>      +  * gitattributes mechanism gives a uniform way to associate various attributes
>      +  * to set of paths.
>       @@ attr.h: void attr_check_free(struct attr_check *check);
>        const char *git_attr_name(const struct git_attr *);
>        
>      @@ builtin/check-attr.c
>        static const char * const check_attr_usage[] = {
>       -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>...]"),
>      ++N_("git check-attr [--source <tree-ish>] [-a | --all | <attr>...] [--] <pathname>..."),
>      ++N_("git check-attr --stdin [-z] [--source <tree-ish>] [-a | --all | <attr>...]"),
>        NULL
>        };
>        
>      @@ t/t0003-attributes.sh: attr_check_quote () {
>        
>       +	git $git_opts check-attr --source $source test -- "$path" >actual 2>err &&
>       +	echo "$path: test: $expect" >expect &&
>      -+	test_cmp expect actual
>      ++	test_cmp expect actual &&
>       +	test_must_be_empty err
>        }
>        
>      @@ t/t0003-attributes.sh: test_expect_success 'setup' '
>        
>       +test_expect_success 'setup branches' '
>       +	mkdir -p foo/bar &&
>      -+	test_commit --printf "add .gitattributes" foo/bar/.gitattribute \
>      ++	test_commit --printf "add .gitattributes" foo/bar/.gitattributes \
>       +		"f test=f\na/i test=n\n" tag-1 &&
>      -+
>      -+	mkdir -p foo/bar &&
>      -+	test_commit --printf "add .gitattributes" foo/bar/.gitattribute \
>      -+		"g test=g\na/i test=m\n" tag-2
>      ++	test_commit --printf "add .gitattributes" foo/bar/.gitattributes \
>      ++		"g test=g\na/i test=m\n" tag-2 &&
>      ++	rm foo/bar/.gitattributes
>       +'
>       +
>        test_expect_success 'command line checks' '

The changes in the range-diff look good - this version addresses all of 
my previous comments - thanks.

Best Wishes

Phillip

> Karthik Nayak (2):
>    t0003: move setup for `--all` into new block
>    attr: add flag `--source` to work with tree-ish
> 
>   Documentation/git-check-attr.txt |  9 ++-
>   archive.c                        |  2 +-
>   attr.c                           | 97 +++++++++++++++++++++++---------
>   attr.h                           |  7 ++-
>   builtin/check-attr.c             | 35 +++++++-----
>   builtin/pack-objects.c           |  2 +-
>   convert.c                        |  2 +-
>   ll-merge.c                       |  4 +-
>   pathspec.c                       |  2 +-
>   t/t0003-attributes.sh            | 48 +++++++++++++++-
>   userdiff.c                       |  2 +-
>   ws.c                             |  2 +-
>   12 files changed, 157 insertions(+), 55 deletions(-)
>