Re: [PATCH 0/5] fix pack-refs pruning of refs/foo
On Fri, Aug 22, 2014 at 10:23 PM, Jeff King p...@peff.net 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 sahlb...@google.com -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] fix pack-refs pruning of refs/foo
On Mon, Aug 25, 2014 at 10:38:56AM -0700, Ronnie Sahlberg wrote: [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. OK, I think we're in agreement then. Thanks for looking them over. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] fix pack-refs pruning of refs/foo
On 08/23/2014 07:23 AM, Jeff King 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 patches 1 and 2 Patch 3 is outside of my area of competence +1 on patch 4, which looks trivially correct. +1 on patch 5, though I agree with peff that it can be omitted in deference to Ronnie's work. By the way, while cleaning up in patch 5 you might take the chance to rename the local variable ref_name to refname to be consistent with most of our code, but this is by no means required. Michael -- Michael Haggerty mhag...@alum.mit.edu -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] fix pack-refs pruning of refs/foo
On Sat, Aug 23, 2014 at 09:29:36AM +0200, Michael Haggerty wrote: [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 patches 1 and 2 Patch 3 is outside of my area of competence +1 on patch 4, which looks trivially correct. +1 on patch 5, though I agree with peff that it can be omitted in deference to Ronnie's work. Thanks. By the way, while cleaning up in patch 5 you might take the chance to rename the local variable ref_name to refname to be consistent with most of our code, but this is by no means required. I had the exact same inclination, but dismissed it as me being too picky. :) I'll change it if I re-roll, but I think we'll end up just dropping that patch entirely. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
