On Wed, May 29, 2024 at 01:55:13PM -0700, Junio C Hamano wrote:

Patrick Steinhardt [off-list ref] writes:

quoted

We never free any of the config strings that we populate into the
`struct imap_server_conf`. Fix this by creating a common exit path where
we can free resources.

This is more like the previous step got rid of the anchor that made
these strings reachable, so we need to turn around to free them,
which is sort-of Meh, especially given that the leaked pieces of
memory are small and very much bounded.

The main benefit of this change is to allow us prepare on the
constness change in the other (read: API this thing uses from
elsewhere) parts of the system, which is a very worthy goal.

That's the motivation in this series at least. But I also see it as a
good goal by itself to get rid of the global state that we had before
the preceding patch. It may not be necessary, but it certainly helps me
personally to reason about code better.

quoted

While at it, drop the unused variables `imap_server_conf::name` and
`nongit_ok`.

The removal of the .name member may be correct, but I suspect the
change to nongit_ok is a change in behaviour, and it could even be a
regression.

quoted

-	setup_git_directory_gently(&nongit_ok);
+	setup_git_directory_gently(NULL);

The general idea behind &nongit_ok is that

 - Usually setup_git_directory_gently() dies if NULL is passed
   instead of a pointer to &nongit_ok.  Most of the Git command
   wants to make sure they have a repository to operate on, so this
   is a reasonable default behaviour.

 - Some commands would want to work also without having any
   repository, possibly with limited capability (e.g., "git apply"
   may want to work as a better "GNU patch", but obviously it cannot
   do "git apply --binary --3way" without having the object
   database).  They tell setup_git_directory_gently() not to die
   when outside a repository by passing a pointer to &nongit_ok, and
   instead tell if we are in a repository by storing 0/1 in it.

The idea is that a command that is willing to work outside a
repository can disable selected features based on what it sees in
nongit_ok.  In the case of "imap-send", there is no such features
that it needs to special case, perhaps because everything it does is
supposed to work outside a repository?

So the short version of what worries me in this change is that we
used to be able to operate without having a repository at all, but
now we would barf if run outside a repository, no?

Oh, I wasn't aware that the parameter being `NULL` actually causes a
change in behaviour. Which nicely demonstrates that we have some missing
test coverage for git-imap-send(1).

In fact, it's not only "some". We don't have any test coverage at all
for git-imap-send(1) as far as I can see. Which does make me rest a bit
uneasy. And I suspect that it wouldn't be trivial to add given that it
kind of requires something that talks IMAP on the receiving end.

Patrick