| Message ID | 20181107225619.6683-1-rafa.almas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | branch: make --show-current use already resolved HEAD | expand |
Rafael Ascensão <rafa.almas@gmail.com> writes: > print_current_branch_name() tries to resolve HEAD and die() when it > doesn't resolve it successfully. But the conditions being tested are > always unreachable because early in branch:cmd_branch() the same logic > is performed. > > Eliminate the duplicate and unreachable code, and update the current > logic to the more reliable check for the detached head. Nice. > I still think the mention about scripting should be removed from the > original commit message, leaving it open to being taught other tricks > like --verbose that aren't necessarily script-friendly. I'd prefer to see scriptors avoid using "git branch", too. Unlike end-user facing documentation where we promise "we do X and will continue to do so because of Y" to the readers, the log message is primarily for recording the original motivation of the change, so that we can later learn "we did X back then because we thought Y". When we want to revise X, we revisit if the reason Y is still valid. So in that sense, the door to "break" the scriptability is still open. > But the main goal of this patch is to just bring some attention to this, > as I mentioned it in a previous thread but it got lost. This idea of yours seems to lead to a better implementation, and indeed "got lost" is a good way to describe what happened---I do not recall seeing it, for example. Thanks for bringing it back. > diff --git a/builtin/branch.c b/builtin/branch.c > index 46f91dc06d..1c51d0a8ca 100644 > --- a/builtin/branch.c > +++ b/builtin/branch.c > @@ -38,6 +38,7 @@ static const char * const builtin_branch_usage[] = { > > static const char *head; > static struct object_id head_oid; > +static int head_flags = 0; You've eliminated the "now unnecessary" helper and do everything inside cmd_branch(), so perhaps this can be made function local, no? > @@ -668,10 +654,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix) > > track = git_branch_track; > > - head = resolve_refdup("HEAD", 0, &head_oid, NULL); > + head = resolve_refdup("HEAD", 0, &head_oid, &head_flags); > if (!head) > die(_("Failed to resolve HEAD as a valid ref.")); > - if (!strcmp(head, "HEAD")) > + if (!(head_flags & REF_ISSYMREF)) > filter.detached = 1; Nice to see we can reuse the resolve_refdup() we already have. > else if (!skip_prefix(head, "refs/heads/", &head)) > die(_("HEAD not found below refs/heads!")); > @@ -716,7 +702,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix) > die(_("branch name required")); > return delete_branches(argc, argv, delete > 1, filter.kind, quiet); > } else if (show_current) { > - print_current_branch_name(); > + if (!filter.detached) > + puts(head); Ah, I wondered why we do not have to skip-prefix, but it is already done for us when we validated that an attached HEAD points at a local branch. Good. > return 0; > } else if (list) { > /* git branch --local also shows HEAD when it is detached */
I did something that resulted in the mailing list not being cc'd. Apologies to Junio and Daniels for the double send. :( On Thu, Nov 08, 2018 at 10:11:02AM +0900, Junio C Hamano wrote: > I'd prefer to see scriptors avoid using "git branch", too. > > Unlike end-user facing documentation where we promise "we do X and > will continue to do so because of Y" to the readers, the log message > is primarily for recording the original motivation of the change, so > that we can later learn "we did X back then because we thought Y". > When we want to revise X, we revisit if the reason Y is still valid. > > So in that sense, the door to "break" the scriptability is still > open. > Over at #git, commit messages are sometimes consulted to disambiguate or clarify certain details. Often the documentation is correct but people dispute over interpretations. If someone came asking if `git branch` is parsable, I would advise against and direct them to the plumbing or format alternative. But if someone came over with a link to this commit asking the same question, I suspect the answer would be: it's probably safe to parse the output of this specific option because the commit says so. Thanks for clarifying this is wrong. > > > > static const char *head; > > static struct object_id head_oid; > > +static int head_flags = 0; > > You've eliminated the "now unnecessary" helper and do everything > inside cmd_branch(), so perhaps this can be made function local, no? > I was not sure if these 3 lines were global intentionally or if it was just an artifact from the past. Since it looks like the latter, I'll make them local. -- Rafael Ascensão
diff --git a/builtin/branch.c b/builtin/branch.c index 46f91dc06d..1c51d0a8ca 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -38,6 +38,7 @@ static const char * const builtin_branch_usage[] = { static const char *head; static struct object_id head_oid; +static int head_flags = 0; static int branch_use_color = -1; static char branch_colors[][COLOR_MAXLEN] = { @@ -443,21 +444,6 @@ 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, &flags); - const char *shortname; - if (!refname) - die(_("could not resolve HEAD")); - else if (!(flags & REF_ISSYMREF)) - return; - else if (skip_prefix(refname, "refs/heads/", &shortname)) - 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); @@ -668,10 +654,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix) track = git_branch_track; - head = resolve_refdup("HEAD", 0, &head_oid, NULL); + head = resolve_refdup("HEAD", 0, &head_oid, &head_flags); if (!head) die(_("Failed to resolve HEAD as a valid ref.")); - if (!strcmp(head, "HEAD")) + if (!(head_flags & REF_ISSYMREF)) filter.detached = 1; else if (!skip_prefix(head, "refs/heads/", &head)) die(_("HEAD not found below refs/heads!")); @@ -716,7 +702,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix) die(_("branch name required")); return delete_branches(argc, argv, delete > 1, filter.kind, quiet); } else if (show_current) { - print_current_branch_name(); + if (!filter.detached) + puts(head); return 0; } else if (list) { /* git branch --local also shows HEAD when it is detached */
print_current_branch_name() tries to resolve HEAD and die() when it doesn't resolve it successfully. But the conditions being tested are always unreachable because early in branch:cmd_branch() the same logic is performed. Eliminate the duplicate and unreachable code, and update the current logic to the more reliable check for the detached head. Signed-off-by: Rafael Ascensão <rafa.almas@gmail.com> --- This patch is meant to be either applied or squashed on top of the current series. I am basing the claims of it being more reliable of what Junio suggested on a previous iteration of this series: https://public-inbox.org/git/xmqq4ldtgehs.fsf@gitster-ct.c.googlers.com/ But the main goal of this patch is to just bring some attention to this, as I mentioned it in a previous thread but it got lost. After asking on #git-devel, the suggestion was to send it as an incremental patch. So here it is. :) I still think the mention about scripting should be removed from the original commit message, leaving it open to being taught other tricks like --verbose that aren't necessarily script-friendly. Cheers builtin/branch.c | 23 +++++------------------ 1 file changed, 5 insertions(+), 18 deletions(-)