Re: [PATCH 2/2] attr: add flag `-r|--revisions` to work with revisions
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Dec 6, 2022, 11:27 UTC
- Message-ID
- <221206.861qpcdarb.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <20221206103736.53909-3-karthik.188@gmail.com>
On Tue, Dec 06 2022, Karthik Nayak wrote:
Show 57 quoted lines
> 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@gmail.com>
> Co-authored-by: toon@iotcl.com
> ---
> 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.
Show 7 quoted lines
> + struct object_id oid; > + unsigned long sz; > + enum object_type type; > + void *buf; > + struct strbuf sb; > + > + if (object_name == NULL)
Ditto !object_name test.
Show 6 quoted lines
> + 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..
Show 7 quoted lines
> 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.
Show 6 quoted lines
> +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..