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

> On Fri, May 24, 2019 at 4:10 PM Eric Rannaud <e@nanocritical.com> wrote:
> >
> > 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?

> -- 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
>