Hello Phillip, On Wed, Dec 21, 2022 at 9:57 PM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote: > > Since we use a tree-ish object, the user can pass "--revision > > HEAD:subdirectory" and all the attributes will be looked up as if > > subdirectory was the root directory of the repository. > > We should be clear in the documentation and option help that --revision > takes a tree-ish (i.e. --revision=<tree-ish>). Maybe calling the option > --tree would be clearer. > I think we had a discussion around this a bit earlier in the series. https://lore.kernel.org/git/CAOLa=ZTSzUh2Ma_EMHHWcDunGyKMaUW9BaG=QdegtMqLd+69Wg@xxxxxxxxxxxxxx/ Mostly, that the idea of using `--revision` was taken from `git-svn(1)`. I'm good to make that change, what do you think would be best? `--source` or `-tree`? I like `--tree` better, but I'm open to suggestions. > > The implementation looks good apart from failing to bail out if it > cannot parse the argument to --revision (perhaps we should add a test > for that). I've left a few suggestions below. > Thank you for the review! > > Signed-off-by: Karthik Nayak <karthik.188@xxxxxxxxx> > > Signed-off-by: Toon Claes <toon@xxxxxxxxx> > > Co-authored-by: toon@xxxxxxxxx > > > diff --git a/attr.c b/attr.c > > index 42ad6de8c7..6c69e82080 100644 > > --- a/attr.c > > +++ b/attr.c > > @@ -11,8 +11,12 @@ > > #include "exec-cmd.h" > > #include "attr.h" > > #include "dir.h" > > +#include "strbuf.h" > > +#include "tree-walk.h" > > These new includes are not required. > Will remove! > > diff --git a/attr.h b/attr.h > > index 3fb40cced0..f4a2bedd68 100644 > > --- a/attr.h > > +++ b/attr.h > > @@ -1,6 +1,8 @@ > > #ifndef ATTR_H > > #define ATTR_H > > > > +#include "hash.h" > > This include is not required. > Will remove! > > diff --git a/builtin/check-attr.c b/builtin/check-attr.c > > index 0fef10eb6b..04640e0297 100644 > > --- a/builtin/check-attr.c > > +++ b/builtin/check-attr.c > > @@ -1,3 +1,4 @@ > > +#include "repository.h" > > This include is not required. Also please add any new includes below > cache.h as Junio has previously mentioned. > Understood. I missed this. > > + if (revision) { > > + tree_oid = xmalloc(sizeof(struct object_id)); > > I think we prefer 'var = xmalloc(sizeof(*var));' to avoid errors if the > type of var changes. This allocation does not appear to be freed > anywhere. We could avoid the allocation by delcaring an automatic > variable above and setting tree_oid to point to it here. > Understood, let me do that. > > + if (repo_get_oid_tree(the_repository, revision, tree_oid)) > > + error("%s: not a valid revision", revision); > > We should die() here rather than continuing with a bad tree. > Will switch to `die(...)` > > + git $git_opts check-attr --revision $revision test -- "$path" >actual 2>err && > > err is never used. Should we be doing 'test_must_be_empty err'? > Yeah this makes sense, let me add it in. > > + echo "$path: test: $expect" >expect && > > + test_cmp expect actual > > } > > > > [...] > > +test_expect_success 'setup branches' ' > > + ( > > + echo "f test=f" && > > + echo "a/i test=n" > > + ) > > We'd normally write this as > > test_write_lines "f test=f" "a/i test=n" | git hash-object ... > > However I think it would be simpler to create the commit with something like > > mkdir -p foo/bar && > test_commit --printf "add .gitattributes" foo/bar/.gitattributes \ > "t test=f\na/i test=n\n" tag-1 && > rm -r foo/bar/.gitattributes > > which would also reduce the number of processes. Failing that a helper > function to reduce the duplication would be a good idea. > Thanks for this, my method was mostly put together with what I could make work, this is much cleaner. We don't need to ` rm -r foo/bar/.gitattributes` as far as I see. > | git hash-object -w --stdin >id && > > + git update-index --add --cacheinfo 100644,$(cat id),foo/bar/.gitattributes && > > + git write-tree >id && > > + tree_id=$(cat id) && > > For future reference it is perfectly fine to write > tree_oid=$(git write-tree) && > > as we will still detect a non-zero exit code from git. > Noted. > > + git commit-tree $tree_id -m "random commit message" >id && > > + commit_id=$(cat id) && > > + git update-ref refs/heads/branch1 $commit_id && > > + > > + ( > > + echo "g test=g" && > > + echo "a/i test=m" > > + ) | git hash-object -w --stdin >id && > > + git update-index --add --cacheinfo 100644,$(cat id),foo/bar/.gitattributes && > > + git write-tree >id && > > + tree_id=$(cat id) && > > + git commit-tree $tree_id -m "random commit message" >id && > > + commit_id=$(cat id) && > > + git update-ref refs/heads/branch2 $commit_id > > +' > > [...] > > test_expect_success 'setup bare' ' > > git clone --template= --bare . bare.git > > ' > > @@ -306,6 +347,27 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' ' > > ) > > ' > > > > +test_expect_success 'bare repository: with --revision' ' > > + ( > > + cd bare.git && > > You could create a bare clone of the existing repo rather than having to > recreate the commits here. > Makes sense, let me simplify this too. -- - Karthik
