From: Pranit Bauva <pranit.bauva@gmail.com>
Date: Wed, 08 Jun 2016 18:04:06 GMT
Subject: Re: [PATCH 4/4] bisect--helper: `bisect_reset` shell function in C
Message-ID: <CAFZEwPMs+O7r8HKZ0bVxPtT2kNWmmJi0wruR1au_C527M+Ne-g@mail.gmail.com>
In-Reply-To: <CAPig+cQpBcy+Sdhv7EXorJFZsYvH=ikh0PqGF1YShQEcPjMn_Q@mail.gmail.com>

Hey Eric,

On Wed, Jun 8, 2016 at 11:23 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Jun 8, 2016 at 9:20 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> On Wed, Jun 8, 2016 at 1:29 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>>> +       if (!file_exists(git_path_bisect_head())) {
>>>> +               struct argv_array argv = ARGV_ARRAY_INIT;
>>>> +               argv_array_pushl(&argv, "checkout", branch.buf, "--", NULL);
>>>> +               status = run_command_v_opt(argv.argv, RUN_GIT_CMD);
>>>> +               argv_array_clear(&argv);
>>>> +       }
>>>> +
>>>> +       if (status) {
>>>
>>> What's the purpose of having this 'status' conditional outside of the
>>> above !file_exists() conditional, which is the only place that
>>> 'status' gets assigned. Likewise, 'status' itself could be declared
>>> within the scope of that conditional block.
>>
>> I wanted to avoid nesting. In a few other parts of the code also,
>> nesting is avoided as much as possible.
>
> I figured as much, but I'm not sure that that is such a good idea in
> this case since it makes the code more difficult to reason about. To
> wit: as a reader of the code, I spent extra time trying to figure why
> it was structured this way and if 'status' is assigned or accessed in
> some other non-obvious way.

I can put status inside the if() .

Regards,
Pranit Bauva