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

Reply via email to