Re: [PATCH v15 23/27] bisect--helper: `bisect_replay` shell function in C

From: Stephan Beyer <s-beyer@gmx.net>
Date: Dec 6, 2016, 23:40 UTC
Message-ID: <58ab6600-5ac2-9e7d-7cf0-452db40d5f6b@gmx.net>
In-Reply-To: <CAFZEwPNmB7rYvUTPy6dvfqfbUsjDeEcteLBBH5Wk-G_suE+YTw@mail.gmail.com>

Hi Pranit and Christian and Lars ;)

On 12/07/2016 12:02 AM, Pranit Bauva wrote:

Show 12 quoted lines

> On Tue, Nov 22, 2016 at 6:19 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
>> Okay Pranit,
>>
>> this is the last patch for me to review in this series.
>>
>> Now that I have a coarse overview of what you did, I have the general
>> remark that imho the "terms" variable should simply be global instead of
>> being passed around all the time.
> 
> In a personal conversation with my mentors, we thought it is the best
> fit to keep it in a struct and pass it around so that it is easier in
> libification.

I guess you had that conversation at the beginning of the project and I think that at that stage I would have recommended it that way, too.

However, looking at the v15 bisect--helper.c, it looks like it is rather a pain to do it that way. I mean, you use the terms like they were global variables: you initialize them in cmd_bisect__helper() and then you always pass them around; you update them "globally", never using restricted scope, never ever use a second instantiation besides the one of cmd_bisect__helper(). These are all signs that *in the current code* using (static) global variables is the better way (making the code simpler).

The libification argument may be worth considering, though. I am not sure if there is really a use-case where you need two different instantiations of the terms, *except* if the lib allows bisecting with different terms at the same time (in different repos, of course). Is the lib "aware" of such use-cases like doing stuff in different repos at the same time? If that's the case, not using global variables is the right thing to do. In any other case I can imagine, I'd suggest to use global variables.

~Stephan