On Sat, 29 Jul 2006, Junio C Hamano wrote:
>
> > +#define NEEDS_PREFIX 1
> >...
> > { "init-db", cmd_init_db },
>
> This is an oddball that wants to always use $GIT_DIR environment
> or "./.git" if $GIT_DIR is not exported, and should never go
> looking for GIT_DIR, so we should not give NEEDS_PREFIX, ever.
Well, the way I did things, each command _can_ still decide to call
"setup_git_directory()" on their own, and we don't actually say up front
whether they will or not.
So some commands will just choose to let the git.c wrapper do it for them,
and others can do it their own way, which leaves the maximum of
flexibility, and doesn't mean that odd-balls like "init-db" really have to
even be an issue.
> > { "stripspace", cmd_stripspace },
> > { "mailsplit", cmd_mailsplit },
> > { "get-tar-commit-id", cmd_get_tar_commit_id },
>
> These are the other extreme -- they really do not care about
> operating in a git context.
Right.
> { "push", cmd_push },
> { "count-objects", cmd_count_objects },
> { "check-ref-format", cmd_check_ref_format },
>
> One rule seems to be that the commands that work at the while
> repository level do not have NEEDS_PREFIX -- they will never
> work from a subdirectory.
Well, maybe we will make them do so, and maybe we won't.
The way I chose the NEED_PREFIX ones was actually really simple and
totally automated: I changed every builtin function to first _not_ have
NEEDS_PREFIX, but changed the prototype to take "const char *prefix"
instead of the "char **envp" that nobody actually used.
I then compiled them, and for each function that _already_ had a "prefix"
in their existing implementation (which the compiler warned about), I
just removed the old "prefix = setup_git_directory()", and added the
NEEDS_PREFIX flag for that command.
> There is no technical reason not to let count-objects find
> $GIT_DIR on its own, but the current implementation does not do
> that and you are keeping the behaviour bug-to-bug compatible.
Exactly. My conversions (both the first smaller one, and the second thing)
were really totally mindless translations, nothing fancy. I fixed a few
bugs as I hit them (ie the first one moved the "setup_git_directory()"
call earlier over some "git_config()" calls), but other than those kinds
of local changes, I tried to keep not just the patch, but very much the
process of me creating it, very straightforward.
> > { "rev-list", cmd_rev_list },
>
> I think we should mark this with NEEDS_PREFIX, and lose the call
> to setup_git_directory() it has here:
I think you're right, and the above explanation of how the patch was
mindlessly created explains why my patch didn't do it that way ;)
When I do re-organizations, I like doing them in two stages: the
"mindless" stage that does the bulk of the syntactic stuff and the
movement of code, but that pretty much by definition should not introduce
any new bugs, and then the second stage is the "ok, now we have the new
organization, we can clean up and fix problems".
Linus
-
: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html