Re: [PATCH v1 1/3] replace: add --graft option

From: Junio C Hamano <gitster@pobox.com>
Date: May 23, 2014, 20:05 UTC
Message-ID: <xmqqlhtsi7l7.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20140523195153.GB19088@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 34 quoted lines

> On Thu, May 22, 2014 at 11:33:04PM +0200, Christian Couder wrote:
>
>> The usage string for this option is:
>> 
>> git replace [-f] --graft <commit> [<parent>...]
>> 
>> First we create a new commit that is the same as <commit>
>> except that its parents are [<parents>...]
>> 
>> Then we create a replace ref that replace <commit> with
>> the commit we just created.
>> 
>> With this new option, it should be straightforward to
>> convert grafts to replace refs, with something like:
>> 
>> cat .git/info/grafts | while read line
>> do git replace --graft $line; done
>
> I think this script at the end should end up in the documentation or a
> contrib script (probably the former, as it is short enough that somebody
> might just cut-and-paste).
>
> The graft file ignores comments and blank lines, so maybe "grep '^[^#]'"
> would be in order.
>
> And maybe it's just me, but I think spacing it like:
>
>   grep '^[^#]' .git/info/grafts |
>   while read line; do
> 	git replace --graft $line
>   done
>
> is more readable (I think some would even argue for putting the "do" on
> a separate line).

Yes, I would ;-)

I just read read_graft_line(); it allows an empty line (both length-0 before the terminating LF or CRLF, and a line with isspace() only) and ignore them, so "grep '^[^#]'" is not sufficient.

Show 18 quoted lines

>> +	/* make sure the commit is not corrupt */
>> +	if (parse_commit_buffer(commit, buf.buf, buf.len))
>> +		die(_("Could not parse commit: '%s'"), old_ref);
>
> I guess the checks here are sufficient to make...
>
>> +	/* find existing parents */
>> +	parent_start = buf.buf;
>> +	parent_start += 46; /* "tree " + "hex sha1" + "\n" */
>> +	parent_end = parent_start;
>> +
>> +	while (starts_with(parent_end, "parent "))
>> +		parent_end += 48; /* "parent " + "hex sha1" + "\n" */
>
> ...this number-based parsing safe, though it would miss removing a stray
> parent line elsewhere in the commit. It still feels rather magical to
> me, as we are depending on unspoken format guarantees defined inside
> parse_commit_buffer.

Do you mean "we have a carved-in-stone rule that all 'parent ' lines must come immediately after a single 'tree ' line, and that is implemented in parse_commit_buffer(). The above code follows the exact same rule. It would be nice to have some mechanism to tell somebody who wants to update the former that s/he must update this new code at the same time"?

I think a commit object that violates the rule is simply broken, and it is OK to add a mode to tell parse-commit-buffer to tolerate such a broken object, if only so that filter-branch or some other tools can be used to correct a history that contains it.

Perhaps a more future-proof way to write Christian's code may be:

    - find "tree ";

    - splice the new parents in immediately after that "tree " line;

    - starting from the end of these new parents, scan up to the end
      of the header line-by-line, and splice out any line that
      begins with "parent ".

which may not be too bad.