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

From: Jeff King <redacted>

On Thu, May 22, 2014 at 11:33:04PM +0200, Christian Couder wrote:

quoted

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).

Thanks, I used something like that in the contrib script.
 

quoted

+	/* 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...

quoted

+	/* 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.

Yeah, but I don't think that it is a problem.

Those parent lines are not standard in the first place, because they
are not parsed by parse_commit_buffer(). And I don't think this option
should mess with non standard stuff.

It still feels rather magical to
me, as we are depending on unspoken format guarantees defined inside
parse_commit_buffer. 

My opinion is that we are depending on the standard way to parse
headers, and that's good. I think it would be "black magic" to mess
with non standard stuff.

I'd prefer something like the line-based parser I
showed in the other thread, but I suppose it may just be a matter of
preference.

Yeah probably.

Thanks,
Christian.