Re: [PATCH v14 08/27] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C
Pranit Bauva writes:
> Hey Junio,
>
> On Thu, Aug 25, 2016 at 3:43 AM, Junio C Hamano wrote:
>> Pranit Bauva writes:
>>
>>> +static int is_expected_rev(const char *expected_hex)
>>> +{
>>> + struct strbuf actual_hex = STRBUF_INIT;
>>> + int res = 0;
>>> + if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0)
>>> >= 0) {
>>> + strbuf_trim(&actual_hex);
>>> + res = !strcmp(actual_hex.buf, expected_hex);
>>
>> If it is known to have 40-hex:
>>
>> (1) accepting ">= 0" seems way too lenient. You only expect a
>> 41-byte file (or 42 if somebody would write CRLF, but I do not
>> think anybody other than yourself is expected to write into
>> this file, and you do not write CRLF yourself);
>>
>> (2) strbuf_trim() is overly loose. You only want to trim the
>> terimnating LF and it is an error to have other trailing
>> whitespaces.
>>
>> I think the latter is not a new problem and it is OK to leave it
>> as-is; limiting (1) to >= 40 may still be a good change, though,
>> because it makes the intention of the code clearer.
>
> I can do the first change. Since this wasn't present in the shell
> code, I will also mention it in the commit message.
There is no need for that, as that was present in the original. The
original compared the string with something that is known to be
40-hex, so anything shorter than 40 wouldn't have matched.
It is merely a trivial and obvious optimization to do that in C
rewrite.
Re: [PATCH v14 08/27] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C
Hey Junio,
On Thu, Aug 25, 2016 at 3:43 AM, Junio C Hamano wrote:
> Pranit Bauva writes:
>
>> +static int is_expected_rev(const char *expected_hex)
>> +{
>> + struct strbuf actual_hex = STRBUF_INIT;
>> + int res = 0;
>> + if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0)
>> >= 0) {
>> + strbuf_trim(&actual_hex);
>> + res = !strcmp(actual_hex.buf, expected_hex);
>
> If it is known to have 40-hex:
>
> (1) accepting ">= 0" seems way too lenient. You only expect a
> 41-byte file (or 42 if somebody would write CRLF, but I do not
> think anybody other than yourself is expected to write into
> this file, and you do not write CRLF yourself);
>
> (2) strbuf_trim() is overly loose. You only want to trim the
> terimnating LF and it is an error to have other trailing
> whitespaces.
>
> I think the latter is not a new problem and it is OK to leave it
> as-is; limiting (1) to >= 40 may still be a good change, though,
> because it makes the intention of the code clearer.
I can do the first change. Since this wasn't present in the shell
code, I will also mention it in the commit message.
Regards,
Pranit Bauva
--
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 v14 08/27] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C
Pranit Bauva writes:
> +static int is_expected_rev(const char *expected_hex)
> +{
> + struct strbuf actual_hex = STRBUF_INIT;
> + int res = 0;
> + if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) >=
> 0) {
> + strbuf_trim(&actual_hex);
> + res = !strcmp(actual_hex.buf, expected_hex);
If it is known to have 40-hex:
(1) accepting ">= 0" seems way too lenient. You only expect a
41-byte file (or 42 if somebody would write CRLF, but I do not
think anybody other than yourself is expected to write into
this file, and you do not write CRLF yourself);
(2) strbuf_trim() is overly loose. You only want to trim the
terimnating LF and it is an error to have other trailing
whitespaces.
I think the latter is not a new problem and it is OK to leave it
as-is; limiting (1) to >= 40 may still be a good change, though,
because it makes the intention of the code clearer.
--
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
