Re: [PATCH] commit: introduce set_merge_remote_desc()
On Sat, Aug 13, 2016 at 02:07:42PM +0200, René Scharfe wrote: > So let's turn this dish into a full menu: > > commit: use xstrdup() in get_merge_parent() > commit: factor out set_merge_remote_desc() > merge-recursive: fix verbose output for multiple base trees > commit: use FLEX_ARRAY in struct merge_remote_desc Very nice. Four patches seems like a lot for such a small fix, but each one is so trivial and easy to understand, I think it's worth it. They all look good to me. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] commit: introduce set_merge_remote_desc()
Am 13.08.2016 um 11:23 schrieb Jeff King:
On Sat, Aug 13, 2016 at 11:14:31AM +0200, René Scharfe wrote:
Add a helper function for allocating, populating and attaching struct
merge_remote_desc to a commit and use it consistently. It allocates the
necessary memory in a single block.
commit.c::get_merge_parent() forgot to check for memory allocation
failures of strdup(3).
merge-recursive.c::make_virtual_commit() didn't duplicate the string for
the name member, even though one of it's callers (indirectly through
get_ref()) may pass the result of oid_to_hex(), i.e. a static buffer.
It seems like you've buried the interesting part here. This isn't just
for cleanup, but a bugfix that the oids in our virtual commits might get
overwritten by subsequent actions.
It seems like that should be the subject and beginning of the commit
message. And then the fix is to allocate, and by the way we can do so
easily with this nice new helper. :)
Bugs are usually hidden, so why not hide fixes? ;-)
diff --git a/commit.c b/commit.c
index 71a360d..372b200 100644
--- a/commit.c
+++ b/commit.c
@@ -1576,6 +1576,15 @@ int commit_tree_extended(const char *msg, size_t msg_len,
return result;
}
+void set_merge_remote_desc(struct commit *commit,
+ const char *name, struct object *obj)
+{
+ struct merge_remote_desc *desc;
+ FLEXPTR_ALLOC_STR(desc, name, name);
+ desc->obj = obj;
+ commit->util = desc;
+}
I don't think there is any reason to prefer FLEXPTR_ALLOC over
FLEX_ALLOC, unless your struct interface is constrained by non-flex
users (that's why it is necessary for "struct exclude", for example,
which sometimes needs to carry its own string and sometimes not).
Using FLEX_ALLOC saves a few bytes per struct, and avoids an extra
pointer indirection when accessing the data.
Since it looks like you touch all of the allocations here, I think they
would both be happy as a regular flex array.
Good idea.
So let's turn this dish into a full menu:
commit: use xstrdup() in get_merge_parent()
commit: factor out set_merge_remote_desc()
merge-recursive: fix verbose output for multiple base trees
commit: use FLEX_ARRAY in struct merge_remote_desc
commit.c | 18 +++---
commit.h | 4 +++-
merge-recursive.c | 5 +
t/t3030-merge-recursive.sh | 18 ++
4 files changed, 33 insertions(+), 12 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] commit: introduce set_merge_remote_desc()
On Sat, Aug 13, 2016 at 11:14:31AM +0200, René Scharfe wrote:
> Add a helper function for allocating, populating and attaching struct
> merge_remote_desc to a commit and use it consistently. It allocates the
> necessary memory in a single block.
>
> commit.c::get_merge_parent() forgot to check for memory allocation
> failures of strdup(3).
>
> merge-recursive.c::make_virtual_commit() didn't duplicate the string for
> the name member, even though one of it's callers (indirectly through
> get_ref()) may pass the result of oid_to_hex(), i.e. a static buffer.
It seems like you've buried the interesting part here. This isn't just
for cleanup, but a bugfix that the oids in our virtual commits might get
overwritten by subsequent actions.
It seems like that should be the subject and beginning of the commit
message. And then the fix is to allocate, and by the way we can do so
easily with this nice new helper. :)
> diff --git a/commit.c b/commit.c
> index 71a360d..372b200 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -1576,6 +1576,15 @@ int commit_tree_extended(const char *msg, size_t
> msg_len,
> return result;
> }
>
> +void set_merge_remote_desc(struct commit *commit,
> +const char *name, struct object *obj)
> +{
> + struct merge_remote_desc *desc;
> + FLEXPTR_ALLOC_STR(desc, name, name);
> + desc->obj = obj;
> + commit->util = desc;
> +}
I don't think there is any reason to prefer FLEXPTR_ALLOC over
FLEX_ALLOC, unless your struct interface is constrained by non-flex
users (that's why it is necessary for "struct exclude", for example,
which sometimes needs to carry its own string and sometimes not).
Using FLEX_ALLOC saves a few bytes per struct, and avoids an extra
pointer indirection when accessing the data.
Since it looks like you touch all of the allocations here, I think they
would both be happy as a regular flex array.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
