On 12/05/2014 01:23 AM, 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.
This reminder is important (and forgettable) enough that I will add a
comment within the function explaining it.
> Should updates to the HEAD reflog acquire HEAD.lock? (They don't
> currently.)
Yes, they should; good catch. I assume that you are referring to the
code at the bottom of write_ref_sha1()? Or did you find a problem in
this patch series?
If the former, then I propose that we address this bug in a separate
patch series.
> [...]
>> --- 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.
For now it is only used within this function, so I will move it into the
function as you suggest. (As you know, it does need to remain static,
because of the way the lock_file module takes over ownership of these
objects.)
>> +
>> 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;
> }
Thanks; will add.
> (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;
> }
>
> )
Thanks for the heads-up. The compiler will complain when the branches
are merged, and hopefully the fix will be obvious.
>> + 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.
That sounds reasonable, but it is not manifestly obvious given that at
least one caller of fdopen_lock_file() (in fast-import.c) tries to
recover if fdopen_lock_file() fails. Let's address this in a separate
patch series if that is OK with you. For now I will add explicit
error-reporting code here before "goto 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).
Thanks; will fix.
> commit_lock_file() can take care of the close_lock_file automatically.
The existing code is a tiny bit safer: first make sure both files can be
written, *then* rename each of them into place. If either write fails,
then both files will get rolled back. But if we switch to using
commit_lock_file(), then a failure when writing the reference would
leave the reflog updated but the reference rolled back.
> [...]
>> @@ -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.
I will improve the error message.
Thanks for your detailed review!
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
