On Thu, Dec 04, 2014 at 04:23:31PM -0800, Jonathan Nieder wrote:
> Michael Haggerty wrote:
>
> > We don't actually need the locking functionality, because we already
> > hold the lock on the reference itself, which is how the reflog file is
> > locked. But the lock_file code still does some of the bookkeeping for
> > us and is more careful than the old code here was.
>
> As you say, the ref lock takes care of mutual exclusion, so we do not
> have to be too careful about compatibility with other tools that might
> not know to lock the reflog. And this is not tying our hands for a
> future when I might want to lock logs/refs/heads/topic/1 while
> logs/refs/heads/topic still exists as part of the implementation of
> "git mv topic/1 topic".
>
> Stefan and I had forgotten about that guarantee when looking at that
> kind of operation --- thanks for the reminder.
>
> Should updates to the HEAD reflog acquire HEAD.lock? (They don't
> currently.)
>
> [...]
> > --- a/builtin/reflog.c
> > +++ b/builtin/reflog.c
> > @@ -349,12 +349,14 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int
> > return 0;
> > }
> >
> > +static struct lock_file reflog_lock;
>
> If this lockfile is only used in that one function, it can be declared
> inside the function.
>
> If it is meant to be used throughout the 'git reflog' command, then it
> can go near the top of the file.
>
After the series completes, this lock is only used in reflog_expire.
So I'd rather move it inside the function? Then we could run the reflog_expire
function in parallel for different locks in theory?
> > +
> > static int expire_reflog(const char *refname, const unsigned char *sha1, void *cb_data)
> > {
> > struct cmd_reflog_expire_cb *cmd = cb_data;
> > struct expire_reflog_cb cb;
> > struct ref_lock *lock;
> > - char *log_file, *newlog_path = NULL;
> > + char *log_file;
> > struct commit *tip_commit;
> > struct commit_list *tips;
> > int status = 0;
> > @@ -372,10 +374,14 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
> > unlock_ref(lock);
> > return 0;
> > }
> > +
> > log_file = git_pathdup("logs/%s", refname);
> > if (!cmd->dry_run) {
> > - newlog_path = git_pathdup("logs/%s.lock", refname);
> > - cb.newlog = fopen(newlog_path, "w");
> > + if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)
> > + goto failure;
>
> hold_lock_file_for_update doesn't print a message. Code to print one
> looks like
>
> if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0) {
> unable_to_lock_message(log_file, errno, &err);
> error("%s", err.buf);
> goto failure;
> }
>
> (A patch in flight changes that to
>
> if (hold_lock_file_for_update(&reflog_lock, log_file, 0, &err) < 0) {
> error("%s", err.buf);
> goto failure;
> }
>
> )
>
> > + cb.newlog = fdopen_lock_file(&reflog_lock, "w");
> > + if (!cb.newlog)
> > + goto failure;
>
> Hm. lockfile.c::fdopen_lock_file ought to use xfdopen to make this
> case impossible. And xfdopen should use try_to_free_routine() and
> try again on failure.
>
> [...]
> > @@ -423,10 +429,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
> > }
> >
> > if (cb.newlog) {
> > - if (fclose(cb.newlog)) {
> > - status |= error("%s: %s", strerror(errno),
> > - newlog_path);
> > - unlink(newlog_path);
> > + if (close_lock_file(&reflog_lock)) {
> > + status |= error("Couldn't write %s: %s", log_file,
> > + strerror(errno));
>
> Style nit: error messages usually start with a lowercase letter
> (though I realize nearby examples are already inconsistent).
>
> commit_lock_file() can take care of the close_lock_file automatically.
>
> [...]
> > @@ -434,21 +439,23 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
> > close_ref(lock) < 0)) {
> > status |= error("Couldn't write %s",
> > lock->lk->filename.buf);
> > - unlink(newlog_path);
> > - } else if (rename(newlog_path, log_file)) {
> > - status |= error("cannot rename %s to %s",
> > - newlog_path, log_file);
> > - unlink(newlog_path);
> > + rollback_lock_file(&reflog_lock);
> > + } else if (commit_lock_file(&reflog_lock)) {
> > + status |= error("cannot rename %s.lock to %s",
> > + log_file, log_file);
>
> Most callers say "unable to commit reflog '%s'", log_file to hedge their
> bets in case the close failed (which may be what you were avoiding
> above.
>
> errno is meaningful when commit_lock_file fails, making a more
> detailed diagnosis from strerror(errno) possible.
>
> Thanks,
> Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html