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

From: Derrick Stolee <derrickstolee@github.com>
To: Josh Steadmon <steadmon@google.com>,
	Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, gitster@pobox.com, me@ttaylorr.com,
	newren@gmail.com, avarab@gmail.com, dyroneteng@gmail.com,
	Johannes.Schindelin@gmx.de
Subject: Re: [PATCH v2 3/6] bundle-uri: create basic file-copy logic
Date: Fri, 22 Jul 2022 09:18:25 -0400	[thread overview]
Message-ID: <ea70aea1-139e-ab92-6dfb-b9442e915383@github.com> (raw)
In-Reply-To: <YtnJEm1N5IQ/y3mj@google.com>

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@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!

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