Policy on global variables

Policy on global variables

[Jason]

Hi guys,

Major question tormenting my cgitified soul...

Sometimes we use a global ctx variable. Other times we do not. It
appears that the global usage is more common, but that there's some
nice logic for passing around the ctx variable to function to function
as an argument or even in callback functions.

In theory, passing around the variable, and not relying on a global,
is better. It allows us at somepoint to have multiple contexts, for,
say, implementing FastCGI or an event loop single-process multi
response model. On the other hand, it's messier and uglier and harder
to deal with. And beyond ctx, we use several other globals in various
places.

After discussing this with him, Lukas whipped up a massive patch-set
(which I squashed down) that removes the parameter passage usage of
ctx and uses only globals:
http://git.zx2c4.com/cgit/commit/?h=lf/global-ctx

I'm torn over whether or not to merge this. It's a lot of nice careful
C that went in to the non-global references, and it seems like a shame
to trash that. A part of me feels like we should work on the opposite
patch -- where we get rid of all global variables. On the other hand,
maybe that's an unrealistic expectation, and we should instead
standardize on the global approach, and merge this patch.

I'd appreciate it if you all weighed in on this topic. I'm very torn
and am changing my mind every couple of seconds. What do you think?

Thanks,
Jason

On Wed, Jan 15, 2014 at 10:37 PM, Lukas Fleischer <cgit at cryptocrack.de> wrote:
> Jason noticed that sometimes, we pass a reference (pointer) to the
> global context variable. This series removes all such references and
> replaces them with direct use of the global variable.

Policy on global variables

[Jason]

A stranger, "bone" in ##c on freenode:


At first:

bone: "do you not care about reentrancy?"
...


After some discussion:

bone: given the practical considerations, you'll probably introduce
bugs by passing a context around, so you might as well leave it until
you actually need reentrancy. but i would try to refactor the code
functions that only need a subset of data can be passed that without
needing the whole context. then later you can change the upper level
functions to receive a context instead of using a static

me: so the first step, move to all global. then later on, refactor
things to pass a reference around, but to less places?

bone: yeah, presumably you have some functions that are only called by
one parent function, so they only do a subset of the work, and if they
can work on data from the context via parameters instead of
implicitly, all the better. then the parent can update the context as
needed, eventually you can reduce the number of accesses to the
context, then the move to passing the context around becomes much
easier implement

Policy on global variables

[normalperson]

"Jason A. Donenfeld" <Jason at zx2c4.com> wrote:
> In theory, passing around the variable, and not relying on a global,
> is better. It allows us at somepoint to have multiple contexts, for,
> say, implementing FastCGI or an event loop single-process multi
> response model.

This.  I prefer we keep passing around the ctx variable to keep the code
more flexible for future reuse.  Of course, IIRC git itself has this
limitation, too...

Policy on global variables

[Jason]

On Thu, Jan 16, 2014 at 1:59 AM, Eric Wong <normalperson at yhbt.net> wrote:
> This.  I prefer we keep passing around the ctx variable to keep the code
> more flexible for future reuse.  Of course, IIRC git itself has this
> limitation, too...

Can anyone confirm or deny this? Is it a pointless endeavor because of
git's design?

Policy on global variables

[hjemli]

On Thu, Jan 16, 2014 at 2:00 AM, Jason A. Donenfeld <Jason at zx2c4.com> wrote:
> On Thu, Jan 16, 2014 at 1:59 AM, Eric Wong <normalperson at yhbt.net> wrote:
>> This.  I prefer we keep passing around the ctx variable to keep the code
>> more flexible for future reuse.  Of course, IIRC git itself has this
>> limitation, too...
>
> Can anyone confirm or deny this? Is it a pointless endeavor because of
> git's design?

Supporting something like FCGI in cgit will require a fork(2) for each
request, before invoking libgit.a functions, since these functions are
not generally reentrant (they tend to use global state and/or
inconveniently die(3)).

Therefore, passing a pointer to a context variable serves no real
purpose since each request would be processed in a different child
process (thus having a private copy of the global context).

But FCGI support could still be a nice addition to cgit, since parsing
of cgitrc and/or scanning for repos could then be done once, in the
parent process.

-- 
larsh

Policy on global variables

[normalperson]

Lars Hjemli <hjemli at gmail.com> wrote:
> On Thu, Jan 16, 2014 at 2:00 AM, Jason A. Donenfeld <Jason at zx2c4.com> wrote:
> > On Thu, Jan 16, 2014 at 1:59 AM, Eric Wong <normalperson at yhbt.net> wrote:
> >> This.  I prefer we keep passing around the ctx variable to keep the code
> >> more flexible for future reuse.  Of course, IIRC git itself has this
> >> limitation, too...
> >
> > Can anyone confirm or deny this? Is it a pointless endeavor because of
> > git's design?
> 
> Supporting something like FCGI in cgit will require a fork(2) for each
> request, before invoking libgit.a functions, since these functions are
> not generally reentrant (they tend to use global state and/or
> inconveniently die(3)).

Unfortunately true for now, but libgit.a could evolve (or cgit can use
something like libgit2 instead).

Policy on global variables

[Jason]

On Thu, Jan 16, 2014 at 11:47 AM, Eric Wong <normalperson at yhbt.net> wrote:
> Lars Hjemli <hjemli at gmail.com> wrote:
>> Supporting something like FCGI in cgit will require a fork(2) for each
>> request, before invoking libgit.a functions, since these functions are
>> not generally reentrant (they tend to use global state and/or
>> inconveniently die(3)).
>
> Unfortunately true for now, but libgit.a could evolve (or cgit can use
> something like libgit2 instead).

Cgit is unlikely to move to libgit2 in the near future. (Unless
someone is willing to do the job and argue for why it's preferred over
mainline git, beyond its reentrancy...)

I guess, though, libgit.a is likely to never evolve to receive
reentrant functions and do away with die() (though the die calls could
easily be circumvented by hooking libc's exit...yuck), because libgit2
exists for this reason.

I am therefore tempted to follow Lars' suggestion, and merge lf/global-ctx.

Policy on global variables

[john]

On Thu, Jan 16, 2014 at 12:31:15PM +0100, Jason A. Donenfeld wrote:
> On Thu, Jan 16, 2014 at 11:47 AM, Eric Wong <normalperson at yhbt.net> wrote:
> > Lars Hjemli <hjemli at gmail.com> wrote:
> >> Supporting something like FCGI in cgit will require a fork(2) for each
> >> request, before invoking libgit.a functions, since these functions are
> >> not generally reentrant (they tend to use global state and/or
> >> inconveniently die(3)).
> >
> > Unfortunately true for now, but libgit.a could evolve (or cgit can use
> > something like libgit2 instead).
> 
> Cgit is unlikely to move to libgit2 in the near future. (Unless
> someone is willing to do the job and argue for why it's preferred over
> mainline git, beyond its reentrancy...)
> 
> I guess, though, libgit.a is likely to never evolve to receive
> reentrant functions and do away with die() (though the die calls could
> easily be circumvented by hooking libc's exit...yuck), because libgit2
> exists for this reason.

I had a look at porting to libgit2 about a year ago and it mostly isn't
too bad.  IIRC the only problematic area is the graph output which we
currently get from libgit.a but would have to do ourselves if we switch
to libgit2.

Policy on global variables

[Jason]

On Thu, Jan 16, 2014 at 2:08 PM, John Keeping <john at keeping.me.uk> wrote:
>
> I had a look at porting to libgit2 about a year ago and it mostly isn't
> too bad.  IIRC the only problematic area is the graph output which we
> currently get from libgit.a but would have to do ourselves if we switch
> to libgit2.

Are there any downsides of doing this? I know we've put a lot of work
into cozying up with internal git utility functions and their build
system. Would we have to reimplement a lot of this? Would it be worth
it? Are there general benefits of using libgit2 over what we have now?
Are there general downsides?

More importantly, is this something you would be willing to do, if we
decided it was the best direction?

Policy on global variables

[john]

On Thu, Jan 16, 2014 at 07:38:02PM +0100, Jason A. Donenfeld wrote:
> On Thu, Jan 16, 2014 at 2:08 PM, John Keeping <john at keeping.me.uk> wrote:
> >
> > I had a look at porting to libgit2 about a year ago and it mostly isn't
> > too bad.  IIRC the only problematic area is the graph output which we
> > currently get from libgit.a but would have to do ourselves if we switch
> > to libgit2.
> 
> Are there any downsides of doing this? I know we've put a lot of work
> into cozying up with internal git utility functions and their build
> system. Would we have to reimplement a lot of this? Would it be worth
> it? Are there general benefits of using libgit2 over what we have now?
> Are there general downsides?

Given the CGit and Git are both GPLv2, we could always just take
strbuf.[ch] and the argv-array bits from git.git and copy them into
CGit.  Likewise the test suite could switch to using Sharness with very
little effort.

So I don't think the recent changes towards using more bits of Git
actually have too much impact here.

> More importantly, is this something you would be willing to do, if we
> decided it was the best direction?

I won't have time to do this in the near future.

The first step in this direction may actually be useful even if we stick
with embedding libgit.a.  Re-writing the commit graph drawing ourselves
could allow prettier output than the ASCII we inherit from Git.

Policy on global variables

[Jason]

On Thu, Jan 16, 2014 at 10:21 PM, John Keeping <john at keeping.me.uk> wrote:
> The first step in this direction may actually be useful even if we stick
> with embedding libgit.a.

So what do you think ought to be done with the global-ctx patch? Merge
it, and then refactor afterward (whenever we "step in this
direction"), to get rid of the global? Or use what we have now
(without the patch) as a starting point for gradually getting rid of
the global?

Policy on global variables

[john]

On Thu, Jan 16, 2014 at 10:26:08PM +0100, Jason A. Donenfeld wrote:
> On Thu, Jan 16, 2014 at 10:21 PM, John Keeping <john at keeping.me.uk> wrote:
> > The first step in this direction may actually be useful even if we stick
> > with embedding libgit.a.
> 
> So what do you think ought to be done with the global-ctx patch? Merge
> it, and then refactor afterward (whenever we "step in this
> direction"), to get rid of the global? Or use what we have now
> (without the patch) as a starting point for gradually getting rid of
> the global?

I'm not sure it makes much difference either way.  Even if we use
libgit2, providing we're not processing more than one request at once we
can still use a global cgit_context.

Policy on global variables

[Jason]

On Thu, Jan 16, 2014 at 10:34 PM, John Keeping <john at keeping.me.uk> wrote:
>
> I'm not sure it makes much difference either way.  Even if we use
> libgit2, providing we're not processing more than one request at once we
> can still use a global cgit_context.

Well, the idea of moving to libgit2, in the first place, would be to
benefit from its reentrancy, so that we could process multiple
requests at once (potentially).

Policy on global variables

[john]

On Thu, Jan 16, 2014 at 10:36:34PM +0100, Jason A. Donenfeld wrote:
> On Thu, Jan 16, 2014 at 10:34 PM, John Keeping <john at keeping.me.uk> wrote:
> >
> > I'm not sure it makes much difference either way.  Even if we use
> > libgit2, providing we're not processing more than one request at once we
> > can still use a global cgit_context.
> 
> Well, the idea of moving to libgit2, in the first place, would be to
> benefit from its reentrancy, so that we could process multiple
> requests at once (potentially).

At once (as in in parallel), or without needing to fork for every
request?  I think that many requests serially in the same process is a
much more likely scenario (that's what FastCGI does); in that case all
we need to do is clean up after each request and it doesn't make much
difference if that state is global or passed down through the functions
that need it.

Policy on global variables

[Jason]

On Thu, Jan 16, 2014 at 11:20 PM, John Keeping <john at keeping.me.uk> wrote:
> At once (as in in parallel), or without needing to fork for every
> request?  I think that many requests serially in the same process is a
> much more likely scenario (that's what FastCGI does); in that case all
> we need to do is clean up after each request and it doesn't make much
> difference if that state is global or passed down through the functions
> that need it.

Yea, that's a good point I suppose. Parallel request capability would
still be nice, in some abstract sense, but I guess it's not clear what
the benefits would be practically. I'll merge Lukas' patch, and we'll
refactor later if necessary.