Re: [PATCH 2/2] attr: add flag `-r|--revisions` to work with revisions

[Karthik Nayak <karthik.188@gmail.com>]

Hello Ævar,

> >
> > 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.

>
> > +             more = (*ep == '\n');
>
> Nit: parens not needed.
>

I rather let this be, since it's existing code that I just move.

> > +     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.

> > +             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.

>
> >  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.

> >       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.

> > +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?

> > +             ) | 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.

> > +             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