threads / patch / 43803

patchcommit: introduce set_merge_remote_desc()

Subject: [PATCH] commit: introduce set_merge_remote_desc()

## tl;dr

8 messages between Aug 13, 2016 and Aug 13, 2016. Diffs are folded; open one to read it.

replies: 7people: 2as markdown or json

René Scharfe· Aug 13, 2016, 09:14 UTC · lore

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.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 commit.c          | 18 +++++++++++-------
 commit.h          |  2 ++
 merge-recursive.c |  5 +----
 3 files changed, 14 insertions(+), 11 deletions(-)
Show changes to 3 files +14 −11

commit.c, commit.h, merge-recursive.c

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;
+}
+
 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;
 }
 
diff --git a/commit.h b/commit.h
index 23ae0c1..84bb507 100644
--- a/commit.h
+++ b/commit.h
@@ -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
diff --git a/merge-recursive.c b/merge-recursive.c
index e5243c2..e349126 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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;
 }

-- 
2.9.3
Jeff King· Aug 13, 2016, 09:23 UTC · re: René Scharfe · lore

Re: [PATCH] commit: introduce set_merge_remote_desc()

On Sat, Aug 13, 2016 at 11:14:31AM +0200, René Scharfe wrote:
Show 10 quoted lines
> 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. :)

Show 16 quoted lines
> 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
René Scharfe· Aug 13, 2016, 12:07 UTC · re: Jeff King · lore

Re: [PATCH] commit: introduce set_merge_remote_desc()

Am 13.08.2016 um 11:23 schrieb Jeff King:
Show 20 quoted lines
> 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? ;-)
Show 27 quoted lines
>> 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(-)
René Scharfe· Aug 13, 2016, 12:09 UTC · re: René Scharfe · lore

[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.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 commit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to commit.c +1 −1
diff --git a/commit.c b/commit.c
index 71a360d..ccd232a 100644
--- a/commit.c
+++ b/commit.c
@@ -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;

-- 
2.9.3
René Scharfe· Aug 13, 2016, 12:11 UTC · re: René Scharfe · lore

[PATCH v2 2/4] commit: factor out set_merge_remote_desc()

Export a helper function for allocating, populating and attaching a merge_remote_desc to a commit.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 commit.c | 19 ++++++++++++-------
 commit.h |  2 ++
 2 files changed, 14 insertions(+), 7 deletions(-)
Show changes to 2 files +14 −7

commit.c, commit.h

diff --git a/commit.c b/commit.c
index ccd232a..8bad713 100644
--- a/commit.c
+++ b/commit.c
@@ -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;
 }
 
diff --git a/commit.h b/commit.h
index 23ae0c1..84bb507 100644
--- a/commit.h
+++ b/commit.h
@@ -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

-- 
2.9.3
René Scharfe· Aug 13, 2016, 12:16 UTC · re: René Scharfe · lore

[PATCH v2 3/4] merge-recursive: fix verbose output for multiple base trees

One 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.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 merge-recursive.c          |  5 +----
 t/t3030-merge-recursive.sh | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+), 4 deletions(-)
Show changes to 2 files +19 −4

merge-recursive.c, t/t3030-merge-recursive.sh

diff --git a/merge-recursive.c b/merge-recursive.c
index e5243c2..e349126 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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;
 }
diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index f7b0e59..470f334 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -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

-- 
2.9.3
René Scharfe· Aug 13, 2016, 12:21 UTC · re: René Scharfe · lore

[PATCH v2 4/4] commit: use FLEX_ARRAY in struct merge_remote_desc

Convert 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.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 commit.c | 3 +--
 commit.h | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)
Show changes to 2 files +2 −3

commit.c, commit.h

diff --git a/commit.c b/commit.c
index 8bad713..ba6dee3 100644
--- a/commit.c
+++ b/commit.c
@@ -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;
 }
 
diff --git a/commit.h b/commit.h
index 84bb507..32e1a11 100644
--- a/commit.h
+++ b/commit.h
@@ -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,

-- 
2.9.3
Jeff King· Aug 13, 2016, 12:24 UTC · re: René Scharfe · lore

Re: [PATCH] commit: introduce set_merge_remote_desc()

On Sat, Aug 13, 2016 at 02:07:42PM +0200, René Scharfe wrote:
Show 6 quoted lines
> 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

← back to recent threads