Re: [PATCH 2/3 v2] Teach git diff-tree --stdin to diff trees

From: Karl Hasselström <kha@treskal.com>
Date: Aug 10, 2008, 15:38 UTC
Message-ID: <20080810153848.GA28032@diana.vm.bytemark.co.uk>
In-Reply-To: <7vwsip53bg.fsf@gitster.siamese.dyndns.org>

On 2008-08-09 13:41:23 -0700, Junio C Hamano wrote:

Show 10 quoted lines

> Karl Hasselström <kha@treskal.com> writes:
>
> > When diffing trees (either specified on the command line or from
> > the standard input), the -m, -s, -v, --pretty, --abbrev-commit,
> > --encoding, --no-commit-id, -c, --cc, and --always options are
> > ignored, since they do not apply to trees.
>
> I've commented on this part already; -m, -c, --cc are excluded
> because they make sense only when you are dealing with three or more
> trees.

Fixed.

Show 9 quoted lines

> > +	if (len != 82 || !isspace(line[40]) || get_sha1_hex(line + 41, sha1)) {
> > +		error("Need precisely two trees, separated by one space");
> > +		return -1;
> > +	}
>
> error() returns -1, so:
>
> 	if (len != 82 || !isspace(line[40]) || get_sha1_hex(line + 41, sha1))
> 		return error("Need two trees, separated by one space");

Fixed.

Show 5 quoted lines

> > +	tree2 = lookup_tree(sha1);
> > +	if (!tree2 || parse_tree(tree2))
> > +		return -1;
>
> Don't you want to make error() say something here as well?

Looking at lookup_tree() and parse_tree(), I got the impression that they take care of that themselves. Do they miss some case that I need to cover?

Show 5 quoted lines

> > +	printf("%s %s\n", sha1_to_hex(tree1->object.sha1),
> > +			  sha1_to_hex(tree2->object.sha1));
>
> Since this is strictly for Porcelain's use, you may want to document
> this output format.

Yes. Fixed.

> Two-tree form from the command line does not have anything like
> this, and two-commit form from --stdin have either a single object
> name, the log message under -v or --pretty options. I notice that
> these are not documented but we may want to document it while at it.

I'll whip something up and send it out as a separate patch.

> Other than that, the patch looks good. Thanks.

Thanks for the feedback.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle