John Cai <johncai86@xxxxxxxxx> 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.
