Re: [RFC/PATCH 1/2] commit: add parse_commit_repl() to replace commits at parsing time

[Christian Couder <chriscool@tuxfamily.org>]

On Wednesday 18 August 2010 05:17:52 Nguyen Thai Ngoc Duy wrote:
> On Wed, Aug 18, 2010 at 7:18 AM, Junio C Hamano <gitster@pobox.com> wrote:
> > Christian Couder <chriscool@tuxfamily.org> writes:
> >> The function parse_commit() is not safe regarding replaced commits
> >> because it uses the buffer of the replacement commit but the object
> >> part of the commit struct stay the same. Especially the sha1 is not
> >> changed so it doesn't match the content of the commit.
> > 
> > This all sounds backwards to me, if I am reading the discussion
> > correctly.
> > 
> > If a replace record says commit 0123 is replaced by commit 4567 (iow,
> > 0123 was a mistake, and pretend that its content is what is recorded in
> > 4567), and when we are honoring the replace records (iow, we are not
> > fsck), shouldn't read_sha1("0123") give us a piece of memory that stores
> > what is recorded in 4567, parse_object("0123") return a struct commit
> > whose buffer points at a block of memory that has what is recorded in
> > 4567 _while_ its object.sha1[] say "0 123"?
> 
> 1. parse_object() as it is now would return object.sha1[] = "4567".
> 2. lookup_commit(), then parse_commit() would return object.sha1[] =
> "0123".
> 
> > What problem are you trying to solve?
> 
> Inconsistency in replacing objects. I have no comments whether #1 or
> #2 is expected behavior. But at least it should stick to one behavior
> only.

We discussed this inconsistency in this thread:

http://thread.gmane.org/gmane.comp.version-control.git/152321/ 

So we can resolve the inconsistency with Duy's patch to make parse_object() 
return object.sha1[] = "0123".

It's simpler and probably safer. The downside is that the sha1 will not be 
consistent with the content anymore and that it will be more difficult to 
realize that an object has been replaced as there will be no sha1 change to be 
seen.

Best regards,
Christian.