git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 0/3] Add --graft option to git replace
@ 2014-05-22 21:33 Christian Couder
  2014-05-22 21:33 ` [PATCH v1 1/3] replace: add --graft option Christian Couder
                   ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: Christian Couder @ 2014-05-22 21:33 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King, Michael Haggerty

Here is a small patch series to implement:

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

The changes since the RFC/PATCH are the following:

- in patch 1/3, parse_commit_buffer() is now used to
  make sure <commit> is not corrupt
- patch 2/3 add some tests
- patch 3/3 add some documentation

About the documentation, maybe we should add that --graft
can now be used instead of grafts in .git/info/grafts,
and maybe we could add an example that shows how it can
be done.

Christian Couder (3):
  replace: add --graft option
  replace: add test for --graft
  Documentation: replace: add --graft option

 Documentation/git-replace.txt |  8 +++++
 builtin/replace.c             | 84 ++++++++++++++++++++++++++++++++++++++++++-
 t/t6050-replace.sh            | 12 +++++++
 3 files changed, 103 insertions(+), 1 deletion(-)

-- 
1.9.rc0.17.g651113e

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v1 1/3] replace: add --graft option
  2014-05-22 21:33 [PATCH v1 0/3] Add --graft option to git replace Christian Couder
@ 2014-05-22 21:33 ` Christian Couder
  2014-05-23 19:27   ` Junio C Hamano
  2014-05-23 19:51   ` Jeff King
  2014-05-22 21:33 ` [PATCH v1 2/3] replace: add test for --graft Christian Couder
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 15+ messages in thread
From: Christian Couder @ 2014-05-22 21:33 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King, Michael Haggerty

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

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",
-- 
1.9.rc0.17.g651113e

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v1 2/3] replace: add test for --graft
  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-22 21:33 ` Christian Couder
  2014-05-22 21:33 ` [PATCH v1 3/3] Documentation: replace: add --graft option Christian Couder
  2014-05-23 16:59 ` [PATCH v1 0/3] Add --graft option to git replace Junio C Hamano
  3 siblings, 0 replies; 15+ messages in thread
From: Christian Couder @ 2014-05-22 21:33 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King, Michael Haggerty

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
-- 
1.9.rc0.17.g651113e

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v1 3/3] Documentation: replace: add --graft option
  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-22 21:33 ` [PATCH v1 2/3] replace: add test for --graft Christian Couder
@ 2014-05-22 21:33 ` Christian Couder
  2014-05-23 17:06   ` Jakub Narębski
  2014-05-23 16:59 ` [PATCH v1 0/3] Add --graft option to git replace Junio C Hamano
  3 siblings, 1 reply; 15+ messages in thread
From: Christian Couder @ 2014-05-22 21:33 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King, Michael Haggerty

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
-- 
1.9.rc0.17.g651113e

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 0/3] Add --graft option to git replace
  2014-05-22 21:33 [PATCH v1 0/3] Add --graft option to git replace Christian Couder
                   ` (2 preceding siblings ...)
  2014-05-22 21:33 ` [PATCH v1 3/3] Documentation: replace: add --graft option Christian Couder
@ 2014-05-23 16:59 ` Junio C Hamano
  2014-05-27 19:05   ` Christian Couder
  3 siblings, 1 reply; 15+ messages in thread
From: Junio C Hamano @ 2014-05-23 16:59 UTC (permalink / raw)
  To: Christian Couder; +Cc: git, Jeff King, Michael Haggerty

Christian Couder <chriscool@tuxfamily.org> writes:

> Here is a small patch series to implement:
>
> 	git replace [-f] --graft <commit> [<parent>...]
>
> The changes since the RFC/PATCH are the following:
>
> - in patch 1/3, parse_commit_buffer() is now used to
>   make sure <commit> is not corrupt
> - patch 2/3 add some tests
> - patch 3/3 add some documentation
>
> About the documentation, maybe we should add that --graft
> can now be used instead of grafts in .git/info/grafts,
> and maybe we could add an example that shows how it can
> be done.

Or a procedure that reads .git/info/grafts, converts it to a set of
replacements and drops .git/info/grafts.  A sample script could be
thrown in to contrib/ somewhere as "convert-graft-to-replace.sh".

> Christian Couder (3):
>   replace: add --graft option
>   replace: add test for --graft
>   Documentation: replace: add --graft option
>
>  Documentation/git-replace.txt |  8 +++++
>  builtin/replace.c             | 84 ++++++++++++++++++++++++++++++++++++++++++-
>  t/t6050-replace.sh            | 12 +++++++
>  3 files changed, 103 insertions(+), 1 deletion(-)

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 3/3] Documentation: replace: add --graft option
  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
  0 siblings, 1 reply; 15+ messages in thread
From: Jakub Narębski @ 2014-05-23 17:06 UTC (permalink / raw)
  To: Christian Couder, Junio C Hamano; +Cc: git, Jeff King, Michael Haggerty

W dniu 2014-05-22 23:33, Christian Couder pisze:

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

Here I think you can add the graft replacing example:

   cat .git/info/grafts | while read line
   do git replace --graft $line; done

-- 
Jakub Narębski

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 3/3] Documentation: replace: add --graft option
  2014-05-23 17:06   ` Jakub Narębski
@ 2014-05-23 18:26     ` Junio C Hamano
  0 siblings, 0 replies; 15+ messages in thread
From: Junio C Hamano @ 2014-05-23 18:26 UTC (permalink / raw)
  To: Jakub Narębski; +Cc: Christian Couder, git, Jeff King, Michael Haggerty

Jakub Narębski <jnareb@gmail.com> writes:

> W dniu 2014-05-22 23:33, Christian Couder pisze:
>
>> +--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
>
> Here I think you can add the graft replacing example:
>
>   cat .git/info/grafts | while read line
>   do git replace --graft $line; done

Do not cat a single file into a pipeline.

    while read definition
    do
    	git replace --graft $definition
    done <"${GIT_DIR:-.git}/info/grafts"

or something.  You might also have to be careful to use "read -r"
and/or avoid feeding a comment line (if info/grafts supports it) to
the command inside do ... done, but I didn't check what the graft
reading code does myself ;-)

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 1/3] replace: add --graft option
  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
  1 sibling, 0 replies; 15+ messages in thread
From: Junio C Hamano @ 2014-05-23 19:27 UTC (permalink / raw)
  To: Christian Couder; +Cc: git, Jeff King, Michael Haggerty

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.

Shucks.  An older version of this is already in 'next'.  I'll
postpone queuing the rerolled one until post-release when we will
rewind 'next'.

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

Don't cat a single file into a pipeline.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 1/3] replace: add --graft option
  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
  2014-06-01 16:06     ` Christian Couder
  1 sibling, 2 replies; 15+ messages in thread
From: Jeff King @ 2014-05-23 19:51 UTC (permalink / raw)
  To: Christian Couder; +Cc: Junio C Hamano, git, Michael Haggerty

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

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

-Peff

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 1/3] replace: add --graft option
  2014-05-23 19:51   ` Jeff King
@ 2014-05-23 20:05     ` Junio C Hamano
  2014-05-23 20:28       ` Jeff King
  2014-06-01 16:06     ` Christian Couder
  1 sibling, 1 reply; 15+ messages in thread
From: Junio C Hamano @ 2014-05-23 20:05 UTC (permalink / raw)
  To: Jeff King; +Cc: Christian Couder, git, Michael Haggerty

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 1/3] replace: add --graft option
  2014-05-23 20:05     ` Junio C Hamano
@ 2014-05-23 20:28       ` Jeff King
  2014-05-23 21:22         ` Junio C Hamano
  0 siblings, 1 reply; 15+ messages in thread
From: Jeff King @ 2014-05-23 20:28 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Christian Couder, git, Michael Haggerty

On Fri, May 23, 2014 at 01:05:40PM -0700, Junio C Hamano wrote:

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

Thanks, I missed the space trimming. I think:

  grep '^[^# \t]'

would be enough, though I am not sure that "\t" is portable over a raw
tab.

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

Yes, exactly.

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

Sounds familiar:

  http://article.gmane.org/gmane.comp.version-control.git/249575

-Peff

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 1/3] replace: add --graft option
  2014-05-23 20:28       ` Jeff King
@ 2014-05-23 21:22         ` Junio C Hamano
  2014-05-23 22:59           ` Eric Sunshine
  0 siblings, 1 reply; 15+ messages in thread
From: Junio C Hamano @ 2014-05-23 21:22 UTC (permalink / raw)
  To: Jeff King; +Cc: Christian Couder, git, Michael Haggerty

Jeff King <peff@peff.net> writes:

> On Fri, May 23, 2014 at 01:05:40PM -0700, Junio C Hamano wrote:
>
>> 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.
>
> Thanks, I missed the space trimming. I think:
>
>   grep '^[^# \t]'
>
> would be enough, though I am not sure that "\t" is portable over a raw
> tab.

Perhaps.  That would filter out an empty line as well, which would
be good.

>> Perhaps a more future-proof way to write Christian's code may be:
>> ...
>> which may not be too bad.
>
> Sounds familiar:
>
>   http://article.gmane.org/gmane.comp.version-control.git/249575

Yup, instead of having to memmove() repeatedly, copying into it
while skipping 'parent ' lines may be better.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 1/3] replace: add --graft option
  2014-05-23 21:22         ` Junio C Hamano
@ 2014-05-23 22:59           ` Eric Sunshine
  0 siblings, 0 replies; 15+ messages in thread
From: Eric Sunshine @ 2014-05-23 22:59 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, Christian Couder, Git List, Michael Haggerty

On Fri, May 23, 2014 at 5:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> On Fri, May 23, 2014 at 01:05:40PM -0700, Junio C Hamano wrote:
>>
>>> 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.
>>
>> Thanks, I missed the space trimming. I think:
>>
>>   grep '^[^# \t]'
>>
>> would be enough, though I am not sure that "\t" is portable over a raw
>> tab.

'grep' does not recognize "\t" on BSD or derivatives, such as Mac OS
X. (Nor does 'sed'.)

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 0/3] Add --graft option to git replace
  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
  0 siblings, 0 replies; 15+ messages in thread
From: Christian Couder @ 2014-05-27 19:05 UTC (permalink / raw)
  To: gitster; +Cc: git, peff, mhagger

From: Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v1 0/3] Add --graft option to git replace
Date: Fri, 23 May 2014 09:59:05 -0700

> Christian Couder <chriscool@tuxfamily.org> writes:
> 
>> Here is a small patch series to implement:
>>
>> 	git replace [-f] --graft <commit> [<parent>...]
>>
>> The changes since the RFC/PATCH are the following:
>>
>> - in patch 1/3, parse_commit_buffer() is now used to
>>   make sure <commit> is not corrupt
>> - patch 2/3 add some tests
>> - patch 3/3 add some documentation
>>
>> About the documentation, maybe we should add that --graft
>> can now be used instead of grafts in .git/info/grafts,
>> and maybe we could add an example that shows how it can
>> be done.
> 
> Or a procedure that reads .git/info/grafts, converts it to a set of
> replacements and drops .git/info/grafts.  A sample script could be
> thrown in to contrib/ somewhere as "convert-graft-to-replace.sh".

Ok, I just sent a patch that adds such a sample script.

Thanks,
Christian.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 1/3] replace: add --graft option
  2014-05-23 19:51   ` Jeff King
  2014-05-23 20:05     ` Junio C Hamano
@ 2014-06-01 16:06     ` Christian Couder
  1 sibling, 0 replies; 15+ messages in thread
From: Christian Couder @ 2014-06-01 16:06 UTC (permalink / raw)
  To: peff; +Cc: gitster, git, mhagger

From: Jeff King <peff@peff.net>

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

Thanks, I used something like that in the contrib script.
 
>> +	/* 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.

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2014-06-01 16:06 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).