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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Mar 22, 2016 at 8:07 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Elena Petrashen <elena.petrashen@xxxxxxxxx> writes:
>
>> +static int expand_dash_shortcut(const char **argv, int dash_position)
>> +{
>> +     if (!strcmp(argv[dash_position], "-")){
>> +             argv[dash_position] = "@{-1}";
>> +             return 1;
>> +     }
>> +     return 0;
>> +}
>>       int i;
>>       int ret = 0;
>> +     int dash_shortcut = 0;
>>       int remote_branch = 0;
>>       struct strbuf bname = STRBUF_INIT;
>>
>> @@ -213,7 +223,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>>       for (i = 0; i < argc; i++, strbuf_release(&bname)) {
>>               const char *target;
>>               int flags = 0;
>> -
>> +             if (expand_dash_shortcut (argv, i))
>> +                     dash_shortcut = 1;
>>               strbuf_branchname(&bname, argv[i]);
>
> I think this code special cases "-" too much.  Have you considered
> doing this without "dash_shortcut" variable?  With that variable,
> your code says "there is no previous" when the user says "-", but
> isn't that message also appropriate when she says "@{-1}" on the
> command line?  Furthermore, wouldn't the same apply to the case in
> which she said "@{-4}"?
>
> I suspect that you can check that condition immediately after
> calling expand-dash-shortcut and then strbuf-branchname, in other
> words, right here.  And if there is not enough branch switches, you
> can say something like "you gave me @{-4} but you haven't made that
> many branch switches" and continue the loop.
>
> I _think_ strbuf_branchname() leaves "@{-<N>}" when you do not have
> enough branch switches in the reflog, so perhaps
>
>         strbuf_branchname(&bname, (!strcmp(argv[i], "-") ? "@{-1}" : argv[i]));
>         if (starts_with(bname.buf, "@{-")) {
>                 ... say "you do not have enough branch switches" here.
>                 ... when adjusting the message to end-user input,
>                 ... you can look at argv[i] to notice that the original
>                 ... input was "-".
>                 error(...);
>                 continue;
>         }
>
> or something?
>
> That way, there is no change necessary below this line, i.e. from
> here...
>
>>               if (kinds == FILTER_REFS_BRANCHES && !strcmp(head, bname.buf)) {
>>                       error(_("Cannot delete the branch '%s' "
>> @@ -231,9 +242,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(dash_shortcut
>> +                             ? _("There is no previous branch that could be"
>> +                                     " referred to at the moment.")
>> +                             : remote_branch
>> +                                     ? _("remote-tracking branch '%s' not found.")
>> +                                     : _("branch '%s' not found."), bname.buf);
>>                       ret = 1;
>>                       continue;
>>               }
>> @@ -262,6 +276,10 @@ 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));

Right, thank you for the idea and the detailed explanation! I will try
to implement the "not enough switches" message for the v3 version
of the patch the, along with the necessary style corrections that
Matthieu pointed out above.

>
> ... to here.
>
>
>> +                     if (dash_shortcut == 1)
>> +                            printf( _("\nIf that happened by mistake, you may want to restore"
>> +                             " it with:\n\ngit branch %s %s\n"), bname.buf,
>> +                             find_unique_abbrev(sha1, DEFAULT_ABBREV));
>
> This change can be justified only if we believe that people who say
>
>     $ git branch -D -
>
> by mistake are much less clueful than those who say
>
>     $ git branch -D @{-1}
>     $ git branch -D a-misspelled-branch-name
>
> by mistake and need extra help recovering.  Is there an evidence to
> support such an assumption?

I'd think it's a little bit more likely to be the "I thought the
previous branch is "foo" but turns out it's "bar" which I didn't
mean to delete" case for -/@{-1}. case, then just misspelling.
The idea of the warning message was brought up because of
this I think? If we allow deleting via - or even @{-1}, which is
currently possible, it might make sense to additionally enable
the user to recover if she deleted the wrong branch instead of
the required one.

>
> I would actually understand it if this were more like
>
>         if (advice_mistaken_branch_deletion)
>                 printf(_("If you deleted the branch by mistake, you can..."));
>
> so that everybody who ran "git branch -D" on a (wrong) branch by
> mistake can get the extra help.

Would you think this is actually a welcome addition - a (suppressable)
warning for every type of deletion, regardless of whether the shortcuts
are used? There seems to be quite a lot of topics in google where people
are asking how to restore a branch they accidentally deleted. Or that
would be not really consistent with the other situations when people can
delete something (like reset a commit), and they are not immediately
told how can they remedy a situation if that was a mistake?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]