fast-import: checkpoint: dump branches/tags/marks even if object_count==0

The checkpoint command cycles packfiles if object_count != 0, a sensible
test or there would be no pack files to write. Since 820b931012, the
command also dumps branches, tags and marks, but still conditionally.
However, it is possible for a command stream to modify refs or create
marks without creating any new objects.

For example, reset a branch (and keep fast-import running):

$ git fast-import
reset refs/heads/master
from refs/heads/master^

checkpoint

but refs/heads/master remains unchanged.

Other example: a commit command that re-creates an object that already
exists in the object database.

The man page also states that checkpoint "updates the refs" and that
"placing a progress command immediately after a checkpoint will inform
the reader when the checkpoint has been completed and it can safely
access the refs that fast-import updated". This wasn't always true
without this patch.

This fix unconditionally calls dump_{branches,tags,marks}() for all
checkpoint commands. dump_branches() and dump_tags() are cheap to call
in the case of a no-op.

Add tests to t9300 that observe the (non-packfiles) effects of
checkpoint.

Signed-off-by: Eric Rannaud <e@nanocritical.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/fast-import.c b/fast-import.c
index a959161b4631e134602aa9b5502105ee5c5bcfb8..365d3191aa1abfb42b4d0da6973612126cc74f3b 100644 (file)
--- a/fast-import.c
+++ b/fast-import.c
@@ -3188,10 +3188,10 @@ static void checkpoint(void)
        checkpoint_requested = 0;
        if (object_count) {
                cycle_packfile();
-               dump_branches();
-               dump_tags();
-               dump_marks();
        }
+       dump_branches();
+       dump_tags();
+       dump_marks();
 }
 
 static void parse_checkpoint(void)

diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 67b8c50a5ab431345de2909209811fc8e4f12548..d47560b6343db7006461259e4808837f3d35eecb 100755 (executable)
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -3120,4 +3120,146 @@ test_expect_success 'U: validate root delete result' '
        compare_diff_raw expect actual
 '
 
+###
+### series V (checkpoint)
+###
+
+# The commands in input_file should not produce any output on the file
+# descriptor set with --cat-blob-fd (or stdout if unspecified).
+#
+# To make sure you're observing the side effects of checkpoint *before*
+# fast-import terminates (and thus writes out its state), check that the
+# fast-import process is still running using background_import_still_running
+# *after* evaluating the test conditions.
+background_import_then_checkpoint () {
+       options=$1
+       input_file=$2
+
+       mkfifo V.input
+       exec 8<>V.input
+       rm V.input
+
+       mkfifo V.output
+       exec 9<>V.output
+       rm V.output
+
+       git fast-import $options <&8 >&9 &
+       echo $! >V.pid
+       # We don't mind if fast-import has already died by the time the test
+       # ends.
+       test_when_finished "exec 8>&-; exec 9>&-; kill $(cat V.pid) || true"
+
+       # Start in the background to ensure we adhere strictly to (blocking)
+       # pipes writing sequence. We want to assume that the write below could
+       # block, e.g. if fast-import blocks writing its own output to &9
+       # because there is no reader on &9 yet.
+       (
+               cat "$input_file"
+               echo "checkpoint"
+               echo "progress checkpoint"
+       ) >&8 &
+
+       error=1 ;# assume the worst
+       while read output <&9
+       do
+               if test "$output" = "progress checkpoint"
+               then
+                       error=0
+                       break
+               fi
+               # otherwise ignore cruft
+               echo >&2 "cruft: $output"
+       done
+
+       if test $error -eq 1
+       then
+               false
+       fi
+}
+
+background_import_still_running () {
+       if ! kill -0 "$(cat V.pid)"
+       then
+               echo >&2 "background fast-import terminated too early"
+               false
+       fi
+}
+
+test_expect_success PIPE 'V: checkpoint helper does not get stuck with extra output' '
+       cat >input <<-INPUT_END &&
+       progress foo
+       progress bar
+
+       INPUT_END
+
+       background_import_then_checkpoint "" input &&
+       background_import_still_running
+'
+
+test_expect_success PIPE 'V: checkpoint updates refs after reset' '
+       cat >input <<-\INPUT_END &&
+       reset refs/heads/V
+       from refs/heads/U
+
+       INPUT_END
+
+       background_import_then_checkpoint "" input &&
+       test "$(git rev-parse --verify V)" = "$(git rev-parse --verify U)" &&
+       background_import_still_running
+'
+
+test_expect_success PIPE 'V: checkpoint updates refs and marks after commit' '
+       cat >input <<-INPUT_END &&
+       commit refs/heads/V
+       mark :1
+       committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+       data 0
+       from refs/heads/U
+
+       INPUT_END
+
+       background_import_then_checkpoint "--export-marks=marks.actual" input &&
+
+       echo ":1 $(git rev-parse --verify V)" >marks.expected &&
+
+       test "$(git rev-parse --verify V^)" = "$(git rev-parse --verify U)" &&
+       test_cmp marks.expected marks.actual &&
+       background_import_still_running
+'
+
+# Re-create the exact same commit, but on a different branch: no new object is
+# created in the database, but the refs and marks still need to be updated.
+test_expect_success PIPE 'V: checkpoint updates refs and marks after commit (no new objects)' '
+       cat >input <<-INPUT_END &&
+       commit refs/heads/V2
+       mark :2
+       committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+       data 0
+       from refs/heads/U
+
+       INPUT_END
+
+       background_import_then_checkpoint "--export-marks=marks.actual" input &&
+
+       echo ":2 $(git rev-parse --verify V2)" >marks.expected &&
+
+       test "$(git rev-parse --verify V2)" = "$(git rev-parse --verify V)" &&
+       test_cmp marks.expected marks.actual &&
+       background_import_still_running
+'
+
+test_expect_success PIPE 'V: checkpoint updates tags after tag' '
+       cat >input <<-INPUT_END &&
+       tag Vtag
+       from refs/heads/V
+       tagger $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+       data 0
+
+       INPUT_END
+
+       background_import_then_checkpoint "" input &&
+       git show-ref -d Vtag &&
+       background_import_still_running
+'
+
 test_done