git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] commit: Add commit_list prefix to reduce_heads function.
@ 2010-12-05  1:59 Thiago Farina
  2010-12-05  2:18 ` Jonathan Nieder
  2010-12-05  5:24 ` Junio C Hamano
  0 siblings, 2 replies; 13+ messages in thread
From: Thiago Farina @ 2010-12-05  1:59 UTC (permalink / raw)
  To: git

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

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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] commit: Add commit_list prefix to reduce_heads function.
  2010-12-05  1:59 [PATCH] commit: Add commit_list prefix to reduce_heads function Thiago Farina
@ 2010-12-05  2:18 ` Jonathan Nieder
  2010-12-05 12:18   ` Thiago Farina
  2010-12-05  5:24 ` Junio C Hamano
  1 sibling, 1 reply; 13+ messages in thread
From: Jonathan Nieder @ 2010-12-05  2:18 UTC (permalink / raw)
  To: Thiago Farina; +Cc: git

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?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] commit: Add commit_list prefix to reduce_heads function.
  2010-12-05  1:59 [PATCH] commit: Add commit_list prefix to reduce_heads function Thiago Farina
  2010-12-05  2:18 ` Jonathan Nieder
@ 2010-12-05  5:24 ` Junio C Hamano
  2010-12-05 12:23   ` Thiago Farina
  1 sibling, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2010-12-05  5:24 UTC (permalink / raw)
  To: Thiago Farina; +Cc: git

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] commit: Add commit_list prefix to reduce_heads function.
  2010-12-05  2:18 ` Jonathan Nieder
@ 2010-12-05 12:18   ` Thiago Farina
  2010-12-05 17:09     ` Jonathan Nieder
  0 siblings, 1 reply; 13+ messages in thread
From: Thiago Farina @ 2010-12-05 12:18 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: git

On Sun, Dec 5, 2010 at 12:18 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> 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.

> 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?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] commit: Add commit_list prefix to reduce_heads function.
  2010-12-05  5:24 ` Junio C Hamano
@ 2010-12-05 12:23   ` Thiago Farina
  2010-12-05 20:40     ` Junio C Hamano
  0 siblings, 1 reply; 13+ messages in thread
From: Thiago Farina @ 2010-12-05 12:23 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On Sun, Dec 5, 2010 at 3:24 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

> 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?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] commit: Add commit_list prefix to reduce_heads function.
  2010-12-05 12:18   ` Thiago Farina
@ 2010-12-05 17:09     ` Jonathan Nieder
  2010-12-05 17:29       ` Thiago Farina
  0 siblings, 1 reply; 13+ messages in thread
From: Jonathan Nieder @ 2010-12-05 17:09 UTC (permalink / raw)
  To: Thiago Farina; +Cc: git

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] commit: Add commit_list prefix to reduce_heads function.
  2010-12-05 17:09     ` Jonathan Nieder
@ 2010-12-05 17:29       ` Thiago Farina
  2010-12-05 17:32         ` Thiago Farina
  0 siblings, 1 reply; 13+ messages in thread
From: Thiago Farina @ 2010-12-05 17:29 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: git

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] commit: Add commit_list prefix to reduce_heads function.
  2010-12-05 17:29       ` Thiago Farina
@ 2010-12-05 17:32         ` Thiago Farina
  2010-12-05 21:07           ` Junio C Hamano
  0 siblings, 1 reply; 13+ messages in thread
From: Thiago Farina @ 2010-12-05 17:32 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: git

On Sun, Dec 5, 2010 at 3:29 PM, Thiago Farina <tfransosi@gmail.com> wrote:
> 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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] commit: Add commit_list prefix to reduce_heads function.
  2010-12-05 12:23   ` Thiago Farina
@ 2010-12-05 20:40     ` Junio C Hamano
  0 siblings, 0 replies; 13+ messages in thread
From: Junio C Hamano @ 2010-12-05 20:40 UTC (permalink / raw)
  To: Thiago Farina; +Cc: Junio C Hamano, git

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] commit: Add commit_list prefix to reduce_heads function.
  2010-12-05 17:32         ` Thiago Farina
@ 2010-12-05 21:07           ` Junio C Hamano
  2010-12-05 21:29             ` Thiago Farina
  0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2010-12-05 21:07 UTC (permalink / raw)
  To: Thiago Farina; +Cc: Jonathan Nieder, git

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] commit: Add commit_list prefix to reduce_heads function.
  2010-12-05 21:07           ` Junio C Hamano
@ 2010-12-05 21:29             ` Thiago Farina
  2010-12-06  3:58               ` Junio C Hamano
  2010-12-06  4:01               ` Junio C Hamano
  0 siblings, 2 replies; 13+ messages in thread
From: Thiago Farina @ 2010-12-05 21:29 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jonathan Nieder, git

On Sun, Dec 5, 2010 at 7:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] commit: Add commit_list prefix to reduce_heads function.
  2010-12-05 21:29             ` Thiago Farina
@ 2010-12-06  3:58               ` Junio C Hamano
  2010-12-06  4:01               ` Junio C Hamano
  1 sibling, 0 replies; 13+ messages in thread
From: Junio C Hamano @ 2010-12-06  3:58 UTC (permalink / raw)
  To: Thiago Farina; +Cc: Jonathan Nieder, git

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] commit: Add commit_list prefix to reduce_heads function.
  2010-12-05 21:29             ` Thiago Farina
  2010-12-06  3:58               ` Junio C Hamano
@ 2010-12-06  4:01               ` Junio C Hamano
  1 sibling, 0 replies; 13+ messages in thread
From: Junio C Hamano @ 2010-12-06  4:01 UTC (permalink / raw)
  To: Thiago Farina; +Cc: Junio C Hamano, Jonathan Nieder, git

Thiago Farina <tfransosi@gmail.com> writes:

>> "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/

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2010-12-06  4:01 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-12-05  1:59 [PATCH] commit: Add commit_list prefix to reduce_heads function Thiago Farina
2010-12-05  2:18 ` Jonathan Nieder
2010-12-05 12:18   ` Thiago Farina
2010-12-05 17:09     ` Jonathan Nieder
2010-12-05 17:29       ` Thiago Farina
2010-12-05 17:32         ` Thiago Farina
2010-12-05 21:07           ` Junio C Hamano
2010-12-05 21:29             ` Thiago Farina
2010-12-06  3:58               ` Junio C Hamano
2010-12-06  4:01               ` Junio C Hamano
2010-12-05  5:24 ` Junio C Hamano
2010-12-05 12:23   ` Thiago Farina
2010-12-05 20:40     ` Junio C Hamano

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).