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 <s-beyer@xxxxxxx> 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