Re: [PATCH v2 1/4] wt-status: split wt_status_state parsing function out
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:56:19
Nguyễn Thái Ngọc Duy [off-list ref] writes:
quoted hunk
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted> --- wt-status.c | 52 +++++++++++++++++++++++++++++++--------------------- wt-status.h | 5 +++-- 2 files changed, 34 insertions(+), 23 deletions(-)diff --git a/wt-status.c b/wt-status.c index ef405d0..183aafe 100644 --- a/wt-status.c +++ b/wt-status.c@@ -970,7 +970,7 @@ static void show_bisect_in_progress(struct wt_status *s, * Extract branch information from rebase/bisect */ static void read_and_strip_branch(struct strbuf *sb, - const char **branch, + char **branch, const char *path) { unsigned char sha1[20];@@ -994,52 +994,62 @@ static void read_and_strip_branch(struct strbuf *sb, strbuf_addstr(sb, abbrev); *branch = sb->buf; } else if (!strcmp(sb->buf, "detached HEAD")) /* rebase */ - ; + *branch = NULL; else /* bisect */ *branch = sb->buf; + if (*branch) + *branch = xstrdup(*branch); }
The reason why the original print_state() kept two strbufs in it was because its use of the return value (in *branch) from this function was private and it did not want to have to strdup anything. With this change, I suspect that it is much saner to make this function *not* take any external strbuf as input, because you are always returning a piece of memory that belongs to the caller, or a NULL. In other words, with the new world order, wouldn't a saner function signature be: static const char *read_and_strip_branch(const char **path); after this patch? Also I notice that the error-return cases of this function may be wrong even before your patch. Shouldn't it be doing *branch = NULL (and 'return NULL' after the suggested change)?