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

Elena Petrashen [off-list ref] writes:

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

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

On Tue, Mar 22, 2016 at 8:07 PM, Junio C Hamano [off-list ref] wrote:

Elena Petrashen [off-list ref] writes:

quoted

+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...

quoted

              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.

quoted

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