From: Jonathan Nieder <jrnieder@gmail.com>
To: Thiago Farina <tfransosi@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] commit: Add commit_list prefix to reduce_heads function.
Date: Sun, 5 Dec 2010 11:09:19 -0600 [thread overview]
Message-ID: <20101205170919.GA7913@burratino> (raw)
In-Reply-To: <AANLkTinAT3kotKQTS6eS1SLigNzSp6grAU7WNRbHf3N=@mail.gmail.com>
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.
next prev parent reply other threads:[~2010-12-05 17:09 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20101205170919.GA7913@burratino \
--to=jrnieder@gmail.com \
--cc=git@vger.kernel.org \
--cc=tfransosi@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).