On Thu, Mar 31, 2016 at 10:26 PM, Junio C Hamano <[email protected]> wrote:
> Elena Petrashen <[email protected]> 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.
Thank you, and I apologize it takes me kind of long to figure it all right.
For me the reason I did the realisation the way it is it's to distinguish the
error messages in cases:
1. the branch @{-1} was deleted already
2. the "not enough switches case", there was no previous branch
as far as I understand in #1 we should recieve "branch foo was not found"
in #2 "not enough swiches to delete @{-1}". I believe if we check for "@{-"
immediately, there would be no opportunity to distinguish, and we will be
getting "not enough swithes" even if there was enough switches, it's just
that the branch was deleted?
>
>> @@ -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
Wouldn't people most of the time have somewhat more different names
than foo/fooo/foooo? Anyways, I guess that should be reasonable. Will just
add advice for every branch deletion next time.
Thank you!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
