On 11/18/2020 3:54 PM, Eric Sunshine wrote: > 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? The reason to use the object dir for the 'git maintenance run' lock is to avoid multiple enlistments pointing at a common alternate from running concurrent maintenance on the same object directory. That doesn't really apply to the temp files in this patch. > 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. I'll adopt your recommendations here. Thanks. -Stolee