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

Re: [PATCH v2 04/12] refs: implement simple transactions for the packed-refs file

From
Junio C Hamano <gitster@pobox.com>
Date
Jun 19, 2013, 19:18 UTC
Message-ID
<7vfvwdzz6k.fsf@alter.siamese.dyndns.org>
In-Reply-To
<1371628293-28824-5-git-send-email-mhagger@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
Show 42 quoted lines
> Handle simple transactions for the packed-refs file at the
> packed_ref_cache level via new functions lock_packed_refs(),
> commit_packed_refs(), and rollback_packed_refs().
>
> Only allow the packed ref cache to be modified (via add_packed_ref())
> while the packed refs file is locked.
>
> Change clone to add the new references within a transaction.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  builtin/clone.c |  7 ++++-
>  refs.c          | 83 ++++++++++++++++++++++++++++++++++++++++++++++-----------
>  refs.h          | 27 +++++++++++++++++--
>  3 files changed, 98 insertions(+), 19 deletions(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 66bff57..b0c000a 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -489,17 +489,22 @@ static struct ref *wanted_peer_refs(const struct ref *refs,
>  	return local_refs;
>  }
>  
> +static struct lock_file packed_refs_lock;
> +
>  static void write_remote_refs(const struct ref *local_refs)
>  {
>  	const struct ref *r;
>  
> +	lock_packed_refs(&packed_refs_lock, LOCK_DIE_ON_ERROR);
> +
>  	for (r = local_refs; r; r = r->next) {
>  		if (!r->peer_ref)
>  			continue;
>  		add_packed_ref(r->peer_ref->name, r->old_sha1);
>  	}
>  
> -	pack_refs(PACK_REFS_ALL);
> +	if (commit_packed_refs())
> +		die_errno("unable to overwrite old ref-pack file");
>  }

The calling convention used here looks somewhat strange. You allow callers to specify which lock-file structure is used when locking, but when you are done, commit_packed_refs() does not take any parameter.

lock_packed_refs() make the singleton in-core packed-ref-cache be aware of which lock it is under, so commit_packed_refs() does not need to be told (the singleton already knows what lockfile is in effect), so I am not saying the code is broken, though.

Does the caller need to even have an access to this lock_file instance?

Show 206 quoted lines
>  static void write_followtags(const struct ref *refs, const char *msg)
> diff --git a/refs.c b/refs.c
> index 373d95b..ad73251 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -808,6 +808,13 @@ static int is_refname_available(const char *refname, const char *oldrefname,
>  
>  struct packed_ref_cache {
>  	struct ref_entry *root;
> +
> +	/*
> +	 * Iff the packed-refs file associated with this instance is
> +	 * currently locked for writing, this points at the associated
> +	 * lock (which is owned by somebody else).
> +	 */
> +	struct lock_file *lock;
>  };
>  
>  /*
> @@ -829,6 +836,8 @@ static struct ref_cache {
>  static void clear_packed_ref_cache(struct ref_cache *refs)
>  {
>  	if (refs->packed) {
> +		if (refs->packed->lock)
> +			die("internal error: packed-ref cache cleared while locked");
>  		free_ref_entry(refs->packed->root);
>  		free(refs->packed);
>  		refs->packed = NULL;
> @@ -1038,7 +1047,12 @@ static struct ref_dir *get_packed_refs(struct ref_cache *refs)
>  
>  void add_packed_ref(const char *refname, const unsigned char *sha1)
>  {
> -	add_ref(get_packed_refs(&ref_cache),
> +	struct packed_ref_cache *packed_ref_cache =
> +		get_packed_ref_cache(&ref_cache);
> +
> +	if (!packed_ref_cache->lock)
> +		die("internal error: packed refs not locked");
> +	add_ref(get_packed_ref_dir(packed_ref_cache),
>  		create_ref_entry(refname, sha1, REF_ISPACKED, 1));
>  }
>  
> @@ -2035,6 +2049,52 @@ static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)
>  	return 0;
>  }
>  
> +int lock_packed_refs(struct lock_file *lock, int flags)
> +{
> +	struct packed_ref_cache *packed_ref_cache;
> +
> +	/* Discard the old cache because it might be invalid: */
> +	clear_packed_ref_cache(&ref_cache);
> +	if (hold_lock_file_for_update(lock, git_path("packed-refs"), flags) < 0)
> +		return -1;
> +	/* Read the current packed-refs while holding the lock: */
> +	packed_ref_cache = get_packed_ref_cache(&ref_cache);
> +	packed_ref_cache->lock = lock;
> +	return 0;
> +}
> +
> +int commit_packed_refs(void)
> +{
> +	struct packed_ref_cache *packed_ref_cache =
> +		get_packed_ref_cache(&ref_cache);
> +	int error = 0;
> +
> +	if (!packed_ref_cache->lock)
> +		die("internal error: packed-refs not locked");
> +	write_or_die(packed_ref_cache->lock->fd,
> +		     PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));
> +
> +	do_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),
> +				 0, write_packed_entry_fn,
> +				 &packed_ref_cache->lock->fd);
> +	if (commit_lock_file(packed_ref_cache->lock))
> +		error = -1;
> +	packed_ref_cache->lock = NULL;
> +	return error;
> +}
> +
> +void rollback_packed_refs(void)
> +{
> +	struct packed_ref_cache *packed_ref_cache =
> +		get_packed_ref_cache(&ref_cache);
> +
> +	if (!packed_ref_cache->lock)
> +		die("internal error: packed-refs not locked");
> +	rollback_lock_file(packed_ref_cache->lock);
> +	packed_ref_cache->lock = NULL;
> +	clear_packed_ref_cache(&ref_cache);
> +}
> +
>  struct ref_to_prune {
>  	struct ref_to_prune *next;
>  	unsigned char sha1[20];
> @@ -2153,23 +2213,19 @@ static struct lock_file packlock;
>  int pack_refs(unsigned int flags)
>  {
>  	struct pack_refs_cb_data cbdata;
> -	int fd;
>  
>  	memset(&cbdata, 0, sizeof(cbdata));
>  	cbdata.flags = flags;
>  
> -	fd = hold_lock_file_for_update(&packlock, git_path("packed-refs"),
> -				       LOCK_DIE_ON_ERROR);
> +	lock_packed_refs(&packlock, LOCK_DIE_ON_ERROR);
>  	cbdata.packed_refs = get_packed_refs(&ref_cache);
>  
>  	do_for_each_entry_in_dir(get_loose_refs(&ref_cache), 0,
>  				 pack_if_possible_fn, &cbdata);
>  
> -	write_or_die(fd, PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));
> -	do_for_each_entry_in_dir(cbdata.packed_refs, 0, write_packed_entry_fn, &fd);
> -
> -	if (commit_lock_file(&packlock) < 0)
> +	if (commit_packed_refs())
>  		die_errno("unable to overwrite old ref-pack file");
> +
>  	prune_refs(cbdata.ref_to_prune);
>  	return 0;
>  }
> @@ -2233,7 +2289,6 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)
>  
>  static int repack_without_ref(const char *refname)
>  {
> -	int fd;
>  	struct ref_dir *packed;
>  	struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
>  	struct string_list_item *ref_to_delete;
> @@ -2241,12 +2296,10 @@ static int repack_without_ref(const char *refname)
>  	if (!get_packed_ref(refname))
>  		return 0; /* refname does not exist in packed refs */
>  
> -	fd = hold_lock_file_for_update(&packlock, git_path("packed-refs"), 0);
> -	if (fd < 0) {
> +	if (lock_packed_refs(&packlock, 0)) {
>  		unable_to_lock_error(git_path("packed-refs"), errno);
>  		return error("cannot delete '%s' from packed refs", refname);
>  	}
> -	clear_packed_ref_cache(&ref_cache);
>  	packed = get_packed_refs(&ref_cache);
>  
>  	/* Remove refname from the cache: */
> @@ -2255,7 +2308,7 @@ static int repack_without_ref(const char *refname)
>  		 * The packed entry disappeared while we were
>  		 * acquiring the lock.
>  		 */
> -		rollback_lock_file(&packlock);
> +		rollback_packed_refs();
>  		return 0;
>  	}
>  
> @@ -2267,9 +2320,7 @@ static int repack_without_ref(const char *refname)
>  	}
>  
>  	/* Write what remains: */
> -	write_or_die(fd, PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));
> -	do_for_each_entry_in_dir(packed, 0, write_packed_entry_fn, &fd);
> -	return commit_lock_file(&packlock);
> +	return commit_packed_refs();
>  }
>  
>  int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
> diff --git a/refs.h b/refs.h
> index 246bf60..b275124 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -77,12 +77,35 @@ extern int for_each_rawref(each_ref_fn, void *);
>  extern void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname);
>  
>  /*
> - * Add a reference to the in-memory packed reference cache.  To actually
> - * write the reference to the packed-refs file, call pack_refs().
> + * Lock the packed-refs file for writing using the specified lock_file
> + * instance.  Flags is passed to hold_lock_file_for_update().  Return
> + * 0 on success.
> + */
> +extern int lock_packed_refs(struct lock_file *lock, int flags);
> +
> +/*
> + * Add a reference to the in-memory packed reference cache.  This may
> + * only be called while the packed-refs file is locked (see
> + * lock_packed_refs()).  To actually write the packed-refs file, call
> + * commit_packed_refs().
>   */
>  extern void add_packed_ref(const char *refname, const unsigned char *sha1);
>  
>  /*
> + * Write the current version of the packed refs cache from memory to
> + * disk.  The packed-refs file must already be locked for writing (see
> + * lock_packed_refs()).  Return zero on success.
> + */
> +extern int commit_packed_refs(void);
> +
> +/*
> + * Rollback the lockfile for the packed-refs file, and discard the
> + * in-memory packed reference cache.  (The packed-refs file will be
> + * read anew if it is needed again after this function is called.)
> + */
> +extern void rollback_packed_refs(void);
> +
> +/*
>   * Flags for controlling behaviour of pack_refs()
>   * PACK_REFS_PRUNE: Prune loose refs after packing
>   * PACK_REFS_ALL:   Pack _all_ refs, not just tags and already packed refs
Previous: Michael HaggertyNext: Michael Haggerty
Message 6 of 24 in “Fix some reference-related races”
  1. 00/12 Fix some reference-related racesMichael Haggerty, Jun 19, 2013
  2. 01/12 repack_without_ref(): split list curation and entry writingMichael Haggerty, Jun 19, 2013
  3. 02/12 pack_refs(): split creation of packed refs and entry writingMichael Haggerty, Jun 19, 2013
  4. 03/12 refs: wrap the packed refs cache in a level of indirectionMichael Haggerty, Jun 19, 2013
  5. 04/12 refs: implement simple transactions for the packed-refs fileMichael Haggerty, Jun 19, 2013
  6. Junio C HamanoJun 19, 2013
  7. Michael HaggertyJun 20, 2013
  8. Jeff KingJun 20, 2013
  9. Michael HaggertyJun 20, 2013
  10. Jeff KingJun 20, 2013
  11. Junio C HamanoJun 20, 2013
  12. Michael HaggertyJun 20, 2013
  13. Junio C HamanoJun 20, 2013
  14. 05/12 refs: manage lifetime of packed refs cache via reference countingMichael Haggerty, Jun 19, 2013
  15. 06/12 do_for_each_entry(): increment the packed refs cache refcountMichael Haggerty, Jun 19, 2013
  16. 07/12 packed_ref_cache: increment refcount when lockedMichael Haggerty, Jun 19, 2013
  17. 08/12 Extract a struct stat_data from cache_entryMichael Haggerty, Jun 19, 2013
  18. 09/12 add a stat_validity structMichael Haggerty, Jun 19, 2013
  19. 10/12 get_packed_ref_cache: reload packed-refs file when it changesMichael Haggerty, Jun 19, 2013
  20. 11/12 for_each_ref: load all loose refs before packed refsMichael Haggerty, Jun 19, 2013
  21. 12/12 refs: do not invalidate the packed-refs cache unnecessarilyMichael Haggerty, Jun 19, 2013
  22. Jeff KingJun 19, 2013
  23. Michael HaggertyJun 20, 2013
  24. Jeff KingJun 20, 2013

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.