Re: [PATCH] fast-import: checkpoint: dump branches/tags/marks even if object_count==0
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 28, 2017, 03:48 UTC
- Message-ID
- <xmqqh8vn32mn.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <cccb06b75df3cad9f013d5a9ab0371f0a2d9c2ce.1506541322.git.e@nanocritical.com>
"Eric Rannaud" <e@nanocritical.com> writes:
Show 29 quoted lines
> diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
> index 67b8c50a5ab4..9aa3470d895b 100755
> --- a/t/t9300-fast-import.sh
> +++ b/t/t9300-fast-import.sh
> @@ -3120,4 +3120,133 @@ test_expect_success 'U: validate root delete result' '
> compare_diff_raw expect actual
> '
>
> +###
> +### series V (checkpoint)
> +###
> +
> +# 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_until_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
> +
> + cat $input_file >&8It probably is a good idea to quote "$input_file" in case other people later use a full path to the file or something; for now this is OK.
fd#8 at this point does not have a reader; unless the contents of the $input_file is small enough, wouldn't this "cat" block until somebody else comes and reads from it to drain? Should we instead start fast-import first in the background, arrange it to be killed when we are done with it, and then start feeding the input?
> + git fast-import $options <&8 >&9 & > + echo $! >V.pid > + test_when_finished "kill $(cat V.pid) || true"
This '|| true' is here because the process might already have died on its own, which sounds like a sensible precaution.
Show 12 quoted lines
> + error=0 > + if read output <&9 > + then > + if ! test "$output" = "progress checkpoint" > + then > + echo >&2 "no progress checkpoint received: $output" > + error=1 > + fi > + else > + echo >&2 "failed to read fast-import output" > + error=1 > + fi
And we expect "progress checkpoint" would be the first and only output after fast-import consumes all the input stream up to the "progress" thing we feed, so this is not "read and discard until we see 'progress checkpoint'" but is "read one and that must be 'progress checkpoint'". Makes sense to me.
If this script is (and will be in the future) all about issuing a checkpoint command and observing its effect, we can reasonably expect that the input file _must_ end with "checkpoint" followed by "progress checkpoint", no? If that is the case, perhaps feeding these two from this helper function to >&8, instead of forcing the caller to prepare the input file to always end with these two, may be a better organization.
> + exec 8>&- > + exec 9>&-
These are to make sure that nobody (after fast-import dies) has these file descriptors hanging open for writing. Makes one wonder what happens to the reader side of the file descriptor, though ;-)
Before we return from this function, we expect (as the comment before the function says) that fast-import is still running, waiting further input. Wouldn't closing the other side of the pipe here like these make it notice that there is no more data by causing read_next_command() find EOF? IOW, is "use import_until_checkout, test the outcome and then make sure import_still_running reports that the outcome was not due to the process terminating and flushing" somewhat racy?
Or are we closing these file descriptors for different reason (i.e. not to tell fast-import we are done feeding it input) and I am reading the code incorrectly? Puzzled.
Show 13 quoted lines
> + if test $error -eq 1
> + then
> + exit 1
> + fi
> +}
> +
> +background_import_still_running () {
> + if ! kill -0 "$(cat V.pid)"
> + then
> + echo >&2 "background fast-import terminated too early"
> + exit 1
> + fi
> +}I suspect these "exit 1" above should be "false", to give the calling test_expect_success a chance to notice the failure and react to it.
Show 13 quoted lines
> +test_expect_success 'V: checkpoint updates refs after reset' ' > + cat >input <<-\INPUT_END && > + reset refs/heads/V > + from refs/heads/U > + > + checkpoint > + progress checkpoint > + INPUT_END > + > + background_import_until_checkpoint "" input && > + test "$(git rev-parse --verify V)" = "$(git rev-parse --verify U)" && > + background_import_still_running > +'