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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 31, 2016, 19:26 UTC
Message-ID: <xmqqmvpemot7.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1459416327-795-1-git-send-email-elena.petrashen@gmail.com>

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

Show 23 quoted lines

> @@ -214,6 +221,9 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>  		const char *target;
>  		int flags = 0;
>  
> +		expand_dash_shortcut (argv, i);
> +		if(!strncmp(argv[i], "@{-", strlen("@{-")))
> +			at_shortcut = 1;
>  		strbuf_branchname(&bname, argv[i]);
>  		if (kinds == FILTER_REFS_BRANCHES && !strcmp(head, bname.buf)) {
>  			error(_("Cannot delete the branch '%s' "
> @@ -231,9 +241,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((!strncmp(bname.buf, "@{-", strlen("@{-")))
> +				? _("There is not enough branch switches to"
> +					" delete '%s'.")
> +				: remote_branch
> +					? _("remote-tracking branch '%s' not found.")
> +					: _("branch '%s' not found."), bname.buf);

I was expecting that the check for "@{-" in bname.buf would be done immediately after strbuf_branchname(&bname, argv[i]) we see in the previous hunk (and an error message issued there), i.e. something like:

        orig_arg = argv[i];
        if (!strcmp(orig_arg, "-"))
		strbuf_branchname(&bname, "@{-1}");
	else
		strbuf_branchname(&bname, argv[i]);
        if (starts_with(bname.buf, "@{-")) {
		error("Not enough branch switches to delete %s", orig_arg);
                ... clean up and fail ...
	}

That would give you sensible error message for "branch -d -", "branch -d @{-1}" and "branch -d @{-4}" if you haven't visited different branches enough times.

The hope was that the remainder of the code (including this error message) would not have to worry about this "not enough switches" error at all if done that way.

Show 8 quoted lines

> @@ -262,6 +275,9 @@ 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));
> +			if (at_shortcut && advice_delete_branch_via_at_ref)
> +			       delete_branch_advice (bname.buf,
> +				find_unique_abbrev(sha1, DEFAULT_ABBREV));
>  		}

The existing !quiet report already said "deleted branch" with the concrete branch name, not "@{-1}" or "-", taken from bname.buf at this point.

If the advice on how to recover a deletion by mistake would help the user, wouldn't that apply equally to the case where the user made a typo in the original command line, i.e. "branch -d foo" when she meant to delete "branch -d fooo", as well? If we drop the "at_shortcut" check from this if() statement, wouldn't the result be more helpful?

Thanks