* [PATCH v5 0/2] check-attr: add support to work with tree-ish
[not found] <https://lore.kernel.org/git/cover.1671630304.git.karthik.188@gmail.com/>
@ 2023-01-02 11:04 ` Karthik Nayak
2023-01-02 11:04 ` [PATCH v5 1/2] t0003: move setup for `--all` into new block Karthik Nayak
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Karthik Nayak @ 2023-01-02 11:04 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak
v1: https://lore.kernel.org/git/20221206103736.53909-1-karthik.188@gmail.com/
v2: https://lore.kernel.org/git/CAOLa=ZSsFGBw3ta1jWN8cmUch2ca=zTEjp1xMA6Linafx9W53g@mail.gmail.com/T/#t
v3: https://lore.kernel.org/git/20221216093552.3171319-1-karthik.188@gmail.com/
v4: https://lore.kernel.org/git/cover.1671630304.git.karthik.188@gmail.com
Given a pathname, git-check-attr(1) will list the attributes which apply to that
pathname by reading all relevant gitattributes files. Currently there is no way
to specify a tree-ish to read the gitattributes from.
This is specifically useful in bare repositories wherein the gitattributes are
only present in the git working tree but not available directly on the
filesystem.
This series aims to add a new flag `--source` to git-check-attr(1) which
allows us to read gitattributes from the specified tree-ish.
Changes since version 4:
- Changed the flag from `--revision` to `--source`
- Removed uneeded header imports
- Using a pre-initialized variable instead of malloc for the tree_oid
- Using `die()` instead of `error()` for bad tree-ish provided
Range-diff against v4:
1: 6224754179 = 1: 6224754179 t0003: move setup for `--all` into new block
2: a161dbdf8b ! 2: d835d989ad attr: add flag `--revision` to work with revisions
@@ Metadata
Author: Karthik Nayak <karthik.188@gmail.com>
## Commit message ##
- attr: add flag `--revision` to work with revisions
+ attr: add flag `--source` to work with tree-ish
The contents of the .gitattributes files may evolve over time, but "git
check-attr" always checks attributes against them in the working tree
and/or in the index. It may be beneficial to optionally allow the users
to check attributes taken from a commit other than HEAD against paths.
- Add a new flag `--revision` which will allow users to check the
+ Add a new flag `--source` which will allow users to check the
attributes against a commit (actually any tree-ish would do). When the
user uses this flag, we go through the stack of .gitattributes files but
instead of checking the current working tree and/or in the index, we
check the blobs from the provided tree-ish object. This allows the
command to also be used in bare repositories.
- Since we use a tree-ish object, the user can pass "--revision
+ Since we use a tree-ish object, the user can pass "--source
HEAD:subdirectory" and all the attributes will be looked up as if
subdirectory was the root directory of the repository.
- We cannot simply use the `<rev>:<path>` syntax without the `--revision`
+ We cannot simply use the `<rev>:<path>` syntax without the `--source`
flag, similar to how it is used in `git show` because any non-flag
parameter before `--` is treated as an attribute and any parameter after
`--` is treated as a pathname.
The change involves creating a new function `read_attr_from_blob`, which
- given the path reads the blob for the path against the provided revision and
+ given the path reads the blob for the path against the provided source and
parses the attributes line by line. This function is plugged into
`read_attr()` function wherein we go through the stack of attributes
files.
@@ Documentation/git-check-attr.txt: git-check-attr - Display gitattributes informa
[verse]
-'git check-attr' [-a | --all | <attr>...] [--] <pathname>...
-'git check-attr' --stdin [-z] [-a | --all | <attr>...]
-+'git check-attr' [--revision <revision>] [-a | --all | <attr>...] [--] <pathname>...
-+'git check-attr' --stdin [-z] [--revision <revision>] [-a | --all | <attr>...]
++'git check-attr' [--source <tree>] [-a | --all | <attr>...] [--] <pathname>...
++'git check-attr' --stdin [-z] [--source <tree>] [-a | --all | <attr>...]
DESCRIPTION
-----------
@@ Documentation/git-check-attr.txt: OPTIONS
If `--stdin` is also given, input paths are separated
with a NUL character instead of a linefeed character.
-+--revision=<revision>::
-+ Check attributes against the specified commit. All the attributes will
-+ be checked against the provided revision. Paths provided as part of the
-+ revision will be treated as the root directory.
++--source=<tree>::
++ Check attributes against the specified tree-ish. Paths provided as part
++ of the revision will be treated as the root directory. It is common to
++ specify the source tree by naming a commit, branch or tag associated
++ with it.
+
\--::
Interpret all preceding arguments as attributes and all following
@@ archive.c: static const struct attr_check *get_archive_attrs(struct index_state
## attr.c ##
@@
- #include "exec-cmd.h"
- #include "attr.h"
#include "dir.h"
-+#include "strbuf.h"
-+#include "tree-walk.h"
#include "utf8.h"
#include "quote.h"
+#include "revision.h"
@@ attr.c: void git_check_attr(struct index_state *istate,
const char *name = check->all_attrs[i].attr->name;
## attr.h ##
-@@
- #ifndef ATTR_H
- #define ATTR_H
-
-+#include "hash.h"
-+
- /**
- * gitattributes mechanism gives a uniform way to associate various attributes
- * to set of paths.
@@ attr.h: void attr_check_free(struct attr_check *check);
const char *git_attr_name(const struct git_attr *);
@@ attr.h: void attr_check_free(struct attr_check *check);
enum git_attr_direction {
## builtin/check-attr.c ##
-@@
-+#include "repository.h"
- #define USE_THE_INDEX_VARIABLE
- #include "builtin.h"
- #include "cache.h"
@@
static int all_attrs;
static int cached_attrs;
static int stdin_paths;
-+static char *revision;
++static char *source;
static const char * const check_attr_usage[] = {
-N_("git check-attr [-a | --all | <attr>...] [--] <pathname>..."),
-N_("git check-attr --stdin [-z] [-a | --all | <attr>...]"),
-+N_("git check-attr [--revision <revision>] [-a | --all | <attr>...] [--] <pathname>..."),
-+N_("git check-attr --stdin [-z] [--revision <revision>] [-a | --all | <attr>...]"),
++N_("git check-attr [--source <tree>] [-a | --all | <attr>...] [--] <pathname>..."),
++N_("git check-attr --stdin [-z] [--source <tree>] [-a | --all | <attr>...]"),
NULL
};
@@ builtin/check-attr.c: static const struct option check_attr_options[] = {
OPT_BOOL(0 , "stdin", &stdin_paths, N_("read file names from stdin")),
OPT_BOOL('z', NULL, &nul_term_line,
N_("terminate input and output records by a NUL character")),
-+ OPT_STRING(0, "revision", &revision, N_("revision"), N_("check attributes at this revision")),
++ OPT_STRING(0, "source", &source, N_("<tree-ish>"), N_("which tree-ish to check attributes at")),
OPT_END()
};
@@ builtin/check-attr.c: static NORETURN void error_with_usage(const char *msg)
{
struct attr_check *check;
+ struct object_id *tree_oid = NULL;
++ struct object_id initialized_oid;
int cnt, i, doubledash, filei;
if (!is_bare_repository())
@@ builtin/check-attr.c: int cmd_check_attr(int argc, const char **argv, const char
}
}
-+ if (revision) {
-+ tree_oid = xmalloc(sizeof(struct object_id));
-+
-+ if (repo_get_oid_tree(the_repository, revision, tree_oid))
-+ error("%s: not a valid revision", revision);
++ if (source) {
++ if (repo_get_oid_tree(the_repository, source, &initialized_oid))
++ die("%s: not a valid tree-ish source", source);
++ tree_oid = &initialized_oid;
+ }
+
if (stdin_paths)
@@ t/t0003-attributes.sh: attr_check_quote () {
test_cmp expect actual
+}
+
-+attr_check_revision () {
-+ path="$1" expect="$2" revision="$3" git_opts="$4" &&
++attr_check_source () {
++ path="$1" expect="$2" source="$3" git_opts="$4" &&
-+ git $git_opts check-attr --revision $revision test -- "$path" >actual 2>err &&
++ git $git_opts check-attr --source $source test -- "$path" >actual 2>err &&
+ echo "$path: test: $expect" >expect &&
+ test_cmp expect actual
++ test_must_be_empty err
}
test_expect_success 'open-quoted pathname' '
@@ t/t0003-attributes.sh: test_expect_success 'setup' '
'
+test_expect_success 'setup branches' '
-+ (
-+ echo "f test=f" &&
-+ echo "a/i test=n"
-+ ) | git hash-object -w --stdin >id &&
-+ git update-index --add --cacheinfo 100644,$(cat id),foo/bar/.gitattributes &&
-+ git write-tree >id &&
-+ tree_id=$(cat id) &&
-+ git commit-tree $tree_id -m "random commit message" >id &&
-+ commit_id=$(cat id) &&
-+ git update-ref refs/heads/branch1 $commit_id &&
++ mkdir -p foo/bar &&
++ test_commit --printf "add .gitattributes" foo/bar/.gitattribute \
++ "f test=f\na/i test=n\n" tag-1 &&
+
-+ (
-+ echo "g test=g" &&
-+ echo "a/i test=m"
-+ ) | git hash-object -w --stdin >id &&
-+ git update-index --add --cacheinfo 100644,$(cat id),foo/bar/.gitattributes &&
-+ git write-tree >id &&
-+ tree_id=$(cat id) &&
-+ git commit-tree $tree_id -m "random commit message" >id &&
-+ commit_id=$(cat id) &&
-+ git update-ref refs/heads/branch2 $commit_id
++ mkdir -p foo/bar &&
++ test_commit --printf "add .gitattributes" foo/bar/.gitattribute \
++ "g test=g\na/i test=m\n" tag-2
+'
+
test_expect_success 'command line checks' '
@@ t/t0003-attributes.sh: test_expect_success 'setup' '
test_must_fail git check-attr test &&
test_must_fail git check-attr test -- &&
test_must_fail git check-attr -- f &&
-+ test_must_fail git check-attr --revision &&
-+ test_must_fail git check-attr --revision not-a-valid-ref &&
++ test_must_fail git check-attr --source &&
++ test_must_fail git check-attr --source not-a-valid-ref &&
echo "f" | test_must_fail git check-attr --stdin &&
echo "f" | test_must_fail git check-attr --stdin -- f &&
echo "f" | test_must_fail git check-attr --stdin test -- f &&
@@ t/t0003-attributes.sh: test_expect_success 'using --git-dir and --work-tree' '
)
'
-+test_expect_success 'using --revision' '
-+ attr_check_revision foo/bar/f f branch1 &&
-+ attr_check_revision foo/bar/a/i n branch1 &&
-+ attr_check_revision foo/bar/f unspecified branch2 &&
-+ attr_check_revision foo/bar/a/i m branch2 &&
-+ attr_check_revision foo/bar/g g branch2 &&
-+ attr_check_revision foo/bar/g unspecified branch1
++test_expect_success 'using --source' '
++ attr_check_source foo/bar/f f tag-1 &&
++ attr_check_source foo/bar/a/i n tag-1 &&
++ attr_check_source foo/bar/f unspecified tag-2 &&
++ attr_check_source foo/bar/a/i m tag-2 &&
++ attr_check_source foo/bar/g g tag-2 &&
++ attr_check_source foo/bar/g unspecified tag-1
+'
+
test_expect_success 'setup bare' '
@@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr
)
'
-+test_expect_success 'bare repository: with --revision' '
++test_expect_success 'bare repository: with --source' '
+ (
+ cd bare.git &&
-+ (
-+ echo "f test=f" &&
-+ echo "a/i test=a/i"
-+ ) | git hash-object -w --stdin >id &&
-+ git update-index --add --cacheinfo 100644 $(cat id) .gitattributes &&
-+ git write-tree >id &&
-+ tree_id=$(cat id) &&
-+ git commit-tree $tree_id -m "random commit message" >id &&
-+ commit_id=$(cat id) &&
-+ git update-ref refs/heads/master $commit_id &&
-+ attr_check_revision f f HEAD &&
-+ attr_check_revision a/f f HEAD &&
-+ attr_check_revision a/c/f f HEAD &&
-+ attr_check_revision a/i a/i HEAD &&
-+ attr_check_revision subdir/a/i unspecified HEAD
++ attr_check_source foo/bar/f f tag-1 &&
++ attr_check_source foo/bar/a/i n tag-1 &&
++ attr_check_source foo/bar/f unspecified tag-2 &&
++ attr_check_source foo/bar/a/i m tag-2 &&
++ attr_check_source foo/bar/g g tag-2 &&
++ attr_check_source foo/bar/g unspecified tag-1
+ )
+'
+
Karthik Nayak (2):
t0003: move setup for `--all` into new block
attr: add flag `--source` to work with tree-ish
Documentation/git-check-attr.txt | 10 +++-
archive.c | 2 +-
attr.c | 97 +++++++++++++++++++++++---------
attr.h | 5 +-
builtin/check-attr.c | 35 +++++++-----
builtin/pack-objects.c | 2 +-
convert.c | 2 +-
ll-merge.c | 4 +-
pathspec.c | 2 +-
t/t0003-attributes.sh | 49 +++++++++++++++-
userdiff.c | 2 +-
ws.c | 2 +-
12 files changed, 157 insertions(+), 55 deletions(-)
--
2.39.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v5 1/2] t0003: move setup for `--all` into new block
2023-01-02 11:04 ` [PATCH v5 0/2] check-attr: add support to work with tree-ish Karthik Nayak
@ 2023-01-02 11:04 ` Karthik Nayak
2023-01-02 11:04 ` [PATCH v5 2/2] attr: add flag `--source` to work with tree-ish Karthik Nayak
2023-01-09 9:10 ` [PATCH v5 0/2] check-attr: add support " Karthik Nayak
2 siblings, 0 replies; 10+ messages in thread
From: Karthik Nayak @ 2023-01-02 11:04 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, toon
There is some setup code which is used by multiple tests being setup in
`attribute test: --all option`. This means when we run "sh
./t0003-attributes.sh --run=setup,<num>" there is a chance of failing
since we missed this setup block.
So to ensure that setups are independent of test logic, move this to a
new setup block.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
Co-authored-by: toon@iotcl.com
---
t/t0003-attributes.sh | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index f7ee2f2ff0..b3aabb8aa3 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -203,9 +203,12 @@ test_expect_success 'attribute test: read paths from stdin' '
test_cmp expect actual
'
-test_expect_success 'attribute test: --all option' '
+test_expect_success 'setup --all option' '
grep -v unspecified <expect-all | sort >specified-all &&
- sed -e "s/:.*//" <expect-all | uniq >stdin-all &&
+ sed -e "s/:.*//" <expect-all | uniq >stdin-all
+'
+
+test_expect_success 'attribute test: --all option' '
git check-attr --stdin --all <stdin-all >tmp &&
sort tmp >actual &&
test_cmp specified-all actual
--
2.39.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v5 2/2] attr: add flag `--source` to work with tree-ish
2023-01-02 11:04 ` [PATCH v5 0/2] check-attr: add support to work with tree-ish Karthik Nayak
2023-01-02 11:04 ` [PATCH v5 1/2] t0003: move setup for `--all` into new block Karthik Nayak
@ 2023-01-02 11:04 ` Karthik Nayak
2023-01-02 23:38 ` Junio C Hamano
` (2 more replies)
2023-01-09 9:10 ` [PATCH v5 0/2] check-attr: add support " Karthik Nayak
2 siblings, 3 replies; 10+ messages in thread
From: Karthik Nayak @ 2023-01-02 11:04 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, Toon Claes
The contents of the .gitattributes files may evolve over time, but "git
check-attr" always checks attributes against them in the working tree
and/or in the index. It may be beneficial to optionally allow the users
to check attributes taken from a commit other than HEAD against paths.
Add a new flag `--source` which will allow users to check the
attributes against a commit (actually any tree-ish would do). When the
user uses this flag, we go through the stack of .gitattributes files but
instead of checking the current working tree and/or in the index, we
check the blobs from the provided tree-ish object. This allows the
command to also be used in bare repositories.
Since we use a tree-ish object, the user can pass "--source
HEAD:subdirectory" and all the attributes will be looked up as if
subdirectory was the root directory of the repository.
We cannot simply use the `<rev>:<path>` syntax without the `--source`
flag, similar to how it is used in `git show` because any non-flag
parameter before `--` is treated as an attribute and any parameter after
`--` is treated as a pathname.
The change involves creating a new function `read_attr_from_blob`, which
given the path reads the blob for the path against the provided source and
parses the attributes line by line. This function is plugged into
`read_attr()` function wherein we go through the stack of attributes
files.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: Toon Claes <toon@iotcl.com>
Co-authored-by: toon@iotcl.com
---
Documentation/git-check-attr.txt | 10 +++-
archive.c | 2 +-
attr.c | 97 +++++++++++++++++++++++---------
attr.h | 5 +-
builtin/check-attr.c | 35 +++++++-----
builtin/pack-objects.c | 2 +-
convert.c | 2 +-
ll-merge.c | 4 +-
pathspec.c | 2 +-
t/t0003-attributes.sh | 42 +++++++++++++-
userdiff.c | 2 +-
ws.c | 2 +-
12 files changed, 152 insertions(+), 53 deletions(-)
diff --git a/Documentation/git-check-attr.txt b/Documentation/git-check-attr.txt
index 84f41a8e82..fb15c44598 100644
--- a/Documentation/git-check-attr.txt
+++ b/Documentation/git-check-attr.txt
@@ -9,8 +9,8 @@ git-check-attr - Display gitattributes information
SYNOPSIS
--------
[verse]
-'git check-attr' [-a | --all | <attr>...] [--] <pathname>...
-'git check-attr' --stdin [-z] [-a | --all | <attr>...]
+'git check-attr' [--source <tree>] [-a | --all | <attr>...] [--] <pathname>...
+'git check-attr' --stdin [-z] [--source <tree>] [-a | --all | <attr>...]
DESCRIPTION
-----------
@@ -36,6 +36,12 @@ OPTIONS
If `--stdin` is also given, input paths are separated
with a NUL character instead of a linefeed character.
+--source=<tree>::
+ Check attributes against the specified tree-ish. Paths provided as part
+ of the revision will be treated as the root directory. It is common to
+ specify the source tree by naming a commit, branch or tag associated
+ with it.
+
\--::
Interpret all preceding arguments as attributes and all following
arguments as path names.
diff --git a/archive.c b/archive.c
index 941495f5d7..81ff76fce9 100644
--- a/archive.c
+++ b/archive.c
@@ -120,7 +120,7 @@ static const struct attr_check *get_archive_attrs(struct index_state *istate,
static struct attr_check *check;
if (!check)
check = attr_check_initl("export-ignore", "export-subst", NULL);
- git_check_attr(istate, path, check);
+ git_check_attr(istate, NULL, path, check);
return check;
}
diff --git a/attr.c b/attr.c
index 42ad6de8c7..a63f267187 100644
--- a/attr.c
+++ b/attr.c
@@ -13,6 +13,8 @@
#include "dir.h"
#include "utf8.h"
#include "quote.h"
+#include "revision.h"
+#include "object-store.h"
#include "thread-utils.h"
const char git_attr__true[] = "(builtin)true";
@@ -729,14 +731,62 @@ static struct attr_stack *read_attr_from_file(const char *path, unsigned flags)
return res;
}
-static struct attr_stack *read_attr_from_index(struct index_state *istate,
- const char *path,
- unsigned flags)
+static struct attr_stack *read_attr_from_buf(char *buf, const char *path,
+ unsigned flags)
{
struct attr_stack *res;
- char *buf, *sp;
+ char *sp;
int lineno = 0;
+ if (!buf)
+ return NULL;
+
+ CALLOC_ARRAY(res, 1);
+ for (sp = buf; *sp;) {
+ char *ep;
+ int more;
+
+ ep = strchrnul(sp, '\n');
+ more = (*ep == '\n');
+ *ep = '\0';
+ handle_attr_line(res, sp, path, ++lineno, flags);
+ sp = ep + more;
+ }
+ free(buf);
+
+ return res;
+}
+
+static struct attr_stack *read_attr_from_blob(struct index_state *istate,
+ const struct object_id *tree_oid,
+ const char *path, unsigned flags)
+{
+ struct object_id oid;
+ unsigned long sz;
+ enum object_type type;
+ void *buf;
+ unsigned short mode;
+
+ if (!tree_oid)
+ return NULL;
+
+ if (get_tree_entry(istate->repo, tree_oid, path, &oid, &mode))
+ return NULL;
+
+ buf = read_object_file(&oid, &type, &sz);
+ if (!buf || type != OBJ_BLOB) {
+ free(buf);
+ return NULL;
+ }
+
+ return read_attr_from_buf(buf, path, flags);
+}
+
+static struct attr_stack *read_attr_from_index(struct index_state *istate,
+ const char *path, unsigned flags)
+{
+ char *buf;
+
if (!istate)
return NULL;
@@ -758,28 +808,19 @@ static struct attr_stack *read_attr_from_index(struct index_state *istate,
if (!buf)
return NULL;
- CALLOC_ARRAY(res, 1);
- for (sp = buf; *sp; ) {
- char *ep;
- int more;
-
- ep = strchrnul(sp, '\n');
- more = (*ep == '\n');
- *ep = '\0';
- handle_attr_line(res, sp, path, ++lineno, flags);
- sp = ep + more;
- }
- free(buf);
- return res;
+ return read_attr_from_buf(buf, path, flags);
}
static struct attr_stack *read_attr(struct index_state *istate,
+ const struct object_id *tree_oid,
const char *path, unsigned flags)
{
struct attr_stack *res = NULL;
if (direction == GIT_ATTR_INDEX) {
res = read_attr_from_index(istate, path, flags);
+ } else if (tree_oid) {
+ res = read_attr_from_blob(istate, tree_oid, path, flags);
} else if (!is_bare_repository()) {
if (direction == GIT_ATTR_CHECKOUT) {
res = read_attr_from_index(istate, path, flags);
@@ -839,6 +880,7 @@ static void push_stack(struct attr_stack **attr_stack_p,
}
static void bootstrap_attr_stack(struct index_state *istate,
+ const struct object_id *tree_oid,
struct attr_stack **stack)
{
struct attr_stack *e;
@@ -864,7 +906,7 @@ static void bootstrap_attr_stack(struct index_state *istate,
}
/* root directory */
- e = read_attr(istate, GITATTRIBUTES_FILE, flags | READ_ATTR_NOFOLLOW);
+ e = read_attr(istate, tree_oid, GITATTRIBUTES_FILE, flags | READ_ATTR_NOFOLLOW);
push_stack(stack, e, xstrdup(""), 0);
/* info frame */
@@ -878,6 +920,7 @@ static void bootstrap_attr_stack(struct index_state *istate,
}
static void prepare_attr_stack(struct index_state *istate,
+ const struct object_id *tree_oid,
const char *path, int dirlen,
struct attr_stack **stack)
{
@@ -899,7 +942,7 @@ static void prepare_attr_stack(struct index_state *istate,
* .gitattributes in deeper directories to shallower ones,
* and finally use the built-in set as the default.
*/
- bootstrap_attr_stack(istate, stack);
+ bootstrap_attr_stack(istate, tree_oid, stack);
/*
* Pop the "info" one that is always at the top of the stack.
@@ -954,7 +997,7 @@ static void prepare_attr_stack(struct index_state *istate,
strbuf_add(&pathbuf, path + pathbuf.len, (len - pathbuf.len));
strbuf_addf(&pathbuf, "/%s", GITATTRIBUTES_FILE);
- next = read_attr(istate, pathbuf.buf, READ_ATTR_NOFOLLOW);
+ next = read_attr(istate, tree_oid, pathbuf.buf, READ_ATTR_NOFOLLOW);
/* reset the pathbuf to not include "/.gitattributes" */
strbuf_setlen(&pathbuf, len);
@@ -1074,8 +1117,8 @@ static void determine_macros(struct all_attrs_item *all_attrs,
* Otherwise all attributes are collected.
*/
static void collect_some_attrs(struct index_state *istate,
- const char *path,
- struct attr_check *check)
+ const struct object_id *tree_oid,
+ const char *path, struct attr_check *check)
{
int pathlen, rem, dirlen;
const char *cp, *last_slash = NULL;
@@ -1094,7 +1137,7 @@ static void collect_some_attrs(struct index_state *istate,
dirlen = 0;
}
- prepare_attr_stack(istate, path, dirlen, &check->stack);
+ prepare_attr_stack(istate, tree_oid, path, dirlen, &check->stack);
all_attrs_init(&g_attr_hashmap, check);
determine_macros(check->all_attrs, check->stack);
@@ -1103,12 +1146,12 @@ static void collect_some_attrs(struct index_state *istate,
}
void git_check_attr(struct index_state *istate,
- const char *path,
+ const struct object_id *tree_oid, const char *path,
struct attr_check *check)
{
int i;
- collect_some_attrs(istate, path, check);
+ collect_some_attrs(istate, tree_oid, path, check);
for (i = 0; i < check->nr; i++) {
size_t n = check->items[i].attr->attr_nr;
@@ -1119,13 +1162,13 @@ void git_check_attr(struct index_state *istate,
}
}
-void git_all_attrs(struct index_state *istate,
+void git_all_attrs(struct index_state *istate, const struct object_id *tree_oid,
const char *path, struct attr_check *check)
{
int i;
attr_check_reset(check);
- collect_some_attrs(istate, path, check);
+ collect_some_attrs(istate, tree_oid, path, check);
for (i = 0; i < check->all_attrs_nr; i++) {
const char *name = check->all_attrs[i].attr->name;
diff --git a/attr.h b/attr.h
index 3fb40cced0..84a3279215 100644
--- a/attr.h
+++ b/attr.h
@@ -190,13 +190,14 @@ void attr_check_free(struct attr_check *check);
const char *git_attr_name(const struct git_attr *);
void git_check_attr(struct index_state *istate,
- const char *path, struct attr_check *check);
+ const struct object_id *tree_oid, const char *path,
+ struct attr_check *check);
/*
* Retrieve all attributes that apply to the specified path.
* check holds the attributes and their values.
*/
-void git_all_attrs(struct index_state *istate,
+void git_all_attrs(struct index_state *istate, const struct object_id *tree_oid,
const char *path, struct attr_check *check);
enum git_attr_direction {
diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 0fef10eb6b..4ffef29d9c 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -9,9 +9,10 @@
static int all_attrs;
static int cached_attrs;
static int stdin_paths;
+static char *source;
static const char * const check_attr_usage[] = {
-N_("git check-attr [-a | --all | <attr>...] [--] <pathname>..."),
-N_("git check-attr --stdin [-z] [-a | --all | <attr>...]"),
+N_("git check-attr [--source <tree>] [-a | --all | <attr>...] [--] <pathname>..."),
+N_("git check-attr --stdin [-z] [--source <tree>] [-a | --all | <attr>...]"),
NULL
};
@@ -23,6 +24,7 @@ static const struct option check_attr_options[] = {
OPT_BOOL(0 , "stdin", &stdin_paths, N_("read file names from stdin")),
OPT_BOOL('z', NULL, &nul_term_line,
N_("terminate input and output records by a NUL character")),
+ OPT_STRING(0, "source", &source, N_("<tree-ish>"), N_("which tree-ish to check attributes at")),
OPT_END()
};
@@ -55,27 +57,26 @@ static void output_attr(struct attr_check *check, const char *file)
}
}
-static void check_attr(const char *prefix,
- struct attr_check *check,
- int collect_all,
+static void check_attr(const char *prefix, struct attr_check *check,
+ const struct object_id *tree_oid, int collect_all,
const char *file)
+
{
char *full_path =
prefix_path(prefix, prefix ? strlen(prefix) : 0, file);
if (collect_all) {
- git_all_attrs(&the_index, full_path, check);
+ git_all_attrs(&the_index, tree_oid, full_path, check);
} else {
- git_check_attr(&the_index, full_path, check);
+ git_check_attr(&the_index, tree_oid, full_path, check);
}
output_attr(check, file);
free(full_path);
}
-static void check_attr_stdin_paths(const char *prefix,
- struct attr_check *check,
- int collect_all)
+static void check_attr_stdin_paths(const char *prefix, struct attr_check *check,
+ const struct object_id *tree_oid, int collect_all)
{
struct strbuf buf = STRBUF_INIT;
struct strbuf unquoted = STRBUF_INIT;
@@ -89,7 +90,7 @@ static void check_attr_stdin_paths(const char *prefix,
die("line is badly quoted");
strbuf_swap(&buf, &unquoted);
}
- check_attr(prefix, check, collect_all, buf.buf);
+ check_attr(prefix, check, tree_oid, collect_all, buf.buf);
maybe_flush_or_die(stdout, "attribute to stdout");
}
strbuf_release(&buf);
@@ -105,6 +106,8 @@ static NORETURN void error_with_usage(const char *msg)
int cmd_check_attr(int argc, const char **argv, const char *prefix)
{
struct attr_check *check;
+ struct object_id *tree_oid = NULL;
+ struct object_id initialized_oid;
int cnt, i, doubledash, filei;
if (!is_bare_repository())
@@ -176,11 +179,17 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)
}
}
+ if (source) {
+ if (repo_get_oid_tree(the_repository, source, &initialized_oid))
+ die("%s: not a valid revision", source);
+ tree_oid = &initialized_oid;
+ }
+
if (stdin_paths)
- check_attr_stdin_paths(prefix, check, all_attrs);
+ check_attr_stdin_paths(prefix, check, tree_oid, all_attrs);
else {
for (i = filei; i < argc; i++)
- check_attr(prefix, check, all_attrs, argv[i]);
+ check_attr(prefix, check, tree_oid, all_attrs, argv[i]);
maybe_flush_or_die(stdout, "attribute to stdout");
}
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 573d0b20b7..89535cfa6a 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1318,7 +1318,7 @@ static int no_try_delta(const char *path)
if (!check)
check = attr_check_initl("delta", NULL);
- git_check_attr(the_repository->index, path, check);
+ git_check_attr(the_repository->index, NULL, path, check);
if (ATTR_FALSE(check->items[0].value))
return 1;
return 0;
diff --git a/convert.c b/convert.c
index 9b67649032..a54d1690c0 100644
--- a/convert.c
+++ b/convert.c
@@ -1308,7 +1308,7 @@ void convert_attrs(struct index_state *istate,
git_config(read_convert_config, NULL);
}
- git_check_attr(istate, path, check);
+ git_check_attr(istate, NULL, path, check);
ccheck = check->items;
ca->crlf_action = git_path_check_crlf(ccheck + 4);
if (ca->crlf_action == CRLF_UNDEFINED)
diff --git a/ll-merge.c b/ll-merge.c
index 22a603e8af..130d26501c 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -391,7 +391,7 @@ enum ll_merge_result ll_merge(mmbuffer_t *result_buf,
normalize_file(theirs, path, istate);
}
- git_check_attr(istate, path, check);
+ git_check_attr(istate, NULL, path, check);
ll_driver_name = check->items[0].value;
if (check->items[1].value) {
marker_size = atoi(check->items[1].value);
@@ -419,7 +419,7 @@ int ll_merge_marker_size(struct index_state *istate, const char *path)
if (!check)
check = attr_check_initl("conflict-marker-size", NULL);
- git_check_attr(istate, path, check);
+ git_check_attr(istate, NULL, path, check);
if (check->items[0].value) {
marker_size = atoi(check->items[0].value);
if (marker_size <= 0)
diff --git a/pathspec.c b/pathspec.c
index 46e77a85fe..48dec2c709 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -732,7 +732,7 @@ int match_pathspec_attrs(struct index_state *istate,
if (name[namelen])
name = to_free = xmemdupz(name, namelen);
- git_check_attr(istate, name, item->attr_check);
+ git_check_attr(istate, NULL, name, item->attr_check);
free(to_free);
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index b3aabb8aa3..78e9f47dbf 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -25,7 +25,15 @@ attr_check_quote () {
git check-attr test -- "$path" >actual &&
echo "\"$quoted_path\": test: $expect" >expect &&
test_cmp expect actual
+}
+
+attr_check_source () {
+ path="$1" expect="$2" source="$3" git_opts="$4" &&
+ git $git_opts check-attr --source $source test -- "$path" >actual 2>err &&
+ echo "$path: test: $expect" >expect &&
+ test_cmp expect actual
+ test_must_be_empty err
}
test_expect_success 'open-quoted pathname' '
@@ -33,7 +41,6 @@ test_expect_success 'open-quoted pathname' '
attr_check a unspecified
'
-
test_expect_success 'setup' '
mkdir -p a/b/d a/c b &&
(
@@ -80,12 +87,24 @@ test_expect_success 'setup' '
EOF
'
+test_expect_success 'setup branches' '
+ mkdir -p foo/bar &&
+ test_commit --printf "add .gitattributes" foo/bar/.gitattribute \
+ "f test=f\na/i test=n\n" tag-1 &&
+
+ mkdir -p foo/bar &&
+ test_commit --printf "add .gitattributes" foo/bar/.gitattribute \
+ "g test=g\na/i test=m\n" tag-2
+'
+
test_expect_success 'command line checks' '
test_must_fail git check-attr &&
test_must_fail git check-attr -- &&
test_must_fail git check-attr test &&
test_must_fail git check-attr test -- &&
test_must_fail git check-attr -- f &&
+ test_must_fail git check-attr --source &&
+ test_must_fail git check-attr --source not-a-valid-ref &&
echo "f" | test_must_fail git check-attr --stdin &&
echo "f" | test_must_fail git check-attr --stdin -- f &&
echo "f" | test_must_fail git check-attr --stdin test -- f &&
@@ -287,6 +306,15 @@ test_expect_success 'using --git-dir and --work-tree' '
)
'
+test_expect_success 'using --source' '
+ attr_check_source foo/bar/f f tag-1 &&
+ attr_check_source foo/bar/a/i n tag-1 &&
+ attr_check_source foo/bar/f unspecified tag-2 &&
+ attr_check_source foo/bar/a/i m tag-2 &&
+ attr_check_source foo/bar/g g tag-2 &&
+ attr_check_source foo/bar/g unspecified tag-1
+'
+
test_expect_success 'setup bare' '
git clone --template= --bare . bare.git
'
@@ -306,6 +334,18 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' '
)
'
+test_expect_success 'bare repository: with --source' '
+ (
+ cd bare.git &&
+ attr_check_source foo/bar/f f tag-1 &&
+ attr_check_source foo/bar/a/i n tag-1 &&
+ attr_check_source foo/bar/f unspecified tag-2 &&
+ attr_check_source foo/bar/a/i m tag-2 &&
+ attr_check_source foo/bar/g g tag-2 &&
+ attr_check_source foo/bar/g unspecified tag-1
+ )
+'
+
test_expect_success 'bare repository: check that --cached honors index' '
(
cd bare.git &&
diff --git a/userdiff.c b/userdiff.c
index 151d9a5278..b66f090a0b 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -412,7 +412,7 @@ struct userdiff_driver *userdiff_find_by_path(struct index_state *istate,
check = attr_check_initl("diff", NULL);
if (!path)
return NULL;
- git_check_attr(istate, path, check);
+ git_check_attr(istate, NULL, path, check);
if (ATTR_TRUE(check->items[0].value))
return &driver_true;
diff --git a/ws.c b/ws.c
index 6e69877f25..eadbbe5667 100644
--- a/ws.c
+++ b/ws.c
@@ -78,7 +78,7 @@ unsigned whitespace_rule(struct index_state *istate, const char *pathname)
if (!attr_whitespace_rule)
attr_whitespace_rule = attr_check_initl("whitespace", NULL);
- git_check_attr(istate, pathname, attr_whitespace_rule);
+ git_check_attr(istate, NULL, pathname, attr_whitespace_rule);
value = attr_whitespace_rule->items[0].value;
if (ATTR_TRUE(value)) {
/* true (whitespace) */
--
2.39.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v5 2/2] attr: add flag `--source` to work with tree-ish
2023-01-02 11:04 ` [PATCH v5 2/2] attr: add flag `--source` to work with tree-ish Karthik Nayak
@ 2023-01-02 23:38 ` Junio C Hamano
2023-01-04 10:25 ` Karthik Nayak
2023-01-11 11:30 ` Phillip Wood
2023-01-12 13:20 ` Ævar Arnfjörð Bjarmason
2 siblings, 1 reply; 10+ messages in thread
From: Junio C Hamano @ 2023-01-02 23:38 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, Toon Claes
Karthik Nayak <karthik.188@gmail.com> writes:
> The contents of the .gitattributes files may evolve over time, but "git
> check-attr" always checks attributes against them in the working tree
> and/or in the index. It may be beneficial to optionally allow the users
> to check attributes taken from a commit other than HEAD against paths.
>
> Add a new flag `--source` which will allow users to check the
> attributes against a commit (actually any tree-ish would do). When the
> user uses this flag, we go through the stack of .gitattributes files but
> instead of checking the current working tree and/or in the index, we
> check the blobs from the provided tree-ish object. This allows the
> command to also be used in bare repositories.
>
> Since we use a tree-ish object, the user can pass "--source
> HEAD:subdirectory" and all the attributes will be looked up as if
> subdirectory was the root directory of the repository.
>
> We cannot simply use the `<rev>:<path>` syntax without the `--source`
> flag, similar to how it is used in `git show` because any non-flag
> parameter before `--` is treated as an attribute and any parameter after
> `--` is treated as a pathname.
>
> The change involves creating a new function `read_attr_from_blob`, which
> given the path reads the blob for the path against the provided source and
> parses the attributes line by line. This function is plugged into
> `read_attr()` function wherein we go through the stack of attributes
> files.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> Signed-off-by: Toon Claes <toon@iotcl.com>
> Co-authored-by: toon@iotcl.com
> ---
> Documentation/git-check-attr.txt | 10 +++-
> archive.c | 2 +-
> attr.c | 97 +++++++++++++++++++++++---------
> attr.h | 5 +-
> builtin/check-attr.c | 35 +++++++-----
> builtin/pack-objects.c | 2 +-
> convert.c | 2 +-
> ll-merge.c | 4 +-
> pathspec.c | 2 +-
> t/t0003-attributes.sh | 42 +++++++++++++-
> userdiff.c | 2 +-
> ws.c | 2 +-
> 12 files changed, 152 insertions(+), 53 deletions(-)
>
> diff --git a/Documentation/git-check-attr.txt b/Documentation/git-check-attr.txt
> index 84f41a8e82..fb15c44598 100644
> --- a/Documentation/git-check-attr.txt
> +++ b/Documentation/git-check-attr.txt
> @@ -9,8 +9,8 @@ git-check-attr - Display gitattributes information
> SYNOPSIS
> --------
> [verse]
> -'git check-attr' [-a | --all | <attr>...] [--] <pathname>...
> -'git check-attr' --stdin [-z] [-a | --all | <attr>...]
> +'git check-attr' [--source <tree>] [-a | --all | <attr>...] [--] <pathname>...
> +'git check-attr' --stdin [-z] [--source <tree>] [-a | --all | <attr>...]
>
> DESCRIPTION
> -----------
> @@ -36,6 +36,12 @@ OPTIONS
> If `--stdin` is also given, input paths are separated
> with a NUL character instead of a linefeed character.
>
> +--source=<tree>::
> + Check attributes against the specified tree-ish. Paths provided as part
> + of the revision will be treated as the root directory. It is common to
> + specify the source tree by naming a commit, branch or tag associated
> + with it.
> +
> \--::
> Interpret all preceding arguments as attributes and all following
> arguments as path names.
> diff --git a/archive.c b/archive.c
> index 941495f5d7..81ff76fce9 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -120,7 +120,7 @@ static const struct attr_check *get_archive_attrs(struct index_state *istate,
> static struct attr_check *check;
> if (!check)
> check = attr_check_initl("export-ignore", "export-subst", NULL);
> - git_check_attr(istate, path, check);
> + git_check_attr(istate, NULL, path, check);
> return check;
> }
>
> diff --git a/attr.c b/attr.c
> index 42ad6de8c7..a63f267187 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -13,6 +13,8 @@
> #include "dir.h"
> #include "utf8.h"
> #include "quote.h"
> +#include "revision.h"
> +#include "object-store.h"
> #include "thread-utils.h"
>
> const char git_attr__true[] = "(builtin)true";
> @@ -729,14 +731,62 @@ static struct attr_stack *read_attr_from_file(const char *path, unsigned flags)
> return res;
> }
>
> -static struct attr_stack *read_attr_from_index(struct index_state *istate,
> - const char *path,
> - unsigned flags)
> +static struct attr_stack *read_attr_from_buf(char *buf, const char *path,
> + unsigned flags)
> {
> struct attr_stack *res;
> - char *buf, *sp;
> + char *sp;
> int lineno = 0;
>
> + if (!buf)
> + return NULL;
> +
> + CALLOC_ARRAY(res, 1);
> + for (sp = buf; *sp;) {
> + char *ep;
> + int more;
> +
> + ep = strchrnul(sp, '\n');
> + more = (*ep == '\n');
> + *ep = '\0';
> + handle_attr_line(res, sp, path, ++lineno, flags);
> + sp = ep + more;
> + }
> + free(buf);
> +
> + return res;
> +}
> +
> +static struct attr_stack *read_attr_from_blob(struct index_state *istate,
> + const struct object_id *tree_oid,
> + const char *path, unsigned flags)
> +{
> + struct object_id oid;
> + unsigned long sz;
> + enum object_type type;
> + void *buf;
> + unsigned short mode;
> +
> + if (!tree_oid)
> + return NULL;
> +
> + if (get_tree_entry(istate->repo, tree_oid, path, &oid, &mode))
> + return NULL;
> +
> + buf = read_object_file(&oid, &type, &sz);
> + if (!buf || type != OBJ_BLOB) {
> + free(buf);
> + return NULL;
> + }
> +
> + return read_attr_from_buf(buf, path, flags);
> +}
> +
> +static struct attr_stack *read_attr_from_index(struct index_state *istate,
> + const char *path, unsigned flags)
> +{
> + char *buf;
> +
> if (!istate)
> return NULL;
>
> @@ -758,28 +808,19 @@ static struct attr_stack *read_attr_from_index(struct index_state *istate,
> if (!buf)
> return NULL;
>
> - CALLOC_ARRAY(res, 1);
> - for (sp = buf; *sp; ) {
> - char *ep;
> - int more;
> -
> - ep = strchrnul(sp, '\n');
> - more = (*ep == '\n');
> - *ep = '\0';
> - handle_attr_line(res, sp, path, ++lineno, flags);
> - sp = ep + more;
> - }
> - free(buf);
> - return res;
> + return read_attr_from_buf(buf, path, flags);
> }
>
> static struct attr_stack *read_attr(struct index_state *istate,
> + const struct object_id *tree_oid,
> const char *path, unsigned flags)
> {
> struct attr_stack *res = NULL;
>
> if (direction == GIT_ATTR_INDEX) {
> res = read_attr_from_index(istate, path, flags);
> + } else if (tree_oid) {
> + res = read_attr_from_blob(istate, tree_oid, path, flags);
> } else if (!is_bare_repository()) {
> if (direction == GIT_ATTR_CHECKOUT) {
> res = read_attr_from_index(istate, path, flags);
> @@ -839,6 +880,7 @@ static void push_stack(struct attr_stack **attr_stack_p,
> }
>
> static void bootstrap_attr_stack(struct index_state *istate,
> + const struct object_id *tree_oid,
> struct attr_stack **stack)
> {
> struct attr_stack *e;
> @@ -864,7 +906,7 @@ static void bootstrap_attr_stack(struct index_state *istate,
> }
>
> /* root directory */
> - e = read_attr(istate, GITATTRIBUTES_FILE, flags | READ_ATTR_NOFOLLOW);
> + e = read_attr(istate, tree_oid, GITATTRIBUTES_FILE, flags | READ_ATTR_NOFOLLOW);
> push_stack(stack, e, xstrdup(""), 0);
>
> /* info frame */
> @@ -878,6 +920,7 @@ static void bootstrap_attr_stack(struct index_state *istate,
> }
>
> static void prepare_attr_stack(struct index_state *istate,
> + const struct object_id *tree_oid,
> const char *path, int dirlen,
> struct attr_stack **stack)
> {
> @@ -899,7 +942,7 @@ static void prepare_attr_stack(struct index_state *istate,
> * .gitattributes in deeper directories to shallower ones,
> * and finally use the built-in set as the default.
> */
> - bootstrap_attr_stack(istate, stack);
> + bootstrap_attr_stack(istate, tree_oid, stack);
>
> /*
> * Pop the "info" one that is always at the top of the stack.
> @@ -954,7 +997,7 @@ static void prepare_attr_stack(struct index_state *istate,
> strbuf_add(&pathbuf, path + pathbuf.len, (len - pathbuf.len));
> strbuf_addf(&pathbuf, "/%s", GITATTRIBUTES_FILE);
>
> - next = read_attr(istate, pathbuf.buf, READ_ATTR_NOFOLLOW);
> + next = read_attr(istate, tree_oid, pathbuf.buf, READ_ATTR_NOFOLLOW);
>
> /* reset the pathbuf to not include "/.gitattributes" */
> strbuf_setlen(&pathbuf, len);
> @@ -1074,8 +1117,8 @@ static void determine_macros(struct all_attrs_item *all_attrs,
> * Otherwise all attributes are collected.
> */
> static void collect_some_attrs(struct index_state *istate,
> - const char *path,
> - struct attr_check *check)
> + const struct object_id *tree_oid,
> + const char *path, struct attr_check *check)
> {
> int pathlen, rem, dirlen;
> const char *cp, *last_slash = NULL;
> @@ -1094,7 +1137,7 @@ static void collect_some_attrs(struct index_state *istate,
> dirlen = 0;
> }
>
> - prepare_attr_stack(istate, path, dirlen, &check->stack);
> + prepare_attr_stack(istate, tree_oid, path, dirlen, &check->stack);
> all_attrs_init(&g_attr_hashmap, check);
> determine_macros(check->all_attrs, check->stack);
>
> @@ -1103,12 +1146,12 @@ static void collect_some_attrs(struct index_state *istate,
> }
>
> void git_check_attr(struct index_state *istate,
> - const char *path,
> + const struct object_id *tree_oid, const char *path,
> struct attr_check *check)
> {
> int i;
>
> - collect_some_attrs(istate, path, check);
> + collect_some_attrs(istate, tree_oid, path, check);
>
> for (i = 0; i < check->nr; i++) {
> size_t n = check->items[i].attr->attr_nr;
> @@ -1119,13 +1162,13 @@ void git_check_attr(struct index_state *istate,
> }
> }
>
> -void git_all_attrs(struct index_state *istate,
> +void git_all_attrs(struct index_state *istate, const struct object_id *tree_oid,
> const char *path, struct attr_check *check)
> {
> int i;
>
> attr_check_reset(check);
> - collect_some_attrs(istate, path, check);
> + collect_some_attrs(istate, tree_oid, path, check);
>
> for (i = 0; i < check->all_attrs_nr; i++) {
> const char *name = check->all_attrs[i].attr->name;
> diff --git a/attr.h b/attr.h
> index 3fb40cced0..84a3279215 100644
> --- a/attr.h
> +++ b/attr.h
> @@ -190,13 +190,14 @@ void attr_check_free(struct attr_check *check);
> const char *git_attr_name(const struct git_attr *);
>
> void git_check_attr(struct index_state *istate,
> - const char *path, struct attr_check *check);
> + const struct object_id *tree_oid, const char *path,
> + struct attr_check *check);
FYI, this breaks "make hdr-check".
> /*
> * Retrieve all attributes that apply to the specified path.
> * check holds the attributes and their values.
> */
> -void git_all_attrs(struct index_state *istate,
> +void git_all_attrs(struct index_state *istate, const struct object_id *tree_oid,
> const char *path, struct attr_check *check);
Likewise.
Perhaps squash something like this in in the next iteration?
attr.h | 1 +
1 file changed, 1 insertion(+)
diff --git i/attr.h w/attr.h
index 84a3279215..fca6c30430 100644
--- i/attr.h
+++ w/attr.h
@@ -108,6 +108,7 @@
*/
struct index_state;
+struct object_id;
/**
* An attribute is an opaque object that is identified by its name. Pass the
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v5 2/2] attr: add flag `--source` to work with tree-ish
2023-01-02 23:38 ` Junio C Hamano
@ 2023-01-04 10:25 ` Karthik Nayak
0 siblings, 0 replies; 10+ messages in thread
From: Karthik Nayak @ 2023-01-04 10:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Toon Claes
On Tue, Jan 3, 2023 at 12:38 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Likewise.
>
> Perhaps squash something like this in in the next iteration?
>
> attr.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git i/attr.h w/attr.h
> index 84a3279215..fca6c30430 100644
> --- i/attr.h
> +++ w/attr.h
> @@ -108,6 +108,7 @@
> */
>
> struct index_state;
> +struct object_id;
>
> /**
> * An attribute is an opaque object that is identified by its name. Pass the
Thanks Junio, I'll squash this in, waiting for more reviews before
rolling out a new version.
--
- Karthik
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 0/2] check-attr: add support to work with tree-ish
2023-01-02 11:04 ` [PATCH v5 0/2] check-attr: add support to work with tree-ish Karthik Nayak
2023-01-02 11:04 ` [PATCH v5 1/2] t0003: move setup for `--all` into new block Karthik Nayak
2023-01-02 11:04 ` [PATCH v5 2/2] attr: add flag `--source` to work with tree-ish Karthik Nayak
@ 2023-01-09 9:10 ` Karthik Nayak
2 siblings, 0 replies; 10+ messages in thread
From: Karthik Nayak @ 2023-01-09 9:10 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, phillip.wood
Bumping for reviews, before I re-roll with the fix for `make hdr-check`
- Karthik
On Mon, Jan 2, 2023 at 12:04 PM Karthik Nayak <karthik.188@gmail.com> wrote:
>
> v1: https://lore.kernel.org/git/20221206103736.53909-1-karthik.188@gmail.com/
> v2: https://lore.kernel.org/git/CAOLa=ZSsFGBw3ta1jWN8cmUch2ca=zTEjp1xMA6Linafx9W53g@mail.gmail.com/T/#t
> v3: https://lore.kernel.org/git/20221216093552.3171319-1-karthik.188@gmail.com/
> v4: https://lore.kernel.org/git/cover.1671630304.git.karthik.188@gmail.com
>
> Given a pathname, git-check-attr(1) will list the attributes which apply to that
> pathname by reading all relevant gitattributes files. Currently there is no way
> to specify a tree-ish to read the gitattributes from.
>
> This is specifically useful in bare repositories wherein the gitattributes are
> only present in the git working tree but not available directly on the
> filesystem.
>
> This series aims to add a new flag `--source` to git-check-attr(1) which
> allows us to read gitattributes from the specified tree-ish.
>
> Changes since version 4:
> - Changed the flag from `--revision` to `--source`
> - Removed uneeded header imports
> - Using a pre-initialized variable instead of malloc for the tree_oid
> - Using `die()` instead of `error()` for bad tree-ish provided
>
> Range-diff against v4:
>
> 1: 6224754179 = 1: 6224754179 t0003: move setup for `--all` into new block
> 2: a161dbdf8b ! 2: d835d989ad attr: add flag `--revision` to work with revisions
> @@ Metadata
> Author: Karthik Nayak <karthik.188@gmail.com>
>
> ## Commit message ##
> - attr: add flag `--revision` to work with revisions
> + attr: add flag `--source` to work with tree-ish
>
> The contents of the .gitattributes files may evolve over time, but "git
> check-attr" always checks attributes against them in the working tree
> and/or in the index. It may be beneficial to optionally allow the users
> to check attributes taken from a commit other than HEAD against paths.
>
> - Add a new flag `--revision` which will allow users to check the
> + Add a new flag `--source` which will allow users to check the
> attributes against a commit (actually any tree-ish would do). When the
> user uses this flag, we go through the stack of .gitattributes files but
> instead of checking the current working tree and/or in the index, we
> check the blobs from the provided tree-ish object. This allows the
> command to also be used in bare repositories.
>
> - Since we use a tree-ish object, the user can pass "--revision
> + Since we use a tree-ish object, the user can pass "--source
> HEAD:subdirectory" and all the attributes will be looked up as if
> subdirectory was the root directory of the repository.
>
> - We cannot simply use the `<rev>:<path>` syntax without the `--revision`
> + We cannot simply use the `<rev>:<path>` syntax without the `--source`
> flag, similar to how it is used in `git show` because any non-flag
> parameter before `--` is treated as an attribute and any parameter after
> `--` is treated as a pathname.
>
> The change involves creating a new function `read_attr_from_blob`, which
> - given the path reads the blob for the path against the provided revision and
> + given the path reads the blob for the path against the provided source and
> parses the attributes line by line. This function is plugged into
> `read_attr()` function wherein we go through the stack of attributes
> files.
> @@ Documentation/git-check-attr.txt: git-check-attr - Display gitattributes informa
> [verse]
> -'git check-attr' [-a | --all | <attr>...] [--] <pathname>...
> -'git check-attr' --stdin [-z] [-a | --all | <attr>...]
> -+'git check-attr' [--revision <revision>] [-a | --all | <attr>...] [--] <pathname>...
> -+'git check-attr' --stdin [-z] [--revision <revision>] [-a | --all | <attr>...]
> ++'git check-attr' [--source <tree>] [-a | --all | <attr>...] [--] <pathname>...
> ++'git check-attr' --stdin [-z] [--source <tree>] [-a | --all | <attr>...]
>
> DESCRIPTION
> -----------
> @@ Documentation/git-check-attr.txt: OPTIONS
> If `--stdin` is also given, input paths are separated
> with a NUL character instead of a linefeed character.
>
> -+--revision=<revision>::
> -+ Check attributes against the specified commit. All the attributes will
> -+ be checked against the provided revision. Paths provided as part of the
> -+ revision will be treated as the root directory.
> ++--source=<tree>::
> ++ Check attributes against the specified tree-ish. Paths provided as part
> ++ of the revision will be treated as the root directory. It is common to
> ++ specify the source tree by naming a commit, branch or tag associated
> ++ with it.
> +
> \--::
> Interpret all preceding arguments as attributes and all following
> @@ archive.c: static const struct attr_check *get_archive_attrs(struct index_state
>
> ## attr.c ##
> @@
> - #include "exec-cmd.h"
> - #include "attr.h"
> #include "dir.h"
> -+#include "strbuf.h"
> -+#include "tree-walk.h"
> #include "utf8.h"
> #include "quote.h"
> +#include "revision.h"
> @@ attr.c: void git_check_attr(struct index_state *istate,
> const char *name = check->all_attrs[i].attr->name;
>
> ## attr.h ##
> -@@
> - #ifndef ATTR_H
> - #define ATTR_H
> -
> -+#include "hash.h"
> -+
> - /**
> - * gitattributes mechanism gives a uniform way to associate various attributes
> - * to set of paths.
> @@ attr.h: void attr_check_free(struct attr_check *check);
> const char *git_attr_name(const struct git_attr *);
>
> @@ attr.h: void attr_check_free(struct attr_check *check);
> enum git_attr_direction {
>
> ## builtin/check-attr.c ##
> -@@
> -+#include "repository.h"
> - #define USE_THE_INDEX_VARIABLE
> - #include "builtin.h"
> - #include "cache.h"
> @@
> static int all_attrs;
> static int cached_attrs;
> static int stdin_paths;
> -+static char *revision;
> ++static char *source;
> static const char * const check_attr_usage[] = {
> -N_("git check-attr [-a | --all | <attr>...] [--] <pathname>..."),
> -N_("git check-attr --stdin [-z] [-a | --all | <attr>...]"),
> -+N_("git check-attr [--revision <revision>] [-a | --all | <attr>...] [--] <pathname>..."),
> -+N_("git check-attr --stdin [-z] [--revision <revision>] [-a | --all | <attr>...]"),
> ++N_("git check-attr [--source <tree>] [-a | --all | <attr>...] [--] <pathname>..."),
> ++N_("git check-attr --stdin [-z] [--source <tree>] [-a | --all | <attr>...]"),
> NULL
> };
>
> @@ builtin/check-attr.c: static const struct option check_attr_options[] = {
> OPT_BOOL(0 , "stdin", &stdin_paths, N_("read file names from stdin")),
> OPT_BOOL('z', NULL, &nul_term_line,
> N_("terminate input and output records by a NUL character")),
> -+ OPT_STRING(0, "revision", &revision, N_("revision"), N_("check attributes at this revision")),
> ++ OPT_STRING(0, "source", &source, N_("<tree-ish>"), N_("which tree-ish to check attributes at")),
> OPT_END()
> };
>
> @@ builtin/check-attr.c: static NORETURN void error_with_usage(const char *msg)
> {
> struct attr_check *check;
> + struct object_id *tree_oid = NULL;
> ++ struct object_id initialized_oid;
> int cnt, i, doubledash, filei;
>
> if (!is_bare_repository())
> @@ builtin/check-attr.c: int cmd_check_attr(int argc, const char **argv, const char
> }
> }
>
> -+ if (revision) {
> -+ tree_oid = xmalloc(sizeof(struct object_id));
> -+
> -+ if (repo_get_oid_tree(the_repository, revision, tree_oid))
> -+ error("%s: not a valid revision", revision);
> ++ if (source) {
> ++ if (repo_get_oid_tree(the_repository, source, &initialized_oid))
> ++ die("%s: not a valid tree-ish source", source);
> ++ tree_oid = &initialized_oid;
> + }
> +
> if (stdin_paths)
> @@ t/t0003-attributes.sh: attr_check_quote () {
> test_cmp expect actual
> +}
> +
> -+attr_check_revision () {
> -+ path="$1" expect="$2" revision="$3" git_opts="$4" &&
> ++attr_check_source () {
> ++ path="$1" expect="$2" source="$3" git_opts="$4" &&
>
> -+ git $git_opts check-attr --revision $revision test -- "$path" >actual 2>err &&
> ++ git $git_opts check-attr --source $source test -- "$path" >actual 2>err &&
> + echo "$path: test: $expect" >expect &&
> + test_cmp expect actual
> ++ test_must_be_empty err
> }
>
> test_expect_success 'open-quoted pathname' '
> @@ t/t0003-attributes.sh: test_expect_success 'setup' '
> '
>
> +test_expect_success 'setup branches' '
> -+ (
> -+ echo "f test=f" &&
> -+ echo "a/i test=n"
> -+ ) | git hash-object -w --stdin >id &&
> -+ git update-index --add --cacheinfo 100644,$(cat id),foo/bar/.gitattributes &&
> -+ git write-tree >id &&
> -+ tree_id=$(cat id) &&
> -+ git commit-tree $tree_id -m "random commit message" >id &&
> -+ commit_id=$(cat id) &&
> -+ git update-ref refs/heads/branch1 $commit_id &&
> ++ mkdir -p foo/bar &&
> ++ test_commit --printf "add .gitattributes" foo/bar/.gitattribute \
> ++ "f test=f\na/i test=n\n" tag-1 &&
> +
> -+ (
> -+ echo "g test=g" &&
> -+ echo "a/i test=m"
> -+ ) | git hash-object -w --stdin >id &&
> -+ git update-index --add --cacheinfo 100644,$(cat id),foo/bar/.gitattributes &&
> -+ git write-tree >id &&
> -+ tree_id=$(cat id) &&
> -+ git commit-tree $tree_id -m "random commit message" >id &&
> -+ commit_id=$(cat id) &&
> -+ git update-ref refs/heads/branch2 $commit_id
> ++ mkdir -p foo/bar &&
> ++ test_commit --printf "add .gitattributes" foo/bar/.gitattribute \
> ++ "g test=g\na/i test=m\n" tag-2
> +'
> +
> test_expect_success 'command line checks' '
> @@ t/t0003-attributes.sh: test_expect_success 'setup' '
> test_must_fail git check-attr test &&
> test_must_fail git check-attr test -- &&
> test_must_fail git check-attr -- f &&
> -+ test_must_fail git check-attr --revision &&
> -+ test_must_fail git check-attr --revision not-a-valid-ref &&
> ++ test_must_fail git check-attr --source &&
> ++ test_must_fail git check-attr --source not-a-valid-ref &&
> echo "f" | test_must_fail git check-attr --stdin &&
> echo "f" | test_must_fail git check-attr --stdin -- f &&
> echo "f" | test_must_fail git check-attr --stdin test -- f &&
> @@ t/t0003-attributes.sh: test_expect_success 'using --git-dir and --work-tree' '
> )
> '
>
> -+test_expect_success 'using --revision' '
> -+ attr_check_revision foo/bar/f f branch1 &&
> -+ attr_check_revision foo/bar/a/i n branch1 &&
> -+ attr_check_revision foo/bar/f unspecified branch2 &&
> -+ attr_check_revision foo/bar/a/i m branch2 &&
> -+ attr_check_revision foo/bar/g g branch2 &&
> -+ attr_check_revision foo/bar/g unspecified branch1
> ++test_expect_success 'using --source' '
> ++ attr_check_source foo/bar/f f tag-1 &&
> ++ attr_check_source foo/bar/a/i n tag-1 &&
> ++ attr_check_source foo/bar/f unspecified tag-2 &&
> ++ attr_check_source foo/bar/a/i m tag-2 &&
> ++ attr_check_source foo/bar/g g tag-2 &&
> ++ attr_check_source foo/bar/g unspecified tag-1
> +'
> +
> test_expect_success 'setup bare' '
> @@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr
> )
> '
>
> -+test_expect_success 'bare repository: with --revision' '
> ++test_expect_success 'bare repository: with --source' '
> + (
> + cd bare.git &&
> -+ (
> -+ echo "f test=f" &&
> -+ echo "a/i test=a/i"
> -+ ) | git hash-object -w --stdin >id &&
> -+ git update-index --add --cacheinfo 100644 $(cat id) .gitattributes &&
> -+ git write-tree >id &&
> -+ tree_id=$(cat id) &&
> -+ git commit-tree $tree_id -m "random commit message" >id &&
> -+ commit_id=$(cat id) &&
> -+ git update-ref refs/heads/master $commit_id &&
> -+ attr_check_revision f f HEAD &&
> -+ attr_check_revision a/f f HEAD &&
> -+ attr_check_revision a/c/f f HEAD &&
> -+ attr_check_revision a/i a/i HEAD &&
> -+ attr_check_revision subdir/a/i unspecified HEAD
> ++ attr_check_source foo/bar/f f tag-1 &&
> ++ attr_check_source foo/bar/a/i n tag-1 &&
> ++ attr_check_source foo/bar/f unspecified tag-2 &&
> ++ attr_check_source foo/bar/a/i m tag-2 &&
> ++ attr_check_source foo/bar/g g tag-2 &&
> ++ attr_check_source foo/bar/g unspecified tag-1
> + )
> +'
> +
>
>
> Karthik Nayak (2):
> t0003: move setup for `--all` into new block
> attr: add flag `--source` to work with tree-ish
>
> Documentation/git-check-attr.txt | 10 +++-
> archive.c | 2 +-
> attr.c | 97 +++++++++++++++++++++++---------
> attr.h | 5 +-
> builtin/check-attr.c | 35 +++++++-----
> builtin/pack-objects.c | 2 +-
> convert.c | 2 +-
> ll-merge.c | 4 +-
> pathspec.c | 2 +-
> t/t0003-attributes.sh | 49 +++++++++++++++-
> userdiff.c | 2 +-
> ws.c | 2 +-
> 12 files changed, 157 insertions(+), 55 deletions(-)
>
> --
> 2.39.0
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 2/2] attr: add flag `--source` to work with tree-ish
2023-01-02 11:04 ` [PATCH v5 2/2] attr: add flag `--source` to work with tree-ish Karthik Nayak
2023-01-02 23:38 ` Junio C Hamano
@ 2023-01-11 11:30 ` Phillip Wood
2023-01-12 10:53 ` Karthik Nayak
2023-01-12 13:20 ` Ævar Arnfjörð Bjarmason
2 siblings, 1 reply; 10+ messages in thread
From: Phillip Wood @ 2023-01-11 11:30 UTC (permalink / raw)
To: Karthik Nayak, git; +Cc: Toon Claes
Hi Karthik
On 02/01/2023 11:04, Karthik Nayak wrote:
> The contents of the .gitattributes files may evolve over time, but "git
> check-attr" always checks attributes against them in the working tree
> and/or in the index. It may be beneficial to optionally allow the users
> to check attributes taken from a commit other than HEAD against paths.
>
> Add a new flag `--source` which will allow users to check the
> attributes against a commit (actually any tree-ish would do). When the
> user uses this flag, we go through the stack of .gitattributes files but
> instead of checking the current working tree and/or in the index, we
> check the blobs from the provided tree-ish object. This allows the
> command to also be used in bare repositories.
>
> Since we use a tree-ish object, the user can pass "--source
> HEAD:subdirectory" and all the attributes will be looked up as if
> subdirectory was the root directory of the repository.
I think changing to --source is a good idea. I've left a few comments
below - the tests are broken at the moment. I didn't look very closely
at the implementation beyond scanning the range-diff as it looks like
there are not any significant changes there.
> We cannot simply use the `<rev>:<path>` syntax without the `--source`
> flag, similar to how it is used in `git show` because any non-flag
> parameter before `--` is treated as an attribute and any parameter after
> `--` is treated as a pathname.
>
> The change involves creating a new function `read_attr_from_blob`, which
> given the path reads the blob for the path against the provided source and
> parses the attributes line by line. This function is plugged into
> `read_attr()` function wherein we go through the stack of attributes
> files.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> Signed-off-by: Toon Claes <toon@iotcl.com>
> Co-authored-by: toon@iotcl.com
> ---
> -'git check-attr' [-a | --all | <attr>...] [--] <pathname>...
> -'git check-attr' --stdin [-z] [-a | --all | <attr>...]
> +'git check-attr' [--source <tree>] [-a | --all | <attr>...] [--] <pathname>...
> +'git check-attr' --stdin [-z] [--source <tree>] [-a | --all | <attr>...]
I think "<tree>" would be better as "<tree-ish>" (see my comments on the
--source option implementation below)
>
> DESCRIPTION
> -----------
> @@ -36,6 +36,12 @@ OPTIONS
> If `--stdin` is also given, input paths are separated
> with a NUL character instead of a linefeed character.
>
> +--source=<tree>::
> + Check attributes against the specified tree-ish. Paths provided as part
> + of the revision will be treated as the root directory. It is common to
> + specify the source tree by naming a commit, branch or tag associated
> + with it.
I think it is confusing to keep the reference to "revision" here, we
could just drop that sentence.
> -N_("git check-attr [-a | --all | <attr>...] [--] <pathname>..."),
> -N_("git check-attr --stdin [-z] [-a | --all | <attr>...]"),
> +N_("git check-attr [--source <tree>] [-a | --all | <attr>...] [--] <pathname>..."),
> +N_("git check-attr --stdin [-z] [--source <tree>] [-a | --all | <attr>...]"),
I think we should use "<tree-ish>" rather than "<tree>" so it is clear
one can specify a commit or tag. That's what "git restore" does.
> diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
> index b3aabb8aa3..78e9f47dbf 100755
> --- a/t/t0003-attributes.sh
> +++ b/t/t0003-attributes.sh
> @@ -25,7 +25,15 @@ attr_check_quote () {
> git check-attr test -- "$path" >actual &&
> echo "\"$quoted_path\": test: $expect" >expect &&
> test_cmp expect actual
> +}
> +
> +attr_check_source () {
> + path="$1" expect="$2" source="$3" git_opts="$4" &&
>
> + git $git_opts check-attr --source $source test -- "$path" >actual 2>err &&
> + echo "$path: test: $expect" >expect &&
> + test_cmp expect actual
This is missing && which means we miss some test failures later
> + test_must_be_empty err
> }
>
> +test_expect_success 'setup branches' '
> + mkdir -p foo/bar &&
> + test_commit --printf "add .gitattributes" foo/bar/.gitattribute \
The file should be foo/bar/.gitattributes (not .gitattribute). We're
missing failures due to this because of the missing && above
> + "f test=f\na/i test=n\n" tag-1 &&
> +
> + mkdir -p foo/bar &&
We don't need to make the directory again here
> + test_commit --printf "add .gitattributes" foo/bar/.gitattribute \
> + "g test=g\na/i test=m\n" tag-2
I think it would be worth either removing foo/bar/.gitattributes or
donig test_write_lines to change it. That way we can be sure all the
--source tests are actually using the tree-ish we give it and not just
reading from the filesystem.
Best Wishes
Phillip
> +'
> +
> test_expect_success 'command line checks' '
> test_must_fail git check-attr &&
> test_must_fail git check-attr -- &&
> test_must_fail git check-attr test &&
> test_must_fail git check-attr test -- &&
> test_must_fail git check-attr -- f &&
> + test_must_fail git check-attr --source &&
> + test_must_fail git check-attr --source not-a-valid-ref &&
> echo "f" | test_must_fail git check-attr --stdin &&
> echo "f" | test_must_fail git check-attr --stdin -- f &&
> echo "f" | test_must_fail git check-attr --stdin test -- f &&
> @@ -287,6 +306,15 @@ test_expect_success 'using --git-dir and --work-tree' '
> )
> '
>
> +test_expect_success 'using --source' '
> + attr_check_source foo/bar/f f tag-1 &&
> + attr_check_source foo/bar/a/i n tag-1 &&
> + attr_check_source foo/bar/f unspecified tag-2 &&
> + attr_check_source foo/bar/a/i m tag-2 &&
> + attr_check_source foo/bar/g g tag-2 &&
> + attr_check_source foo/bar/g unspecified tag-1
> +'
> +
> test_expect_success 'setup bare' '
> git clone --template= --bare . bare.git
> '
> @@ -306,6 +334,18 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' '
> )
> '
>
> +test_expect_success 'bare repository: with --source' '
> + (
> + cd bare.git &&
> + attr_check_source foo/bar/f f tag-1 &&
> + attr_check_source foo/bar/a/i n tag-1 &&
> + attr_check_source foo/bar/f unspecified tag-2 &&
> + attr_check_source foo/bar/a/i m tag-2 &&
> + attr_check_source foo/bar/g g tag-2 &&
> + attr_check_source foo/bar/g unspecified tag-1
> + )
> +'
> +
> test_expect_success 'bare repository: check that --cached honors index' '
> (
> cd bare.git &&
> diff --git a/userdiff.c b/userdiff.c
> index 151d9a5278..b66f090a0b 100644
> --- a/userdiff.c
> +++ b/userdiff.c
> @@ -412,7 +412,7 @@ struct userdiff_driver *userdiff_find_by_path(struct index_state *istate,
> check = attr_check_initl("diff", NULL);
> if (!path)
> return NULL;
> - git_check_attr(istate, path, check);
> + git_check_attr(istate, NULL, path, check);
>
> if (ATTR_TRUE(check->items[0].value))
> return &driver_true;
> diff --git a/ws.c b/ws.c
> index 6e69877f25..eadbbe5667 100644
> --- a/ws.c
> +++ b/ws.c
> @@ -78,7 +78,7 @@ unsigned whitespace_rule(struct index_state *istate, const char *pathname)
> if (!attr_whitespace_rule)
> attr_whitespace_rule = attr_check_initl("whitespace", NULL);
>
> - git_check_attr(istate, pathname, attr_whitespace_rule);
> + git_check_attr(istate, NULL, pathname, attr_whitespace_rule);
> value = attr_whitespace_rule->items[0].value;
> if (ATTR_TRUE(value)) {
> /* true (whitespace) */
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 2/2] attr: add flag `--source` to work with tree-ish
2023-01-11 11:30 ` Phillip Wood
@ 2023-01-12 10:53 ` Karthik Nayak
0 siblings, 0 replies; 10+ messages in thread
From: Karthik Nayak @ 2023-01-12 10:53 UTC (permalink / raw)
To: phillip.wood; +Cc: git, Toon Claes
On Wed, Jan 11, 2023 at 12:30 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Karthik
>
Hello Philip!
> On 02/01/2023 11:04, Karthik Nayak wrote:
> > The contents of the .gitattributes files may evolve over time, but "git
> > check-attr" always checks attributes against them in the working tree
> > and/or in the index. It may be beneficial to optionally allow the users
> > to check attributes taken from a commit other than HEAD against paths.
> >
> > Add a new flag `--source` which will allow users to check the
> > attributes against a commit (actually any tree-ish would do). When the
> > user uses this flag, we go through the stack of .gitattributes files but
> > instead of checking the current working tree and/or in the index, we
> > check the blobs from the provided tree-ish object. This allows the
> > command to also be used in bare repositories.
> >
> > Since we use a tree-ish object, the user can pass "--source
> > HEAD:subdirectory" and all the attributes will be looked up as if
> > subdirectory was the root directory of the repository.
>
> I think changing to --source is a good idea. I've left a few comments
> below - the tests are broken at the moment. I didn't look very closely
> at the implementation beyond scanning the range-diff as it looks like
> there are not any significant changes there.
>
Thanks for the review!
> > We cannot simply use the `<rev>:<path>` syntax without the `--source`
> > flag, similar to how it is used in `git show` because any non-flag
> > parameter before `--` is treated as an attribute and any parameter after
> > `--` is treated as a pathname.
> >
> > The change involves creating a new function `read_attr_from_blob`, which
> > given the path reads the blob for the path against the provided source and
> > parses the attributes line by line. This function is plugged into
> > `read_attr()` function wherein we go through the stack of attributes
> > files.
> >
> > Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> > Signed-off-by: Toon Claes <toon@iotcl.com>
> > Co-authored-by: toon@iotcl.com
> > ---
> > -'git check-attr' [-a | --all | <attr>...] [--] <pathname>...
> > -'git check-attr' --stdin [-z] [-a | --all | <attr>...]
> > +'git check-attr' [--source <tree>] [-a | --all | <attr>...] [--] <pathname>...
> > +'git check-attr' --stdin [-z] [--source <tree>] [-a | --all | <attr>...]
>
> I think "<tree>" would be better as "<tree-ish>" (see my comments on the
> --source option implementation below)
Will change!
> >
> > DESCRIPTION
> > -----------
> > @@ -36,6 +36,12 @@ OPTIONS
> > If `--stdin` is also given, input paths are separated
> > with a NUL character instead of a linefeed character.
> >
> > +--source=<tree>::
> > + Check attributes against the specified tree-ish. Paths provided as part
> > + of the revision will be treated as the root directory. It is common to
> > + specify the source tree by naming a commit, branch or tag associated
> > + with it.
>
> I think it is confusing to keep the reference to "revision" here, we
> could just drop that sentence.
>
Will do!
> > -N_("git check-attr [-a | --all | <attr>...] [--] <pathname>..."),
> > -N_("git check-attr --stdin [-z] [-a | --all | <attr>...]"),
> > +N_("git check-attr [--source <tree>] [-a | --all | <attr>...] [--] <pathname>..."),
> > +N_("git check-attr --stdin [-z] [--source <tree>] [-a | --all | <attr>...]"),
>
> I think we should use "<tree-ish>" rather than "<tree>" so it is clear
> one can specify a commit or tag. That's what "git restore" does.
>
Yeah sure!
> > diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
> > index b3aabb8aa3..78e9f47dbf 100755
> > --- a/t/t0003-attributes.sh
> > +++ b/t/t0003-attributes.sh
> > @@ -25,7 +25,15 @@ attr_check_quote () {
> > git check-attr test -- "$path" >actual &&
> > echo "\"$quoted_path\": test: $expect" >expect &&
> > test_cmp expect actual
> > +}
> > +
> > +attr_check_source () {
> > + path="$1" expect="$2" source="$3" git_opts="$4" &&
> >
> > + git $git_opts check-attr --source $source test -- "$path" >actual 2>err &&
> > + echo "$path: test: $expect" >expect &&
> > + test_cmp expect actual
>
> This is missing && which means we miss some test failures later
>
Good catch on this!
> > + test_must_be_empty err
> > }
> >
>
> > +test_expect_success 'setup branches' '
> > + mkdir -p foo/bar &&
> > + test_commit --printf "add .gitattributes" foo/bar/.gitattribute \
>
> The file should be foo/bar/.gitattributes (not .gitattribute). We're
> missing failures due to this because of the missing && above
>
Yup, this fixes the test. Thanks!
> > + "f test=f\na/i test=n\n" tag-1 &&
> > +
> > + mkdir -p foo/bar &&
>
> We don't need to make the directory again here
>
> > + test_commit --printf "add .gitattributes" foo/bar/.gitattribute \
> > + "g test=g\na/i test=m\n" tag-2
>
> I think it would be worth either removing foo/bar/.gitattributes or
> donig test_write_lines to change it. That way we can be sure all the
> --source tests are actually using the tree-ish we give it and not just
> reading from the filesystem.
>
Will do a `rm` on the file.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 2/2] attr: add flag `--source` to work with tree-ish
2023-01-02 11:04 ` [PATCH v5 2/2] attr: add flag `--source` to work with tree-ish Karthik Nayak
2023-01-02 23:38 ` Junio C Hamano
2023-01-11 11:30 ` Phillip Wood
@ 2023-01-12 13:20 ` Ævar Arnfjörð Bjarmason
2023-01-14 8:26 ` Karthik Nayak
2 siblings, 1 reply; 10+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-12 13:20 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, Toon Claes
On Mon, Jan 02 2023, Karthik Nayak wrote:
> +static struct attr_stack *read_attr_from_blob(struct index_state *istate,
> + const struct object_id *tree_oid,
> + const char *path, unsigned flags)
> +{
> + struct object_id oid;
> + unsigned long sz;
> + enum object_type type;
> + void *buf;
> + unsigned short mode;
> +
> + if (!tree_oid)
> + return NULL;
> +
> + if (get_tree_entry(istate->repo, tree_oid, path, &oid, &mode))
> + return NULL;
> +
> + buf = read_object_file(&oid, &type, &sz);
Here you flip-flop between istate->repo and "the_repository". I think
you want to use repo_read_object_file(istate->repo, ...) instead.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 2/2] attr: add flag `--source` to work with tree-ish
2023-01-12 13:20 ` Ævar Arnfjörð Bjarmason
@ 2023-01-14 8:26 ` Karthik Nayak
0 siblings, 0 replies; 10+ messages in thread
From: Karthik Nayak @ 2023-01-14 8:26 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Toon Claes
On Thu, Jan 12, 2023 at 2:21 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> On Mon, Jan 02 2023, Karthik Nayak wrote:
>
> > +static struct attr_stack *read_attr_from_blob(struct index_state *istate,
> > + const struct object_id *tree_oid,
> > + const char *path, unsigned flags)
> > +{
> > + struct object_id oid;
> > + unsigned long sz;
> > + enum object_type type;
> > + void *buf;
> > + unsigned short mode;
> > +
> > + if (!tree_oid)
> > + return NULL;
> > +
> > + if (get_tree_entry(istate->repo, tree_oid, path, &oid, &mode))
> > + return NULL;
> > +
> > + buf = read_object_file(&oid, &type, &sz);
>
> Here you flip-flop between istate->repo and "the_repository". I think
> you want to use repo_read_object_file(istate->repo, ...) instead.
Let me add this to v7! Thanks
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-01-14 8:27 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <https://lore.kernel.org/git/cover.1671630304.git.karthik.188@gmail.com/>
2023-01-02 11:04 ` [PATCH v5 0/2] check-attr: add support to work with tree-ish Karthik Nayak
2023-01-02 11:04 ` [PATCH v5 1/2] t0003: move setup for `--all` into new block Karthik Nayak
2023-01-02 11:04 ` [PATCH v5 2/2] attr: add flag `--source` to work with tree-ish Karthik Nayak
2023-01-02 23:38 ` Junio C Hamano
2023-01-04 10:25 ` Karthik Nayak
2023-01-11 11:30 ` Phillip Wood
2023-01-12 10:53 ` Karthik Nayak
2023-01-12 13:20 ` Ævar Arnfjörð Bjarmason
2023-01-14 8:26 ` Karthik Nayak
2023-01-09 9:10 ` [PATCH v5 0/2] check-attr: add support " Karthik Nayak
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).