commit: introduce set_merge_remote_desc()
PATCH 1/1 commit: introduce set_merge_remote_desc()
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.
--- commit.c | 18 +++++++++++------- commit.h | 2 ++ merge-recursive.c | 5 +---- 3 files changed, 14 insertions(+), 11 deletions(-)
commit.c +11 -7
@@ -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; +} +struct commit *get_merge_parent (const char *name) {struct object *obj;
@@ -1585,13 +1594,8 @@ struct commit *get_merge_parent(const char *name)return NULL ; obj =parse_object (oid.hash); commit = (struct commit *)peel_to_type (name,0 , obj,OBJ_COMMIT ); -if (commit && !commit->util) { -struct merge_remote_desc *desc; - desc =xmalloc (sizeof (*desc)); - desc->obj = obj; - desc->name =strdup (name); - commit->util = desc; - } +if (commit && !commit->util) +set_merge_remote_desc (commit, name, obj);return commit; }
commit.h +2 -0
@@ -365,6 +365,8 @@ struct merge_remote_desc {const char *name; };#define merge_remote_util (commit) ((struct merge_remote_desc *)((commit)->util)) +extern void set_merge_remote_desc (struct commit *commit, +const char *name,struct object *obj);/* * Given "name" from the command line to merge, find the commit object
merge-recursive.c +1 -4
@@ -73,12 +73,9 @@ static struct tree *shift_tree_object(struct tree *one, struct tree *two,static struct commit *make_virtual_commit (struct tree *tree,const char *comment) {struct commit *commit =alloc_commit_node (); -struct merge_remote_desc *desc =xmalloc (sizeof (*desc)); - desc->name = comment; - desc->obj = (struct object *)commit; +set_merge_remote_desc (commit, comment, (struct object *)commit); commit->tree = tree; - commit->util = desc; commit->object.parsed =1 ;return commit; }
Signature · 1 line
2.9.3
Quoted 10 lines · René Scharfe
Add a helper function for allocating, populating and attaching struct
merge_remote_descto 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 ofoid_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. :)
Quoted 6 lines
commit.c +0
-0@@ -1576,6 +1576,15 @@ int commit_tree_extended(const char *msg, size_t msg_len,return result;
commit.c int commit_tree_extended(const char *msg, size_t msg_len, · line 1586
Quoted 10 lines
commit.c +8
-0} +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
Am 13.08.2016 um 11:23 schrieb Jeff King:
Quoted 13 lines · René Scharfe
Quoted 11 lines
Add a helper function for allocating, populating and attaching struct
merge_remote_descto 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 ofoid_to_hex(), i.e. a static buffer.
Quoted 7 lines
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? ;-)
Quoted 22 lines
Quoted 6 lines
commit.c +0
-0@@ -1576,6 +1576,15 @@ int commit_tree_extended(const char *msg, size_t msg_len,return result;commit.c int commit_tree_extended(const char *msg, size_t msg_len, · line 1586
Quoted 10 lines
commit.c +8
-0} +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_ALLOCoverFLEX_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).
Quoted 5 lines
Using
FLEX_ALLOCsaves 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(-)
PATCH v2 1/4 commit: use xstrdup() in get_merge_parent()
Handle allocation errors for the name member just like we already do for the struct merge_remote_desc itself.
--- commit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
commit.c +1 -1
@@ -1589,7 +1589,7 @@ struct commit *get_merge_parent(const char *name)struct merge_remote_desc *desc; desc =xmalloc (sizeof (*desc)); desc->obj = obj; - desc->name =strdup (name); + desc->name =xstrdup (name); commit->util = desc; }return commit;
Signature · 1 line
2.9.3
PATCH v2 2/4 commit: factor out set_merge_remote_desc()
In reply to René ScharfeExport a helper function for allocating, populating and attaching a merge_remote_desc to a commit.
--- commit.c | 19 ++++++++++++------- commit.h | 2 ++ 2 files changed, 14 insertions(+), 7 deletions(-)
commit.c +12 -7
@@ -1576,6 +1576,16 @@ 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; + desc =xmalloc (sizeof (*desc)); + desc->obj = obj; + desc->name =xstrdup (name); + commit->util = desc; +} +struct commit *get_merge_parent (const char *name) {struct object *obj;
@@ -1585,13 +1595,8 @@ struct commit *get_merge_parent(const char *name)return NULL ; obj =parse_object (oid.hash); commit = (struct commit *)peel_to_type (name,0 , obj,OBJ_COMMIT ); -if (commit && !commit->util) { -struct merge_remote_desc *desc; - desc =xmalloc (sizeof (*desc)); - desc->obj = obj; - desc->name =xstrdup (name); - commit->util = desc; - } +if (commit && !commit->util) +set_merge_remote_desc (commit, name, obj);return commit; }
commit.h +2 -0
@@ -365,6 +365,8 @@ struct merge_remote_desc {const char *name; };#define merge_remote_util (commit) ((struct merge_remote_desc *)((commit)->util)) +extern void set_merge_remote_desc (struct commit *commit, +const char *name,struct object *obj);/* * Given "name" from the command line to merge, find the commit object
Signature · 1 line
2.9.3
PATCH v2 3/4 merge-recursive: fix verbose output for multiple base trees
In reply to René ScharfeOne of the indirect callers of make_virtual_commit() passes the result of oid_to_hex() as the name, i.e. a pointer to a static buffer. Since the function uses that string pointer directly in building a struct merge_remote_desc, multiple entries can end up sharing the same name inadvertently.
Fix that by calling set_merge_remote_desc(), which creates a copy of the string, instead of building the struct by hand.
--- merge-recursive.c | 5 +---- t/t3030-merge-recursive.sh | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 4 deletions(-)
merge-recursive.c +1 -4
@@ -73,12 +73,9 @@ static struct tree *shift_tree_object(struct tree *one, struct tree *two,static struct commit *make_virtual_commit (struct tree *tree,const char *comment) {struct commit *commit =alloc_commit_node (); -struct merge_remote_desc *desc =xmalloc (sizeof (*desc)); - desc->name = comment; - desc->obj = (struct object *)commit; +set_merge_remote_desc (commit, comment, (struct object *)commit); commit->tree = tree; - commit->util = desc; commit->object.parsed =1 ;return commit; }
t/t3030-merge-recursive.sh +18 -0
@@ -660,4 +660,22 @@ test_expect_success 'merging with triple rename across D/F conflict' ' git merge other ' +test_expect_success 'merge-recursive remembers the names of all base trees' ' + git reset --hard HEAD && + + # more trees than static slots used by oid_to_hex() + for commit in $c0 $c2 $c4 $c5 $c6 $c7 + do + git rev-parse "$commit^{tree}" + done >trees && + + # ignore the return code -- it only fails because the input is weird + test_must_fail git -c merge.verbosity=5 merge-recursive $(cat trees) -- $c1 $c3 >out && + + # merge-recursive prints in reverse order, but we do not care + sort <trees >expect && + sed -n "s/^virtual //p" out | sort >actual && + test_cmp expect actual +' + test_done
Signature · 1 line
2.9.3
PATCH v2 4/4 commit: use FLEX_ARRAY in struct merge_remote_desc
In reply to René ScharfeConvert the name member of struct merge_remote_desc to a FLEX_ARRAY and use FLEX_ALLOC_STR to build the struct. This halves the number of memory allocations, saves the storage for a pointer and avoids an indirection when reading the name.
--- commit.c | 3 +-- commit.h | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-)
commit.c +1 -2
@@ -1580,9 +1580,8 @@ void set_merge_remote_desc(struct commit *commit, const char *name, struct object *obj) { struct merge_remote_desc *desc; - desc =xmalloc (sizeof (*desc)); +FLEX_ALLOC_STR (desc, name, name); desc->obj = obj; - desc->name =xstrdup (name); commit->util = desc; }
commit.h +1 -1
@@ -362,7 +362,7 @@ extern void for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *struct merge_remote_desc {struct object *obj;/* the named object, could be a tag */ -const char *name; +char name[FLEX_ARRAY ]; };#define merge_remote_util (commit) ((struct merge_remote_desc *)((commit)->util)) extern void set_merge_remote_desc(struct commit *commit,
Signature · 1 line
2.9.3
Quoted 6 lines · René Scharfe
So let's turn this dish into a full menu:
commit: use
xstrdup()inget_merge_parent()
commit: factor outset_merge_remote_desc()
merge-recursive: fix verbose output for multiple base trees
commit: useFLEX_ARRAYin structmerge_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