Re: [PATCH v5 29/39] http-fetch: set up git directory before parsing pack hashes

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 29, 2020, 02:36 UTC
Message-ID: <CAPig+cSP-NKjBHfD+REAUECJm4qjhKMc0vhYg9rMaeapWSTKWg@mail.gmail.com>
In-Reply-To: <20200728233446.3066485-30-sandals@crustytoothpaste.net>

On Tue, Jul 28, 2020 at 7:35 PM brian m. carlson <sandals@crustytoothpaste.net> wrote:

Show 24 quoted lines

> 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.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> 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_gently(NULL);
> +
>         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();

In the previous version of this series the setup_git_directory() call was moved verbatim above the 'while' loop, and my review pointed out that doing so made "git-http-fetch -h" fail when not in a Git repository. It was recommended, therefore, to take advantage of setup_git_directory_gently() to fix this problem.

But note that setup_git_directory() is defined like this:

    const char *setup_git_directory(void)
    {
        return setup_git_directory_gently(NULL);
    }

So, your use of setup_git_directory_gently(NULL) in this version of the patch is no better than the plain setup_git_directory() call in the previous version. It is still impossible to ask git-http-fetch for help when not in a Git repository.

To fix this, you must pass non-NULL for the 'nongit' argument of setup_git_directory_gently().

For robustness, in order to retain the original behavior (prior to this patch), it would be best to check the value of 'nongit' _after_ the 'while' loop and error out if not in a repository. Perhaps something like this (taking a hint from builtin/diff.c):

    setup_git_directory_gently(&nongit);

    while (...) {
        ...
    }

    if (nongit)
        die(_("not a git repository"));