{"thread":{"id":"58907","subject":"[PATCH 0/2] check-attr: add support to work with revisions","startedAt":"2022-12-06T10:38:05Z","lastAt":"2022-12-07T12:34:18Z","messageCount":17,"participants":["Karthik Nayak","Philip Oakley","Ævar Arnfjörð Bjarmason","Junio C Hamano","Taylor Blau","Eric Sunshine","brian m. carlson"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"468612","messageId":"20221206103736.53909-1-karthik.188@gmail.com","threadId":"58907","inReplyTo":null,"subject":"[PATCH 0/2] check-attr: add support to work with revisions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2022-12-06T10:37:34Z","receivedAt":"2022-12-06T10:38:05Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Given 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\nKarthik Nayak (2):\n t0003: move setup for `--all` into new block\n attr: add flag `-r|--revisions` to work with revisions\n\n archive.c | 2 +-\n attr.c | 120 ++++++++++++++++++++++++++++-------------\n attr.h | 7 ++-\n builtin/check-attr.c | 25 ++++-----\n builtin/pack-objects.c | 2 +-\n convert.c | 2 +-\n ll-merge.c | 4 +-\n pathspec.c | 2 +-\n t/t0003-attributes.sh | 63 ++++++++++++++++++++--\n userdiff.c | 2 +-\n ws.c | 2 +-\n 11 files changed, 170 insertions(+), 61 deletions(-)\n\n-- \n2.38.1\n\n"},{"id":"468613","messageId":"20221206103736.53909-2-karthik.188@gmail.com","threadId":"58907","inReplyTo":"20221206103736.53909-1-karthik.188@gmail.com","subject":"[PATCH 1/2] t0003: move setup for `--all` into new block","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2022-12-06T10:37:35Z","receivedAt":"2022-12-06T10:38:06Z","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,\" 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 \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 specified-all &&\n-\tsed -e \"s/:.*//\" stdin-all &&\n+\tsed -e \"s/:.*//\" stdin-all\n+'\n+\n+test_expect_success 'attribute test: --all option' '\n \tgit check-attr --stdin --all tmp &&\n \tsort tmp >actual &&\n \ttest_cmp specified-all actual\n-- \n2.38.1\n\n"},{"id":"468614","messageId":"20221206103736.53909-3-karthik.188@gmail.com","threadId":"58907","inReplyTo":"20221206103736.53909-1-karthik.188@gmail.com","subject":"[PATCH 2/2] attr: add flag `-r|--revisions` to work with revisions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2022-12-06T10:37:36Z","receivedAt":"2022-12-06T10:38:10Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Git check-attr currently doesn't check the git worktree, it either\nchecks the index or the files directly. This means we cannot check the\nattributes for a file against a certain revision.\n\nAdd a new flag `--revision`/`-r` which will allow it work with\nrevisions. This command will now, instead of checking the files/index,\ntry and receive the blob for the given attribute file against the\nprovided revision. The flag overrides checking against the index and\nfilesystem and also works with bare repositories.\n\nWe cannot use the `:` 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\nThis involves creating a new function `read_attr_from_blob`, which given\nthe 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 different attributes.\n\nSigned-off-by: Karthik Nayak \nCo-authored-by: toon@iotcl.com\n---\n archive.c | 2 +-\n attr.c | 120 ++++++++++++++++++++++++++++-------------\n attr.h | 7 ++-\n builtin/check-attr.c | 25 ++++-----\n builtin/pack-objects.c | 2 +-\n convert.c | 2 +-\n ll-merge.c | 4 +-\n pathspec.c | 2 +-\n t/t0003-attributes.sh | 56 ++++++++++++++++++-\n userdiff.c | 2 +-\n ws.c | 2 +-\n 11 files changed, 165 insertions(+), 59 deletions(-)\n\ndiff --git a/archive.c b/archive.c\nindex 941495f5d7..bf64dbdce1 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, path, check, NULL);\n \treturn check;\n }\n \ndiff --git a/attr.c b/attr.c\nindex 42ad6de8c7..42b67a401f 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -11,8 +11,12 @@\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 \"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 +733,67 @@ 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 == NULL)\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(const char *path,\n+\t\t\t\t\t const char *object_name,\n+\t\t\t\t\t unsigned flags)\n+{\n+\tstruct object_id oid;\n+\tunsigned long sz;\n+\tenum object_type type;\n+\tvoid *buf;\n+\tstruct strbuf sb;\n+\n+\tif (object_name == NULL)\n+\t\treturn NULL;\n+\n+\tstrbuf_init(&sb, strlen(path) + 1 + strlen(object_name));\n+\tstrbuf_addstr(&sb, object_name);\n+\tstrbuf_addstr(&sb, \":\");\n+\tstrbuf_addstr(&sb, path);\n+\n+\tif (get_oid(sb.buf, &oid))\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 +815,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 char *path, unsigned flags)\n+\t\t\t\t const char *path, const char *object_name,\n+\t\t\t\t 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 (object_name != NULL) {\n+\t\tres = read_attr_from_blob(path, object_name, 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,7 +887,8 @@ 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 struct attr_stack **stack)\n+\t\t\t\t struct attr_stack **stack,\n+\t\t\t\t const char *object_name)\n {\n \tstruct attr_stack *e;\n \tunsigned flags = READ_ATTR_MACRO_OK;\n@@ -864,7 +913,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, GITATTRIBUTES_FILE, object_name, flags | READ_ATTR_NOFOLLOW);\n \tpush_stack(stack, e, xstrdup(\"\"), 0);\n \n \t/* info frame */\n@@ -877,9 +926,9 @@ static void bootstrap_attr_stack(struct index_state *istate,\n \tpush_stack(stack, e, NULL, 0);\n }\n \n-static void prepare_attr_stack(struct index_state *istate,\n-\t\t\t const char *path, int dirlen,\n-\t\t\t struct attr_stack **stack)\n+static void prepare_attr_stack(struct index_state *istate, const char *path,\n+\t\t\t int dirlen, struct attr_stack **stack,\n+\t\t\t const char *object_name)\n {\n \tstruct attr_stack *info;\n \tstruct strbuf pathbuf = STRBUF_INIT;\n@@ -899,7 +948,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, stack, object_name);\n \n \t/*\n \t * Pop the \"info\" one that is always at the top of the stack.\n@@ -954,7 +1003,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, pathbuf.buf, object_name, READ_ATTR_NOFOLLOW);\n \n \t\t/* reset the pathbuf to not include \"/.gitattributes\" */\n \t\tstrbuf_setlen(&pathbuf, len);\n@@ -1073,9 +1122,9 @@ static void determine_macros(struct all_attrs_item *all_attrs,\n * If check->check_nr is non-zero, only attributes in check[] are collected.\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+static void collect_some_attrs(struct index_state *istate, const char *path,\n+\t\t\t struct attr_check *check,\n+\t\t\t const char *object_name)\n {\n \tint pathlen, rem, dirlen;\n \tconst char *cp, *last_slash = NULL;\n@@ -1094,7 +1143,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, path, dirlen, &check->stack, object_name);\n \tall_attrs_init(&g_attr_hashmap, check);\n \tdetermine_macros(check->all_attrs, check->stack);\n \n@@ -1102,13 +1151,12 @@ static void collect_some_attrs(struct index_state *istate,\n \tfill(path, pathlen, basename_offset, check->stack, check->all_attrs, rem);\n }\n \n-void git_check_attr(struct index_state *istate,\n-\t\t const char *path,\n-\t\t struct attr_check *check)\n+void git_check_attr(struct index_state *istate, const char *path,\n+\t\t struct attr_check *check, const char *object_name)\n {\n \tint i;\n \n-\tcollect_some_attrs(istate, path, check);\n+\tcollect_some_attrs(istate, path, check, object_name);\n \n \tfor (i = 0; i < check->nr; i++) {\n \t\tsize_t n = check->items[i].attr->attr_nr;\n@@ -1119,13 +1167,13 @@ void git_check_attr(struct index_state *istate,\n \t}\n }\n \n-void git_all_attrs(struct index_state *istate,\n-\t\t const char *path, struct attr_check *check)\n+void git_all_attrs(struct index_state *istate, const char *path,\n+\t\t struct attr_check *check, const char *object_name)\n {\n \tint i;\n \n \tattr_check_reset(check);\n-\tcollect_some_attrs(istate, path, check);\n+\tcollect_some_attrs(istate, path, check, object_name);\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..0708d6e046 100644\n--- a/attr.h\n+++ b/attr.h\n@@ -169,6 +169,7 @@ struct attr_check {\n \tint all_attrs_nr;\n \tstruct all_attrs_item *all_attrs;\n \tstruct attr_stack *stack;\n+\tchar *object_id;\n };\n \n struct attr_check *attr_check_alloc(void);\n@@ -190,14 +191,16 @@ 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\t\t\tconst char *path, struct attr_check *check,\n+\t\t\t\t\tconst char *object_name);\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-\t\t const char *path, struct attr_check *check);\n+\t\t\t\t const char *path, struct attr_check *check,\n+\t\t\t\t const char *object_name);\n \n enum git_attr_direction {\n \tGIT_ATTR_CHECKIN,\ndiff --git a/builtin/check-attr.c b/builtin/check-attr.c\nindex 0fef10eb6b..3e032420ed 100644\n--- a/builtin/check-attr.c\n+++ b/builtin/check-attr.c\n@@ -9,6 +9,7 @@\n static int all_attrs;\n static int cached_attrs;\n static int stdin_paths;\n+static char *object_name;\n static const char * const check_attr_usage[] = {\n N_(\"git check-attr [-a | --all | ...] [--] ...\"),\n N_(\"git check-attr --stdin [-z] [-a | --all | ...]\"),\n@@ -23,6 +24,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\", &object_name, N_(\"revision\"), N_(\"check attributes at this revision\")),\n \tOPT_END()\n };\n \n@@ -55,27 +57,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-\t\t const char *file)\n+static void check_attr(const char *prefix, struct attr_check *check,\n+\t\t int collect_all, const char *file,\n+\t\t const char *object_name\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, full_path, check, object_name);\n \t} else {\n-\t\tgit_check_attr(&the_index, full_path, check);\n+\t\tgit_check_attr(&the_index, full_path, check, object_name);\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 int collect_all, const char *object_name)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct strbuf unquoted = STRBUF_INIT;\n@@ -89,7 +90,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, collect_all, buf.buf, object_name);\n \t\tmaybe_flush_or_die(stdout, \"attribute to stdout\");\n \t}\n \tstrbuf_release(&buf);\n@@ -177,10 +178,10 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (stdin_paths)\n-\t\tcheck_attr_stdin_paths(prefix, check, all_attrs);\n+\t\tcheck_attr_stdin_paths(prefix, check, all_attrs, object_name);\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, all_attrs, argv[i], object_name);\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 573d0b20b7..f57a0188e6 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, path, check, NULL);\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..095729004f 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, path, check, NULL);\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..45a534badf 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, path, check, NULL);\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, path, check, NULL);\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..b5217e4c36 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, name, item->attr_check, NULL);\n \n \tfree(to_free);\n \ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex b3aabb8aa3..90a3680a31 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,6 +86,26 @@ 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+\tgit commit-tree $(cat id) -m \"random commit message\" > id &&\n+\tgit update-ref refs/heads/branch1 $(cat 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+\tgit commit-tree $(cat id) -m \"random commit message\" > id &&\n+\tgit update-ref refs/heads/branch2 $(cat 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@@ -287,6 +313,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 +341,25 @@ 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\tgit commit-tree $(cat id) -m \"random commit message\" > id &&\n+\t\tgit update-ref refs/heads/master $(cat 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..364f5a5ea2 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, path, check, NULL);\n \n \tif (ATTR_TRUE(check->items[0].value))\n \t\treturn &driver_true;\ndiff --git a/ws.c b/ws.c\nindex 6e69877f25..8cc3e429c1 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, pathname, attr_whitespace_rule, NULL);\n \tvalue = attr_whitespace_rule->items[0].value;\n \tif (ATTR_TRUE(value)) {\n \t\t/* true (whitespace) */\n-- \n2.38.1\n\n"},{"id":"468617","messageId":"64522fbf-c006-700f-67b5-749093d6e962@iee.email","threadId":"58907","inReplyTo":"20221206103736.53909-1-karthik.188@gmail.com","subject":"Re: [PATCH 0/2] check-attr: add support to work with revisions","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2022-12-06T11:20:47Z","receivedAt":"2022-12-06T11:20:55Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"Hi Karthik ,\n\nOn 06/12/2022 10:37, Karthik Nayak wrote:\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\nWon't this also need a man page update? I don't see any changes to\ngit/Documentation/git-check-attr.txt.\n\nPhilip\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> archive.c | 2 +-\n> attr.c | 120 ++++++++++++++++++++++++++++-------------\n> attr.h | 7 ++-\n> builtin/check-attr.c | 25 ++++-----\n> builtin/pack-objects.c | 2 +-\n> convert.c | 2 +-\n> ll-merge.c | 4 +-\n> pathspec.c | 2 +-\n> t/t0003-attributes.sh | 63 ++++++++++++++++++++--\n> userdiff.c | 2 +-\n> ws.c | 2 +-\n> 11 files changed, 170 insertions(+), 61 deletions(-)\n>\n\n"},{"id":"468619","messageId":"221206.861qpcdarb.gmgdl@evledraar.gmail.com","threadId":"58907","inReplyTo":"20221206103736.53909-3-karthik.188@gmail.com","subject":"Re: [PATCH 2/2] attr: add flag `-r|--revisions` to work with revisions","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-06T11:27:46Z","receivedAt":"2022-12-06T11:36:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Dec 06 2022, Karthik Nayak wrote:\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>\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>\n> We cannot use the `:` 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> parses the attributes line by line. This function is plugged into\n> `read_attr()` function wherein we go through the different attributes.\n>\n> Signed-off-by: Karthik Nayak \n> Co-authored-by: toon@iotcl.com\n> ---\n> archive.c | 2 +-\n> attr.c | 120 ++++++++++++++++++++++++++++-------------\n> attr.h | 7 ++-\n> builtin/check-attr.c | 25 ++++-----\n> builtin/pack-objects.c | 2 +-\n> convert.c | 2 +-\n> ll-merge.c | 4 +-\n> pathspec.c | 2 +-\n> t/t0003-attributes.sh | 56 ++++++++++++++++++-\n> userdiff.c | 2 +-\n> ws.c | 2 +-\n> 11 files changed, 165 insertions(+), 59 deletions(-)\n>\n> diff --git a/archive.c b/archive.c\n> index 941495f5d7..bf64dbdce1 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, path, check, NULL);\n> \treturn check;\n> }\n> \n> diff --git a/attr.c b/attr.c\n> index 42ad6de8c7..42b67a401f 100644\n> --- a/attr.c\n> +++ b/attr.c\n> @@ -11,8 +11,12 @@\n> #include \"exec-cmd.h\"\n> #include \"attr.h\"\n> #include \"dir.h\"\n> +#include \"git-compat-util.h\"\n\nAs a rule in this project we include either cache.h at the top, or\ngit-compat-util.h, and the former includes the latter.\n\nThis file already has cache.h at the top, so it won't need this.\n\n> +\tif (buf == NULL)\n\nif (!buf)\n\n> +\t\tmore = (*ep == '\\n');\n\nNit: parens not needed.\n\n> +\tstruct object_id oid;\n> +\tunsigned long sz;\n> +\tenum object_type type;\n> +\tvoid *buf;\n> +\tstruct strbuf sb;\n> +\n> +\tif (object_name == NULL)\n\nDitto !object_name test.\n\n> +\t\treturn NULL;\n> +\n> +\tstrbuf_init(&sb, strlen(path) + 1 + strlen(object_name));\n> +\tstrbuf_addstr(&sb, object_name);\n> +\tstrbuf_addstr(&sb, \":\");\n> +\tstrbuf_addstr(&sb, path);\n\nIs this really performance sensitive so we need to pre-size this, or\nwould a simpler:\n\n\tstruct strbuf sb = STRBUF_INIT;\n strbuf_addf(&sb, \"%s:%s\", path, object_name);\n\nDo?\n\n> +\t} else if (object_name != NULL) {\n\nelse if (object_name)\n\n> void git_check_attr(struct index_state *istate,\n> -\t\t const char *path, struct attr_check *check);\n> +\t\t\t\t\tconst char *path, struct attr_check *check,\n> +\t\t\t\t\tconst char *object_name);\n\nThis (and possibly other places) seem funnily indented..\n\n> \tif (collect_all) {\n> -\t\tgit_all_attrs(&the_index, full_path, check);\n> +\t\tgit_all_attrs(&the_index, full_path, check, object_name);\n> \t} else {\n> -\t\tgit_check_attr(&the_index, full_path, check);\n> +\t\tgit_check_attr(&the_index, full_path, check, object_name);\n> \t}\n\nEarlier you do a get_oid(), shouldn't that be a\nrepo_get_oid(istate->repo, ...) to be future-proof? I.e. use the repo of\nthe passed-in index.\n\nI think it'll always be \"the_repository\" for now, but for an API it\nmakes sense to hardcode that assumption in fewer places.\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\nYou don't need a subshell just to echo a string. You can use {}-braces,\nbut in this case just:\n\n printf \"%s\\n\", \"f test=f\" \"a/i test=a/i\" | git hash-object .... &&\n\n\n> +\t\t) | git hash-object -w --stdin > id &&\n> +\t\tgit update-index --add --cacheinfo 100644 $(cat id) .gitattributes &&\n\nSplit the \"cat\" into a varible, otherwise its failure will be hidden.\n\n> +\t\tgit write-tree > id &&\n\nWe use \">id\" style for redirection, not \"> id\".\n\n> +\t\tgit commit-tree $(cat id) -m \"random commit message\" > id &&\n\nDitto..\n"},{"id":"468622","messageId":"CAOLa=ZSUpCSUvB==LzTHE5Hz_zzV8xL8KUjj3qMg6WqyexpXhg@mail.gmail.com","threadId":"58907","inReplyTo":"64522fbf-c006-700f-67b5-749093d6e962@iee.email","subject":"Re: [PATCH 0/2] check-attr: add support to work with revisions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2022-12-06T13:00:21Z","receivedAt":"2022-12-06T13:01:40Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Hey Philip,\n\nOn Tue, Dec 6, 2022 at 12:20 PM Philip Oakley wrote:\n>\n> Won't this also need a man page update? I don't see any changes to\n> git/Documentation/git-check-attr.txt.\n>\n\nYou're right, I totally missed that, will add it to the v2 of the\nseries. Thanks!\n\n- Karthik\n"},{"id":"468623","messageId":"CAOLa=ZSOhtgmY-j3rj+=qANKOfTH8H+1YpwM63uTKUT04vsM-Q@mail.gmail.com","threadId":"58907","inReplyTo":"221206.861qpcdarb.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 2/2] attr: add flag `-r|--revisions` to work with revisions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2022-12-06T13:06:36Z","receivedAt":"2022-12-06T13:09:19Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Hello Ævar,\n\n> >\n> > diff --git a/attr.c b/attr.c\n> > index 42ad6de8c7..42b67a401f 100644\n> > --- a/attr.c\n> > +++ b/attr.c\n> > @@ -11,8 +11,12 @@\n> > #include \"exec-cmd.h\"\n> > #include \"attr.h\"\n> > #include \"dir.h\"\n> > +#include \"git-compat-util.h\"\n>\n> As a rule in this project we include either cache.h at the top, or\n> git-compat-util.h, and the former includes the latter.\n>\n> This file already has cache.h at the top, so it won't need this.\n>\n\nRight, will remove.\n\n> > + if (buf == NULL)\n>\n> if (!buf)\n\nMakes sense.\n\n>\n> > + more = (*ep == '\\n');\n>\n> Nit: parens not needed.\n>\n\nI rather let this be, since it's existing code that I just move.\n\n> > + struct object_id oid;\n> > + unsigned long sz;\n> > + enum object_type type;\n> > + void *buf;\n> > + struct strbuf sb;\n> > +\n> > + if (object_name == NULL)\n>\n> Ditto !object_name test.\n>\n\nWill change.\n\n> > + return NULL;\n> > +\n> > + strbuf_init(&sb, strlen(path) + 1 + strlen(object_name));\n> > + strbuf_addstr(&sb, object_name);\n> > + strbuf_addstr(&sb, \":\");\n> > + strbuf_addstr(&sb, path);\n>\n> Is this really performance sensitive so we need to pre-size this, or\n> would a simpler:\n>\n> struct strbuf sb = STRBUF_INIT;\n> strbuf_addf(&sb, \"%s:%s\", path, object_name);\n>\n> Do?\n>\n\nThis is much simpler and should do, will change.\n\n> > + } else if (object_name != NULL) {\n>\n> else if (object_name)\n\nWill change.\n\n>\n> > void git_check_attr(struct index_state *istate,\n> > - const char *path, struct attr_check *check);\n> > + const char *path, struct attr_check *check,\n> > + const char *object_name);\n>\n> This (and possibly other places) seem funnily indented..\n>\n\nI think it's due to tab-indent being set to a default 4, I fixed it in\nthe other files, forgot to\ncheck the header. Will fix it.\n\n> > if (collect_all) {\n> > - git_all_attrs(&the_index, full_path, check);\n> > + git_all_attrs(&the_index, full_path, check, object_name);\n> > } else {\n> > - git_check_attr(&the_index, full_path, check);\n> > + git_check_attr(&the_index, full_path, check, object_name);\n> > }\n>\n> Earlier you do a get_oid(), shouldn't that be a\n> repo_get_oid(istate->repo, ...) to be future-proof? I.e. use the repo of\n> the passed-in index.\n>\n> I think it'll always be \"the_repository\" for now, but for an API it\n> makes sense to hardcode that assumption in fewer places.\n>\n\nI will make this change, I didn't know about repo_get_oid() before.\n\n> > +test_expect_success 'bare repository: with --revision' '\n> > + (\n> > + cd bare.git &&\n> > + (\n> > + echo \"f test=f\" &&\n> > + echo \"a/i test=a/i\"\n>\n> You don't need a subshell just to echo a string. You can use {}-braces,\n> but in this case just:\n>\n> printf \"%s\\n\", \"f test=f\" \"a/i test=a/i\" | git hash-object .... &&\n>\n>\n\nWhile I agree with what you're saying, the whole test file does it\nthis way (echo in a subshell),\nwouldn't it be better to stay consistent?\n\n> > + ) | git hash-object -w --stdin > id &&\n> > + git update-index --add --cacheinfo 100644 $(cat id) .gitattributes &&\n>\n> Split the \"cat\" into a varible, otherwise its failure will be hidden.\n>\n\nDone.\n\n> > + git write-tree > id &&\n>\n> We use \">id\" style for redirection, not \"> id\".\n>\n> > + git commit-tree $(cat id) -m \"random commit message\" > id &&\n>\n> Ditto..\n\nWill make this change too.\n\nThanks for the review. Will wait a day or two before sending in the\nnext version.\n\n-- \n- Karthik\n"},{"id":"468652","messageId":"xmqqedtc842m.fsf@gitster.g","threadId":"58907","inReplyTo":"20221206103736.53909-3-karthik.188@gmail.com","subject":"Re: [PATCH 2/2] attr: add flag `-r|--revisions` to work with revisions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-07T00:12:17Z","receivedAt":"2022-12-07T00:12:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\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\nAs \"check-attr\" was not invented as a user-facing subcommand but was\na hack for debugging, I would have minded this change, but these\ndays people seem to treat it as if it is just one of the proper\nplumbing commands, the new command line convention bothers me a\nbit. No other command uses -- to signal that what comes\nafter it is a rev.\n\nBut I do not think of a better alternative without making the\ncommand line ambiguous, so I'll stop at raising a concern, so that\nothers who may be better at UI can come up with one.\n\nAs to the C API, please do not append the new parameter at the end.\nWhen there are no logical ordering among the things listed, be it in\nthe members of a struct or the parameters to a function, we encourage\nto append, but in this case\n\n void git_check_attr(struct index_state *istate,\n const char *path,\n struct attr_check *check)\n\nwe are saying \"pick , referring to .gitattributes files from\nthe index as needed, and apply the checks in check[]\", and the new\nbehaviour is \"pick , referring to .gitattributes files from\nthe tree-ish as needed, and do the same\", so istate and the tree-ish\nobject should sit together.\n\nAlso, at the C API level, I suspect that we strongly prefer to pass\naround either the \"struct object_id *\" or \"struct tree *\", not working\nwith end-user supplied character strings without any sanity-checking\nor parsing.\n\nAnother concern, among many existing callers of git_check_attr(),\nis there any that will conceivably benefit in the future if they\ncould read the attributes from a tree-ish? If not, it may make a\nlot more sense if you did not butcher them, and instead introduce a\nnew API function git_check_attr_from_tree() and use it in the only\nplace you handle the \"-r tree-ish\" command line option in the\nupdated part of the \"git check-attr\" command.\n\nThanks.\n"},{"id":"468663","messageId":"Y4/ntYotGKz2dx0E@nand.local","threadId":"58907","inReplyTo":"20221206103736.53909-1-karthik.188@gmail.com","subject":"Re: [PATCH 0/2] check-attr: add support to work with revisions","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-12-07T01:09:09Z","receivedAt":"2022-12-07T01:09:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Dec 06, 2022 at 11:37:34AM +0100, Karthik Nayak wrote:\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 haven't looked at the patches below yet, so take my $.02 with a grain\nof salt, but I have definitely wished for something like this in the\npast.\n\nWhen scripting around to figure out which files at a given revision are\naffected by a given attribute, I will often have to resort to writing\nsomething like this when working in a bare repository:\n\n for repo in $LIST_OF_REPOS\n do\n export GIT_DIR=\"$repo\"\n\n index=\"$(mktemp ...)\"\n\t\tgit read-tree --index-output=\"$index\" HEAD 2>/dev/null || continue\n\n git ls-tree -rz --name-only HEAD |\n GIT_INDEX_FILE=\"$index\" git check-attr --cached --stdin -z $attr |\n ruby -e '\n print $stdin.readlines.join.split(\"\\0\").\n each_slice(3).\n select { |path, _, val| val != \"unspecified\" && val != \"unset\" }.\n map(&:first).join(\"\\0\")\n '\n done\n\nWhich is just kind of gross.\n\nI had at one point when writing the above script wished for a '--blob'\nsource option to check-attr. That might be sensible, but I think being\nable to read from an arbitrary revision (looking at all of the relevant\n.gitattributes file(s) recursively throughout the tree) is even more\nuseful, since it allows you to accurately construct the attributes state\nin its entirety.\n\nAnyway, my point is that I think that this is a useful feature, and one\nthat I (and I suspect other users, too) have wished for frequently in\nthe past.\n\nThanks,\nTaylor\n"},{"id":"468664","messageId":"CAPig+cT_xz3m-3kkL_Scf0opNXkikU94kSF46VO8KHkNMWbGzg@mail.gmail.com","threadId":"58907","inReplyTo":"xmqqedtc842m.fsf@gitster.g","subject":"Re: [PATCH 2/2] attr: add flag `-r|--revisions` to work with revisions","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-12-07T01:10:26Z","receivedAt":"2022-12-07T01:10:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Dec 6, 2022 at 7:15 PM Junio C Hamano wrote:\n> Karthik Nayak writes:\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>\n> As \"check-attr\" was not invented as a user-facing subcommand but was\n> a hack for debugging, I would have minded this change, but these\n> days people seem to treat it as if it is just one of the proper\n> plumbing commands, the new command line convention bothers me a\n> bit. No other command uses -- to signal that what comes\n> after it is a rev.\n>\n> But I do not think of a better alternative without making the\n> command line ambiguous, so I'll stop at raising a concern, so that\n> others who may be better at UI can come up with one.\n\nA few minor comments...\n\nWe don't usually squat on short options, such as `-r`, right from the\nstart but only add the short alias once shown that there is demand.\n\nOption `-r` has strong association with \"recursive\" elsewhere, so I'd\nworry about giving it such a completely different meaning here.\n\nRather than calling the option `--revision`, perhaps pattern it after\ngit-restore's `--source` option which allows specifying a particular\ncommit, tree, tag, etc.?\n"},{"id":"468673","messageId":"Y4/1uNEcM1If4sA9@tapette.crustytoothpaste.net","threadId":"58907","inReplyTo":"Y4/ntYotGKz2dx0E@nand.local","subject":"Re: [PATCH 0/2] check-attr: add support to work with revisions","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2022-12-07T02:11:03Z","receivedAt":"2022-12-07T02:11:09Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2022-12-07 at 01:09:09, Taylor Blau wrote:\n> Anyway, my point is that I think that this is a useful feature, and one\n> that I (and I suspect other users, too) have wished for frequently in\n> the past.\n\nI fully agree. This would have come in useful for me many times, and I\ncan tell you as one of the maintainers of Git LFS that this will be\nenormously useful to many users of that tool as well. I'm glad to see\nthis series come by.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"468701","messageId":"CAOLa=ZTSzUh2Ma_EMHHWcDunGyKMaUW9BaG=QdegtMqLd+69Wg@mail.gmail.com","threadId":"58907","inReplyTo":"CAPig+cT_xz3m-3kkL_Scf0opNXkikU94kSF46VO8KHkNMWbGzg@mail.gmail.com","subject":"Re: [PATCH 2/2] attr: add flag `-r|--revisions` to work with revisions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2022-12-07T11:05:27Z","receivedAt":"2022-12-07T11:08:45Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Hey Eric,\n\nOn Wed, Dec 7, 2022 at 2:10 AM Eric Sunshine wrote:\n>\n> On Tue, Dec 6, 2022 at 7:15 PM Junio C Hamano wrote:\n> > Karthik Nayak writes:\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> >\n> > As \"check-attr\" was not invented as a user-facing subcommand but was\n> > a hack for debugging, I would have minded this change, but these\n> > days people seem to treat it as if it is just one of the proper\n> > plumbing commands, the new command line convention bothers me a\n> > bit. No other command uses -- to signal that what comes\n> > after it is a rev.\n> >\n> > But I do not think of a better alternative without making the\n> > command line ambiguous, so I'll stop at raising a concern, so that\n> > others who may be better at UI can come up with one.\n>\n> A few minor comments...\n>\n> We don't usually squat on short options, such as `-r`, right from the\n> start but only add the short alias once shown that there is demand.\n>\n> Option `-r` has strong association with \"recursive\" elsewhere, so I'd\n> worry about giving it such a completely different meaning here.\n>\n> Rather than calling the option `--revision`, perhaps pattern it after\n> git-restore's `--source` option which allows specifying a particular\n> commit, tree, tag, etc.?\n\n\nI'm okay with removing the shortform `-r`. I decided to use `-r|--revision`\nbecause it is also used in `git svn`.\n\nI don't have strong feeling about the naming here, but I do feel that\n`-r|--revision` sounds better than `--source`, because source doesn't directly\nimply a revision and in the context of `git check-attr` is a bit ambiguous\nbecause it could imply the source regarding which `gitattributes` file to\ncheck against (although this feature doesn't exist).\n\n-- \n- Karthik\n"},{"id":"468703","messageId":"CAOLa=ZS5k=s98Bo9GE+RRa3jtanehL35y-hhLhy1DoM7GyO0cQ@mail.gmail.com","threadId":"58907","inReplyTo":"xmqqedtc842m.fsf@gitster.g","subject":"Re: [PATCH 2/2] attr: add flag `-r|--revisions` to work with revisions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2022-12-07T11:40:29Z","receivedAt":"2022-12-07T11:41:02Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Dec 7, 2022 at 1:12 AM Junio C Hamano wrote:\n>\n> Karthik Nayak writes:\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>\n> As \"check-attr\" was not invented as a user-facing subcommand but was\n> a hack for debugging, I would have minded this change, but these\n> days people seem to treat it as if it is just one of the proper\n> plumbing commands, the new command line convention bothers me a\n> bit. No other command uses -- to signal that what comes\n> after it is a rev.\n>\n> But I do not think of a better alternative without making the\n> command line ambiguous, so I'll stop at raising a concern, so that\n> others who may be better at UI can come up with one.\n>\n\nI understand this concern, but I do think it would be really useful. I see that\nTaylor and Brian have also mentioned how this would be useful. As such, I\nthink it's a good indication to take this forward.\n\n> As to the C API, please do not append the new parameter at the end.\n> When there are no logical ordering among the things listed, be it in\n> the members of a struct or the parameters to a function, we encourage\n> to append, but in this case\n>\n> void git_check_attr(struct index_state *istate,\n> const char *path,\n> struct attr_check *check)\n>\n> we are saying \"pick , referring to .gitattributes files from\n> the index as needed, and apply the checks in check[]\", and the new\n> behaviour is \"pick , referring to .gitattributes files from\n> the tree-ish as needed, and do the same\", so istate and the tree-ish\n> object should sit together.\n>\n\nI will be more cognizant of this approach and make these changes.\n\n> Also, at the C API level, I suspect that we strongly prefer to pass\n> around either the \"struct object_id *\" or \"struct tree *\", not working\n> with end-user supplied character strings without any sanity-checking\n> or parsing.\n>\n\nI must admit, I did take the path of least resistance here. So we finally need\nto parse the `revision:` where the `` is generated\ndynamically as we move through the check-attr stack.\n\nMy question is, if I generate an `object_id` at the start (in\nbuiltin/check-attr.c)\nwith only the `revision`, is there a way to traverse to find the blob\nfor each of\nthe different ? I haven't looked at Git code for a while now, and\nI'm not sure what the best way to do this. Maybe I've missed some API which\nwould make this simple, any help is appreciated.\n\n> Another concern, among many existing callers of git_check_attr(),\n> is there any that will conceivably benefit in the future if they\n> could read the attributes from a tree-ish? If not, it may make a\n> lot more sense if you did not butcher them, and instead introduce a\n> new API function git_check_attr_from_tree() and use it in the only\n> place you handle the \"-r tree-ish\" command line option in the\n> updated part of the \"git check-attr\" command.\n>\n\nThe concern then is that we'll also have to duplicate some of the code in\n`git_all_attrs` and `git_check_attr` and have some logic to work\ncorrectly for with\nand without the `-a` option. All in all, this was much easier.\n\nI don't have enough knowledge to say if the other callers will someday\nwant to extend\nto providing a revision too, so I'll leave that to you!\n\n> Thanks.\n\nThanks for the review.\n\n--\n- Karthik\n"},{"id":"468704","messageId":"221207.86lenja0zi.gmgdl@evledraar.gmail.com","threadId":"58907","inReplyTo":"xmqqedtc842m.fsf@gitster.g","subject":"Re: [PATCH 2/2] attr: add flag `-r|--revisions` to work with revisions","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-07T11:38:23Z","receivedAt":"2022-12-07T11:48:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Dec 07 2022, Junio C Hamano wrote:\n\n> Karthik Nayak writes:\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>\n> As \"check-attr\" was not invented as a user-facing subcommand but was\n> a hack for debugging, I would have minded this change, but these\n> days people seem to treat it as if it is just one of the proper\n> plumbing commands, the new command line convention bothers me a\n> bit. No other command uses -- to signal that what comes\n> after it is a rev.\n>\n> But I do not think of a better alternative without making the\n> command line ambiguous, so I'll stop at raising a concern, so that\n> others who may be better at UI can come up with one.\n\nI don't really like it either, but maybe we've backed ourselves into a\ncorner here.\n\nBut let's look at that. So the command takes:\n\n\tgit check-attr ... -- ...\n\nOr:\n\n\techo \"\" |\n\tgit check-attr --stdin ...\n\nSo we'd want to specify a , without making the or\n ambiguous.\n\nNow, when we map the attributes we go through attr_name_valid(), which\nchecks that the attribute names are valid. A commentthere says:\n\n * Attribute name cannot begin with '-' and must consist of \n * characters from [-A-Za-z0-9_.].\n\nSo can't we instead accept:\n\n\tgit check-attr [:]... -- ...\n\nI.e.:\n\n\tgit check-attr HEAD~:foo -- path\n\nAnd it wouldn't be ambiguous because attribute names can't contain \":\"?\nThis would be consistent with e.g. \"git show\" and \"git cat-file\", just\nwith \"\" instead of the \"\".\n\nThis would also mean that we'd support:\n\n\tgit check-attr HEAD:foo HEAD~:bar HEAD~2:baz\n\netc., i.e. the ability to support multiple revision/attribute\npairs. Skimming the currently proposed code there seems to be no good\nreason not to support that (we just need to look up other blobs), and it\nwould allow querying those without spawning N processes.\n"},{"id":"468705","messageId":"221207.86h6y7a0l7.gmgdl@evledraar.gmail.com","threadId":"58907","inReplyTo":"CAOLa=ZS5k=s98Bo9GE+RRa3jtanehL35y-hhLhy1DoM7GyO0cQ@mail.gmail.com","subject":"Re: [PATCH 2/2] attr: add flag `-r|--revisions` to work with revisions","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-07T11:53:00Z","receivedAt":"2022-12-07T11:58:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Dec 07 2022, Karthik Nayak wrote:\n\n> On Wed, Dec 7, 2022 at 1:12 AM Junio C Hamano wrote:\n> [...]\n>> Also, at the C API level, I suspect that we strongly prefer to pass\n>> around either the \"struct object_id *\" or \"struct tree *\", not working\n>> with end-user supplied character strings without any sanity-checking\n>> or parsing.\n>>\n>\n> I must admit, I did take the path of least resistance here. So we finally need\n> to parse the `revision:` where the `` is generated\n> dynamically as we move through the check-attr stack.\n>\n> My question is, if I generate an `object_id` at the start (in\n> builtin/check-attr.c)\n> with only the `revision`, is there a way to traverse to find the blob\n> for each of\n> the different ? I haven't looked at Git code for a while now, and\n> I'm not sure what the best way to do this. Maybe I've missed some API which\n> would make this simple, any help is appreciated.\n\nThe get_oid() that you're doing now happens in a loop, and should be\npulled out of it.\n\nI suggested making that a feature in\nhttps://lore.kernel.org/git/221207.86lenja0zi.gmgdl@evledraar.gmail.com/;\nbut if you keep the interface you've got where you only support a single\n it would make the most sense to do that get_oid() in the builtin/\ncode at the start, and then pass the oid/path pair down.\n\nYou'd still need to call read_object_file() or equivalent for each\n: pair though.\n"},{"id":"468709","messageId":"CAOLa=ZTQ9R1dyCb5xp0c5JBLjE33dJFVPg8jQfprU1iSGq+O4g@mail.gmail.com","threadId":"58907","inReplyTo":"221207.86h6y7a0l7.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 2/2] attr: add flag `-r|--revisions` to work with revisions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2022-12-07T12:29:55Z","receivedAt":"2022-12-07T12:30:27Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Dec 7, 2022 at 12:56 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n>\n> On Wed, Dec 07 2022, Karthik Nayak wrote:\n>\n> > On Wed, Dec 7, 2022 at 1:12 AM Junio C Hamano wrote:\n> > [...]\n> >> Also, at the C API level, I suspect that we strongly prefer to pass\n> >> around either the \"struct object_id *\" or \"struct tree *\", not working\n> >> with end-user supplied character strings without any sanity-checking\n> >> or parsing.\n> >>\n> >\n> > I must admit, I did take the path of least resistance here. So we finally need\n> > to parse the `revision:` where the `` is generated\n> > dynamically as we move through the check-attr stack.\n> >\n> > My question is, if I generate an `object_id` at the start (in\n> > builtin/check-attr.c)\n> > with only the `revision`, is there a way to traverse to find the blob\n> > for each of\n> > the different ? I haven't looked at Git code for a while now, and\n> > I'm not sure what the best way to do this. Maybe I've missed some API which\n> > would make this simple, any help is appreciated.\n>\n> The get_oid() that you're doing now happens in a loop, and should be\n> pulled out of it.\n>\n> I suggested making that a feature in\n> https://lore.kernel.org/git/221207.86lenja0zi.gmgdl@evledraar.gmail.com/;\n> but if you keep the interface you've got where you only support a single\n> it would make the most sense to do that get_oid() in the builtin/\n> code at the start, and then pass the oid/path pair down.\n>\n> You'd still need to call read_object_file() or equivalent for each\n> : pair though.\n\nThat's exactly my question, now we're calling `get_oid()` with the pathname.\nWhich would directly give us the object_id for the blob. Whereas if we call\n`get_oid()` _without_ the pathname, we would still need to get the object_id\nfor the blob.\n\nMy specific question being, how do I, given:\n\nstruct object_id oid_1;\nget_oid(revision, &oid)\n\nand pathname, get the equivalent of:\n\nstruct object_id oid_2;\nstrbuf_addf(&sb, \"%s:%s\", revision, path);\nget_oid(sb.buf, &oid)\n\n?\n\nBasically, I fail to see how to transform oid_1 to oid_2, using\npathname. I agree\nthis would eventually be fed into `read_object_file`.\n\n-- \n- Karthik\n"},{"id":"468711","messageId":"CAOLa=ZQua8TfApCdzoK06_2fkWb4ZCfWewXKOSaXno1fqFSq2A@mail.gmail.com","threadId":"58907","inReplyTo":"221207.86lenja0zi.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 2/2] attr: add flag `-r|--revisions` to work with revisions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2022-12-07T12:33:22Z","receivedAt":"2022-12-07T12:34:18Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Dec 7, 2022 at 12:48 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n>\n> On Wed, Dec 07 2022, Junio C Hamano wrote:\n>\n> > Karthik Nayak writes:\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> >\n> > As \"check-attr\" was not invented as a user-facing subcommand but was\n> > a hack for debugging, I would have minded this change, but these\n> > days people seem to treat it as if it is just one of the proper\n> > plumbing commands, the new command line convention bothers me a\n> > bit. No other command uses -- to signal that what comes\n> > after it is a rev.\n> >\n> > But I do not think of a better alternative without making the\n> > command line ambiguous, so I'll stop at raising a concern, so that\n> > others who may be better at UI can come up with one.\n>\n> I don't really like it either, but maybe we've backed ourselves into a\n> corner here.\n>\n> But let's look at that. So the command takes:\n>\n> git check-attr ... -- ...\n>\n> Or:\n>\n> echo \"\" |\n> git check-attr --stdin ...\n>\n> So we'd want to specify a , without making the or\n> ambiguous.\n>\n> Now, when we map the attributes we go through attr_name_valid(), which\n> checks that the attribute names are valid. A commentthere says:\n>\n> * Attribute name cannot begin with '-' and must consist of\n> * characters from [-A-Za-z0-9_.].\n>\n> So can't we instead accept:\n>\n> git check-attr [:]... -- ...\n>\n> I.e.:\n>\n> git check-attr HEAD~:foo -- path\n>\n> And it wouldn't be ambiguous because attribute names can't contain \":\"?\n> This would be consistent with e.g. \"git show\" and \"git cat-file\", just\n> with \"\" instead of the \"\".\n>\n> This would also mean that we'd support:\n>\n> git check-attr HEAD:foo HEAD~:bar HEAD~2:baz\n>\n> etc., i.e. the ability to support multiple revision/attribute\n> pairs. Skimming the currently proposed code there seems to be no good\n> reason not to support that (we just need to look up other blobs), and it\n> would allow querying those without spawning N processes.\n\nThanks for this walkthrough, quick question, this wouldn't work with\nthe `-a` condition, right?\nCurrent patch series tends to work with/without `-a`.\n\nAlso, my personal opinion is that when being consistent we need to be fully\nconsistent, i.e. :, tweaking this slightly to be\n: is\nworse than breaking consistency.\n\n-- \n- Karthik\n"}]}