Re: [PATCH 2/2] attr: add flag `-r|--revisions` to work with revisions
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Dec 6, 2022, 13:06 UTC
- Message-ID
- <CAOLa=ZSOhtgmY-j3rj+=qANKOfTH8H+1YpwM63uTKUT04vsM-Q@mail.gmail.com>
- In-Reply-To
- <221206.861qpcdarb.gmgdl@evledraar.gmail.com>
Hello Ævar,
Show 16 quoted lines
> > > > 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. >
Right, will remove.
> > + if (buf == NULL) > > if (!buf)
Makes sense.
Show 5 quoted lines
> > > + more = (*ep == '\n'); > > Nit: parens not needed. >
I rather let this be, since it's existing code that I just move.
Show 10 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. >
Will change.
Show 15 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? >
This is much simpler and should do, will change.
> > + } else if (object_name != NULL) {
>
> else if (object_name)Will change.
Show 8 quoted lines
> > > 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.. >
I think it's due to tab-indent being set to a default 4, I fixed it in the other files, forgot to check the header. Will fix it.
Show 15 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.
>I will make this change, I didn't know about repo_get_oid() before.
Show 13 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 .... &&
>
>While I agree with what you're saying, the whole test file does it this way (echo in a subshell), wouldn't it be better to stay consistent?
Show 5 quoted lines
> > + ) | 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. >
Done.
Show 7 quoted lines
> > + git write-tree > id && > > We use ">id" style for redirection, not "> id". > > > + git commit-tree $(cat id) -m "random commit message" > id && > > Ditto..
Will make this change too.
Thanks for the review. Will wait a day or two before sending in the next version.
-- - Karthik