Re: [PATCH v2 1/3] remote rm: delete remote configuration as the last

From: Junio C Hamano <gitster@pobox.com>
Date: May 23, 2014, 18:55 UTC
Message-ID: <xmqqa9a8jpdy.fsf@gitster.dls.corp.google.com>
In-Reply-To: <537F22DB.7070102@opera.com>

Jens Lindström <jl@opera.com> writes:

Show 6 quoted lines

> When removing a remote, delete the remote-tracking branches before
> deleting the remote configuration.  This way, if the operation fails or
> is aborted while deleting the remote-tracking branches, the command can
> be rerun to complete the operation.
>
> Signed-off-by: Jens Lindström <jl@opera.com>

I think this is a good change, regardless of the "calling delete-ref millions of times, each time rewriting the whole packed-ref file, is inefficient" issue.

I wonder if the new "if (!result)" block wants to have its own else clause to let the users know that the definition of the remote was still left intact under such an unusual condition where ref deletion somehow fails, but it would be OK as the users have presumably seen error messages and understand that removal did not happen.

Will queue.  Thanks.

Show 31 quoted lines

> ---
>  builtin/remote.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/remote.c b/builtin/remote.c
> index b3ab4cf..84802cd 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -789,10 +789,6 @@ static int rm(int argc, const char **argv)
>  	known_remotes.to_delete = remote;
>  	for_each_remote(add_known_remote, &known_remotes);
>  
> -	strbuf_addf(&buf, "remote.%s", remote->name);
> -	if (git_config_rename_section(buf.buf, NULL) < 1)
> -		return error(_("Could not remove config section '%s'"), buf.buf);
> -
>  	read_branches();
>  	for (i = 0; i < branch_list.nr; i++) {
>  		struct string_list_item *item = branch_list.items + i;
> @@ -837,6 +833,12 @@ static int rm(int argc, const char **argv)
>  	}
>  	string_list_clear(&skipped, 0);
>  
> +	if (!result) {
> +		strbuf_addf(&buf, "remote.%s", remote->name);
> +		if (git_config_rename_section(buf.buf, NULL) < 1)
> +			return error(_("Could not remove config section '%s'"), buf.buf);
> +	}
> +
>  	return result;
>  }