Re: [PATCH v2 0/4] cat-file: optionally convert to worktree version

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]