Re: [PATCH 21/28] commit_packed_refs(): use a staging file separate from the lockfile

[Michael Haggerty]

I just submitted a patch [1] that fixes the `packed-refs`-is-a-symlink
problem in combination with git-new-workdir.

I haven't considered any possible problems related to
`core.preferSymlinkRefs`. That behavior should be unaffected by the
packed-ref-store patch series and I'm not very interested in working
on it.

Michael

[1] 
https://public-inbox.org/git/d0da02a8b6f0272fa70ae3b1dc80fee6c6ee8d18.150803.git.mhag...@alum.mit.edu/

On Mon, Jul 24, 2017 at 2:43 PM, Junio C Hamano  wrote:
> Jeff King  writes:
>
>> On Mon, Jul 24, 2017 at 10:09:15AM -0700, Jonathan Nieder wrote:
>>
>>> Jeff King wrote:
>>>
>>> > This seems like the correct path to me. If the existing behavior is to
>>> > lock the referring symref, that seems like a violation of the lock
>>> > procedure in the first place. Because if "A" points to "B", we take
>>> > "A.lock" and then modify "B". But "B" may have any number of "A" links
>>> > pointing to it, eliminating the purpose of the lock.
>>> >
>>> > I thought we already did this, though. And that modifying HEAD (which
>>> > might be a symlink) required LOCK_NO_DEREF.
>>>
>>> Yes, I believe the lockfile API already does so.  Since this patch
>>> creates a ".new" file, not using the lockfile API, it doesn't benefit
>>> from that, though.
>>
>> Ah, I see. This bug makes much more sense, then. And I agree doubly that
>> matching the lock-code's behavior is the right thing to do.
>
> OK.  Anybody wants to take a crack at it (it is of lower priority to
> me during the -rc freeze to deal with problems in topics on 'next'
> or higher)?
>
> Thanks.

Re: [PATCH 21/28] commit_packed_refs(): use a staging file separate from the lockfile

[Junio C Hamano]

Jeff King  writes:

> On Mon, Jul 24, 2017 at 10:09:15AM -0700, Jonathan Nieder wrote:
>
>> Jeff King wrote:
>> 
>> > This seems like the correct path to me. If the existing behavior is to
>> > lock the referring symref, that seems like a violation of the lock
>> > procedure in the first place. Because if "A" points to "B", we take
>> > "A.lock" and then modify "B". But "B" may have any number of "A" links
>> > pointing to it, eliminating the purpose of the lock.
>> >
>> > I thought we already did this, though. And that modifying HEAD (which
>> > might be a symlink) required LOCK_NO_DEREF.
>> 
>> Yes, I believe the lockfile API already does so.  Since this patch
>> creates a ".new" file, not using the lockfile API, it doesn't benefit
>> from that, though.
>
> Ah, I see. This bug makes much more sense, then. And I agree doubly that
> matching the lock-code's behavior is the right thing to do.

OK.  Anybody wants to take a crack at it (it is of lower priority to
me during the -rc freeze to deal with problems in topics on 'next'
or higher)?

Thanks.

Re: [PATCH 21/28] commit_packed_refs(): use a staging file separate from the lockfile

[Jeff King]

On Mon, Jul 24, 2017 at 10:09:15AM -0700, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > This seems like the correct path to me. If the existing behavior is to
> > lock the referring symref, that seems like a violation of the lock
> > procedure in the first place. Because if "A" points to "B", we take
> > "A.lock" and then modify "B". But "B" may have any number of "A" links
> > pointing to it, eliminating the purpose of the lock.
> >
> > I thought we already did this, though. And that modifying HEAD (which
> > might be a symlink) required LOCK_NO_DEREF.
> 
> Yes, I believe the lockfile API already does so.  Since this patch
> creates a ".new" file, not using the lockfile API, it doesn't benefit
> from that, though.

Ah, I see. This bug makes much more sense, then. And I agree doubly that
matching the lock-code's behavior is the right thing to do.

-Peff

Re: [PATCH 21/28] commit_packed_refs(): use a staging file separate from the lockfile

[Jonathan Nieder]

Hi,

Jeff King wrote:

> This seems like the correct path to me. If the existing behavior is to
> lock the referring symref, that seems like a violation of the lock
> procedure in the first place. Because if "A" points to "B", we take
> "A.lock" and then modify "B". But "B" may have any number of "A" links
> pointing to it, eliminating the purpose of the lock.
>
> I thought we already did this, though. And that modifying HEAD (which
> might be a symlink) required LOCK_NO_DEREF.

Yes, I believe the lockfile API already does so.  Since this patch
creates a ".new" file, not using the lockfile API, it doesn't benefit
from that, though.

Thanks,
Jonathan

Re: [PATCH 21/28] commit_packed_refs(): use a staging file separate from the lockfile

[Jeff King]

On Thu, Jul 20, 2017 at 11:01:45PM -0700, Junio C Hamano wrote:

> The general strategy we take for these atomic update of a file
> entity at $path is to:
> 
>  (1) try to create $path.lock exclusively; if we cannot, somebody
>  else holds the lock so fail (or backoff and retry, which
>  amounts to the same thing in the larger picture).
> 
>  (2) update $path.lock and close the fd.
> 
>  (3) rename $path.lock to $path atomically to unlock.
> 
> Would it be sufficent to tweak the above procedure with a new,
> zeroth step?
> 
>  (0) see $path is a symlink; if so, readlink it and assign the
>  result to $path.  Then go on to step (1) above.
> 
> I do not think such an enhancement would break atomicity guarantee
> we want from the locking.  When updating .git/packed-refs in an
> ancient new-workdir'ed directory, we would end up locking the
> corresponding file in the real repository, which is exactly what we
> want anyway.  As the basic assumption of having a symbolic link in
> the new-workdir'ed directory is that a symlink can stay the same
> forever while the linked target will be updated, I think this would
> be a reasonable short-term "fix".

This seems like the correct path to me. If the existing behavior is to
lock the referring symref, that seems like a violation of the lock
procedure in the first place. Because if "A" points to "B", we take
"A.lock" and then modify "B". But "B" may have any number of "A" links
pointing to it, eliminating the purpose of the lock.

I thought we already did this, though. And that modifying HEAD (which
might be a symlink) required LOCK_NO_DEREF.

-Peff

Re: [PATCH 21/28] commit_packed_refs(): use a staging file separate from the lockfile

[Junio C Hamano]

Junio C Hamano  writes:

> The general strategy we take for these atomic update of a file
> entity at $path is to:
>
>  (1) try to create $path.lock exclusively; if we cannot, somebody
>  else holds the lock so fail (or backoff and retry, which
>  amounts to the same thing in the larger picture).
>
>  (2) update $path.lock and close the fd.
>
>  (3) rename $path.lock to $path atomically to unlock.
>
> Would it be sufficent to tweak the above procedure with a new,
> zeroth step?
>
>  (0) see $path is a symlink; if so, readlink it and assign the
>  result to $path.  Then go on to step (1) above.
>
> I do not think such an enhancement would break atomicity guarantee
> we want from the locking.  When updating .git/packed-refs in an
> ancient new-workdir'ed directory, we would end up locking the
> corresponding file in the real repository, which is exactly what we
> want anyway.  As the basic assumption of having a symbolic link in
> the new-workdir'ed directory is that a symlink can stay the same
> forever while the linked target will be updated, I think this would
> be a reasonable short-term "fix".
>
> It would be ideal if we can do this at somewhat a low level, i.e. in
> the lockfile subsystem.

One thing I forgot to mention.  For the above to work safely, we
should think through the possible interaction this may have with the
core.preferSymlinkRefs configuration.  If the above is implemented
naively, an update of a symref (e.g. "HEAD") that points at
something to point at something else would end up taking a lock on
the underlying ref and updating it, which is not what we want at
all.

Perhaps it is about time we stopped supporting the configuration.
We may be able to come up with a solution while keeping it alive,
but dropping it would mean we would have one less thing we need to
worry about.

Re: [PATCH 21/28] commit_packed_refs(): use a staging file separate from the lockfile

[Junio C Hamano]

Jonathan Nieder  writes:

>> Do we care about the ancient layout that used symlinks to emulate
>> the more modern worktree one?
>
> I think we do care.  In the context of people's changing workflows,
> "git worktree" is a relatively new tool.  Breaking the older
> git-new-workdir (and tools that emulate it) would affect a large
> number of users that don't necessarily know how to clean up the
> result.

Fair enough.  Even though the git-new-workdir is on its way out, and
we really do want to do certain updates under lock, we cannot just
allow the topic to graduate to 'master' in its current form without
some transition plan or mitigation strategy.

The general strategy we take for these atomic update of a file
entity at $path is to:

 (1) try to create $path.lock exclusively; if we cannot, somebody
 else holds the lock so fail (or backoff and retry, which
 amounts to the same thing in the larger picture).

 (2) update $path.lock and close the fd.

 (3) rename $path.lock to $path atomically to unlock.

Would it be sufficent to tweak the above procedure with a new,
zeroth step?

 (0) see $path is a symlink; if so, readlink it and assign the
 result to $path.  Then go on to step (1) above.

I do not think such an enhancement would break atomicity guarantee
we want from the locking.  When updating .git/packed-refs in an
ancient new-workdir'ed directory, we would end up locking the
corresponding file in the real repository, which is exactly what we
want anyway.  As the basic assumption of having a symbolic link in
the new-workdir'ed directory is that a symlink can stay the same
forever while the linked target will be updated, I think this would
be a reasonable short-term "fix".

It would be ideal if we can do this at somewhat a low level, i.e. in
the lockfile subsystem.

Thoughts?  An even easier way out may be to just make pack-refs a
no-op when .git/refs/ is a symbolic link, but as a solution that is
far from satisfactory, while "locking the right file" smells right,
at least to me.

Re: [PATCH 21/28] commit_packed_refs(): use a staging file separate from the lockfile

[Jonathan Nieder]

Junio C Hamano wrote:
> Michael Haggerty  writes:

>> We will want to be able to hold the lockfile for `packed-refs` even
>> after we have activated the new values. So use a separate tempfile,
>> `packed-refs.new`, as a place to stage the new contents of the
>> `packed-refs` file. For now this is all done within
>> `commit_packed_refs()`, but that will change shortly.
>>
>> Signed-off-by: Michael Haggerty 
>> ---
>
> The layout created by "contrib/workdir/git-new-workdir" will be
> broken by this line of change.  "git worktree" is supposed to know
> that refs/packed-refs is a shared thing and lives in common-dir,
> so it shouldn't be affected.
>
> Do we care about the ancient layout that used symlinks to emulate
> the more modern worktree one?

I think we do care.  In the context of people's changing workflows,
"git worktree" is a relatively new tool.  Breaking the older
git-new-workdir (and tools that emulate it) would affect a large
number of users that don't necessarily know how to clean up the
result.

Thanks,
Jonathan

Re: [PATCH 21/28] commit_packed_refs(): use a staging file separate from the lockfile

[Junio C Hamano]

Michael Haggerty  writes:

> We will want to be able to hold the lockfile for `packed-refs` even
> after we have activated the new values. So use a separate tempfile,
> `packed-refs.new`, as a place to stage the new contents of the
> `packed-refs` file. For now this is all done within
> `commit_packed_refs()`, but that will change shortly.
>
> Signed-off-by: Michael Haggerty 
> ---

The layout created by "contrib/workdir/git-new-workdir" will be
broken by this line of change.  "git worktree" is supposed to know
that refs/packed-refs is a shared thing and lives in common-dir,
so it shouldn't be affected.

Do we care about the ancient layout that used symlinks to emulate
the more modern worktree one?

[PATCH 21/28] commit_packed_refs(): use a staging file separate from the lockfile

[Michael Haggerty]

We will want to be able to hold the lockfile for `packed-refs` even
after we have activated the new values. So use a separate tempfile,
`packed-refs.new`, as a place to stage the new contents of the
`packed-refs` file. For now this is all done within
`commit_packed_refs()`, but that will change shortly.

Signed-off-by: Michael Haggerty 
---
 refs/packed-backend.c | 40 
 1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 5bee49d497..6619052e96 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -68,6 +68,13 @@ struct packed_ref_store {
 * thus the enclosing `packed_ref_store`) must not be freed.
 */
struct lock_file lock;
+
+   /*
+* Temporary file used when rewriting new contents to the
+* "packed-refs" file. Note that this (and thus the enclosing
+* `packed_ref_store`) must not be freed.
+*/
+   struct tempfile tempfile;
 };
 
 struct ref_store *packed_ref_store_create(const char *path,
@@ -522,10 +529,16 @@ int lock_packed_refs(struct ref_store *ref_store, int 
flags)
timeout_configured = 1;
}
 
+   /*
+* Note that we close the lockfile immediately because we
+* don't write new content to it, but rather to a separate
+* tempfile.
+*/
if (hold_lock_file_for_update_timeout(
>lock,
refs->path,
-   flags, timeout_value) < 0)
+   flags, timeout_value) < 0 ||
+   close_lock_file(>lock))
return -1;
 
/*
@@ -567,13 +580,23 @@ int commit_packed_refs(struct ref_store *ref_store, 
struct strbuf *err)
get_packed_ref_cache(refs);
int ok;
int ret = -1;
+   struct strbuf sb = STRBUF_INIT;
FILE *out;
struct ref_iterator *iter;
 
if (!is_lock_file_locked(>lock))
die("BUG: commit_packed_refs() called when unlocked");
 
-   out = fdopen_lock_file(>lock, "w");
+   strbuf_addf(, "%s.new", refs->path);
+   if (create_tempfile(>tempfile, sb.buf) < 0) {
+   strbuf_addf(err, "unable to create file %s: %s",
+   sb.buf, strerror(errno));
+   strbuf_release();
+   goto out;
+   }
+   strbuf_release();
+
+   out = fdopen_tempfile(>tempfile, "w");
if (!out) {
strbuf_addf(err, "unable to fdopen packed-refs tempfile: %s",
strerror(errno));
@@ -582,7 +605,7 @@ int commit_packed_refs(struct ref_store *ref_store, struct 
strbuf *err)
 
if (fprintf(out, "%s", PACKED_REFS_HEADER) < 0) {
strbuf_addf(err, "error writing to %s: %s",
-   get_lock_file_path(>lock), strerror(errno));
+   get_tempfile_path(>tempfile), 
strerror(errno));
goto error;
}
 
@@ -594,7 +617,7 @@ int commit_packed_refs(struct ref_store *ref_store, struct 
strbuf *err)
if (write_packed_entry(out, iter->refname, iter->oid->hash,
   peel_error ? NULL : peeled.hash)) {
strbuf_addf(err, "error writing to %s: %s",
-   get_lock_file_path(>lock),
+   get_tempfile_path(>tempfile),
strerror(errno));
ref_iterator_abort(iter);
goto error;
@@ -602,13 +625,13 @@ int commit_packed_refs(struct ref_store *ref_store, 
struct strbuf *err)
}
 
if (ok != ITER_DONE) {
-   strbuf_addf(err, "unable to write packed-refs file: "
+   strbuf_addf(err, "unable to rewrite packed-refs file: "
"error iterating over old contents");
goto error;
}
 
-   if (commit_lock_file(>lock)) {
-   strbuf_addf(err, "error overwriting %s: %s",
+   if (rename_tempfile(>tempfile, refs->path)) {
+   strbuf_addf(err, "error replacing %s: %s",
refs->path, strerror(errno));
goto out;
}
@@ -617,9 +640,10 @@ int commit_packed_refs(struct ref_store *ref_store, struct 
strbuf *err)
goto out;
 
 error:
-   rollback_lock_file(>lock);
+   delete_tempfile(>tempfile);
 
 out:
+   rollback_lock_file(>lock);
release_packed_ref_cache(packed_ref_cache);
return ret;
 }
-- 
2.11.0