Re: [PATCH 0/5] fix pack-refs pruning of refs/foo

On Fri, Aug 22, 2014 at 10:23 PM, Jeff King [off-list ref] wrote:

I noticed that "git pack-refs --all" will pack a top-level ref like
"refs/foo", but will not actually prune "$GIT_DIR/refs/foo". I do not
see the point in having a policy not to pack "refs/foo" if "--all" is
given. But even if we did have such a policy, this seems broken; we
should either pack and prune, or leave them untouched. I don't see any
indication that the existing behavior was intentional.

The problem is that pack-refs's prune_ref calls lock_ref_sha1, which
enforces this "no toplevel" behavior. I am not sure there is any real
point to this, given that most callers use lock_any_ref_for_update,
which is exactly equivalent but without the toplevel check.

The first two patches deal with this by switching pack-refs to use
lock_any_ref_for_update. This will conflict slightly with Ronnie's
ref-transaction work, as he gets rid of lock_ref_sha1 entirely, and
moves the code directly into prune_ref. This can be trivially resolved
in favor of my patch, I think.

The third patch is a cleanup I noticed while looking around, and I think
should not conflict with anyone (and is a good thing to do).

The last two are trickier. I wondered if we could get rid of
lock_ref_sha1 entirely. After pack-refs, there are two callers:
fast-import.c and walker.c. After converting the first, it occurred to
me that Ronnie might be touching the same areas, and I see that yes,
indeed, there's quite a bit of conflict (and he reaches the same end
goal of dropping it entirely).

So in that sense I do not mind dropping the final two patches. Ronnie's
endpoint is much better, moving to a ref_transaction. However, there is
actually a buffer overflow in the existing code. Ronnie's series fixes
it in a similar way (moving to a strbuf), and I'm fine with that
endpoint. But given that the ref transaction code is not yet merged (and
would certainly never be maint-track), I think it is worth applying the
buffer overflow fix separately.

I think the final patch can probably be dropped, then. It is a clean-up,
but one that we can just get when Ronnie's series is merged.

  [1/5]: git-prompt: do not look for refs/stash in $GIT_DIR
  [2/5]: pack-refs: prune top-level refs like "refs/foo"
  [3/5]: fast-import: clean up pack_data pointer in end_packfile
  [4/5]: fast-import: fix buffer overflow in dump_tags
  [5/5]: fast-import: stop using lock_ref_sha1

+1 on 1-3
+1 on 4. While I have a similar fix in the transaction series, you
should not need to wait for that series to address a security concern.
5: I think this one is not as urgent as the others so would prefer if
it is dropped, just so it doesn't cause more merge conflicts than is
already present in the transaction series.


1-4:
Reviewed-by: Ronnie Sahlberg <redacted>

-Peff