Re: [PATCH] filter-branch: resolve $commit^{tree} in no-index case

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 20, 2016, 02:43 UTC
Message-ID: <xmqq4me96kd2.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160120020039.GD24541@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 24 quoted lines

> On Tue, Jan 19, 2016 at 05:51:58PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> >> Mph.  We could get the best of both worlds by introducing a "git
>> >> rev-parse --compare <a> <b>" that compares object ids.  Actually...
>> >> 
>> >> How about something like this?
>> >
>> > Thanks. I had in my head that we could do something like that, but
>> > hadn't quite worked it out. I think what you wrote works.
>> 
>> But wouldn't "diff-tree --quiet" essentially be that command?
>
> I think Jonathan was responding to my point that "diff-tree --quiet"
> _isn't_ quite the same, if you have mis-formatted tree objects. If the
> sha1s are different, a rev-parse comparison will keep the commit. But
> "diff-tree" will actually do the diff, and may consider different sha1s
> to have the same content, dropping the second one.
>
> It's a minor point, but I find one of my primary uses for filter-branch
> these days is massaging out bogus objects made by older or buggy git
> clients (not that I see _that_ many of them; I think it speaks more to
> the fact that I don't really use filter-branch much these days).

I (think I) understand that use case, but this function compares the parent tree and the tree of the commit we just created, and it does so in order to skip the one we just created (when --prune-empty is given). It is not about "tree-filter returned a tree, let's compare them and if the old one and the new one looks the same (even though they do not have identical object name) and replace the old one with the new before giving it to commit-filter, so that we have a larger chance of noticing a case where we did not have to rewrite and instead were able to reuse the old commit and tree".

In other words, I do not think "broken tree object may look the same to diff-tree, but I do want to replace it" is relevant to this codepath; it is not something this function handles, I think.

What am I not seeing?