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
