describe: split into gather and format steps

Instead of printing out to the buffer inside the information-gathering
phase, write the data to a intermediate result structure.

This allows us to split the options into gathering options and
formatting options, simplifying the gathering code.

6 files changed, 242 insertions, 101 deletions

diff --git a/include/git2/describe.h b/include/git2/describe.h
index 0a845f6be..6007b004e 100644
--- a/include/git2/describe.h
+++ b/include/git2/describe.h
@@ -38,10 +38,8 @@ typedef struct git_describe_opts {
 	unsigned int version;
 
 	unsigned int max_candidates_tags; /** default: 10 */
-	unsigned int abbreviated_size;
 	unsigned int describe_strategy; /** default: GIT_DESCRIBE_DEFAULT */
 	const char *pattern;
-	int always_use_long_format;
 	int only_follow_first_parent;
 	int show_commit_oid_as_fallback;
 } git_describe_opts;
@@ -53,13 +51,34 @@ typedef struct git_describe_opts {
 #define GIT_DESCRIBE_OPTIONS_INIT { \
 	GIT_DESCRIBE_OPTIONS_VERSION, \
 	GIT_DESCRIBE_DEFAULT_MAX_CANDIDATES_TAGS, \
-	GIT_DESCRIBE_DEFAULT_ABBREVIATED_SIZE}
+}
+
+typedef struct {
+	unsigned int version;
+
+	unsigned int abbreviated_size;
+
+	int always_use_long_format;
+	char *dirty_suffix;
+} git_describe_format_options;
+
+#define GIT_DESCRIBE_FORMAT_OPTIONS_VERSION 1
+#define GIT_DESCRIBE_FORMAT_OPTIONS_INIT { \
+		GIT_DESCRIBE_FORMAT_OPTIONS_VERSION,   \
+		GIT_DESCRIBE_DEFAULT_ABBREVIATED_SIZE, \
+ }
+
+typedef struct git_describe_result git_describe_result;
 
 GIT_EXTERN(int) git_describe_commit(
-	git_buf *out,
+	git_describe_result **result,
 	git_object *committish,
 	git_describe_opts *opts);
 
+GIT_EXTERN(int) git_describe_format(git_buf *out, const git_describe_result *result, const git_describe_format_options *opts);
+
+GIT_EXTERN(void) git_describe_result_free(git_describe_result *result);
+
 /** @} */
 GIT_END_DECL
 
diff --git a/src/describe.c b/src/describe.c
index c7b45d46f..0c77f1537 100644
--- a/src/describe.c
+++ b/src/describe.c
@@ -162,13 +162,45 @@ cleanup:
 	return error;
 }
 
+typedef struct git_describe_result {
+	int dirty;
+	int exact_match;
+	int fallback_to_id;
+	git_oid commit_id;
+	git_repository *repo;
+	struct commit_name *name;
+	struct possible_tag *tag;
+} git_describe_result;
+
 struct get_name_data
 {
 	git_describe_opts *opts;
 	git_repository *repo;
 	git_oidmap *names;
+	git_describe_result *result;
 };
 
+static int commit_name_dup(struct commit_name **out, struct commit_name *in)
+{
+	struct commit_name *name;
+
+	name = git__malloc(sizeof(struct commit_name));
+	GITERR_CHECK_ALLOC(name);
+
+	memcpy(name, in,  sizeof(struct commit_name));
+	name->tag = NULL;
+	name->path = NULL;
+
+	if (in->tag && git_object_dup((git_object **) &name->tag, (git_object *) in->tag) < 0)
+		return -1;
+
+	name->path = git__strdup(in->path);
+	GITERR_CHECK_ALLOC(name->path);
+
+	*out = name;
+	return 0;
+}
+
 static int get_name(const char *refname, void *payload)
 {
 	struct get_name_data *data;
@@ -223,6 +255,23 @@ struct possible_tag {
 	unsigned flag_within;
 };
 
+static int possible_tag_dup(struct possible_tag **out, struct possible_tag *in)
+{
+	struct possible_tag *tag;
+
+	tag = git__malloc(sizeof(struct possible_tag));
+	GITERR_CHECK_ALLOC(tag);
+
+	memcpy(tag, in, sizeof(struct possible_tag));
+	tag->name = NULL;
+
+	if (commit_name_dup(&tag->name, in->name) < 0)
+		return -1;
+
+	*out = tag;
+	return 0;
+}
+
 static int compare_pt(const void *a_, const void *b_)
 {
 	struct possible_tag *a = (struct possible_tag *)a_;
@@ -352,15 +401,12 @@ static int describe_not_found(const git_oid *oid, const char *message_format) {
 }
 
 static int describe(
-	git_buf *out,
 	struct get_name_data *data,
-	git_commit *commit,
-	const char *dirty_suffix)
+	git_commit *commit)
 {
 	struct commit_name *n;
 	struct possible_tag *best;
 	bool all, tags;
-	git_buf buf = GIT_BUF_INIT;
 	git_revwalk *walk = NULL;
 	git_pqueue list;
 	git_commit_list_node *cmit, *gave_up_on = NULL;
@@ -379,28 +425,18 @@ static int describe(
 	all = data->opts->describe_strategy == GIT_DESCRIBE_ALL;
 	tags = data->opts->describe_strategy == GIT_DESCRIBE_TAGS;
 
+	git_oid_cpy(&data->result->commit_id, git_commit_id(commit));
+
 	n = find_commit_name(data->names, git_commit_id(commit));
 	if (n && (tags || all || n->prio == 2)) {
 		/*
 		 * Exact match to an existing ref.
 		 */
-		if ((error = display_name(&buf, data->repo, n)) < 0)
+		data->result->exact_match = 1;
+		if ((error = commit_name_dup(&data->result->name, n)) < 0)
 			goto cleanup;
 
-		if (data->opts->always_use_long_format) {
-			if ((error = show_suffix(&buf, 0,
-				n->tag ? git_tag_target_id(n->tag) : git_commit_id(commit),
-				data->opts->abbreviated_size)) < 0)
-					goto cleanup;
-		}
-
-		if (dirty_suffix)
-			git_buf_printf(&buf, "%s", dirty_suffix);
-
-		if (git_buf_oom(&buf))
-			return -1;
-
-		goto found;
+		goto cleanup;
 	}
 
 	if (!data->opts->max_candidates_tags) {
@@ -492,23 +528,10 @@ static int describe(
 
 	if (!match_cnt) {
 		if (data->opts->show_commit_oid_as_fallback) {
-			char hex_oid[GIT_OID_HEXSZ];
-			int size;
-
-			if ((error = find_unique_abbrev_size(
-				&size, &cmit->oid, data->opts->abbreviated_size)) < 0)
-					goto cleanup;
-
-			git_oid_fmt(hex_oid, &cmit->oid);
-			git_buf_put(&buf, hex_oid, size);
+			data->result->fallback_to_id = 1;
+			git_oid_cpy(&data->result->commit_id, &cmit->oid);
 
-			if (dirty_suffix)
-				git_buf_printf(&buf, "%s", dirty_suffix);
-
-			if (git_buf_oom(&buf))
-				return -1;
-
-			goto found;
+			goto cleanup;
 		}
 		if (unannotated_cnt) {
 			error = describe_not_found(git_commit_id(commit), 
@@ -538,7 +561,10 @@ static int describe(
 	if ((error = finish_depth_computation(
 		&list, walk, best)) < 0)
 		goto cleanup;
+
 	seen_commits += error;
+	if ((error = possible_tag_dup(&data->result->tag, best)) < 0)
+		goto cleanup;
 
 	/*
 	{
@@ -568,25 +594,7 @@ static int describe(
 	}
 	*/
 
-	if ((error = display_name(&buf, data->repo, best->name)) < 0)
-		goto cleanup;
-
-	if (data->opts->abbreviated_size) {
-		if ((error = show_suffix(&buf, best->depth,
-			&cmit->oid, data->opts->abbreviated_size)) < 0)
-				goto cleanup;
-	}
-
-	if (dirty_suffix)
-		git_buf_printf(&buf, "%s", dirty_suffix);
-
-	if (git_buf_oom(&buf))
-		return -1;
-
-found:
-	out->ptr = buf.ptr;
-	out->asize = buf.asize;
-	out->size = buf.size;
+	git_oid_cpy(&data->result->commit_id, &cmit->oid);
 
 cleanup:
 	{
@@ -614,20 +622,13 @@ static int normalize_options(
 	if (dst->max_candidates_tags > GIT_DESCRIBE_DEFAULT_MAX_CANDIDATES_TAGS)
 		dst->max_candidates_tags = GIT_DESCRIBE_DEFAULT_MAX_CANDIDATES_TAGS;
 
-	if (dst->always_use_long_format && dst->abbreviated_size == 0) {
-		giterr_set(GITERR_DESCRIBE, "Cannot describe - "
-			"'always_use_long_format' is incompatible with a zero"
-			"'abbreviated_size'");
-		return -1;
-	}
-
 	return 0;
 }
 
 /** TODO: Add git_object_describe_workdir(git_buf *, const char *dirty_suffix, git_describe_opts *); */
 
 int git_describe_commit(
-	git_buf *out,
+	git_describe_result **result,
 	git_object *committish,
 	git_describe_opts *opts)
 {
@@ -635,10 +636,13 @@ int git_describe_commit(
 	struct commit_name *name;
 	git_commit *commit;
 	int error = -1;
-	const char *dirty_suffix = NULL;
 	git_describe_opts normOptions;
 
-	assert(out && committish);
+	assert(committish);
+
+	data.result = git__calloc(1, sizeof(git_describe_result));
+	GITERR_CHECK_ALLOC(data.result);
+	data.result->repo = git_object_owner(committish);
 
 	data.opts = opts;
 	data.repo = git_object_owner(committish);
@@ -673,10 +677,12 @@ int git_describe_commit(
 		goto cleanup;
 	}
 
-	if ((error = describe(out, &data, commit, dirty_suffix)) < 0)
+	if ((error = describe(&data, commit)) < 0)
 		goto cleanup;
 
 cleanup:
+	git_commit_free(commit);
+
 	git_oidmap_foreach_value(data.names, name, {
 		git_tag_free(name->tag);
 		git__free(name->path);
@@ -684,7 +690,108 @@ cleanup:
 	});
 
 	git_oidmap_free(data.names);
-	git_commit_free(commit);
+
+	if (error < 0)
+		git_describe_result_free(data.result);
+	else
+		*result = data.result;
 
 	return error;
 }
+
+int git_describe_format(git_buf *out, const git_describe_result *result, const git_describe_format_options *opts)
+{
+	int error;
+	git_repository *repo;
+	struct commit_name *name;
+
+	assert(out && result);
+
+	GITERR_CHECK_VERSION(opts, GIT_DESCRIBE_FORMAT_OPTIONS_VERSION, "git_describe_format_options");
+	git_buf_sanitize(out);
+
+
+	if (opts->always_use_long_format && opts->abbreviated_size == 0) {
+		giterr_set(GITERR_DESCRIBE, "Cannot describe - "
+			"'always_use_long_format' is incompatible with a zero"
+			"'abbreviated_size'");
+		return -1;
+	}
+
+
+	repo = result->repo;
+
+	/* If we did find an exact match, then it's the easier method */
+	if (result->exact_match) {
+		name = result->name;
+		if ((error = display_name(out, repo, name)) < 0)
+			return error;
+
+		if (opts->always_use_long_format) {
+			const git_oid *id = name->tag ? git_tag_target_id(name->tag) : &result->commit_id;
+			if ((error = show_suffix(out, 0, id, opts->abbreviated_size)) < 0)
+				return error;
+		}
+
+		if (result->dirty && opts->dirty_suffix)
+			git_buf_puts(out, opts->dirty_suffix);
+
+		return git_buf_oom(out) ? -1 : 0;
+	}
+
+	/* If we didn't find *any* tags, we fall back to the commit's id */
+	if (result->fallback_to_id) {
+		char hex_oid[GIT_OID_HEXSZ + 1] = {0};
+		int size;
+		if ((error = find_unique_abbrev_size(
+			     &size, &result->commit_id, opts->abbreviated_size)) < 0)
+			return -1;
+
+		git_oid_fmt(hex_oid, &result->commit_id);
+		git_buf_put(out, hex_oid, size);
+
+		if (opts->dirty_suffix)
+			git_buf_puts(out, opts->dirty_suffix);
+
+		return git_buf_oom(out) ? -1 : 0;
+	}
+
+	/* Lastly, if we found a matching tag, we show that */
+	name = result->tag->name;
+
+	if ((error = display_name(out, repo, name)) < 0)
+		return error;
+
+	if (opts->abbreviated_size) {
+		if ((error = show_suffix(out, result->tag->depth,
+			&result->commit_id, opts->abbreviated_size)) < 0)
+			return -1;
+	}
+
+	if (opts->dirty_suffix)
+		git_buf_puts(out, opts->dirty_suffix);
+
+
+	return git_buf_oom(out) ? -1 : 0;
+}
+
+void git_describe_result_free(git_describe_result *result)
+{
+	if (result == NULL)
+		return;
+
+	if (result->name) {
+		git_tag_free(result->name->tag);
+		git__free(result->name->path);
+		git__free(result->name);
+	}
+
+	if (result->tag) {
+		git_tag_free(result->tag->name->tag);
+		git__free(result->tag->name->path);
+		git__free(result->tag->name);
+		git__free(result->tag);
+	}
+
+	git__free(result);
+}
diff --git a/tests/describe/describe.c b/tests/describe/describe.c
index a7f3c84df..e2dda96cd 100644
--- a/tests/describe/describe.c
+++ b/tests/describe/describe.c
@@ -5,14 +5,15 @@ void test_describe_describe__can_describe_against_a_bare_repo(void)
 {
 	git_repository *repo;
 	git_describe_opts opts = GIT_DESCRIBE_OPTIONS_INIT;
+	git_describe_format_options fmt_opts = GIT_DESCRIBE_FORMAT_OPTIONS_INIT;
 
 	cl_git_pass(git_repository_open(&repo, cl_fixture("testrepo.git")));
 
-	assert_describe("hard_tag", "HEAD", repo, &opts, false);
+	assert_describe("hard_tag", "HEAD", repo, &opts, &fmt_opts, false);
 
 	opts.show_commit_oid_as_fallback = 1;
 
-	assert_describe("be3563a", "HEAD^", repo, &opts, true);
+	assert_describe("be3563a", "HEAD^", repo, &opts, &fmt_opts, true);
 
 	git_repository_free(repo);
 }
@@ -33,14 +34,16 @@ void test_describe_describe__cannot_describe_against_a_repo_with_no_ref(void)
 	git_describe_opts opts = GIT_DESCRIBE_OPTIONS_INIT;
 	git_buf buf = GIT_BUF_INIT;
 	git_object *object;
+	git_describe_result *result = NULL;
 
 	repo = cl_git_sandbox_init("testrepo.git");
 	cl_git_pass(git_revparse_single(&object, repo, "HEAD"));
 
 	cl_git_pass(git_reference_foreach(repo, delete_cb, NULL));
 
-	cl_git_fail(git_describe_commit(&buf, object, &opts));
+	cl_git_fail(git_describe_commit(&result, object, &opts));
 
+	git_describe_result_free(result);
 	git_object_free(object);
 	git_buf_free(&buf);
 	cl_git_sandbox_cleanup();
diff --git a/tests/describe/describe_helpers.c b/tests/describe/describe_helpers.c
index 7235d320f..d975ddf4b 100644
--- a/tests/describe/describe_helpers.c
+++ b/tests/describe/describe_helpers.c
@@ -5,20 +5,24 @@ void assert_describe(
 	const char *revparse_spec,
 	git_repository *repo,
 	git_describe_opts *opts,
+	git_describe_format_options *fmt_opts,
 	bool is_prefix_match)
 {
 	git_object *object;
-	git_buf label;
+	git_buf label = GIT_BUF_INIT;
+	git_describe_result *result;
 
 	cl_git_pass(git_revparse_single(&object, repo, revparse_spec));
 
-	cl_git_pass(git_describe_commit(&label, object, opts));
+	cl_git_pass(git_describe_commit(&result, object, opts));
+	cl_git_pass(git_describe_format(&label, result, fmt_opts));
 
 	if (is_prefix_match)
 		cl_assert_equal_i(0, git__prefixcmp(git_buf_cstr(&label), expected_output));
 	else
 		cl_assert_equal_s(expected_output, label);
 
+	git_describe_result_free(result);
 	git_object_free(object);
 	git_buf_free(&label);
 }
diff --git a/tests/describe/describe_helpers.h b/tests/describe/describe_helpers.h
index 0f107f5a7..a666b46cf 100644
--- a/tests/describe/describe_helpers.h
+++ b/tests/describe/describe_helpers.h
@@ -6,4 +6,5 @@ extern void assert_describe(
 	const char *revparse_spec,
 	git_repository *repo,
 	git_describe_opts *opts,
+	git_describe_format_options *fmt_opts,
 	bool is_prefix_match);
diff --git a/tests/describe/t6120.c b/tests/describe/t6120.c
index d589e82a6..39489f3c5 100644
--- a/tests/describe/t6120.c
+++ b/tests/describe/t6120.c
@@ -19,56 +19,62 @@ void test_describe_t6120__cleanup(void)
 void test_describe_t6120__default(void)
 {
 	git_describe_opts opts = GIT_DESCRIBE_OPTIONS_INIT;
-
-	assert_describe("A-", "HEAD", repo, &opts, true);
-	assert_describe("A-", "HEAD^", repo, &opts, true);
-	assert_describe("R-", "HEAD^^", repo, &opts, true);
-	assert_describe("A-", "HEAD^^2", repo, &opts, true);
-	assert_describe("B", "HEAD^^2^", repo, &opts, false);
-	assert_describe("R-", "HEAD^^^", repo, &opts, true);
+	git_describe_format_options fmt_opts = GIT_DESCRIBE_FORMAT_OPTIONS_INIT;
+
+	assert_describe("A-", "HEAD", repo, &opts, &fmt_opts, true);
+	assert_describe("A-", "HEAD^", repo, &opts, &fmt_opts, true);
+	assert_describe("R-", "HEAD^^", repo, &opts, &fmt_opts, true);
+	assert_describe("A-", "HEAD^^2", repo, &opts, &fmt_opts, true);
+	assert_describe("B", "HEAD^^2^", repo, &opts, &fmt_opts, false);
+	assert_describe("R-", "HEAD^^^", repo, &opts, &fmt_opts, true);
 }
 
 void test_describe_t6120__tags(void)
 {
 	git_describe_opts opts = GIT_DESCRIBE_OPTIONS_INIT;
+	git_describe_format_options fmt_opts = GIT_DESCRIBE_FORMAT_OPTIONS_INIT;
 	opts.describe_strategy = GIT_DESCRIBE_TAGS;
 
-	assert_describe("c-", "HEAD", repo, &opts, true);
-	assert_describe("c-", "HEAD^", repo, &opts, true);
-	assert_describe("e-", "HEAD^^", repo, &opts, true);
-	assert_describe("c-", "HEAD^^2", repo, &opts, true);
-	assert_describe("B", "HEAD^^2^", repo, &opts, false);
-	assert_describe("e", "HEAD^^^", repo, &opts, false);
+	assert_describe("c-", "HEAD", repo, &opts, &fmt_opts, true);
+	assert_describe("c-", "HEAD^", repo, &opts, &fmt_opts, true);
+	assert_describe("e-", "HEAD^^", repo, &opts, &fmt_opts, true);
+	assert_describe("c-", "HEAD^^2", repo, &opts, &fmt_opts, true);
+	assert_describe("B", "HEAD^^2^", repo, &opts, &fmt_opts, false);
+	assert_describe("e", "HEAD^^^", repo, &opts, &fmt_opts, false);
 }
 
 void test_describe_t6120__all(void)
 {
 	git_describe_opts opts = GIT_DESCRIBE_OPTIONS_INIT;
+	git_describe_format_options fmt_opts = GIT_DESCRIBE_FORMAT_OPTIONS_INIT;
 	opts.describe_strategy = GIT_DESCRIBE_ALL;
 
-	assert_describe("heads/master", "HEAD", repo, &opts, false);
-	assert_describe("tags/c-", "HEAD^", repo, &opts, true);
-	assert_describe("tags/e", "HEAD^^^", repo, &opts, false);
+	assert_describe("heads/master", "HEAD", repo, &opts, &fmt_opts, false);
+	assert_describe("tags/c-", "HEAD^", repo, &opts, &fmt_opts, true);
+	assert_describe("tags/e", "HEAD^^^", repo, &opts, &fmt_opts, false);
 }
 
 void test_describe_t6120__longformat(void)
 {
 	git_describe_opts opts = GIT_DESCRIBE_OPTIONS_INIT;
-	opts.always_use_long_format = 1;
+	git_describe_format_options fmt_opts = GIT_DESCRIBE_FORMAT_OPTIONS_INIT;
+
+	fmt_opts.always_use_long_format = 1;
 
-	assert_describe("B-0-", "HEAD^^2^", repo, &opts, true);
-	assert_describe("A-3-", "HEAD^^2", repo, &opts, true);
+	assert_describe("B-0-", "HEAD^^2^", repo, &opts, &fmt_opts, true);
+	assert_describe("A-3-", "HEAD^^2", repo, &opts, &fmt_opts, true);
 }
 
 void test_describe_t6120__firstparent(void)
 {
 	git_describe_opts opts = GIT_DESCRIBE_OPTIONS_INIT;
+	git_describe_format_options fmt_opts = GIT_DESCRIBE_FORMAT_OPTIONS_INIT;
 	opts.describe_strategy = GIT_DESCRIBE_TAGS;
 
-	assert_describe("c-7-", "HEAD", repo, &opts, true);
+	assert_describe("c-7-", "HEAD", repo, &opts, &fmt_opts, true);
 
 	opts.only_follow_first_parent = 1;
-	assert_describe("e-3-", "HEAD", repo, &opts, true);
+	assert_describe("e-3-", "HEAD", repo, &opts, &fmt_opts, true);
 }
 
 static void commit_and_tag(
@@ -100,6 +106,7 @@ static void commit_and_tag(
 void test_describe_t6120__pattern(void)
 {
 	git_describe_opts opts = GIT_DESCRIBE_OPTIONS_INIT;
+	git_describe_format_options fmt_opts = GIT_DESCRIBE_FORMAT_OPTIONS_INIT;
 	git_oid tag_id;
 	git_object *head;
 	git_signature *tagger;
@@ -121,15 +128,15 @@ void test_describe_t6120__pattern(void)
 
 	/* Exercize */
 	opts.pattern = "test-*";
-	assert_describe("test-annotated-", "HEAD", repo, &opts, true);
+	assert_describe("test-annotated-", "HEAD", repo, &opts, &fmt_opts, true);
 
 	opts.describe_strategy = GIT_DESCRIBE_TAGS;
 	opts.pattern = "test1-*";
-	assert_describe("test1-lightweight-", "HEAD", repo, &opts, true);
+	assert_describe("test1-lightweight-", "HEAD", repo, &opts, &fmt_opts, true);
 
 	opts.pattern = "test2-*";
-	assert_describe("test2-lightweight-", "HEAD", repo, &opts, true);
+	assert_describe("test2-lightweight-", "HEAD", repo, &opts, &fmt_opts, true);
 
-	opts.always_use_long_format = 1;
-	assert_describe("test2-lightweight-", "HEAD^", repo, &opts, true);
+	fmt_opts.always_use_long_format = 1;
+	assert_describe("test2-lightweight-", "HEAD^", repo, &opts, &fmt_opts, true);
 }