From: Junio C Hamano <gitster@pobox•com>
To: Jeff King <peff@peff•net>
Cc: Christian Couder <chriscool@tuxfamily•org>,
git@vger•kernel.org, Michael Haggerty <mhagger@alum•mit.edu>
Subject: Re: [PATCH v1 1/3] replace: add --graft option
Date: Fri, 23 May 2014 13:05:40 -0700 [thread overview]
Message-ID: <xmqqlhtsi7l7.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <20140523195153.GB19088@sigill.intra.peff.net> (Jeff King's message of "Fri, 23 May 2014 15:51:53 -0400")
Jeff King <peff@peff•net> writes:
> 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.
>> + /* 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.
next prev parent reply other threads:[~2014-05-23 20:05 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-05-22 21:33 [PATCH v1 0/3] Add --graft option to git replace Christian Couder
2014-05-22 21:33 ` [PATCH v1 1/3] replace: add --graft option Christian Couder
2014-05-23 19:27 ` Junio C Hamano
2014-05-23 19:51 ` Jeff King
2014-05-23 20:05 ` Junio C Hamano [this message]
2014-05-23 20:28 ` Jeff King
2014-05-23 21:22 ` Junio C Hamano
2014-05-23 22:59 ` Eric Sunshine
2014-06-01 16:06 ` Christian Couder
2014-05-22 21:33 ` [PATCH v1 2/3] replace: add test for --graft Christian Couder
2014-05-22 21:33 ` [PATCH v1 3/3] Documentation: replace: add --graft option Christian Couder
2014-05-23 17:06 ` Jakub Narębski
2014-05-23 18:26 ` Junio C Hamano
2014-05-23 16:59 ` [PATCH v1 0/3] Add --graft option to git replace Junio C Hamano
2014-05-27 19:05 ` Christian Couder
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqlhtsi7l7.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=chriscool@tuxfamily$(echo .)org \
--cc=git@vger$(echo .)kernel.org \
--cc=mhagger@alum$(echo .)mit.edu \
--cc=peff@peff$(echo .)net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox