On Wed, Nov 18, 2020 at 1:30 PM Derrick Stolee <stolee@xxxxxxxxx> wrote:
> On 11/18/2020 2:15 AM, Eric Sunshine wrote:
> > On Tue, Nov 17, 2020 at 4:13 PM Derrick Stolee via GitGitGadget
> > <gitgitgadget@xxxxxxxxx> wrote:
> >> + xmlpath = xstrfmt("%s/schedule-%s.xml",
> >> + the_repository->objects->odb->path,
> >> + frequency);
> >
> > I missed this in the earlier rounds since I wasn't paying close enough
> > attention, but placing this XML file within the object database
> > directory (.git/objects/) feels rather odd, even if it is just a
> > temporary file. Using the .git/ directory itself might be better,
> > perhaps like this:
>
> It does look odd, and in this case we could use the .git directory
> instead. I specifically use the objects directory for the maintenance
> lock in 'git maintenance run' to allow maintenance to run when
> GIT_OBJECT_DIRECTORY points to an alternate, allowing us to maintain
> object databases that don't have a full .git directory around them.
I guess I'm confused. Won't a Git "common" directory exist even for
such a case when GIT_OBJECT_DIRECTORY is pointing elsewhere, whether
the "common" directory is .git/ or a bare repository, or whatnot?
Anyhow, this brings us back to my original suggestion of creating
these temporary files in a genuine temporary directory (/tmp or
$TMPDIR or $TEMP) instead of arbitrarily choosing a path within the
repository itself. An important reason for using a genuine temporary
directory for these temporary XML files is that it makes it less
confusing for those who come along later and try to understand this
code; they won't have to puzzle out why it is using a repository
location for a file which is clearly temporary.
To make this really simple, you could use one of the
x?mks_tempfile_t*() functions from tempfile.h which will automatically
place the file in $TMPDIR, thus relieving this code from having to
make the choice. Doing so would simplify this code even further since
you would replace create_tempfile() with x?mks_tempfile_t*(), and
wouldn't have to maintain (or free) `xmlpath` manually.
As for the test script, the `print-args` is already picking up the
pathname of the temporary file specified by the /xml option, so it
should be possible to make the rest of the test work with the
generated temporary filename.
