On 7/21/2022 5:45 PM, Josh Steadmon wrote:
> On 2022.06.29 20:40, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <derrickstolee@xxxxxxxxxx>
>> +static void find_temp_filename(struct strbuf *name)
>> +{
>> + int fd;
>> + /*
>> + * Find a temporary filename that is available. This is briefly
>> + * racy, but unlikely to collide.
>> + */
>> + fd = odb_mkstemp(name, "bundles/tmp_uri_XXXXXX");
>> + if (fd < 0)
>> + die(_("failed to create temporary file"));
>> + close(fd);
>> + unlink(name->buf);
>
> Is there a reason why we unlink() here? If we allow the empty file to
> remain on-disk until we write to it, wouldn't that prevent odb_mkstemp()
> from being racy?
I still need to test this, but that should work. Thanks!
>> +static int copy_uri_to_file(const char *uri, const char *file)
>
> Nitpick: from a brief glance, it seems that most other copy* functions
> take the destination as the first parameter, and the source second. I
> don't feel strongly about it, because to me src followed by dst feels
> more natural, but perhaps we should be consistent with other functions.
Yeah, this is definitely my personal option that (src, dst) feels more
natural to me and I need to do a mental swap whenever dealing with the
standard methods. However, it's best to be consistent, and...
>
>> +{
>> + /* Copy as a file */
>> + return copy_file(file, uri, 0444);
...we have exactly that standard usage right here.
>> + if ((result = unbundle(r, &header, bundle_fd, &extra_index_pack_args)))
>
> Can we just pass NULL here instead of creating an empty
> extra_index_pack_args?
This isn't the first time I've populated an option instead of just
passing NULL. I'll work on fixing that bad habit.
Thanks,
-Stolee
