Re: [PATCH v3 18/23] transport-helper: change import semantics

From

Dmitry Ivankov <divanorama@gmail.com>

Date

Jul 18, 2011, 11:13 UTC

Message-ID

<CA+gfSn_Cxkmrz6k0ScZ+fTNZvzU7oA9Li5G1VBFtkohzYOFm7w@mail.gmail.com>

In-Reply-To

<1310821424-4750-19-git-send-email-srabbelier@gmail.com>

Hi,

@@ -417,11 +418,8 @@ static int fetch_with_import(struct transport *transport,
 		sendline(data, &buf);
 		strbuf_reset(&buf);
 	}
-	if (disconnect_helper(transport))
-		die("Error while disconnecting helper");
 	if (finish_command(&fastimport))
 		die("Error while running fast-import");
-
 	free(fastimport.argv);
 	fastimport.argv = NULL;

This is related to the done feature but a helper can use the done
feature without this hunk.
And it does change the import semantics, so I'd suggest moving the
hunk here, to
"[PATCH v3 18/23] transport-helper: change import semantics".

On Sat, Jul 16, 2011 at 7:03 PM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> Currently the helper must somehow guess how many import statements to
> read before it starts outputting its fast-export stream. This is
> because the remote helper infrastructure runs fast-import only once,
> so the helper is forced to output one stream for all import commands
> it will receive. The only reason this worked in the past was because
> only one ref was imported at a time.
>
> Change the semantics of the import statement such that it matches
> that of the push statement. That is, the import statement is followed
> by a series of import statements that are terminated by a '\n'.

I'll add more comments to the "import" job termination.

Before these two patches:
The helper input was closed which caused it to exit just after writing
the import stream.
(first, helper can terminate on eof; second it can terminate on '\n'
just before eof; third, it may know that the import always was the
last command and terminate before reading '\n').
The transport-helper.o didn't held a copy of helper's stdout, so once
the helper exits this end of import-stream pipe becomes closed.
And then the importer saw EOF on stdin and could terminate normally.

After these patches:
transport-helper waits for importer to terminate, so the helper must either
a) crash or exit during or just after the import and reading no more
than '\n' (blocking read beyond this character may hang us up)
or
b) use the 'done' feature to make importer terminate normally

So the old helper won't hang us up if it terminates (or at least
closes stdout) on just after the import or if it can read '\n' (but
not blocking read further) and terminate.
But if it just ignores empty lines and waits for new commands or eof
as an exit indicator, it all hangs.

Sadly, old documentation didn't mention many of these quirks, but on
the other hand the old semantic is a bit broken and not documented so
it's all ok.

We should definitely add a new description for import command to
Documentation/git-remote-helpers.txt
Something roughly like this addition:

A batch sequence of one or more import commands is terminated
with a blank line. Single fast-import stream should be produced for
the whole batch.
If the helper is able to proceed with more commands after the import
(in earlier versions import used to be the last command for a helper)
it must use the "done" feature to indicate the end of this batch's
import stream for the importer, otherwise the importer will wait forever
and the caller will wait for the importer to finish.

Aside from documenting and squashing I think this patch is the best
way to proceed with improving the import command.

>
> Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
> ---
>
>  As Jonathan suggested we now follow push' example, rather than
>  'list'. It makes the remote-testgit code a bit longer, but it means
>  less changes to remote-helper.c.
>
>  git-remote-testgit.py     |   16 +++++++++++++++-
>  t/t5800-remote-helpers.sh |    2 +-
>  transport-helper.c        |    3 +++
>  3 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/git-remote-testgit.py b/git-remote-testgit.py
> index 0b5928d..1ed7a56 100644
> --- a/git-remote-testgit.py
> +++ b/git-remote-testgit.py
> @@ -120,8 +120,22 @@ def do_import(repo, args):
>     if not repo.gitdir:
>         die("Need gitdir to import")
>
> +    ref = args[0]
> +    refs = [ref]
> +
> +    while True:
> +        line = sys.stdin.readline()
> +        if line == '\n':
> +            break
> +        if not line.startswith('import '):
> +            die("Expected import line.")
> +
> +        # strip of leading 'import '
> +        ref = line[7:].strip()
> +        refs.append(ref)
> +
>     repo = update_local_repo(repo)
> -    repo.exporter.export_repo(repo.gitdir, args)
> +    repo.exporter.export_repo(repo.gitdir, refs)
>
>     print "done"
>
> diff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh
> index 12f471c..1c62001 100755
> --- a/t/t5800-remote-helpers.sh
> +++ b/t/t5800-remote-helpers.sh
> @@ -98,7 +98,7 @@ test_expect_success 'fetch new branch' '
>        compare_refs public HEAD localclone FETCH_HEAD
>  '
>
> -test_expect_failure 'fetch multiple branches' '
> +test_expect_success 'fetch multiple branches' '
>        (cd localclone &&
>         git fetch
>        ) &&
> diff --git a/transport-helper.c b/transport-helper.c
> index a8f69b0..0c00be9 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -418,6 +418,9 @@ static int fetch_with_import(struct transport *transport,
>                sendline(data, &buf);
>                strbuf_reset(&buf);
>        }
> +
> +       write_constant(data->helper->in, "\n");
> +
>        if (finish_command(&fastimport))
>                die("Error while running fast-import");
>        free(fastimport.argv);
> --
> 1.7.5.1.292.g728120
>
>