Hey Eric,
On Thu, Jun 16, 2016 at 2:44 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva <pranit.bauva@xxxxxxxxx> wrote:
>> Reimplement `is_expected_rev` & `check_expected_revs` shell function in
>> C and add a `--check-expected-revs` subcommand to `git bisect--helper` to
>> call it from git-bisect.sh .
>> [...]
>> Signed-off-by: Pranit Bauva <pranit.bauva@xxxxxxxxx>
>> ---
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> @@ -162,13 +162,44 @@ static int bisect_reset(const char *commit)
>> +static int is_expected_rev(const char *expected_hex)
>> +{
>> + struct strbuf actual_hex = STRBUF_INIT;
>> + int res;
>> +
>> + if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) < 0) {
>> + strbuf_release(&actual_hex);
>> + return 0;
>> + }
>> +
>> + strbuf_trim(&actual_hex);
>> + res = !strcmp(actual_hex.buf, expected_hex);
>> + strbuf_release(&actual_hex);
>> + return res;
>> +}
>
> Not worth a re-roll, but this could be re-structured to avoid having
> to remember to release the strbuf at all exits:
>
> struct strbuf actual_hex = ...;
> int res = 0;
>
> if (strbuf_read_file(...) >= 0) {
> strbuf_trim(...);
> res = !strcmp(...);
> }
> strbuf_release(...);
> return res;
>
> Alternately:
>
> if (strbuf_read_file(...) < 0)
> goto done;
>
> strbuf_trim(...);
> res = !strcmp(...);
>
> done:
> strbuf_release(...);
> return res;
>
> which is a bit less compact.
I will avoid this for the reason that I will have to create a label
for a lot of functions. If I choose to do this for one function, I
think it would be more appropriate to do the same for other functions.
There would be a lot of functions in future which would be in the same
scenario and creating a separate label for each of them would be quite
tedious. What do you think?
>> +static int check_expected_revs(const char **revs, int rev_nr)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < rev_nr; i++) {
>> + if (!is_expected_rev(revs[i])) {
>> + remove_path(git_path_bisect_ancestors_ok());
>> + remove_path(git_path_bisect_expected_rev());
>> + return 0;
>> + }
>> + }
>> + return 0;
>> +}
>
> Hmm, all execution paths return 0, so it feels a bit pointless to have
> this function return a value at all.
>
> You could also use a 'break' inside the loop rather than 'return'
> since the return value is the same inside or outside the loop and
> nothing else happens after the loop.
Sure!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html