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

Hi Pranit and Christian and Lars ;)

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

On Tue, Nov 22, 2016 at 6:19 AM, Stephan Beyer [off-list ref] wrote:

quoted

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