Re: [PATCH v2 1/1] bundle: cleanup lock files on error

From: Martin Ågren <martin.agren@gmail.com>
Date: Nov 14, 2018, 21:43 UTC
Message-ID: <CAN0heSqsjKksKnBHjffVMDEyX4A2DAY6Naw4tbBXEm+AdhOLtA@mail.gmail.com>
In-Reply-To: <c88887f05a145709be9e86d56f4c1e620eb5ea89.1542209112.git.gitgitgadget@gmail.com>

On Wed, 14 Nov 2018 at 16:26, Gaël Lhez via GitGitGadget <gitgitgadget@gmail.com> wrote:

> However, the `.lock` file was still open and on Windows that means
> that it could not be deleted properly. This patch fixes that issue.

Hmmm, doesn't the tempfile machinery remove the lock file when we die?

Show 9 quoted lines

>         ref_count = write_bundle_refs(bundle_fd, &revs);
> -       if (!ref_count)
> -               die(_("Refusing to create empty bundle."));
> -       else if (ref_count < 0)
> +       if (ref_count <= 0)  {
> +               if (!ref_count)
> +                       error(_("Refusing to create empty bundle."));
>                 goto err;
> +       }

One less `die()` in libgit -- nice.

> +test_expect_success 'try to create a bundle with empty ref count' '
> +       test_expect_code 1 git bundle create foobar.bundle master..master
> +'

This tries to make sure that we don't just die, but that we exit in a "controlled" way with exit code 1. After reading the log message, I had perhaps expected something like

  test_must_fail git bundle [...] &&
  test_path_is_missing foobar.bundle.lock

That relies on magically knowing the ".lock" suffix, but my point is that for me (on Linux), this alternative test passes both before and after the patch. Before, because tempfile.c cleans up; after, because bundle.c does so. Doesn't this happen on Windows too? What am I missing?

Martin