Re: [PATCH v4] fast-import: checkpoint: only write out refs and tags if we changed them

From: Elijah Newren <newren@gmail.com>
Date: May 27, 2019, 21:12 UTC
Message-ID: <20190527211245.12292-1-newren@gmail.com>
In-Reply-To: <CAH_=xoYQEvXKnhyZsUeZ=VTEMOpOdgbd-57f7-6M+2KQLSAAUQ@mail.gmail.com>

On Fri, May 24, 2019 at 4:10 PM Eric Rannaud <e@nanocritical.com> wrote:

Show 8 quoted lines

>
> With this patch, we keep track of whether a particular branch or tag has
> been changed by this fast-import process since our last checkpoint (or
> startup). At the next checkpoint, only refs and tags that new commands
> have changed are written to disk. To be clear, fast-import still does
> not update its internal view of branches in response to external
> changes, but now it avoids interfering with external changes unless
> there was an explicit command to do so since the last checkpoint.

This patch looks reasonable to me; you can view it one of two basic ways:
  1) an optimization (only update what needs to be updated)
  2) a way to have early access to a completed branch

In particular, if the fast-import stream is dealing with unrelated (or very divergent) branches and it completes one branch before others, then your change allows people to have early access to both read & update the completed branches while the import continues working on other branches.

If you changed your description to sell it this way, it'd be fine as-is. But you use --force and make multiple mentions of concurrent updates to branches in external processes during the fast-import process. That kind of description makes it really clear we need to tighten up what happens with the checkpoint command when it hits a failure (as mentioned in the commentary on V3). Below is a patch that does this.

I think we should either:

  1) update your commit message to sell it without mentioning the
     concurrent rewrites, and then I'll update my patch to not
     conflict (we both add new tests at the same location to the same
     file causing a simple conflict) by building on yours, OR
     
  2) update your patch to come after mine and add a comment to your
     commit message about how checkpoint will abort if it hits an
     error, suggesting that people should only update branches
     fast-import will not be updating further or they should use
     --force and deal with their changes being overwritten by
     fast-import.  Then you can submit our two patches as a series.

Thoughts?

Elijah

-- 8< --
Subject: [PATCH] fast-import: remember to check failure flag when
 checkpointing

fast-import, when finished, will flush out the current packfile and update branches, tags, and marks, returning whether it was successful. The point of the 'checkpoint' command was to do all the same things, but continue processing the stream of input commands if it was successful. Unfortunately, the checkpoint code forgot to check the 'failure' flag to see if there was an error in e.g. updating the branches, which meant it would also continue if there was a failure. Fix this oversight.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 fast-import.c          |  2 ++
 t/t9300-fast-import.sh | 42 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/fast-import.c b/fast-import.c
index f38d04fa58..d0e12b03a0 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -3033,6 +3033,8 @@ static void checkpoint(void)
 	dump_branches();
 	dump_tags();
 	dump_marks();
+	if (failure)
+		exit(1);
 }
 
 static void parse_checkpoint(void)
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 3668263c40..788a543b82 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -3262,6 +3262,48 @@ test_expect_success PIPE 'V: checkpoint updates tags after tag' '
 	background_import_still_running
 '
 
+background_fast_import_race () {
+	options=$1
+	input_file=$2
+	exit_status=$3
+	extra_cmd="$4"
+
+	mkfifo V.input
+	exec 8<>V.input
+	rm V.input
+
+	git fast-import $options <&8 &
+	echo $! >V.pid &&
+	$extra_cmd &&
+
+	cat $input_file >&8
+	wait $(cat V.pid)
+	test $? -eq $exit_status
+}
+
+test_expect_success PIPE 'V: checkpoint fails if refs updated beforehand' '
+	git checkout --orphan V3 &&
+	git commit --allow-empty -m initial &&
+	INITIAL=$(git rev-parse HEAD) &&
+
+	cat >input <<-INPUT_END &&
+	feature done
+	commit refs/heads/V3
+	mark :3
+	committer Me My <self@and.eye> 1234567890 +0123
+	data 10
+	generated
+	from $INITIAL
+
+	checkpoint
+	done
+	INPUT_END
+
+	background_fast_import_race "" input 1 "git commit --allow-empty -m conflicting" &&
+	background_fast_import_race "--force" input 0 "git commit --allow-empty -m conflicting"
+'
+
+
 ###
 ### series W (get-mark and empty orphan commits)
 ###

-- 
2.22.0.rc1.dirty