[PATCH 0/6] Add git-rewrite-commits v2

[PATCH 0/6] Add git-rewrite-commits v2

[skimo]

From: Sven Verdoolaege <skimo@kotnet.org>

[PATCH 1/6] revision: allow selection of commits that do not match a pattern
[PATCH 2/6] export get_short_sha1
[PATCH 3/6] Define ishex(x) in git-compat-util.h
[PATCH 4/6] refs.c: lock cached_refs during for_each_ref
[PATCH 5/6] revision: mark commits that didn't match a pattern for later use
[PATCH 6/6] Add git-rewrite-commits

The first is fairly independent, but it is used in the tests
of the last patch and may be replaced by Johannes' extended patch.
The next three should be fairly uncontroversial.
The fifth may be considered a waste of a precious bit.
If so, any suggestions for other ways of passing on this information
are welcomed.
The sixth contains the actual git-rewrite-commits builtin.
My main motivation was that cg-admin-rewritehist doesn't
change the SHA1's in commit message and I don't like shell
programming.

The main difference with the previous series is that
the options are now called --index-filter and --commit-filter
instead of --index-map and --commit-map.

The commit filter should now produce a list of commit SHA1
instead of the modified raw commit.
The refs are now rewritten at the very end, which should
be safer if anything goes wrong along the way and obviates
the need for the call to add_ref_decoration of the previous
series.

Most of the changes were requested by Johannes Schindelin.
I'm sure he'll let me know if I forgot anything.
In a follow-up patch (series), he will add more helper functions
for the filters.

skimo

[PATCH 1/6] revision: allow selection of commits that do not match a pattern

[skimo]

From: Sven Verdoolaege <skimo@kotnet.org>

We do this by maintaining two lists of patterns, one for
those that should match and one for those that should not match.

A negative pattern is specified by putting a '!' in front.
For example, to show the commits of Jakub Narebski that
are not about gitweb, you'd do a

	git log --author='Narebski' --grep='!gitweb' --all-match

As an added bonus, this patch also documents --all-match.

Signed-off-by: Sven Verdoolaege <skimo@kotnet.org>
Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/git-rev-list.txt |   17 ++++++++++
 revision.c                     |   64 +++++++++++++++++++++++++++++++++------
 revision.h                     |    1 +
 3 files changed, 72 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt
index 20dcac6..c462f5d 100644
--- a/Documentation/git-rev-list.txt
+++ b/Documentation/git-rev-list.txt
@@ -214,11 +214,19 @@ limiting may be applied.
 
 	Limit the commits output to ones with author/committer
 	header lines that match the specified pattern (regular expression).
+	A pattern starting with a '!' will show only commits that do
+	not match the remainder of the pattern.
+	To match lines starting with '!', escape the initial '!'
+	with a backslash.
 
 --grep='pattern'::
 
 	Limit the commits output to ones with log message that
 	matches the specified pattern (regular expression).
+	A pattern starting with a '!' will show only commits that do
+	not match the remainder of the pattern.
+	To match lines starting with '!', escape the initial '!'
+	with a backslash.
 
 --regexp-ignore-case::
 
@@ -229,6 +237,15 @@ limiting may be applied.
 	Consider the limiting patterns to be extended regular expressions
 	instead of the default basic regular expressions.
 
+--all-match::
+
+	Without this option, a commit is shown if any of the
+	(positive or negative) patterns matches, i.e., there
+	is at least one positive match or not all of the negative
+	patterns match.  With this options, a commit is only
+	shown if all of the patterns match, i.e., all positive
+	patterns match and no negative pattern matches.
+
 --remove-empty::
 
 	Stop when a given path disappears from the tree.
diff --git a/revision.c b/revision.c
index 27cce09..38061f9 100644
--- a/revision.c
+++ b/revision.c
@@ -826,34 +826,50 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,
 	return 0;
 }
 
-static void add_grep(struct rev_info *revs, const char *ptn, enum grep_pat_token what)
+static void add_grep(struct rev_info *revs, const char *ptn,
+		    enum grep_pat_token what)
 {
-	if (!revs->grep_filter) {
+	int negated = 0;
+	struct grep_opt **filter;
+
+	if (ptn[0] == '\\' && ptn[1] == '!')
+		ptn++;
+	if (*ptn == '!') {
+		negated = 1;
+		ptn++;
+	}
+	filter = negated ? &revs->grep_neg_filter : &revs->grep_filter;
+	if (!*filter) {
 		struct grep_opt *opt = xcalloc(1, sizeof(*opt));
 		opt->status_only = 1;
 		opt->pattern_tail = &(opt->pattern_list);
 		opt->regflags = REG_NEWLINE;
-		revs->grep_filter = opt;
+		*filter = opt;
 	}
-	append_grep_pattern(revs->grep_filter, ptn,
-			    "command line", 0, what);
+	append_grep_pattern(*filter, ptn, "command line", 0, what);
 }
 
-static void add_header_grep(struct rev_info *revs, const char *field, const char *pattern)
+static void add_header_grep(struct rev_info *revs, const char *field,
+			    const char *pattern)
 {
 	char *pat;
-	const char *prefix;
+	const char *prefix, *negated;
 	int patlen, fldlen;
 
 	fldlen = strlen(field);
 	patlen = strlen(pattern);
 	pat = xmalloc(patlen + fldlen + 10);
+	negated = "";
+	if (*pattern == '!') {
+		negated = "!";
+		pattern++;
+	}
 	prefix = ".*";
 	if (*pattern == '^') {
 		prefix = "";
 		pattern++;
 	}
-	sprintf(pat, "^%s %s%s", field, prefix, pattern);
+	sprintf(pat, "%s^%s %s%s", negated, field, prefix, pattern);
 	add_grep(revs, pat, GREP_PATTERN_HEAD);
 }
 
@@ -1217,6 +1233,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
 	if (revs->grep_filter)
 		revs->grep_filter->regflags |= regflags;
 
+	if (revs->grep_neg_filter)
+		revs->grep_neg_filter->regflags |= regflags;
+
 	if (show_merge)
 		prepare_show_merge(revs);
 	if (def && !revs->pending.nr) {
@@ -1254,6 +1273,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
 		compile_grep_patterns(revs->grep_filter);
 	}
 
+	if (revs->grep_neg_filter) {
+		revs->grep_neg_filter->all_match = !all_match;
+		compile_grep_patterns(revs->grep_neg_filter);
+	}
+
 	return left;
 }
 
@@ -1352,11 +1376,31 @@ static int rewrite_parents(struct rev_info *revs, struct commit *commit)
 	return 0;
 }
 
+/*
+ * If all_match is set, then a commit matches if all the positive
+ * patterns match and not one of the negative patterns matches.
+ * If all_match is not set, then a commit matches if at least one
+ * of the positive patterns matches or not all of the negative
+ * patterns match.
+ */
 static int commit_match(struct commit *commit, struct rev_info *opt)
 {
-	if (!opt->grep_filter)
+	int pos_match, all_match;
+
+	pos_match = !opt->grep_filter ||
+		    grep_buffer(opt->grep_filter,
+			   NULL, /* we say nothing, not even filename */
+			   commit->buffer, strlen(commit->buffer));
+	if (!opt->grep_neg_filter)
+		return pos_match;
+
+	all_match = !opt->grep_neg_filter->all_match;
+	if (!all_match && opt->grep_filter && pos_match)
 		return 1;
-	return grep_buffer(opt->grep_filter,
+	if (all_match && !pos_match)
+		return 0;
+
+	return !grep_buffer(opt->grep_neg_filter,
 			   NULL, /* we say nothing, not even filename */
 			   commit->buffer, strlen(commit->buffer));
 }
diff --git a/revision.h b/revision.h
index f46b4d5..9728d4c 100644
--- a/revision.h
+++ b/revision.h
@@ -84,6 +84,7 @@ struct rev_info {
 
 	/* Filter by commit log message */
 	struct grep_opt	*grep_filter;
+	struct grep_opt	*grep_neg_filter;
 
 	/* special limits */
 	int skip_count;
-- 
1.5.3.rc0.100.ge60b4

[PATCH 2/6] export get_short_sha1

[skimo]

From: Sven Verdoolaege <skimo@kotnet.org>

Sometimes it is useful to check whether a given string
is exactly a short SHA1.

Signed-off-by: Sven Verdoolaege <skimo@kotnet.org>
---
 cache.h     |    1 +
 sha1_name.c |    3 +--
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index e64071e..8fda8ee 100644
--- a/cache.h
+++ b/cache.h
@@ -391,6 +391,7 @@ static inline unsigned int hexval(unsigned char c)
 
 extern int get_sha1(const char *str, unsigned char *sha1);
 extern int get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode);
+extern int get_short_sha1(const char *name, int len, unsigned char *sha1, int quietly);
 extern int get_sha1_hex(const char *hex, unsigned char *sha1);
 extern char *sha1_to_hex(const unsigned char *sha1);	/* static buffer result! */
 extern int read_ref(const char *filename, unsigned char *sha1);
diff --git a/sha1_name.c b/sha1_name.c
index 858f08c..0bed79d 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -154,8 +154,7 @@ static int find_unique_short_object(int len, char *canonical,
 	return 0;
 }
 
-static int get_short_sha1(const char *name, int len, unsigned char *sha1,
-			  int quietly)
+int get_short_sha1(const char *name, int len, unsigned char *sha1, int quietly)
 {
 	int i, status;
 	char canonical[40];
-- 
1.5.3.rc0.100.ge60b4

[PATCH 3/6] Define ishex(x) in git-compat-util.h

[skimo]

From: Sven Verdoolaege <skimo@kotnet.org>

Signed-off-by: Sven Verdoolaege <skimo@kotnet.org>
---
 builtin-name-rev.c |    1 -
 ctype.c            |    5 +++--
 git-compat-util.h  |    5 ++++-
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/builtin-name-rev.c b/builtin-name-rev.c
index 61eba34..b2ac40c 100644
--- a/builtin-name-rev.c
+++ b/builtin-name-rev.c
@@ -233,7 +233,6 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
 				break;
 
 			for (p_start = p; *p; p++) {
-#define ishex(x) (isdigit((x)) || ((x) >= 'a' && (x) <= 'f'))
 				if (!ishex(*p))
 					forty = 0;
 				else if (++forty == 40 &&
diff --git a/ctype.c b/ctype.c
index ee06eb7..97b5724 100644
--- a/ctype.c
+++ b/ctype.c
@@ -6,7 +6,8 @@
 #include "cache.h"
 
 #define SS GIT_SPACE
-#define AA GIT_ALPHA
+#define HA GIT_HEXAL
+#define AA GIT_OTHAL
 #define DD GIT_DIGIT
 
 unsigned char sane_ctype[256] = {
@@ -16,7 +17,7 @@ unsigned char sane_ctype[256] = {
 	DD, DD, DD, DD, DD, DD, DD, DD, DD, DD,  0,  0,  0,  0,  0,  0,		/* 48-15 */
 	 0, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA,		/* 64-15 */
 	AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA,  0,  0,  0,  0,  0,		/* 80-15 */
-	 0, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA,		/* 96-15 */
+	 0, HA, HA, HA, HA, HA, HA, AA, AA, AA, AA, AA, AA, AA, AA, AA,		/* 96-15 */
 	AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA,  0,  0,  0,  0,  0,		/* 112-15 */
 	/* Nothing in the 128.. range */
 };
diff --git a/git-compat-util.h b/git-compat-util.h
index 362e040..1a36f4c 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -325,12 +325,15 @@ static inline int has_extension(const char *filename, const char *ext)
 extern unsigned char sane_ctype[256];
 #define GIT_SPACE 0x01
 #define GIT_DIGIT 0x02
-#define GIT_ALPHA 0x04
+#define GIT_HEXAL 0x04
+#define GIT_OTHAL 0x08
+#define GIT_ALPHA (GIT_HEXAL | GIT_OTHAL)
 #define sane_istest(x,mask) ((sane_ctype[(unsigned char)(x)] & (mask)) != 0)
 #define isspace(x) sane_istest(x,GIT_SPACE)
 #define isdigit(x) sane_istest(x,GIT_DIGIT)
 #define isalpha(x) sane_istest(x,GIT_ALPHA)
 #define isalnum(x) sane_istest(x,GIT_ALPHA | GIT_DIGIT)
+#define ishex(x) sane_istest(x,GIT_HEXAL | GIT_DIGIT)
 #define tolower(x) sane_case((unsigned char)(x), 0x20)
 #define toupper(x) sane_case((unsigned char)(x), 0)
 
-- 
1.5.3.rc0.100.ge60b4

[PATCH 4/6] refs.c: lock cached_refs during for_each_ref

[skimo]

From: Sven Verdoolaege <skimo@kotnet.org>

If the function called by for_each_ref modifies a ref in any way,
the cached_refs that for_each_ref was looping over would be
removed, resulting in undefined behavior.

This patch prevents the cached_refs from being removed
while for_each_ref is still iterating over them.

Signed-off-by: Sven Verdoolaege <skimo@kotnet.org>
---
 refs.c |   37 +++++++++++++++++++++++++++++++++----
 1 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index 4dc7e8b..e710903 100644
--- a/refs.c
+++ b/refs.c
@@ -153,6 +153,8 @@ static struct ref_list *sort_ref_list(struct ref_list *list)
 static struct cached_refs {
 	char did_loose;
 	char did_packed;
+	char is_locked;
+	char is_invalidated;
 	struct ref_list *loose;
 	struct ref_list *packed;
 } cached_refs;
@@ -170,6 +172,11 @@ static void invalidate_cached_refs(void)
 {
 	struct cached_refs *ca = &cached_refs;
 
+	if (ca->is_locked) {
+		ca->is_invalidated = 1;
+		return;
+	}
+
 	if (ca->did_loose && ca->loose)
 		free_ref_list(ca->loose);
 	if (ca->did_packed && ca->packed)
@@ -178,6 +185,24 @@ static void invalidate_cached_refs(void)
 	ca->did_loose = ca->did_packed = 0;
 }
 
+static void lock_cached_refs(void)
+{
+	struct cached_refs *ca = &cached_refs;
+
+	ca->is_locked = 1;
+}
+
+static void unlock_cached_refs(void)
+{
+	struct cached_refs *ca = &cached_refs;
+
+	ca->is_locked = 0;
+	if (ca->is_invalidated) {
+		invalidate_cached_refs();
+		ca->is_invalidated = 0;
+	}
+}
+
 static void read_packed_refs(FILE *f, struct cached_refs *cached_refs)
 {
 	struct ref_list *list = NULL;
@@ -518,10 +543,12 @@ int peel_ref(const char *ref, unsigned char *sha1)
 static int do_for_each_ref(const char *base, each_ref_fn fn, int trim,
 			   void *cb_data)
 {
-	int retval;
+	int retval = 0;
 	struct ref_list *packed = get_packed_refs();
 	struct ref_list *loose = get_loose_refs();
 
+	lock_cached_refs();
+
 	while (packed && loose) {
 		struct ref_list *entry;
 		int cmp = strcmp(packed->name, loose->name);
@@ -538,15 +565,17 @@ static int do_for_each_ref(const char *base, each_ref_fn fn, int trim,
 		}
 		retval = do_one_ref(base, fn, trim, cb_data, entry);
 		if (retval)
-			return retval;
+			goto out;
 	}
 
 	for (packed = packed ? packed : loose; packed; packed = packed->next) {
 		retval = do_one_ref(base, fn, trim, cb_data, packed);
 		if (retval)
-			return retval;
+			goto out;
 	}
-	return 0;
+ out:
+	unlock_cached_refs();
+	return retval;
 }
 
 int head_ref(each_ref_fn fn, void *cb_data)
-- 
1.5.3.rc0.100.ge60b4

[PATCH 5/6] revision: mark commits that didn't match a pattern for later use

[skimo]

From: Sven Verdoolaege <skimo@kotnet.org>

Signed-off-by: Sven Verdoolaege <skimo@kotnet.org>
---
 revision.c |    4 +++-
 revision.h |    1 +
 2 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/revision.c b/revision.c
index 38061f9..cbeb137 100644
--- a/revision.c
+++ b/revision.c
@@ -1446,8 +1446,10 @@ static struct commit *get_revision_1(struct rev_info *revs)
 		if (revs->no_merges &&
 		    commit->parents && commit->parents->next)
 			continue;
-		if (!commit_match(commit, revs))
+		if (!commit_match(commit, revs)) {
+			commit->object.flags |= PRUNED;
 			continue;
+		}
 		if (revs->prune_fn && revs->dense) {
 			/* Commit without changes? */
 			if (!(commit->object.flags & TREECHANGE)) {
diff --git a/revision.h b/revision.h
index 9728d4c..d3609ab 100644
--- a/revision.h
+++ b/revision.h
@@ -10,6 +10,7 @@
 #define CHILD_SHOWN	(1u<<6)
 #define ADDED		(1u<<7)	/* Parents already parsed and added? */
 #define SYMMETRIC_LEFT	(1u<<8)
+#define PRUNED		(1u<<9)
 
 struct rev_info;
 struct log_info;
-- 
1.5.3.rc0.100.ge60b4

[PATCH 6/6] Add git-rewrite-commits

[skimo]

From: Sven Verdoolaege <skimo@kotnet.org>

This builtin is similar to git-filter-branch (and cg-admin-rewritehist).
The main difference is that git-rewrite-commits will automatically
rewrite any SHA1 in a commit message to the rewritten SHA1 as
well as any reference (in .git/refs/) that points to a rewritten commit.

It's also a lot faster than git-filter-branch if no external command
is called.  For example, running either to eliminating a commit specified
as a graft results in the following timings, both being performed
on a freshly cloned copy of a small repo:

	bash-3.00$ time git-filter-branch test
	Rewrite 274fe3dfb8e8c7d0a6ce05138bdb650de7b459ea (425/425)
	Rewritten history saved to the test branch

	real    0m30.845s
	user    0m13.400s
	sys     0m19.640s

	bash-3.00$ time git-rewrite-commits

	real    0m0.223s
	user    0m0.080s
	sys     0m0.140s

The command line is more reminiscent of git-log.
For example you can say

	git-rewrite-commits --all

to incorporate grafts in all branches, or

	git rewrite-commits --author='!Darl McBribe' --all

to remove all commits by Darl McBribe.

Signed-off-by: Sven Verdoolaege <skimo@kotnet.org>
Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 .gitignore                            |    1 +
 Documentation/cmd-list.perl           |    1 +
 Documentation/git-rewrite-commits.txt |  156 +++++++
 Makefile                              |    1 +
 builtin-rewrite-commits.c             |  712 +++++++++++++++++++++++++++++++++
 builtin.h                             |    1 +
 git.c                                 |    1 +
 t/t7005-rewrite-commits.sh            |  109 +++++
 8 files changed, 982 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/git-rewrite-commits.txt
 create mode 100644 builtin-rewrite-commits.c
 create mode 100755 t/t7005-rewrite-commits.sh

diff --git a/.gitignore b/.gitignore
index 20ee642..bcd95a9 100644
--- a/.gitignore
+++ b/.gitignore
@@ -109,6 +109,7 @@ git-reset
 git-rev-list
 git-rev-parse
 git-revert
+git-rewrite-commits
 git-rm
 git-runstatus
 git-send-email
diff --git a/Documentation/cmd-list.perl b/Documentation/cmd-list.perl
index 2143995..63911b7 100755
--- a/Documentation/cmd-list.perl
+++ b/Documentation/cmd-list.perl
@@ -166,6 +166,7 @@ git-reset                               mainporcelain
 git-revert                              mainporcelain
 git-rev-list                            plumbinginterrogators
 git-rev-parse                           ancillaryinterrogators
+git-rewrite-commits                     mainporcelain
 git-rm                                  mainporcelain
 git-runstatus                           ancillaryinterrogators
 git-send-email                          foreignscminterface
diff --git a/Documentation/git-rewrite-commits.txt b/Documentation/git-rewrite-commits.txt
new file mode 100644
index 0000000..fb4e220
--- /dev/null
+++ b/Documentation/git-rewrite-commits.txt
@@ -0,0 +1,156 @@
+git-rewrite-commits(1)
+======================
+
+NAME
+----
+git-rewrite-commits - Rewrite commits
+
+SYNOPSIS
+--------
+'git-rewrite-commits' [--index-filter <command>] [--commit-filter <command>]
+	[<rev-list options>...]
+
+DESCRIPTION
+-----------
+Lets you rewrite the commits selected by the gitlink:git-rev-list[1]
+options, optionally applying custom filters on each of them.
+These filters either modify the tree associated to a commit
+(through manipulations of the index) or the (raw) commit itself.
+Any commit within the specified range that is filtered out
+through a pattern is removed from its children (assuming they
+are in range) and replaced by the closest ancestors that
+are either not being rewritten or not filtered out.
+
+Any branch pointing to any of the rewritten commits is replaced
+by a pointer to the new commit.  The original pointers are saved
+in the refs/original hierarchy.  Any abbreviated SHA1 in the commit
+message of any of the rewritten commits that points to (another)
+rewritten commit is replaced by the SHA1 of the corresponding new commit.
+
+Besides the actions specified by the filters, the new commits
+will also reflect any grafts that may apply to any of the selected commits.
+
+*WARNING*! The rewritten history will have different object names for all
+the objects and will not converge with the original branch.  You will not
+be able to easily push and distribute the rewritten branch on top of the
+original branch.  Please do not use this command if you do not know the
+full implications, and avoid using it anyway, if a simple single commit
+would suffice to fix your problem.
+
+Filters
+~~~~~~~
+
+The filters are applied in the order as listed below.  The <command>
+argument is run as "sh -c '<command'>", with the $GIT_COMMIT
+environment variable set to the commit that is being rewritten.
+If any call to a filter fails, then git-rewrite-commits will abort.
+
+
+OPTIONS
+-------
+
+--index-filter <command>::
+	This filter should only modify the index specified by 'GIT_INDEX_FILE'.
+	Prior to running the filter, this temporary index is populated
+	with the tree of the commit that is being rewritten.
+	This tree will be replaced according to the new state of this
+	temporary index after the filter finishes.
+
+--commit-filter <command>::
+	This filter receives the (raw) commit on stdin and should produce
+	zero or more SHA1s of commits that should replace the given commit
+	on stdout.
+	In other words, the filter "git-hash-object -w -t commit --stdin"
+	leaves the current commit intact.  This command is also available
+	as the shell function "commit".
+	In the input commit, the tree has already been modified by the
+	index filter (if any) and has all references to older commits
+	(including the parents) changed to the possibly rewritten commits.
+
+--write-sha1-mapping
+	Write mapping of old SHA1s to new SHA1s for use in filters.
+
+<rev-list-options>::
+	Selects the commits to be rewritten, defaulting to the history
+	that lead to HEAD.  If commits are filtered using a (negative)
+	pattern then all the commits filtered out will be removed
+	from the history of the selected commits.
+
+
+Examples
+--------
+
+Suppose you want to remove a file (containing confidential information
+or copyright violation) from all commits:
+
+----------------------------------------------------------------------------
+git rewrite-commits --index-filter 'git update-index --remove filename || :'
+----------------------------------------------------------------------------
+
+Now, you will get the rewritten history saved in your current branch
+(the old branch is saved in refs/original).
+
+To set a commit "$graft-id" (which typically is at the tip of another
+history) to be the parent of the current initial commit "$commit-id", in
+order to paste the other history behind the current history:
+
+-----------------------------------------------
+echo "$commit-id $graft-id" >> .git/info/grafts
+git rewrite-commits
+-----------------------------------------------
+
+To remove commits authored by "Darl McBribe" from the history:
+
+--------------------------------------------
+git rewrite-commits --author='!Darl McBribe'
+--------------------------------------------
+
+Note that the changes introduced by the commits, and not reverted by
+subsequent commits, will still be in the rewritten branch. If you want
+to throw out _changes_ together with the commits, you should use the
+interactive mode of gitlink:git-rebase[1].
+
+Consider this history:
+
+------------------
+     D--E--F--G--H
+    /     /
+A--B-----C
+------------------
+
+To rewrite only commits D,E,F,G,H, but leave A, B and C alone, use:
+
+--------------------------------
+git rewrite-commits ... C..H
+--------------------------------
+
+To rewrite commits E,F,G,H, use one of these:
+
+----------------------------------------
+git rewrite-commits ... C..H --not D
+git rewrite-commits ... D..H --not C
+----------------------------------------
+
+To move the whole tree into a subdirectory, or remove it from there:
+
+---------------------------------------------------------------
+git rewrite-commits --index-filter \
+	'git ls-files -s | sed "s-\t-&newsubdir/-" |
+		GIT_INDEX_FILE=$GIT_INDEX_FILE.new \
+			git update-index --index-info &&
+	 mv $GIT_INDEX_FILE.new $GIT_INDEX_FILE'
+---------------------------------------------------------------
+
+
+Author
+------
+Written by Sven Verdoolaege.
+Inspired by cg-admin-rewritehist by Petr "Pasky" Baudis <pasky@suse.cz>.
+
+Documentation
+--------------
+Documentation by Petr Baudis, Sven Verdoolaege and the git list.
+
+GIT
+---
+Part of the gitlink:git[7] suite
diff --git a/Makefile b/Makefile
index d7541b4..252ef8e 100644
--- a/Makefile
+++ b/Makefile
@@ -370,6 +370,7 @@ BUILTIN_OBJS = \
 	builtin-rev-list.o \
 	builtin-rev-parse.o \
 	builtin-revert.o \
+	builtin-rewrite-commits.o \
 	builtin-rm.o \
 	builtin-runstatus.o \
 	builtin-shortlog.o \
diff --git a/builtin-rewrite-commits.c b/builtin-rewrite-commits.c
new file mode 100644
index 0000000..d95a16c
--- /dev/null
+++ b/builtin-rewrite-commits.c
@@ -0,0 +1,712 @@
+#include "cache.h"
+#include "refs.h"
+#include "builtin.h"
+#include "commit.h"
+#include "diff.h"
+#include "revision.h"
+#include "list-objects.h"
+#include "run-command.h"
+#include "cache-tree.h"
+#include "grep.h"
+
+struct decoration rewrite_decoration = { "rewritten as" };
+static const char *original_prefix = "original";
+static const char *commit_filter;
+static const char *index_filter;
+static const char *absolute_git_dir;
+static const char *rewrite_dir;
+static char commit_env[] = "GIT_COMMIT=0000000000000000000000000000000000000000";
+static int write_sha1_mapping;
+
+struct rewrite_decoration {
+	struct rewrite_decoration *next;
+	unsigned char sha1[20];
+};
+
+static void add_rewrite_decoration(struct object *obj, unsigned char *sha1)
+{
+	struct rewrite_decoration *deco = xmalloc(sizeof(struct rewrite_decoration));
+	hashcpy(deco->sha1, sha1);
+	deco->next = add_decoration(&rewrite_decoration, obj, deco);
+}
+
+static int get_rewritten_sha1(unsigned char *sha1)
+{
+	struct rewrite_decoration *deco;
+	struct object *obj = lookup_object(sha1);
+
+	if (!obj)
+		return 1;
+
+	deco = lookup_decoration(&rewrite_decoration, obj);
+	if (!deco)
+		return 1;
+
+	hashcpy(sha1, deco->sha1);
+	return 0;
+}
+
+static char *add_parents(char *dest, struct commit_list *parents)
+{
+	unsigned char sha1[20];
+	struct commit_list *list;
+
+	for (list = parents; list; list = list->next) {
+		hashcpy(sha1, list->item->object.sha1);
+		get_rewritten_sha1(sha1);
+		memcpy(dest, "parent ", 7);
+		dest += 7;
+		memcpy(dest, sha1_to_hex(sha1), 40);
+		dest += 40;
+		*dest++ = '\n';
+	}
+	return dest;
+}
+
+static int skip_one_line(char **buf_p, unsigned long *len_p)
+{
+	int linelen;
+	char *end = memchr(*buf_p, '\n', *len_p);
+
+	if (!end)
+		linelen = *len_p;
+	if (end)
+		linelen = end - *buf_p + 1;
+	*buf_p += linelen;
+	*len_p -= linelen;
+
+	return linelen;
+}
+
+static char *filter_index(char *orig_hex, struct commit *commit)
+{
+	int argc;
+	const char *argv[10];
+	static char index_env[16+PATH_MAX];
+	char *tmp_index = mkpath("%s/rewrite_index", absolute_git_dir);
+	const char *env[] = { index_env, commit_env, NULL };
+	char *hex;
+
+	memcpy(index_env, "GIT_INDEX_FILE=", 15);
+	strcpy(index_env+15, tmp_index);
+	tmp_index = index_env+15;
+
+	memcpy(commit_env+sizeof(commit_env)-41,
+		sha1_to_hex(commit->object.sha1), 40);
+
+	/* First write out tree to temporary index */
+	argc = 0;
+	argv[argc++] = "read-tree";
+	argv[argc++] = orig_hex;
+	argv[argc] = NULL;
+	if (run_command_v_opt_cd_env(argv, RUN_GIT_CMD, NULL, env))
+		die("Cannot write index '%s' for filter", tmp_index);
+
+	/* Then filter the index */
+	argc = 0;
+	argv[argc++] = "sh";
+	argv[argc++] = "-c";
+	argv[argc++] = index_filter;
+	argv[argc] = NULL;
+	if (run_command_v_opt_cd_env(argv, 0, rewrite_dir, env))
+		die("Index filter '%s' failed", index_filter);
+
+	/* Finally read it back in */
+	if (read_cache_from(tmp_index) < 0)
+		die("Error reading index '%s'", tmp_index);
+	active_cache_tree = cache_tree();
+	if (cache_tree_update(active_cache_tree, active_cache, active_nr,
+			      0, 0) < 0)
+		die("Error building trees");
+	hex = sha1_to_hex(active_cache_tree->sha1);
+	discard_cache();
+
+	unlink(tmp_index);
+
+	return hex;
+}
+
+static char *rewrite_header(char *dest, unsigned long *len_p, char **buf_p,
+			    struct commit *commit)
+{
+	struct commit_list *parents = commit->parents;
+	int linelen;
+
+	do {
+		char *line = *buf_p;
+		linelen = skip_one_line(buf_p, len_p);
+
+		if (!linelen)
+			return dest;
+
+		if (index_filter && !memcmp(line, "tree ", 5)) {
+			if (linelen != 46)
+				die("bad tree line in commit");
+			memcpy(dest, "tree ", 5);
+			line[45] = '\0';
+			memcpy(dest+5, filter_index(line+5, commit), 40);
+			line[45] = '\n';
+			dest[45] = '\n';
+			dest += 46;
+			continue;
+		}
+
+		/* drop old parents */
+		if (!memcmp(line, "parent ", 7)) {
+			if (linelen != 48)
+				die("bad parent line in commit");
+			continue;
+		}
+
+		/* insert new parents before author */
+		if (!memcmp(line, "author ", 7))
+			dest = add_parents(dest, parents);
+
+		memcpy(dest, line, linelen);
+		dest += linelen;
+	} while (linelen > 1);
+	return dest;
+}
+
+static size_t hex_len(const char *s, size_t n)
+{
+	size_t offset;
+
+	for (offset = 0; offset < n; ++offset)
+		if (!ishex(s[offset]))
+			break;
+	return offset;
+}
+
+static size_t non_hex_len(const char *s, size_t n)
+{
+	size_t offset;
+
+	for (offset = 0; offset < n; ++offset)
+		if (ishex(s[offset]))
+			break;
+	return offset;
+}
+
+/* Replace any (short) sha1 of a rewritten commit by the new (short) sha1 */
+static char *rewrite_body(char *dest, unsigned long len, char *buf)
+{
+	unsigned char sha1[20];
+
+	while (len) {
+		size_t ll = non_hex_len(buf, len);
+		memcpy(dest, buf, ll);
+		dest += ll;
+		buf += ll;
+		len -= ll;
+
+		ll = hex_len(buf, len);
+		if (ll >= 8 && ll <= 40 &&
+		    !get_short_sha1(buf, ll, sha1, 1) &&
+		    !get_rewritten_sha1(sha1))
+			memcpy(dest, sha1_to_hex(sha1), ll);
+		else
+			memcpy(dest, buf, ll);
+		dest += ll;
+		buf += ll;
+		len -= ll;
+	}
+	return dest;
+}
+
+static void write_ref_sha1_or_die(const char *ref, const unsigned char *old_sha1,
+				  const unsigned char *new_sha1,
+				  const char *logmsg, int flags)
+{
+	struct ref_lock *lock;
+
+	lock = lock_any_ref_for_update(ref, old_sha1, flags);
+	if (!lock)
+		die("%s: cannot lock the ref", ref);
+	if (write_ref_sha1(lock, new_sha1, logmsg) < 0)
+		die("%s: cannot update the ref", ref);
+}
+
+static int is_ref_to_be_rewritten(const char *ref)
+{
+	unsigned char sha1[20];
+	int flag;
+
+	if (prefixcmp(ref, "refs/"))
+		return 0;
+	if (!prefixcmp(ref, "refs/remotes/"))
+		return 0;
+	if (!prefixcmp(ref+5, original_prefix))
+		return 0;
+
+	resolve_ref(ref, sha1, 0, &flag);
+	if (flag & REF_ISSYMREF)
+		return 0;
+
+	return 1;
+}
+
+static int is_pruned(struct commit *commit, int path_pruning)
+{
+	if (commit->object.flags & PRUNED)
+		return 1;
+	if (path_pruning &&
+	    !(commit->object.flags & (TREECHANGE | UNINTERESTING)))
+		return 1;
+	return 0;
+}
+
+static void rewrite_sha1(struct object *obj, unsigned char *new_sha1)
+{
+	if (!hashcmp(obj->sha1, new_sha1))
+		return;
+
+	add_rewrite_decoration(obj, new_sha1);
+}
+
+/*
+ * Replace any parent that has been removed by its parents
+ * and return the number of new parents.
+ * We directly modify the parent list, so any libification
+ * should probably adapt this function.
+ */
+static int rewrite_parents(struct commit *commit, int path_pruning)
+{
+	int n;
+	struct commit_list *list, *parents, **prev;
+	unsigned char sha1[20];
+
+	for (n = 0, prev = &commit->parents; *prev; ++n) {
+		list = *prev;
+
+		rewrite_parents(list->item, path_pruning);
+		if (!is_pruned(list->item, path_pruning)) {
+			prev = &list->next;
+			continue;
+		}
+
+		hashcpy(sha1, list->item->object.sha1);
+		get_rewritten_sha1(sha1);
+		if (!is_null_sha1(sha1)) {
+			hashclr(sha1);
+			rewrite_sha1(&list->item->object, sha1);
+		}
+
+		parents = list->item->parents;
+		if (!parents) {
+			*prev = list->next;
+			free(list);
+			--n;
+			continue;
+		}
+		list->item = parents->item;
+		prev = &list->next;
+		list = list->next;
+		while ((parents = parents->next)) {
+			commit_list_insert(parents->item, prev);
+			prev = &(*prev)->next;
+			++n;
+		}
+		*prev = list;
+	}
+
+	return n;
+}
+
+static int rewrite_ref(const char *refname, const unsigned char *sha1,
+			int flags, void *cb_data)
+{
+	int prefix_len;
+	int len;
+	char buffer[256], *p;
+	struct object *obj = parse_object(sha1);
+	unsigned char new_sha1[20];
+	struct commit *commit;
+	int pruned;
+
+	if (!obj)
+		return 0;
+	if (obj->type == OBJ_TAG)
+		return 0;
+	if (!is_ref_to_be_rewritten(refname))
+		return 0;
+
+	commit = lookup_commit_reference(sha1);
+	pruned = is_pruned(commit, !!cb_data);
+
+	hashcpy(new_sha1, sha1);
+	if (!pruned && get_rewritten_sha1(new_sha1))
+		return 0;
+
+	prefix_len = strlen(original_prefix);
+	len = strlen(refname);
+
+	if (len + prefix_len + 1 + 1 > sizeof(buffer))
+		die("rewrite ref of '%s' too long", refname);
+	p = buffer;
+	memcpy(p, "refs/", 5);
+	p += 5;
+	memcpy(p, original_prefix, prefix_len);
+	p += prefix_len;
+	memcpy(p, refname+4, len-4 + 1);
+
+	if (safe_create_leading_directories(git_path(buffer)))
+		die("Unable to create leading directories of '%s'", buffer);
+	write_ref_sha1_or_die(buffer, NULL, obj->sha1,
+				"copied during rewrite", 0);
+
+	if (pruned) {
+		int i = 1;
+		delete_ref(refname, obj->sha1);
+		mkdir(git_path(refname), 0777);
+		struct commit_list *parent;
+		rewrite_parents(commit, !!cb_data);
+		for (parent = commit->parents; parent; parent = parent->next) {
+			hashcpy(new_sha1, parent->item->object.sha1);
+			get_rewritten_sha1(new_sha1);
+			snprintf(buffer, sizeof(buffer), "%s/%d", refname, i);
+			write_ref_sha1_or_die(buffer, NULL, new_sha1,
+						"ancestor of pruned", 0);
+			++i;
+		}
+		return 0;
+	}
+	else
+		write_ref_sha1_or_die(refname, obj->sha1, new_sha1,
+					"rewritten", 0);
+
+	return 0;
+}
+
+static void add_sha1_map(const char *old_sha1, const char *new_sha1)
+{
+	int fd;
+
+	if (!write_sha1_mapping)
+		return;
+	if (new_sha1 && !hashcmp(old_sha1, new_sha1))
+		return;
+
+	fd = open(mkpath("%s/map/%s", rewrite_dir, sha1_to_hex(old_sha1)),
+		  O_CREAT | O_WRONLY | O_APPEND, 0777);
+	if (fd < 0)
+		die("unable to open map file '%s/map/%s'", rewrite_dir,
+			sha1_to_hex(old_sha1));
+	if (new_sha1)
+		write_or_die(fd, sha1_to_hex(new_sha1), 41);
+	close(fd);
+}
+
+static void filter_and_write_commit(char *commit_body, size_t len,
+				    struct commit *commit, unsigned char *sha1)
+{
+	char commit_path[PATH_MAX];
+	struct child_process cmd;
+	int argc;
+	const char *argv[10];
+	int fd;
+	const char *env[] = { commit_env, NULL };
+	char hex[41];
+	struct commit_list *list = NULL, **end = &list;
+
+	memcpy(commit_env+sizeof(commit_env)-41,
+		sha1_to_hex(commit->object.sha1), 40);
+
+	fd = git_mkstemp(commit_path, sizeof(commit_path), ".commit_XXXXXX");;
+	write_or_die(fd, commit_body, len);
+
+	argc = 0;
+	argv[argc++] = "sh";
+	argv[argc++] = "-c";
+	argv[argc++] = commit_filter;
+	argv[argc] = NULL;
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.in = open(commit_path, O_RDONLY);
+	if (cmd.in < 0)
+		die("Unable to read commit from file '%s'", commit_path);
+	unlink(commit_path);
+	cmd.out = -1;
+	cmd.argv = argv;
+	cmd.dir = rewrite_dir;
+	cmd.env = env;
+	if (start_command(&cmd))
+		die("Commit filter '%s' failed to start", commit_filter);
+	while (xread(cmd.out, hex, 41) == 41) {
+		struct commit *new_commit;
+		hex[40] = '\0';
+		if (get_sha1(hex, sha1))
+			die("Unexpected output from commit filter: '%s'",
+			    hex);
+		if (!(new_commit = lookup_commit_reference(sha1)))
+			die("Invalid SHA1 in output from commit filter: '%s'",
+			    hex);
+		commit_list_insert(new_commit, end);
+		add_sha1_map(commit->object.sha1, new_commit->object.sha1);
+		end = &(*end)->next;
+	}
+	if (list && !list->next)
+		free_commit_list(list);
+	else {
+		hashclr(sha1);
+		commit->object.flags |= PRUNED;
+		/*
+		 * If the filter returns two or more commits,
+		 * we consider the original commit to have been
+		 * removed and put the list in the old commit's
+		 * parent list so that all the old commit's children
+		 * will copy them.
+		 */
+		if (list) {
+			free_commit_list(commit->parents);
+			commit->parents = list;
+		} else
+		    add_sha1_map(commit->object.sha1, NULL);
+	}
+	if (finish_command(&cmd))
+		die("Commit filter '%s' failed", commit_filter);
+	close(cmd.in);
+}
+
+static void rewrite_commit(struct commit *commit, int path_pruning)
+{
+	char *buf, *p;
+	int n;
+	char *orig_buf = commit->buffer;
+	unsigned long orig_len = strlen(orig_buf);
+	unsigned char sha1[20];
+
+	n = rewrite_parents(commit, path_pruning);
+
+	/* Make enough remove for n (possibly extra) parents */
+	p = buf = xmalloc(orig_len + n*48);
+	p = rewrite_header(p, &orig_len, &orig_buf, commit);
+	p = rewrite_body(p, orig_len, orig_buf);
+	if (!commit_filter) {
+		if (write_sha1_file(buf, p-buf, commit_type, sha1))
+			die("Unable to write new commit");
+		add_sha1_map(commit->object.sha1, sha1);
+	} else
+		filter_and_write_commit(buf, p-buf, commit, sha1);
+	free(buf);
+
+	rewrite_sha1(&commit->object, sha1);
+}
+
+static void move_head_forward(char *ref, unsigned char *old_sha1, int detached,
+				int path_limiting)
+{
+	unsigned char new_sha1[20];
+	int rewritten = 0;
+	struct commit *commit;
+
+	commit = lookup_commit_reference(old_sha1);
+	if (is_pruned(commit, path_limiting)) {
+		if (commit->parents) {
+			hashcpy(new_sha1, commit->parents->item->object.sha1);
+			get_rewritten_sha1(new_sha1);
+			rewritten = 1;
+		} else
+			return;
+	} else {
+		hashcpy(new_sha1, old_sha1);
+		rewritten = !get_rewritten_sha1(new_sha1);
+	}
+	if (rewritten && !is_bare_repository()) {
+		int argc;
+		const char *argv[10];
+
+		argc = 0;
+		argv[argc++] = "read-tree";
+		argv[argc++] = "-m";
+		argv[argc++] = "-u";
+		argv[argc++] = sha1_to_hex(old_sha1);
+		argv[argc++] = sha1_to_hex(new_sha1);
+		argv[argc] = NULL;
+		if (run_command_v_opt(argv, RUN_GIT_CMD))
+			die("Cannot move HEAD forward");
+	}
+	if (!detached) {
+		unsigned char sha1[20];
+
+		if (resolve_ref(ref, sha1, 1, NULL))
+			create_symref("HEAD", ref, "reattached after rewrite");
+		else {
+			char buffer[256];
+			snprintf(buffer, sizeof(buffer), "%s/1", ref);
+			if (resolve_ref(buffer, sha1, 1, NULL))
+				create_symref("HEAD", buffer,
+					"attached to ancestor of pruned commit");
+		}
+	}
+	else if (rewritten)
+		write_ref_sha1_or_die("HEAD", NULL, new_sha1,
+				    "rewritten", REF_NODEREF);
+}
+
+static char aux_functions[] =
+"export GIT_REWRITE_DIR=`pwd`\n"
+"commit()\n"
+"{\n"
+"	git-hash-object -w -t commit --stdin\n"
+"}\n"
+"map()\n"
+"{\n"
+"	# if it was not rewritten, take the original\n"
+"	if test -r \"$GIT_REWRITE_DIR/map/$1\"\n"
+"	then\n"
+"		cat \"$GIT_REWRITE_DIR/map/$1\"\n"
+"	else\n"
+"		echo \"$1\"\n"
+"	fi\n"
+"}\n"
+"cd t\n";
+
+static char filter_prefix[] = ". aux;";
+
+static char *create_filter(const char *command)
+{
+	int prefix_len = sizeof(filter_prefix)-1;
+	int command_len = strlen(command);
+	char *filter = xmalloc(prefix_len + command_len + 1);
+
+	memcpy(filter, filter_prefix, prefix_len);
+	memcpy(filter+prefix_len, command, command_len+1);
+	return filter;
+}
+
+static const char *create_absolute_path(const char *path)
+{
+	int path_len;
+	static char cwd[PATH_MAX];
+	static int cwd_len = -1;
+	char *absolute;
+
+	if (path[0] == '/')
+		return path;
+
+	if (cwd_len == -1) {
+		if (!getcwd(cwd, sizeof(cwd)))
+			die("unable to get current working directory");
+		cwd_len = strlen(cwd);
+	}
+
+	path_len = strlen(path);
+	absolute = xmalloc(cwd_len+path_len+2);
+	memcpy(absolute, cwd, cwd_len);
+	absolute[cwd_len] = '/';
+	memcpy(absolute+cwd_len+1, path, path_len+1);
+	return absolute;
+}
+
+static int rm_rf(const char *dir)
+{
+	int argc;
+	const char *argv[10];
+
+	argc = 0;
+	argv[argc++] = "rm";
+	argv[argc++] = "-rf";
+	argv[argc++] = dir;
+	argv[argc] = NULL;
+	return run_command_v_opt(argv, 0);
+}
+
+static void cleanup_temp_dir()
+{
+	if (rm_rf(rewrite_dir))
+		die("Unable to clean up rewrite directory '%s'", rewrite_dir);
+}
+
+static void setup_temp_dir()
+{
+	int aux;
+
+	absolute_git_dir = create_absolute_path(get_git_dir());
+	setenv(GIT_DIR_ENVIRONMENT, absolute_git_dir, 1);
+
+	if (!rewrite_dir) {
+		rewrite_dir = xstrdup(git_path("rewrite"));
+	}
+	if (mkdir(rewrite_dir, 0777)) {
+		if (errno == EEXIST)
+			die("rewrite directory '%s' already exists",
+				rewrite_dir);
+		die("unable to create rewrite directory '%s'", rewrite_dir);
+	}
+	atexit(cleanup_temp_dir);
+
+	if (mkdir(mkpath("%s/map", rewrite_dir), 0777))
+		die("unable to create map directory '%s/map'", rewrite_dir);
+	if (mkdir(mkpath("%s/t", rewrite_dir), 0777))
+		die("unable to create temp directory '%s/t'", rewrite_dir);
+	aux = open(mkpath("%s/aux", rewrite_dir), O_CREAT | O_WRONLY, 0777);
+	if (aux < 0)
+		die("unable to create aux file '%s/aux'", rewrite_dir);
+	write_or_die(aux, aux_functions, sizeof(aux_functions)-1);
+	close(aux);
+}
+
+int cmd_rewrite_commits(int argc, const char **argv, const char *prefix)
+{
+	struct rev_info rev;
+	struct commit *commit;
+	unsigned char HEAD_sha1[20];
+	char *HEAD_ref;
+	int flag;
+	int i, j;
+
+	init_revisions(&rev, prefix);
+
+	for (i = 1, j = 1; i < argc; ++i) {
+		if (!strcmp(argv[i], "--index-filter")) {
+			if (++i == argc)
+				die("Argument required for --index-filter");
+			index_filter = argv[i];
+		} else if (!strcmp(argv[i], "--commit-filter")) {
+			if (++i == argc)
+				die("Argument required for --commit-filter");
+			commit_filter = argv[i];
+		} else if (!strcmp(argv[i], "--write-sha1-mapping")) {
+			write_sha1_mapping = 1;
+		} else
+			argv[j++] = argv[i];
+	}
+	argc = j;
+	argc = setup_revisions(argc, argv, &rev, "HEAD");
+	rev.ignore_merges = 0;
+	rev.topo_order = 1;
+	rev.reverse = 1;
+	rev.parents = 1;
+
+	if (index_filter || commit_filter)
+		setup_temp_dir();
+	else
+		/* They'll never know.  BWUHAHA */
+		write_sha1_mapping = 0;
+
+	if (index_filter)
+		index_filter = create_filter(index_filter);
+	if (commit_filter)
+		commit_filter = create_filter(commit_filter);
+
+	prepare_revision_walk(&rev);
+	while ((commit = get_revision(&rev)) != NULL) {
+		rewrite_commit(commit, !!rev.prune_fn);
+	}
+
+	HEAD_ref = xstrdup(resolve_ref("HEAD", HEAD_sha1, 1, &flag));
+	if (flag & REF_ISSYMREF) {
+		/* Detach HEAD at its current position */
+		write_ref_sha1_or_die("HEAD", NULL, HEAD_sha1,
+					"detached for rewrite", REF_NODEREF);
+	}
+
+	rm_rf(git_path("refs/%s", original_prefix));
+	for_each_ref(rewrite_ref, rev.prune_fn);
+
+	move_head_forward(HEAD_ref, HEAD_sha1, !(flag & REF_ISSYMREF),
+			 !!rev.prune_fn);
+	free(HEAD_ref);
+
+	return 0;
+}
diff --git a/builtin.h b/builtin.h
index 4cc228d..5a08ec8 100644
--- a/builtin.h
+++ b/builtin.h
@@ -63,6 +63,7 @@ extern int cmd_rerere(int argc, const char **argv, const char *prefix);
 extern int cmd_rev_list(int argc, const char **argv, const char *prefix);
 extern int cmd_rev_parse(int argc, const char **argv, const char *prefix);
 extern int cmd_revert(int argc, const char **argv, const char *prefix);
+extern int cmd_rewrite_commits(int argc, const char **argv, const char *prefix);
 extern int cmd_rm(int argc, const char **argv, const char *prefix);
 extern int cmd_runstatus(int argc, const char **argv, const char *prefix);
 extern int cmd_shortlog(int argc, const char **argv, const char *prefix);
diff --git a/git.c b/git.c
index a647f9c..ce02b0b 100644
--- a/git.c
+++ b/git.c
@@ -356,6 +356,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "rev-list", cmd_rev_list, RUN_SETUP },
 		{ "rev-parse", cmd_rev_parse, RUN_SETUP },
 		{ "revert", cmd_revert, RUN_SETUP | NEED_WORK_TREE },
+		{ "rewrite-commits", cmd_rewrite_commits, RUN_SETUP },
 		{ "rm", cmd_rm, RUN_SETUP | NEED_WORK_TREE },
 		{ "runstatus", cmd_runstatus, RUN_SETUP | NEED_WORK_TREE },
 		{ "shortlog", cmd_shortlog, RUN_SETUP | USE_PAGER },
diff --git a/t/t7005-rewrite-commits.sh b/t/t7005-rewrite-commits.sh
new file mode 100755
index 0000000..eef4129
--- /dev/null
+++ b/t/t7005-rewrite-commits.sh
@@ -0,0 +1,109 @@
+#!/bin/sh
+
+test_description='git-rewrite-commits'
+. ./test-lib.sh
+
+make_commit () {
+	lower=$(echo $1 | tr A-Z a-z)
+	echo $lower > $lower
+	git add $lower
+	test_tick
+	git commit -m $1
+	git tag $1
+}
+
+test_expect_success 'setup' '
+	make_commit A
+	make_commit B
+	git checkout -b branch B
+	make_commit D
+	make_commit E
+	git checkout master
+	make_commit C
+	git checkout branch
+	git merge C
+	git tag F
+	make_commit G
+	make_commit H
+'
+
+orig_H=$(git rev-parse H)
+test_expect_success 'rewrite identically' '
+	git-rewrite-commits
+'
+
+test_expect_success 'result is really identical' '
+	test $orig_H = $(git rev-parse H)
+'
+
+test_expect_success 'rewrite identically using commit filter' '
+	git-rewrite-commits --commit-filter="commit"
+'
+
+test_expect_success 'result is really identical' '
+	test $orig_H = $(git rev-parse H)
+'
+
+# for lack of 'git-mv --cached d doh'
+test_expect_success 'rewrite, renaming a specific file' '
+	git-rewrite-commits --index-filter \
+		"git-ls-files -s | sed \"s-\\td\\\$-\\tdoh-\" |
+	         GIT_INDEX_FILE=\$GIT_INDEX_FILE.new \
+		     git-update-index --index-info &&
+		 mv \$GIT_INDEX_FILE.new \$GIT_INDEX_FILE"
+'
+
+test_expect_success 'test that the file was renamed' '
+	test d = $(git-show H:doh) &&
+	! git-show H:d --
+'
+
+orig_H=$(git rev-parse H)
+test_expect_success 'use index-filter to move into a subdirectory' '
+	git-rewrite-commits --index-filter \
+		 "git ls-files -s | sed \"s-\\t-&newsubdir/-\" |
+	          GIT_INDEX_FILE=\$GIT_INDEX_FILE.new \
+			git update-index --index-info &&
+		  mv \$GIT_INDEX_FILE.new \$GIT_INDEX_FILE" &&
+	test -z "$(git diff $orig_H H:newsubdir)"'
+
+test_expect_success 'remove merge commit' '
+	git-rewrite-commits --grep="!Merge" &&
+	test 2 = `git-log ^G^@ G --pretty=format:%P | wc -w`'
+
+test_expect_success 'remove new merge commit using commit filter' '
+	git-rewrite-commits --commit-filter \
+		"if test \$GIT_COMMIT = $(git rev-parse G); then \
+			cat > /dev/null; \
+		 else \
+			commit; \
+		 fi" &&
+	test 2 = `git-log ^H^@ H --pretty=format:%P | wc -w`'
+
+test_expect_success 'remove first commit' '
+	git-rewrite-commits --grep="!A"'
+
+test_expect_success 'stops when index filter fails' '
+	! git-rewrite-commits --index-filter false &&
+	git-checkout branch
+'
+
+test_expect_success 'author information is preserved' '
+	: > i &&
+	git add i &&
+	test_tick &&
+	GIT_AUTHOR_NAME="B V Uips" git commit -m bvuips &&
+	git-rewrite-commits --commit-filter "(cat; \
+			test \$GIT_COMMIT != $(git rev-parse master) || \
+			echo Hallo) | commit" &&
+	test 1 = $(git rev-list --author="B V Uips" HEAD | wc -l)
+'
+
+test_expect_success 'use index-filter to select a subdirectory' '
+	git-rewrite-commits --index-filter \
+		 "git read-tree \$GIT_COMMIT:newsubdir" -- newsubdir &&
+	test 0 = $(git rev-list  HEAD -- i | wc -l) &&
+	test 0 = $(git rev-list  HEAD -- newsubdir | wc -l)
+'
+
+test_done
-- 
1.5.3.rc0.100.ge60b4

Re: [PATCH 6/6] Add git-rewrite-commits

[Sven Verdoolaege]

On Thu, Jul 12, 2007 at 09:06:03PM +0200, skimo@liacs.nl wrote:
> +static int rewrite_parents(struct commit *commit, int path_pruning)
> +{
> +	int n;
> +	struct commit_list *list, *parents, **prev;
> +	unsigned char sha1[20];
> +
> +	for (n = 0, prev = &commit->parents; *prev; ++n) {
> +		list = *prev;
> +
> +		rewrite_parents(list->item, path_pruning);
> +		if (!is_pruned(list->item, path_pruning)) {
> +			prev = &list->next;
> +			continue;
> +		}

Oops... that should be the other way around...

diff --git a/builtin-rewrite-commits.c b/builtin-rewrite-commits.c
index d95a16c..4cd17ae 100644
--- a/builtin-rewrite-commits.c
+++ b/builtin-rewrite-commits.c
@@ -279,11 +279,11 @@ static int rewrite_parents(struct commit *commit, int path_pruning)
 	for (n = 0, prev = &commit->parents; *prev; ++n) {
 		list = *prev;
 
-		rewrite_parents(list->item, path_pruning);
 		if (!is_pruned(list->item, path_pruning)) {
 			prev = &list->next;
 			continue;
 		}
+		rewrite_parents(list->item, path_pruning);
 
 		hashcpy(sha1, list->item->object.sha1);
 		get_rewritten_sha1(sha1);

I'll include it in my next version.

skimo

Re: [PATCH 3/6] Define ishex(x) in git-compat-util.h

[Johannes Schindelin]

Hi,

On Thu, 12 Jul 2007, skimo@liacs.nl wrote:

> diff --git a/git-compat-util.h b/git-compat-util.h
> index 362e040..1a36f4c 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -325,12 +325,15 @@ static inline int has_extension(const char *filename, const char *ext)
>  extern unsigned char sane_ctype[256];
>  #define GIT_SPACE 0x01
>  #define GIT_DIGIT 0x02
> -#define GIT_ALPHA 0x04
> +#define GIT_HEXAL 0x04
> +#define GIT_OTHAL 0x08
> +#define GIT_ALPHA (GIT_HEXAL | GIT_OTHAL)

I'd have left GIT_ALPHA, and added GIT_HEXDIGIT.

>  #define sane_istest(x,mask) ((sane_ctype[(unsigned char)(x)] & (mask)) != 0)
>  #define isspace(x) sane_istest(x,GIT_SPACE)
>  #define isdigit(x) sane_istest(x,GIT_DIGIT)
>  #define isalpha(x) sane_istest(x,GIT_ALPHA)
>  #define isalnum(x) sane_istest(x,GIT_ALPHA | GIT_DIGIT)
> +#define ishex(x) sane_istest(x,GIT_HEXAL | GIT_DIGIT)

I know, I originally proposed this.  In the mean time, however, I found 
that this gem should be even better (in terms of diff size):

#define ishex(x) (hexval(x) >= 0)

Ciao,
Dscho

Re: [PATCH 6/6] Add git-rewrite-commits

[Johannes Schindelin]

Hi,

On Thu, 12 Jul 2007, skimo@liacs.nl wrote:

> +SYNOPSIS
> +--------
> +'git-rewrite-commits' [--index-filter <command>] [--commit-filter <command>]
> +	[<rev-list options>...]

--write-sha1-mappings is missing.

> +Filters
> +~~~~~~~
> +
> +The filters are applied in the order as listed below.  The <command>
> +argument is run as "sh -c '<command'>", with the $GIT_COMMIT
> +environment variable set to the commit that is being rewritten.
> +If any call to a filter fails, then git-rewrite-commits will abort.

Here you should note that a couple of helper functions are defined for 
ease of use.

> +--write-sha1-mapping
> +	Write mapping of old SHA1s to new SHA1s for use in filters.

Where?  How to use it?

> +<rev-list-options>::
> +	Selects the commits to be rewritten, defaulting to the history
> +	that lead to HEAD.

s/the history that lead to HEAD/the current branch/

> +Examples
> +--------
> +
> +Suppose you want to remove a file (containing confidential information
> +or copyright violation) from all commits:
> +
> +----------------------------------------------------------------------------
> +git rewrite-commits --index-filter 'git update-index --remove filename || :'

We seem to prefer "$ git" instead of just "git" in the other man pages' 
examples.

> +----------------------------------------------------------------------------
> +
> +Now, you will get the rewritten history saved in your current branch
> +(the old branch is saved in refs/original).

						The "|| :" construct 
+ prevents the filter to fail when the given file was not present in the 
+ index.

> +To move the whole tree into a subdirectory, or remove it from there:
> +
> +---------------------------------------------------------------
> +git rewrite-commits --index-filter \
> +	'git ls-files -s | sed "s-\t-&newsubdir/-" |
> +		GIT_INDEX_FILE=$GIT_INDEX_FILE.new \
> +			git update-index --index-info &&
> +	 mv $GIT_INDEX_FILE.new $GIT_INDEX_FILE'
> +---------------------------------------------------------------

I imagine that this should be replaced (later! in a different patch!) by 
either --subdir-filter, or by a shell helper function.

> +#include "grep.h"

I did not compile test, but do you really need that?

> +struct decoration rewrite_decoration = { "rewritten as" };
> [...]
> +
> +struct rewrite_decoration {
> +	struct rewrite_decoration *next;
> +	unsigned char sha1[20];
> +};

I wonder why you give the instance of a "struct decoration" the same name 
as that of a _different_ struct. IOW it is confusing that there is a 
"struct rewrite_decoration", but the variable "rewrite_decoration" is no 
instance of that struct.

> +static void add_rewrite_decoration(struct object *obj, unsigned char *sha1)

Why "struct object *"?  You only rewrite commits.  Which makes me suspect 
that your "struct rewrite_decoration" should be a "struct commit_list" to 
begin with.

> +static char *add_parents(char *dest, struct commit_list *parents)

Since one commit can be rewritten to multiple commits now, you do not know 
how much space you need to add the parents.  Thus you should take a buf_p 
and a space_p, and use ALLOC_GROW() to grow the buffer as needed.

> +static int skip_one_line(char **buf_p, unsigned long *len_p)

Better rename get_one_line to get_line_length and export it, avoiding code 
duplication.  I have an upcoming patch series adding commit notes, which 
has that patch.

Besides, you do not _skip_ that line.  You use the line, but advance 
*buf_p before doing so.

> +static char *filter_index(char *orig_hex, struct commit *commit)

const char *, in both cases.  You do not plan to modify orig_hex, and you 
return a sha1_to_hex() buffer, which you also do not plan to modify.

> +{
> +	int argc;
> +	const char *argv[10];
> +	static char index_env[16+PATH_MAX];

Style: "16 + PATH_MAX".  That happens elsewhere, too.

> +/* Replace any (short) sha1 of a rewritten commit by the new (short) sha1 */
> +static char *rewrite_body(char *dest, unsigned long len, char *buf)
> +{
> +	unsigned char sha1[20];
> +
> +	while (len) {
> +		size_t ll = non_hex_len(buf, len);
> +		memcpy(dest, buf, ll);
> +		dest += ll;
> +		buf += ll;
> +		len -= ll;

Why not make it simpler?

		while (!ishex(buf[i]))
			dest[i] = buf[i++];

> +		ll = hex_len(buf, len);

It is really shorter (because you spare the whole hex_len() function, to 
say

		for (ll = 0; i + ll < len && ishex(buf[i + ll]); ll++)
			; /* do nothing */

> +		if (ll >= 8 && ll <= 40 &&

AFAICT our abbreviation allows for 4 characters and up.  Default 
abbreviation is 7, IIRC.

> +		    !get_short_sha1(buf, ll, sha1, 1) &&
> +		    !get_rewritten_sha1(sha1))
> +			memcpy(dest, sha1_to_hex(sha1), ll);
> +		else
> +			memcpy(dest, buf, ll);

What do you do if the rewritten sha1, truncated to ll characters, is 
ambiguous?  Wouldn't you need more than

> +static int is_ref_to_be_rewritten(const char *ref)
> +{
> +	unsigned char sha1[20];
> +	int flag;
> +
> +	if (prefixcmp(ref, "refs/"))
> +		return 0;
> +	if (!prefixcmp(ref, "refs/remotes/"))
> +		return 0;
> +	if (!prefixcmp(ref+5, original_prefix))

Should original_prefix not be "refs/original", then?

> +static int is_pruned(struct commit *commit, int path_pruning)
> +{
> +	if (commit->object.flags & PRUNED)
> +		return 1;

Hmm.  I thought about changing get_revision_1() to mark that commit as 
uninteresting, since the parents were already added.  But I guess this has 
too much side effect potential.

In any case, I think that "NO_MATCH" would be a more descriptive name.

> +	if (path_pruning &&
> +	    !(commit->object.flags & (TREECHANGE | UNINTERESTING)))
> +		return 1;

Why only with "path_pruning"?  Ah yes.  Because otherwise, you would 
assume "A" in "A..B" to be pruned.  But what do you do if someone says

	git rewrite-commits A.. x/y

because she wants _only_ commits later than A rewritten, but everything 
from A backwards kept as-is?

If I understand your code correctly, the given command would cut off the 
history at A.

I wonder why you bother at all: my impression was that the revisions you 
want to filter out here were already filtered out by 
revision.c:rewrite_parents()...

> +static void rewrite_sha1(struct object *obj, unsigned char *new_sha1)
> +{
> +	if (!hashcmp(obj->sha1, new_sha1))
> +		return;
> +
> +	add_rewrite_decoration(obj, new_sha1);

This is not so much "rewrite_sha1", as "append_rewritten_sha1", right?

> +/*
> + * Replace any parent that has been removed by its parents
> + * and return the number of new parents.
> + * We directly modify the parent list, so any libification
> + * should probably adapt this function.

I do not think that this code will be libified any time soon.

> +static int rewrite_parents(struct commit *commit, int path_pruning)

Of course, you could always pass a "struct commit_list 
**rewritten_parents_p" to this function.

> +		get_rewritten_sha1(sha1);
> +		if (!is_null_sha1(sha1)) {
> +			hashclr(sha1);
> +			rewrite_sha1(&list->item->object, sha1);

I guess you'll admit that it is unintuitive to read 
"get_rewritten_sha1(sha1); rewrite_sha1(o, sha1);".  I thought: "What?  
Again?"  IMHO that is a strong hint that "rewrite_sha1" is not an apt name 
for that function.

> +static int rewrite_ref(const char *refname, const unsigned char *sha1,
> +			int flags, void *cb_data)
> +{
> +	int prefix_len;
> +	int len;
> +	char buffer[256], *p;
> +	struct object *obj = parse_object(sha1);
> +	unsigned char new_sha1[20];
> +	struct commit *commit;
> +	int pruned;
> +
> +	if (!obj)
> +		return 0;
> +	if (obj->type == OBJ_TAG)

Speaking of tags...  We will have to add (in a separate patch, to keep 
things reviewable) a method to rewrite them, too.

> +	if (!is_ref_to_be_rewritten(refname))
> +		return 0;

Hmm.  When looking at that code, I wonder if

	git rewrite-commits A..B

will rewrite C, too, if it happens to lie in A..B.  That would be not 
brilliant.

I guess you will need to copy the positive refs in revs->pending, if there 
are any, and later _not_ call for_each_ref if that list was empty.

> +	commit = lookup_commit_reference(sha1);
> +	pruned = is_pruned(commit, !!cb_data);
> +
> +	hashcpy(new_sha1, sha1);
> +	if (!pruned && get_rewritten_sha1(new_sha1))
> +		return 0;

So if pruned == 1, you _do_ rewrite it?  I'm not quite sure.  Care to 
explain?

> +static void filter_and_write_commit(char *commit_body, size_t len,
> +				    struct commit *commit, unsigned char *sha1)
> +{
> +	char commit_path[PATH_MAX];
> +	struct child_process cmd;
> +	int argc;
> +	const char *argv[10];
> +	int fd;
> +	const char *env[] = { commit_env, NULL };
> +	char hex[41];
> +	struct commit_list *list = NULL, **end = &list;
> +
> +	memcpy(commit_env+sizeof(commit_env)-41,
> +		sha1_to_hex(commit->object.sha1), 40);
> +
> +	fd = git_mkstemp(commit_path, sizeof(commit_path), ".commit_XXXXXX");;
> +	write_or_die(fd, commit_body, len);
> +
> +	argc = 0;
> +	argv[argc++] = "sh";
> +	argv[argc++] = "-c";
> +	argv[argc++] = commit_filter;
> +	argv[argc] = NULL;
> +	memset(&cmd, 0, sizeof(cmd));
> +	cmd.in = open(commit_path, O_RDONLY);
> +	if (cmd.in < 0)
> +		die("Unable to read commit from file '%s'", commit_path);
> +	unlink(commit_path);

This will fail on Windows.  You do not catch that error, so it is almost 
fine: just put the file into rewrite_dir, so the leftovers will be removed 
later anyway.

> +		hashclr(sha1);
> +		commit->object.flags |= PRUNED;
> +		/*
> +		 * If the filter returns two or more commits,
> +		 * we consider the original commit to have been
> +		 * removed and put the list in the old commit's
> +		 * parent list so that all the old commit's children
> +		 * will copy them.
> +		 */
> +		if (list) {
> +			free_commit_list(commit->parents);
> +			commit->parents = list;
> +		} else
> +		    add_sha1_map(commit->object.sha1, NULL);

That is almost certainly wrong.  If you step away from the notion that 
get_rewritten_sha1() returns _one_ SHA-1, all will become clearer.  And 
the filters should know about them SHA-1s, too.

> +	/* Make enough remove for n (possibly extra) parents */
> +	p = buf = xmalloc(orig_len + n*48);

As stated above, I'd prefer ALLOC_GROW().

> +	p = rewrite_header(p, &orig_len, &orig_buf, commit);
> +	p = rewrite_body(p, orig_len, orig_buf);
> +	if (!commit_filter) {
> +		if (write_sha1_file(buf, p-buf, commit_type, sha1))
> +			die("Unable to write new commit");
> +		add_sha1_map(commit->object.sha1, sha1);
> +	} else
> +		filter_and_write_commit(buf, p-buf, commit, sha1);
> +	free(buf);

You can really discard the commit->buffer, too, no?

> +
> +	rewrite_sha1(&commit->object, sha1);
> +}

> +static char aux_functions[] =
> +"export GIT_REWRITE_DIR=`pwd`\n"

Better put the correct absolute path there.

> +"commit()\n"
> +"{\n"
> +"	git-hash-object -w -t commit --stdin\n"
> +"}\n"
> +"map()\n"
> +"{\n"
> +"	# if it was not rewritten, take the original\n"
> +"	if test -r \"$GIT_REWRITE_DIR/map/$1\"\n"
> +"	then\n"
> +"		cat \"$GIT_REWRITE_DIR/map/$1\"\n"
> +"	else\n"
> +"		echo \"$1\"\n"
> +"	fi\n"
> +"}\n"
> +"cd t\n";

Then you do not need this extra fork() and cd all the time the filters 
source the helper file.

You do not even need the one static global buffer, if you put that script 
into the function writing it, inside an fprintf() call.

> +static char filter_prefix[] = ". aux;";

And here, I'd put the absolute path, too.

> +static char *create_filter(const char *command)
> +{
> +	int prefix_len = sizeof(filter_prefix)-1;
> +	int command_len = strlen(command);
> +	char *filter = xmalloc(prefix_len + command_len + 1);
> +
> +	memcpy(filter, filter_prefix, prefix_len);
> +	memcpy(filter+prefix_len, command, command_len+1);
> +	return filter;
> +}

I really like that approach.  This makes it so much more convenient for 
users.

> +static void setup_temp_dir()
> +{
> +	int aux;
> +
> +	absolute_git_dir = create_absolute_path(get_git_dir());
> +	setenv(GIT_DIR_ENVIRONMENT, absolute_git_dir, 1);
> +
> +	if (!rewrite_dir) {
> +		rewrite_dir = xstrdup(git_path("rewrite"));

I might read the code wrong, but this does not guarantee that rewrite_dir 
is an absolute path, right?

> +	if (mkdir(mkpath("%s/map", rewrite_dir), 0777))
> +		die("unable to create map directory '%s/map'", rewrite_dir);

Maybe make this dependent on --write-sha1-mappings, and have a check in 
"map ()"?

> +	if (index_filter || commit_filter)
> +		setup_temp_dir();
> +	else
> +		/* They'll never know.  BWUHAHA */
> +		write_sha1_mapping = 0;

I like the comment ;-)

> +	prepare_revision_walk(&rev);
> +	while ((commit = get_revision(&rev)) != NULL) {
> +		rewrite_commit(commit, !!rev.prune_fn);
> +	}

Please lose the curly brackets for single lines; otherwise it would look 
too perl like.

> +	rm_rf(git_path("refs/%s", original_prefix));

Would it not be better to move refs/original to refs/original/original?

Pooh.  A lot of comments.  Please take that as a sign that I am interested 
in rewrite-commits.  BTW did I mention already that I like the name 
"rewrite-commits"?

Ciao,
Dscho

Re: [PATCH 6/6] Add git-rewrite-commits

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> +Examples
>> +--------
>> +
>> +Suppose you want to remove a file (containing confidential information
>> +or copyright violation) from all commits:
>> +
>> +----------------------------------------------------------------------------
>> +git rewrite-commits --index-filter 'git update-index --remove filename || :'
>
> We seem to prefer "$ git" instead of just "git" in the other man pages' 
> examples.

"git update-index --remove Foo" does not remove the index entry
Foo if the file Foo still exists in the working tree (use "git
update-index --force-remove" for that).

But this leads to more fundamental issues.  It is not obvious
from the description what environment rewrite-commits runs in.
Does it run at the toplevel of the current working tree, or is
it run in a separate temporary directory like filter-branch
does?  What "index" and "HEAD" do operations done by filters
affect (I think it is safe to assume that readers familiar
enough with other parts of git would be able to guess that
filters should operate on the "HEAD" and index given by
rewrite-commits to its execution environment without mucking
with GIT_DIR nor GIT_INDEX_FILE)?  Are filters allowed to modify
files in the working tree, and if so what is the consequence of
doing so?

>> +----------------------------------------------------------------------------
>> +
>> +Now, you will get the rewritten history saved in your current branch
>> +(the old branch is saved in refs/original).
>
> 						The "|| :" construct 
> + prevents the filter to fail when the given file was not present in the 
> + index.

prevents the filter from failing?  But is that really what we
want?  Why are we ignoring the error, and if there is a valid
reason to ignore shouldn't we explain why?

>> +To move the whole tree into a subdirectory, or remove it from there:
>> +
>> +---------------------------------------------------------------
>> +git rewrite-commits --index-filter \
>> +	'git ls-files -s | sed "s-\t-&newsubdir/-" |
>> +		GIT_INDEX_FILE=$GIT_INDEX_FILE.new \
>> +			git update-index --index-info &&
>> +	 mv $GIT_INDEX_FILE.new $GIT_INDEX_FILE'
>> +---------------------------------------------------------------

I see only one operation in the example, and "or remove it from
there" confuses the reader.

I'll refrain from comments on the code right now, until I read
the series over.

Re: [PATCH 6/6] Add git-rewrite-commits

[Sven Verdoolaege]

On Sat, Jul 14, 2007 at 01:49:59PM +0100, Johannes Schindelin wrote:
> Pooh.  A lot of comments.  Please take that as a sign that I am interested 

Thanks.  I'll look through them tomorrow.

> in rewrite-commits.  BTW did I mention already that I like the name 
> "rewrite-commits"?

You should like it.  You suggested it.
(http://article.gmane.org/gmane.comp.version-control.git/45637)
Not that I had read that when I named it.  I'm about 2.5 months
behind on my git mailing list reading.

skimo

Re: [PATCH 6/6] Add git-rewrite-commits

[Sven Verdoolaege]

On Sat, Jul 14, 2007 at 12:26:01PM -0700, Junio C Hamano wrote:
> But this leads to more fundamental issues.  It is not obvious
> from the description what environment rewrite-commits runs in.

I'll try to make that more clear in the next round.

> >> +To move the whole tree into a subdirectory, or remove it from there:
> >> +
> >> +---------------------------------------------------------------
> >> +git rewrite-commits --index-filter \
> >> +	'git ls-files -s | sed "s-\t-&newsubdir/-" |
> >> +		GIT_INDEX_FILE=$GIT_INDEX_FILE.new \
> >> +			git update-index --index-info &&
> >> +	 mv $GIT_INDEX_FILE.new $GIT_INDEX_FILE'
> >> +---------------------------------------------------------------
> 
> I see only one operation in the example, and "or remove it from
> there" confuses the reader.

I found it confusing too (I copied it from the filter-branch manual).
I'll remove it.

skimo

Re: [PATCH 6/6] Add git-rewrite-commits

[Sven Verdoolaege]

On Sat, Jul 14, 2007 at 01:49:59PM +0100, Johannes Schindelin wrote:
> On Thu, 12 Jul 2007, skimo@liacs.nl wrote:
> > +#include "grep.h"
> 
> I did not compile test, but do you really need that?

Not anymore.

> > +struct decoration rewrite_decoration = { "rewritten as" };
> > [...]
> > +
> > +struct rewrite_decoration {
> > +	struct rewrite_decoration *next;
> > +	unsigned char sha1[20];
> > +};
> 
> I wonder why you give the instance of a "struct decoration" the same name 
> as that of a _different_ struct. IOW it is confusing that there is a 
> "struct rewrite_decoration", but the variable "rewrite_decoration" is no 
> instance of that struct.

I was just following Linus's example.

> > +static char *add_parents(char *dest, struct commit_list *parents)
> 
> Since one commit can be rewritten to multiple commits now, you do not know 
> how much space you need to add the parents.

I count them beforehand.

> > +		    !get_short_sha1(buf, ll, sha1, 1) &&
> > +		    !get_rewritten_sha1(sha1))
> > +			memcpy(dest, sha1_to_hex(sha1), ll);
> > +		else
> > +			memcpy(dest, buf, ll);
> 
> What do you do if the rewritten sha1, truncated to ll characters, is 
> ambiguous?  Wouldn't you need more than

More than what?
Are you saying I should add some more of it then?
(As you can see, I simply don't consider that case here, so right now
I just leave it possibly ambiguous.)

> > +	if (!prefixcmp(ref+5, original_prefix))
> 
> Should original_prefix not be "refs/original", then?

The idea was that we could add a command line option later
to override it.

> > +static int is_pruned(struct commit *commit, int path_pruning)
> > +{
> > +	if (commit->object.flags & PRUNED)
> > +		return 1;
> 
> Hmm.  I thought about changing get_revision_1() to mark that commit as 
> uninteresting, since the parents were already added.  But I guess this has 
> too much side effect potential.
> 
> In any case, I think that "NO_MATCH" would be a more descriptive name.

Right now, it's only set for matching stuff, but it could be set
for other kinds of pruning too.

> > +	if (path_pruning &&
> > +	    !(commit->object.flags & (TREECHANGE | UNINTERESTING)))
> > +		return 1;
> 
> Why only with "path_pruning"?  Ah yes.  Because otherwise, you would 
> assume "A" in "A..B" to be pruned.

TREECHANGE is only set when path pruning is in effect.
If I didn't check for path_pruning, then all commits would be
considered to have been pruned.  (Or am I missing something?
Honestyl, I found all that TREECHANGE stuff difficult to follow.)

revision.c itself is also riddled with "prune_fn && ".
Wouldn't it make sense to invert the meaning of this bit and call
it, say, PRUNED, so that the default is off and you would only
have to check if the bit was set ?

> I wonder why you bother at all: my impression was that the revisions you 
> want to filter out here were already filtered out by 
> revision.c:rewrite_parents()...

I also want to make reference that pointed to something that has
been pruned (in either way) to now point to something in the new
history.

> > +static void rewrite_sha1(struct object *obj, unsigned char *new_sha1)
> > +{
> > +	if (!hashcmp(obj->sha1, new_sha1))
> > +		return;
> > +
> > +	add_rewrite_decoration(obj, new_sha1);
> 
> This is not so much "rewrite_sha1", as "append_rewritten_sha1", right?

Well, it's only called once on each commit.

> > +		get_rewritten_sha1(sha1);
> > +		if (!is_null_sha1(sha1)) {
> > +			hashclr(sha1);
> > +			rewrite_sha1(&list->item->object, sha1);
> 
> I guess you'll admit that it is unintuitive to read 
> "get_rewritten_sha1(sha1); rewrite_sha1(o, sha1);".  I thought: "What?  
> Again?"  IMHO that is a strong hint that "rewrite_sha1" is not an apt name 
> for that function.

I guess our brains work in a slightly different way.

> Hmm.  When looking at that code, I wonder if
> 
> 	git rewrite-commits A..B
> 
> will rewrite C, too, if it happens to lie in A..B.  That would be not 
> brilliant.

That was the idea.  Why is it not brilliant?
What would you like this to do instead?

Assume that in your new history all the commit that used to be pointed
to have been removed.  If these pointers are left to point to the
old history, then how are you going to get at the new history?

> > +	commit = lookup_commit_reference(sha1);
> > +	pruned = is_pruned(commit, !!cb_data);
> > +
> > +	hashcpy(new_sha1, sha1);
> > +	if (!pruned && get_rewritten_sha1(new_sha1))
> > +		return 0;
> 
> So if pruned == 1, you _do_ rewrite it?  I'm not quite sure.  Care to 
> explain?

Yes.  See above.

> > +	cmd.in = open(commit_path, O_RDONLY);
> > +	if (cmd.in < 0)
> > +		die("Unable to read commit from file '%s'", commit_path);
> > +	unlink(commit_path);
> 
> This will fail on Windows.  You do not catch that error, so it is almost 
> fine: just put the file into rewrite_dir, so the leftovers will be removed 
> later anyway.

You mean unlinking an opened file?

> 
> > +		hashclr(sha1);
> > +		commit->object.flags |= PRUNED;
> > +		/*
> > +		 * If the filter returns two or more commits,
> > +		 * we consider the original commit to have been
> > +		 * removed and put the list in the old commit's
> > +		 * parent list so that all the old commit's children
> > +		 * will copy them.
> > +		 */
> > +		if (list) {
> > +			free_commit_list(commit->parents);
> > +			commit->parents = list;
> > +		} else
> > +		    add_sha1_map(commit->object.sha1, NULL);
> 
> That is almost certainly wrong.

What specifically ?

> If you step away from the notion that 
> get_rewritten_sha1() returns _one_ SHA-1, all will become clearer.  And 
> the filters should know about them SHA-1s, too.

The filters do know about them.  The corresponding map file contains
all the new SHA1s.

> > +		add_sha1_map(commit->object.sha1, sha1);
> > +	} else
> > +		filter_and_write_commit(buf, p-buf, commit, sha1);
> > +	free(buf);
> 
> You can really discard the commit->buffer, too, no?

I'm not familiar enough with the internals to say for sure.

> > +static void setup_temp_dir()
> > +{
> > +	int aux;
> > +
> > +	absolute_git_dir = create_absolute_path(get_git_dir());
> > +	setenv(GIT_DIR_ENVIRONMENT, absolute_git_dir, 1);
> > +
> > +	if (!rewrite_dir) {
> > +		rewrite_dir = xstrdup(git_path("rewrite"));
> 
> I might read the code wrong, but this does not guarantee that rewrite_dir 
> is an absolute path, right?

It doesn't right now.

> > +	if (mkdir(mkpath("%s/map", rewrite_dir), 0777))
> > +		die("unable to create map directory '%s/map'", rewrite_dir);
> 
> Maybe make this dependent on --write-sha1-mappings, and have a check in 
> "map ()"?

I'll leave that to you :-)

> > +	while ((commit = get_revision(&rev)) != NULL) {
> > +		rewrite_commit(commit, !!rev.prune_fn);
> > +	}
> 
> Please lose the curly brackets for single lines; otherwise it would look 
> too perl like.

That would be
	
	rewrite_commit($commit) while $commit = get_revision;

> > +	rm_rf(git_path("refs/%s", original_prefix));
> 
> Would it not be better to move refs/original to refs/original/original?

Is that what you want?
You'd end up with refs/original/original/original/original/original/original/original/
pretty quickly.

skimo

Re: [PATCH 6/6] Add git-rewrite-commits

[Johannes Schindelin]

Hi,

On Sun, 15 Jul 2007, Sven Verdoolaege wrote:

> On Sat, Jul 14, 2007 at 01:49:59PM +0100, Johannes Schindelin wrote:
> > On Thu, 12 Jul 2007, skimo@liacs.nl wrote:
> > > +struct decoration rewrite_decoration = { "rewritten as" };
> > > [...]
> > > +
> > > +struct rewrite_decoration {
> > > +	struct rewrite_decoration *next;
> > > +	unsigned char sha1[20];
> > > +};
> > 
> > I wonder why you give the instance of a "struct decoration" the same name 
> > as that of a _different_ struct. IOW it is confusing that there is a 
> > "struct rewrite_decoration", but the variable "rewrite_decoration" is no 
> > instance of that struct.
> 
> I was just following Linus's example.

*Sigh*  I hate to repeat arguments.

> > > +static char *add_parents(char *dest, struct commit_list *parents)
> > 
> > Since one commit can be rewritten to multiple commits now, you do not 
> > know how much space you need to add the parents.
> 
> I count them beforehand.

Yes.  And it is extremely hard to follow through your code, just to verify 
that it really works.  Using ALLOC_GROW instead of trying to play cute 
games would spare these tours, and I am quite convinced that it makes for 
less bugs.  If not now, then when somebody _else_ than you touches the 
code.

> > > +		    !get_short_sha1(buf, ll, sha1, 1) &&
> > > +		    !get_rewritten_sha1(sha1))
> > > +			memcpy(dest, sha1_to_hex(sha1), ll);
> > > +		else
> > > +			memcpy(dest, buf, ll);
> > 
> > What do you do if the rewritten sha1, truncated to ll characters, is 
> > ambiguous?  Wouldn't you need more than
> 
> More than what?

Right.  I did not complete that sentence.  But isn't it obvious?  Usually, 
you put in short, but (at least for some time) _unambiguous_ short names.  
IMHO people would expect the same to be true of rewritten commit messages.

> Are you saying I should add some more of it then? (As you can see, I 
> simply don't consider that case here, so right now I just leave it 
> possibly ambiguous.)

Yes, I saw that.  Just wanted to remind you that some people might 
consider this a bug.

> > > +	if (!prefixcmp(ref+5, original_prefix))
> > 
> > Should original_prefix not be "refs/original", then?
> 
> The idea was that we could add a command line option later to override 
> it.

Okay, I'll try again.  You do not use the canonical form.  Why not use 
it, if only for consistency?  Besides, if you want to be able to override 
it with a command line option, you _will_ have to canonicalise _that_ 
original_prefix.

> > > +static int is_pruned(struct commit *commit, int path_pruning)
> > > +{
> > > +	if (commit->object.flags & PRUNED)
> > > +		return 1;
> > 
> > Hmm.  I thought about changing get_revision_1() to mark that commit as 
> > uninteresting, since the parents were already added.  But I guess this 
> > has too much side effect potential.
> > 
> > In any case, I think that "NO_MATCH" would be a more descriptive name.
> 
> Right now, it's only set for matching stuff, but it could be set for 
> other kinds of pruning too.

Yes, it could.

> > > +	if (path_pruning &&
> > > +	    !(commit->object.flags & (TREECHANGE | UNINTERESTING)))
> > > +		return 1;
> > 
> > Why only with "path_pruning"?  Ah yes.  Because otherwise, you would 
> > assume "A" in "A..B" to be pruned.
> 
> TREECHANGE is only set when path pruning is in effect.
> If I didn't check for path_pruning, then all commits would be
> considered to have been pruned.  (Or am I missing something?
> Honestyl, I found all that TREECHANGE stuff difficult to follow.)

AFAICT TREECHANGE means that parents were rewritten.

> revision.c itself is also riddled with "prune_fn && ".
> Wouldn't it make sense to invert the meaning of this bit and call
> it, say, PRUNED, so that the default is off and you would only
> have to check if the bit was set ?

You meant the TREECHANGE bit?  No.

BTW what do you plan to do about my objection to UNINTERESTING, given the 
example "git rewrite-commits A..B x/y"?

> > I wonder why you bother at all: my impression was that the revisions 
> > you want to filter out here were already filtered out by 
> > revision.c:rewrite_parents()...
> 
> I also want to make reference that pointed to something that has
> been pruned (in either way) to now point to something in the new
> history.

I did not understand that intention.  Maybe I am too dumb, that's all.

> > > +static void rewrite_sha1(struct object *obj, unsigned char *new_sha1)
> > > +{
> > > +	if (!hashcmp(obj->sha1, new_sha1))
> > > +		return;
> > > +
> > > +	add_rewrite_decoration(obj, new_sha1);
> > 
> > This is not so much "rewrite_sha1", as "append_rewritten_sha1", right?
> 
> Well, it's only called once on each commit.

Didn't I mention that it was a severe limitation to think of the sha1 
mapping of a 1-to-1 mapping?  Think of it more as a relation.

> > > +		get_rewritten_sha1(sha1);
> > > +		if (!is_null_sha1(sha1)) {
> > > +			hashclr(sha1);
> > > +			rewrite_sha1(&list->item->object, sha1);
> > 
> > I guess you'll admit that it is unintuitive to read 
> > "get_rewritten_sha1(sha1); rewrite_sha1(o, sha1);".  I thought: "What?  
> > Again?"  IMHO that is a strong hint that "rewrite_sha1" is not an apt 
> > name for that function.
> 
> I guess our brains work in a slightly different way.

I doubt that.  What would you expect a code

	get_tagged_commit(commit);
	tag_commit(tag, commit);

to do, if not tag a tagged commit _again_?  So I bet if it wasn't your 
code to begin with, you would have experienced the same confusion as me.

> > Hmm.  When looking at that code, I wonder if
> > 
> > 	git rewrite-commits A..B
> > 
> > will rewrite C, too, if it happens to lie in A..B.  That would be not 
> > brilliant.
> 
> That was the idea.  Why is it not brilliant?

Because it is called rewrite-commits, not 
rewrite-all-refs-that-touch-this-commit-range.

> What would you like this to do instead?

Only touch the positive refs given in the command line.  If you say 
"--all", then it's all.  If you say "A..B", then it's "B".

> Assume that in your new history all the commit that used to be pointed 
> to have been removed.  If these pointers are left to point to the old 
> history, then how are you going to get at the new history?

I still have the superproject splitting as the main application in mind.  
Only for big applications like these does the performance improvement of 
rewrite-commits over filter-branch buy us anything.

> > > +	commit = lookup_commit_reference(sha1);
> > > +	pruned = is_pruned(commit, !!cb_data);
> > > +
> > > +	hashcpy(new_sha1, sha1);
> > > +	if (!pruned && get_rewritten_sha1(new_sha1))
> > > +		return 0;
> > 
> > So if pruned == 1, you _do_ rewrite it?  I'm not quite sure.  Care to 
> > explain?
> 
> Yes.  See above.

Sorry, this explanation does not help me.  My impression was that a pruned 
commit should be _mapped_ to the rewritten sha1s of the original parents, 
but not be rewritten.

> > > +	cmd.in = open(commit_path, O_RDONLY);
> > > +	if (cmd.in < 0)
> > > +		die("Unable to read commit from file '%s'", commit_path);
> > > +	unlink(commit_path);
> > 
> > This will fail on Windows.  You do not catch that error, so it is almost 
> > fine: just put the file into rewrite_dir, so the leftovers will be removed 
> > later anyway.
> 
> You mean unlinking an opened file?

Yes, I mean unlinking an opened file.

> > > +		hashclr(sha1);
> > > +		commit->object.flags |= PRUNED;
> > > +		/*
> > > +		 * If the filter returns two or more commits,
> > > +		 * we consider the original commit to have been
> > > +		 * removed and put the list in the old commit's
> > > +		 * parent list so that all the old commit's children
> > > +		 * will copy them.
> > > +		 */
> > > +		if (list) {
> > > +			free_commit_list(commit->parents);
> > > +			commit->parents = list;
> > > +		} else
> > > +		    add_sha1_map(commit->object.sha1, NULL);
> > 
> > That is almost certainly wrong.
> 
> What specifically ?

It takes a few minutes to verify that the correct calls to add_sha1_map() 
are done in every case.

Also, why should commit->parents be reset only in the multiple-sha1 case?

But most seriously, why do you replace the _parents_ of a given commit by 
the sha1s of the _rewritten_ commits?  They are not even the rewritten 
_parents_.

> > If you step away from the notion that 
> > get_rewritten_sha1() returns _one_ SHA-1, all will become clearer.  And 
> > the filters should know about them SHA-1s, too.
> 
> The filters do know about them.  The corresponding map file contains
> all the new SHA1s.

But the code does not reflect that in some parts.  For example when you 
call "get_rewritten_sha1(sha1);" which one is it?  And what is it if the 
commit was rewritten to no commit at all, i.e. the history was cut 
forcefully?

> > > +		add_sha1_map(commit->object.sha1, sha1);
> > > +	} else
> > > +		filter_and_write_commit(buf, p-buf, commit, sha1);
> > > +	free(buf);
> > 
> > You can really discard the commit->buffer, too, no?
> 
> I'm not familiar enough with the internals to say for sure.

Hmm.  You play with them pretty heavily, then, for example when you just 
reset commit->parents.

> > > +static void setup_temp_dir()
> > > +{
> > > +	int aux;
> > > +
> > > +	absolute_git_dir = create_absolute_path(get_git_dir());
> > > +	setenv(GIT_DIR_ENVIRONMENT, absolute_git_dir, 1);
> > > +
> > > +	if (!rewrite_dir) {
> > > +		rewrite_dir = xstrdup(git_path("rewrite"));
> > 
> > I might read the code wrong, but this does not guarantee that rewrite_dir 
> > is an absolute path, right?
> 
> It doesn't right now.

Umm.  Sorry to ask so bluntly: are you planning to change that?

> > > +	if (mkdir(mkpath("%s/map", rewrite_dir), 0777))
> > > +		die("unable to create map directory '%s/map'", rewrite_dir);
> > 
> > Maybe make this dependent on --write-sha1-mappings, and have a check in 
> > "map ()"?
> 
> I'll leave that to you :-)

IMHO the first part should be in the initial commit of 
builtin-rewrite-commits.c, so I will not do that.

> > > +	while ((commit = get_revision(&rev)) != NULL) {
> > > +		rewrite_commit(commit, !!rev.prune_fn);
> > > +	}
> > 
> > Please lose the curly brackets for single lines; otherwise it would look 
> > too perl like.
> 
> That would be
> 	
> 	rewrite_commit($commit) while $commit = get_revision;

No, it would not, because it is not valid C.

> > > +	rm_rf(git_path("refs/%s", original_prefix));
> > 
> > Would it not be better to move refs/original to 
> > refs/original/original?
> 
> Is that what you want? You'd end up with 
> refs/original/original/original/original/original/original/original/ 
> pretty quickly.

We are pretty anal about not losing data too quickly, and possibly 
unwantedly.

It would be surprising to me if this would be the first program to change 
that behaviour.

IOW, refs/original/ is not a temporary directory that you easily blow 
away.  It is meant for the user to be inspected.  And it is the user's 
obligation to say when that happened, and that it should go now.

Thinking about it again, I'd even go so far as to disallow operation with 
an existing refs/original/.

Ciao,
Dscho

Re: [PATCH 6/6] Add git-rewrite-commits

[Sven Verdoolaege]

On Mon, Jul 16, 2007 at 01:38:11AM +0100, Johannes Schindelin wrote:
> On Sun, 15 Jul 2007, Sven Verdoolaege wrote:
> > > > +	if (path_pruning &&
> > > > +	    !(commit->object.flags & (TREECHANGE | UNINTERESTING)))
> > > > +		return 1;
> > > 
> > > Why only with "path_pruning"?  Ah yes.  Because otherwise, you would 
> > > assume "A" in "A..B" to be pruned.
> > 
> > TREECHANGE is only set when path pruning is in effect.
> > If I didn't check for path_pruning, then all commits would be
> > considered to have been pruned.  (Or am I missing something?
> > Honestly, I found all that TREECHANGE stuff difficult to follow.)
> 
> AFAICT TREECHANGE means that parents were rewritten.

I think you'll find that if all commits touch a path in the
path specifiers then all commits will have TREECHANGE set and
so no parents will be rewritten.

> > revision.c itself is also riddled with "prune_fn && ".
> > Wouldn't it make sense to invert the meaning of this bit and call
> > it, say, PRUNED, so that the default is off and you would only
> > have to check if the bit was set ?
> 
> You meant the TREECHANGE bit?  No.

Yes.  Why?

> BTW what do you plan to do about my objection to UNINTERESTING, given the 
> example "git rewrite-commits A..B x/y"?

That was based on an apparent misunderstanding of my code
that I tried to address above.  I did not intend to do what
you claim I do and a quick test confirms that my code does
indeed not to what you claim it does.

More specifically, the history will not be cut off at A
because A is marked UNINTERESTING and is therefore not considered
to have been pruned.
A commit is considered pruned if it was either explicitly marked
as such or if TREECHANGE is not set, but the later check (in is_pruned)
is only done on commits that were checked for tree changes.

skimo

Re: [PATCH 6/6] Add git-rewrite-commits

[Sven Verdoolaege]

On Mon, Jul 16, 2007 at 01:38:11AM +0100, Johannes Schindelin wrote:
> Didn't I mention that it was a severe limitation to think of the sha1 
> mapping of a 1-to-1 mapping?  Think of it more as a relation.

The mapping is used in several operations.
First, there are several things that can happen to a commit

- it's pruned.  This includes, for me, path pruning, matching
  and a commit filter returning no SHA1s.
- it's rewritten to another commit that can be considered the
  "moral equivalent" of that commit.  This occurs when a commit
  is not pruned, but something else happened to the commit itself or
  one of its ancestors.  This excludes, for me, the case
  where a commit filter returns more than one SHA1.
- it's replaced by more than one SHA1.  This can only happen
  in a commit filter.

There are at least four operations in which this mapping is used:

- if the parents of a commit have been rewritten to one or more
  commits, then they are replaced by the new commits.
  If any parent has been pruned, then it is replaced by
  the result of applying this operation on _its_ parents.
- any reference (in refs/) that points to a rewritten or pruned
  commit is removed and
    * if the commit was rewritten to a single commit, then it is
      replaced by this commit
    * otherwise, there is no moral equivalent single commit, but
      we want to ensure we can still access the new commits, so
      I create several references, either to each of the many
      commits the old commit was rewritten to, or to each of
      its nearest unpruned ancestors (i.e., the same set as
      described in the previous operation).
- a SHA1 of a commit that appears in a commit message is replaced
  by the rewritten commit iff it was rewritten to a single commit.
  That is, if the commit was pruned or rewritten (through a commit
  filter to more than one commit), then the SHA1 is left alone.
- the mapping available to filters
    * if the commit was pruned, an empty file is created
    * otherwise a file is created containing all rewritten SHA1s

I understand you want the second operation to only apply
to refs explicitly mentioned on the command line.
What else would you like to change?

skimo

Re: [PATCH 6/6] Add git-rewrite-commits

[Sven Verdoolaege]

On Mon, Jul 16, 2007 at 10:04:04PM +0200, Sven Verdoolaege wrote:
> - a SHA1 of a commit that appears in a commit message is replaced
>   by the rewritten commit iff it was rewritten to a single commit.
>   That is, if the commit was pruned or rewritten (through a commit
>   filter) to more than one commit, then the SHA1 is left alone.

Sorry.  I misremembered.  I considered doing it this way, but
then thought it was better to replace the SHA1 with a(n abbreviated)
null SHA1 to signify that the commit had gone.

skimo

Re: [PATCH 6/6] Add git-rewrite-commits

[Johannes Schindelin]

Hi,

On Mon, 16 Jul 2007, Sven Verdoolaege wrote:

> On Mon, Jul 16, 2007 at 01:38:11AM +0100, Johannes Schindelin wrote:
> > On Sun, 15 Jul 2007, Sven Verdoolaege wrote:
> > > > > +	if (path_pruning &&
> > > > > +	    !(commit->object.flags & (TREECHANGE | UNINTERESTING)))
> > > > > +		return 1;
> > > > 
> > > > Why only with "path_pruning"?  Ah yes.  Because otherwise, you would 
> > > > assume "A" in "A..B" to be pruned.
> > > 
> > > TREECHANGE is only set when path pruning is in effect.
> > > If I didn't check for path_pruning, then all commits would be
> > > considered to have been pruned.  (Or am I missing something?
> > > Honestly, I found all that TREECHANGE stuff difficult to follow.)
> > 
> > AFAICT TREECHANGE means that parents were rewritten.
> 
> I think you'll find that if all commits touch a path in the
> path specifiers then all commits will have TREECHANGE set and
> so no parents will be rewritten.

The code suggests otherwise.

But I really have to wonder: why do you play games with TREECHANGE?  I had 
the impression that commit->parents is set appropriately by the revision 
walker, and that you do not have to do _anything_ for that to work.

Maybe the "--grep" thing does not yet.  But then you should fix it in 
revision.c.  Not in builtin-rewrite-commits.c

> > > revision.c itself is also riddled with "prune_fn && ".
> > > Wouldn't it make sense to invert the meaning of this bit and call
> > > it, say, PRUNED, so that the default is off and you would only
> > > have to check if the bit was set ?
> > 
> > You meant the TREECHANGE bit?  No.
> 
> Yes.  Why?

Why invert the meaning of a perfectly fine bit?  Because you can?  It is 
working right now, and it is not even a buglet, so what is there to fix?

> > BTW what do you plan to do about my objection to UNINTERESTING, given 
> > the example "git rewrite-commits A..B x/y"?
> 
> That was based on an apparent misunderstanding of my code
> that I tried to address above.  I did not intend to do what
> you claim I do and a quick test confirms that my code does
> indeed not to what you claim it does.
> 
> More specifically, the history will not be cut off at A
> because A is marked UNINTERESTING and is therefore not considered
> to have been pruned.

Why do you test for TREECHANGE | UNINTERESTING then?

> A commit is considered pruned if it was either explicitly marked
> as such or if TREECHANGE is not set, but the later check (in is_pruned)
> is only done on commits that were checked for tree changes.

I don't understand.  What do you mean by "a commit is pruned"?  Does it 
mean that this commit was left out from the revision walk?  What does that 
have to do with TREECHANGE, which means that the parents set was modified?

Ciao,
Dscho

Re: [PATCH 6/6] Add git-rewrite-commits

[Johannes Schindelin]

Hi,

On Mon, 16 Jul 2007, Sven Verdoolaege wrote:

> On Mon, Jul 16, 2007 at 10:04:04PM +0200, Sven Verdoolaege wrote:
> > - a SHA1 of a commit that appears in a commit message is replaced
> >   by the rewritten commit iff it was rewritten to a single commit.
> >   That is, if the commit was pruned or rewritten (through a commit
> >   filter) to more than one commit, then the SHA1 is left alone.
> 
> Sorry.  I misremembered.  I considered doing it this way, but then 
> thought it was better to replace the SHA1 with a(n abbreviated) null 
> SHA1 to signify that the commit had gone.

Yes.  This shows that you did not get my objection.  The commit is not 
replaced with _no_ commits, but with _multiple_ commits.

Ciao,
Dscho

Re: [PATCH 6/6] Add git-rewrite-commits

[Johannes Schindelin]

Hi,

On Mon, 16 Jul 2007, Sven Verdoolaege wrote:

> On Mon, Jul 16, 2007 at 01:38:11AM +0100, Johannes Schindelin wrote:
> > Didn't I mention that it was a severe limitation to think of the sha1 
> > mapping of a 1-to-1 mapping?  Think of it more as a relation.
> 
> The mapping is used in several operations.
> First, there are several things that can happen to a commit
> 
> - it's pruned.  This includes, for me, path pruning, matching
>   and a commit filter returning no SHA1s.

Okay.

> - it's rewritten to another commit that can be considered the
>   "moral equivalent" of that commit.  This occurs when a commit
>   is not pruned, but something else happened to the commit itself or
>   one of its ancestors.  This excludes, for me, the case
>   where a commit filter returns more than one SHA1.

Okay.  For me it does not at all exclude that.  If I want to replace a 
commit by no commit, I write a commit-filter which does not return 
anything.  If I return more than one SHA1s, I damned well want all of 
those be the replacement "commit".

To say "the" replacement "commit", means to mistake the mapping as a 
function, a non-relation.

> - it's replaced by more than one SHA1.  This can only happen
>   in a commit filter.

For the moment, yes.

> There are at least four operations in which this mapping is used:
> 
> - if the parents of a commit have been rewritten to one or more
>   commits, then they are replaced by the new commits.

Yes, that is the primary use for the mapping.

>   If any parent has been pruned, then it is replaced by
>   the result of applying this operation on _its_ parents.

Why?  This is overy complicated.  If a commit has been pruned, why does 
the mapping not point to the _non-pruned_ parent? IOW if you have 
something like this:

	A - B - C - D - E - F

and all commits except A and F are pruned, the mapping for A, B, C, D and 
E should _all_ point to the (possibly rewritten) A.

> - any reference (in refs/) that points to a rewritten or pruned
>   commit is removed and
>     * if the commit was rewritten to a single commit, then it is
>       replaced by this commit
>     * otherwise, there is no moral equivalent single commit, but
>       we want to ensure we can still access the new commits, so
>       I create several references, either to each of the many
>       commits the old commit was rewritten to, or to each of
>       its nearest unpruned ancestors (i.e., the same set as
>       described in the previous operation).

I'd argue that it should be an error if a to-be-rewritten ref (and I still 
strongly disagree with you that all refs should be rewritten) would point 
to multiple commits.  Possibly overridable with "--allow-octopus-refs".  
But the default should be to error out.

> - a SHA1 of a commit that appears in a commit message is replaced
>   by the rewritten commit iff it was rewritten to a single commit.
>   That is, if the commit was pruned or rewritten (through a commit
>   filter to more than one commit), then the SHA1 is left alone.

Both this behaviour and the one you described in your reply are wrong.

> - the mapping available to filters
>     * if the commit was pruned, an empty file is created
>     * otherwise a file is created containing all rewritten SHA1s

As I stated above: it is utterly wrong to create an empty mapping for a 
commit that was pruned.  It does not take long to think of an example:

	A - B - C - D

Now, A and D get pruned.  Do you want the whole branch to vanish?  _Hell, 
no_.

> I understand you want the second operation to only apply to refs 
> explicitly mentioned on the command line.

You have to at least give the users a chance to grasp what they are doing.  
And if that means to change the semantics to something saner, then so be 
it.

Ciao,
Dscho

Re: [PATCH 6/6] Add git-rewrite-commits

[Sven Verdoolaege]

On Wed, Jul 18, 2007 at 12:02:50PM +0100, Johannes Schindelin wrote:
> Hi,
> 
> On Mon, 16 Jul 2007, Sven Verdoolaege wrote:
> 
> > On Mon, Jul 16, 2007 at 01:38:11AM +0100, Johannes Schindelin wrote:
> > > On Sun, 15 Jul 2007, Sven Verdoolaege wrote:
> > > > TREECHANGE is only set when path pruning is in effect.
> > > > If I didn't check for path_pruning, then all commits would be
> > > > considered to have been pruned.  (Or am I missing something?
> > > > Honestly, I found all that TREECHANGE stuff difficult to follow.)
> > > 
> > > AFAICT TREECHANGE means that parents were rewritten.
> > 
> > I think you'll find that if all commits touch a path in the
> > path specifiers then all commits will have TREECHANGE set and
> > so no parents will be rewritten.
> 
> The code suggests otherwise.

Check again.

> But I really have to wonder: why do you play games with TREECHANGE?  I had 
> the impression that commit->parents is set appropriately by the revision 
> walker,

Only for unpruned commits and the references (explicitly specified
on the command line if you wish) may have been pruned.

> > > > revision.c itself is also riddled with "prune_fn && ".
> > > > Wouldn't it make sense to invert the meaning of this bit and call
> > > > it, say, PRUNED, so that the default is off and you would only
> > > > have to check if the bit was set ?
> > > 
> > > You meant the TREECHANGE bit?  No.
> > 
> > Yes.  Why?
> 
> Why invert the meaning of a perfectly fine bit?  Because you can?  It is 
> working right now, and it is not even a buglet, so what is there to fix?

Because it is confusing.  As explained above, the bit doesn't have a
meaning of its own.  You can only interpret the bit if some other
conditions are met.
It would be even more confusing if it meant what you claim it means.

> 
> > > BTW what do you plan to do about my objection to UNINTERESTING, given 
> > > the example "git rewrite-commits A..B x/y"?
> > 
> > That was based on an apparent misunderstanding of my code
> > that I tried to address above.  I did not intend to do what
> > you claim I do and a quick test confirms that my code does
> > indeed not to what you claim it does.
> > 
> > More specifically, the history will not be cut off at A
> > because A is marked UNINTERESTING and is therefore not considered
> > to have been pruned.
> 
> Why do you test for TREECHANGE | UNINTERESTING then?

Exactly for the reason mentioned above.
If the commit is marked UNINTERESTING then it has not been pruned,
because it hasn't even been checked for TREECHANGE.

> > A commit is considered pruned if it was either explicitly marked
> > as such or if TREECHANGE is not set, but the later check (in is_pruned)
> > is only done on commits that were checked for tree changes.
> 
> I don't understand.  What do you mean by "a commit is pruned"?  Does it 
> mean that this commit was left out from the revision walk?  What does that 
> have to do with TREECHANGE, which means that the parents set was modified?

You just claim that that is what it means.  The code (see try_to_simplify_commit
where the bit is set) and a simple experiment (explained above) show otherwise.

skimo

Re: [PATCH 6/6] Add git-rewrite-commits

[Sven Verdoolaege]

On Wed, Jul 18, 2007 at 12:17:03PM +0100, Johannes Schindelin wrote:
> Okay.  For me it does not at all exclude that.  If I want to replace a 
> commit by no commit, I write a commit-filter which does not return 
> anything.  If I return more than one SHA1s, I damned well want all of 
> those be the replacement "commit".

So how about you telling me what it _means_ for one commit to
be replaced by more than one commit or at least giving me an
example?

> > - if the parents of a commit have been rewritten to one or more
> >   commits, then they are replaced by the new commits.
> 
> Yes, that is the primary use for the mapping.
> 
> >   If any parent has been pruned, then it is replaced by
> >   the result of applying this operation on _its_ parents.
> 
> Why?  This is overy complicated.  If a commit has been pruned, why does 
> the mapping not point to the _non-pruned_ parent?

It may not have any non-pruned parents and for the pruned ones, we
wouldn't want to lose the relation with the non-pruned ancestors.

> IOW if you have 
> something like this:
> 
> 	A - B - C - D - E - F
> 
> and all commits except A and F are pruned, the mapping for A, B, C, D and 
> E should _all_ point to the (possibly rewritten) A.

I'm not sure what you mean by "mapping" here, but the operation
described above would make all of B, C, D, E and F have (the
possibly rewritten) A as single parent (and parenthood was all
I was talking about above).

> > - a SHA1 of a commit that appears in a commit message is replaced
> >   by the rewritten commit iff it was rewritten to a single commit.
> >   That is, if the commit was pruned or rewritten (through a commit
> >   filter to more than one commit), then the SHA1 is left alone.
> 
> Both this behaviour and the one you described in your reply are wrong.

So tell me what you would do then and why that would make sense.

> > - the mapping available to filters
> >     * if the commit was pruned, an empty file is created
> >     * otherwise a file is created containing all rewritten SHA1s
> 
> As I stated above: it is utterly wrong to create an empty mapping for a 
> commit that was pruned.  It does not take long to think of an example:
> 
> 	A - B - C - D
> 
> Now, A and D get pruned.  Do you want the whole branch to vanish?  _Hell, 
> no_.

Define "vanish" and, again, tell me what you would do.

> You have to at least give the users a chance to grasp what they are doing.  
> And if that means to change the semantics to something saner, then so be 
> it.

Let's get things straight.  I've added the map files and the possibility
for a commit filter to return more than one commit because you asked me to.
I've tried to make sense of it, but if you think the behavior I defined
is not what it is supposed to be, then it is up to _you_ to tell me what
you think it should be instead of letting me guess.

I think I'll just remove the possibility for the commit filter to
return more than one SHA1 (or maybe even no SHA1s).
filter-branch doesn't seem to allow either of those either.

skimo