diff mbox series

[v4,29/39] http-fetch: set up git directory before parsing pack hashes

Message ID 20200726195424.626969-30-sandals@crustytoothpaste.net (mailing list archive)
State New, archived
Headers show
Series [v4,01/39] t: make test-bloom initialize repository | expand

Commit Message

brian m. carlson July 26, 2020, 7:54 p.m. UTC
In dd4b732df7 ("upload-pack: send part of packfile response as uri",
2020-06-10), the git http-fetch code learned how to take  ac --packfile
option.  This option takes an argument, which is the name of a packfile
hash, and parses it using parse_oid_hex.  It does so before calling
setup_git_directory.

However, in a SHA-256 repository this fails to work, since we have not
set the hash algorithm in use and parse_oid_hex fails as a consequence.
To ensure that we can parse packfile hashes of the right length, let's
set up the git directory before we start parsing arguments.
---
 http-fetch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Eric Sunshine July 26, 2020, 11:28 p.m. UTC | #1
On Sun, Jul 26, 2020 at 3:56 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> In dd4b732df7 ("upload-pack: send part of packfile response as uri",
> 2020-06-10), the git http-fetch code learned how to take  ac --packfile

s/take\s+ac/take a/

> option.  This option takes an argument, which is the name of a packfile
> hash, and parses it using parse_oid_hex.  It does so before calling
> setup_git_directory.
>
> However, in a SHA-256 repository this fails to work, since we have not
> set the hash algorithm in use and parse_oid_hex fails as a consequence.
> To ensure that we can parse packfile hashes of the right length, let's
> set up the git directory before we start parsing arguments.
> ---

Missing sign-off.

> diff --git a/http-fetch.c b/http-fetch.c
> @@ -86,6 +86,8 @@ int cmd_main(int argc, const char **argv)
> +       setup_git_directory();
> +
>         while (arg < argc && argv[arg][0] == '-') {
>                 const char *p;
> @@ -115,8 +117,6 @@ int cmd_main(int argc, const char **argv)
>         if (argc != arg + 2 - (commits_on_stdin || packfile))
>                 usage(http_fetch_usage);
>
> -       setup_git_directory();

This change has the unfortunate side-effect that "git http-fetch -h"
will no longer work if setup_git_directory() fails, which could easily
be the case if the user is trying to get help for the command from
some arbitrary location in the filesystem.

It might be better to setup_git_directory() on-demand when it's needed
by --packfile= rather than doing it unconditionally before the
argument-parsing loop.
Eric Sunshine July 26, 2020, 11:30 p.m. UTC | #2
On Sun, Jul 26, 2020 at 7:28 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> This change has the unfortunate side-effect that "git http-fetch -h"
> will no longer work if setup_git_directory() fails, which could easily
> be the case if the user is trying to get help for the command from
> some arbitrary location in the filesystem.
>
> It might be better to setup_git_directory() on-demand when it's needed
> by --packfile= rather than doing it unconditionally before the
> argument-parsing loop.

Better yet, use setup_git_directory_gently() before entering the
argument-parsing loop.
diff mbox series

Patch

diff --git a/http-fetch.c b/http-fetch.c
index 1df376e745..8db7eb669f 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -86,6 +86,8 @@  int cmd_main(int argc, const char **argv)
 	int packfile = 0;
 	struct object_id packfile_hash;
 
+	setup_git_directory();
+
 	while (arg < argc && argv[arg][0] == '-') {
 		const char *p;
 
@@ -115,8 +117,6 @@  int cmd_main(int argc, const char **argv)
 	if (argc != arg + 2 - (commits_on_stdin || packfile))
 		usage(http_fetch_usage);
 
-	setup_git_directory();
-
 	git_config(git_default_config, NULL);
 
 	if (packfile) {