Re: [PATCH] transport: drop support for git-over-rsync

From: Jeff King <peff@peff.net>
Date: Jan 30, 2016, 07:28 UTC
Message-ID: <20160130072813.GB14696@sigill.intra.peff.net>
In-Reply-To: <20160130072126.GA14696@sigill.intra.peff.net>

On Sat, Jan 30, 2016 at 02:21:26AM -0500, Jeff King wrote:

Show 5 quoted lines

> Even the commit porting rsync over to C from shell (cd547b4)
> lists it as deprecated! So between the 10 years of informal
> warnings, and the fact that it has been severely broken
> since 2007, it's probably safe to simply remove it without
> further deprecation warnings.

Obviously it would not be the end of the world to start with a warning("git-over-rsync is going away!") patch, or to mention it in the release notes and hold this patch back for a version or two. But after seeing the extent of the breakage, I can't believe anybody has used it for years. But I'm also open to erring on the conservative side.

Show 12 quoted lines

> ---
>  Documentation/config.txt           |   2 +-
>  Documentation/git-bundle.txt       |   2 +-
>  Documentation/git-clone.txt        |   3 +-
>  Documentation/git-repack.txt       |   2 +-
>  Documentation/git.txt              |   2 -
>  Documentation/gitcore-tutorial.txt |  18 +-
>  Documentation/gittutorial.txt      |   2 +-
>  Documentation/urls.txt             |   6 +-
>  t/t5510-fetch.sh                   |  36 ----
>  transport.c                        | 332 +------------------------------------
>  10 files changed, 10 insertions(+), 395 deletions(-)

I cleaned up all of the documentation references I could find, except one: the git-svn manual notes that because SVN metadata is kept outside of refs, you should use rsync for cloning. I'm not sure what to recommend there. I don't eve nthink that "git clone rsync://" would copy that metadata. So perhaps it just meant "rsync the whole thing yourself" (in which case it is OK to leave it).

Show 10 quoted lines

> @@ -984,11 +658,7 @@ struct transport *transport_get(struct remote *remote, const char *url)
>  	if (helper) {
>  		transport_helper_init(ret, helper);
>  	} else if (starts_with(url, "rsync:")) {
> -		transport_check_allowed("rsync");
> -		ret->get_refs_list = get_refs_via_rsync;
> -		ret->fetch = fetch_objs_via_rsync;
> -		ret->push = rsync_transport_push;
> -		ret->smart_options = NULL;
> +		die("git-over-rsync is no longer supported");

I added this as a convenience to anybody who does try to use it (otherwise they get a more confusing "eh, what is rsync" message).

But if we drop this "if" entirely, then somebody can ship git-remote-rsync, if they really wanted to (you can use it either way as "rsync::whatever" but this blocks the fallback of "rsync:whatever").

-Peff