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

Elena Petrashen [off-list ref] writes:

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.

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

On Thu, Mar 31, 2016 at 10:26 PM, Junio C Hamano [off-list ref] wrote:

Elena Petrashen [off-list ref] writes:

quoted

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

quoted

@@ -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!