| Message ID | 23813496fc73b7e5cb9f09b166e05c9a02bac43c.1671793109.git.karthik.188@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | check-attr: add support to work with tree-ish | expand |
Karthik Nayak <karthik.188@gmail.com> writes: > 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. > > 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@gmail.com> > Signed-off-by: Toon Claes <toon@iotcl.com> > Co-authored-by: toon@iotcl.com > --- > Documentation/git-check-attr.txt | 10 +++- > archive.c | 2 +- > attr.c | 97 +++++++++++++++++++++++--------- > attr.h | 5 +- > builtin/check-attr.c | 35 +++++++----- > builtin/pack-objects.c | 2 +- > convert.c | 2 +- > ll-merge.c | 4 +- > pathspec.c | 2 +- > t/t0003-attributes.sh | 42 +++++++++++++- > userdiff.c | 2 +- > ws.c | 2 +- > 12 files changed, 152 insertions(+), 53 deletions(-) > > diff --git a/Documentation/git-check-attr.txt b/Documentation/git-check-attr.txt > index 84f41a8e82..fb15c44598 100644 > --- a/Documentation/git-check-attr.txt > +++ b/Documentation/git-check-attr.txt > @@ -9,8 +9,8 @@ git-check-attr - Display gitattributes information > SYNOPSIS > -------- > [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>...] > > 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. > + > \--:: > Interpret all preceding arguments as attributes and all following > arguments as path names. > diff --git a/archive.c b/archive.c > index 941495f5d7..81ff76fce9 100644 > --- a/archive.c > +++ b/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, path, check); > + git_check_attr(istate, NULL, path, check); > return check; > } > > diff --git a/attr.c b/attr.c > index 42ad6de8c7..a63f267187 100644 > --- a/attr.c > +++ b/attr.c > @@ -13,6 +13,8 @@ > #include "dir.h" > #include "utf8.h" > #include "quote.h" > +#include "revision.h" > +#include "object-store.h" > #include "thread-utils.h" > > const char git_attr__true[] = "(builtin)true"; > @@ -729,14 +731,62 @@ static struct attr_stack *read_attr_from_file(const char *path, unsigned flags) > return res; > } > > -static struct attr_stack *read_attr_from_index(struct index_state *istate, > - const char *path, > - unsigned flags) > +static struct attr_stack *read_attr_from_buf(char *buf, const char *path, > + unsigned flags) > { > struct attr_stack *res; > - char *buf, *sp; > + char *sp; > int lineno = 0; > > + if (!buf) > + return NULL; > + > + CALLOC_ARRAY(res, 1); > + for (sp = buf; *sp;) { > + char *ep; > + int more; > + > + ep = strchrnul(sp, '\n'); > + more = (*ep == '\n'); > + *ep = '\0'; > + handle_attr_line(res, sp, path, ++lineno, flags); > + sp = ep + more; > + } > + free(buf); > + > + return res; > +} > + > +static struct attr_stack *read_attr_from_blob(struct index_state *istate, > + const struct object_id *tree_oid, > + const char *path, unsigned flags) > +{ > + struct object_id oid; > + unsigned long sz; > + enum object_type type; > + void *buf; > + unsigned short mode; > + > + if (!tree_oid) > + return NULL; > + > + if (get_tree_entry(istate->repo, tree_oid, path, &oid, &mode)) > + return NULL; > + > + buf = read_object_file(&oid, &type, &sz); > + if (!buf || type != OBJ_BLOB) { > + free(buf); > + return NULL; > + } > + > + return read_attr_from_buf(buf, path, flags); > +} > + > +static struct attr_stack *read_attr_from_index(struct index_state *istate, > + const char *path, unsigned flags) > +{ > + char *buf; > + > if (!istate) > return NULL; > > @@ -758,28 +808,19 @@ static struct attr_stack *read_attr_from_index(struct index_state *istate, > if (!buf) > return NULL; > > - CALLOC_ARRAY(res, 1); > - for (sp = buf; *sp; ) { > - char *ep; > - int more; > - > - ep = strchrnul(sp, '\n'); > - more = (*ep == '\n'); > - *ep = '\0'; > - handle_attr_line(res, sp, path, ++lineno, flags); > - sp = ep + more; > - } > - free(buf); > - return res; > + return read_attr_from_buf(buf, path, flags); > } > > static struct attr_stack *read_attr(struct index_state *istate, > + const struct object_id *tree_oid, > const char *path, unsigned flags) > { > struct attr_stack *res = NULL; > > if (direction == GIT_ATTR_INDEX) { > res = read_attr_from_index(istate, path, flags); > + } else if (tree_oid) { > + res = read_attr_from_blob(istate, tree_oid, path, flags); > } else if (!is_bare_repository()) { > if (direction == GIT_ATTR_CHECKOUT) { > res = read_attr_from_index(istate, path, flags); > @@ -839,6 +880,7 @@ static void push_stack(struct attr_stack **attr_stack_p, > } > > static void bootstrap_attr_stack(struct index_state *istate, > + const struct object_id *tree_oid, > struct attr_stack **stack) > { > struct attr_stack *e; > @@ -864,7 +906,7 @@ static void bootstrap_attr_stack(struct index_state *istate, > } > > /* root directory */ > - e = read_attr(istate, GITATTRIBUTES_FILE, flags | READ_ATTR_NOFOLLOW); > + e = read_attr(istate, tree_oid, GITATTRIBUTES_FILE, flags | READ_ATTR_NOFOLLOW); > push_stack(stack, e, xstrdup(""), 0); > > /* info frame */ > @@ -878,6 +920,7 @@ static void bootstrap_attr_stack(struct index_state *istate, > } > > static void prepare_attr_stack(struct index_state *istate, > + const struct object_id *tree_oid, > const char *path, int dirlen, > struct attr_stack **stack) > { > @@ -899,7 +942,7 @@ static void prepare_attr_stack(struct index_state *istate, > * .gitattributes in deeper directories to shallower ones, > * and finally use the built-in set as the default. > */ > - bootstrap_attr_stack(istate, stack); > + bootstrap_attr_stack(istate, tree_oid, stack); > > /* > * Pop the "info" one that is always at the top of the stack. > @@ -954,7 +997,7 @@ static void prepare_attr_stack(struct index_state *istate, > strbuf_add(&pathbuf, path + pathbuf.len, (len - pathbuf.len)); > strbuf_addf(&pathbuf, "/%s", GITATTRIBUTES_FILE); > > - next = read_attr(istate, pathbuf.buf, READ_ATTR_NOFOLLOW); > + next = read_attr(istate, tree_oid, pathbuf.buf, READ_ATTR_NOFOLLOW); > > /* reset the pathbuf to not include "/.gitattributes" */ > strbuf_setlen(&pathbuf, len); > @@ -1074,8 +1117,8 @@ static void determine_macros(struct all_attrs_item *all_attrs, > * Otherwise all attributes are collected. > */ > static void collect_some_attrs(struct index_state *istate, > - const char *path, > - struct attr_check *check) > + const struct object_id *tree_oid, > + const char *path, struct attr_check *check) > { > int pathlen, rem, dirlen; > const char *cp, *last_slash = NULL; > @@ -1094,7 +1137,7 @@ static void collect_some_attrs(struct index_state *istate, > dirlen = 0; > } > > - prepare_attr_stack(istate, path, dirlen, &check->stack); > + prepare_attr_stack(istate, tree_oid, path, dirlen, &check->stack); > all_attrs_init(&g_attr_hashmap, check); > determine_macros(check->all_attrs, check->stack); > > @@ -1103,12 +1146,12 @@ static void collect_some_attrs(struct index_state *istate, > } > > void git_check_attr(struct index_state *istate, > - const char *path, > + const struct object_id *tree_oid, const char *path, > struct attr_check *check) > { > int i; > > - collect_some_attrs(istate, path, check); > + collect_some_attrs(istate, tree_oid, path, check); > > for (i = 0; i < check->nr; i++) { > size_t n = check->items[i].attr->attr_nr; > @@ -1119,13 +1162,13 @@ void git_check_attr(struct index_state *istate, > } > } > > -void git_all_attrs(struct index_state *istate, > +void git_all_attrs(struct index_state *istate, const struct object_id *tree_oid, > const char *path, struct attr_check *check) > { > int i; > > attr_check_reset(check); > - collect_some_attrs(istate, path, check); > + collect_some_attrs(istate, tree_oid, path, check); > > for (i = 0; i < check->all_attrs_nr; i++) { > const char *name = check->all_attrs[i].attr->name; > diff --git a/attr.h b/attr.h > index 3fb40cced0..84a3279215 100644 > --- a/attr.h > +++ b/attr.h > @@ -190,13 +190,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 char *path, struct attr_check *check); > + const struct object_id *tree_oid, const char *path, > + struct attr_check *check); FYI, this breaks "make hdr-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, > +void git_all_attrs(struct index_state *istate, const struct object_id *tree_oid, > const char *path, struct attr_check *check); Likewise. Perhaps squash something like this in in the next iteration? attr.h | 1 + 1 file changed, 1 insertion(+) diff --git i/attr.h w/attr.h index 84a3279215..fca6c30430 100644 --- i/attr.h +++ w/attr.h @@ -108,6 +108,7 @@ */ struct index_state; +struct object_id; /** * An attribute is an opaque object that is identified by its name. Pass the
On Tue, Jan 3, 2023 at 12:38 AM Junio C Hamano <gitster@pobox.com> wrote: > > Likewise. > > Perhaps squash something like this in in the next iteration? > > attr.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git i/attr.h w/attr.h > index 84a3279215..fca6c30430 100644 > --- i/attr.h > +++ w/attr.h > @@ -108,6 +108,7 @@ > */ > > struct index_state; > +struct object_id; > > /** > * An attribute is an opaque object that is identified by its name. Pass the Thanks Junio, I'll squash this in, waiting for more reviews before rolling out a new version.
Hi Karthik 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. > 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@gmail.com> > Signed-off-by: Toon Claes <toon@iotcl.com> > Co-authored-by: toon@iotcl.com > --- > -'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) > > 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. > -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. > 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 > + 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 > + "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. Best Wishes Phillip > +' > + > test_expect_success 'command line checks' ' > test_must_fail git check-attr && > test_must_fail git check-attr -- && > test_must_fail git check-attr test && > test_must_fail git check-attr test -- && > test_must_fail git check-attr -- f && > + test_must_fail git check-attr --source && > + test_must_fail git check-attr --source not-a-valid-ref && > echo "f" | test_must_fail git check-attr --stdin && > echo "f" | test_must_fail git check-attr --stdin -- f && > echo "f" | test_must_fail git check-attr --stdin test -- f && > @@ -287,6 +306,15 @@ test_expect_success 'using --git-dir and --work-tree' ' > ) > ' > > +test_expect_success 'using --source' ' > + attr_check_source foo/bar/f f tag-1 && > + attr_check_source foo/bar/a/i n tag-1 && > + attr_check_source foo/bar/f unspecified tag-2 && > + attr_check_source foo/bar/a/i m tag-2 && > + attr_check_source foo/bar/g g tag-2 && > + attr_check_source foo/bar/g unspecified tag-1 > +' > + > test_expect_success 'setup bare' ' > git clone --template= --bare . bare.git > ' > @@ -306,6 +334,18 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' ' > ) > ' > > +test_expect_success 'bare repository: with --source' ' > + ( > + cd bare.git && > + attr_check_source foo/bar/f f tag-1 && > + attr_check_source foo/bar/a/i n tag-1 && > + attr_check_source foo/bar/f unspecified tag-2 && > + attr_check_source foo/bar/a/i m tag-2 && > + attr_check_source foo/bar/g g tag-2 && > + attr_check_source foo/bar/g unspecified tag-1 > + ) > +' > + > test_expect_success 'bare repository: check that --cached honors index' ' > ( > cd bare.git && > diff --git a/userdiff.c b/userdiff.c > index 151d9a5278..b66f090a0b 100644 > --- a/userdiff.c > +++ b/userdiff.c > @@ -412,7 +412,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, path, check); > + git_check_attr(istate, NULL, path, check); > > if (ATTR_TRUE(check->items[0].value)) > return &driver_true; > diff --git a/ws.c b/ws.c > index 6e69877f25..eadbbe5667 100644 > --- a/ws.c > +++ b/ws.c > @@ -78,7 +78,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, pathname, attr_whitespace_rule); > + git_check_attr(istate, NULL, pathname, attr_whitespace_rule); > value = attr_whitespace_rule->items[0].value; > if (ATTR_TRUE(value)) { > /* true (whitespace) */
On Wed, Jan 11, 2023 at 12:30 PM Phillip Wood <phillip.wood123@gmail.com> 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@gmail.com> > > Signed-off-by: Toon Claes <toon@iotcl.com> > > Co-authored-by: toon@iotcl.com > > --- > > -'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.
On Mon, Jan 02 2023, Karthik Nayak wrote: > +static struct attr_stack *read_attr_from_blob(struct index_state *istate, > + const struct object_id *tree_oid, > + const char *path, unsigned flags) > +{ > + struct object_id oid; > + unsigned long sz; > + enum object_type type; > + void *buf; > + unsigned short mode; > + > + if (!tree_oid) > + return NULL; > + > + if (get_tree_entry(istate->repo, tree_oid, path, &oid, &mode)) > + return NULL; > + > + buf = read_object_file(&oid, &type, &sz); Here you flip-flop between istate->repo and "the_repository". I think you want to use repo_read_object_file(istate->repo, ...) instead.
On Thu, Jan 12, 2023 at 2:21 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > > On Mon, Jan 02 2023, Karthik Nayak wrote: > > > +static struct attr_stack *read_attr_from_blob(struct index_state *istate, > > + const struct object_id *tree_oid, > > + const char *path, unsigned flags) > > +{ > > + struct object_id oid; > > + unsigned long sz; > > + enum object_type type; > > + void *buf; > > + unsigned short mode; > > + > > + if (!tree_oid) > > + return NULL; > > + > > + if (get_tree_entry(istate->repo, tree_oid, path, &oid, &mode)) > > + return NULL; > > + > > + buf = read_object_file(&oid, &type, &sz); > > Here you flip-flop between istate->repo and "the_repository". I think > you want to use repo_read_object_file(istate->repo, ...) instead. Let me add this to v7! Thanks
diff --git a/Documentation/git-check-attr.txt b/Documentation/git-check-attr.txt index 84f41a8e82..fb15c44598 100644 --- a/Documentation/git-check-attr.txt +++ b/Documentation/git-check-attr.txt @@ -9,8 +9,8 @@ git-check-attr - Display gitattributes information SYNOPSIS -------- [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>...] 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. + \--:: Interpret all preceding arguments as attributes and all following arguments as path names. diff --git a/archive.c b/archive.c index 941495f5d7..81ff76fce9 100644 --- a/archive.c +++ b/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, path, check); + git_check_attr(istate, NULL, path, check); return check; } diff --git a/attr.c b/attr.c index 42ad6de8c7..a63f267187 100644 --- a/attr.c +++ b/attr.c @@ -13,6 +13,8 @@ #include "dir.h" #include "utf8.h" #include "quote.h" +#include "revision.h" +#include "object-store.h" #include "thread-utils.h" const char git_attr__true[] = "(builtin)true"; @@ -729,14 +731,62 @@ static struct attr_stack *read_attr_from_file(const char *path, unsigned flags) return res; } -static struct attr_stack *read_attr_from_index(struct index_state *istate, - const char *path, - unsigned flags) +static struct attr_stack *read_attr_from_buf(char *buf, const char *path, + unsigned flags) { struct attr_stack *res; - char *buf, *sp; + char *sp; int lineno = 0; + if (!buf) + return NULL; + + CALLOC_ARRAY(res, 1); + for (sp = buf; *sp;) { + char *ep; + int more; + + ep = strchrnul(sp, '\n'); + more = (*ep == '\n'); + *ep = '\0'; + handle_attr_line(res, sp, path, ++lineno, flags); + sp = ep + more; + } + free(buf); + + return res; +} + +static struct attr_stack *read_attr_from_blob(struct index_state *istate, + const struct object_id *tree_oid, + const char *path, unsigned flags) +{ + struct object_id oid; + unsigned long sz; + enum object_type type; + void *buf; + unsigned short mode; + + if (!tree_oid) + return NULL; + + if (get_tree_entry(istate->repo, tree_oid, path, &oid, &mode)) + return NULL; + + buf = read_object_file(&oid, &type, &sz); + if (!buf || type != OBJ_BLOB) { + free(buf); + return NULL; + } + + return read_attr_from_buf(buf, path, flags); +} + +static struct attr_stack *read_attr_from_index(struct index_state *istate, + const char *path, unsigned flags) +{ + char *buf; + if (!istate) return NULL; @@ -758,28 +808,19 @@ static struct attr_stack *read_attr_from_index(struct index_state *istate, if (!buf) return NULL; - CALLOC_ARRAY(res, 1); - for (sp = buf; *sp; ) { - char *ep; - int more; - - ep = strchrnul(sp, '\n'); - more = (*ep == '\n'); - *ep = '\0'; - handle_attr_line(res, sp, path, ++lineno, flags); - sp = ep + more; - } - free(buf); - return res; + return read_attr_from_buf(buf, path, flags); } static struct attr_stack *read_attr(struct index_state *istate, + const struct object_id *tree_oid, const char *path, unsigned flags) { struct attr_stack *res = NULL; if (direction == GIT_ATTR_INDEX) { res = read_attr_from_index(istate, path, flags); + } else if (tree_oid) { + res = read_attr_from_blob(istate, tree_oid, path, flags); } else if (!is_bare_repository()) { if (direction == GIT_ATTR_CHECKOUT) { res = read_attr_from_index(istate, path, flags); @@ -839,6 +880,7 @@ static void push_stack(struct attr_stack **attr_stack_p, } static void bootstrap_attr_stack(struct index_state *istate, + const struct object_id *tree_oid, struct attr_stack **stack) { struct attr_stack *e; @@ -864,7 +906,7 @@ static void bootstrap_attr_stack(struct index_state *istate, } /* root directory */ - e = read_attr(istate, GITATTRIBUTES_FILE, flags | READ_ATTR_NOFOLLOW); + e = read_attr(istate, tree_oid, GITATTRIBUTES_FILE, flags | READ_ATTR_NOFOLLOW); push_stack(stack, e, xstrdup(""), 0); /* info frame */ @@ -878,6 +920,7 @@ static void bootstrap_attr_stack(struct index_state *istate, } static void prepare_attr_stack(struct index_state *istate, + const struct object_id *tree_oid, const char *path, int dirlen, struct attr_stack **stack) { @@ -899,7 +942,7 @@ static void prepare_attr_stack(struct index_state *istate, * .gitattributes in deeper directories to shallower ones, * and finally use the built-in set as the default. */ - bootstrap_attr_stack(istate, stack); + bootstrap_attr_stack(istate, tree_oid, stack); /* * Pop the "info" one that is always at the top of the stack. @@ -954,7 +997,7 @@ static void prepare_attr_stack(struct index_state *istate, strbuf_add(&pathbuf, path + pathbuf.len, (len - pathbuf.len)); strbuf_addf(&pathbuf, "/%s", GITATTRIBUTES_FILE); - next = read_attr(istate, pathbuf.buf, READ_ATTR_NOFOLLOW); + next = read_attr(istate, tree_oid, pathbuf.buf, READ_ATTR_NOFOLLOW); /* reset the pathbuf to not include "/.gitattributes" */ strbuf_setlen(&pathbuf, len); @@ -1074,8 +1117,8 @@ static void determine_macros(struct all_attrs_item *all_attrs, * Otherwise all attributes are collected. */ static void collect_some_attrs(struct index_state *istate, - const char *path, - struct attr_check *check) + const struct object_id *tree_oid, + const char *path, struct attr_check *check) { int pathlen, rem, dirlen; const char *cp, *last_slash = NULL; @@ -1094,7 +1137,7 @@ static void collect_some_attrs(struct index_state *istate, dirlen = 0; } - prepare_attr_stack(istate, path, dirlen, &check->stack); + prepare_attr_stack(istate, tree_oid, path, dirlen, &check->stack); all_attrs_init(&g_attr_hashmap, check); determine_macros(check->all_attrs, check->stack); @@ -1103,12 +1146,12 @@ static void collect_some_attrs(struct index_state *istate, } void git_check_attr(struct index_state *istate, - const char *path, + const struct object_id *tree_oid, const char *path, struct attr_check *check) { int i; - collect_some_attrs(istate, path, check); + collect_some_attrs(istate, tree_oid, path, check); for (i = 0; i < check->nr; i++) { size_t n = check->items[i].attr->attr_nr; @@ -1119,13 +1162,13 @@ void git_check_attr(struct index_state *istate, } } -void git_all_attrs(struct index_state *istate, +void git_all_attrs(struct index_state *istate, const struct object_id *tree_oid, const char *path, struct attr_check *check) { int i; attr_check_reset(check); - collect_some_attrs(istate, path, check); + collect_some_attrs(istate, tree_oid, path, check); for (i = 0; i < check->all_attrs_nr; i++) { const char *name = check->all_attrs[i].attr->name; diff --git a/attr.h b/attr.h index 3fb40cced0..84a3279215 100644 --- a/attr.h +++ b/attr.h @@ -190,13 +190,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 char *path, struct attr_check *check); + const struct object_id *tree_oid, 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, +void git_all_attrs(struct index_state *istate, const struct object_id *tree_oid, const char *path, struct attr_check *check); enum git_attr_direction { diff --git a/builtin/check-attr.c b/builtin/check-attr.c index 0fef10eb6b..4ffef29d9c 100644 --- a/builtin/check-attr.c +++ b/builtin/check-attr.c @@ -9,9 +9,10 @@ static int all_attrs; static int cached_attrs; static int stdin_paths; +static char *source; 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>...]"), NULL }; @@ -23,6 +24,7 @@ static const struct option check_attr_options[] = { OPT_BOOL(0 , "stdin", &stdin_paths, N_("read file names from stdin")), OPT_BOOL('z', NULL, &nul_term_line, N_("terminate input and output records by a NUL character")), + OPT_STRING(0, "source", &source, N_("<tree-ish>"), N_("which tree-ish to check attributes at")), OPT_END() }; @@ -55,27 +57,26 @@ static void output_attr(struct attr_check *check, const char *file) } } -static void check_attr(const char *prefix, - struct attr_check *check, - int collect_all, +static void check_attr(const char *prefix, struct attr_check *check, + const struct object_id *tree_oid, int collect_all, const char *file) + { char *full_path = prefix_path(prefix, prefix ? strlen(prefix) : 0, file); if (collect_all) { - git_all_attrs(&the_index, full_path, check); + git_all_attrs(&the_index, tree_oid, full_path, check); } else { - git_check_attr(&the_index, full_path, check); + git_check_attr(&the_index, tree_oid, full_path, check); } output_attr(check, file); free(full_path); } -static void check_attr_stdin_paths(const char *prefix, - struct attr_check *check, - int collect_all) +static void check_attr_stdin_paths(const char *prefix, struct attr_check *check, + const struct object_id *tree_oid, int collect_all) { struct strbuf buf = STRBUF_INIT; struct strbuf unquoted = STRBUF_INIT; @@ -89,7 +90,7 @@ static void check_attr_stdin_paths(const char *prefix, die("line is badly quoted"); strbuf_swap(&buf, &unquoted); } - check_attr(prefix, check, collect_all, buf.buf); + check_attr(prefix, check, tree_oid, collect_all, buf.buf); maybe_flush_or_die(stdout, "attribute to stdout"); } strbuf_release(&buf); @@ -105,6 +106,8 @@ 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; if (!is_bare_repository()) @@ -176,11 +179,17 @@ 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 revision", source); + tree_oid = &initialized_oid; + } + if (stdin_paths) - check_attr_stdin_paths(prefix, check, all_attrs); + check_attr_stdin_paths(prefix, check, tree_oid, all_attrs); else { for (i = filei; i < argc; i++) - check_attr(prefix, check, all_attrs, argv[i]); + check_attr(prefix, check, tree_oid, all_attrs, argv[i]); maybe_flush_or_die(stdout, "attribute to stdout"); } diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 573d0b20b7..89535cfa6a 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -1318,7 +1318,7 @@ static int no_try_delta(const char *path) if (!check) check = attr_check_initl("delta", NULL); - git_check_attr(the_repository->index, path, check); + git_check_attr(the_repository->index, NULL, path, check); if (ATTR_FALSE(check->items[0].value)) return 1; return 0; diff --git a/convert.c b/convert.c index 9b67649032..a54d1690c0 100644 --- a/convert.c +++ b/convert.c @@ -1308,7 +1308,7 @@ void convert_attrs(struct index_state *istate, git_config(read_convert_config, NULL); } - git_check_attr(istate, path, check); + git_check_attr(istate, NULL, path, check); ccheck = check->items; ca->crlf_action = git_path_check_crlf(ccheck + 4); if (ca->crlf_action == CRLF_UNDEFINED) diff --git a/ll-merge.c b/ll-merge.c index 22a603e8af..130d26501c 100644 --- a/ll-merge.c +++ b/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, path, check); + git_check_attr(istate, NULL, 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, path, check); + git_check_attr(istate, NULL, path, check); if (check->items[0].value) { marker_size = atoi(check->items[0].value); if (marker_size <= 0) diff --git a/pathspec.c b/pathspec.c index 46e77a85fe..48dec2c709 100644 --- a/pathspec.c +++ b/pathspec.c @@ -732,7 +732,7 @@ int match_pathspec_attrs(struct index_state *istate, if (name[namelen]) name = to_free = xmemdupz(name, namelen); - git_check_attr(istate, name, item->attr_check); + git_check_attr(istate, NULL, name, item->attr_check); free(to_free); 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 + test_must_be_empty err } test_expect_success 'open-quoted pathname' ' @@ -33,7 +41,6 @@ test_expect_success 'open-quoted pathname' ' attr_check a unspecified ' - test_expect_success 'setup' ' mkdir -p a/b/d a/c b && ( @@ -80,12 +87,24 @@ test_expect_success 'setup' ' EOF ' +test_expect_success 'setup branches' ' + mkdir -p foo/bar && + test_commit --printf "add .gitattributes" foo/bar/.gitattribute \ + "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_expect_success 'command line checks' ' test_must_fail git check-attr && test_must_fail git check-attr -- && test_must_fail git check-attr test && test_must_fail git check-attr test -- && test_must_fail git check-attr -- f && + test_must_fail git check-attr --source && + test_must_fail git check-attr --source not-a-valid-ref && echo "f" | test_must_fail git check-attr --stdin && echo "f" | test_must_fail git check-attr --stdin -- f && echo "f" | test_must_fail git check-attr --stdin test -- f && @@ -287,6 +306,15 @@ test_expect_success 'using --git-dir and --work-tree' ' ) ' +test_expect_success 'using --source' ' + attr_check_source foo/bar/f f tag-1 && + attr_check_source foo/bar/a/i n tag-1 && + attr_check_source foo/bar/f unspecified tag-2 && + attr_check_source foo/bar/a/i m tag-2 && + attr_check_source foo/bar/g g tag-2 && + attr_check_source foo/bar/g unspecified tag-1 +' + test_expect_success 'setup bare' ' git clone --template= --bare . bare.git ' @@ -306,6 +334,18 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' ' ) ' +test_expect_success 'bare repository: with --source' ' + ( + cd bare.git && + attr_check_source foo/bar/f f tag-1 && + attr_check_source foo/bar/a/i n tag-1 && + attr_check_source foo/bar/f unspecified tag-2 && + attr_check_source foo/bar/a/i m tag-2 && + attr_check_source foo/bar/g g tag-2 && + attr_check_source foo/bar/g unspecified tag-1 + ) +' + test_expect_success 'bare repository: check that --cached honors index' ' ( cd bare.git && diff --git a/userdiff.c b/userdiff.c index 151d9a5278..b66f090a0b 100644 --- a/userdiff.c +++ b/userdiff.c @@ -412,7 +412,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, path, check); + git_check_attr(istate, NULL, path, check); if (ATTR_TRUE(check->items[0].value)) return &driver_true; diff --git a/ws.c b/ws.c index 6e69877f25..eadbbe5667 100644 --- a/ws.c +++ b/ws.c @@ -78,7 +78,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, pathname, attr_whitespace_rule); + git_check_attr(istate, NULL, pathname, attr_whitespace_rule); value = attr_whitespace_rule->items[0].value; if (ATTR_TRUE(value)) { /* true (whitespace) */