From: Jeff King <peff@peff.net>
Date: Wed, 24 Aug 2016 16:19:40 GMT
Subject: Re: [PATCH v2 0/4] cat-file: optionally convert to worktree version
Message-ID: <20160824161939.d2h4qlaxhhi6limm@sigill.intra.peff.net>
In-Reply-To: <xmqqk2f6rvx9.fsf@gitster.mtv.corp.google.com>

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