From: Michael Haggerty <mhagger@alum.mit.edu>
To: Aleksey Mokhovikov <moxobukob@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()
Date: Mon, 24 Mar 2014 13:22:45 +0100 [thread overview]
Message-ID: <53302395.6090207@alum.mit.edu> (raw)
In-Reply-To: <lgomc3$rh0$1@ger.gmane.org>
On 03/24/2014 08:28 AM, Aleksey Mokhovikov wrote:
> On 03/22/2014 04:13 AM, Michael Haggerty wrote:
>> My expectation when I invented that microproject was that converting the
>> code to be table-driven would be judged *not* to be an improvement. I
>> was hoping that a student would say "the 'if' statement is OK, but let's
>> delete this ridiculous unreachable else branch". Possibly they would
>> convert the "if" chain into nested "if"s, which I think would allow some
>> code consolidation in one of the branches.
>>
>> But not a single student agreed with me, so I must be in a minority of
>> one (which, unfortunately, is the definition of lunacy).
>>
>> The multidimensional array lookup table is not so terrible, but I
>> personally still prefer the "if".
>
> That was expectable. But the main goal for me was to participate in git
> development process, to become familiar with it.
> It looks hard to participate when not proposing a patch.
> I thought about make a small change in if chain, but it looked to minor
> to feel whole development process.
> I've used git features for formatting and sending a patch to mailing list.
> I've met the GNU gettext restrictions when proposed a first patch.
> Proposed another patch and tried to show Pros and Cons.
> It didn't look like applying a patch to git master branch was the main goal.
> As for me that was quite interesting and useful.
Sorry if there was a misunderstanding. I didn't mean to criticize you
and certainly not to single you out among the many students who went for
a table-driven solution. I was rather replying to Junio's general
comment, that maybe changing the code to be table-driven wasn't such a
good idea. These things are always a matter a taste, and sometimes hard
to predict before one has tried writing the code a couple of different ways.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
prev parent reply other threads:[~2014-03-24 12:22 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-03-17 9:55 [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config() Aleksey Mokhovikov
2014-03-17 10:01 ` Matthieu Moy
2014-03-18 14:42 ` Aleksey Mokhovikov
2014-03-17 10:53 ` Eric Sunshine
2014-03-18 14:37 ` Aleksey Mokhovikov
2014-03-18 12:22 ` Aleksey Mokhovikov
2014-03-18 14:33 ` Aleksey Mokhovikov
2014-03-19 9:21 ` Eric Sunshine
2014-03-20 11:56 ` Aleksey Mokhovikov
2014-03-21 4:03 ` Eric Sunshine
2014-03-21 17:09 ` Junio C Hamano
2014-03-21 21:13 ` Michael Haggerty
2014-03-21 21:33 ` Eric Sunshine
2014-03-21 21:45 ` Michael Haggerty
2014-03-24 7:28 ` Aleksey Mokhovikov
2014-03-24 12:22 ` Michael Haggerty [this message]
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=53302395.6090207@alum.mit.edu \
--to=mhagger@alum.mit.edu \
--cc=git@vger.kernel.org \
--cc=moxobukob@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).