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

Signed-off-by: Elena Petrashen <redacted>
---
This micro-patch is meant to allow “-“ as a short-hand for
“@{-1} for branch -d (Cf. $gmane/230828). Based on feedback
for v2:

* suppressable advice on restoring if a user deletes a branch
via @{-x} or - reference (to ensure safety: if a user deleted
the wrong branch instead what she thought is @{-1}, which seems
to be more likely compared with the situation when branch name
has to be typed in)

* if not enough switches exist to delete branch via @{-x} or -
reference, a corresponding warning is displayed 

Thank you! Looking forward to any feedback.

 Documentation/git-branch.txt |  2 ++
 advice.c                     | 10 ++++++++++
 advice.h                     |  2 ++
 builtin/branch.c             | 22 +++++++++++++++++++---
 t/t3200-branch.sh            | 10 ++++++++++
 5 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 4a7037f..42b96ed 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt

@@ -65,6 +65,8 @@ to happen.
 With a `-d` or `-D` option, `<branchname>` will be deleted.  You may
 specify more than one branch for deletion.  If the branch currently
 has a reflog then the reflog will also be deleted.
+The "@{-N}" syntax for the N-th last branch deletes the specified branch.
+You may also specify - which is synonymous with "@{-1}".
 
 Use `-r` together with `-d` to delete remote-tracking branches. Note, that it
 only makes sense to delete remote-tracking branches if they no longer exist

diff --git a/advice.c b/advice.c
index 4dc5cf1..f14eb68 100644
--- a/advice.c
+++ b/advice.c

@@ -15,6 +15,7 @@ int advice_detached_head = 1;
 int advice_set_upstream_failure = 1;
 int advice_object_name_warning = 1;
 int advice_rm_hints = 1;
+int advice_delete_branch_via_at_ref = 1;
 
 static struct {
 	const char *name;

@@ -35,6 +36,7 @@ static struct {
 	{ "setupstreamfailure", &advice_set_upstream_failure },
 	{ "objectnamewarning", &advice_object_name_warning },
 	{ "rmhints", &advice_rm_hints },
+	{ "deletebranchviaatref", &advice_delete_branch_via_at_ref },
 
 	/* make this an alias for backward compatibility */
 	{ "pushnonfastforward", &advice_push_update_rejected }

@@ -117,3 +119,11 @@ void detach_advice(const char *new_name)
 
 	fprintf(stderr, fmt, new_name);
 }
+
+void delete_branch_advice(const char *name, const char *ref)
+{
+	const char fmt[] =
+	"\nNote: to restore the deleted branch:\n\ngit branch %s %s\n";
+
+	fprintf(stderr, fmt, name, ref);
+}

diff --git a/advice.h b/advice.h
index b341a55..192eef7 100644
--- a/advice.h
+++ b/advice.h

@@ -18,6 +18,7 @@ extern int advice_detached_head;
 extern int advice_set_upstream_failure;
 extern int advice_object_name_warning;
 extern int advice_rm_hints;
+extern int advice_delete_branch_via_at_ref;
 
 int git_default_advice_config(const char *var, const char *value);
 __attribute__((format (printf, 1, 2)))

@@ -26,5 +27,6 @@ int error_resolve_conflict(const char *me);
 extern void NORETURN die_resolve_conflict(const char *me);
 void NORETURN die_conclude_merge(void);
 void detach_advice(const char *new_name);
+void delete_branch_advice(const char *name, const char *ref);
 
 #endif /* ADVICE_H */

diff --git a/builtin/branch.c b/builtin/branch.c
index 7b45b6b..4f5ec72 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c

@@ -178,6 +178,12 @@ static void delete_branch_config(const char *branchname)
 	strbuf_release(&buf);
 }
 
+static void expand_dash_shortcut(const char **argv, int dash_position)
+{
+	if (!strcmp(argv[dash_position], "-"))
+		argv[dash_position] = "@{-1}";
+}
+
 static int delete_branches(int argc, const char **argv, int force, int kinds,
 			   int quiet)
 {

@@ -187,6 +193,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 	const char *fmt;
 	int i;
 	int ret = 0;
+	int at_shortcut = 0;
 	int remote_branch = 0;
 	struct strbuf bname = STRBUF_INIT;
 

@@ -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);
 			ret = 1;
 			continue;
 		}

@@ -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));
 		}
 		delete_branch_config(bname.buf);
 	}

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index a897248..0b59c94 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh

@@ -372,6 +372,16 @@ test_expect_success 'test overriding tracking setup via --no-track' '
 	! test "$(git config branch.my2.merge)" = refs/heads/master
 '
 
+test_expect_success 'test deleting "-" deletes previous branch' '
+	git checkout -b prev &&
+	test_commit prev &&
+	git checkout master &&
+	git branch -D - >actual &&
+	sha1=$(git rev-parse prev | cut -c 1-7) &&
+	echo "Deleted branch prev (was $sha1)." >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'no tracking without .fetch entries' '
 	git config branch.autosetupmerge true &&
 	git branch my6 s &&

-- 
2.8.0.dirty

Elena Petrashen [off-list ref] wrote:

+void delete_branch_advice(const char *name, const char *ref)
+{
+        const char fmt[] =
+        "\nNote: to restore the deleted branch:\n\ngit branch %s %s\n";

Shouldn't that be marked for translation, like is done with the other
strings?

Thanks,
Rémi

On Thu, Mar 31, 2016 at 6:31 PM, Remi Galan Alfonso
[off-list ref] wrote:

Elena Petrashen [off-list ref] wrote:

quoted

+void delete_branch_advice(const char *name, const char *ref)
+{
+        const char fmt[] =
+        "\nNote: to restore the deleted branch:\n\ngit branch %s %s\n";

Shouldn't that be marked for translation, like is done with the other
strings?

Thanks,
Rémi

Thank you for letting me know about that! Could you please
help me out and explain how do I mark it for translation? I tried
to do it the same way as with the other strings but evidently
didn't quite succeed.

Thanks!
Elena

elena petrashen [off-list ref] wrote:

On Thu, Mar 31, 2016 at 6:31 PM, Remi Galan Alfonso
[off-list ref] wrote:

quoted

Elena Petrashen [off-list ref] wrote:

quoted

+void delete_branch_advice(const char *name, const char *ref)
+{
+        const char fmt[] =
+        "\nNote: to restore the deleted branch:\n\ngit branch %s %s\n";

Shouldn't that be marked for translation, like is done with the other
strings?

Thanks,
Rémi

Thank you for letting me know about that! Could you please
help me out and explain how do I mark it for translation? I tried
to do it the same way as with the other strings but evidently
didn't quite succeed.

I am not sure.
I tried to grep similar cases, it seems that you can do the following:

	const char fmt[] = N_("\nNote: to restore [...] \ngit branch %s %s\n");
	fprintf(stderr, _(fmt), name, ref);

Some similar example in builtin/add.c:

	static const char ignore_error[] =
	N_("The following paths are ignored by one of your .gitignore files:\n");
	[...]
			fprintf(stderr, _(ignore_error));

Or you can define fmt as a 'const char *' and in that case do the
following:

	const char *fmt = _("\nNote: to restore [...] \n git branch %s %s\n");
	fprintf(stderr, fmt, name, ref);


In builtin/am.c:
		const char *invalid_line = _("Malformed input line: '%s'.");
	[...]
				ret = error(invalid_line, sb.buf);

I don't know which one is the best way to go though.

Thanks,
Rémi

On Tue, Apr 5, 2016 at 12:53 AM, Remi Galan Alfonso
[off-list ref] wrote:

elena petrashen [off-list ref] wrote:

quoted

On Thu, Mar 31, 2016 at 6:31 PM, Remi Galan Alfonso
[off-list ref] wrote:

quoted

Elena Petrashen [off-list ref] wrote:

quoted

+void delete_branch_advice(const char *name, const char *ref)
+{
+        const char fmt[] =
+        "\nNote: to restore the deleted branch:\n\ngit branch %s %s\n";

Shouldn't that be marked for translation, like is done with the other
strings?

Thanks,
Rémi

Thank you for letting me know about that! Could you please
help me out and explain how do I mark it for translation? I tried
to do it the same way as with the other strings but evidently
didn't quite succeed.

I am not sure.
I tried to grep similar cases, it seems that you can do the following:

        const char fmt[] = N_("\nNote: to restore [...] \ngit branch %s %s\n");
        fprintf(stderr, _(fmt), name, ref);

Some similar example in builtin/add.c:

        static const char ignore_error[] =
        N_("The following paths are ignored by one of your .gitignore files:\n");
        [...]
                        fprintf(stderr, _(ignore_error));

Or you can define fmt as a 'const char *' and in that case do the
following:

        const char *fmt = _("\nNote: to restore [...] \n git branch %s %s\n");
        fprintf(stderr, fmt, name, ref);


In builtin/am.c:
                const char *invalid_line = _("Malformed input line: '%s'.");
        [...]
                                ret = error(invalid_line, sb.buf);

I don't know which one is the best way to go though.

Thanks,
Rémi

Thank you! I'm a bit confused though as the previous implemented advice function
in advice.c (the one I used for reference) doesn't seem to employ this... does
this mean it's also not marked for translation?

void detach_advice(const char *new_name)
{
const char fmt[] =
"Note: checking out '%s'.\n\n"
"You are in 'detached HEAD' state. <.........>Example:\n\n"
"  git checkout -b <new-branch-name>\n\n";

fprintf(stderr, fmt, new_name);
}

Cheers,
Elena

elena petrashen [off-list ref] wrote:

On Tue, Apr 5, 2016 at 12:53 AM, Remi Galan Alfonso
[off-list ref] wrote:

quoted

elena petrashen [off-list ref] wrote:

quoted

On Thu, Mar 31, 2016 at 6:31 PM, Remi Galan Alfonso
[off-list ref] wrote:

quoted

Elena Petrashen [off-list ref] wrote:

quoted

+void delete_branch_advice(const char *name, const char *ref)
+{
+        const char fmt[] =
+        "\nNote: to restore the deleted branch:\n\ngit branch %s %s\n";

Shouldn't that be marked for translation, like is done with the other
strings?

Thanks,
Rémi

Thank you for letting me know about that! Could you please
help me out and explain how do I mark it for translation? I tried
to do it the same way as with the other strings but evidently
didn't quite succeed.

I am not sure.
I tried to grep similar cases, it seems that you can do the following:

        const char fmt[] = N_("\nNote: to restore [...] \ngit branch %s %s\n");
        fprintf(stderr, _(fmt), name, ref);

Some similar example in builtin/add.c:

        static const char ignore_error[] =
        N_("The following paths are ignored by one of your .gitignore files:\n");
        [...]
                        fprintf(stderr, _(ignore_error));

Or you can define fmt as a 'const char *' and in that case do the
following:

        const char *fmt = _("\nNote: to restore [...] \n git branch %s %s\n");
        fprintf(stderr, fmt, name, ref);


In builtin/am.c:
                const char *invalid_line = _("Malformed input line: '%s'.");
        [...]
                                ret = error(invalid_line, sb.buf);

I don't know which one is the best way to go though.

Thanks,
Rémi

Thank you! I'm a bit confused though as the previous implemented advice function
in advice.c (the one I used for reference) doesn't seem to employ this... does
this mean it's also not marked for translation?

void detach_advice(const char *new_name)
{
const char fmt[] =
"Note: checking out '%s'.\n\n"
"You are in 'detached HEAD' state. <.........>Example:\n\n"
"  git checkout -b <new-branch-name>\n\n";

fprintf(stderr, fmt, new_name);
}

It doesn't seem so, at least it doesn't translate:

# $ LANG=fr_FR.UTF-8 git checkout 9494c39
# Note: checking out '9494c39'.
# 
# You are in 'detached HEAD' state. You can look around, make experimental
# changes and commit them, and you can discard any commits you make in this
# state without impacting any branches by performing another checkout.
# 
# If you want to create a new branch to retain commits you create, you may
# do so (now or later) by using -b with the checkout command again. Example:
# 
#   git checkout -b <new-branch-name>
# 
# HEAD est maintenant sur 9494c39... Sync with Git 2.8.1
Only the last line is translated.

Blame traces it back to commit 2857093b that was in 2012 so obviously
the lack of translation isn't due to the message being recent.

Thanks,
Rémi