Re: [PATCH v6 02/10] replace: add --graft option
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 9, 2014, 14:59 UTC
- Message-ID
- <xmqqsima7f3r.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <20140707063540.3708.51047.chriscool@tuxfamily.org>
Christian Couder <chriscool@tuxfamily.org> writes:
Show 12 quoted lines
> 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.
Show 39 quoted lines
>
> 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.
Show 15 quoted lines
> +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.