[PATCH] commit: introduce set_merge_remote_desc()

[PATCH] commit: introduce set_merge_remote_desc()

[René Scharfe]

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(-)

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

Re: [PATCH] commit: introduce set_merge_remote_desc()

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

> 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

Re: [PATCH] commit: introduce set_merge_remote_desc()

[René Scharfe]

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(-)

[PATCH v2 1/4] commit: use xstrdup() in get_merge_parent()

[René Scharfe]

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(-)

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

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

[René Scharfe]

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(-)

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

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

[René Scharfe]

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(-)

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

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

[René Scharfe]

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(-)

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

Re: [PATCH] commit: introduce set_merge_remote_desc()

[Jeff King]

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