Re: [PATCH v2] branch -D: allow - as abbreviation of '@{-1}'

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 22, 2016, 17:07 UTC
Message-ID: <xmqq1t72jvac.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1458635056-26633-1-git-send-email-elena.petrashen@gmail.com>

Elena Petrashen <elena.petrashen@gmail.com> writes:

Show 22 quoted lines

> +static int expand_dash_shortcut(const char **argv, int dash_position)
> +{
> +	if (!strcmp(argv[dash_position], "-")){
> +		argv[dash_position] = "@{-1}";
> +		return 1;
> +	}
> +	return 0;
> +}
>  	int i;
>  	int ret = 0;
> +	int dash_shortcut = 0;
>  	int remote_branch = 0;
>  	struct strbuf bname = STRBUF_INIT;
>  
> @@ -213,7 +223,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>  	for (i = 0; i < argc; i++, strbuf_release(&bname)) {
>  		const char *target;
>  		int flags = 0;
> -
> +		if (expand_dash_shortcut (argv, i))
> +			dash_shortcut = 1;
>  		strbuf_branchname(&bname, argv[i]);

I think this code special cases "-" too much. Have you considered doing this without "dash_shortcut" variable? With that variable, your code says "there is no previous" when the user says "-", but isn't that message also appropriate when she says "@{-1}" on the command line? Furthermore, wouldn't the same apply to the case in which she said "@{-4}"?

I suspect that you can check that condition immediately after calling expand-dash-shortcut and then strbuf-branchname, in other words, right here. And if there is not enough branch switches, you can say something like "you gave me @{-4} but you haven't made that many branch switches" and continue the loop.

I _think_ strbuf_branchname() leaves "@{-<N>}" when you do not have enough branch switches in the reflog, so perhaps

	strbuf_branchname(&bname, (!strcmp(argv[i], "-") ? "@{-1}" : argv[i]));
	if (starts_with(bname.buf, "@{-")) {
		... say "you do not have enough branch switches" here.
		... when adjusting the message to end-user input,
		... you can look at argv[i] to notice that the original
                ... input was "-".
		error(...);
		continue;
	}

or something?

That way, there is no change necessary below this line, i.e. from here...

Show 22 quoted lines

>  		if (kinds == FILTER_REFS_BRANCHES && !strcmp(head, bname.buf)) {
>  			error(_("Cannot delete the branch '%s' "
> @@ -231,9 +242,12 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>  					    | RESOLVE_REF_ALLOW_BAD_NAME,
>  					    sha1, &flags);
>  		if (!target) {
> -			error(remote_branch
> -			      ? _("remote-tracking branch '%s' not found.")
> -			      : _("branch '%s' not found."), bname.buf);
> +			error(dash_shortcut
> +				? _("There is no previous branch that could be"
> +					" referred to at the moment.")
> +				: remote_branch
> +					? _("remote-tracking branch '%s' not found.")
> +					: _("branch '%s' not found."), bname.buf);
>  			ret = 1;
>  			continue;
>  		}
> @@ -262,6 +276,10 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>  			       (flags & REF_ISBROKEN) ? "broken"
>  			       : (flags & REF_ISSYMREF) ? target
>  			       : find_unique_abbrev(sha1, DEFAULT_ABBREV));

... to here.

> +			if (dash_shortcut == 1)
> +			       printf( _("\nIf that happened by mistake, you may want to restore"
> +				" it with:\n\ngit branch %s %s\n"), bname.buf,
> +				find_unique_abbrev(sha1, DEFAULT_ABBREV));

This change can be justified only if we believe that people who say

    $ git branch -D -

by mistake are much less clueful than those who say

    $ git branch -D @{-1}
    $ git branch -D a-misspelled-branch-name

by mistake and need extra help recovering. Is there an evidence to support such an assumption?

I would actually understand it if this were more like

	if (advice_mistaken_branch_deletion)
        	printf(_("If you deleted the branch by mistake, you can..."));

so that everybody who ran "git branch -D" on a (wrong) branch by mistake can get the extra help.