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

[elena petrashen <elena.petrashen@gmail.com>]

On Thu, Mar 31, 2016 at 10:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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!