From: Karthik Nayak <karthik.188@gmail.com>
Date: Mon, 10 Mar 2025 11:41:26 GMT
Subject: Re: [PATCH 1/2] reflog: drop usage of global variables
Message-ID: <CAOLa=ZRqMnus=MZV1ASce4pxnMkVRGqhZZ+vZNcGhWdUQE-JRg@mail.gmail.com>
In-Reply-To: <xmqqo6ycczdr.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> 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.
>

Hmm. But this is the existing behavior, no?

  # Inside a git directory
  $ eza .git
  b4.template  branches  COMMIT_EDITMSG  config  description
FETCH_HEAD  filter-repo  HEAD  hooks  index  info  objects  refs
reftable  rr-cache

  $ git reflog -h
  usage: git reflog [show] [<log-options>] [<ref>]
     or: git reflog list
     or: git reflog expire [--expire=<time>] [--expire-unreachable=<time>]
                           [--rewrite] [--updateref] [--stale-fix]
                           [--dry-run | -n] [--verbose] [--all
[--single-worktree] | <refs>...]
     or: git reflog delete [--rewrite] [--updateref]
                           [--dry-run | -n] [--verbose] <ref>@{<specifier>}...
     or: git reflog exists <ref>

  $ git reflog list -h
  usage: git reflog list

  $ ~/code/git/build/bin-wrappers/git reflog -h
  usage: git reflog [show] [<log-options>] [<ref>]
     or: git reflog list
     or: git reflog expire [--expire=<time>] [--expire-unreachable=<time>]
                           [--rewrite] [--updateref] [--stale-fix]
                           [--dry-run | -n] [--verbose] [--all
[--single-worktree] | <refs>...]
     or: git reflog delete [--rewrite] [--updateref]
                           [--dry-run | -n] [--verbose] <ref>@{<specifier>}...
     or: git reflog exists <ref>
     or: git reflog drop [--all | <refs>...]

  $ ~/code/git/build/bin-wrappers/git reflog list -h
  usage: git reflog list

  # Outside a git repository
  $ eza .git
  ".git": No such file or directory (os error 2)

  $ git reflog -h
  usage: git reflog [show] [<log-options>] [<ref>]
     or: git reflog list
     or: git reflog expire [--expire=<time>] [--expire-unreachable=<time>]
                           [--rewrite] [--updateref] [--stale-fix]
                           [--dry-run | -n] [--verbose] [--all
[--single-worktree] | <refs>...]
     or: git reflog delete [--rewrite] [--updateref]
                           [--dry-run | -n] [--verbose] <ref>@{<specifier>}...
     or: git reflog exists <ref>


  $ git reflog list -h
  fatal: not a git repository (or any of the parent directories): .git

  $ ~/code/git/build/bin-wrappers/git reflog -h
  usage: git reflog [show] [<log-options>] [<ref>]
     or: git reflog list
     or: git reflog expire [--expire=<time>] [--expire-unreachable=<time>]
                           [--rewrite] [--updateref] [--stale-fix]
                           [--dry-run | -n] [--verbose] [--all
[--single-worktree] | <refs>...]
     or: git reflog delete [--rewrite] [--updateref]
                           [--dry-run | -n] [--verbose] <ref>@{<specifier>}...
     or: git reflog exists <ref>
     or: git reflog drop [--all | <refs>...]


  $ ~/code/git/build/bin-wrappers/git reflog list -h
  fatal: not a git repository (or any of the parent directories): .git

Seems like the behavior is the same with as with (git 2.48.1).

> 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.

But point taken, I'll drop this patch in the next version! Thanks