threads / patch / 25968

patchcommit: Add commit_list prefix to reduce_heads function.

Subject: [PATCH] commit: Add commit_list prefix to reduce_heads function.

## tl;dr

13 messages between Dec 5, 2010 and Dec 6, 2010. Diffs are folded; open one to read it.

replies: 12people: 3as markdown or json

Thiago Farina· Dec 5, 2010, 01:59 UTC · lore
Signed-off-by: Thiago Farina <tfransosi@gmail.com>
---
 builtin/commit.c     |    2 +-
 builtin/merge-base.c |    2 +-
 builtin/merge.c      |    2 +-
 commit.c             |    2 +-
 commit.h             |    2 +-
 revision.c           |    2 +-
 6 files changed, 6 insertions(+), 6 deletions(-)
Show changes to 6 files +6 −6

builtin/commit.c, builtin/merge-base.c, builtin/merge.c, commit.c, commit.h, revision.c

diff --git a/builtin/commit.c b/builtin/commit.c
index 4fd1a16..11a0412 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1321,7 +1321,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 				allow_fast_forward = 0;
 		}
 		if (allow_fast_forward)
-			parents = reduce_heads(parents);
+			parents = commit_list_reduce_reads(parents);
 	} else {
 		if (!reflog_msg)
 			reflog_msg = "commit";
diff --git a/builtin/merge-base.c b/builtin/merge-base.c
index 96dd160..9a8b445 100644
--- a/builtin/merge-base.c
+++ b/builtin/merge-base.c
@@ -54,7 +54,7 @@ static int handle_octopus(int count, const char **args, int reduce, int show_all
 	for (i = count - 1; i >= 0; i--)
 		commit_list_insert(get_commit_reference(args[i]), &revs);
 
-	result = reduce ? reduce_heads(revs) : get_octopus_merge_bases(revs);
+	result = reduce ? commit_list_reduce_reads(revs) : get_octopus_merge_bases(revs);
 
 	if (!result)
 		return 1;
diff --git a/builtin/merge.c b/builtin/merge.c
index c24a7be..de21499 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -821,7 +821,7 @@ static int finish_automerge(struct commit_list *common,
 	if (allow_fast_forward) {
 		parents = remoteheads;
 		commit_list_insert(lookup_commit(head), &parents);
-		parents = reduce_heads(parents);
+		parents = commit_list_reduce_reads(parents);
 	} else {
 		struct commit_list **pptr = &parents;
 
diff --git a/commit.c b/commit.c
index 2d9265d..50bd85b 100644
--- a/commit.c
+++ b/commit.c
@@ -759,7 +759,7 @@ int in_merge_bases(struct commit *commit, struct commit **reference, int num)
 	return ret;
 }
 
-struct commit_list *reduce_heads(struct commit_list *heads)
+struct commit_list *commit_list_reduce_reads(struct commit_list *heads)
 {
 	struct commit_list *p;
 	struct commit_list *result = NULL, **tail = &result;
diff --git a/commit.h b/commit.h
index 9113bbe..648cadc 100644
--- a/commit.h
+++ b/commit.h
@@ -165,7 +165,7 @@ static inline int single_parent(struct commit *commit)
 	return commit->parents && !commit->parents->next;
 }
 
-struct commit_list *reduce_heads(struct commit_list *heads);
+struct commit_list *commit_list_reduce_reads(struct commit_list *heads);
 
 extern int commit_tree(const char *msg, unsigned char *tree,
 		struct commit_list *parents, unsigned char *ret,
diff --git a/revision.c b/revision.c
index b1c1890..a000d5d 100644
--- a/revision.c
+++ b/revision.c
@@ -1795,7 +1795,7 @@ static struct commit_list **simplify_one(struct rev_info *revs, struct commit *c
 	 * Further reduce the parents by removing redundant parents.
 	 */
 	if (1 < cnt) {
-		struct commit_list *h = reduce_heads(commit->parents);
+		struct commit_list *h = commit_list_reduce_reads(commit->parents);
 		cnt = commit_list_count(h);
 		free_commit_list(commit->parents);
 		commit->parents = h;

-- 
1.7.3.2.343.g7d43d
Jonathan Nieder· Dec 5, 2010, 02:18 UTC · re: Thiago Farina · lore

Re: [PATCH] commit: Add commit_list prefix to reduce_heads function.

Thiago Farina wrote:
> Signed-off-by: Thiago Farina <tfransosi@gmail.com>

I know that the context is part of an effort to make the commit_list functions into something more of a self-contained API, but the reader does not know that. Perhaps you could say some words about that in the change description: what's wrong with the current situation, what context does this change come from, and what positive effect would it have?

Beyond that, I must say I do not think this goes far enough to seem useful. If I wondered what reduce_heads did, wouldn't commit_list_reduce_heads be even more confusing? (ignoring the typo)

Perhaps a more natural way to proceed would be as follows:
 . first, collect the functions to be treated as a module and
   list them in Documentation/technical (in this case, perhaps
   api-revision-walking or a new api-commit-list)
 . next, describe their current meaning.  If this requires
   apologizing for the name, that's a good hint that a name
   change might be worthwhile
 . finally, tweak signatures (names and arguments) based on the
   results from step 2 and update the documentation at the same
   time.

That way, people used to the current functions would at least have some documentation to help them adjust. What do you think?

Thiago Farina· Dec 5, 2010, 12:18 UTC · re: Jonathan Nieder · lore

Re: [PATCH] commit: Add commit_list prefix to reduce_heads function.

On Sun, Dec 5, 2010 at 12:18 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 21 quoted lines
> Thiago Farina wrote:
>
>> Signed-off-by: Thiago Farina <tfransosi@gmail.com>
>
> I know that the context is part of an effort to make the commit_list
> functions into something more of a self-contained API, but the
> reader does not know that.  Perhaps you could say some words about
> that in the change description: what's wrong with the current
> situation, what context does this change come from, and what positive
> effect would it have?
>
> Beyond that, I must say I do not think this goes far enough to seem
> useful.  If I wondered what reduce_heads did, wouldn't
> commit_list_reduce_heads be even more confusing? (ignoring the typo)
>
> Perhaps a more natural way to proceed would be as follows:
>
>  . first, collect the functions to be treated as a module and
>   list them in Documentation/technical (in this case, perhaps
>   api-revision-walking or a new api-commit-list)
>

What you want here? That I describe the functions in these files? Why me? Why not the person who wrote them?

>  . next, describe their current meaning.  If this requires
>   apologizing for the name,
Apologize? For what? I don't understand what you mean here.
Show 6 quoted lines
> that's a good hint that a name  change might be worthwhile
>
>  . finally, tweak signatures (names and arguments) based on the
>   results from step 2 and update the documentation at the same
>   time.
>
I'd prefer to do just that step.
> That way, people used to the current functions would at least have
> some documentation to help them adjust.  What do you think?
>

I think it's a good procedure for someone more familiar with this functions to do this. Perhaps, you or Junio?

Jonathan Nieder· Dec 5, 2010, 17:09 UTC · re: Thiago Farina · lore

Re: [PATCH] commit: Add commit_list prefix to reduce_heads function.

Thiago Farina wrote:
> I think it's a good procedure for someone more familiar with this
> functions to do this. Perhaps, you or Junio?

If you are not familiar enough with the functions to document them (perhaps with help from the list) then yes, renaming them is a bad idea. I am not inclined to do it because I like the current name.

The ideal patch is a great sort of present: first a bug report, then the resolution to that bug. When the patch proper goes awry, at least there is the bug report. I think you are trying to convey a bug but you haven't explained it. Maybe it is

 - "The reduce_heads function being used in various contexts, where it
   is not obvious what it means.  If you add commit_list to the name,
   then <such and such> becomes obvious.  So I suggest renaming."	or
 - "In my program, I have my _own_ reduce_heads function with
   different meaning so I cannot easily copy the commit_list functions
   to use them.  Please make it easier by putting commit_list functions
   in a well defined namespace."	or
 - "Some code is manipulating commit_lists directly and violating
   their invariants.  Please make it easier to build a cheat-sheet
   listing commit_list functions, to translate from
   bad-field-manipulation-ese to using-the-right-functions-ese."	or
 - "At my office there is a style guide indicating that each function
   should live in a module with some other functions and be named to
   indicate so (like perf, with its sched__* etc functions).  The idea
   is that code with a simple high-level structure tends to be easier
   to understand and we need to understand the code we use.  Can we
   start changing the code to fit this style guide, so there is less
   resistance to using it at my office?"

In a way, these are straw men; sorry about that. The answer to each would be different. FWIW from my pov the answer to _none_ of these would be "sure, let's rename the functions", for different reasons in each case.

I do not think this is an atypical example at all. I would have prefered not to spend time on patches that require guessing what problem is being solved.

Thiago Farina· Dec 5, 2010, 17:29 UTC · re: Jonathan Nieder · lore

Re: [PATCH] commit: Add commit_list prefix to reduce_heads function.

On Sun, Dec 5, 2010 at 3:09 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 8 quoted lines
>  - "At my office there is a style guide indicating that each function
>   should live in a module with some other functions and be named to
>   indicate so (like perf, with its sched__* etc functions).  The idea
>   is that code with a simple high-level structure tends to be easier
>   to understand and we need to understand the code we use.  Can we
>   start changing the code to fit this style guide, so there is less
>   resistance to using it at my office?"
>

For me that is a good reason and I think it matches with what I had in mind but didn't write. Thanks for pointing it out.

Thiago Farina· Dec 5, 2010, 17:32 UTC · re: Thiago Farina · lore

Re: [PATCH] commit: Add commit_list prefix to reduce_heads function.

On Sun, Dec 5, 2010 at 3:29 PM, Thiago Farina <tfransosi@gmail.com> wrote:
Show 12 quoted lines
> On Sun, Dec 5, 2010 at 3:09 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>  - "At my office there is a style guide indicating that each function
>>   should live in a module with some other functions and be named to
>>   indicate so (like perf, with its sched__* etc functions).  The idea
>>   is that code with a simple high-level structure tends to be easier
>>   to understand and we need to understand the code we use.  Can we
>>   start changing the code to fit this style guide, so there is less
>>   resistance to using it at my office?"
>>
> For me that is a good reason and I think it matches with what I had in
> mind but didn't write. Thanks for pointing it out.
>

Also I thought that as Junio already picked up the other patch. It's was a hint that the other functions that has "struct commit_list *l" as its parameters could be renamed as well. But I was wrong it seems.

Junio C Hamano· Dec 5, 2010, 21:07 UTC · re: Thiago Farina · lore

Re: [PATCH] commit: Add commit_list prefix to reduce_heads function.

Thiago Farina <tfransosi@gmail.com> writes:
> Also I thought that as Junio already picked up the other patch. It's
> was a hint that the other functions that has "struct commit_list *l"
> as its parameters could be renamed as well.

You took a wrong hint and I think that is because you didn't think about what naming is for.

"insert-by-date" does not say _why_ you want things to be inserted by date (neither "sort-by-date"). They are pretty generic looking names for any function that deal with a list of elements that record date. It makes sense to anticipate there will be many other such functions that deal with different kinds of lists that hold date-recording things, and naming one of them "this deals with list of COMMITS" by saying "commit_list_foo" makes quite a lot of sense, as "insert-by-date" does not give sufficient information to the reader.

On the other hand, "reduce-heads" is with quite a higher level of semantics than "insert-by-date" and friends. The caller has a set of commits and wants to remove the ones that can be reached by other commits in that set, typically because it wants to come up with a list of commits to be used as parents of a merge commit across them. It has much stronger "why" associated with it; unlike "insert-by-date" and friends, there can't be many other such functions that deal with different datastructures that hold commits to reduce the heads the same way.

A related tangent. There are two ways to name functions with richer "why" component. Some people name them after what the caller expects them to do (e.g. they would name "compute merge parents" the function in question), and others names them after what they themselves do (e.g. it is about reducing the set of heads by removing redundant parents, and it does not question for what purpose the caller wants to do so). In general, it is preferrable to name them after what they do, not why the caller wants them to do so, especially when the semantics is clear. It will allow easier reuse of the function by new callers that do not create a new merge commit but wants the same head reduction.

Thiago Farina· Dec 5, 2010, 21:29 UTC · re: Junio C Hamano · lore

Re: [PATCH] commit: Add commit_list prefix to reduce_heads function.

On Sun, Dec 5, 2010 at 7:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 18 quoted lines
> Thiago Farina <tfransosi@gmail.com> writes:
>
>> Also I thought that as Junio already picked up the other patch. It's
>> was a hint that the other functions that has "struct commit_list *l"
>> as its parameters could be renamed as well.
>
> You took a wrong hint and I think that is because you didn't think about
> what naming is for.
>
> "insert-by-date" does not say _why_ you want things to be inserted by date
> (neither "sort-by-date").  They are pretty generic looking names for any
> function that deal with a list of elements that record date.  It makes
> sense to anticipate there will be many other such functions that deal with
> different kinds of lists that hold date-recording things, and naming one
> of them "this deals with list of COMMITS" by saying "commit_list_foo"
> makes quite a lot of sense, as "insert-by-date" does not give sufficient
> information to the reader.
>

That makes sense to me. And clarified why the complain at all. And you are right.

Would these be a candidates for adding commit_list_ prefix?

free_commit_list -> commit_list_free contains pop_most_recent_commit -> I'm not sure about this because of the length of it, as Jonathan pointed in this thread. pop_commit

Junio C Hamano· Dec 6, 2010, 03:58 UTC · re: Thiago Farina · lore

Re: [PATCH] commit: Add commit_list prefix to reduce_heads function.

Thiago Farina <tfransosi@gmail.com> writes:
> Would these be a candidates for adding commit_list_ prefix?
>
> free_commit_list -> commit_list_free

I think free_commit_list is a reasonable name for a function to free a commit-list already.

> contains

Historically I think we had two functions with this name, and both were named perfectly fine in the context they were introduced in.

One is "does this haystack contain the needle we are looking for?", which is a private helper in diffcore-pickaxe.c and considering what that module does, it is crystal clear that it is about "needle in haystack" without anything else tucked to its name.

The other is in 'pu' that came from Peff's "How about this" patch to compute something similar to is-descendant-of more efficiently, while sacrificing the ability to be usable as a general helper function.

As I already said in the review of that stalled series, the particular implementation of that function is good enough within the scope of the command it is used for (namely "tag --contains"); its implementation needs to be cleaned up, moved from commit.c to builtin/tag.c and made static to the file, but as long as that happens, it is named appropriately.

> pop_most_recent_commit -> I'm not sure about this because of the
> length of it, as Jonathan pointed in this thread.
> pop_commit

These two do not sound wrong---pop/push implies queue/list-ness and is quite clear that we are removing the topmost element from it.

Junio C Hamano· Dec 6, 2010, 04:01 UTC · re: Thiago Farina · lore

Re: [PATCH] commit: Add commit_list prefix to reduce_heads function.

Thiago Farina <tfransosi@gmail.com> writes:
Show 11 quoted lines
>> "insert-by-date" does not say _why_ you want things to be inserted by date
>> (neither "sort-by-date").  They are pretty generic looking names for any
>> function that deal with a list of elements that record date.  It makes
>> sense to anticipate there will be many other such functions that deal with
>> different kinds of lists that hold date-recording things, and naming one
>> of them "this deals with list of COMMITS" by saying "commit_list_foo"
>> makes quite a lot of sense, as "insert-by-date" does not give sufficient
>> information to the reader.
>>
> That makes sense to me. And clarified why the complain at all. And you
> are right.

Actually I think s/insert_by_date/commit_list_insert_by_date/ is a mistake. Something like insert-commit-by-date would be more appropriate. Similarly for s/sort_by_date/commit_list_sort_by_date/

Junio C Hamano· Dec 5, 2010, 05:24 UTC · re: Thiago Farina · lore

Re: [PATCH] commit: Add commit_list prefix to reduce_heads function.

Thiago Farina <tfransosi@gmail.com> writes:
> Signed-off-by: Thiago Farina <tfransosi@gmail.com>
I really do not like this.

The use of type "struct commit_list" to hold the set of parent commits is incidental; if we had "struct commit_set", we would have written a function with the same purpose, and named it the same "reduce_HEADS".

Adding commit_list to the name makes the code harder to read (and type) with little added benefit. "LIST"-ness is not the important part.

If a function takes a commit_list, named "reduce_HEADS", and returns a commit_list, what it does should be obvious to you; otherwise you shouldn't be touching the internal of git.

Having said that, I do not claim "reduce_heads" is the world most wonderful short-sweet-descriptive name for what this function does and there cannot be any better name. But commit_list_reduce_head is not it.

If the patch were to rename the function, especially the HEADs part, to clarify what it does, instead of how (iow, what type it uses to hold the data) it does it, my reaction would have been very different.

Thiago Farina· Dec 5, 2010, 12:23 UTC · re: Junio C Hamano · lore

Re: [PATCH] commit: Add commit_list prefix to reduce_heads function.

On Sun, Dec 5, 2010 at 3:24 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 6 quoted lines
> Thiago Farina <tfransosi@gmail.com> writes:
>
>> Signed-off-by: Thiago Farina <tfransosi@gmail.com>
>
> I really do not like this.
>

I don't feel very strong about it. And as I learned from Jonathan, I don't care if you will take or not. I think my intention was good, but I can't please everybody

I was just trying to put commit_list in a better shape and resemble it in a more explicit API.

Show 8 quoted lines
> The use of type "struct commit_list" to hold the set of parent commits is
> incidental; if we had "struct commit_set", we would have written a
> function with the same purpose, and named it the same "reduce_HEADS".
>
> Adding commit_list to the name makes the code harder to read (and type)
> with little added benefit.  "LIST"-ness is not the important part.
>
> If a function takes a commit_list, named "reduce_HEADS",
What? reduce_HEADS ? HEADS with CAPSLOCK?
Junio C Hamano· Dec 5, 2010, 20:40 UTC · re: Thiago Farina · lore

Re: [PATCH] commit: Add commit_list prefix to reduce_heads function.

Thiago Farina <tfransosi@gmail.com> writes:
>> If a function takes a commit_list, named "reduce_HEADS",
>
> What? reduce_HEADS ? HEADS with CAPSLOCK?

I was just hiliting the relevant parts of the name for you. Read it as if it was painted in red or something.

← back to recent threads