Re: [PATCH] grep: remove "repo" arg from non-supporting funcs

> On Tue, 27 Mar 2018 16:20:25 -0700
> Stefan Beller <sbeller@google.com> wrote:
>
>> On Tue, Mar 27, 2018 at 3:58 PM, Jonathan Tan <jonathantanmy@google.com> wrote:
>> > As part of commit f9ee2fcdfa ("grep: recurse in-process using 'struct
>> > repository'", 2017-08-02), many functions in builtin/grep.c were
>> > converted to also take "struct repository *" arguments. Among them were
>> > grep_object() and grep_objects().
>> >
>> > However, at least grep_objects() was converted incompletely - it calls
>> > gitmodules_config_oid(), which references the_repository.
>> >
>> > But it turns out that the conversion was extraneous anyway - there has
>> > been no user-visible effect - because grep_objects() is never invoked
>> > except with the_repository.
>> >
>> > Revert the changes to grep_objects() and grep_object() (which conversion
>> > is also extraneous) to show that both these functions do not support
>> > repositories other than the_repository.
>>
>> I'd rather convert gitmodules_config_oid instead of reverting the other
>> functions into a world without an arbitrary repository object.
>
> I don't really think of it as a reversion, since grep_objects() didn't
> fully support repos other than the_repository anyway.
>
> Besides that, that's fine, but then:
>
>  (1) You would have to explain both in the gitmodules_config_oid() and
>      in this patch why "gitmodules_config_oid(...)" ->
>      "gitmodules_config_oid(repo, ...)" and "submodule_free()" ->
>      "submodule_free(repo)" are safe, since they have different behavior
>      upon first glance. (They have the same behavior only because
>      grep_objects() is always called with the_repository.) I was trying
>      to explain this in as clear a way as possible, by showing (with
>      code) that grep_objects() only works with, and is only called with,
>      the_repository.
>  (2) The code path where repo != the_repository would still never be
>      exercised, and I prefer to not have such code paths.
>
> I don't feel too strongly about (1), since they just concern commit
> messages, of which there are many valid opinions on how to write them. I
> feel a bit more strongly about (2), but can concede my point if the
> project is OK with a code path not being exercised.