[PATCH v3 0/4] Add --graft option to git replace

[PATCH v3 0/4] Add --graft option to git replace

[Christian Couder]

Here is a small patch series to implement:

        git replace [-f] --graft <commit> [<parent>...]

This patch series goes on top of the patch series that
implements --edit.

There is only one change since v2 thanks to Eric:

- improve error messages in convert-grafts-to-replace-refs.sh
  (patch 4/4)

Christian Couder (4):
  replace: add --graft option
  replace: add test for --graft
  Documentation: replace: add --graft option
  contrib: add convert-grafts-to-replace-refs.sh

 Documentation/git-replace.txt             | 10 ++++
 builtin/replace.c                         | 84 ++++++++++++++++++++++++++++++-
 contrib/convert-grafts-to-replace-refs.sh | 29 +++++++++++
 t/t6050-replace.sh                        | 12 +++++
 4 files changed, 134 insertions(+), 1 deletion(-)
 create mode 100755 contrib/convert-grafts-to-replace-refs.sh

-- 
2.0.0.rc0.40.gd30ccc4

[PATCH v3 1/4] replace: add --graft option

[Christian Couder]

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.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/replace.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 83 insertions(+), 1 deletion(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index 1bb491d..9d1e82f 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,76 @@ static int edit_and_replace(const char *object_ref, int force)
 	return replace_object_sha1(object_ref, old, "replacement", new, force);
 }
 
+static int read_sha1_commit(const unsigned char *sha1, struct strbuf *dst)
+{
+	void *buf;
+	enum object_type type;
+	unsigned long size;
+
+	buf = read_sha1_file(sha1, &type, &size);
+	if (!buf)
+		return error(_("cannot read object %s"), sha1_to_hex(sha1));
+	if (type != OBJ_COMMIT) {
+		free(buf);
+		return error(_("object %s is not a commit"), sha1_to_hex(sha1));
+	}
+	strbuf_attach(dst, buf, size, size + 1);
+	return 0;
+}
+
+static int create_graft(int argc, const char **argv, int force)
+{
+	unsigned char old[20], new[20];
+	const char *old_ref = argv[0];
+	struct commit *commit;
+	struct strbuf buf = STRBUF_INIT;
+	struct strbuf new_parents = STRBUF_INIT;
+	const char *parent_start, *parent_end;
+	int i;
+
+	if (get_sha1(old_ref, old) < 0)
+		die(_("Not a valid object name: '%s'"), old_ref);
+	commit = lookup_commit_or_die(old, old_ref);
+	if (read_sha1_commit(old, &buf))
+		die(_("Invalid commit: '%s'"), old_ref);
+
+	/* make sure the commit is not corrupt */
+	if (parse_commit_buffer(commit, buf.buf, buf.len))
+		die(_("Could not parse commit: '%s'"), old_ref);
+
+	/* 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++) {
+		unsigned char sha1[20];
+		if (get_sha1(argv[i], sha1) < 0)
+			die(_("Not a valid object name: '%s'"), argv[i]);
+		lookup_commit_or_die(sha1, argv[i]);
+		strbuf_addf(&new_parents, "parent %s\n", sha1_to_hex(sha1));
+	}
+
+	/* replace existing parents with new ones */
+	strbuf_splice(&buf, parent_start - buf.buf, parent_end - parent_start,
+		      new_parents.buf, new_parents.len);
+
+	if (write_sha1_file(buf.buf, buf.len, commit_type, new))
+		die(_("could not write replacement commit for: '%s'"), old_ref);
+
+	strbuf_release(&new_parents);
+	strbuf_release(&buf);
+
+	if (!hashcmp(old, new))
+		return error("new commit is the same as the old one: '%s'", sha1_to_hex(old));
+
+	return replace_object_sha1(old_ref, old, "replacement", new, force);
+}
+
 int cmd_replace(int argc, const char **argv, const char *prefix)
 {
 	int force = 0;
@@ -303,12 +374,14 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
 		MODE_LIST,
 		MODE_DELETE,
 		MODE_EDIT,
+		MODE_GRAFT,
 		MODE_REPLACE
 	} cmdmode = MODE_UNSPECIFIED;
 	struct option options[] = {
 		OPT_CMDMODE('l', "list", &cmdmode, N_("list replace refs"), MODE_LIST),
 		OPT_CMDMODE('d', "delete", &cmdmode, N_("delete replace refs"), MODE_DELETE),
 		OPT_CMDMODE('e', "edit", &cmdmode, N_("edit existing object"), MODE_EDIT),
+		OPT_CMDMODE('g', "graft", &cmdmode, N_("change a commit's parents"), MODE_GRAFT),
 		OPT_BOOL('f', "force", &force, N_("replace the ref if it exists")),
 		OPT_STRING(0, "format", &format, N_("format"), N_("use this format")),
 		OPT_END()
@@ -325,7 +398,10 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
 		usage_msg_opt("--format cannot be used when not listing",
 			      git_replace_usage, options);
 
-	if (force && cmdmode != MODE_REPLACE && cmdmode != MODE_EDIT)
+	if (force &&
+	    cmdmode != MODE_REPLACE &&
+	    cmdmode != MODE_EDIT &&
+	    cmdmode != MODE_GRAFT)
 		usage_msg_opt("-f only makes sense when writing a replacement",
 			      git_replace_usage, options);
 
@@ -348,6 +424,12 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
 				      git_replace_usage, options);
 		return edit_and_replace(argv[0], force);
 
+	case MODE_GRAFT:
+		if (argc < 1)
+			usage_msg_opt("-g needs at least one argument",
+				      git_replace_usage, options);
+		return create_graft(argc, argv, force);
+
 	case MODE_LIST:
 		if (argc > 1)
 			usage_msg_opt("only one pattern can be given with -l",
-- 
2.0.0.rc0.40.gd30ccc4

[PATCH v3 2/4] replace: add test for --graft

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t6050-replace.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index 68b3cb2..ca45a84 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -351,4 +351,16 @@ test_expect_success 'replace ref cleanup' '
 	test -z "$(git replace)"
 '
 
+test_expect_success '--graft with and without already replaced object' '
+	test $(git log --oneline | wc -l) = 7 &&
+	git replace --graft $HASH5 &&
+	test $(git log --oneline | wc -l) = 3 &&
+	git cat-file -p $HASH5 | test_must_fail grep parent &&
+	test_must_fail git replace --graft $HASH5 $HASH4 $HASH3 &&
+	git replace --force -g $HASH5 $HASH4 $HASH3 &&
+	git cat-file -p $HASH5 | grep "parent $HASH4" &&
+	git cat-file -p $HASH5 | grep "parent $HASH3" &&
+	git replace -d $HASH5
+'
+
 test_done
-- 
2.0.0.rc0.40.gd30ccc4

[PATCH v3 3/4] Documentation: replace: add --graft option

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-replace.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/git-replace.txt b/Documentation/git-replace.txt
index 61461b9..491875e 100644
--- a/Documentation/git-replace.txt
+++ b/Documentation/git-replace.txt
@@ -10,6 +10,7 @@ SYNOPSIS
 [verse]
 'git replace' [-f] <object> <replacement>
 'git replace' [-f] --edit <object>
+'git replace' [-f] --graft <commit> [<parent>...]
 'git replace' -d <object>...
 'git replace' [--format=<format>] [-l [<pattern>]]
 
@@ -73,6 +74,13 @@ OPTIONS
 	newly created object. See linkgit:git-var[1] for details about
 	how the editor will be chosen.
 
+--graft <commit> [<parent>...]::
+	Create a graft commit. A new commit is created with the same
+	content as <commit> except that its parents will be
+	[<parent>...] instead of <commit>'s parents. A replacement ref
+	is then created to replace <commit> with the newly created
+	commit.
+
 -l <pattern>::
 --list <pattern>::
 	List replace refs for objects that match the given pattern (or
-- 
2.0.0.rc0.40.gd30ccc4

[PATCH v3 4/4] contrib: add convert-grafts-to-replace-refs.sh

[Christian Couder]

This patch adds into contrib/ an example script to convert
grafts from an existing grafts file into replace refs using
the new --graft option of "git replace".

While at it let's mention this new script in the
"git replace" documentation for the --graft option.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-replace.txt             |  4 +++-
 contrib/convert-grafts-to-replace-refs.sh | 29 +++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 1 deletion(-)
 create mode 100755 contrib/convert-grafts-to-replace-refs.sh

diff --git a/Documentation/git-replace.txt b/Documentation/git-replace.txt
index 491875e..e1be2e6 100644
--- a/Documentation/git-replace.txt
+++ b/Documentation/git-replace.txt
@@ -79,7 +79,9 @@ OPTIONS
 	content as <commit> except that its parents will be
 	[<parent>...] instead of <commit>'s parents. A replacement ref
 	is then created to replace <commit> with the newly created
-	commit.
+	commit. See contrib/convert-grafts-to-replace-refs.sh for an
+	example script based on this option that can convert grafts to
+	replace refs.
 
 -l <pattern>::
 --list <pattern>::
diff --git a/contrib/convert-grafts-to-replace-refs.sh b/contrib/convert-grafts-to-replace-refs.sh
new file mode 100755
index 0000000..8472879
--- /dev/null
+++ b/contrib/convert-grafts-to-replace-refs.sh
@@ -0,0 +1,29 @@
+#!/bin/sh
+
+# You should execute this script in the repository where you
+# want to convert grafts to replace refs.
+
+die () {
+	echo >&2 "$@"
+	exit 1
+}
+
+GRAFTS_FILE="${GIT_DIR:-.git}/info/grafts"
+
+test -f "$GRAFTS_FILE" || die "Could not find graft file: '$GRAFTS_FILE'"
+
+grep '^[^# ]' "$GRAFTS_FILE" | while read definition
+do
+	test -n "$definition" && {
+		echo "Converting: $definition"
+		git replace --graft $definition ||
+			die "Conversion failed for: $definition"
+	}
+done
+
+mv "$GRAFTS_FILE" "$GRAFTS_FILE.bak" ||
+	die "Could not rename '$GRAFTS_FILE' to '$GRAFTS_FILE.bak'"
+
+echo "Success!"
+echo "All the grafts in '$GRAFTS_FILE' have been converted to replace refs!"
+echo "The grafts file '$GRAFTS_FILE' has been renamed: '$GRAFTS_FILE.bak'"
-- 
2.0.0.rc0.40.gd30ccc4

Re: [PATCH v3 1/4] replace: add --graft option

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> +static int create_graft(int argc, const char **argv, int force)
> +{
> +	unsigned char old[20], new[20];
> +	const char *old_ref = argv[0];
> +	struct commit *commit;
> +	struct strbuf buf = STRBUF_INIT;
> +	struct strbuf new_parents = STRBUF_INIT;
> +	const char *parent_start, *parent_end;
> +	int i;
> +
> +	if (get_sha1(old_ref, old) < 0)
> +		die(_("Not a valid object name: '%s'"), old_ref);
> +	commit = lookup_commit_or_die(old, old_ref);

Not a problem with this patch, but the above sequence somehow makes
me wonder if lookup-commit-or-die is a good API for this sort of
thing.  Wouldn't it be more natural if it took not "unsigned char
old[20]" but anything that would be understood by get_sha1()?

It could be that this particular caller is peculiar and all the
existing callers are happy, though.  I didn't "git grep" to spot
patterns in existing callers.

> +	if (read_sha1_commit(old, &buf))
> +		die(_("Invalid commit: '%s'"), old_ref);
> +	/* make sure the commit is not corrupt */
> +	if (parse_commit_buffer(commit, buf.buf, buf.len))
> +		die(_("Could not parse commit: '%s'"), old_ref);

It is unclear to me what you are trying to achieve with these two.
If the earlier lookup-commit has returned a commit object that has
already been parsed, parse_commit_buffer() would not check anything,
would it?

A typical sequence would look more like this:

    commit = lookup_commit(...);
    if (parse_commit(commit))
	oops there is an error;
    /* otherwise */
    use(commit->buffer);

without reading a commit object using low-level read-sha1-file
interface yourself, no?

Re: [PATCH v3 4/4] contrib: add convert-grafts-to-replace-refs.sh

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> diff --git a/contrib/convert-grafts-to-replace-refs.sh b/contrib/convert-grafts-to-replace-refs.sh
> new file mode 100755
> index 0000000..8472879
> --- /dev/null
> +++ b/contrib/convert-grafts-to-replace-refs.sh
> @@ -0,0 +1,29 @@
> +#!/bin/sh
> +
> +# You should execute this script in the repository where you
> +# want to convert grafts to replace refs.
> +
> +die () {
> +	echo >&2 "$@"
> +	exit 1
> +}

Don't we install git-sh-setup in GIT_EXEC_PATH, in order to allow
these third-party scripts to begin with:

        . $(git --exec-path)/git-sh-setup

just like our own scripted Porcelains?

> +GRAFTS_FILE="${GIT_DIR:-.git}/info/grafts"
> +
> +test -f "$GRAFTS_FILE" || die "Could not find graft file: '$GRAFTS_FILE'"
> +
> +grep '^[^# ]' "$GRAFTS_FILE" | while read definition
> +do

Format the above like so:

    grep '^[^# ]' "$GRAFTS_FILE" |
    while read definition
    do

which is easier to see what that "do" is doing.

> +	test -n "$definition" && {
> +		echo "Converting: $definition"
> +		git replace --graft $definition ||
> +			die "Conversion failed for: $definition"
> +	}

Hmph, why not if/then/fi?

Re: [PATCH v3 1/4] replace: add --graft option

[Christian Couder]

On Thu, Jun 5, 2014 at 11:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <chriscool@tuxfamily.org> writes:
>
>> +static int create_graft(int argc, const char **argv, int force)
>> +{
>> +     unsigned char old[20], new[20];
>> +     const char *old_ref = argv[0];
>> +     struct commit *commit;
>> +     struct strbuf buf = STRBUF_INIT;
>> +     struct strbuf new_parents = STRBUF_INIT;
>> +     const char *parent_start, *parent_end;
>> +     int i;
>> +
>> +     if (get_sha1(old_ref, old) < 0)
>> +             die(_("Not a valid object name: '%s'"), old_ref);
>> +     commit = lookup_commit_or_die(old, old_ref);
>
> Not a problem with this patch, but the above sequence somehow makes
> me wonder if lookup-commit-or-die is a good API for this sort of
> thing.  Wouldn't it be more natural if it took not "unsigned char
> old[20]" but anything that would be understood by get_sha1()?
>
> It could be that this particular caller is peculiar and all the
> existing callers are happy, though.  I didn't "git grep" to spot
> patterns in existing callers.

lookup_commit_or_die() looked like a good API to me because I saw that
it checked a lot of things and die in case of problems, which could
make the patch shorter.

>> +     if (read_sha1_commit(old, &buf))
>> +             die(_("Invalid commit: '%s'"), old_ref);
>> +     /* make sure the commit is not corrupt */
>> +     if (parse_commit_buffer(commit, buf.buf, buf.len))
>> +             die(_("Could not parse commit: '%s'"), old_ref);
>
> It is unclear to me what you are trying to achieve with these two.
> If the earlier lookup-commit has returned a commit object that has
> already been parsed, parse_commit_buffer() would not check anything,
> would it?

Yeah, you are right. I missed the fact that lookup_commit_or_die()
calls parse_object() which itself calls read_sha1_file() and then
parse_object_buffer() which calls parse_commit_buffer().

Here is a backtrace that shows this:

#0  parse_commit_buffer (item=0x8597b0, buffer=0x851730, size=228) at
commit.c:251
#1  0x00000000004fa215 in parse_object_buffer (sha1=0x7fffffffdbf0
"\t>A\247\235J\213\376<u\212\226\311^[\371\343^\330\234",
type=OBJ_COMMIT, size=228,
    buffer=0x851730, eaten_p=0x7fffffffdacc) at object.c:198
#2  0x00000000004fa50a in parse_object (sha1=0x7fffffffdbf0
"\t>A\247\235J\213\376<u\212\226\311^[\371\343^\330\234") at
object.c:264
#3  0x00000000004a89ef in lookup_commit_reference_gently
(sha1=0x7fffffffdbf0
"\t>A\247\235J\213\376<u\212\226\311^[\371\343^\330\234", quiet=0) at
commit.c:38
#4  0x00000000004a8a48 in lookup_commit_reference (sha1=0x7fffffffdbf0
"\t>A\247\235J\213\376<u\212\226\311^[\371\343^\330\234") at
commit.c:47
#5  0x00000000004a8a67 in lookup_commit_or_die (sha1=0x7fffffffdbf0
"\t>A\247\235J\213\376<u\212\226\311^[\371\343^\330\234",
    ref_name=0x7fffffffe465
"093e41a79d4a8bfe3c758a96c95e5bf9e35ed89c") at commit.c:52
#6  0x000000000047f89a in create_graft (argc=1, argv=0x7fffffffe130,
refdir=0x0, force=0) at builtin/replace.c:353
#7  0x000000000047ff71 in cmd_replace (argc=1, argv=0x7fffffffe130,
prefix=0x0) at builtin/replace.c:461
#8  0x0000000000405441 in run_builtin (p=0x7eee90, argc=3,
argv=0x7fffffffe130) at git.c:314
#9  0x000000000040563a in handle_builtin (argc=3, argv=0x7fffffffe130)
at git.c:487
#10 0x0000000000405754 in run_argv (argcp=0x7fffffffe01c,
argv=0x7fffffffe020) at git.c:533
#11 0x00000000004058f9 in main (argc=3, av=0x7fffffffe128) at git.c:616

> A typical sequence would look more like this:
>
>     commit = lookup_commit(...);
>     if (parse_commit(commit))
>         oops there is an error;
>     /* otherwise */
>     use(commit->buffer);
>
> without reading a commit object using low-level read-sha1-file
> interface yourself, no?

Yeah, or I could just rely on the fact that lookup_commit_or_die()
already parses the commit, with something like this:

      if (get_sha1(old_ref, old) < 0)
              die(_("Not a valid object name: '%s'"), old_ref);

      /* parse the commit buffer to make sure the commit is not corrupt */
      commit = lookup_commit_or_die(old, old_ref);

      /* find existing parents */
      parent_start = buf.buf;
      parent_start += 46; /* "tree " + "hex sha1" + "\n" */
      parent_end = parent_start;
...

Thanks,
Christian.

Re: [PATCH v3 1/4] replace: add --graft option

[Christian Couder]

On Fri, Jun 6, 2014 at 5:29 PM, Christian Couder
<christian.couder@gmail.com> wrote:
>
> Yeah, or I could just rely on the fact that lookup_commit_or_die()
> already parses the commit, with something like this:
>
>       if (get_sha1(old_ref, old) < 0)
>               die(_("Not a valid object name: '%s'"), old_ref);
>
>       /* parse the commit buffer to make sure the commit is not corrupt */
>       commit = lookup_commit_or_die(old, old_ref);
>
>       /* find existing parents */
>       parent_start = buf.buf;
>       parent_start += 46; /* "tree " + "hex sha1" + "\n" */
>       parent_end = parent_start;

This last part should be:

        /* find existing parents */
        strbuf_addstr(&buf, commit->buffer);
        parent_start = buf.buf;
        parent_start += 46; /* "tree " + "hex sha1" + "\n" */
        parent_end = parent_start;
...

I will send an updated patch series soon.

Re: [PATCH v3 4/4] contrib: add convert-grafts-to-replace-refs.sh

[Christian Couder]

On Thu, Jun 5, 2014 at 11:55 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <chriscool@tuxfamily.org> writes:
>
>> diff --git a/contrib/convert-grafts-to-replace-refs.sh b/contrib/convert-grafts-to-replace-refs.sh
>> new file mode 100755
>> index 0000000..8472879
>> --- /dev/null
>> +++ b/contrib/convert-grafts-to-replace-refs.sh
>> @@ -0,0 +1,29 @@
>> +#!/bin/sh
>> +
>> +# You should execute this script in the repository where you
>> +# want to convert grafts to replace refs.
>> +
>> +die () {
>> +     echo >&2 "$@"
>> +     exit 1
>> +}
>
> Don't we install git-sh-setup in GIT_EXEC_PATH, in order to allow
> these third-party scripts to begin with:
>
>         . $(git --exec-path)/git-sh-setup
>
> just like our own scripted Porcelains?

Ok, I will use that.

>> +GRAFTS_FILE="${GIT_DIR:-.git}/info/grafts"
>> +
>> +test -f "$GRAFTS_FILE" || die "Could not find graft file: '$GRAFTS_FILE'"
>> +
>> +grep '^[^# ]' "$GRAFTS_FILE" | while read definition
>> +do
>
> Format the above like so:
>
>     grep '^[^# ]' "$GRAFTS_FILE" |
>     while read definition
>     do
>
> which is easier to see what that "do" is doing.

Ok.

>> +     test -n "$definition" && {
>> +             echo "Converting: $definition"
>> +             git replace --graft $definition ||
>> +                     die "Conversion failed for: $definition"
>> +     }
>
> Hmph, why not if/then/fi?

Ok, I will use if/then/fi.

Thanks,
Christian.

Re: [PATCH v3 1/4] replace: add --graft option

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> On Thu, Jun 5, 2014 at 11:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Christian Couder <chriscool@tuxfamily.org> writes:
>>
>>> +static int create_graft(int argc, const char **argv, int force)
>>> +{
>>> +     unsigned char old[20], new[20];
>>> +     const char *old_ref = argv[0];
>>> +     struct commit *commit;
>>> +     struct strbuf buf = STRBUF_INIT;
>>> +     struct strbuf new_parents = STRBUF_INIT;
>>> +     const char *parent_start, *parent_end;
>>> +     int i;
>>> +
>>> +     if (get_sha1(old_ref, old) < 0)
>>> +             die(_("Not a valid object name: '%s'"), old_ref);
>>> +     commit = lookup_commit_or_die(old, old_ref);
>>
>> Not a problem with this patch, but the above sequence somehow makes
>> me wonder if lookup-commit-or-die is a good API for this sort of
>> thing.  Wouldn't it be more natural if it took not "unsigned char
>> old[20]" but anything that would be understood by get_sha1()?
>>
>> It could be that this particular caller is peculiar and all the
>> existing callers are happy, though.  I didn't "git grep" to spot
>> patterns in existing callers.
>
> lookup_commit_or_die() looked like a good API to me because I saw that
> it checked a lot of things and die in case of problems, which could
> make the patch shorter.

Perhaps I was vague.  "find the commit object and die if that object
is not properly formed" is a good thing.  I was referring to the
fact that you

    - first had to do get-sha1 yourself to turn the extended sha1
      you got from the user into a binary object name, and die with
      your own error message if the user input was malformed.

    - and then had to call lookup-commit-or-die to do the checking
      and let it die.

It would have been simpler for *this* particular codepath if we had
another helper function you can use like so:

	commit = lookup_commit_with_extended_sha1_or_die(old_ref);

which did the two-call sequence you handrolled above, and I was
wondering if other existing callers to lookup-commit-or-die wished
the same thing.

Re: [PATCH v3 1/4] replace: add --graft option

[Christian Couder]

On Fri, Jun 6, 2014 at 5:44 PM, Christian Couder
<christian.couder@gmail.com> wrote:
>
>         /* find existing parents */
>         strbuf_addstr(&buf, commit->buffer);

Unfortunately, it looks like the above will not work if the commit->buffer
contains an embedded NUL. I wonder if it is a real problem or not.

Re: [PATCH v3 1/4] replace: add --graft option

[Jeff King]

On Sun, Jun 08, 2014 at 08:49:45AM +0200, Christian Couder wrote:

> On Fri, Jun 6, 2014 at 5:44 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
> >
> >         /* find existing parents */
> >         strbuf_addstr(&buf, commit->buffer);
> 
> Unfortunately, it looks like the above will not work if the commit->buffer
> contains an embedded NUL. I wonder if it is a real problem or not.

I ran into a similar problem recently[1] and have been pondering
solutions to know the size of commit->buffer. What I've been come up
with is:

  1. Look up the object size via sha1_object_info. Besides being
     inefficient (which probably does not matter for you here, but might
     for using commit->buffer in a traversal), it strikes me as
     inelegant; is it possible for commit->buffer to ever disagree in
     size with the results of sha1_object_info, and if so, what happens?

  2. Add an extra member "len" to "struct commit". This is simple, but
     bloats "struct commit", which may have a performance impact for
     things like rev-list, where the field will be unused.

  3. Store the length of objects as a size_t, exactly sizeof(size_t)
     bytes before the object buffer. Provide a macro:

       #define OBJECT_SIZE(buf) (((size_t *)(buf))[-1])

     to access it. Most callers can just use the buffer as-is, but
     anybody who calls free() would need to be adjusted to use a special
     "object_free".

  4. Keep a static commit_slab that points to the length for each parsed
     commit. We pay the same memory cost as (2), but as it's not part of
     the struct, the cache effects are minimized.

-Peff

[1] http://article.gmane.org/gmane.comp.version-control.git/250480

Re: [PATCH v3 1/4] replace: add --graft option

[Jeff King]

On Sun, Jun 08, 2014 at 07:23:33AM -0400, Jeff King wrote:

>   4. Keep a static commit_slab that points to the length for each parsed
>      commit. We pay the same memory cost as (2), but as it's not part of
>      the struct, the cache effects are minimized.

I think I favor this solution, which would look something like this:

-- >8 --
Subject: [PATCH] commit: add slab for commit buffer size

We store the commit object buffer for later reuse as
commit->buffer. However, since we store only a pointer, we
must treat the result as a NUL-terminated string. This is
generally OK for pretty-printing, but could be a problem for
other uses.

Adding a "len" member to "struct commit" would solve this,
but at the cost of bloating the struct even for callers who
do not care about the size or buffer (e.g., traversals like
rev-list or merge-base). Instead, let's use a commit_slab so
that the memory is used only when save_commit_buffer is in
effect (and even then, it should have less cache impact on
most uses of "struct commit").

Signed-off-by: Jeff King <peff@peff.net>
---
I think it would make sense to actually take this one step further,
though, and move commit->buffer into the slab, as well. That has two
advantages:

  1. It further decreases the size of "struct commit" for callers who do
     not use save_commit_buffer.

  2. It ensures that no new callers crop up who set "commit->buffer" but
     to not save the size in the slab (you can see in the patch below
     that I had to modify builtin/blame.c, which (ab)uses
     commit->buffer).

It would be more disruptive to existing callers, but I think the end
result would be pretty clean. The API would be something like:

  /* attach buffer to commit */
  set_commit_buffer(struct commit *, void *buf, unsigned long size);

  /* get buffer, either from slab cache or by calling read_sha1_file */
  void *get_commit_buffer(struct commit *, unsigned long *size);

  /* free() an allocated buffer from above, noop for cached buffer */
  void unused_commit_buffer(struct commit *, void *buf);

  /* drop saved commit buffer to free memory */
  void free_commit_buffer(struct commit *);

The "get" function would serve the existing callers in pretty.c, as well
as the one I'm adding elsewhere in show_signature. And it should work as
a drop-in read_sha1_file/free replacement for you here.

 builtin/blame.c |  2 +-
 commit.c        | 13 ++++++++++++-
 commit.h        |  1 +
 object.c        |  2 +-
 4 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index a52a279..1945ea4 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2313,7 +2313,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
 		    ident, ident, path,
 		    (!contents_from ? path :
 		     (!strcmp(contents_from, "-") ? "standard input" : contents_from)));
-	commit->buffer = strbuf_detach(&msg, NULL);
+	set_commit_buffer(commit, strbuf_detach(&msg, NULL), msg.len);
 
 	if (!contents_from || strcmp("-", contents_from)) {
 		struct stat st;
diff --git a/commit.c b/commit.c
index f479331..71143ed 100644
--- a/commit.c
+++ b/commit.c
@@ -302,6 +302,17 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s
 	return 0;
 }
 
+define_commit_slab(commit_size_slab, unsigned long);
+static struct commit_size_slab commit_size;
+
+void set_commit_buffer(struct commit *item, void *buffer, unsigned long size)
+{
+	if (!commit_size.stride)
+		init_commit_size_slab(&commit_size);
+	*commit_size_slab_at(&commit_size, item) = size;
+	item->buffer = buffer;
+}
+
 int parse_commit(struct commit *item)
 {
 	enum object_type type;
@@ -324,7 +335,7 @@ int parse_commit(struct commit *item)
 	}
 	ret = parse_commit_buffer(item, buffer, size);
 	if (save_commit_buffer && !ret) {
-		item->buffer = buffer;
+		set_commit_buffer(item, buffer, size);
 		return 0;
 	}
 	free(buffer);
diff --git a/commit.h b/commit.h
index a9f177b..7704ab2 100644
--- a/commit.h
+++ b/commit.h
@@ -48,6 +48,7 @@ struct commit *lookup_commit_reference_by_name(const char *name);
 struct commit *lookup_commit_or_die(const unsigned char *sha1, const char *ref_name);
 
 int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long size);
+void set_commit_buffer(struct commit *item, void *buffer, unsigned long size);
 int parse_commit(struct commit *item);
 void parse_commit_or_die(struct commit *item);
 
diff --git a/object.c b/object.c
index 57a0890..c1c6a24 100644
--- a/object.c
+++ b/object.c
@@ -198,7 +198,7 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t
 			if (parse_commit_buffer(commit, buffer, size))
 				return NULL;
 			if (!commit->buffer) {
-				commit->buffer = buffer;
+				set_commit_buffer(commit, buffer, size);
 				*eaten_p = 1;
 			}
 			obj = &commit->object;
-- 
2.0.0.729.g520999f

Re: [PATCH v3 1/4] replace: add --graft option

[Jeff King]

On Sun, Jun 08, 2014 at 08:04:39AM -0400, Jeff King wrote:

> diff --git a/builtin/blame.c b/builtin/blame.c
> index a52a279..1945ea4 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -2313,7 +2313,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
>  		    ident, ident, path,
>  		    (!contents_from ? path :
>  		     (!strcmp(contents_from, "-") ? "standard input" : contents_from)));
> -	commit->buffer = strbuf_detach(&msg, NULL);
> +	set_commit_buffer(commit, strbuf_detach(&msg, NULL), msg.len);

Side note: this is wrong, as the fake commit created by blame here does
not have its "index" field set. This is a bug waiting to happen the
first time somebody uses a slab in builtin/blame.c. It looks like
merge-recursive does the same thing in make_virtual_commit.

We probably want to provide a function to allocate a commit, including
the index, and use it consistently.

I'll try to work up a series doing that, and the fuller slab API I
mentioned in the previous message, but probably not until tomorrow.

-Peff

Re: [PATCH v3 1/4] replace: add --graft option

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I think it would make sense to actually take this one step further,
> though, and move commit->buffer into the slab, as well. That has two
> advantages:
>
>   1. It further decreases the size of "struct commit" for callers who do
>      not use save_commit_buffer.
>
>   2. It ensures that no new callers crop up who set "commit->buffer" but
>      to not save the size in the slab (you can see in the patch below
>      that I had to modify builtin/blame.c, which (ab)uses
>      commit->buffer).
>
> It would be more disruptive to existing callers, but I think the end
> result would be pretty clean. The API would be something like:
>
>   /* attach buffer to commit */
>   set_commit_buffer(struct commit *, void *buf, unsigned long size);
>
>   /* get buffer, either from slab cache or by calling read_sha1_file */
>   void *get_commit_buffer(struct commit *, unsigned long *size);
>
>   /* free() an allocated buffer from above, noop for cached buffer */
>   void unused_commit_buffer(struct commit *, void *buf);
>
>   /* drop saved commit buffer to free memory */
>   void free_commit_buffer(struct commit *);
>
> The "get" function would serve the existing callers in pretty.c, as well
> as the one I'm adding elsewhere in show_signature. And it should work as
> a drop-in read_sha1_file/free replacement for you here.

This solution has the kind of niceness to make everybody (certainly
including me) who has been involved in the code path should say "why
didn't I think of that!" ;-)

Re: [PATCH v3 1/4] replace: add --graft option

[Christian Couder]

On Sun, Jun 8, 2014 at 10:18 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> On Sat, Jun 7, 2014 at 11:49 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
>>
>> On Fri, Jun 6, 2014 at 5:44 PM, Christian Couder
>> <christian.couder@gmail.com> wrote:
>> >
>> >         /* find existing parents */
>> >         strbuf_addstr(&buf, commit->buffer);
>>
>> Unfortunately, it looks like the above will not work if the commit->buffer
>> contains an embedded NUL. I wonder if it is a real problem or not.
>
> Yes, it is a real problem (there was another thread on this regarding the
> code path that verifies GPG signature on the commit itself), which
> incidentally reminds us to another thing to think about in your patch as
> well. I *think* you shoud drop the GPG signature on the commit itself, and
> you also should drop the merge-tag of a parent you are not going to keep,
> but should keep the merge-tag of a parent you are keeping.

In the v5 of the patch series, I now drop the GPG signature on the commit
itself.

Now, after having read the recent thread about "git verify-commit", I understand
that you also want me to drop the signature of a tag that was merged, because
such signatures are added to the commit message.

But I wonder how far should we go in this path. For example merge commits
have a title like "Merge branch 'dev'" or "Merge tag 'stuff'", but
this does not make
sense any more if the replacement commit drops the parent corresponding to
'dev' or 'stuff'. And I don't think we should change the commit title.

Thanks,
Christian.

Re: [PATCH v3 1/4] replace: add --graft option

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> Now, after having read the recent thread about "git verify-commit", I understand
> that you also want me to drop the signature of a tag that was merged, because
> such signatures are added to the commit message.

Huh?  I am not sure if I follow.  Perhaps we are talking about
different things?

When you are preparing a replacement for an existing commit that
merges a signed tag, there are two cases:

 - The replacement commit still merges the same signed tag; or

 - The replacement commit does not merge that signed tag (it may
   become a single-parent commit, or it may stay to be a merge but
   merges a different commit on the side branch).

In the former, it would be sensible to keep the "mergetag" and
propagate it to the replacement; it is a signature over the tip of
the side branch being merged by the original (and the replacement)
merge, and the replacement will not affect the validity of the
signature at all.  In the latter, we do want to drop the "mergetag"
for the parent you are losing in the replacement, because by
definition it will be irrelevant.

Re: [PATCH v3 1/4] replace: add --graft option

[Christian Couder]

On Mon, Jun 30, 2014 at 8:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> Now, after having read the recent thread about "git verify-commit", I understand
>> that you also want me to drop the signature of a tag that was merged, because
>> such signatures are added to the commit message.
>
> Huh?  I am not sure if I follow.  Perhaps we are talking about
> different things?

I think we are talking about the same thing but I might not have been clear.

> When you are preparing a replacement for an existing commit that
> merges a signed tag, there are two cases:
>
>  - The replacement commit still merges the same signed tag; or
>
>  - The replacement commit does not merge that signed tag (it may
>    become a single-parent commit, or it may stay to be a merge but
>    merges a different commit on the side branch).
>
> In the former, it would be sensible to keep the "mergetag" and
> propagate it to the replacement; it is a signature over the tip of
> the side branch being merged by the original (and the replacement)
> merge, and the replacement will not affect the validity of the
> signature at all.

Ok, this is what is done right now by the patch series.

> In the latter, we do want to drop the "mergetag"
> for the parent you are losing in the replacement, because by
> definition it will be irrelevant.

Yeah, it might be a good idea to drop the "mergetag", but note that
anyway such a commit probably has a title like "Merge tag '<tag>'" and
we won't automatically change this title and this title will be wrong
(because we are not merging anymore this tag).

So anyway in this case, --graft will do something that is not good. So
it might be better in this case to just error out and say that it
would be better to use --edit instead of --graft.