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

From: Junio C Hamano <[email-masked]>
Date: Tue, 19 Jan 2016 18:43:53 -0800

Jeff King <[email-masked]> writes:

> On Tue, Jan 19, 2016 at 05:51:58PM -0800, Junio C Hamano wrote:
>
>> Jeff King <[email-masked]> 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?