fix pack-refs pruning of refs/foo
Get series with b4
b4 am [email protected]Versions newest first
- v1 Latest5 patches 8 repliesIn review
Patches in v1 No applies recorded yet
| # | Subject and files | Size | Replies | Applied |
|---|---|---|---|---|
| 1/5 | git-prompt: do not look for refs/stash in $GIT_DIRcontrib/ | +2 | 0 replies | |
| 2/5 | pack-refs: prune top-level refs like "refs/foo"refs.c t/ | +9 | 0 replies | |
| 3/5 | fast-import: clean up pack_data pointer in end_packfilefast-import.c | +7 | 2 replies | |
| 4/5 | fast-import: fix buffer overflow in dump_tagsfast-import.c | +6 | 1 reply | |
| 5/5 | fast-import: stop using lock_ref_sha1fast-import.c t/ | +6 | 1 reply |
Reviews trailers per patch
None of the 5 patches has a Reviewed-by or Acked-by of their own; the series has one for all of them.
| Patch | Ronnie Sahlberg |
|---|---|
| Series | R-b |
| 1/5 | |
| 2/5 | |
| 3/5 | |
| 4/5 | |
| 5/5 |
Trailers given in replies are filled; those carried in the patches from earlier versions are outlined with dashes.
Cover letter [PATCH 0/5] fix pack-refs pruning of refs/foo
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
-Peff