Re: [PATCH v2 5/6] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C

Hey Christian,

On Fri, Jun 17, 2016 at 2:17 AM, Christian Couder
[off-list ref] wrote:

On Thu, Jun 16, 2016 at 9:25 PM, Pranit Bauva [off-list ref] wrote:

quoted

Hey Eric,

On Fri, Jun 17, 2016 at 12:46 AM, Eric Sunshine [off-list ref] wrote:

quoted

On Thu, Jun 16, 2016 at 3:05 PM, Pranit Bauva [off-list ref] wrote:

quoted

On Thu, Jun 16, 2016 at 2:44 AM, Eric Sunshine [off-list ref] wrote:

quoted

On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva [off-list ref] wrote:

quoted

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 <redacted>
---

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?

Not sure what you're talking about. Label names are not shared across
functions. Anyhow, the first suggestion I presented above is more
concise than the 'goto' version.

Yes I am aware of the fact that labels aren't shared across functions.
What I meant by "separate label" was that I will have to make a label
"fail" in each function. But I recently noticed that its used quite a
lot so I think it would be okay to use it. Will re-roll with using
labels and goto.

My opinion is that if there is a more concise version without labels
and gotos, it's better to use it, so I would suggest Eric's first
suggestion which is:

quoted

    struct strbuf actual_hex = ...;
    int res = 0;

    if (strbuf_read_file(...) >= 0) {
        strbuf_trim(...);
        res = !strcmp(...);
    }
    strbuf_release(...);
    return res;

Sure I could do that!

Regards,
Pranit Bauva