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
