On Wed, Aug 24, 2016 at 09:09:06AM -0700, Junio C Hamano wrote:
> > + if (!path)
> > + path = obj_context.path;
> > + else if (obj_context.mode == S_IFINVALID)
> > + obj_context.mode = 0100644;
> > +
> > buf = NULL;
> > switch (opt) {
> > case 't':
>
> The above two hunks make all the difference in the ease of reading
> the remainder of the function. Very good.
Yeah, I agree. Though it took me a moment to figure out why we were
setting obj_context.mode but not obj_context.path; the reason is that
"mode" is convenient to use as local storage, but "path" is not, because
it is not a pointer but an array.
So it would have been a little clearer to me as:
const char *path;
unsigned mode;
...
if (!force_path) {
/* use file info from sha1 lookup */
path = obj_context.path;
mode = obj_context.mode;
} else {
/* use path requested by user, and assume it is a regular file */
path = force_path;
mode = 0100644;
}
but I don't know if that is even worth a re-roll.
> > +test_expect_success '----path=<path> complains without
> > --textconv/--filters' '
> > + sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
> > + test_must_fail git cat-file --path=hello.txt blob $sha1 >actual 2>err &&
> > + test ! -s actual &&
> > + grep "path.*needs.*filters" err
> > +'
>
> This will need to become test_i18ngrep once the error message is
> made translatable, but it is correct for now. I personally think
> there is no need to check "actual" or "err", though---just running
> cat-file under test_must_fail should be sufficient.
>
> Thanks, will queue.
The whole thing looks good to me, except for the weird doubled "--" in
the test description you quoted above. :)
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
