{"thread":{"id":"58966","subject":"[PATCH v3 0/2] check-attr: add support to work with revisions","startedAt":"2022-12-16T09:36:03Z","lastAt":"2022-12-21T06:34:37Z","messageCount":19,"participants":["Karthik Nayak","Ævar Arnfjörð Bjarmason","Junio C Hamano","Phillip Wood","Toon Claes"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"469164","messageId":"20221216093552.3171319-1-karthik.188@gmail.com","threadId":"58966","inReplyTo":null,"subject":"[PATCH v3 0/2] check-attr: add support to work with revisions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2022-12-16T09:35:50Z","receivedAt":"2022-12-16T09:36:03Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"v1: https://lore.kernel.org/git/20221206103736.53909-1-karthik.188@gmail.com/\nv2: https://lore.kernel.org/git/CAOLa=ZSsFGBw3ta1jWN8cmUch2ca=zTEjp1xMA6Linafx9W53g@mail.gmail.com/T/#t\n\nGiven a pathname, git-check-attr(1) will list the attributes which apply to that\npathname by reading all relevant gitattributes files. Currently there is no way\nto specify a revision to read the gitattributes from.\n\nThis is specifically useful in bare repositories wherein the gitattributes are\nonly present in the git working tree but not available directly on the\nfilesystem.\n\nThis series aims to add a new flag `-r|--revisions` to git-check-attr(1) which\nallows us to read gitattributes from the specified revision.\n\nChanges since version 2:\n- Changes to the commit message [1/2] to use more specific terms and to\n  be more descriptive.\n- Moved the flag's position in the documentation to be before the unbound\n  list of non-options.\n\nRange-diff against v2:\n\n1:  2e71cbbddd < -:  ---------- Git 2.39-rc2\n-:  ---------- > 1:  57e2c6ebbe Start the 2.40 cycle\n2:  898041f243 = 2:  c386de2d42 t0003: move setup for `--all` into new block\n3:  12a72e09e0 ! 3:  b93a68b0c9 attr: add flag `-r|--revisions` to work with revisions\n    @@ Metadata\n      ## Commit message ##\n         attr: add flag `-r|--revisions` to work with revisions\n     \n    -    Git check-attr currently doesn't check the git worktree, it either\n    -    checks the index or the files directly. This means we cannot check the\n    -    attributes for a file against a certain revision.\n    +    The contents of the .gitattributes files may evolve over time, but \"git\n    +    check-attr\" always checks attributes against them in the working tree\n    +    and/or in the index. It may be beneficial to optionally allow the users\n    +    to check attributes against paths from older commits.\n     \n    -    Add a new flag `--revision`/`-r` which will allow it work with\n    -    revisions. This command will now, instead of checking the files/index,\n    -    try and receive the blob for the given attribute file against the\n    -    provided revision. The flag overrides checking against the index and\n    -    filesystem and also works with bare repositories.\n    +    Add a new flag `--revision`/`-r` which will allow users to check the\n    +    attributes against a tree-ish revision. When the user uses this flag, we\n    +    go through the stack of .gitattributes files but instead of checking the\n    +    current working tree and/or in the index, we check the blobs from the\n    +    provided tree-ish object. This allows the command to also be used in\n    +    bare repositories.\n    +\n    +    Since we use a tree-ish object, the user can pass \"-r HEAD:subdirectory\"\n    +    and all the attributes will be looked up as if subdirectory was the root\n    +    directory of the repository.\n     \n         We cannot use the `<rev>:<path>` syntax like the one used in `git show`\n         because any non-flag parameter before `--` is treated as an attribute\n         and any parameter after `--` is treated as a pathname.\n     \n    -    This involves creating a new function `read_attr_from_blob`, which given\n    -    the path reads the blob for the path against the provided revision and\n    +    The change involves creating a new function `read_attr_from_blob`, which\n    +    given the path reads the blob for the path against the provided revision and\n         parses the attributes line by line. This function is plugged into\n    -    `read_attr()` function wherein we go through the different attributes.\n    +    `read_attr()` function wherein we go through the stack of attributes\n    +    files.\n     \n         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n         Co-authored-by: toon@iotcl.com\n    @@ Documentation/git-check-attr.txt: git-check-attr - Display gitattributes informa\n      [verse]\n     -'git check-attr' [-a | --all | <attr>...] [--] <pathname>...\n     -'git check-attr' --stdin [-z] [-a | --all | <attr>...]\n    -+'git check-attr' [-a | --all | <attr>...] [-r <revision>] [--] <pathname>...\n    -+'git check-attr' --stdin [-z] [-a | --all | <attr>...] [-r <revision>]\n    ++'git check-attr' [-r <revision>] [-a | --all | <attr>...] [--] <pathname>...\n    ++'git check-attr' --stdin [-z] [-r <revision>] [-a | --all | <attr>...]\n      \n      DESCRIPTION\n      -----------\n    @@ Documentation/git-check-attr.txt: OPTIONS\n      \n     +--r <revision>::\n     +--revision=<revision>::\n    -+\tCheck attributes against the specified revision.\n    ++\tCheck attributes against the specified tree-ish revision. All the\n    ++\tattributes will be checked against the provided revision. Paths provided\n    ++\tas part of the revision will be treated as the root directory.\n     +\n      \\--::\n      \tInterpret all preceding arguments as attributes and all following\n    @@ builtin/check-attr.c\n      static const char * const check_attr_usage[] = {\n     -N_(\"git check-attr [-a | --all | <attr>...] [--] <pathname>...\"),\n     -N_(\"git check-attr --stdin [-z] [-a | --all | <attr>...]\"),\n    -+N_(\"git check-attr [-a | --all | <attr>...] [-r <revision>] [--] <pathname>...\"),\n    -+N_(\"git check-attr --stdin [-z] [-a | --all | <attr>...] [-r <revision>]\"),\n    ++N_(\"git check-attr [-r <revision>] [-a | --all | <attr>...] [--] <pathname>...\"),\n    ++N_(\"git check-attr --stdin [-z] [-r <revision>] [-a | --all | <attr>...]\"),\n      NULL\n      };\n      \n\n\nKarthik Nayak (2):\n  t0003: move setup for `--all` into new block\n  attr: add flag `-r|--revisions` to work with revisions\n\n Documentation/git-check-attr.txt |  10 +++-\n archive.c                        |   2 +-\n attr.c                           | 100 ++++++++++++++++++++++---------\n attr.h                           |   7 ++-\n builtin/check-attr.c             |  33 ++++++----\n builtin/pack-objects.c           |   2 +-\n convert.c                        |   2 +-\n ll-merge.c                       |   4 +-\n pathspec.c                       |   2 +-\n t/t0003-attributes.sh            |  71 +++++++++++++++++++++-\n userdiff.c                       |   2 +-\n ws.c                             |   2 +-\n 12 files changed, 182 insertions(+), 55 deletions(-)\n\n-- \n2.39.0\n\n"},{"id":"469165","messageId":"20221216093552.3171319-2-karthik.188@gmail.com","threadId":"58966","inReplyTo":"20221216093552.3171319-1-karthik.188@gmail.com","subject":"[PATCH v3 1/2] t0003: move setup for `--all` into new block","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2022-12-16T09:35:51Z","receivedAt":"2022-12-16T09:36:04Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"There is some setup code which is used by multiple tests being setup in\n`attribute test: --all option`. This means when we run \"sh\n./t0003-attributes.sh --run=setup,<num>\" there is a chance of failing\nsince we missed this setup block.\n\nSo to ensure that setups are independent of test logic, move this to a\nnew setup block.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\nCo-authored-by: toon@iotcl.com\n---\n t/t0003-attributes.sh | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex f7ee2f2ff0..b3aabb8aa3 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -203,9 +203,12 @@ test_expect_success 'attribute test: read paths from stdin' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'attribute test: --all option' '\n+test_expect_success 'setup --all option' '\n \tgrep -v unspecified <expect-all | sort >specified-all &&\n-\tsed -e \"s/:.*//\" <expect-all | uniq >stdin-all &&\n+\tsed -e \"s/:.*//\" <expect-all | uniq >stdin-all\n+'\n+\n+test_expect_success 'attribute test: --all option' '\n \tgit check-attr --stdin --all <stdin-all >tmp &&\n \tsort tmp >actual &&\n \ttest_cmp specified-all actual\n-- \n2.39.0\n\n"},{"id":"469166","messageId":"20221216093552.3171319-3-karthik.188@gmail.com","threadId":"58966","inReplyTo":"20221216093552.3171319-1-karthik.188@gmail.com","subject":"[PATCH v3 2/2] attr: add flag `-r|--revisions` to work with revisions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2022-12-16T09:35:52Z","receivedAt":"2022-12-16T09:36:08Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The contents of the .gitattributes files may evolve over time, but \"git\ncheck-attr\" always checks attributes against them in the working tree\nand/or in the index. It may be beneficial to optionally allow the users\nto check attributes against paths from older commits.\n\nAdd a new flag `--revision`/`-r` which will allow users to check the\nattributes against a tree-ish revision. When the user uses this flag, we\ngo through the stack of .gitattributes files but instead of checking the\ncurrent working tree and/or in the index, we check the blobs from the\nprovided tree-ish object. This allows the command to also be used in\nbare repositories.\n\nSince we use a tree-ish object, the user can pass \"-r HEAD:subdirectory\"\nand all the attributes will be looked up as if subdirectory was the root\ndirectory of the repository.\n\nWe cannot use the `<rev>:<path>` syntax like the one used in `git show`\nbecause any non-flag parameter before `--` is treated as an attribute\nand any parameter after `--` is treated as a pathname.\n\nThe change involves creating a new function `read_attr_from_blob`, which\ngiven the path reads the blob for the path against the provided revision and\nparses the attributes line by line. This function is plugged into\n`read_attr()` function wherein we go through the stack of attributes\nfiles.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\nCo-authored-by: toon@iotcl.com\n---\n Documentation/git-check-attr.txt |  10 +++-\n archive.c                        |   2 +-\n attr.c                           | 100 ++++++++++++++++++++++---------\n attr.h                           |   7 ++-\n builtin/check-attr.c             |  33 ++++++----\n builtin/pack-objects.c           |   2 +-\n convert.c                        |   2 +-\n ll-merge.c                       |   4 +-\n pathspec.c                       |   2 +-\n t/t0003-attributes.sh            |  64 +++++++++++++++++++-\n userdiff.c                       |   2 +-\n ws.c                             |   2 +-\n 12 files changed, 177 insertions(+), 53 deletions(-)\n\ndiff --git a/Documentation/git-check-attr.txt b/Documentation/git-check-attr.txt\nindex 84f41a8e82..f9757d9136 100644\n--- a/Documentation/git-check-attr.txt\n+++ b/Documentation/git-check-attr.txt\n@@ -9,8 +9,8 @@ git-check-attr - Display gitattributes information\n SYNOPSIS\n --------\n [verse]\n-'git check-attr' [-a | --all | <attr>...] [--] <pathname>...\n-'git check-attr' --stdin [-z] [-a | --all | <attr>...]\n+'git check-attr' [-r <revision>] [-a | --all | <attr>...] [--] <pathname>...\n+'git check-attr' --stdin [-z] [-r <revision>] [-a | --all | <attr>...]\n \n DESCRIPTION\n -----------\n@@ -36,6 +36,12 @@ OPTIONS\n \tIf `--stdin` is also given, input paths are separated\n \twith a NUL character instead of a linefeed character.\n \n+--r <revision>::\n+--revision=<revision>::\n+\tCheck attributes against the specified tree-ish revision. All the\n+\tattributes will be checked against the provided revision. Paths provided\n+\tas part of the revision will be treated as the root directory.\n+\n \\--::\n \tInterpret all preceding arguments as attributes and all following\n \targuments as path names.\ndiff --git a/archive.c b/archive.c\nindex 941495f5d7..81ff76fce9 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -120,7 +120,7 @@ static const struct attr_check *get_archive_attrs(struct index_state *istate,\n \tstatic struct attr_check *check;\n \tif (!check)\n \t\tcheck = attr_check_initl(\"export-ignore\", \"export-subst\", NULL);\n-\tgit_check_attr(istate, path, check);\n+\tgit_check_attr(istate, NULL, path, check);\n \treturn check;\n }\n \ndiff --git a/attr.c b/attr.c\nindex 42ad6de8c7..99883024ec 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -11,8 +11,13 @@\n #include \"exec-cmd.h\"\n #include \"attr.h\"\n #include \"dir.h\"\n+#include \"git-compat-util.h\"\n+#include \"strbuf.h\"\n+#include \"tree-walk.h\"\n #include \"utf8.h\"\n #include \"quote.h\"\n+#include \"revision.h\"\n+#include \"object-store.h\"\n #include \"thread-utils.h\"\n \n const char git_attr__true[] = \"(builtin)true\";\n@@ -729,14 +734,62 @@ static struct attr_stack *read_attr_from_file(const char *path, unsigned flags)\n \treturn res;\n }\n \n-static struct attr_stack *read_attr_from_index(struct index_state *istate,\n-\t\t\t\t\t       const char *path,\n-\t\t\t\t\t       unsigned flags)\n+static struct attr_stack *read_attr_from_buf(char *buf, const char *path,\n+\t\t\t\t\t     unsigned flags)\n {\n \tstruct attr_stack *res;\n-\tchar *buf, *sp;\n+\tchar *sp;\n \tint lineno = 0;\n \n+\tif (!buf)\n+\t\treturn NULL;\n+\n+\tCALLOC_ARRAY(res, 1);\n+\tfor (sp = buf; *sp;) {\n+\t\tchar *ep;\n+\t\tint more;\n+\n+\t\tep = strchrnul(sp, '\\n');\n+\t\tmore = (*ep == '\\n');\n+\t\t*ep = '\\0';\n+\t\thandle_attr_line(res, sp, path, ++lineno, flags);\n+\t\tsp = ep + more;\n+\t}\n+\tfree(buf);\n+\n+\treturn res;\n+}\n+\n+static struct attr_stack *read_attr_from_blob(struct index_state *istate,\n+\t\t\t\t\t      const struct object_id *tree_oid,\n+\t\t\t\t\t      const char *path, unsigned flags)\n+{\n+\tstruct object_id oid;\n+\tunsigned long sz;\n+\tenum object_type type;\n+\tvoid *buf;\n+\tunsigned short mode;\n+\n+\tif (!tree_oid)\n+\t\treturn NULL;\n+\n+\tif (get_tree_entry(istate->repo, tree_oid, path, &oid, &mode))\n+\t\treturn NULL;\n+\n+\tbuf = read_object_file(&oid, &type, &sz);\n+\tif (!buf || type != OBJ_BLOB) {\n+\t\tfree(buf);\n+\t\treturn NULL;\n+\t}\n+\n+\treturn read_attr_from_buf(buf, path, flags);\n+}\n+\n+static struct attr_stack *read_attr_from_index(struct index_state *istate,\n+\t\t\t\t\t       const char *path, unsigned flags)\n+{\n+\tchar *buf;\n+\n \tif (!istate)\n \t\treturn NULL;\n \n@@ -758,28 +811,19 @@ static struct attr_stack *read_attr_from_index(struct index_state *istate,\n \tif (!buf)\n \t\treturn NULL;\n \n-\tCALLOC_ARRAY(res, 1);\n-\tfor (sp = buf; *sp; ) {\n-\t\tchar *ep;\n-\t\tint more;\n-\n-\t\tep = strchrnul(sp, '\\n');\n-\t\tmore = (*ep == '\\n');\n-\t\t*ep = '\\0';\n-\t\thandle_attr_line(res, sp, path, ++lineno, flags);\n-\t\tsp = ep + more;\n-\t}\n-\tfree(buf);\n-\treturn res;\n+\treturn read_attr_from_buf(buf, path, flags);\n }\n \n static struct attr_stack *read_attr(struct index_state *istate,\n+\t\t\t\t    const struct object_id *tree_oid,\n \t\t\t\t    const char *path, unsigned flags)\n {\n \tstruct attr_stack *res = NULL;\n \n \tif (direction == GIT_ATTR_INDEX) {\n \t\tres = read_attr_from_index(istate, path, flags);\n+\t} else if (tree_oid) {\n+\t\tres = read_attr_from_blob(istate, tree_oid, path, flags);\n \t} else if (!is_bare_repository()) {\n \t\tif (direction == GIT_ATTR_CHECKOUT) {\n \t\t\tres = read_attr_from_index(istate, path, flags);\n@@ -839,6 +883,7 @@ static void push_stack(struct attr_stack **attr_stack_p,\n }\n \n static void bootstrap_attr_stack(struct index_state *istate,\n+\t\t\t\t const struct object_id *tree_oid,\n \t\t\t\t struct attr_stack **stack)\n {\n \tstruct attr_stack *e;\n@@ -864,7 +909,7 @@ static void bootstrap_attr_stack(struct index_state *istate,\n \t}\n \n \t/* root directory */\n-\te = read_attr(istate, GITATTRIBUTES_FILE, flags | READ_ATTR_NOFOLLOW);\n+\te = read_attr(istate, tree_oid, GITATTRIBUTES_FILE, flags | READ_ATTR_NOFOLLOW);\n \tpush_stack(stack, e, xstrdup(\"\"), 0);\n \n \t/* info frame */\n@@ -878,6 +923,7 @@ static void bootstrap_attr_stack(struct index_state *istate,\n }\n \n static void prepare_attr_stack(struct index_state *istate,\n+\t\t\t       const struct object_id *tree_oid,\n \t\t\t       const char *path, int dirlen,\n \t\t\t       struct attr_stack **stack)\n {\n@@ -899,7 +945,7 @@ static void prepare_attr_stack(struct index_state *istate,\n \t * .gitattributes in deeper directories to shallower ones,\n \t * and finally use the built-in set as the default.\n \t */\n-\tbootstrap_attr_stack(istate, stack);\n+\tbootstrap_attr_stack(istate, tree_oid, stack);\n \n \t/*\n \t * Pop the \"info\" one that is always at the top of the stack.\n@@ -954,7 +1000,7 @@ static void prepare_attr_stack(struct index_state *istate,\n \t\tstrbuf_add(&pathbuf, path + pathbuf.len, (len - pathbuf.len));\n \t\tstrbuf_addf(&pathbuf, \"/%s\", GITATTRIBUTES_FILE);\n \n-\t\tnext = read_attr(istate, pathbuf.buf, READ_ATTR_NOFOLLOW);\n+\t\tnext = read_attr(istate, tree_oid, pathbuf.buf, READ_ATTR_NOFOLLOW);\n \n \t\t/* reset the pathbuf to not include \"/.gitattributes\" */\n \t\tstrbuf_setlen(&pathbuf, len);\n@@ -1074,8 +1120,8 @@ static void determine_macros(struct all_attrs_item *all_attrs,\n  * Otherwise all attributes are collected.\n  */\n static void collect_some_attrs(struct index_state *istate,\n-\t\t\t       const char *path,\n-\t\t\t       struct attr_check *check)\n+\t\t\t       const struct object_id *tree_oid,\n+\t\t\t       const char *path, struct attr_check *check)\n {\n \tint pathlen, rem, dirlen;\n \tconst char *cp, *last_slash = NULL;\n@@ -1094,7 +1140,7 @@ static void collect_some_attrs(struct index_state *istate,\n \t\tdirlen = 0;\n \t}\n \n-\tprepare_attr_stack(istate, path, dirlen, &check->stack);\n+\tprepare_attr_stack(istate, tree_oid, path, dirlen, &check->stack);\n \tall_attrs_init(&g_attr_hashmap, check);\n \tdetermine_macros(check->all_attrs, check->stack);\n \n@@ -1103,12 +1149,12 @@ static void collect_some_attrs(struct index_state *istate,\n }\n \n void git_check_attr(struct index_state *istate,\n-\t\t    const char *path,\n+\t\t    const struct object_id *tree_oid, const char *path,\n \t\t    struct attr_check *check)\n {\n \tint i;\n \n-\tcollect_some_attrs(istate, path, check);\n+\tcollect_some_attrs(istate, tree_oid, path, check);\n \n \tfor (i = 0; i < check->nr; i++) {\n \t\tsize_t n = check->items[i].attr->attr_nr;\n@@ -1119,13 +1165,13 @@ void git_check_attr(struct index_state *istate,\n \t}\n }\n \n-void git_all_attrs(struct index_state *istate,\n+void git_all_attrs(struct index_state *istate, const struct object_id *tree_oid,\n \t\t   const char *path, struct attr_check *check)\n {\n \tint i;\n \n \tattr_check_reset(check);\n-\tcollect_some_attrs(istate, path, check);\n+\tcollect_some_attrs(istate, tree_oid, path, check);\n \n \tfor (i = 0; i < check->all_attrs_nr; i++) {\n \t\tconst char *name = check->all_attrs[i].attr->name;\ndiff --git a/attr.h b/attr.h\nindex 3fb40cced0..f4a2bedd68 100644\n--- a/attr.h\n+++ b/attr.h\n@@ -1,6 +1,8 @@\n #ifndef ATTR_H\n #define ATTR_H\n \n+#include \"hash.h\"\n+\n /**\n  * gitattributes mechanism gives a uniform way to associate various attributes\n  * to set of paths.\n@@ -190,13 +192,14 @@ void attr_check_free(struct attr_check *check);\n const char *git_attr_name(const struct git_attr *);\n \n void git_check_attr(struct index_state *istate,\n-\t\t    const char *path, struct attr_check *check);\n+\t\t    const struct object_id *tree_oid, const char *path,\n+\t\t    struct attr_check *check);\n \n /*\n  * Retrieve all attributes that apply to the specified path.\n  * check holds the attributes and their values.\n  */\n-void git_all_attrs(struct index_state *istate,\n+void git_all_attrs(struct index_state *istate, const struct object_id *tree_oid,\n \t\t   const char *path, struct attr_check *check);\n \n enum git_attr_direction {\ndiff --git a/builtin/check-attr.c b/builtin/check-attr.c\nindex 0fef10eb6b..eba3d7835c 100644\n--- a/builtin/check-attr.c\n+++ b/builtin/check-attr.c\n@@ -1,3 +1,4 @@\n+#include \"repository.h\"\n #define USE_THE_INDEX_VARIABLE\n #include \"builtin.h\"\n #include \"cache.h\"\n@@ -9,9 +10,10 @@\n static int all_attrs;\n static int cached_attrs;\n static int stdin_paths;\n+static char *revision;\n static const char * const check_attr_usage[] = {\n-N_(\"git check-attr [-a | --all | <attr>...] [--] <pathname>...\"),\n-N_(\"git check-attr --stdin [-z] [-a | --all | <attr>...]\"),\n+N_(\"git check-attr [-r <revision>] [-a | --all | <attr>...] [--] <pathname>...\"),\n+N_(\"git check-attr --stdin [-z] [-r <revision>] [-a | --all | <attr>...]\"),\n NULL\n };\n \n@@ -23,6 +25,7 @@ static const struct option check_attr_options[] = {\n \tOPT_BOOL(0 , \"stdin\", &stdin_paths, N_(\"read file names from stdin\")),\n \tOPT_BOOL('z', NULL, &nul_term_line,\n \t\t N_(\"terminate input and output records by a NUL character\")),\n+\tOPT_STRING('r', \"revision\", &revision, N_(\"revision\"), N_(\"check attributes at this revision\")),\n \tOPT_END()\n };\n \n@@ -55,27 +58,26 @@ static void output_attr(struct attr_check *check, const char *file)\n \t}\n }\n \n-static void check_attr(const char *prefix,\n-\t\t       struct attr_check *check,\n-\t\t       int collect_all,\n+static void check_attr(const char *prefix, struct attr_check *check,\n+\t\t       const struct object_id *tree_oid, int collect_all,\n \t\t       const char *file)\n+\n {\n \tchar *full_path =\n \t\tprefix_path(prefix, prefix ? strlen(prefix) : 0, file);\n \n \tif (collect_all) {\n-\t\tgit_all_attrs(&the_index, full_path, check);\n+\t\tgit_all_attrs(&the_index, tree_oid, full_path, check);\n \t} else {\n-\t\tgit_check_attr(&the_index, full_path, check);\n+\t\tgit_check_attr(&the_index, tree_oid, full_path, check);\n \t}\n \toutput_attr(check, file);\n \n \tfree(full_path);\n }\n \n-static void check_attr_stdin_paths(const char *prefix,\n-\t\t\t\t   struct attr_check *check,\n-\t\t\t\t   int collect_all)\n+static void check_attr_stdin_paths(const char *prefix, struct attr_check *check,\n+\t\t\t\t   const struct object_id *tree_oid, int collect_all)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct strbuf unquoted = STRBUF_INIT;\n@@ -89,7 +91,7 @@ static void check_attr_stdin_paths(const char *prefix,\n \t\t\t\tdie(\"line is badly quoted\");\n \t\t\tstrbuf_swap(&buf, &unquoted);\n \t\t}\n-\t\tcheck_attr(prefix, check, collect_all, buf.buf);\n+\t\tcheck_attr(prefix, check, tree_oid, collect_all, buf.buf);\n \t\tmaybe_flush_or_die(stdout, \"attribute to stdout\");\n \t}\n \tstrbuf_release(&buf);\n@@ -105,6 +107,7 @@ static NORETURN void error_with_usage(const char *msg)\n int cmd_check_attr(int argc, const char **argv, const char *prefix)\n {\n \tstruct attr_check *check;\n+\tstruct object_id tree_oid;\n \tint cnt, i, doubledash, filei;\n \n \tif (!is_bare_repository())\n@@ -176,11 +179,15 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n+\tif (revision)\n+\t\tif (repo_get_oid_tree(the_repository, revision, &tree_oid))\n+\t\t\terror(\"%s: not a valid revision\", revision);\n+\n \tif (stdin_paths)\n-\t\tcheck_attr_stdin_paths(prefix, check, all_attrs);\n+\t\tcheck_attr_stdin_paths(prefix, check, &tree_oid, all_attrs);\n \telse {\n \t\tfor (i = filei; i < argc; i++)\n-\t\t\tcheck_attr(prefix, check, all_attrs, argv[i]);\n+\t\t\tcheck_attr(prefix, check, &tree_oid, all_attrs, argv[i]);\n \t\tmaybe_flush_or_die(stdout, \"attribute to stdout\");\n \t}\n \ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 2193f80b89..cabace4abb 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1318,7 +1318,7 @@ static int no_try_delta(const char *path)\n \n \tif (!check)\n \t\tcheck = attr_check_initl(\"delta\", NULL);\n-\tgit_check_attr(the_repository->index, path, check);\n+\tgit_check_attr(the_repository->index, NULL, path, check);\n \tif (ATTR_FALSE(check->items[0].value))\n \t\treturn 1;\n \treturn 0;\ndiff --git a/convert.c b/convert.c\nindex 9b67649032..a54d1690c0 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -1308,7 +1308,7 @@ void convert_attrs(struct index_state *istate,\n \t\tgit_config(read_convert_config, NULL);\n \t}\n \n-\tgit_check_attr(istate, path, check);\n+\tgit_check_attr(istate, NULL, path, check);\n \tccheck = check->items;\n \tca->crlf_action = git_path_check_crlf(ccheck + 4);\n \tif (ca->crlf_action == CRLF_UNDEFINED)\ndiff --git a/ll-merge.c b/ll-merge.c\nindex 22a603e8af..130d26501c 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -391,7 +391,7 @@ enum ll_merge_result ll_merge(mmbuffer_t *result_buf,\n \t\tnormalize_file(theirs, path, istate);\n \t}\n \n-\tgit_check_attr(istate, path, check);\n+\tgit_check_attr(istate, NULL, path, check);\n \tll_driver_name = check->items[0].value;\n \tif (check->items[1].value) {\n \t\tmarker_size = atoi(check->items[1].value);\n@@ -419,7 +419,7 @@ int ll_merge_marker_size(struct index_state *istate, const char *path)\n \n \tif (!check)\n \t\tcheck = attr_check_initl(\"conflict-marker-size\", NULL);\n-\tgit_check_attr(istate, path, check);\n+\tgit_check_attr(istate, NULL, path, check);\n \tif (check->items[0].value) {\n \t\tmarker_size = atoi(check->items[0].value);\n \t\tif (marker_size <= 0)\ndiff --git a/pathspec.c b/pathspec.c\nindex 46e77a85fe..48dec2c709 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -732,7 +732,7 @@ int match_pathspec_attrs(struct index_state *istate,\n \tif (name[namelen])\n \t\tname = to_free = xmemdupz(name, namelen);\n \n-\tgit_check_attr(istate, name, item->attr_check);\n+\tgit_check_attr(istate, NULL, name, item->attr_check);\n \n \tfree(to_free);\n \ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex b3aabb8aa3..042af0239f 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -25,7 +25,14 @@ attr_check_quote () {\n \tgit check-attr test -- \"$path\" >actual &&\n \techo \"\\\"$quoted_path\\\": test: $expect\" >expect &&\n \ttest_cmp expect actual\n+}\n+\n+attr_check_revision () {\n+\tpath=\"$1\" expect=\"$2\" revision=\"$3\" git_opts=\"$4\" &&\n \n+\tgit $git_opts check-attr -r $revision test -- \"$path\" >actual 2>err &&\n+\techo \"$path: test: $expect\" >expect &&\n+\ttest_cmp expect actual\n }\n \n test_expect_success 'open-quoted pathname' '\n@@ -33,7 +40,6 @@ test_expect_success 'open-quoted pathname' '\n \tattr_check a unspecified\n '\n \n-\n test_expect_success 'setup' '\n \tmkdir -p a/b/d a/c b &&\n \t(\n@@ -80,12 +86,38 @@ test_expect_success 'setup' '\n \tEOF\n '\n \n+test_expect_success 'setup branches' '\n+\t(\n+\t\techo \"f\ttest=f\" &&\n+\t\techo \"a/i test=n\"\n+\t) | git hash-object -w --stdin >id &&\n+\tgit update-index --add --cacheinfo 100644,$(cat id),foo/bar/.gitattributes &&\n+\tgit write-tree >id &&\n+\ttree_id=$(cat id) &&\n+\tgit commit-tree $tree_id -m \"random commit message\" >id &&\n+\tcommit_id=$(cat id) &&\n+\tgit update-ref refs/heads/branch1 $commit_id &&\n+\n+\t(\n+\t\techo \"g test=g\" &&\n+\t\techo \"a/i test=m\"\n+\t) | git hash-object -w --stdin >id &&\n+\tgit update-index --add --cacheinfo 100644,$(cat id),foo/bar/.gitattributes &&\n+\tgit write-tree >id &&\n+\ttree_id=$(cat id) &&\n+\tgit commit-tree $tree_id -m \"random commit message\" >id &&\n+\tcommit_id=$(cat id) &&\n+\tgit update-ref refs/heads/branch2 $commit_id\n+'\n+\n test_expect_success 'command line checks' '\n \ttest_must_fail git check-attr &&\n \ttest_must_fail git check-attr -- &&\n \ttest_must_fail git check-attr test &&\n \ttest_must_fail git check-attr test -- &&\n \ttest_must_fail git check-attr -- f &&\n+\ttest_must_fail git check-attr -r &&\n+\ttest_must_fail git check-attr -r not-a-valid-ref &&\n \techo \"f\" | test_must_fail git check-attr --stdin &&\n \techo \"f\" | test_must_fail git check-attr --stdin -- f &&\n \techo \"f\" | test_must_fail git check-attr --stdin test -- f &&\n@@ -287,6 +319,15 @@ test_expect_success 'using --git-dir and --work-tree' '\n \t)\n '\n \n+test_expect_success 'using --revision' '\n+\tattr_check_revision foo/bar/f f branch1 &&\n+\tattr_check_revision foo/bar/a/i n branch1 &&\n+\tattr_check_revision foo/bar/f unspecified branch2 &&\n+\tattr_check_revision foo/bar/a/i m branch2 &&\n+\tattr_check_revision foo/bar/g g branch2 &&\n+\tattr_check_revision foo/bar/g unspecified branch1\n+'\n+\n test_expect_success 'setup bare' '\n \tgit clone --template= --bare . bare.git\n '\n@@ -306,6 +347,27 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' '\n \t)\n '\n \n+test_expect_success 'bare repository: with --revision' '\n+\t(\n+\t\tcd bare.git &&\n+\t\t(\n+\t\t\techo \"f\ttest=f\" &&\n+\t\t\techo \"a/i test=a/i\"\n+\t\t) | git hash-object -w --stdin >id &&\n+\t\tgit update-index --add --cacheinfo 100644 $(cat id) .gitattributes &&\n+\t\tgit write-tree >id &&\n+\t\ttree_id=$(cat id) &&\n+\t\tgit commit-tree $tree_id -m \"random commit message\" >id &&\n+\t\tcommit_id=$(cat id) &&\n+\t\tgit update-ref refs/heads/master $commit_id &&\n+\t\tattr_check_revision f f HEAD &&\n+\t\tattr_check_revision a/f f HEAD &&\n+\t\tattr_check_revision a/c/f f HEAD &&\n+\t\tattr_check_revision a/i a/i HEAD &&\n+\t\tattr_check_revision subdir/a/i unspecified HEAD\n+\t)\n+'\n+\n test_expect_success 'bare repository: check that --cached honors index' '\n \t(\n \t\tcd bare.git &&\ndiff --git a/userdiff.c b/userdiff.c\nindex 151d9a5278..b66f090a0b 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -412,7 +412,7 @@ struct userdiff_driver *userdiff_find_by_path(struct index_state *istate,\n \t\tcheck = attr_check_initl(\"diff\", NULL);\n \tif (!path)\n \t\treturn NULL;\n-\tgit_check_attr(istate, path, check);\n+\tgit_check_attr(istate, NULL, path, check);\n \n \tif (ATTR_TRUE(check->items[0].value))\n \t\treturn &driver_true;\ndiff --git a/ws.c b/ws.c\nindex 6e69877f25..eadbbe5667 100644\n--- a/ws.c\n+++ b/ws.c\n@@ -78,7 +78,7 @@ unsigned whitespace_rule(struct index_state *istate, const char *pathname)\n \tif (!attr_whitespace_rule)\n \t\tattr_whitespace_rule = attr_check_initl(\"whitespace\", NULL);\n \n-\tgit_check_attr(istate, pathname, attr_whitespace_rule);\n+\tgit_check_attr(istate, NULL, pathname, attr_whitespace_rule);\n \tvalue = attr_whitespace_rule->items[0].value;\n \tif (ATTR_TRUE(value)) {\n \t\t/* true (whitespace) */\n-- \n2.39.0\n\n"},{"id":"469183","messageId":"221216.86k02r1fcl.gmgdl@evledraar.gmail.com","threadId":"58966","inReplyTo":"20221216093552.3171319-1-karthik.188@gmail.com","subject":"Re: [PATCH v3 0/2] check-attr: add support to work with revisions","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-16T16:17:48Z","receivedAt":"2022-12-16T16:30:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Dec 16 2022, Karthik Nayak wrote:\n\n> v1: https://lore.kernel.org/git/20221206103736.53909-1-karthik.188@gmail.com/\n> v2: https://lore.kernel.org/git/CAOLa=ZSsFGBw3ta1jWN8cmUch2ca=zTEjp1xMA6Linafx9W53g@mail.gmail.com/T/#t\n\nCould you please set the In-Reply-To header appropriately in the future,\nso that each version of this series isn't in its own disconnected\nthread?\n\n> This series aims to add a new flag `-r|--revisions` to git-check-attr(1) which\n> allows us to read gitattributes from the specified revision.\n\nI didn't look at the v2, but expected at least the short form to be gone\nhere re\nhttps://lore.kernel.org/git/CAOLa=ZTSzUh2Ma_EMHHWcDunGyKMaUW9BaG=QdegtMqLd+69Wg@mail.gmail.com/;\n\nI'm still more partial to the alternate suggestion I had in\nhttps://lore.kernel.org/git/221207.86lenja0zi.gmgdl@evledraar.gmail.com/;\nI'm not sure what you meant in your reply at\nhttps://lore.kernel.org/git/CAOLa=ZQua8TfApCdzoK06_2fkWb4ZCfWewXKOSaXno1fqFSq2A@mail.gmail.com/\n(sorry about not following up at the time) with:\n\n\t\"when being consistent we need to be fully consistent,\n\ti.e. <revision>:<path>, tweaking this slightly to be\n\t<revision>:<attr> is worse than breaking consistency.\"\n\nYes, it would, but isn't that by definition the case with any\nproposal?\n\nWe don't have a way to refer to an attribute (or all attributes for -a)\nfor a given revision/path, the task of this series is to invent such a\nsyntax.\n\nSo we could invent that as this series currently does with:\n\n\tgit check-attrs --revision <rev> <attr>... <path>...\n\nOr, as I suggested:\n\n        git check-attr [<rev>:]<attr>... -- <path>...\n\nOr whatever. Here I'm not saying that one is better than the other, but\nadvocating for one on the basis of consistency doesn't make sense to me,\nthis is new syntax.\n\nI think what you mean is that because the log family uses \"<rev>:<path>\"\nwe should not come up with a syntax that looks anything like\n\"<lhs>:<rhs>\"., as the \"<lhs>\" in the mind of some users is going to be\n\"<rev>\", and the \"<rhs>\" is \"<path>\", so it would be confusing to have\nit be \"<attr>\" here, and have the \"<path>...\" come after the \"--\".\n\nI'm not convinced by that. From refspecs to e.g. \"git log\"'s own \"-L\" we\nhave little mini-syntaxes in various places that use this sort of colon\nnotation. I find it more elegant than \"--revision\".\n\nIt's fine if you disagree, I'm just trying to understand the basis of\nthe disagreement.\n\n"},{"id":"469189","messageId":"xmqq7cyrm0tr.fsf@gitster.g","threadId":"58966","inReplyTo":"221216.86k02r1fcl.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 0/2] check-attr: add support to work with revisions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-16T22:38:08Z","receivedAt":"2022-12-16T22:38:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> So we could invent that as this series currently does with:\n>\n> \tgit check-attrs --revision <rev> <attr>... <path>...\n>\n> Or, as I suggested:\n>\n>         git check-attr [<rev>:]<attr>... -- <path>...\n\nWhat does <rev>:<attr> really mean?  As the syntax for the proposed\nfeature, I do not think it makes much sense.  For example:\n\n  $ git check-attr HEAD:text HEAD^:text -- README.txt\n\n - With which README.txt are we checking the attribute?  The one\n   taken from HEAD or HEAD^ or the index or the working tree?\n\n - When we say \"README.txt has the text attribute\", how does the\n   user tell which \"text\" applies to the path?  From HEAD?  From\n   HEAD^?\n\n - Does the same attribute 'text' have different meaning when coming\n   from two different tree-ish?\n\nCompared to that at least the proposed one makes it fairly clear\nthat we are talking about things in a single tree-ish consistently.\n"},{"id":"469194","messageId":"xmqq8rj7kk0r.fsf@gitster.g","threadId":"58966","inReplyTo":"20221216093552.3171319-1-karthik.188@gmail.com","subject":"Re: [PATCH v3 0/2] check-attr: add support to work with revisions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-16T23:26:28Z","receivedAt":"2022-12-16T23:26:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> Changes since version 2:\n> - Changes to the commit message [1/2] to use more specific terms and to\n>   be more descriptive.\n> - Moved the flag's position in the documentation to be before the unbound\n>   list of non-options.\n>\n> Range-diff against v2:\n>\n> 1:  2e71cbbddd < -:  ---------- Git 2.39-rc2\n> -:  ---------- > 1:  57e2c6ebbe Start the 2.40 cycle\n\nDoes this new iteration use something that was added between these\ntwo bases?  Asking because the choice of new base is questionable.\nI would understand it if the rebase were on top of v2.39.0 tag,\nthough.\n\n * If the updated series depends on new APIs and features added\n   since the old base, do rebase on the new one to take advantage of\n   them.\n\n * A bugfix patch series may want to avoid using the newest and\n   greatest if it allows the series to be applied to the older\n   maintenance track, and keeping the older base may make more\n   sense.\n\n * If a series based on an older base no longer merges cleanly to\n   'master' and/or 'next', but rebasing on a newer base makes it\n   merge cleanly, do rebase.\n\n * Otherwise, keeping the same base is preferred.\n\nWhen rebasing is appropriate, choosing a well-known base (e.g. a\ntagged release) helps others.\n"},{"id":"469195","messageId":"xmqq4jtvkjy2.fsf@gitster.g","threadId":"58966","inReplyTo":"221216.86k02r1fcl.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 0/2] check-attr: add support to work with revisions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-16T23:28:05Z","receivedAt":"2022-12-16T23:28:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>> This series aims to add a new flag `-r|--revisions` to git-check-attr(1) which\n>> allows us to read gitattributes from the specified revision.\n>\n> I didn't look at the v2, but expected at least the short form to be gone\n> here re\n> https://lore.kernel.org/git/CAOLa=ZTSzUh2Ma_EMHHWcDunGyKMaUW9BaG=QdegtMqLd+69Wg@mail.gmail.com/;\n\nIt was unexpected to me, too.  Thanks for pointing it out.\n"},{"id":"469196","messageId":"xmqqr0wykj59.fsf@gitster.g","threadId":"58966","inReplyTo":"20221216093552.3171319-3-karthik.188@gmail.com","subject":"Re: [PATCH v3 2/2] attr: add flag `-r|--revisions` to work with revisions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-16T23:45:22Z","receivedAt":"2022-12-16T23:45:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> ... to optionally allow the users\n> to check attributes against paths from older commits.\n\nThe above makes it sounds as if attributes are taken from places\nthat are unrelated to the \"older commits\" and the point of the\nchange allows \"paths in an older commit\" to be inspected.  I do not\nthink that is what is going on, though.  Isn't the point of the patch\nto take attributes definitions from arbitrary tree-ish?\n\nAlso, \"older commits\" is misleading.  You may be chasing a bug in\nolder codebase and you have a checkout of an old commit, but you\nknow you have corrected the attributes definition since that old\nversion.  In such a case, you may want to take the attributes from\nthe latest release and apply to the currently checked out working\ntree or commit that is older.  That is checking attributes taken\nfrom newer commit.\n\n\t... to check attributes taken from a commit other than HEAD\n\tagainst paths.\n\nor something, perhaps?\n\n> Add a new flag `--revision`/`-r` which will allow users to check the\n> attributes against a tree-ish revision.\n\n\"tree-ish revision\" sounds a bit strange.  We used to use \"revision\"\nvery loosely to mean an \"object name\", but we weaned ourselves off\nof such a loose terminology over time.  These days, the word\n\"revision\" only refers to a commit (or commit-ish).\n\nI would understand \"... against a tree-ish.\"  If you feared that\n\"tree-ish\" is not widely understood (which is a valid concern), then\n\"... against a commit (actually any tree-ish would do)\" is probably\nwhat I would write.\n\n> When the user uses this flag, we\n> go through the stack of .gitattributes files but instead of checking the\n> current working tree and/or in the index, we check the blobs from the\n> provided tree-ish object. This allows the command to also be used in\n> bare repositories.\n\nGood.\n\n> Since we use a tree-ish object, the user can pass \"-r HEAD:subdirectory\"\n> and all the attributes will be looked up as if subdirectory was the root\n> directory of the repository.\n\nIs this meant to explain a feature, or a misfeature?  In other\nwords, when would this be useful?  I would omit this paragraph if I\nwere writing it.\n\n> We cannot use the `<rev>:<path>` syntax like the one used in `git show`\n> because any non-flag parameter before `--` is treated as an attribute\n> and any parameter after `--` is treated as a pathname.\n\nI do not see what this one wants to say.  <rev>:<path> is an\nestablished way to name an object that is sitting at the <path> in\nthe tree-ish whose name is <rev>.  The user can use \"-r\n<rev>:<path>\" and if the <path> in <rev> is a tree, then all the\nattributes will be looked up as if <path> were the root.  So the\nusers can use the <rev>:<path> syntax, cannot they?\n\n> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n> Co-authored-by: toon@iotcl.com\n\nCo-authoring is fine, but as one of the copyright holder, the other\nauthor needs to sign it off, too.\n"},{"id":"469198","messageId":"xmqqcz8ikgxs.fsf@gitster.g","threadId":"58966","inReplyTo":"20221216093552.3171319-3-karthik.188@gmail.com","subject":"Re: [PATCH v3 2/2] attr: add flag `-r|--revisions` to work with revisions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-17T00:33:03Z","receivedAt":"2022-12-17T00:33:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"> diff --git a/attr.c b/attr.c\n> index 42ad6de8c7..99883024ec 100644\n> --- a/attr.c\n> +++ b/attr.c\n> @@ -11,8 +11,13 @@\n>  #include \"exec-cmd.h\"\n>  #include \"attr.h\"\n>  #include \"dir.h\"\n> +#include \"git-compat-util.h\"\n\nUnneeded.  cf. Documentation/CodingGuidelines\n\n - The first #include in C files, except in platform specific compat/\n   implementations, must be either \"git-compat-util.h\", \"cache.h\" or\n   \"builtin.h\".  You do not have to include more than one of these.\n\nand this file already begins with including \"cache.h\".\n\nBy the way,\n\n    $ make\n    $ cd t\n    $ sh t0003-attributes.sh -i -x\n    Initialized empty Git repository in /home/gitster/w/git.git/t/trash directory.t0003-attributes/.git/\n    expecting success of 0003.1 'open-quoted pathname': \n            echo \"\\\"a test=a\" >.gitattributes &&\n            attr_check a unspecified\n\n    ++ echo '\"a test=a'\n    ++ attr_check a unspecified\n    ++ attr_check_basic a unspecified\n    ++ path=a\n    ++ expect=unspecified\n    ++ git_opts=\n    ++ git check-attr test -- a\n    t0003-attributes.sh: line 9: 1508946 Segmentation fault      git $git_opts check-attr test -- \"$path\" > actual 2> err\n    error: last command exited with $?=139\n    not ok 1 - open-quoted pathname\n    #\t\n    #\t\techo \"\\\"a test=a\" >.gitattributes &&\n    #\t\tattr_check a unspecified\n    #\t\n    1..1\n    $ exit\n\nthere seems to be something fishy in this patch.\n\n"},{"id":"469207","messageId":"071e44b5-607e-0db5-af90-85daae81f611@dunelm.org.uk","threadId":"58966","inReplyTo":"20221216093552.3171319-1-karthik.188@gmail.com","subject":"Re: [PATCH v3 0/2] check-attr: add support to work with revisions","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-12-17T10:53:43Z","receivedAt":"2022-12-17T10:53:52Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Karthik\n\nOn 16/12/2022 09:35, Karthik Nayak wrote:\n> v1: https://lore.kernel.org/git/20221206103736.53909-1-karthik.188@gmail.com/\n> v2: https://lore.kernel.org/git/CAOLa=ZSsFGBw3ta1jWN8cmUch2ca=zTEjp1xMA6Linafx9W53g@mail.gmail.com/T/#t\n> \n> Given a pathname, git-check-attr(1) will list the attributes which apply to that\n> pathname by reading all relevant gitattributes files. Currently there is no way\n> to specify a revision to read the gitattributes from.\n> \n> This is specifically useful in bare repositories wherein the gitattributes are\n> only present in the git working tree but not available directly on the\n> filesystem.\n\nI was thinking about this and wondering if the problem is really that \nbare repositories ignore attributes because they don't have a working \ncopy. If that's the case then we should perhaps be looking to fix that \nso that all git commands such as diff as log benefit rather than just \nadding a flag to check-attr. A simple solution would be to read the \nattributes from HEAD in a bare repository in the same way that we \nfallback to the index if there are no attributes in the working copy for \nnon-bare repositories.\n\nBest Wishes\n\nPhillip\n\n> This series aims to add a new flag `-r|--revisions` to git-check-attr(1) which\n> allows us to read gitattributes from the specified revision.\n> \n> Changes since version 2:\n> - Changes to the commit message [1/2] to use more specific terms and to\n>    be more descriptive.\n> - Moved the flag's position in the documentation to be before the unbound\n>    list of non-options.\n> \n> Range-diff against v2:\n> \n> 1:  2e71cbbddd < -:  ---------- Git 2.39-rc2\n> -:  ---------- > 1:  57e2c6ebbe Start the 2.40 cycle\n> 2:  898041f243 = 2:  c386de2d42 t0003: move setup for `--all` into new block\n> 3:  12a72e09e0 ! 3:  b93a68b0c9 attr: add flag `-r|--revisions` to work with revisions\n>      @@ Metadata\n>        ## Commit message ##\n>           attr: add flag `-r|--revisions` to work with revisions\n>       \n>      -    Git check-attr currently doesn't check the git worktree, it either\n>      -    checks the index or the files directly. This means we cannot check the\n>      -    attributes for a file against a certain revision.\n>      +    The contents of the .gitattributes files may evolve over time, but \"git\n>      +    check-attr\" always checks attributes against them in the working tree\n>      +    and/or in the index. It may be beneficial to optionally allow the users\n>      +    to check attributes against paths from older commits.\n>       \n>      -    Add a new flag `--revision`/`-r` which will allow it work with\n>      -    revisions. This command will now, instead of checking the files/index,\n>      -    try and receive the blob for the given attribute file against the\n>      -    provided revision. The flag overrides checking against the index and\n>      -    filesystem and also works with bare repositories.\n>      +    Add a new flag `--revision`/`-r` which will allow users to check the\n>      +    attributes against a tree-ish revision. When the user uses this flag, we\n>      +    go through the stack of .gitattributes files but instead of checking the\n>      +    current working tree and/or in the index, we check the blobs from the\n>      +    provided tree-ish object. This allows the command to also be used in\n>      +    bare repositories.\n>      +\n>      +    Since we use a tree-ish object, the user can pass \"-r HEAD:subdirectory\"\n>      +    and all the attributes will be looked up as if subdirectory was the root\n>      +    directory of the repository.\n>       \n>           We cannot use the `<rev>:<path>` syntax like the one used in `git show`\n>           because any non-flag parameter before `--` is treated as an attribute\n>           and any parameter after `--` is treated as a pathname.\n>       \n>      -    This involves creating a new function `read_attr_from_blob`, which given\n>      -    the path reads the blob for the path against the provided revision and\n>      +    The change involves creating a new function `read_attr_from_blob`, which\n>      +    given the path reads the blob for the path against the provided revision and\n>           parses the attributes line by line. This function is plugged into\n>      -    `read_attr()` function wherein we go through the different attributes.\n>      +    `read_attr()` function wherein we go through the stack of attributes\n>      +    files.\n>       \n>           Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n>           Co-authored-by: toon@iotcl.com\n>      @@ Documentation/git-check-attr.txt: git-check-attr - Display gitattributes informa\n>        [verse]\n>       -'git check-attr' [-a | --all | <attr>...] [--] <pathname>...\n>       -'git check-attr' --stdin [-z] [-a | --all | <attr>...]\n>      -+'git check-attr' [-a | --all | <attr>...] [-r <revision>] [--] <pathname>...\n>      -+'git check-attr' --stdin [-z] [-a | --all | <attr>...] [-r <revision>]\n>      ++'git check-attr' [-r <revision>] [-a | --all | <attr>...] [--] <pathname>...\n>      ++'git check-attr' --stdin [-z] [-r <revision>] [-a | --all | <attr>...]\n>        \n>        DESCRIPTION\n>        -----------\n>      @@ Documentation/git-check-attr.txt: OPTIONS\n>        \n>       +--r <revision>::\n>       +--revision=<revision>::\n>      -+\tCheck attributes against the specified revision.\n>      ++\tCheck attributes against the specified tree-ish revision. All the\n>      ++\tattributes will be checked against the provided revision. Paths provided\n>      ++\tas part of the revision will be treated as the root directory.\n>       +\n>        \\--::\n>        \tInterpret all preceding arguments as attributes and all following\n>      @@ builtin/check-attr.c\n>        static const char * const check_attr_usage[] = {\n>       -N_(\"git check-attr [-a | --all | <attr>...] [--] <pathname>...\"),\n>       -N_(\"git check-attr --stdin [-z] [-a | --all | <attr>...]\"),\n>      -+N_(\"git check-attr [-a | --all | <attr>...] [-r <revision>] [--] <pathname>...\"),\n>      -+N_(\"git check-attr --stdin [-z] [-a | --all | <attr>...] [-r <revision>]\"),\n>      ++N_(\"git check-attr [-r <revision>] [-a | --all | <attr>...] [--] <pathname>...\"),\n>      ++N_(\"git check-attr --stdin [-z] [-r <revision>] [-a | --all | <attr>...]\"),\n>        NULL\n>        };\n>        \n> \n> \n> Karthik Nayak (2):\n>    t0003: move setup for `--all` into new block\n>    attr: add flag `-r|--revisions` to work with revisions\n> \n>   Documentation/git-check-attr.txt |  10 +++-\n>   archive.c                        |   2 +-\n>   attr.c                           | 100 ++++++++++++++++++++++---------\n>   attr.h                           |   7 ++-\n>   builtin/check-attr.c             |  33 ++++++----\n>   builtin/pack-objects.c           |   2 +-\n>   convert.c                        |   2 +-\n>   ll-merge.c                       |   4 +-\n>   pathspec.c                       |   2 +-\n>   t/t0003-attributes.sh            |  71 +++++++++++++++++++++-\n>   userdiff.c                       |   2 +-\n>   ws.c                             |   2 +-\n>   12 files changed, 182 insertions(+), 55 deletions(-)\n> \n"},{"id":"469224","messageId":"CAOLa=ZR2XUX1EPgG0g_UgZo3wjuZLWthJerztqUL4+qCJKb=KA@mail.gmail.com","threadId":"58966","inReplyTo":"221216.86k02r1fcl.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 0/2] check-attr: add support to work with revisions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2022-12-17T14:46:37Z","receivedAt":"2022-12-17T14:47:09Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Fri, Dec 16, 2022 at 5:30 PM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n>\n> On Fri, Dec 16 2022, Karthik Nayak wrote:\n>\n> > v1: https://lore.kernel.org/git/20221206103736.53909-1-karthik.188@gmail.com/\n> > v2: https://lore.kernel.org/git/CAOLa=ZSsFGBw3ta1jWN8cmUch2ca=zTEjp1xMA6Linafx9W53g@mail.gmail.com/T/#t\n>\n> Could you please set the In-Reply-To header appropriately in the future,\n> so that each version of this series isn't in its own disconnected\n> thread?\n>\n\nI didn't know, will do this from next time!\n\n> > This series aims to add a new flag `-r|--revisions` to git-check-attr(1) which\n> > allows us to read gitattributes from the specified revision.\n>\n> I didn't look at the v2, but expected at least the short form to be gone\n> here re\n> https://lore.kernel.org/git/CAOLa=ZTSzUh2Ma_EMHHWcDunGyKMaUW9BaG=QdegtMqLd+69Wg@mail.gmail.com/;\n>\n\nRight, I was open to it, but since there wasn't any confirmation, I\ndidn't go forward with it.\nWill remove it from the next version.\n\n> I'm still more partial to the alternate suggestion I had in\n> https://lore.kernel.org/git/221207.86lenja0zi.gmgdl@evledraar.gmail.com/;\n> I'm not sure what you meant in your reply at\n> https://lore.kernel.org/git/CAOLa=ZQua8TfApCdzoK06_2fkWb4ZCfWewXKOSaXno1fqFSq2A@mail.gmail.com/\n> (sorry about not following up at the time) with:\n>\n>         \"when being consistent we need to be fully consistent,\n>         i.e. <revision>:<path>, tweaking this slightly to be\n>         <revision>:<attr> is worse than breaking consistency.\"\n>\n> Yes, it would, but isn't that by definition the case with any\n> proposal?\n>\n\nI'm not opposing the proposal, rather stating my opinion on it. To go\nover my reply\n\nI'm only saying that most users of Git are accustomed to the `<revision>:<path>`\nsyntax and now breaking that only in one command to be `<revision>:<attr>` seems\na bit odd, from the user experience point of view.\n\n> We don't have a way to refer to an attribute (or all attributes for -a)\n> for a given revision/path, the task of this series is to invent such a\n> syntax.\n>\n> So we could invent that as this series currently does with:\n>\n>         git check-attrs --revision <rev> <attr>... <path>...\n>\n> Or, as I suggested:\n>\n>         git check-attr [<rev>:]<attr>... -- <path>...\n>\n> Or whatever. Here I'm not saying that one is better than the other, but\n> advocating for one on the basis of consistency doesn't make sense to me,\n> this is new syntax.\n>\n\nI see what you mean, but I was referring to consistency around how different\noptions are used in other git commands.\n\nMainly that most commands treat the second section after `<rev>:` to\nbe a path, now\nadding a new option where the section after `<rev>:` to be an\nattribute, might be a\nbit confusing.\n\n> I think what you mean is that because the log family uses \"<rev>:<path>\"\n> we should not come up with a syntax that looks anything like\n> \"<lhs>:<rhs>\"., as the \"<lhs>\" in the mind of some users is going to be\n> \"<rev>\", and the \"<rhs>\" is \"<path>\", so it would be confusing to have\n> it be \"<attr>\" here, and have the \"<path>...\" come after the \"--\".\n>\n\nExactly.\n\n> I'm not convinced by that. From refspecs to e.g. \"git log\"'s own \"-L\" we\n> have little mini-syntaxes in various places that use this sort of colon\n> notation. I find it more elegant than \"--revision\".\n>\n> It's fine if you disagree, I'm just trying to understand the basis of\n> the disagreement.\n>\n\nI don't disagree. I think it's healthy to have this discussion,\nespecially since we're\nadding a new option and this is the right time. I'm all ears and finally want to\nget the best solution.\n\n-- \n- Karthik\n"},{"id":"469225","messageId":"CAOLa=ZQ=9MP3BZLoW9QJXheKZz1tDeLHbAm0dkBbEuEV8tpqBw@mail.gmail.com","threadId":"58966","inReplyTo":"xmqq8rj7kk0r.fsf@gitster.g","subject":"Re: [PATCH v3 0/2] check-attr: add support to work with revisions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2022-12-17T14:49:51Z","receivedAt":"2022-12-17T14:50:28Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sat, Dec 17, 2022 at 12:26 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Karthik Nayak <karthik.188@gmail.com> writes:\n>\n> > Changes since version 2:\n> > - Changes to the commit message [1/2] to use more specific terms and to\n> >   be more descriptive.\n> > - Moved the flag's position in the documentation to be before the unbound\n> >   list of non-options.\n> >\n> > Range-diff against v2:\n> >\n> > 1:  2e71cbbddd < -:  ---------- Git 2.39-rc2\n> > -:  ---------- > 1:  57e2c6ebbe Start the 2.40 cycle\n>\n> Does this new iteration use something that was added between these\n> two bases?  Asking because the choice of new base is questionable.\n> I would understand it if the rebase were on top of v2.39.0 tag,\n> though.\n>\n>  * If the updated series depends on new APIs and features added\n>    since the old base, do rebase on the new one to take advantage of\n>    them.\n>\n>  * A bugfix patch series may want to avoid using the newest and\n>    greatest if it allows the series to be applied to the older\n>    maintenance track, and keeping the older base may make more\n>    sense.\n>\n>  * If a series based on an older base no longer merges cleanly to\n>    'master' and/or 'next', but rebasing on a newer base makes it\n>    merge cleanly, do rebase.\n>\n>  * Otherwise, keeping the same base is preferred.\n>\n> When rebasing is appropriate, choosing a well-known base (e.g. a\n> tagged release) helps others.\n\nRight! I think I just have a habit of rebasing on top of master on a general\nbasis. I'll keep the old base and modify my tags to make sure the next\nrange-diff\nwill use `2e71cbbddd` as the base for both ranges.\n\n-- \n- Karthik\n"},{"id":"469226","messageId":"CAOLa=ZRp+JjFbdhyv9wFLyQgKmQN8p6xwUsHe7=254_q6xqPxQ@mail.gmail.com","threadId":"58966","inReplyTo":"071e44b5-607e-0db5-af90-85daae81f611@dunelm.org.uk","subject":"Re: [PATCH v3 0/2] check-attr: add support to work with revisions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2022-12-17T14:52:23Z","receivedAt":"2022-12-17T14:52:54Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sat, Dec 17, 2022 at 11:53 AM Phillip Wood <phillip.wood123@gmail.com> wrote:\n> > This is specifically useful in bare repositories wherein the gitattributes are\n> > only present in the git working tree but not available directly on the\n> > filesystem.\n>\n> I was thinking about this and wondering if the problem is really that\n> bare repositories ignore attributes because they don't have a working\n> copy. If that's the case then we should perhaps be looking to fix that\n> so that all git commands such as diff as log benefit rather than just\n> adding a flag to check-attr. A simple solution would be to read the\n> attributes from HEAD in a bare repository in the same way that we\n> fallback to the index if there are no attributes in the working copy for\n> non-bare repositories.\n>\n\nThis is actually the direction I started this series in, but I soon\nrealized it's also useful\nto have a more generic version (which is currently what we have in\nthis patch series)\nwhich also satisfies the bare repository scenario. It seemed like a\nnatural extension.\n\nI thought it's also useful because it lets you see how attributes\nchanges over history.\n"},{"id":"469227","messageId":"CAOLa=ZSo7zMVB6c-9gjAS-1zKkdVbRmUV02q4hT_LbU8dAt0tw@mail.gmail.com","threadId":"58966","inReplyTo":"xmqqr0wykj59.fsf@gitster.g","subject":"Re: [PATCH v3 2/2] attr: add flag `-r|--revisions` to work with revisions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2022-12-17T15:23:31Z","receivedAt":"2022-12-17T15:24:03Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sat, Dec 17, 2022 at 12:45 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Karthik Nayak <karthik.188@gmail.com> writes:\n>\n> > ... to optionally allow the users\n> > to check attributes against paths from older commits.\n>\n> The above makes it sounds as if attributes are taken from places\n> that are unrelated to the \"older commits\" and the point of the\n> change allows \"paths in an older commit\" to be inspected.  I do not\n> think that is what is going on, though.  Isn't the point of the patch\n> to take attributes definitions from arbitrary tree-ish?\n>\n> Also, \"older commits\" is misleading.  You may be chasing a bug in\n> older codebase and you have a checkout of an old commit, but you\n> know you have corrected the attributes definition since that old\n> version.  In such a case, you may want to take the attributes from\n> the latest release and apply to the currently checked out working\n> tree or commit that is older.  That is checking attributes taken\n> from newer commit.\n>\n>         ... to check attributes taken from a commit other than HEAD\n>         against paths.\n>\n> or something, perhaps?\n>\n\nI agree with your wording, it's much better. I'll stick to it.\n\n> > Add a new flag `--revision`/`-r` which will allow users to check the\n> > attributes against a tree-ish revision.\n>\n> \"tree-ish revision\" sounds a bit strange.  We used to use \"revision\"\n> very loosely to mean an \"object name\", but we weaned ourselves off\n> of such a loose terminology over time.  These days, the word\n> \"revision\" only refers to a commit (or commit-ish).\n>\n> I would understand \"... against a tree-ish.\"  If you feared that\n> \"tree-ish\" is not widely understood (which is a valid concern), then\n> \"... against a commit (actually any tree-ish would do)\" is probably\n> what I would write.\n>\n\nThanks for explaining, I somehow keep associating revision to be the universal\nset, which consists of tree, commit. I'll use your wording though.\n\n>\n> > Since we use a tree-ish object, the user can pass \"-r HEAD:subdirectory\"\n> > and all the attributes will be looked up as if subdirectory was the root\n> > directory of the repository.\n>\n> Is this meant to explain a feature, or a misfeature?  In other\n> words, when would this be useful?  I would omit this paragraph if I\n> were writing it.\n\nIt's a misfeature to be honest, I think it was called out in the\nprevious version\nof the series. I'm happy to drop it, because I initially didn't include it.\n\nhttps://lore.kernel.org/git/674caf56-940b-8130-4a5e-ea8dc4783e81@dunelm.org.uk/\n\n>\n> > We cannot use the `<rev>:<path>` syntax like the one used in `git show`\n> > because any non-flag parameter before `--` is treated as an attribute\n> > and any parameter after `--` is treated as a pathname.\n>\n> I do not see what this one wants to say.  <rev>:<path> is an\n> established way to name an object that is sitting at the <path> in\n> the tree-ish whose name is <rev>.  The user can use \"-r\n> <rev>:<path>\" and if the <path> in <rev> is a tree, then all the\n> attributes will be looked up as if <path> were the root.  So the\n> users can use the <rev>:<path> syntax, cannot they?\n>\n\nYes ofcourse, this one merely is stating why we cannot use `<rev>:<path>`\ndirectly (i.e. without the --revision flag).\n\nI'll correct it to:\n\n    We cannot simply use the `<rev>:<path>` syntax without the `--revision`\n    flag, similar to how it is used in `git show` because any non-flag\n    parameter before `--` is treated as an attribute and any parameter after\n    `--` is treated as a pathname.\n\n> > Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n> > Co-authored-by: toon@iotcl.com\n>\n> Co-authoring is fine, but as one of the copyright holder, the other\n> author needs to sign it off, too.\n\nCan I simply add this, or does Toon need to provide his approval on this list?\n\n-- \n- Karthik\n"},{"id":"469228","messageId":"CAOLa=ZTXQ6e_JY+3jN6r52j71qTo5-OJBs-_vLaE_obn+3Fb1Q@mail.gmail.com","threadId":"58966","inReplyTo":"xmqqcz8ikgxs.fsf@gitster.g","subject":"Re: [PATCH v3 2/2] attr: add flag `-r|--revisions` to work with revisions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2022-12-17T15:27:09Z","receivedAt":"2022-12-17T15:27:40Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sat, Dec 17, 2022 at 1:33 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> > diff --git a/attr.c b/attr.c\n> > index 42ad6de8c7..99883024ec 100644\n> > --- a/attr.c\n> > +++ b/attr.c\n> > @@ -11,8 +11,13 @@\n> >  #include \"exec-cmd.h\"\n> >  #include \"attr.h\"\n> >  #include \"dir.h\"\n> > +#include \"git-compat-util.h\"\n>\n> Unneeded.  cf. Documentation/CodingGuidelines\n>\n>  - The first #include in C files, except in platform specific compat/\n>    implementations, must be either \"git-compat-util.h\", \"cache.h\" or\n>    \"builtin.h\".  You do not have to include more than one of these.\n>\n> and this file already begins with including \"cache.h\".\n>\n\nThanks, I thought this was removed.\n\n> By the way,\n>\n>     $ make\n>     $ cd t\n>     $ sh t0003-attributes.sh -i -x\n>     Initialized empty Git repository in /home/gitster/w/git.git/t/trash directory.t0003-attributes/.git/\n>     expecting success of 0003.1 'open-quoted pathname':\n>             echo \"\\\"a test=a\" >.gitattributes &&\n>             attr_check a unspecified\n>\n>     ++ echo '\"a test=a'\n>     ++ attr_check a unspecified\n>     ++ attr_check_basic a unspecified\n>     ++ path=a\n>     ++ expect=unspecified\n>     ++ git_opts=\n>     ++ git check-attr test -- a\n>     t0003-attributes.sh: line 9: 1508946 Segmentation fault      git $git_opts check-attr test -- \"$path\" > actual 2> err\n>     error: last command exited with $?=139\n>     not ok 1 - open-quoted pathname\n>     #\n>     #           echo \"\\\"a test=a\" >.gitattributes &&\n>     #           attr_check a unspecified\n>     #\n>     1..1\n>     $ exit\n>\n> there seems to be something fishy in this patch.\n>\n\nSeems to be because tree_oid is not NULL initialized. I think I only\ntested the new\nfeature, but the other tests are failing. Should be fixed with\n\n    -+  struct object_id tree_oid;\n    ++  struct object_id *tree_oid = NULL;\n        int cnt, i, doubledash, filei;\n\n        if (!is_bare_repository())\n    @@ builtin/check-attr.c: int cmd_check_attr(int argc, const char\n**argv, const char\n                }\n        }\n\n    -+  if (revision)\n    -+          if (repo_get_oid_tree(the_repository, revision, &tree_oid))\n    ++  if (revision) {\n    ++          tree_oid = xmalloc(sizeof(struct object_id));\n    ++\n    ++          if (repo_get_oid_tree(the_repository, revision, tree_oid))\n     +                  error(\"%s: not a valid revision\", revision);\n    ++  }\n\nwill include in version 4. Thanks for the support, Junio.\n\n-- \n- Karthik\n"},{"id":"469263","messageId":"221219.86fsdb22hb.gmgdl@evledraar.gmail.com","threadId":"58966","inReplyTo":"xmqq7cyrm0tr.fsf@gitster.g","subject":"Re: [PATCH v3 0/2] check-attr: add support to work with revisions","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-19T08:45:34Z","receivedAt":"2022-12-19T08:59:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Dec 17 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>\n>> So we could invent that as this series currently does with:\n>>\n>> \tgit check-attrs --revision <rev> <attr>... <path>...\n>>\n>> Or, as I suggested:\n>>\n>>         git check-attr [<rev>:]<attr>... -- <path>...\n>\n> What does <rev>:<attr> really mean?  As the syntax for the proposed\n> feature, I do not think it makes much sense.  For example:\n>\n>   $ git check-attr HEAD:text HEAD^:text -- README.txt\n>\n>  - With which README.txt are we checking the attribute?  The one\n>    taken from HEAD or HEAD^ or the index or the working tree?\n\nAll of them, but I do think this rightly points out that the \"rev before\npath\" part of this doesn't make sense, but shouldn't we be making this\nwork like \"git grep\" with <rev>/<path> combinations? I.e.:\n\t\n\t$ git -P grep -m 1 oid HEAD~:cache.h v2.26.0:cache.h v1.6.0:cache.h\n\tHEAD~:cache.h:#include \"oid-array.h\"\n\tv2.26.0:cache.h:void git_inflate_init(git_zstream *);\n\tv1.6.0:cache.h:static inline void copy_cache_entry(struct cache_entry *dst, struct cache_entry *src)\n\nI.e. we currently support:\n\n\tgit check-attr [-a | --all | <attr>...] [--] <pathname>...\n\tgit check-attr --stdin [-z] [-a | --all | <attr>...]\n\nSo if we add to that:\n\n\tgit check-attr --stdin [-z] <rev>:<pathname>...\n\nWe'd have this do the right thing:\n\t\n\t$ git check-attr diff -- README.md HEAD:git-send-email.perl v1.6.0:git-send-email.perl\n\tREADME.md: diff: unspecified\n\tHEAD:git-send-email.perl: diff: perl\n\tv1.6.0:git-send-email.perl: diff: perl\n\nWhich would technically break backwards compatibility, as we now\n\"support\" it (we just interpret the whole thing as a path), but I think\nsuch revision-looking paths aren't worth worrying about\n\n>  - When we say \"README.txt has the text attribute\", how does the\n>    user tell which \"text\" applies to the path?  From HEAD?  From\n>    HEAD^?\n\nRegardless of what I'm suggesting here, the \"git check-attr\" output\nalready has a one-to-one line output correspondance with its input, so\njust as it does now we'd print both.\n\nThis looks like a bug though (on master, the missing \"\\n\" is there in\nthe output):\n\n\t$ ./git check-attr diffgit-send-email.perl foo.perl git-send-email.perl\n\tfoo.perl: diffgit-send-email.perl: unspecified\n\tgit-send-email.perl: diffgit-send-email.perl: unspecified\n\n>  - Does the same attribute 'text' have different meaning when coming\n>    from two different tree-ish?\n\nYes, just like \"git grep\", we'd need to parse & apply the .gitattributes\nfor that revision. Whether we call it \"<rev>:<path>\", \"--revision <rev>\n<path>\" or whatever we'd always want to do that, otherwise what's the\npoint?\n"},{"id":"469267","messageId":"221219.86359b20bc.gmgdl@evledraar.gmail.com","threadId":"58966","inReplyTo":"20221216093552.3171319-1-karthik.188@gmail.com","subject":"Re: [PATCH v3 0/2] check-attr: add support to work with revisions","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-19T09:45:10Z","receivedAt":"2022-12-19T09:46:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Dec 16 2022, Karthik Nayak wrote:\n\n> v1: https://lore.kernel.org/git/20221206103736.53909-1-karthik.188@gmail.com/\n> v2: https://lore.kernel.org/git/CAOLa=ZSsFGBw3ta1jWN8cmUch2ca=zTEjp1xMA6Linafx9W53g@mail.gmail.com/T/#t\n>\n> Given a pathname, git-check-attr(1) will list the attributes which apply to that\n> pathname by reading all relevant gitattributes files. Currently there is no way\n> to specify a revision to read the gitattributes from.\n>\n> This is specifically useful in bare repositories wherein the gitattributes are\n> only present in the git working tree but not available directly on the\n> filesystem.\n>\n> This series aims to add a new flag `-r|--revisions` to git-check-attr(1) which\n> allows us to read gitattributes from the specified revision.\n>\n> Changes since version 2:\n> - Changes to the commit message [1/2] to use more specific terms and to\n>   be more descriptive.\n> - Moved the flag's position in the documentation to be before the unbound\n>   list of non-options.\n\nAside from the UX concerns with this series, this segfaults with it, but\nnot on \"master\":\n\t\n\t./git check-attr diff git-send-email.perl foo.perl git-send-email.perl\n\tAddressSanitizer:DEADLYSIGNAL\n\t=================================================================\n\t==1828755==ERROR: AddressSanitizer: SEGV on unknown address (pc 0x0000008ee4a8 bp 0x7fffe4cef820 sp 0x7fffe4cef800 T0)\n\t==1828755==The signal is caused by a READ memory access.\n\t==1828755==Hint: this fault was caused by a dereference of a high value address (see register values below).  Disassemble the provided pc to learn which register was used.\n\t    #0 0x8ee4a8 in hasheq_algop hash.h:236\n\t    #1 0x8ee632 in oideq hash.h:253\n\t    #2 0x8ee657 in is_null_oid hash.h:258\n\t    #3 0x8f79e2 in do_oid_object_info_extended object-file.c:1550\n\t    #4 0x8f8206 in oid_object_info_extended object-file.c:1640\n\t    #5 0x8f860c in read_object object-file.c:1672\n\t    #6 0x8f8a8a in read_object_file_extended object-file.c:1715\n\t    #7 0x8f01ef in repo_read_object_file object-store.h:253\n\t    #8 0x8f8e37 in read_object_with_reference object-file.c:1756\n\t    #9 0xafb411 in get_tree_entry tree-walk.c:612\n\t    #10 0x6d1975 in read_attr_from_blob attr.c:776\n\t    #11 0x6d1b80 in read_attr attr.c:826\n\t    #12 0x6d1f35 in bootstrap_attr_stack attr.c:912\n\t    #13 0x6d2173 in prepare_attr_stack attr.c:948\n\t    #14 0x6d3285 in collect_some_attrs attr.c:1143\n\t    #15 0x6d33e1 in git_check_attr attr.c:1157\n\t    #16 0x453581 in check_attr builtin/check-attr.c:72\n\t    #17 0x453f1f in cmd_check_attr builtin/check-attr.c:190\n\t    #18 0x40b63d in run_builtin git.c:466\n\t    #19 0x40bf7f in handle_builtin git.c:721\n\t    #20 0x40c686 in run_argv git.c:788\n\t    #21 0x40d42f in cmd_main git.c:926\n\t    #22 0x6885b5 in main common-main.c:57\n\t    #23 0x7f96725a8189 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58\n\t    #24 0x7f96725a8244 in __libc_start_main_impl ../csu/libc-start.c:381\n\t    #25 0x407230 in _start (git+0x407230)\n\t\n\tAddressSanitizer can not provide additional info.\n\tSUMMARY: AddressSanitizer: SEGV hash.h:236 in hasheq_algop\n\t==1828755==ABORTING\n\tAborted\n\nIf the tests are still passing for you (I didn't check) then we probably\nhave a bad test blind spot with kthat we should start by addressing\nbefore adding the new feature.\n"},{"id":"469284","messageId":"CAOLa=ZScE7ifK2K=8RYLwdwdXafH05j7ONU1CT1rjPWgYXRZVA@mail.gmail.com","threadId":"58966","inReplyTo":"221219.86359b20bc.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 0/2] check-attr: add support to work with revisions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2022-12-19T13:16:04Z","receivedAt":"2022-12-19T13:16:35Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Mon, Dec 19, 2022 at 10:46 AM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n>\n> On Fri, Dec 16 2022, Karthik Nayak wrote:\n>\n> > v1: https://lore.kernel.org/git/20221206103736.53909-1-karthik.188@gmail.com/\n> > v2: https://lore.kernel.org/git/CAOLa=ZSsFGBw3ta1jWN8cmUch2ca=zTEjp1xMA6Linafx9W53g@mail.gmail.com/T/#t\n> >\n> > Given a pathname, git-check-attr(1) will list the attributes which apply to that\n> > pathname by reading all relevant gitattributes files. Currently there is no way\n> > to specify a revision to read the gitattributes from.\n> >\n> > This is specifically useful in bare repositories wherein the gitattributes are\n> > only present in the git working tree but not available directly on the\n> > filesystem.\n> >\n> > This series aims to add a new flag `-r|--revisions` to git-check-attr(1) which\n> > allows us to read gitattributes from the specified revision.\n> >\n> > Changes since version 2:\n> > - Changes to the commit message [1/2] to use more specific terms and to\n> >   be more descriptive.\n> > - Moved the flag's position in the documentation to be before the unbound\n> >   list of non-options.\n>\n> Aside from the UX concerns with this series, this segfaults with it, but\n> not on \"master\":\n>\n>         ./git check-attr diff git-send-email.perl foo.perl git-send-email.perl\n>         AddressSanitizer:DEADLYSIGNAL\n>         =================================================================\n>         ==1828755==ERROR: AddressSanitizer: SEGV on unknown address (pc 0x0000008ee4a8 bp 0x7fffe4cef820 sp 0x7fffe4cef800 T0)\n>         ==1828755==The signal is caused by a READ memory access.\n>         ==1828755==Hint: this fault was caused by a dereference of a high value address (see register values below).  Disassemble the provided pc to learn which register was used.\n>             #0 0x8ee4a8 in hasheq_algop hash.h:236\n>             #1 0x8ee632 in oideq hash.h:253\n>             #2 0x8ee657 in is_null_oid hash.h:258\n>             #3 0x8f79e2 in do_oid_object_info_extended object-file.c:1550\n>             #4 0x8f8206 in oid_object_info_extended object-file.c:1640\n>             #5 0x8f860c in read_object object-file.c:1672\n>             #6 0x8f8a8a in read_object_file_extended object-file.c:1715\n>             #7 0x8f01ef in repo_read_object_file object-store.h:253\n>             #8 0x8f8e37 in read_object_with_reference object-file.c:1756\n>             #9 0xafb411 in get_tree_entry tree-walk.c:612\n>             #10 0x6d1975 in read_attr_from_blob attr.c:776\n>             #11 0x6d1b80 in read_attr attr.c:826\n>             #12 0x6d1f35 in bootstrap_attr_stack attr.c:912\n>             #13 0x6d2173 in prepare_attr_stack attr.c:948\n>             #14 0x6d3285 in collect_some_attrs attr.c:1143\n>             #15 0x6d33e1 in git_check_attr attr.c:1157\n>             #16 0x453581 in check_attr builtin/check-attr.c:72\n>             #17 0x453f1f in cmd_check_attr builtin/check-attr.c:190\n>             #18 0x40b63d in run_builtin git.c:466\n>             #19 0x40bf7f in handle_builtin git.c:721\n>             #20 0x40c686 in run_argv git.c:788\n>             #21 0x40d42f in cmd_main git.c:926\n>             #22 0x6885b5 in main common-main.c:57\n>             #23 0x7f96725a8189 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58\n>             #24 0x7f96725a8244 in __libc_start_main_impl ../csu/libc-start.c:381\n>             #25 0x407230 in _start (git+0x407230)\n>\n>         AddressSanitizer can not provide additional info.\n>         SUMMARY: AddressSanitizer: SEGV hash.h:236 in hasheq_algop\n>         ==1828755==ABORTING\n>         Aborted\n>\n> If the tests are still passing for you (I didn't check) then we probably\n> have a bad test blind spot with kthat we should start by addressing\n> before adding the new feature.\n\nThis seems to be what Junio mentioned here:\nhttps://lore.kernel.org/git/xmqqcz8ikgxs.fsf@gitster.g/\nShould be fixed in v4!\n\n-- \n- Karthik\n"},{"id":"469424","messageId":"877cylqnlv.fsf@to1.studio","threadId":"58966","inReplyTo":"xmqqr0wykj59.fsf@gitster.g","subject":"Re: [PATCH v3 2/2] attr: add flag `-r|--revisions` to work with revisions","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2022-12-21T06:10:41Z","receivedAt":"2022-12-21T06:34:37Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"\nJunio C Hamano <gitster@pobox.com> writes:\n\n>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n>> Co-authored-by: toon@iotcl.com\n>\n> Co-authoring is fine, but as one of the copyright holder, the other\n> author needs to sign it off, too.\n\nI'm happy to sign off, on both patches:\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n-Co-authored-by: toon@iotcl.com\n+Co-authored-by: Toon Claes <toon@iotcl.com>\n+Signed-off-by: Toon Claes <toon@iotcl.com>\n\n--\nToon\n"}]}