Re: [PATCH 3/4] apply: remove the_repository global variable

[Junio C Hamano <gitster@pobox.com>]

John Cai <johncai86@gmail.com> writes:

>> > -     if (init_apply_state(&state, the_repository, prefix))
>> > +     if (init_apply_state(&state, repo, prefix))
>> >               exit(128);
>>
>> Hmph, the reason why we do not segfault with this patch is because
>> repo will _always_ be the_repository due to the previous change.
>>
>> I am not sure if [1/4] is an improvement, though.  We used to be
>> able to tell if we were running in a repository, or we were running
>> in "nongit" mode, by looking at the NULL-ness of repo (which was
>> UNUSED because we weren't taking advantage of that).
>>
>> With [1/4], it no longer is possible.  From the point of view of API
>> to call into builtin implementations, it smells like a regression.
>
> I see your point here. However, I was wondering about this because
> we are passing in the_repository through run_builtin() as repo--so wouldn't
> this be equivalent to using the_repository and hence the
> same API contract can remain that looks at the NULL-ness of repo?
>
> But I could be missing something here.

As run_builtin() discards the value of nongit, we will always see
repo == the_repository passed to this function, whether _gently()
found that we are in or not in a repository.  I think Patrick also
noticed it and suggested to pass repo or NULL conditionally in a
separate message, and if that is done, then I am fine.  I do not
think your [1/4] as-is did that.

Thanks.