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

[Junio C Hamano <gitster@pobox.com>]

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

> 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.

> ---
>  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;
>  }