git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 07/23] expire_reflog(): use a lock_file for rewriting the reflog file

From
Michael Haggerty <mhagger@alum.mit.edu>
Date
Dec 8, 2014, 14:05 UTC
Message-ID
<5485B039.1070200@alum.mit.edu>
In-Reply-To
<20141205002331.GJ16345@google.com>
On 12/05/2014 01:23 AM, Jonathan Nieder wrote:
Show 16 quoted lines
> 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.

Show 14 quoted lines
> [...]
>> --- 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.)

Show 31 quoted lines
>> +
>>  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.
Show 8 quoted lines
> (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.

Show 7 quoted lines
>> +		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".

Show 15 quoted lines
> [...]
>> @@ -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.

Show 21 quoted lines
> [...]
>> @@ -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
mhagger@alum.mit.edu
Previous: Junio C HamanoNext: ronnie sahlberg
Message 25 of 63 in “Add reflog_expire() to the references API”
  1. 00/23 Add reflog_expire() to the references APIMichael Haggerty, Dec 4, 2014
  2. 01/23 refs.c: make ref_transaction_create a wrapper for ref_transaction_updateMichael Haggerty, Dec 4, 2014
  3. 02/23 refs.c: make ref_transaction_delete a wrapper for ref_transaction_updateMichael Haggerty, Dec 4, 2014
  4. 03/23 refs.c: add a function to append a reflog entry to a fdMichael Haggerty, Dec 4, 2014
  5. 04/23 expire_reflog(): remove unused parameterMichael Haggerty, Dec 4, 2014
  6. Jonathan NiederDec 4, 2014
  7. Jonathan NiederDec 4, 2014
  8. Michael HaggertyDec 5, 2014
  9. 05/23 expire_reflog(): rename "ref" parameter to "refname"Michael Haggerty, Dec 4, 2014
  10. Jonathan NiederDec 4, 2014
  11. 06/23 expire_reflog(): exit early if the reference has no reflogMichael Haggerty, Dec 4, 2014
  12. Jonathan NiederDec 4, 2014
  13. Jonathan NiederDec 4, 2014
  14. Michael HaggertyDec 5, 2014
  15. 07/23 expire_reflog(): use a lock_file for rewriting the reflog fileMichael Haggerty, Dec 4, 2014
  16. Jonathan NiederDec 5, 2014
  17. Stefan BellerDec 5, 2014
  18. Michael HaggertyDec 8, 2014
  19. Junio C HamanoDec 9, 2014
  20. Jeff KingDec 9, 2014
  21. Stefan BellerDec 5, 2014
  22. Junio C HamanoDec 5, 2014
  23. Stefan BellerDec 5, 2014
  24. Junio C HamanoDec 5, 2014
  25. Michael HaggertyDec 8, 2014
  26. ronnie sahlbergDec 5, 2014
  27. Michael HaggertyDec 8, 2014
  28. 08/23 Extract function should_expire_reflog_ent()Michael Haggerty, Dec 4, 2014
  29. Stefan BellerDec 8, 2014
  30. 09/23 expire_reflog(): extract two policy-related functionsMichael Haggerty, Dec 4, 2014
  31. Stefan BellerDec 5, 2014
  32. 10/23 expire_reflog(): add a "flags" argumentMichael Haggerty, Dec 4, 2014
  33. Stefan BellerDec 8, 2014
  34. 11/23 expire_reflog(): move dry_run to flags argumentMichael Haggerty, Dec 4, 2014
  35. Stefan BellerDec 8, 2014
  36. 12/23 expire_reflog(): move updateref to flags argumentMichael Haggerty, Dec 4, 2014
  37. Stefan BellerDec 8, 2014
  38. 13/23 Rename expire_reflog_cb to expire_reflog_policy_cbMichael Haggerty, Dec 4, 2014
  39. Stefan BellerDec 8, 2014
  40. 14/23 struct expire_reflog_cb: a new callback data typeMichael Haggerty, Dec 4, 2014
  41. Stefan BellerDec 8, 2014
  42. 15/23 expire_reflog(): pass flags through to expire_reflog_ent()Michael Haggerty, Dec 4, 2014
  43. Stefan BellerDec 8, 2014
  44. 16/23 expire_reflog(): move verbose to flags argumentMichael Haggerty, Dec 4, 2014
  45. Stefan BellerDec 8, 2014
  46. 17/23 expire_reflog(): move rewrite to flags argumentMichael Haggerty, Dec 4, 2014
  47. Stefan BellerDec 8, 2014
  48. 18/23 Move newlog and last_kept_sha1 to "struct expire_reflog_cb"Michael Haggerty, Dec 4, 2014
  49. Stefan BellerDec 8, 2014
  50. 19/23 expire_reflog(): treat the policy callback data as opaqueMichael Haggerty, Dec 4, 2014
  51. Stefan BellerDec 8, 2014
  52. 20/23 reflog_expire(): new function in the reference APIMichael Haggerty, Dec 4, 2014
  53. Stefan BellerDec 8, 2014
  54. Michael HaggertyDec 12, 2014
  55. Jeff KingDec 12, 2014
  56. Junio C HamanoDec 12, 2014
  57. 21/23 refs.c: remove unlock_ref/close_ref/commit_ref from the refs apiMichael Haggerty, Dec 4, 2014
  58. 22/23 lock_any_ref_for_update(): inline functionMichael Haggerty, Dec 4, 2014
  59. Stefan BellerDec 8, 2014
  60. Michael HaggertyDec 11, 2014
  61. 23/23 refs.c: don't expose the internal struct ref_lock in the header fileMichael Haggerty, Dec 4, 2014
  62. Junio C HamanoDec 4, 2014
  63. Stefan BellerDec 5, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.