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

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

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.

Signed-off-by: Eric Rannaud <e@nanocritical.com>
---
 fast-import.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

"Eric Rannaud" <e@nanocritical.com> writes:

That reasoning sounds sensible. Especially given the discussion of "checkpoint" and "progress" we can see in "git fast-import --help" documentation. E.g.

    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.

would not be true without this change, I suspect.

Can we also add a new test or two that protect this from future breakages?

Thanks.

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>
---
 fast-import.c          |   6 +--
 t/t9300-fast-import.sh | 117 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 120 insertions(+), 3 deletions(-)

Any comments on the testing strategy with a background fast-import?

To summarize:
- fast-import is started in the background with a command stream that
  ends with "checkpoint\nprogress checkpoint\n". fast-import keeps
  running after reaching the last command (we don't want fast-import to
  terminate).
- In the meantime, the test is waiting to read "progress checkpoint" in
  the output of fast-import, then it checks the testing conditions.
- Finally, the test ensures that fast-import is still running in the
  background (and thus that it has just observed the effect of
  checkpoint, and not the side effects of fast-import terminating).
- After 10 sec, no matter what, the background fast-import is sent
  "done" and terminates.

I tried to make sure that the test runs quickly and does not (typically) sleep. Upon failure, the test may take up to 10 sec to fully terminate.

However, the test could break under (very heavy) load if fast-import is unable to make progress in a reasonable amount of time (either "progress checkpoint" is not read within 5 sec, or fast-import receives "done" before the testing conditions are checked). Let me know if this is not acceptable.

Should these test cases be at least separated to a new t9304 to circumscribe any such nuisance? Alternatively, a FIFO-based approach could be considered.

(Note: added cases 2 and 4 pass without this patch, but 1 and 3 do not.)

"Eric Rannaud" <e@nanocritical.com> writes:

Hmmmm, it certainly looks a bit too brittle with many tweakables like 10, 50, 55, etc. that heavily depend on the load on the system.

Sorry for asking you to go through the hoops.

Micronit. Just like you have if/then on different lines, have while/do on different lines, i.e.

	while test "$(cat V.output)" != "progress checkpoint"
	do

"ps --pid" is probably not portable (I think we'd do "kill -0 $pid" instead in our existing tests---and it should be kosher according to POSIX [*1*, *2*]).

It is not wrong per-se but we quote INPUT_END when there is no interpolation necessary in the body of here document to help readers, like so:

	cat >input <<-\INPUT_END &&

This we do want interpolation and the above is correct.

[Footnotes]

*1* http://pubs.opengroup.org/onlinepubs/9699919799/utilities/kill.html 
    lists '0' as an allowed signal number to be sent, and defers to the
    description of the kill() function to explain what happens.

*2* http://pubs.opengroup.org/onlinepubs/9699919799/functions/kill.html
    says """If sig is 0 (the null signal), error checking is
    performed but no signal is actually sent. The null signal can be
    used to check the validity of pid."""

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>
---
 fast-import.c          |   6 +--
 t/t9300-fast-import.sh | 129 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 132 insertions(+), 3 deletions(-)

Use named pipes instead of the polling approach. Also incorporate your other comments.

On 27/09/17 20:46, Eric Rannaud wrote:

Since you are using mkfifo here ...

... you need to set the PIPE prerequisite on all of your new tests.

[snip]

ATB, Ramsay Jones

"Eric Rannaud" <e@nanocritical.com> writes:

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?

This '|| true' is here because the process might already have died on its own, which sounds like a sensible precaution.

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.

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.

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.

On Wed, Sep 27, 2017 at 8:48 PM, Junio C Hamano <gitster@pobox.com> wrote:

Right.

Good point, I will swap the order.

I added a comment.

Agreed. Renamed the function background_import_then_checkpoint to reflect the change.

Closing 8 and 9 was just housekeeping on my part. But you raise a good point: what happens then to the stdin of fast-import?

Doesn't fast-import get a copy of 8 (open for both reading and writing), as a child process, and exec 8>&- only closes the copy of the file descriptor in the parent shell, so the named pipe remains open for writing somewhere (in the fast-import process itself, in fact), therefore fast-import will not find EOF on its stdin?

But in any case, it is sensible to delay the closing of 8 and 9 to test_when_finished.

True.

Will follow-up with an updated patch.

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>
---
 fast-import.c          |   6 +--
 t/t9300-fast-import.sh | 126 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 129 insertions(+), 3 deletions(-)

Updated to include Junio's latest remarks.

Also adding the necessary PIPE prereq, as pointed out by Ramsay Jones.

"Eric Rannaud" <e@nanocritical.com> writes:

Thanks for documenting this. Swapping the order of starting fast-import and feeding its input (which is one change in this version relative to the previous one) alone would not help, because in the updated order in this patch, nobody is reading from fast-import until the parent process finishes feeding it.

On Thu, Sep 28, 2017 at 3:35 AM, Junio C Hamano <gitster@pobox.com> wrote:

Darn. That's correct, on a platform with blocking pipes it would not work. I will start the input cat in the background (so it may block while writing), before reading from the output of fast-import.

On Wed, Sep 27, 2017 at 10:07:41PM -0700, Eric Rannaud wrote:

Cygwin doesn't have the PIPE prereq; I've just confirmed that the previous version of this patch has t9300 failing on Cygwin, but this version passes.

On Thu, Sep 28, 2017 at 5:59 AM, Adam Dinwoodie <adam@dinwoodie.org> wrote:

What's the preferred solution here? I can avoid using named pipes entirely:

	read_checkpoint () {
		if read output
		then
			if ! test "$output" = "progress checkpoint"
			then
				echo >&2 "no progress checkpoint received: $output"
				echo 1 > V.result
			else
				echo 0 > V.result
			fi
		else
			echo >&2 "failed to read fast-import output"
			echo 1 > V.result
		fi
	}
	
	# 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
	
		rm -f V.result
	
		( cat "$input_file"
		echo "checkpoint"
		echo "progress checkpoint"
		sleep 3600 &
		echo $! >V.pid
		wait ) | git fast-import $options | read_checkpoint &
	
		# We don't mind if the pipeline has already died by the time the test
		# ends.
		test_when_finished "kill $(cat V.pid) || true"
	
		while ! test -f V.result
		do
			# Try to sleep less than a second, if supported.
			sleep .1 2>/dev/null || sleep 1
		done
		return $(cat V.result)
	}

Do we like this better?

Adam Dinwoodie <adam@dinwoodie.org> writes:

Thanks for a report. So the patch should be good to go, it seems to me.

Eric, thanks again for working on this one.

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>
---
 fast-import.c          |   6 +--
 t/t9300-fast-import.sh | 130 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 133 insertions(+), 3 deletions(-)

Junio, this last version addresses your last remark regarding the potential for the cat $input_file sequence to block when the FIFOs are unbuffered.

The concern is mainly theoretical (*if* the shell function is used correctly): fast-import will not start writing to its own output until it has fully consumed its input (as the only command generating output should be the last one). Nevertheless, in this version the write is done in the background.

Thanks!

"Eric Rannaud" <e@nanocritical.com> writes:

I agree that the concern is theoretical. Without this fix, we may not be able to feed the input fully up to the final 'progress checkpoint' command---because the other side is not reading, we may get stuck while attempting to write "checkpoint" or "progress checkpoint", and may never get to read what fast-import says to get us unstuck.

But if we wanted to solve the theoretical issue (i.e. the command sequence the user of this helper shell function gives us _might_ trigger an output from fast-import) fully, I do not think backgrounding the feeding side is sufficient. The code that reads fd#9 would need to become a while loop that reads and discards extra output until we see the "progress checkpoint", at least, perhaps like the attached patch.

But even with such a fix, we are still at the mercy of the caller of the helper---the helper will get broken if the input happened to have a "progress checkpoint" command itself. There has to be a "good enough" place to stop.

I think that your patch the last round that feeds fd#8 in the foreground (i.e. fully trusting that the caller is sensibly giving input that produces no output) is already a good place to stop.

Your patch this round that feeds fd#8 in the background, plus the attached patch (i.e. not trusting the caller as much and allowing it to use commands that outputs something, within reason), would also be a good place to stop.

But I am not sure your patch this round alone is a good place to stop. It somehow feels halfway either way.

 t/t9300-fast-import.sh | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

On Thu, Sep 28, 2017 at 8:51 PM, Junio C Hamano <gitster@pobox.com> wrote:

I agree. If we're coding defensively against the caller, we do have to include your patch to be effective, you're right. I reckon we likely don't need to be quite so paranoid, at least until this has more users.

Thanks.

Eric Rannaud <e@nanocritical.com> writes:

OK, let's then pick the (not too excessively) defensive version by taking your last one and suggested "while" loop squashed into it.

Thanks.

Eric Rannaud <e@nanocritical.com> writes:

AHHHHhhhh. If that was done intentionally, well, I really have to marvel at the cleverness of the solution! It makes sense now to me.

On Wed, Sep 27, 2017 at 11:02 PM, Junio C Hamano <gitster@pobox.com> wrote:

I plead the Fifth.