Re: [PATCH] branch: segfault fixes and validation
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:56:13
Possibly related (same subject, not in this thread)
- 2016-06-15 · [PATCH] branch: segfault fixes and validation · Nguyễn Thái Ngọc Duy <hidden>
Nguyễn Thái Ngọc Duy [off-list ref] writes:
branch_get() can return NULL (so far on detached HEAD only)...
Do you anticipate any other cases where the API call should validly return NULL? I offhand do not, ...
quoted hunk
but some code paths in builtin/branch.c cannot deal with that and cause segfaults. Fix it. While at there, make sure to bail out when the user gives 2 or more arguments, but only the first one is processed. Reported-by: Per Cederqvist <redacted> Signed-off-by: Nguyễn Thái Ngọc Duy <redacted> --- builtin/branch.c | 20 ++++++++++++++++++++ t/t3200-branch.sh | 21 +++++++++++++++++++++ 2 files changed, 41 insertions(+)diff --git a/builtin/branch.c b/builtin/branch.c index 6371bf9..c1d688e 100644 --- a/builtin/branch.c +++ b/builtin/branch.c@@ -889,6 +889,13 @@ int cmd_branch(int argc, const char **argv, const char *prefix) } else if (new_upstream) { struct branch *branch = branch_get(argv[0]); + if (argc > 1) + die(_("too many branches to set new upstream")); + + if (!branch) + die(_("could not figure out the branch name from '%s'"), + argc == 1 ? argv[0] : "HEAD");
... and find this "could not figure out" very unfriendly to the user. Is it a bug in the implementation, silly Git failing to find what branch the user meant? What recourse does the user have at this point? Or is it a user error to ask Git to operate on the branch pointed at by HEAD, when HEAD does not refer to any branch? If that is the case, then the message should say that there is no current branch to operate on because the user is on a detached HEAD. That would point the user in the right direction to correct the user error, no? Of course, argv[0] may not be HEAD and in that case you may have to say "no such branch %s" % argv[0] or something. The point is that "could not figure out" feels a bit too lazy.
quoted hunk
@@ -901,6 +908,13 @@ int cmd_branch(int argc, const char **argv, const char *prefix) struct branch *branch = branch_get(argv[0]); struct strbuf buf = STRBUF_INIT; + if (argc > 1) + die(_("too many branches to unset upstream")); + + if (!branch) + die(_("could not figure out the branch name from '%s'"), + argc == 1 ? argv[0] : "HEAD");
Likewise.
quoted hunk
@@ -916,6 +930,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix) int branch_existed = 0, remote_tracking = 0; struct strbuf buf = STRBUF_INIT; + if (!strcmp(argv[0], "HEAD")) + die(_("it does not make sense to create 'HEAD' manually")); + + if (!branch) + die(_("could not figure out the branch name from '%s'"), argv[0]);
Likewise. Thanks.