On Wed, Jan 11, 2023 at 12:30 PM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote:
>
> Hi Karthik
>
Hello Philip!
> On 02/01/2023 11:04, Karthik Nayak wrote:
> > 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.
>
Thanks for the review!
> > 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@xxxxxxxxx>
> > Signed-off-by: Toon Claes <toon@xxxxxxxxx>
> > Co-authored-by: toon@xxxxxxxxx
> > ---
> > -'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)
Will change!
> >
> > 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.
>
Will do!
> > -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.
>
Yeah sure!
> > 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
>
Good catch on this!
> > + 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
>
Yup, this fixes the test. Thanks!
> > + "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.
>
Will do a `rm` on the file.
