Re: [PATCH v2 3/6] bundle-uri: create basic file-copy logic

From: Derrick Stolee <derrickstolee@github.com>
Date: Jul 22, 2022, 13:18 UTC
Message-ID: <ea70aea1-139e-ab92-6dfb-b9442e915383@github.com>
In-Reply-To: <YtnJEm1N5IQ/y3mj@google.com>

On 7/21/2022 5:45 PM, Josh Steadmon wrote:

Show 18 quoted lines

> On 2022.06.29 20:40, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <derrickstolee@github.com>
>> +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!

Show 6 quoted lines

>> +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