Re: [PATCH] fast-import: checkpoint: dump branches/tags/marks even if object_count==0

>> +     cat $input_file >&8
>
> It 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.

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

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