Jeff King <peff@xxxxxxxx> writes:
> This converts two lines into one at each caller. But more
> importantly, it abstracts the concept of freeing the buffer,
> which will make it easier to change later.
>
> Note that we also need to provide a "detach" mechanism for
> the weird case in fsck which passes the buffer back to be
> freed.
I find that last sentence a bit of white lie ;-).
The sole caller of "detach" is in index-pack, and discards the
return value, which is not wrong per-se because it still has the
pointer to the piece of memory it fed to parse_object_buffer(),
knows and/or assumes that the return value must be the same as the
one it already has, and it handles the freeing of that memory
without relying on the object layer at all.
But that is an even more weird special case than the white-lie
version. As an API, "detach" that returns the buffer to be freed
looks much less weird than what is really going on in the current
caller.
I however have to wonder if
if (detach_commit_buffer(commit) != data)
die("BUG");
might want to be there.
> Signed-off-by: Jeff King <peff@xxxxxxxx>
> ---
> builtin/fsck.c | 3 +--
> builtin/index-pack.c | 2 +-
> builtin/log.c | 6 ++----
> builtin/rev-list.c | 3 +--
> commit.c | 13 +++++++++++++
> commit.h | 11 +++++++++++
> 6 files changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index fc150c8..8aadca1 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -310,8 +310,7 @@ static int fsck_obj(struct object *obj)
> if (obj->type == OBJ_COMMIT) {
> struct commit *commit = (struct commit *) obj;
>
> - free(commit->buffer);
> - commit->buffer = NULL;
> + free_commit_buffer(commit);
>
> if (!commit->parents && show_root)
> printf("root %s\n", sha1_to_hex(commit->object.sha1));
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index 18f57de..995df39 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -786,7 +786,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,
> }
> if (obj->type == OBJ_COMMIT) {
> struct commit *commit = (struct commit *) obj;
> - commit->buffer = NULL;
> + detach_commit_buffer(commit);
> }
> obj->flags |= FLAG_CHECKED;
> }
> diff --git a/builtin/log.c b/builtin/log.c
> index 39e8836..226f8f2 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -349,8 +349,7 @@ static int cmd_log_walk(struct rev_info *rev)
> rev->max_count++;
> if (!rev->reflog_info) {
> /* we allow cycles in reflog ancestry */
> - free(commit->buffer);
> - commit->buffer = NULL;
> + free_commit_buffer(commit);
> }
> free_commit_list(commit->parents);
> commit->parents = NULL;
> @@ -1508,8 +1507,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> reopen_stdout(rev.numbered_files ? NULL : commit, NULL, &rev, quiet))
> die(_("Failed to create output files"));
> shown = log_tree_commit(&rev, commit);
> - free(commit->buffer);
> - commit->buffer = NULL;
> + free_commit_buffer(commit);
>
> /* We put one extra blank line between formatted
> * patches and this flag is used by log-tree code
> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index 9f92905..e012ebe 100644
> --- a/builtin/rev-list.c
> +++ b/builtin/rev-list.c
> @@ -173,8 +173,7 @@ static void finish_commit(struct commit *commit, void *data)
> free_commit_list(commit->parents);
> commit->parents = NULL;
> }
> - free(commit->buffer);
> - commit->buffer = NULL;
> + free_commit_buffer(commit);
> }
>
> static void finish_object(struct object *obj,
> diff --git a/commit.c b/commit.c
> index fbdc480..11a05c1 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -245,6 +245,19 @@ int unregister_shallow(const unsigned char *sha1)
> return 0;
> }
>
> +void free_commit_buffer(struct commit *commit)
> +{
> + free(commit->buffer);
> + commit->buffer = NULL;
> +}
> +
> +const void *detach_commit_buffer(struct commit *commit)
> +{
> + void *ret = commit->buffer;
> + commit->buffer = NULL;
> + return ret;
> +}
> +
> int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long size)
> {
> const char *tail = buffer;
> diff --git a/commit.h b/commit.h
> index 4df48cb..d72ed43 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -51,6 +51,17 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s
> int parse_commit(struct commit *item);
> void parse_commit_or_die(struct commit *item);
>
> +/*
> + * Free any cached object buffer associated with the commit.
> + */
> +void free_commit_buffer(struct commit *);
> +
> +/*
> + * Disassociate any cached object buffer from the commit, but do not free it.
> + * The buffer (or NULL, if none) is returned.
> + */
> +const void *detach_commit_buffer(struct commit *);
> +
> /* Find beginning and length of commit subject. */
> int find_commit_subject(const char *commit_buffer, const char **subject);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html