Re: [PATCH] remote: add 'rm' subcommand

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 3, 2007, 10:07 UTC
Message-ID: <7vy7fojc3s.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <11810859232541-git-send-email-jbowes@dangerouslyinc.com>

James Bowes <jbowes@dangerouslyinc.com> writes:

Show 6 quoted lines

> Introduce git-remote rm <name> which will:
>  - Remove the remote config entry for <name>.
>  - Remove any config entries for tracking branches of <name>.
>  - Remove any stored remote branches of <name>.
>
> Signed-off-by: James Bowes <jbowes@dangerouslyinc.com>

Let's continue with this in 1.5.4 cycle. Together with Dscho's "--mirror" patch, this will make "remote" more complete.

> If there is any interest in this, I'll follow up later with additions
> to the docs.

This is not a good thing to say here. If even the original author cannot be bothered to perfect it unconditionally (iow even when other people do not realize how useful it would be initially and nobody seem to be interested), it does not help convincing others that it is a good thing for the community to take the patch.

Show 12 quoted lines

> +sub rm_remote {
> +    my ($name) = @_;
> +	if (!exists $remote->{$name}) {
> +		print STDERR "No such remote $name\n";
> +		return;
> +	}
> +
> +	$git->command('config', '--remove-section', "remote.$name");
> +
> +	eval {
> +	    my @trackers = $git->command('config', '--get-regexp',
> +			'branch.*.remote', $name);

Is this correct, or should it be '^branch\..*\.remote$'

> +		for (@trackers) {
> +			/^branch\.(.*)?\.remote/;

And this one.  Why do we have '?' there?  Perhaps...

	if (/^branch\.(.*\)\.remote /) {
		$git->config('--unset', "branch.$1.remote");
		$git->config('--unset', "branch.$1.merge");
	} else {
        	die "Gaah, why $_ is not branch.<<name>>.remote???"
	}

We seem to have another subroutine to prune remote tracking branches, which does it slightly differently. Maybe we would want to share code with that codepath?

Other than that, I think the patch is sane, with your later "documentation patch".

We would want a handful tests, including ones to check error conditions, such as trying to remove a remote that does not exist.