Michael Haggerty [off-list ref] writes:
quoted
Now that we have ref_transaction_*, I think if git-fetch fed all of the
deletes (along with the updates) into a single transaction, we would get
the same optimization for free. Maybe that is even part of some of the
pending ref_transaction work from Stefan or Michael (both cc'd). I
haven't kept up very well with what is cooking in pu.
I am looking into this now.
For pruning, we can't use a ref_transaction as it is currently
implemented because it would fail if any of the reference deletions
failed. But in this case I think if any deletions fail, we would prefer
to emit a warning but keep going.
I am not quite sure what you mean here. I agree with you if you
meant "we shouldn't fail the fetch only because 'fetch --prune'
failed to remove only one of the remote-tracking refs that are no
longer there" but that can easily be solved by the pruning phase
into a separate transaction. If you meant "we would rather remove
origin/{a,b} non-atomically when we noticed that origin/{a,b,c} are
all gone than leaving all three intact only because we failed to
remove origin/c for whatever reason", my knee-jerk reaction is "does
it make practical difference to the end user between these two?"
What are the plausible cause of failing to prune unused
remote-tracking refs?
On Thu, Mar 19, 2015 at 12:24:21PM -0700, Junio C Hamano wrote:
quoted
For pruning, we can't use a ref_transaction as it is currently
implemented because it would fail if any of the reference deletions
failed. But in this case I think if any deletions fail, we would prefer
to emit a warning but keep going.
I am not quite sure what you mean here. I agree with you if you
meant "we shouldn't fail the fetch only because 'fetch --prune'
failed to remove only one of the remote-tracking refs that are no
longer there" but that can easily be solved by the pruning phase
into a separate transaction. If you meant "we would rather remove
origin/{a,b} non-atomically when we noticed that origin/{a,b,c} are
all gone than leaving all three intact only because we failed to
remove origin/c for whatever reason", my knee-jerk reaction is "does
it make practical difference to the end user between these two?"
What are the plausible cause of failing to prune unused
remote-tracking refs?
I had assumed earlier that Michael meant to use a single ref_transaction
for all updates. Thinking on it more, that is probably a bad idea, as it
makes fetch atomic in a user-visible way, whereas currently the updates
are always per-ref (i.e., some may fail, but we let others succeed). I
don't know if people actually care or not (certainly the exit code of
fetch represents all of the refs, so it is not like you could say "eh,
git-fetch return 1, but it probably got the ref I wanted" without
parsing the human-readable output).
If it is just a single atomic commit for all of the deletions, I agree
it is less of a big deal. They are unlikely to fail, and when they do,
you are not blocking the new refs from coming in.
I think the ref_transaction does have some smarts to handle a case where
we are updating "refs/foo/bar" while "refs/foo" exists but is deleted in
the transaction. We switched to pruning before updating in
10a6cc8 (fetch --prune: Run prune before fetching, 2014-01-02), so it is
a non-issue, but what is there now is technically racy[1], and it would
have been nice to let the ref-storage code handle it. I guess we still
can if we introduce a "git fetch --atomic" option.
-Peff
[1] http://article.gmane.org/gmane.comp.version-control.git/239519
On 03/19/2015 08:24 PM, Junio C Hamano wrote:
Michael Haggerty [off-list ref] writes:
quoted
quoted
Now that we have ref_transaction_*, I think if git-fetch fed all of the
deletes (along with the updates) into a single transaction, we would get
the same optimization for free. Maybe that is even part of some of the
pending ref_transaction work from Stefan or Michael (both cc'd). I
haven't kept up very well with what is cooking in pu.
I am looking into this now.
For pruning, we can't use a ref_transaction as it is currently
implemented because it would fail if any of the reference deletions
failed. But in this case I think if any deletions fail, we would prefer
to emit a warning but keep going.
I am not quite sure what you mean here. I agree with you if you
meant "we shouldn't fail the fetch only because 'fetch --prune'
failed to remove only one of the remote-tracking refs that are no
longer there" but that can easily be solved by the pruning phase
into a separate transaction. If you meant "we would rather remove
origin/{a,b} non-atomically when we noticed that origin/{a,b,c} are
all gone than leaving all three intact only because we failed to
remove origin/c for whatever reason", my knee-jerk reaction is "does
it make practical difference to the end user between these two?"
What are the plausible cause of failing to prune unused
remote-tracking refs?
I wasn't proposing to combine the updates with the prunes in a single
transaction. (Though now that I think about it, when "fetch --atomic" is
specified it wouldn't be so crazy. It would also have the virtue of
allowing the reference transaction code to deal with any D/F conflicts
between references being added and references being deleted.)
What I meant is the second thing you mentioned, namely that currently we
prune each reference on a "best-effort" basis and wouldn't skip *all*
prunes just because one failed.
But you raise an interesting question: what could cause a failure to
prune an unused remote-tracking ref (i.e. if all pruning is done in a
single transaction)?
* I think the most likely reason for a failure would be a lock conflict
with another process. We don't retry lock attempts [1], so this would
cause the whole transaction to fail. This could happen, for example, if
the user launches two "fetch --prune" operations at the same time, or
runs "pack-refs" while fetching [2].
* It is *not* a failure if another process deletes or updates the
reference before we get to it, because we don't provide an "old" value
of the reference for a pre-check.
* I haven't verified this, but I can imagine it could fail if another
process deletes the reference (say refs/remotes/origin/foo/bar) and adds
another reference that would D/F conflict with it (say
refs/remotes/origin/foo) while we are working, because our attempt to
create refs/remotes/origin/foo/bar.lock would fail.
* A repository problem, like for example wrong file permissions
somewhere in the loose refs directory, could prevent us from being able
to create the lockfile or delete the loose reference file.
The lock conflict scenario is the only one that really worries me.
But I don't think it is *so* hard to keep the current "best-effort"
behavior. I am working on a function delete_refs() for the reference API
that deletes a bunch of references on a "best effort" basis, reporting
per-reference errors for any deletions that fail. Longer term, we could
add something like a REFS_NON_ATOMIC flag to the
refs_transaction_update() functions, which allows the rest of the
transaction to proceed even if that particular update fails.
Michael
[1] I hope to submit a patch to make it possible to retry lock
acquisition with a specified timeout. This should help in scenarios like
this.
[2] Maybe a careful analysis or a slight change to our locking protocol
could prove that the only lock acquisition that can fail is the one on
packed-refs, which would cause all of the deletes to fail anyway. In
that case per-reference failures would cease to be a worry.
--
Michael Haggerty
mhagger@alum.mit.edu