Re: [PATCH 4/6] receive-pack: quarantine objects until pre-receive accepts

From: Jeff King <peff@peff.net>
Date: Oct 1, 2016, 09:12 UTC
Message-ID: <20161001091245.jkju5rlytbcb2r52@sigill.intra.peff.net>
In-Reply-To: <20160930193630.vwq6n5hjbthrz6fj@sigill.intra.peff.net>

On Fri, Sep 30, 2016 at 03:36:30PM -0400, Jeff King wrote:

Show 8 quoted lines

> @@ -1639,6 +1666,18 @@ static const char *unpack(int err_fd, struct shallow_info *si)
>  		argv_array_push(&child.args, alt_shallow_file);
>  	}
>  
> +	tmp_objdir = tmp_objdir_create();
> +	if (!tmp_objdir)
> +		return "unable to create temporary object directory";
> +	child.env = tmp_objdir_env(tmp_objdir);

One thing to note here: this new code kicks in all the time. My reasoning was that there's basically no time you _wouldn't_ want it to, and certainly that was the case for us when I wrote it. But I tried to think of user-visible changes. Here's what I came up with:

  - we currently leave the tmp_pack_* for a failed push sitting around
    (e.g., if the client hangs up halfway through, or index-pack rejects
    the pack for some reason). But with this series, it would always be
    cleaned up. That's a very good thing if you're running a git hosting
    site. It might make things harder if you're debugging.

    I don't think it's a good reason not to enable this by default, but
    it _could_ be a reason to have a config switch to turn it off
    temporarily (or just leave the "incoming-*" directory in place).

  - the environment that pre-receive pack runs in has access to objects
    that the rest of the repository doesn't. So if you were to do
    something silly in your pre-receive like:

      # reject the push, but log a copy of the objects
      git update-ref refs/rejected/$(date +%s) $new_sha1
      exit 1

    Then your ref-update would succeed (you have $new_sha1), but the
    objects would be deleted immediately afterwards. I find this a
    somewhat questionable pattern, and I have no idea if anybody else
    has thought of it. But it _does_ work today, and not with this
    series.

I don't think it would be too hard to put a config conditional around this tmp_objdir_create(). And then all of the tmp_objdir_env() calls would just return NULL, and effectively become noops.

-Peff