From: Lex Spoon <lex@lexspoon.org>
To: Luke Diamand <luke@diamand.org>
Cc: Git Users <git@vger.kernel.org>, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCHv1 3/3] git-p4: fixing --changes-block-size handling
Date: Sun, 7 Jun 2015 12:33:20 -0400 [thread overview]
Message-ID: <CALM2SnbVY1baAONo3o2gb2NS+rDSsyhkPffP5EJZKU1MDA7q9w@mail.gmail.com> (raw)
In-Reply-To: <1433672505-11940-4-git-send-email-luke@diamand.org>
The implementation looks fine, especially given the test cases that
back it up. I am only curious why the block size is set to a default
of None. To put it as contcretely as possible: is there any expected
configuration where None would work but 500 would not? We know there
are many cases of the other way around, and those cases are going to
send users to StackOverflow to find the right workaround.
Dropping the option would also simplify the code in several places.
The complex logic around get_another_block could be removed, and
instead there could be a loop from start to mostRecentCommit by
block_size. Several places that check "if not block_size" could just
choose the other branch.
Lex Spoon
next prev parent reply other threads:[~2015-06-07 16:33 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-07 10:21 [PATCHv1 0/3] git-p4: fixing --changes-block-size support Luke Diamand
2015-06-07 10:21 ` [PATCHv1 1/3] git-p4: additional testing of --changes-block-size Luke Diamand
2015-06-07 16:06 ` Lex Spoon
2015-06-07 10:21 ` [PATCHv1 2/3] git-p4: test with limited p4 server results Luke Diamand
2015-06-07 16:11 ` Lex Spoon
2015-06-07 10:21 ` [PATCHv1 3/3] git-p4: fixing --changes-block-size handling Luke Diamand
2015-06-07 16:33 ` Lex Spoon [this message]
2015-06-07 17:06 ` Luke Diamand
2015-06-07 21:35 ` [PATCHv2 0/3] " Luke Diamand
2015-06-07 21:35 ` [PATCHv2 1/3] git-p4: additional testing of --changes-block-size Luke Diamand
2015-06-07 21:35 ` [PATCHv2 2/3] git-p4: test with limited p4 server results Luke Diamand
2015-06-07 21:35 ` [PATCHv2 3/3] git-p4: fixing --changes-block-size handling Luke Diamand
2015-06-07 22:58 ` Lex Spoon
2015-06-08 16:02 ` Junio C Hamano
2015-06-08 16:36 ` Lex Spoon
[not found] ` <xmqqy4juazkz.fsf@gitster.dls.corp.google.com>
[not found] ` <5575E264.6040601@diamand.org>
2015-06-08 22:32 ` Junio C Hamano
2015-06-07 16:01 ` [PATCHv1 0/3] git-p4: fixing --changes-block-size support Lex Spoon
2015-06-07 16:58 ` Luke Diamand
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=CALM2SnbVY1baAONo3o2gb2NS+rDSsyhkPffP5EJZKU1MDA7q9w@mail.gmail.com \
--to=lex@lexspoon.org \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=luke@diamand.org \
/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).