[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH v3][Outreachy] branch -D: allow - as abbreviation of @{-1}
From:       Junio C Hamano <gitster () pobox ! com>
Date:       2016-03-31 19:26:28
Message-ID: xmqqmvpemot7.fsf () gitster ! mtv ! corp ! google ! com
[Download RAW message or body]

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

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

> @@ -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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]