Re: [PATCH v3 09/11] bundle-uri: allow relative URLs in bundle lists

From: Victoria Dye <vdye@github.com>
Date: Dec 5, 2022, 23:33 UTC
Message-ID: <2b21f22b-b707-7705-a41c-9e4acbdb0405@github.com>
In-Reply-To: <48731438d6a289129a5768b80af856fb49692426.1670262639.git.gitgitgadget@gmail.com>

Derrick Stolee via GitGitGadget wrote:

Show 6 quoted lines

> Allow a bundle list to specify a relative URI for the bundles. This URI
> is based on where the client received the bundle list. For a list
> provided in the 'bundle-uri' protocol v2 command, the Git remote URI is
> the base URI. Otherwise, the bundle list was provided from an HTTP URI
> not using the Git protocol, and that URI is the base URI. This allows
> easier distribution of bundle data.

Thanks, this clears up my confusion about the source of 'baseURI'.

Show 8 quoted lines

> +	/**
> +	 * The baseURI of a bundle_list is the URI that provided the list.
> +	 *
> +	 * In the case of the 'bundle-uri' protocol v2 command, the base
> +	 * URI is the URI of the Git remote.
> +	 *
> +	 * Otherewise, the bundle list was downloaded over HTTP from some
> +	 * known URI.

s/Otherewise/Otherwise

Also, this sentence is a bit more vague than what was noted in the commit message; it doesn't actually say what the base URI is set to in this scenario. Feel free to ignore if you think it's overkill, but that could probably be cleared up by adding another sentence after like "The base URI is set to that known URI."

Show 5 quoted lines

> +	 *
> +	 * The baseURI is used as the base for any relative URIs
> +	 * advertised by the bundle list at that location.
> +	 */
> +	char *baseURI;

...

Show 7 quoted lines

> +	# TODO: We would prefer if parsing a bundle list would not cause
> +	# a die() and instead would give a warning and allow the rest of
> +	# a Git command to continue. This test_must_fail is necessary for
> +	# now until the interface for relative_url() allows for reporting
> +	# an error instead of die()ing.
> +	test_must_fail test-tool bundle-uri parse-key-values in >actual 2>err &&
> +	grep "fatal: cannot strip one component off url" err

Thanks for adding this, I'm content to leave this as a TODO for now.