Re: [PATCH v6 02/10] replace: add --graft option

[Junio C Hamano <gitster@pobox.com>]

Christian Couder <chriscool@tuxfamily.org> writes:

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

Sensible.

>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/replace.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 73 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/replace.c b/builtin/replace.c
> index 1bb491d..ad47237 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -17,6 +17,7 @@
>  static const char * const git_replace_usage[] = {
>  	N_("git replace [-f] <object> <replacement>"),
>  	N_("git replace [-f] --edit <object>"),
> +	N_("git replace [-f] --graft <commit> [<parent>...]"),
>  	N_("git replace -d <object>..."),
>  	N_("git replace [--format=<format>] [-l [<pattern>]]"),
>  	NULL
> @@ -294,6 +295,66 @@ static int edit_and_replace(const char *object_ref, int force)
>  	return replace_object_sha1(object_ref, old, "replacement", new, force);
>  }
>  
> +static void replace_parents(struct strbuf *buf, int argc, const char **argv)
> +{
> +	struct strbuf new_parents = STRBUF_INIT;
> +	const char *parent_start, *parent_end;
> +	int i;
> +
> +	/* 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" */
> +
> +	/* prepare new parents */
> +	for (i = 1; i < argc; i++) {

It looks somewhat strange that both replace_parents() and
create_graft() take familiar-looking <argc, argv> pair, but one
ignores argv[0] and uses the remainder and the other uses argv[0].
Shouldn't this function consume argv[] starting from [0] for
consistency?  You'd obviously need to update the caller to adjust
the arguments it gives to this function.

> +static int create_graft(int argc, const char **argv, int force)
> +{
> +	unsigned char old[20], new[20];
> +	const char *old_ref = argv[0];
> +...
> +
> +	replace_parents(&buf, argc, argv);
> +
> +	if (write_sha1_file(buf.buf, buf.len, commit_type, new))
> +		die(_("could not write replacement commit for: '%s'"), old_ref);
> +
> +	strbuf_release(&buf);
> +
> +	if (!hashcmp(old, new))
> +		return error("new commit is the same as the old one: '%s'", sha1_to_hex(old));

Is this really an error?  It may be a warning-worthy situation for a
user or a script to end up doing a no-op graft, e.g.

	git replace --graft HEAD HEAD^

but I wonder if it is more convenient to signal an error (like this
patch does) or just ignore the request and return without adding the
replace ref.

Other than these two points, looks good to me.

Thanks.