Re: [PATCH] fast-export: Allow pruned-references in mark file

Antoine Pelisse [off-list ref] writes:

On Mon, Nov 26, 2012 at 12:37 PM, Felipe Contreras
[off-list ref] wrote:

quoted

On Mon, Nov 26, 2012 at 5:03 AM, Junio C Hamano [off-list ref] wrote:

quoted

Is this a safe and sane thing to do, and if so why?  Could you
describe that in the log message here?

Why would fast-export try to export something that was pruned? Doesn't
that mean it wasn't reachable?

Hello Junio,
Hello Felipe,

Actually the issue happened while using Felipe's branch with his
git-remote-hg.  Everything was going fine until I (or did it run
automatically, I dont remember) ran git gc that pruned unreachable
objects. Of course some of the branch I had pushed to the hg remote
had been changed (most likely rebased).  References no longer exists
in the repository (cleaned by gc), but the reference still exists in
mark file, as it was exported earlier.  Thus the failure when git
fast-export reads the mark file.

You described that part very well in your proposed log message and I
got it just fine.

Then, is it safe ?
Updating the last_idnum as I do in the patch doesn't work because
if the reference is the last, the number is going to be overwriten
in the next run.
From git point of view, I guess it is fine. The file is fully read at
the beginning of fast-export and fully written at the end.

I am not sure I follow the above, but anyway, I think the patch does
is safe because (1) future "fast-export" will not refer to these
pruned objects in its output (we have decided that these pruned
objects are not used anywhere in the history so nobody will refer to
them) and (2) we still need to increment the id number so that later
objects in the marks file get assigned the same id number as they
were assigned originally (otherwise we will not name these objects
consistently when we later talk about them).

And I wanted to see that kind of reasoning behind the patch in the
proposed log message, because other people will need to refer to it
when they read "git log" output to understand the change.

Thanks.

I am not sure I follow the above, but anyway, I think the patch does
is safe because (1) future "fast-export" will not refer to these
pruned objects in its output (we have decided that these pruned
objects are not used anywhere in the history so nobody will refer to
them) and (2) we still need to increment the id number so that later
objects in the marks file get assigned the same id number as they
were assigned originally (otherwise we will not name these objects
consistently when we later talk about them).

I fully agree on (1), not so much on (2) though.

I have the following behavior using my patch and running that script
that doesn't look correct.

echo "Working scenario"
git init test &&
(cd test &&
git commit --allow-empty -m "Commit mark :1" &&
git commit --allow-empty -m "Commit mark :2" &&
git fast-export --export-marks=MARKS master > /dev/null &&
cat MARKS &&
git reset HEAD~1 &&
sleep 1 &&
git reflog expire --all --expire=now &&
git prune --expire=now &&
git commit --allow-empty -m "Commit mark :3" &&
git fast-export --import-marks=MARKS \
  --export-marks=MARKS master > /dev/null &&
cat MARKS) &&
rm -rf test

echo "Non-working scenario"
git init test &&
(cd test &&
git commit --allow-empty -m "Commit mark :1" &&
git commit --allow-empty -m "Commit mark :2" &&
git fast-export --export-marks=MARKS master > /dev/null &&
cat MARKS &&
git reset HEAD~1 &&
sleep 1 &&
git reflog expire --all --expire=now &&
git prune --expire=now &&
git fast-export --import-marks=MARKS \
  --export-marks=MARKS master > /dev/null &&
git commit --allow-empty -m "Commit mark :3" &&
git fast-export --import-marks=MARKS \
  --export-marks=MARKS master > /dev/null &&
cat MARKS) &&
rm -rf test

outputs something like this:
Working scenario
Initialized empty Git repository in /home/antoine/test/.git/
[master (root-commit) 6cf350d] Commit mark :1
[master 8f97f85] Commit mark :2
:1 6cf350d7ecb3dc6573b00f839a6a51625ed28966
:2 8f97f85e1e7badf6a3daf411cf8d1133b00d522e
[master 21cadfd] Commit mark :3
warning: Could not read blob 8f97f85e1e7badf6a3daf411cf8d1133b00d522e
:1 6cf350d7ecb3dc6573b00f839a6a51625ed28966
:3 21cadfd87d90c05ce8770c968e5ed3d072ead4ae
Non-working scenario
Initialized empty Git repository in /home/antoine/test/.git/
[master (root-commit) 5b5f7ec] Commit mark :1
[master b224390] Commit mark :2
:2 b224390daee199644495c15503882eb84df07df5
:1 5b5f7ec77768393aab2a0c2c11b4b8f7773f8678
warning: Could not read blob b224390daee199644495c15503882eb84df07df5
[master 181a774] Commit mark :3
:1 5b5f7ec77768393aab2a0c2c11b4b8f7773f8678
:2 181a7744c6d3428edb01a1adc9df247e9620be5f

Both "commit mark :2" and "commit mark :3" end up being marked :2.
Any tool like git-remote-hg that is using a mapping from mark <-> hg changeset
could then fail.

On Mon, Nov 26, 2012 at 9:04 PM, Antoine Pelisse [off-list ref] wrote:

quoted

I am not sure I follow the above, but anyway, I think the patch does
is safe because (1) future "fast-export" will not refer to these
pruned objects in its output (we have decided that these pruned
objects are not used anywhere in the history so nobody will refer to
them) and (2) we still need to increment the id number so that later
objects in the marks file get assigned the same id number as they
were assigned originally (otherwise we will not name these objects
consistently when we later talk about them).

I fully agree on (1), not so much on (2) though.

I have the following behavior using my patch and running that script
that doesn't look correct.

echo "Working scenario"
git init test &&
(cd test &&
git commit --allow-empty -m "Commit mark :1" &&
git commit --allow-empty -m "Commit mark :2" &&
git fast-export --export-marks=MARKS master > /dev/null &&
cat MARKS &&
git reset HEAD~1 &&
sleep 1 &&
git reflog expire --all --expire=now &&
git prune --expire=now &&
git commit --allow-empty -m "Commit mark :3" &&
git fast-export --import-marks=MARKS \
  --export-marks=MARKS master > /dev/null &&
cat MARKS) &&
rm -rf test

echo "Non-working scenario"
git init test &&
(cd test &&
git commit --allow-empty -m "Commit mark :1" &&
git commit --allow-empty -m "Commit mark :2" &&
git fast-export --export-marks=MARKS master > /dev/null &&
cat MARKS &&
git reset HEAD~1 &&
sleep 1 &&
git reflog expire --all --expire=now &&
git prune --expire=now &&
git fast-export --import-marks=MARKS \
  --export-marks=MARKS master > /dev/null &&
git commit --allow-empty -m "Commit mark :3" &&
git fast-export --import-marks=MARKS \
  --export-marks=MARKS master > /dev/null &&
cat MARKS) &&
rm -rf test

outputs something like this:
Working scenario
Initialized empty Git repository in /home/antoine/test/.git/
[master (root-commit) 6cf350d] Commit mark :1
[master 8f97f85] Commit mark :2
:1 6cf350d7ecb3dc6573b00f839a6a51625ed28966
:2 8f97f85e1e7badf6a3daf411cf8d1133b00d522e
[master 21cadfd] Commit mark :3
warning: Could not read blob 8f97f85e1e7badf6a3daf411cf8d1133b00d522e
:1 6cf350d7ecb3dc6573b00f839a6a51625ed28966
:3 21cadfd87d90c05ce8770c968e5ed3d072ead4ae
Non-working scenario
Initialized empty Git repository in /home/antoine/test/.git/
[master (root-commit) 5b5f7ec] Commit mark :1
[master b224390] Commit mark :2
:2 b224390daee199644495c15503882eb84df07df5
:1 5b5f7ec77768393aab2a0c2c11b4b8f7773f8678
warning: Could not read blob b224390daee199644495c15503882eb84df07df5
[master 181a774] Commit mark :3
:1 5b5f7ec77768393aab2a0c2c11b4b8f7773f8678
:2 181a7744c6d3428edb01a1adc9df247e9620be5f

Both "commit mark :2" and "commit mark :3" end up being marked :2.
Any tool like git-remote-hg that is using a mapping from mark <-> hg changeset
could then fail.

I don't understand. "commit mark :2" 'git fast-export' would never
point to that object again, the new commit would override that mark:

commit refs/heads/master
mark :2
...
commit mark :3

Then 'git remote-hg' should override that mark as well.

But it doesn't matter, because that would be the case only for the
last object, as soon as you find another valid object, that object's
mark will be considered the last one.

And what Junio said is consistent with what you want: last_idnum
should be updated even if the object is not valid.

Cheers.

-- 
Felipe Contreras

fast-export can fail because of some pruned-reference when importing a
mark file.

The problem happens in the following scenario:

    $ git fast-export --export-marks=MARKS master
    (rewrite master)
    $ git prune
    $ git fast-export --import-marks=MARKS master

This might fail if some references have been removed by prune
because some marks will refer to no longer existing commits.
git-fast-export will not need these objects anyway as they were no
longer reachable.

We still need to update last_numid so we don't change the mapping
between marks and objects for remote-helpers.
Unfortunately, the mark file should not be rewritten without lost marks
if no new objects has been exported, as we could lose track of the last
last_numid.

Signed-off-by: Antoine Pelisse <redacted>
---
 Documentation/git-fast-export.txt |    2 ++
 builtin/fast-export.c             |   11 +++++++----
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
index d6487e1..feab7a3 100644
--- a/Documentation/git-fast-export.txt
+++ b/Documentation/git-fast-export.txt

@@ -66,6 +66,8 @@ produced incorrect results if you gave these options.
 	incremental runs.  As <file> is only opened and truncated
 	at completion, the same path can also be safely given to
 	\--import-marks.
+	The file will not be written if no new object has been
+	marked/exported.
 
 --import-marks=<file>::
 	Before processing any input, load the marks specified in

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index d380155..f44b76c 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c

@@ -618,9 +618,12 @@ static void import_marks(char *input_file)
 			|| *mark_end != ' ' || get_sha1(mark_end + 1, sha1))
 			die("corrupt mark line: %s", line);
 
+		if (last_idnum < mark)
+			last_idnum = mark;
+
 		object = parse_object(sha1);
 		if (!object)
-			die ("Could not read blob %s", sha1_to_hex(sha1));
+			continue;
 
 		if (object->flags & SHOWN)
 			error("Object %s already has a mark", sha1_to_hex(sha1));

@@ -630,8 +633,6 @@ static void import_marks(char *input_file)
 			continue;
 
 		mark_object(object, mark);
-		if (last_idnum < mark)
-			last_idnum = mark;
 
 		object->flags |= SHOWN;
 	}

@@ -645,6 +646,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 	struct string_list extra_refs = STRING_LIST_INIT_NODUP;
 	struct commit *commit;
 	char *export_filename = NULL, *import_filename = NULL;
+	uint32_t lastimportid;
 	struct option options[] = {
 		OPT_INTEGER(0, "progress", &progress,
 			    N_("show progress after <n> objects")),

@@ -688,6 +690,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 
 	if (import_filename)
 		import_marks(import_filename);
+	lastimportid = last_idnum;
 
 	if (import_filename && revs.prune_data.nr)
 		full_tree = 1;

@@ -710,7 +713,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 
 	handle_tags_and_duplicates(&extra_refs);
 
-	if (export_filename)
+	if (export_filename && lastimportid != last_idnum)
 		export_marks(export_filename);
 
 	if (use_done_feature)

-- 
1.7.9.5

On Sat, Apr 6, 2013 at 11:04 AM, Antoine Pelisse [off-list ref] wrote:

fast-export can fail because of some pruned-reference when importing a
mark file.

The problem happens in the following scenario:

    $ git fast-export --export-marks=MARKS master
    (rewrite master)
    $ git prune
    $ git fast-export --import-marks=MARKS master

This might fail if some references have been removed by prune
because some marks will refer to no longer existing commits.
git-fast-export will not need these objects anyway as they were no
longer reachable.

We still need to update last_numid so we don't change the mapping
between marks and objects for remote-helpers.
Unfortunately, the mark file should not be rewritten without lost marks
if no new objects has been exported, as we could lose track of the last
last_numid.

Makes sense to me.

Reviewed-by: Felipe Contreras <redacted>

-- 
Felipe Contreras