From: Junio C Hamano <gitster@pobox.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: liam Beguin <liambeguin@gmail.com>,
git@vger.kernel.org, avarab@gmail.com
Subject: Re: [PATCH 4/5] rebase -i: learn to abbreviate command names
Date: Sat, 02 Dec 2017 17:18:50 -0800 [thread overview]
Message-ID: <xmqqk1y4libp.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <alpine.DEB.2.21.1.1711292236010.6482@virtualbox> (Johannes Schindelin's message of "Wed, 29 Nov 2017 22:40:52 +0100 (CET)")
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> I am a fan of not relying too heavily on compiler optimization and e.g.
> extract code from loops when it does not need to be evaluated every single
> iteration. In this case:
>
> const char *pick = abbreviate_commands ? "p" : "pick";
> ...
> strbuf_addf(&buf, "%s %s ", pick,
> oid_to_hex(&commit->object.oid));
I would have called that variable "pick_cmd", not just "pick"; this
preference is minor enough that I would probably reject a patch to
rename from one to the other if the above were already part of the
existing codebase.
I find that the code suggested above easier to follow, simply
because it expresses clearly the flow of thought and that flow of
thought matches how I personally think: we decide how this command
is spelled in the output upfront, and then use that same spelling
consistently throughout the loop.
I do not think it matters performance-wise either way, but I value
how easy it is to follow the code for humans, and it matters much
more in the longer run. If a compiler does a poor job, we can
eventually notice and help it to produce better code that still does
what we wanted it to do (or it may not be performance critical and
we may not even notice). If a code is hard to follow, on the other
hand, what we wanted it to do in the first place becomes harder to
figure out.
next prev parent reply other threads:[~2017-12-03 1:18 UTC|newest]
Thread overview: 70+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-27 4:55 [PATCH 0/5] rebase -i: add config to abbreviate command names Liam Beguin
2017-11-27 4:55 ` [PATCH 1/5] Documentation: move rebase.* configs to new file Liam Beguin
2017-11-27 21:27 ` Johannes Schindelin
2017-11-27 4:55 ` [PATCH 2/5] Documentation: use preferred name for the 'todo list' script Liam Beguin
2017-11-27 21:28 ` Johannes Schindelin
2017-11-27 4:55 ` [PATCH 3/5] rebase -i: add exec commands via the rebase--helper Liam Beguin
2017-11-27 5:14 ` Junio C Hamano
2017-11-27 21:41 ` Johannes Schindelin
2017-11-27 23:45 ` Junio C Hamano
2017-11-29 2:01 ` liam Beguin
2017-11-27 22:42 ` Johannes Schindelin
2017-11-27 23:48 ` Junio C Hamano
2017-11-29 2:06 ` liam Beguin
2017-11-29 21:35 ` Johannes Schindelin
2017-11-27 4:55 ` [PATCH 4/5] rebase -i: learn to abbreviate command names Liam Beguin
2017-11-27 5:19 ` Junio C Hamano
2017-11-29 2:08 ` liam Beguin
2017-11-27 23:04 ` Johannes Schindelin
2017-11-27 23:11 ` Jeff King
2017-11-29 2:11 ` liam Beguin
2017-11-29 2:10 ` liam Beguin
2017-11-29 21:40 ` Johannes Schindelin
2017-12-03 1:18 ` Junio C Hamano [this message]
2017-11-27 4:55 ` [PATCH 5/5] t3404: add test case for abbreviated commands Liam Beguin
2017-11-27 5:28 ` Junio C Hamano
2017-11-27 23:16 ` Johannes Schindelin
2017-11-27 5:23 ` [PATCH 0/5] rebase -i: add config to abbreviate command names Junio C Hamano
2017-11-29 1:56 ` liam Beguin
2017-12-03 22:17 ` [PATCH v2 0/9] " Liam Beguin
2017-12-03 22:17 ` [PATCH v2 1/9] Documentation: move rebase.* configs to new file Liam Beguin
2017-12-03 22:17 ` [PATCH v2 2/9] Documentation: use preferred name for the 'todo list' script Liam Beguin
2017-12-03 22:17 ` [PATCH v2 3/9] rebase -i: set commit to null in exec commands Liam Beguin
2017-12-03 22:17 ` [PATCH v2 4/9] rebase -i: refactor transform_todo_ids Liam Beguin
2017-12-04 14:42 ` Johannes Schindelin
2017-12-04 16:09 ` Junio C Hamano
2017-12-05 3:36 ` liam Beguin
2017-12-05 12:35 ` Junio C Hamano
2017-12-05 3:39 ` liam Beguin
2017-12-03 22:17 ` [PATCH v2 5/9] rebase -i: replace reference to sha1 with oid Liam Beguin
2017-12-03 22:17 ` [PATCH v2 6/9] rebase -i: update functions to use a flags parameter Liam Beguin
2017-12-04 15:46 ` Johannes Schindelin
2017-12-04 16:06 ` Johannes Schindelin
2017-12-05 3:42 ` liam Beguin
2017-12-05 12:37 ` Junio C Hamano
2017-12-05 12:41 ` Kerry, Richard
2017-12-05 14:42 ` liam Beguin
2017-12-05 16:05 ` Junio C Hamano
2017-12-05 16:14 ` Kerry, Richard
2017-12-03 22:17 ` [PATCH v2 7/9] rebase -i -x: add exec commands via the rebase--helper Liam Beguin
2017-12-03 22:17 ` [PATCH v2 8/9] rebase -i: learn to abbreviate command names Liam Beguin
2017-12-25 12:48 ` Duy Nguyen
2017-12-25 15:39 ` Liam Beguin
2017-12-25 23:58 ` Duy Nguyen
2017-12-27 19:15 ` Junio C Hamano
2017-12-27 21:58 ` Liam Beguin
2017-12-28 18:55 ` Junio C Hamano
2017-12-03 22:17 ` [PATCH v2 9/9] t3404: add test case for abbreviated commands Liam Beguin
2017-12-04 16:07 ` [PATCH v2 0/9] rebase -i: add config to abbreviate command names Johannes Schindelin
2017-12-05 17:52 ` Liam Beguin
2017-12-05 17:52 ` [PATCH v3 1/9] Documentation: move rebase.* configs to new file Liam Beguin
2017-12-05 17:52 ` [PATCH v3 2/9] Documentation: use preferred name for the 'todo list' script Liam Beguin
2017-12-05 17:52 ` [PATCH v3 3/9] rebase -i: set commit to null in exec commands Liam Beguin
2017-12-05 17:52 ` [PATCH v3 4/9] rebase -i: refactor transform_todo_ids Liam Beguin
2017-12-05 17:52 ` [PATCH v3 5/9] rebase -i: replace reference to sha1 with oid Liam Beguin
2017-12-05 17:52 ` [PATCH v3 6/9] rebase -i: update functions to use a flags parameter Liam Beguin
2017-12-05 17:52 ` [PATCH v3 7/9] rebase -i -x: add exec commands via the rebase--helper Liam Beguin
2017-12-05 17:52 ` [PATCH v3 8/9] rebase -i: learn to abbreviate command names Liam Beguin
2017-12-05 17:52 ` [PATCH v3 9/9] t3404: add test case for abbreviated commands Liam Beguin
2017-12-05 22:21 ` [PATCH v2 0/9] rebase -i: add config to abbreviate command names Junio C Hamano
2017-12-06 2:42 ` liam Beguin
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqk1y4libp.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=liambeguin@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.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).