Re: [PATCH 1/2] reflog: drop usage of global variables

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 7, 2025, 21:19 UTC
Message-ID: <xmqqo6ycczdr.fsf@gitster.g>
In-Reply-To: <20250307-493-add-command-to-purge-reflog-entries-v1-1-84ab8529cf9e@gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

Show 6 quoted lines

> The 'builtin/reflog.c' file uses the 'the_repository' global variable
> directly and also via 'git_config()'. Since this is a builtin command
> which has access to the 'struct repository', drop usage of the global
> variable and use the available repository struct.
>
> With this, remove the 'USE_THE_REPOSITORY_VARIABLE' macro from the file.

I suspect that this is not quite right.

    $ cd w/git.git/; make
    $ ./git-reflog list -h
    usage: git reflog list
    $ cd .. # not a repository
    $ git.git/git-reflog list -h
    fatal: not a git repository (or any of the parent directories): .git
    $ git.git/git-reflog -h
    usage: git reflog [show] ...

but I also suspect that it is mostly due to the original program structure that uses OPT_SUBCOMMAND() that the subcommands fail to respond to "-h" unlike the top-level command, so this may not be a regression. I do think however that this change is making it harder to fix.

In any case, when you are adding a new feature, I would strongly prefer you did *not* take it hostage to unrelated internal clean-up with a dubious value. For the library-ish parts of the system (e.g. reflog.c at the top-level), not depending on the_repository is absolutely the good thing to do, but the top level cmd_foo() are not meant to be called as helper functions repeatedly with arbirary repository instance, and a churn like this one, only to mollify the USE_THE_REPOSITORY_VARIABLE macro, does not deserve to take a more interesting (in the sense that it improves the life of end users) change hostage by pretending to be its prerequisite clean-up.

Thanks.