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

From: elena petrashen <elena.petrashen@gmail.com>
Date: Apr 6, 2016, 10:42 UTC
Message-ID: <CAJPOeMd9hJpVubRbSTsBGBHtFSi5-LsQOr=h=Gtu+7qZgq_Fbg@mail.gmail.com>
In-Reply-To: <xmqqmvpemot7.fsf@gitster.mtv.corp.google.com>

On Thu, Mar 31, 2016 at 10:26 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 48 quoted lines

> 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?

Show 21 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

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!