Re: [PATCH 1/2] bundle: Accept prerequisites without commit messages
On Sun, Apr 07, 2013 at 09:06:10PM -0400, Jeff King wrote:
> On Sun, Apr 07, 2013 at 10:21:33AM -0700, Junio C Hamano wrote:
>
> > As to the order of comparison to match the order on the number line,
> > e.g. write "0 < something" or "negative < 0" to let readers more
> > easily visualize in what relation on the number line the quantity of
> > each side of the comparison stands, here is a reference to a long
> > and amusing thread:
> >
> > http://thread.gmane.org/gmane.comp.version-control.git/3903/focus=3912
>
> I do not necessarily agree with the "always use less-than" style, but as
> a reviewer of this series, it took me an extra minute to figure out what
> was going on because two things changed. If the diff instead looked
> like:
>
> diff --git a/bundle.c b/bundle.c
> index 505e07e..a9c1335 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -57,7 +57,7 @@ static int parse_bundle_header(int fd, struct bundle_header
> *header,
>* followed by SP and subject line.
>*/
> if (get_sha1_hex(buf.buf, sha1) ||
> - (40 <= buf.len && !isspace(buf.buf[40])) ||
> + (40 < buf.len && !isspace(buf.buf[40])) ||
> (!is_prereq && buf.len <= 40)) {
> if (report_path)
> error(_("unrecognized header: %s%s (%d)"),
>
> then it is immediately obvious that we are only impacting the case where
> buf.len is exactly 40 (and it is even more obvious if you happen to use
> the diff-highlight script, which highlights the single changed
> character).
I changed it for the very same reason -- it took me an extra minute to
figure out what is going on when trying to pinpoint the bug (it was
especially weird since we use "40 <= buf.len" here and "buf.len <= 40"
one line below -- which kind of makes sense now, though). Thanks for the
review (and merging), I won't change operand order in future patches :)
>
> Just my two cents as a reader of the patch. Other than that, it looks
> correct to me. :)
>
> -Peff
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] bundle: Accept prerequisites without commit messages
On Sun, Apr 07, 2013 at 10:21:33AM -0700, Junio C Hamano wrote:
> As to the order of comparison to match the order on the number line,
> e.g. write "0 < something" or "negative < 0" to let readers more
> easily visualize in what relation on the number line the quantity of
> each side of the comparison stands, here is a reference to a long
> and amusing thread:
>
> http://thread.gmane.org/gmane.comp.version-control.git/3903/focus=3912
I do not necessarily agree with the "always use less-than" style, but as
a reviewer of this series, it took me an extra minute to figure out what
was going on because two things changed. If the diff instead looked
like:
diff --git a/bundle.c b/bundle.c
index 505e07e..a9c1335 100644
--- a/bundle.c
+++ b/bundle.c
@@ -57,7 +57,7 @@ static int parse_bundle_header(int fd, struct bundle_header
*header,
* followed by SP and subject line.
*/
if (get_sha1_hex(buf.buf, sha1) ||
- (40 <= buf.len && !isspace(buf.buf[40])) ||
+ (40 < buf.len && !isspace(buf.buf[40])) ||
(!is_prereq && buf.len <= 40)) {
if (report_path)
error(_("unrecognized header: %s%s (%d)"),
then it is immediately obvious that we are only impacting the case where
buf.len is exactly 40 (and it is even more obvious if you happen to use
the diff-highlight script, which highlights the single changed
character).
Just my two cents as a reader of the patch. Other than that, it looks
correct to me. :)
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] bundle: Accept prerequisites without commit messages
Junio C Hamano writes: > Lukas Fleischer writes: > >> While explicitly stating that the commit message in a prerequisite >> line is optional, > > Good spotting. What e9ee84cf28b6 (bundle: allowing to read from an > unseekable fd, 2011-10-13) meant to say was the SP was optional but s/say was the SP was optional/say was not the SP was optional/ > we want to allow a tip bundled to have a commit without any commit > log message (hence making it "optional"), and the check you are > looking at does try to enforce. What was buggy was that the > comparison did not take into account that the codepath earlier > called rtrim() on it, stripping "-SP" of the > trailing SP it wants to look for. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] bundle: Accept prerequisites without commit messages
Lukas Fleischer writes: > While explicitly stating that the commit message in a prerequisite > line is optional, Good spotting. What e9ee84cf28b6 (bundle: allowing to read from an unseekable fd, 2011-10-13) meant to say was the SP was optional but we want to allow a tip bundled to have a commit without any commit log message (hence making it "optional"), and the check you are looking at does try to enforce. What was buggy was that the comparison did not take into account that the codepath earlier called rtrim() on it, stripping "-SP" of the trailing SP it wants to look for. As to the order of comparison to match the order on the number line, e.g. write "0 < something" or "negative < 0" to let readers more easily visualize in what relation on the number line the quantity of each side of the comparison stands, here is a reference to a long and amusing thread: http://thread.gmane.org/gmane.comp.version-control.git/3903/focus=3912 We never had any official guideline on which to use. A patch that changes an existing "b > a" to "a < b" (this directoin only) because of the above thread is _not_ "a patch to fix coding style", and is very much unwelcome. Similarly, a patch that changes an existing "a < b" to "b > a" (or vice versa) only because the author thinks it is easier to read is not welcomed. A switch done as a part of other meaningful rewrite is not a big enough deal to make a fuss over, though. Your patch falls into this category, I think. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] bundle: Accept prerequisites without commit messages
On Sun, Apr 07, 2013 at 01:53:15PM +0200, Lukas Fleischer wrote:
> While explicitly stating that the commit message in a prerequisite line
> is optional, we required all lines with 40 or more characters to contain
> a space after the object name, bailing out if a line consisted of an
> object name only. Fix this off-by-one error and only require lines with
> more than 40 characters to contain the separating space.
>
> Signed-off-by: Lukas Fleischer
> ---
> bundle.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/bundle.c b/bundle.c
> index 505e07e..4b0e5cd 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -57,7 +57,7 @@ static int parse_bundle_header(int fd, struct bundle_header
> *header,
>* followed by SP and subject line.
>*/
> if (get_sha1_hex(buf.buf, sha1) ||
> - (40 <= buf.len && !isspace(buf.buf[40])) ||
> + (buf.len > 40 && !isspace(buf.buf[40])) ||
By the way, I changed this to "buf.len > 40" instead of "40 < buf.len"
because I personally think that the former is much easier to read here.
Is there any general guideline when to use which order? grep(1) says we
use both forms:
$ grep '0 <' *.c | wc -l
119
$ grep '> 0' *.c | wc -l
164
> (!is_prereq && buf.len <= 40)) {
> if (report_path)
> error(_("unrecognized header: %s%s (%d)"),
> --
> 1.8.2.675.gda3bb24.dirty
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] bundle: Accept prerequisites without commit messages
While explicitly stating that the commit message in a prerequisite line
is optional, we required all lines with 40 or more characters to contain
a space after the object name, bailing out if a line consisted of an
object name only. Fix this off-by-one error and only require lines with
more than 40 characters to contain the separating space.
Signed-off-by: Lukas Fleischer
---
bundle.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/bundle.c b/bundle.c
index 505e07e..4b0e5cd 100644
--- a/bundle.c
+++ b/bundle.c
@@ -57,7 +57,7 @@ static int parse_bundle_header(int fd, struct bundle_header
*header,
* followed by SP and subject line.
*/
if (get_sha1_hex(buf.buf, sha1) ||
- (40 <= buf.len && !isspace(buf.buf[40])) ||
+ (buf.len > 40 && !isspace(buf.buf[40])) ||
(!is_prereq && buf.len <= 40)) {
if (report_path)
error(_("unrecognized header: %s%s (%d)"),
--
1.8.2.675.gda3bb24.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
