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

On 2022.06.29 20:40, Derrick Stolee via GitGitGadget wrote:

quoted

From: Derrick Stolee <redacted>
+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!

quoted

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

quoted

+{
+	/* Copy as a file */
+	return copy_file(file, uri, 0444);

...we have exactly that standard usage right here.

quoted

+	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