Re: [PATCH v5] branch: introduce --show-current display option
On Fri, Oct 26, 2018 at 09:52:24AM +0900, Junio C Hamano wrote: > Eric Sunshine writes: > > >> + test_when_finished "git tag -d branch-and-tag-name" && > >> + git tag branch-and-tag-name && > > > > If git-tag crashes before actually creating the new tag, then "git tag > > -d", passed to test_when_finished(), will error out too, which is > > probably undesirable since "cleanup code" isn't expected to error out. > > Ah, I somehow thought that clean-up actions set up via when_finished > are allowed to fail without affecting the outcome, but apparently I > was mistaken. If a when_finished block fails, we consider that a test failure. But if we failed to create the tag, the test is failing anyway. Do we actually care at that point? We would still want to make sure we run the rest of the cleanup, but looking at the definition of test_when_finished(), I think we do. > I haven't gone through the list of when_finished clean-up actions > that do not end with "|| :"; I suspect some of them are simply being > sloppy and would want to have "|| :", but what I want to find out > out of such an audit is if there is a legitimate case where it helps > to catch failures in the clean-up actions. If there is none, then > ... I think in the success case it is legitimately helpful. If that "tag -d" failed above (after the tag creation and the rest of the test succeeded), it would certainly be unexpected and we would want to know that it happened. So I think "|| :" in this case is not just unnecessary, but actively bad. -Peff
Re: [PATCH v5] branch: introduce --show-current display option
Eric Sunshine writes: >> + test_when_finished " >> + git checkout branch-one >> + git branch -D branch-and-tag-name >> + " && >> + git checkout -b branch-and-tag-name && >> + test_when_finished "git tag -d branch-and-tag-name" && >> + git tag branch-and-tag-name && We've discussed about the exit status from clean-up code already, but another thing worth noticing is that it probably is easier to see what is going on if we use a single when-finished to clear both branch and the tag with the same name. Something like test_when_finished " git checkout branch-one git branch -D branch-and-tag-name git tag -d branch-and-tag-name : " && upfront before doing anything else. "checkout" may break if the test that follows when-finished accidentally removes branch-one and that would cascade to a failure to remove branch-and-tag-name branch (because we fail to move away from it), but because there is no && in between, we'd clean as much as we could in such a case, which may or may not be a good thing. And then we hide the exit code by having a ":" at the end.
Re: [PATCH v5] branch: introduce --show-current display option
Eric Sunshine writes: >> + test_when_finished "git tag -d branch-and-tag-name" && >> + git tag branch-and-tag-name && > > If git-tag crashes before actually creating the new tag, then "git tag > -d", passed to test_when_finished(), will error out too, which is > probably undesirable since "cleanup code" isn't expected to error out. Ah, I somehow thought that clean-up actions set up via when_finished are allowed to fail without affecting the outcome, but apparently I was mistaken. This however can be argued both ways---if you create a tag first and try to set up the clean-up action, during which you may get in trouble and end up leaving the tag behind. So rather than swapping the two lines, explicitly preparing for the case the clean-up action fails, i.e. the first alternative below, would be a good fix. Also it is a good question if the tag need to be even cleaned up. > You could fix it this way: > > test_when_finished "git tag -d branch-and-tag-name || :" && > git tag branch-and-tag-name && > > or, even better, just swap the two lines: > > git tag branch-and-tag-name && > test_when_finished "git tag -d branch-and-tag-name" && > However, do you even need to clean up the tag? Are there tests > following this one which expect a certain set of tags and fail if this > new one is present? If not, a simpler approach might be just to leave > the tag alone (and the branch too if that doesn't need to be cleaned > up). > >> + git branch --show-current >actual && >> + test_cmp expect actual >> +' A bigger question we may want to ask ourselves is if we want to detect failures from these clean-up actions in the first place. There are many hits from "git grep 'when_finished .*|| :' t/", which may be a sign that the when_finished mechanism was misdesigned and we should simply ignore the exit status from the clean-up actions instead. I haven't gone through the list of when_finished clean-up actions that do not end with "|| :"; I suspect some of them are simply being sloppy and would want to have "|| :", but what I want to find out out of such an audit is if there is a legitimate case where it helps to catch failures in the clean-up actions. If there is none, then ...
Re: [PATCH v5] branch: introduce --show-current display option
On Thu, Oct 25, 2018 at 3:04 PM Daniels Umanovskis wrote: > When called with --show-current, git branch will print the current > branch name and terminate. Only the actual name gets printed, > without refs/heads. In detached HEAD state, nothing is output. > > Signed-off-by: Daniels Umanovskis > --- > diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh > @@ -100,6 +100,50 @@ test_expect_success 'git branch -v pattern does not show > branch summaries' ' > +test_expect_success 'git branch `--show-current` works properly when tag > exists' ' > + cat >expect <<-\EOF && > + branch-and-tag-name > + EOF > + test_when_finished " > + git checkout branch-one > + git branch -D branch-and-tag-name > + " && > + git checkout -b branch-and-tag-name && > + test_when_finished "git tag -d branch-and-tag-name" && > + git tag branch-and-tag-name && If git-tag crashes before actually creating the new tag, then "git tag -d", passed to test_when_finished(), will error out too, which is probably undesirable since "cleanup code" isn't expected to error out. You could fix it this way: test_when_finished "git tag -d branch-and-tag-name || :" && git tag branch-and-tag-name && or, even better, just swap the two lines: git tag branch-and-tag-name && test_when_finished "git tag -d branch-and-tag-name" && However, do you even need to clean up the tag? Are there tests following this one which expect a certain set of tags and fail if this new one is present? If not, a simpler approach might be just to leave the tag alone (and the branch too if that doesn't need to be cleaned up). > + git branch --show-current >actual && > + test_cmp expect actual > +'
[PATCH v5] branch: introduce --show-current display option
When called with --show-current, git branch will print the current
branch name and terminate. Only the actual name gets printed,
without refs/heads. In detached HEAD state, nothing is output.
Intended both for scripting and interactive/informative use.
Unlike git branch --list, no filtering is needed to just get the
branch name.
Signed-off-by: Daniels Umanovskis
---
Submitting v5 now that a week has passed since latest maintainer
comments.
This is basically v4 but with small fixes to the test, as proposed
by Junio on pu, and additionally replacing a subshell
with { .. } since Dscho and Eric discovered the negative
performance effects of subshell invocations.
Documentation/git-branch.txt | 6 -
builtin/branch.c | 25 ++--
t/t3203-branch-output.sh | 44
3 files changed, 72 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index bf5316ffa9..0babb9b1be 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -9,7 +9,7 @@ SYNOPSIS
[verse]
'git branch' [--color[=] | --no-color] [-r | -a]
- [--list] [-v [--abbrev= | --no-abbrev]]
+ [--list] [--show-current] [-v [--abbrev= | --no-abbrev]]
[--column[=] | --no-column] [--sort=]
[(--merged | --no-merged) []]
[--contains []]
@@ -160,6 +160,10 @@ This option is only applicable in non-verbose mode.
branch --list 'maint-*'`, list only the branches that match
the pattern(s).
+--show-current::
+ Print the name of the current branch. In detached HEAD state,
+ nothing is printed.
+
-v::
-vv::
--verbose::
diff --git a/builtin/branch.c b/builtin/branch.c
index c396c41533..46f91dc06d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -443,6 +443,21 @@ static void print_ref_list(struct ref_filter *filter,
struct ref_sorting *sortin
free(to_free);
}
+static void print_current_branch_name(void)
+{
+ int flags;
+ const char *refname = resolve_ref_unsafe("HEAD", 0, NULL, );
+ const char *shortname;
+ if (!refname)
+ die(_("could not resolve HEAD"));
+ else if (!(flags & REF_ISSYMREF))
+ return;
+ else if (skip_prefix(refname, "refs/heads/", ))
+ puts(shortname);
+ else
+ die(_("HEAD (%s) points outside of refs/heads/"), refname);
+}
+
static void reject_rebase_or_bisect_branch(const char *target)
{
struct worktree **worktrees = get_worktrees(0);
@@ -581,6 +596,7 @@ static int edit_branch_description(const char *branch_name)
int cmd_branch(int argc, const char **argv, const char *prefix)
{
int delete = 0, rename = 0, copy = 0, force = 0, list = 0;
+ int show_current = 0;
int reflog = 0, edit_description = 0;
int quiet = 0, unset_upstream = 0;
const char *new_upstream = NULL;
@@ -620,6 +636,7 @@ int cmd_branch(int argc, const char **argv, const char
*prefix)
OPT_BIT('c', "copy", , N_("copy a branch and its reflog"),
1),
OPT_BIT('C', NULL, , N_("copy a branch, even if target
exists"), 2),
OPT_BOOL('l', "list", , N_("list branch names")),
+ OPT_BOOL(0, "show-current", _current, N_("show current
branch name")),
OPT_BOOL(0, "create-reflog", , N_("create the branch's
reflog")),
OPT_BOOL(0, "edit-description", _description,
N_("edit the description for the branch")),
@@ -662,14 +679,15 @@ int cmd_branch(int argc, const char **argv, const char
*prefix)
argc = parse_options(argc, argv, prefix, options, builtin_branch_usage,
0);
- if (!delete && !rename && !copy && !edit_description && !new_upstream
&& !unset_upstream && argc == 0)
+ if (!delete && !rename && !copy && !edit_description && !new_upstream &&
+ !show_current && !unset_upstream && argc == 0)
list = 1;
if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE ||
filter.points_at.nr ||
filter.no_commit)
list = 1;
- if (!!delete + !!rename + !!copy + !!new_upstream +
+ if (!!delete + !!rename + !!copy + !!new_upstream + !!show_current +
list + unset_upstream > 1)
usage_with_options(builtin_branch_usage, options);
@@ -697,6 +715,9 @@ int cmd_branch(int argc, const char **argv, const char
*prefix)
if (!argc)
die(_("branch name required"));
return delete_branches(argc, argv, delete > 1, filter.kind,
quiet);
+ } else if (show_current) {
+ print_current_branch_name();
+ return 0;
} else if (list) {
/* git branch --local also shows HEAD when it is detached */
if ((filter.kind & FILTER_REFS_BRANCHES) && filter.detached)
diff
