David Turner [off-list ref] writes:
This version incorporates many changes suggested by Michael Haggerty,
Junio, Jonathan Nieder, Eric Sunshine, and Jeff King. I think I have
addressed of the comments that were sent to me. Those that I chose
not to incorporate, I responded to on the mailing list.
Thanks for all of the feed back so far.
Unfortunately this did not compile for me X-< and with a trivial
fix-up, I found that this overlaps with Peff's recent fixes to the
locking of symbolic refs. So for today's integration run, I punted.
I still will push out this topic to the broken-out repository I keep
here:
https://github.com/gitster/git
It's just 'pu' will not have this latest incarnation, but has the
older one.
Thanks.
On Tue, Jan 12, 2016 at 04:22:09PM -0800, Junio C Hamano wrote:
David Turner [off-list ref] writes:
quoted
This version incorporates many changes suggested by Michael Haggerty,
Junio, Jonathan Nieder, Eric Sunshine, and Jeff King. I think I have
addressed of the comments that were sent to me. Those that I chose
not to incorporate, I responded to on the mailing list.
Thanks for all of the feed back so far.
Unfortunately this did not compile for me X-< and with a trivial
fix-up, I found that this overlaps with Peff's recent fixes to the
locking of symbolic refs. So for today's integration run, I punted.
I still will push out this topic to the broken-out repository I keep
here:
https://github.com/gitster/git
It's just 'pu' will not have this latest incarnation, but has the
older one.
I took a look at David's changes. The conflicts come from "refs:
resolve symbolic refs first". I'm not sure I fully understand all that
is going on in that patch, but it looks like after it, we are less
likely to handle ENOTDIR and d/f conflicts for symrefs, as we skip that
whole code path for REF_ISSYMREF.
The rest of the conflicts are related to the fact that all of the
initial resolution is pulled out of lock_ref_sha1_basic(), and the
caller is supposed to do it. So I think if create_symref() is going to
call lock_ref_sha1_basic(), as in my series, when combined with David's
it should also be calling dereference_symrefs(). That uses a
ref_transaction, which we don't have in create_symref() right now, but
it makes sense that we would ultimately want to push symref updates
through the same transaction/backend system.
So sorry, I don't have a quick resolution to this. I'm hoping David can
make more sense of it than I did.
-Peff
On Thu, 2016-01-14 at 04:26 -0500, Jeff King wrote:
On Tue, Jan 12, 2016 at 04:22:09PM -0800, Junio C Hamano wrote:
quoted
David Turner [off-list ref] writes:
quoted
This version incorporates many changes suggested by Michael
Haggerty,
Junio, Jonathan Nieder, Eric Sunshine, and Jeff King. I think I
have
addressed of the comments that were sent to me. Those that I
chose
not to incorporate, I responded to on the mailing list.
Thanks for all of the feed back so far.
Unfortunately this did not compile for me X-< and with a trivial
fix-up, I found that this overlaps with Peff's recent fixes to the
locking of symbolic refs. So for today's integration run, I
punted.
I still will push out this topic to the broken-out repository I
keep
here:
https://github.com/gitster/git
It's just 'pu' will not have this latest incarnation, but has the
older one.
I took a look at David's changes. The conflicts come from "refs:
resolve symbolic refs first". I'm not sure I fully understand all
that
is going on in that patch, but it looks like after it, we are less
likely to handle ENOTDIR and d/f conflicts for symrefs, as we skip
that
whole code path for REF_ISSYMREF.
We only get into the symref part of that codepath if there's already a
symref present, meaning that d/f conflicts can't happen.
The rest of the conflicts are related to the fact that all of the
initial resolution is pulled out of lock_ref_sha1_basic(), and the
caller is supposed to do it. So I think if create_symref() is going
to
call lock_ref_sha1_basic(), as in my series, when combined with
David's
it should also be calling dereference_symrefs(). That uses a
ref_transaction, which we don't have in create_symref() right now,
but
it makes sense that we would ultimately want to push symref updates
through the same transaction/backend system.
I don't think that's quite true. create_symref *always* creates
symrefs, and never creates underlying refs. So it calls
lock_ref_sha1_basic(), but since type_p is NULL, it doesn't go into the
resolved-symlinks path; instead, we get into the original codepath.
So sorry, I don't have a quick resolution to this. I'm hoping David
can
make more sense of it than I did.
I was totally convinced that we were doomed, but I think the stupid
resolution basically works, with some minor tweaks. I'm going to re
-review that patch and resend the series (then go out of town until
Tuesday).
We will need to apply your new d/f conflict check to the LMDB backend's
symref code (presently, it fails your new test), but I'm going to punt
on that for now since d/f conflicts don't cause problems for the LMDB
backend and this is a relatively minor case. I've added a TODO to the
code.
On Thu, Jan 14, 2016 at 11:25:52AM -0500, David Turner wrote:
quoted
I took a look at David's changes. The conflicts come from "refs:
resolve symbolic refs first". I'm not sure I fully understand all
that
is going on in that patch, but it looks like after it, we are less
likely to handle ENOTDIR and d/f conflicts for symrefs, as we skip
that
whole code path for REF_ISSYMREF.
We only get into the symref part of that codepath if there's already a
symref present, meaning that d/f conflicts can't happen.
Ah, right, that makes sense.
quoted
The rest of the conflicts are related to the fact that all of the
initial resolution is pulled out of lock_ref_sha1_basic(), and the
caller is supposed to do it. So I think if create_symref() is going
to
call lock_ref_sha1_basic(), as in my series, when combined with
David's
it should also be calling dereference_symrefs(). That uses a
ref_transaction, which we don't have in create_symref() right now,
but
it makes sense that we would ultimately want to push symref updates
through the same transaction/backend system.
I don't think that's quite true. create_symref *always* creates
symrefs, and never creates underlying refs. So it calls
lock_ref_sha1_basic(), but since type_p is NULL, it doesn't go into the
resolved-symlinks path; instead, we get into the original codepath.
I'm not sure in which version of the code you mean here. I guess in the
merged one, because in your original create_symref is still
pre-lock_ref_sha1_basic. But I think...
I was totally convinced that we were doomed, but I think the stupid
resolution basically works, with some minor tweaks. I'm going to re
-review that patch and resend the series (then go out of town until
Tuesday).
...the best thing for me to do is wait and see your revised patch. :)
Thanks for looking into it.
We will need to apply your new d/f conflict check to the LMDB backend's
symref code (presently, it fails your new test), but I'm going to punt
on that for now since d/f conflicts don't cause problems for the LMDB
backend and this is a relatively minor case. I've added a TODO to the
code.
Makes sense.
-Peff