Re: [PATCH] builtin: replace the_repository parameter in is_bare_repository()

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

"Hardik Kumar" <hardikxk@gmail.com> writes:

>> In general, builtin/foo.c::cmd_foo() are concrete programs that work
>> on specific repository (i.e., the_repository), and there is not much
>> reason to rewrite the use of the_repository to use "repo" given by
>> the caller which is git potty.  You'd also need to deal with the
>> case where "repo" is NULL (hint: "cd / && git foo -h").
>
> Right, but would safety check be required for single instance or better
> to find and work on only the specific ones which could lead to an
> exception.

I do not quite get what you mean.  In a single function you use
the_repository and repo interchangeably, relying on repo, when it is
not NULL), being the same as the_repository for correctness.  If
they ever refer to different things, then your updated code is less
consistent than the original, which would not be an improvement.

I actually think a good medium size project is to fix the last
parameter given to cmd_foo() built-in command implementations that
is a pointer to "struct repository".  It was a misguided design.

This pointer is either NULL or the_repository; it never takes any
other value.  It is misleading and invites confusion that these
functions can take arbitrary repository instance.  It also invites
people to replace all references in cmd_foo() to "the_repository"
with "repo", which may lead to an error.  Depending on where
parse_options() call appears in the cmd_foo(), "cd / && git foo -h"
will cause segfaults with such a change.

If we change the function signature of cmd_foo() to receive a "bool"
that says "true" if the command was run inside a repository and
"false" otherwise, cmd_foo() can still tell if the command was run
outside a repository, and programmers will not be misled to use
anything other than "the_repository" as the repository to work on.

The utility functions builtin/foo.c borrows from outside builtin/
directory are being "libified" to reduce the hardcoded dependence on
the_repository, and cmd_foo() can call these functions with
the_repository as a parameter.  But we have no reason to waste our
time updating (and also reviewing patches that make such updates)
the built-in implementations themselves to take a pointer to an
arbitrary repository.