On Tue, Dec 06 2022, Karthik Nayak wrote:
> Git check-attr currently doesn't check the git worktree, it either
> checks the index or the files directly. This means we cannot check the
> attributes for a file against a certain revision.
>
> Add a new flag `--revision`/`-r` which will allow it work with
> revisions. This command will now, instead of checking the files/index,
> try and receive the blob for the given attribute file against the
> provided revision. The flag overrides checking against the index and
> filesystem and also works with bare repositories.
>
> We cannot use the `<rev>:<path>` syntax like the one used in `git show`
> because any non-flag parameter before `--` is treated as an attribute
> and any parameter after `--` is treated as a pathname.
>
> This involves creating a new function `read_attr_from_blob`, which given
> the path reads the blob for the path against the provided revision and
> parses the attributes line by line. This function is plugged into
> `read_attr()` function wherein we go through the different attributes.
>
> Signed-off-by: Karthik Nayak <karthik.188@xxxxxxxxx>
> Co-authored-by: toon@xxxxxxxxx
> ---
> archive.c | 2 +-
> attr.c | 120 ++++++++++++++++++++++++++++-------------
> attr.h | 7 ++-
> builtin/check-attr.c | 25 ++++-----
> builtin/pack-objects.c | 2 +-
> convert.c | 2 +-
> ll-merge.c | 4 +-
> pathspec.c | 2 +-
> t/t0003-attributes.sh | 56 ++++++++++++++++++-
> userdiff.c | 2 +-
> ws.c | 2 +-
> 11 files changed, 165 insertions(+), 59 deletions(-)
>
> diff --git a/archive.c b/archive.c
> index 941495f5d7..bf64dbdce1 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, path, check, NULL);
> return check;
> }
>
> diff --git a/attr.c b/attr.c
> index 42ad6de8c7..42b67a401f 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -11,8 +11,12 @@
> #include "exec-cmd.h"
> #include "attr.h"
> #include "dir.h"
> +#include "git-compat-util.h"
As a rule in this project we include either cache.h at the top, or
git-compat-util.h, and the former includes the latter.
This file already has cache.h at the top, so it won't need this.
> + if (buf == NULL)
if (!buf)
> + more = (*ep == '\n');
Nit: parens not needed.
> + struct object_id oid;
> + unsigned long sz;
> + enum object_type type;
> + void *buf;
> + struct strbuf sb;
> +
> + if (object_name == NULL)
Ditto !object_name test.
> + return NULL;
> +
> + strbuf_init(&sb, strlen(path) + 1 + strlen(object_name));
> + strbuf_addstr(&sb, object_name);
> + strbuf_addstr(&sb, ":");
> + strbuf_addstr(&sb, path);
Is this really performance sensitive so we need to pre-size this, or
would a simpler:
struct strbuf sb = STRBUF_INIT;
strbuf_addf(&sb, "%s:%s", path, object_name);
Do?
> + } else if (object_name != NULL) {
else if (object_name)
> void git_check_attr(struct index_state *istate,
> - const char *path, struct attr_check *check);
> + const char *path, struct attr_check *check,
> + const char *object_name);
This (and possibly other places) seem funnily indented..
> if (collect_all) {
> - git_all_attrs(&the_index, full_path, check);
> + git_all_attrs(&the_index, full_path, check, object_name);
> } else {
> - git_check_attr(&the_index, full_path, check);
> + git_check_attr(&the_index, full_path, check, object_name);
> }
Earlier you do a get_oid(), shouldn't that be a
repo_get_oid(istate->repo, ...) to be future-proof? I.e. use the repo of
the passed-in index.
I think it'll always be "the_repository" for now, but for an API it
makes sense to hardcode that assumption in fewer places.
> +test_expect_success 'bare repository: with --revision' '
> + (
> + cd bare.git &&
> + (
> + echo "f test=f" &&
> + echo "a/i test=a/i"
You don't need a subshell just to echo a string. You can use {}-braces,
but in this case just:
printf "%s\n", "f test=f" "a/i test=a/i" | git hash-object .... &&
> + ) | git hash-object -w --stdin > id &&
> + git update-index --add --cacheinfo 100644 $(cat id) .gitattributes &&
Split the "cat" into a varible, otherwise its failure will be hidden.
> + git write-tree > id &&
We use ">id" style for redirection, not "> id".
> + git commit-tree $(cat id) -m "random commit message" > id &&
Ditto..
