A few fast export fixups -- round 2

A few fast export fixups -- round 2

[newren]

This patch series fixes a few fast-export bugs I have come across,
plus some new testcases to verify the fixes, and finally a small
addition to the documentation.

Changes since v1:
  - fixed test issues identified by Johannes Sixt and Stephen Boyd
  - squashed patch #2 into patch #1

Elijah Newren (6):
  fast-export: Omit tags that tag trees
  fast-export: Make sure we show actual ref names instead of "(null)"
  fast-export: Do parent rewriting to avoid dropping relevant commits
  fast-export: Add a --tag-of-filtered-object option for newly dangling tags
  Add new fast-export testcases
  fast-export: Document the fact that git-rev-list arguments are accepted

 Documentation/git-fast-export.txt |   17 +++++++++
 builtin-fast-export.c             |   72 +++++++++++++++++++++++++++++++++---
 revision.c                        |   10 +----
 revision.h                        |    8 ++++
 t/t9301-fast-export.sh            |   63 +++++++++++++++++++++++++++++++-
 5 files changed, 155 insertions(+), 15 deletions(-)

[PATCH v2] fast-export: Omit tags that tag trees

[newren]

From: Elijah Newren <newren@gmail.com>

Commit c0582c53bcf4e83bba70e1ad23abbad31f96ebc8 introduced logic to just
omit tags that point to tree objects.  However, a case was missed
resulting in a tag being output which pointed at "mark :0", which would
cause fast-import to crash.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
Includes changes suggested by Stephen Boyd (squashing commits together
and moving the NEEDSWORK comment to where it is still relevant) and
Johannes Sixt (fixing the testcase to remain in the right directory
even if git init fails and to catch failures in git fast-export).

 builtin-fast-export.c  |    8 +++++++-
 t/t9301-fast-export.sh |    8 +++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/builtin-fast-export.c b/builtin-fast-export.c
index 6cef810..891e2d4 100644
--- a/builtin-fast-export.c
+++ b/builtin-fast-export.c
@@ -407,9 +407,15 @@ static void handle_tags_and_duplicates(struct string_list *extra_refs)
 	for (i = extra_refs->nr - 1; i >= 0; i--) {
 		const char *name = extra_refs->items[i].string;
 		struct object *object = extra_refs->items[i].util;
+		struct tag *tag;
 		switch (object->type) {
 		case OBJ_TAG:
-			handle_tag(name, (struct tag *)object);
+			tag = (struct tag *)object;
+			if (tag->tagged->type == OBJ_TREE) {
+				/* Ignore this tag altogether */
+				return;
+			}
+			handle_tag(name, tag);
 			break;
 		case OBJ_COMMIT:
 			/* create refs pointing to already seen commits */
diff --git a/t/t9301-fast-export.sh b/t/t9301-fast-export.sh
index 8c8a9e6..3f13e6b 100755
--- a/t/t9301-fast-export.sh
+++ b/t/t9301-fast-export.sh
@@ -271,8 +271,14 @@ test_expect_success 'set-up a few more tags for tag export tests' '
 	git tag -a tag-obj_tag-obj -m "tagging a tag" tree_tag-obj
 '
 
+test_expect_success 'tree_tag'        '
+	mkdir result &&
+	(cd result && git init) &&
+	git fast-export tree_tag > fe-stream &&
+	(cd result && git fast-import < ../fe-stream)
+'
+
 # NEEDSWORK: not just check return status, but validate the output
-test_expect_success 'tree_tag'        'git fast-export tree_tag'
 test_expect_success 'tree_tag-obj'    'git fast-export tree_tag-obj'
 test_expect_success 'tag-obj_tag'     'git fast-export tag-obj_tag'
 test_expect_success 'tag-obj_tag-obj' 'git fast-export tag-obj_tag-obj'
-- 
1.6.0.6

[PATCH v2] fast-export: Make sure we show actual ref names instead of "(null)"

[newren]

From: Elijah Newren <newren@gmail.com>

Under some cases (e.g. 'git fast-export --parents master -- COPYING' run
in git.git), the output included the lines
  reset (null)
  commit (null)
instead of
  reset refs/heads/master
  commit refs/heads/master
This simple change fixes that.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
(No change to this patch since the first series.)
As noted above, run 'git fast-export --parents master -- COPYING' in
git.git to see this bug triggered.

 builtin-fast-export.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/builtin-fast-export.c b/builtin-fast-export.c
index 891e2d4..9aa409b 100644
--- a/builtin-fast-export.c
+++ b/builtin-fast-export.c
@@ -531,6 +531,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 	get_tags_and_duplicates(&revs.pending, &extra_refs);
 
 	revs.topo_order = 1;
+	revs.show_source = 1;
 	if (prepare_revision_walk(&revs))
 		die("revision walk setup failed");
 	revs.diffopt.format_callback = show_filemodify;
-- 
1.6.0.6

[PATCH v2] fast-export: Do parent rewriting to avoid dropping relevant commits

[newren]

From: Elijah Newren <newren@gmail.com>

When specifying paths to export, parent rewriting must be turned on for
fast-export to output anything at all.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
(No change to this patch since the first series.)
Try 'git fast-export master -- COPYING' in git.git and notice how there
is no output at all (whereas 'git rev-list master -- COPYING' does show
some existing revisions).  This simple patch restores the missing file
and commit in the fast-export output.

 builtin-fast-export.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/builtin-fast-export.c b/builtin-fast-export.c
index 9aa409b..b60a97e 100644
--- a/builtin-fast-export.c
+++ b/builtin-fast-export.c
@@ -532,6 +532,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 
 	revs.topo_order = 1;
 	revs.show_source = 1;
+	revs.rewrite_parents = 1;
 	if (prepare_revision_walk(&revs))
 		die("revision walk setup failed");
 	revs.diffopt.format_callback = show_filemodify;
-- 
1.6.0.6

[PATCH v2] fast-export: Add a --tag-of-filtered-object option for newly dangling tags

[newren]

From: Elijah Newren <newren@gmail.com>

When providing a list of paths to limit what is exported, the object that
a tag points to can be filtered out entirely.  This new switch allows
the user to specify what should happen to the tag in such a case.  The
default action, 'abort' will exit with an error message.  With 'drop', the
tag will simply be omitted from the output.  With 'rewrite', if the object
tagged was a commit, the tag will be modified to tag an ancestor of the
removed commit.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
(No change to this patch since the first series.)
When providing a list of paths to limit what is exported, the object that
a tag points to can be filtered out entirely.  This new switch allows
the user to specify what should happen to the tag in such a case.  The
default action, 'abort' will exit with an error message.  With 'drop', the
tag will simply be omitted from the output.  With 'rewrite', if the object
tagged was a commit, the tag will be modified to tag an ancestor of the
removed commit.

 Documentation/git-fast-export.txt |   11 ++++++
 builtin-fast-export.c             |   64 +++++++++++++++++++++++++++++++++---
 revision.c                        |   10 +-----
 revision.h                        |    8 +++++
 4 files changed, 79 insertions(+), 14 deletions(-)

diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
index 0c9eb56..194abde 100644
--- a/Documentation/git-fast-export.txt
+++ b/Documentation/git-fast-export.txt
@@ -36,6 +36,17 @@ when encountering a signed tag.  With 'strip', the tags will be made
 unsigned, with 'verbatim', they will be silently exported
 and with 'warn', they will be exported, but you will see a warning.
 
+--tag-of-filtered-object=(abort|drop|rewrite)::
+	Specify how to handle tags whose tagged objectis filtered out.
+	Since revisions and files to export can be limited by path,
+	tagged objects may be filtered completely.
++
+When asking to 'abort' (which is the default), this program will die
+when encountering such a tag.  With 'drop' it will omit such tags from
+the output.  With 'rewrite', if the tagged object is a commit, it will
+rewrite the tag to tag an ancestor commit (via parent rewriting; see
+linkgit:git-rev-list[1])
+
 -M::
 -C::
 	Perform move and/or copy detection, as described in the
diff --git a/builtin-fast-export.c b/builtin-fast-export.c
index b60a97e..917ac05 100644
--- a/builtin-fast-export.c
+++ b/builtin-fast-export.c
@@ -23,7 +23,8 @@ static const char *fast_export_usage[] = {
 };
 
 static int progress;
-static enum { VERBATIM, WARN, STRIP, ABORT } signed_tag_mode = ABORT;
+static enum { ABORT, VERBATIM, WARN, STRIP } signed_tag_mode = ABORT;
+static enum { ERROR, DROP, REWRITE } tag_of_filtered_mode = ABORT;
 static int fake_missing_tagger;
 
 static int parse_opt_signed_tag_mode(const struct option *opt,
@@ -42,6 +43,20 @@ static int parse_opt_signed_tag_mode(const struct option *opt,
 	return 0;
 }
 
+static int parse_opt_tag_of_filtered_mode(const struct option *opt,
+					  const char *arg, int unset)
+{
+	if (unset || !strcmp(arg, "abort"))
+		tag_of_filtered_mode = ABORT;
+	else if (!strcmp(arg, "drop"))
+		tag_of_filtered_mode = DROP;
+	else if (!strcmp(arg, "rewrite"))
+		tag_of_filtered_mode = REWRITE;
+	else
+		return error("Unknown tag-of-filtered mode: %s", arg);
+	return 0;
+}
+
 static struct decoration idnums;
 static uint32_t last_idnum;
 
@@ -282,13 +297,14 @@ static void handle_tail(struct object_array *commits, struct rev_info *revs)
 	}
 }
 
-static void handle_tag(const char *name, struct tag *tag)
+static void handle_tag(const char *name, struct tag *tag, struct rev_info *revs)
 {
 	unsigned long size;
 	enum object_type type;
 	char *buf;
 	const char *tagger, *tagger_end, *message;
 	size_t message_size = 0;
+	struct commit *commit;
 
 	buf = read_sha1_file(tag->object.sha1, &type, &size);
 	if (!buf)
@@ -333,10 +349,42 @@ static void handle_tag(const char *name, struct tag *tag)
 			}
 	}
 
+	/* handle tag->tagged having been filtered out due to paths specified */
+	struct object * tagged = tag->tagged;
+	int tagged_mark = get_object_mark(tagged);
+	if (!tagged_mark) {
+		switch(tag_of_filtered_mode) {
+		case ABORT:
+			die ("Tag %s tags unexported commit; use "
+			     "--tag-of-filtered-object=<mode> to handle it.",
+			     sha1_to_hex(tag->object.sha1));
+		case DROP:
+			/* Ignore this tag altogether */
+			return;
+				/* fallthru */
+		case REWRITE:
+			if (tagged->type != OBJ_COMMIT) {
+				die ("Tag %s tags unexported commit; use "
+				     "--tag-of-filtered-object=<mode> to handle it.",
+				     sha1_to_hex(tag->object.sha1));
+			}
+			commit = (struct commit *)tagged;
+			switch (rewrite_one_commit(revs, &commit)) {
+			case rewrite_one_ok:
+				tagged_mark = get_object_mark(&commit->object);
+				break;
+			case rewrite_one_noparents:
+			case rewrite_one_error:
+				die ("Can't find replacement commit for tag %s\n",
+				     sha1_to_hex(tag->object.sha1));
+			}
+		}
+	}
+
 	if (!prefixcmp(name, "refs/tags/"))
 		name += 10;
 	printf("tag %s\nfrom :%d\n%.*s%sdata %d\n%.*s\n",
-	       name, get_object_mark(tag->tagged),
+	       name, tagged_mark,
 	       (int)(tagger_end - tagger), tagger,
 	       tagger == tagger_end ? "" : "\n",
 	       (int)message_size, (int)message_size, message ? message : "");
@@ -399,7 +447,8 @@ static void get_tags_and_duplicates(struct object_array *pending,
 	}
 }
 
-static void handle_tags_and_duplicates(struct string_list *extra_refs)
+static void handle_tags_and_duplicates(struct string_list *extra_refs,
+				       struct rev_info *revs)
 {
 	struct commit *commit;
 	int i;
@@ -415,7 +464,7 @@ static void handle_tags_and_duplicates(struct string_list *extra_refs)
 				/* Ignore this tag altogether */
 				return;
 			}
-			handle_tag(name, tag);
+			handle_tag(name, tag, revs);
 			break;
 		case OBJ_COMMIT:
 			/* create refs pointing to already seen commits */
@@ -504,6 +553,9 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK(0, "signed-tags", &signed_tag_mode, "mode",
 			     "select handling of signed tags",
 			     parse_opt_signed_tag_mode),
+		OPT_CALLBACK(0, "tag-of-filtered-object", &tag_of_filtered_mode, "mode",
+			     "select handling of tags that tag filtered objects",
+			     parse_opt_tag_of_filtered_mode),
 		OPT_STRING(0, "export-marks", &export_filename, "FILE",
 			     "Dump marks to this file"),
 		OPT_STRING(0, "import-marks", &import_filename, "FILE",
@@ -551,7 +603,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	handle_tags_and_duplicates(&extra_refs);
+	handle_tags_and_duplicates(&extra_refs, &revs);
 
 	if (export_filename)
 		export_marks(export_filename);
diff --git a/revision.c b/revision.c
index bf58448..5ed2841 100644
--- a/revision.c
+++ b/revision.c
@@ -1601,13 +1601,7 @@ int prepare_revision_walk(struct rev_info *revs)
 	return 0;
 }
 
-enum rewrite_result {
-	rewrite_one_ok,
-	rewrite_one_noparents,
-	rewrite_one_error,
-};
-
-static enum rewrite_result rewrite_one(struct rev_info *revs, struct commit **pp)
+enum rewrite_result rewrite_one_commit(struct rev_info *revs, struct commit **pp)
 {
 	struct commit_list *cache = NULL;
 
@@ -1633,7 +1627,7 @@ static int rewrite_parents(struct rev_info *revs, struct commit *commit)
 	struct commit_list **pp = &commit->parents;
 	while (*pp) {
 		struct commit_list *parent = *pp;
-		switch (rewrite_one(revs, &parent->item)) {
+		switch (rewrite_one_commit(revs, &parent->item)) {
 		case rewrite_one_ok:
 			break;
 		case rewrite_one_noparents:
diff --git a/revision.h b/revision.h
index 227164c..6bf5b9e 100644
--- a/revision.h
+++ b/revision.h
@@ -158,6 +158,14 @@ extern void add_pending_object(struct rev_info *revs, struct object *obj, const
 
 extern void add_head_to_pending(struct rev_info *);
 
+enum rewrite_result {
+	rewrite_one_ok,
+	rewrite_one_noparents,
+	rewrite_one_error,
+};
+
+extern enum rewrite_result rewrite_one_commit(struct rev_info *revs, struct commit **pp);
+
 enum commit_action {
 	commit_ignore,
 	commit_show,
-- 
1.6.0.6

[PATCH v2] Add new fast-export testcases

[newren]

From: Elijah Newren <newren@gmail.com>

The testcases test the new --tag-of-filtered-object option, and test the
output when limiting what to export by path.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
(No change to this patch since the first series.)

 t/t9301-fast-export.sh |   55 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 55 insertions(+), 0 deletions(-)

diff --git a/t/t9301-fast-export.sh b/t/t9301-fast-export.sh
index 3f13e6b..d03d599 100755
--- a/t/t9301-fast-export.sh
+++ b/t/t9301-fast-export.sh
@@ -262,6 +262,61 @@ test_expect_success 'cope with tagger-less tags' '
 
 '
 
+test_expect_success 'setup for limiting exports by PATH' '
+	mkdir limit-by-paths &&
+	cd limit-by-paths &&
+	git init &&
+	echo hi > there &&
+	git add there &&
+	git commit -m "First file" &&
+	echo foo > bar &&
+	git add bar &&
+	git commit -m "Second file" &&
+	git tag -a -m msg mytag &&
+	cd ..
+'
+
+cat > limit-by-paths/expected << EOF
+blob
+mark :1
+data 3
+hi
+
+reset refs/tags/mytag
+commit refs/tags/mytag
+mark :2
+author A U Thor <author@example.com> 1112912713 -0700
+committer C O Mitter <committer@example.com> 1112912713 -0700
+data 11
+First file
+M 100644 :1 there
+
+EOF
+
+test_expect_success 'dropping tag of filtered out object' '
+	cd limit-by-paths &&
+	git fast-export --tag-of-filtered-object=drop mytag -- there > output &&
+	test_cmp output expected &&
+	cd ..
+'
+
+cat >> limit-by-paths/expected << EOF
+tag mytag
+from :2
+tagger C O Mitter <committer@example.com> 1112912713 -0700
+data 4
+msg
+
+EOF
+
+test_expect_success 'rewriting tag of filtered out object' '
+	cd limit-by-paths &&
+	git fast-export --tag-of-filtered-object=rewrite mytag -- there > output &&
+	test_cmp output expected &&
+	cd ..
+'
+
+
 test_expect_success 'set-up a few more tags for tag export tests' '
 	git checkout -f master &&
 	HEAD_TREE=`git show -s --pretty=raw HEAD | grep tree | sed "s/tree //"` &&
-- 
1.6.0.6

[PATCH v2] fast-export: Document the fact that git-rev-list arguments are accepted

[newren]

From: Elijah Newren <newren@gmail.com>


Signed-off-by: Elijah Newren <newren@gmail.com>
---
(No change to this patch since the first series.)

 Documentation/git-fast-export.txt |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
index 194abde..af2328d 100644
--- a/Documentation/git-fast-export.txt
+++ b/Documentation/git-fast-export.txt
@@ -82,6 +82,12 @@ marks the same across runs.
 	allow that.  So fake a tagger to be able to fast-import the
 	output.
 
+[git-rev-list-args...]::
+       A list of arguments, acceptable to 'git-rev-parse' and
+       'git-rev-list', that specifies the specific objects and references
+       to export.  For example, `master\~10..master` causes the
+       current master reference to be exported along with all objects
+       added since its 10th ancestor commit.
 
 EXAMPLES
 --------
-- 
1.6.0.6

Re: [PATCH v2] fast-export: Omit tags that tag trees

[Junio C Hamano]

newren@gmail.com writes:

> From: Elijah Newren <newren@gmail.com>
>
> Commit c0582c53bcf4e83bba70e1ad23abbad31f96ebc8 introduced logic to just
> omit tags that point to tree objects.  However, a case was missed
> resulting in a tag being output which pointed at "mark :0", which would
> cause fast-import to crash.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> Includes changes suggested by Stephen Boyd (squashing commits together
> and moving the NEEDSWORK comment to where it is still relevant) and
> Johannes Sixt (fixing the testcase to remain in the right directory
> even if git init fails and to catch failures in git fast-export).
>
>  builtin-fast-export.c  |    8 +++++++-
>  t/t9301-fast-export.sh |    8 +++++++-
>  2 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/builtin-fast-export.c b/builtin-fast-export.c
> index 6cef810..891e2d4 100644
> --- a/builtin-fast-export.c
> +++ b/builtin-fast-export.c
> @@ -407,9 +407,15 @@ static void handle_tags_and_duplicates(struct string_list *extra_refs)
>  	for (i = extra_refs->nr - 1; i >= 0; i--) {
>  		const char *name = extra_refs->items[i].string;
>  		struct object *object = extra_refs->items[i].util;
> +		struct tag *tag;
>  		switch (object->type) {
>  		case OBJ_TAG:
> -			handle_tag(name, (struct tag *)object);
> +			tag = (struct tag *)object;
> +			if (tag->tagged->type == OBJ_TREE) {
> +				/* Ignore this tag altogether */
> +				return;
> +			}
> +			handle_tag(name, tag);
>  			break;

This seems to be the only callsite of handle_tag(), and I wonder why the
above special case is not in the called function instead.

If a newer version of fast-import ever supports a tag that points at
non-commits, a patch to add the corresponding support on the export side
logically should go to handle_tag(), no?

Re: [PATCH v2] fast-export: Make sure we show actual ref names instead of "(null)"

[Junio C Hamano]

newren@gmail.com writes:

> From: Elijah Newren <newren@gmail.com>
>
> Under some cases (e.g. 'git fast-export --parents master -- COPYING' run
> in git.git), the output included the lines
>   reset (null)
>   commit (null)
> instead of
>   reset refs/heads/master
>   commit refs/heads/master
> This simple change fixes that.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> (No change to this patch since the first series.)
> As noted above, run 'git fast-export --parents master -- COPYING' in
> git.git to see this bug triggered.

Isn't this easier to read?

> Under some cases (e.g. 'git fast-export --parents master -- COPYING' run
> in git.git), the output included the lines
>
>   reset (null)
>   commit (null)
>
> instead of
>
>   reset refs/heads/master
>   commit refs/heads/master
>
> This simple change fixes that.

The last line does not add much value.  I would have preferred to see
something like this while reviewing, so that I did not have to dig that
information out from the source myself:

	This is because the code expects commit->util to point at the ref
	but that is only set by the revision traversal machinery when
	show_source is asked, in a way similar to "--source" option of
	"git log" family of commands.

But otherwise, good catch.  Thanks.

>  builtin-fast-export.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/builtin-fast-export.c b/builtin-fast-export.c
> index 891e2d4..9aa409b 100644
> --- a/builtin-fast-export.c
> +++ b/builtin-fast-export.c
> @@ -531,6 +531,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
>  	get_tags_and_duplicates(&revs.pending, &extra_refs);
>  
>  	revs.topo_order = 1;
> +	revs.show_source = 1;
>  	if (prepare_revision_walk(&revs))
>  		die("revision walk setup failed");
>  	revs.diffopt.format_callback = show_filemodify;
> -- 
> 1.6.0.6

Re: [PATCH v2] fast-export: Add a --tag-of-filtered-object option for newly dangling tags

[Junio C Hamano]

newren@gmail.com writes:

> From: Elijah Newren <newren@gmail.com>
>
> When providing a list of paths to limit what is exported, the object that
> a tag points to can be filtered out entirely.  This new switch allows
> the user to specify what should happen to the tag in such a case.  The
> default action, 'abort' will exit with an error message.  With 'drop', the
> tag will simply be omitted from the output.  With 'rewrite', if the object
> tagged was a commit, the tag will be modified to tag an ancestor of the
> removed commit.

I had to wonder what "an ancestor" means in the above sentence.

 - It cannot be the "direct ancestor", as such a commit could also have
   been filtered out.  it will probably find an ancestor of the tagged
   commit that is not TREESAME, i.e. changes one of the specified paths.
   is that what the code tries to do?

 - Assuming that the answer is true, finding an ancestor means go back in
   the ancestry graph.  How should a merged history be handled?

   - If two branches merged, only one among which touched the paths, then
     the simplication would have linearized the history already, so it is
     a non issue;

   - If a merge really had changes from both branches, that merge would
     remain in the output, and the tag will be moved there.

   - Did I miss any other case?

If I have to wonder, many other people who read the "git log" output later
will get puzzled, too.  Please clarify.

> @@ -333,10 +349,42 @@ static void handle_tag(const char *name, struct tag *tag)
>  			}
>  	}
>  
> +	/* handle tag->tagged having been filtered out due to paths specified */
> +	struct object * tagged = tag->tagged;
> +	int tagged_mark = get_object_mark(tagged);
> +	if (!tagged_mark) {
> +		switch(tag_of_filtered_mode) {
> +		case ABORT:
> +			die ("Tag %s tags unexported commit; use "
> +			     "--tag-of-filtered-object=<mode> to handle it.",
> +			     sha1_to_hex(tag->object.sha1));
> +		case DROP:
> +			/* Ignore this tag altogether */
> +			return;
> +				/* fallthru */

Doesn't fall through...

> +		case REWRITE:
> +			if (tagged->type != OBJ_COMMIT) {
> +				die ("Tag %s tags unexported commit; use "
> +				     "--tag-of-filtered-object=<mode> to handle it.",
> +				     sha1_to_hex(tag->object.sha1));
> +			}
> +			commit = (struct commit *)tagged;
> +			switch (rewrite_one_commit(revs, &commit)) {
> +			case rewrite_one_ok:
> +				tagged_mark = get_object_mark(&commit->object);
> +				break;
> +			case rewrite_one_noparents:
> +			case rewrite_one_error:
> +				die ("Can't find replacement commit for tag %s\n",
> +				     sha1_to_hex(tag->object.sha1));
> +			}
> +		}
> +	}
> +

This makes me a bit worried.  The rewrite_one() function is never designed
to be called from outside while the main traversal has still work to do,
nor called more than once on the same commit object.  I do not know what
would happen if somebody did so.

Granted, all of this processing happens after the revision traversing
machinery is done with all the commits, and rewriting commits further here
will not have a chance of breaking the subtleties in get_revision() and
everything called from it that already has happened in the main traversal,
but still I would prefer not exposing this function out of revision.c to
avoid mistakes if possible.

Also, are you absolutely sure that your revs is always limited at this
point?  Otherwise, the parents of this commit are queued in rev->list,
expecting somebody else to later pick them up and further process, but
there is nobody who does that in your codepath as far as I can see.  What
will happen to these parent commits?

Re: [PATCH v2] fast-export: Add a --tag-of-filtered-object option for newly dangling tags

[Elijah Newren]

Hi,

Thanks for the detailed feedback and suggestions.  I'll send a new
patch series shortly, which I believe addresses all your suggestions
and requests (and opens up a new issue), but there is one item in
particular I wanted to respond to...

On Mon, Jun 22, 2009 at 12:34 PM, Junio C Hamano<gitster@pobox.com> wrote:
> Also, are you absolutely sure that your revs is always limited at this
> point?  Otherwise, the parents of this commit are queued in rev->list,
> expecting somebody else to later pick them up and further process, but
> there is nobody who does that in your codepath as far as I can see.  What
> will happen to these parent commits?

revs was not limited, but should have been.  Inside setup_revisions,
revs->limited is set if revs->topo_order is, but unfortunately we
weren't setting revs->topo_order until after the call to
setup_revisions.  So, as you suggest, my code caused the parents of
the rewritten commit to needlessly get queued up and left around, with
no one to process them.  Oops.


Elijah