On Tue, Mar 22, 2016 at 8:07 PM, Junio C Hamano <[email protected]> wrote:
> Elena Petrashen <[email protected]> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
