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