Re: [PATCH v3 2/2] fetch/push: readd rsync support

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Sep 30, 2007, 12:58 UTC
Message-ID: <Pine.LNX.4.64.0709301344560.28395@racer.site>
In-Reply-To: <7vhclciszz.fsf@gitster.siamese.dyndns.org>

Hi,

On Sat, 29 Sep 2007, Junio C Hamano wrote:

Show 7 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > We lost rsync support when transitioning from shell to C.  Support it
> > again (even if the transport is technically deprecated, some people just
> > do not have any chance to use anything else).
> 
> s/chance/choice/?

Hehe. I did mean it like that, but feel free to edit!

Show 10 quoted lines

> > +test "$TEST_RSYNC" && {
> 
> Somehow this feels dirty ... perhaps leave early like:
> 
> 	if test -z "$TEST_RSYNC"
>         then
>         	test_expect_success 'skipping rsync transport tests' :
> 		test_done
>                 exit
> 	fi

Okay.

Show 21 quoted lines

> > diff --git a/transport.c b/transport.c
> > index 4f9cddc..a2ee8f3 100644
> > --- a/transport.c
> > +++ b/transport.c
> > @@ -6,6 +6,330 @@
> > ...
> > +{
> > +	struct strbuf buf = STRBUF_INIT, temp_dir = STRBUF_INIT;
> > +	struct ref dummy, *tail = &dummy;
> > +	struct child_process rsync;
> > +	const char *args[5];
> > +	int temp_dir_len;
> > +
> > +	/* copy the refs to the temporary directory */
> > +
> > +	strbuf_addstr(&temp_dir, git_path("rsync-refs-XXXXXX"));
> > +	if (!mkdtemp(temp_dir.buf))
> > +		die ("Could not make temporary directory");
> 
> I wonder how portable mkdtemp() is (it does not seem to be POSIX);
> would we need something in compat/ perhaps based on tempnam()?

My man page said BSD, so I did not bother any more...

Besides, using "tmpnam()" makes gcc say that it is dangerous, and I should use mkstemp() instead...

Show 28 quoted lines

> > +static int fetch_objs_via_rsync(struct transport *transport,
> > +				 int nr_objs, struct ref **to_fetch)
> > +{
> > +	struct strbuf buf = STRBUF_INIT;
> > +	struct child_process rsync;
> > +	const char *args[8];
> > +	int result;
> > +
> > +	strbuf_addstr(&buf, transport->url);
> > +	strbuf_addstr(&buf, "/objects/");
> > +
> > +	memset(&rsync, 0, sizeof(rsync));
> > +	rsync.argv = args;
> > +	rsync.stdout_to_stderr = 1;
> > +	args[0] = "rsync";
> > +	args[1] = transport->verbose ? "-rv" : "-r";
> > +	args[2] = "--ignore-existing";
> > +	args[3] = "--exclude";
> > +	args[4] = "info";
> > +	args[5] = buf.buf;
> > +	args[6] = get_object_directory();
> > +	args[7] = NULL;
> 
> Hmm, we used to do "rsync $remote/objects/ $our/.git/objects/", but this 
> omits the trailing "/" from our side.  I suspect the reason was to deal 
> with the case where our .git/objects was a symlink to elsewhere (which 
> was how you did alternates before alternates was invented), which may 
> not matter anymore these days.

Nevertheless, this was an oversight on my side. Will change to use a second strbuf for the local objects directory.