On Fri, Aug 22, 2014 at 10:23 PM, Jeff King <peff@xxxxxxxx> 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 <sahlberg@xxxxxxxxxx> > -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html