Christian Couder <christian.couder@xxxxxxxxx> writes:
>> IMHO this config thing is a good example of the strength of the separate
>> "show" process. If our goal is to trigger all the niceties of "git
>> show", it is tricky to catch them all. The revision machinery is pretty
>> reusable, but there's no easy way to figure out which config affects
>> git-show and so on. Of course if we had a way to invoke git-show
>> in-process that would work, but I suspect there are unexpected corner
>> cases that might trigger.
>
> Sorry for not following the topic closely and for replying to this so
> late, but I think that by now we should have some kind of guidelines
> about when forking a new process is Ok and when it's not.
I thought we had passed that stage long ago. A case like this one
we see in this patch, where it is run just once immediately before
we give control back to the end-user (as opposed to "gets run each
time in a tight loop"), I would see it a no-brainer to discount the
"fork+exec is so expensive" objection more than we would otherwise,
especially when the upside of running an external command is so much
bigger.
There actually should be a different level of "running it as a
separate command" that we do not have. If we can split out and
encapsulate the global execution context sufficiently into a "bag of
state variables" structure, and rewrite cmd_foo() for each such
command we wish to be able to run from inside an executing Git into
two parts:
- cmd_foo() that prepares the global execution context to a
"pristine" state, calls into cmd__foo() with that "bag of state
variables" structure as one of the parameters, and exits when
everything is done.
- cmd__foo() that does the rest, including reading the
configuration files, parsing of the command line arguments to
override them, doing the actual work.
then the codepath we are changing from using diff-tree to show can
do something like:
struct git_global_state state = GIT_GLOBAL_STATE_INIT;
struct strvec args = STRVEC_INIT;
strvec_pushl(&args, ...);
cmd__show(&state, args.nr , args.v);
and expect that cmd__show() will do the _right thing_, right?
And to reach that ultimate goal, I do not think using run_command()
API in the meantime poses hindrance. The real work should be in the
implementation of cmd__show(), not the open-coded use of revisions
API at each such point where you are tempted to spawn an external
command via run_command() API, which will have to be consolidated
and replaced with a call to cmd__show() anyway.
