On Wed, May 29, 2024 at 01:55:13PM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@xxxxxx> writes: > > > 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. > > 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. > > > - 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
Attachment:
signature.asc
Description: PGP signature
