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

Re: [PATCH 1/6] refs: create and use `ref_update_ref_must_exist()`

From
Patrick Steinhardt <ps@pks.im>
Date
May 16, 2024, 11:09 UTC
Message-ID
<ZkXpW0BPvR3vr2jx@tanuki>
In-Reply-To
<20240514124411.1037019-2-knayak@gitlab.com>
On Tue, May 14, 2024 at 02:44:06PM +0200, Karthik Nayak wrote:
Show 49 quoted lines
> From: Karthik Nayak <karthik.188@gmail.com>
> 
> The files and reftable backend, need to check if a ref must exist, so
> that the required validation can be done. A ref must exist only when the
> `old_oid` value of the update has been explicitly set and it is not the
> `null_oid` value.
> 
> Since we also support symrefs now, we need to ensure that even when
> `old_target` is set a ref must exist. While this was missed when we
> added symref support in transactions, there are no active users of this
> path. As we introduce the 'symref-verify' command in the upcoming
> commits, it is important to fix this.
> 
> So let's export this to a function called `ref_update_ref_must_exist()`
> and expose it internally via 'refs-internal.h'.
> 
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  refs.c                  | 6 ++++++
>  refs/files-backend.c    | 3 +--
>  refs/refs-internal.h    | 6 ++++++
>  refs/reftable-backend.c | 2 +-
>  4 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index fa5471d219..59858fafdb 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2863,3 +2863,9 @@ int ref_update_check_old_target(const char *referent, struct ref_update *update,
>  			    referent, update->old_target);
>  	return -1;
>  }
> +
> +int ref_update_ref_must_exist(struct ref_update *update)
> +{
> +	return (update->flags & REF_HAVE_OLD) &&
> +		(!is_null_oid(&update->old_oid) || update->old_target);
> +}
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 3957bfa579..2df204f891 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2411,8 +2411,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
>  			       struct strbuf *err)
>  {
>  	struct strbuf referent = STRBUF_INIT;
> -	int mustexist = (update->flags & REF_HAVE_OLD) &&
> -		!is_null_oid(&update->old_oid);
> +	int mustexist = ref_update_ref_must_exist(update);

Okay. So we didn't notice this was broken because even though we started writing symrefs via transactions now, none of the calles ever assert that the old ref exists?

Show 16 quoted lines
>  	int ret = 0;
>  	struct ref_lock *lock;
>  
> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index 53a6c5d842..5da3029e6c 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -765,4 +765,10 @@ int ref_update_has_null_new_value(struct ref_update *update);
>  int ref_update_check_old_target(const char *referent, struct ref_update *update,
>  				struct strbuf *err);
>  
> +/*
> + * Check if the ref must exist, this means that the old_oid or
> + * old_target is non NULL.
> + */
> +int ref_update_ref_must_exist(struct ref_update *update);

Seeing `ref_update_ref_must_exist()` as a standalone function wouldn't quite tell me what it really does. It sounds a bit like this would already assert the ref exists at the time of calling it.

We could call this `ref_upate_expects_existing_old_ref()`, which might clarify the intent a bit.

Patrick
Previous: Karthik NayakNext: Karthik Nayak
Message 3 of 50 in “update-ref: add symref support for --stdin”
  1. 0/6 update-ref: add symref support for --stdinKarthik Nayak, May 14, 2024
  2. 1/6 refs: create and use `ref_update_ref_must_exist()`Karthik Nayak, May 14, 2024
  3. Patrick SteinhardtMay 16, 2024
  4. Karthik NayakMay 17, 2024
  5. 2/6 update-ref: add support for 'symref-verify' commandKarthik Nayak, May 14, 2024
  6. Patrick SteinhardtMay 16, 2024
  7. Karthik NayakMay 17, 2024
  8. Patrick SteinhardtMay 21, 2024
  9. 3/6 update-ref: add support for 'symref-delete' commandKarthik Nayak, May 14, 2024
  10. Patrick SteinhardtMay 16, 2024
  11. 4/6 update-ref: add support for 'symref-create' commandKarthik Nayak, May 14, 2024
  12. Patrick SteinhardtMay 16, 2024
  13. Karthik NayakMay 19, 2024
  14. 5/6 reftable: pick either 'oid' or 'target' for new updatesKarthik Nayak, May 14, 2024
  15. 6/6 update-ref: add support for 'symref-update' commandKarthik Nayak, May 14, 2024
  16. Patrick SteinhardtMay 16, 2024
  17. Karthik NayakMay 21, 2024
  18. Karthik NayakMay 22, 2024
  19. 0/6 update-ref: add symref support for --stdinKarthik Nayak, May 22, 2024
  20. 1/6 refs: create and use `ref_update_expects_existing_old_ref()`Karthik Nayak, May 22, 2024
  21. 2/6 update-ref: add support for 'symref-verify' commandKarthik Nayak, May 22, 2024
  22. 3/6 update-ref: add support for 'symref-delete' commandKarthik Nayak, May 22, 2024
  23. 4/6 update-ref: add support for 'symref-create' commandKarthik Nayak, May 22, 2024
  24. 5/6 reftable: pick either 'oid' or 'target' for new updatesKarthik Nayak, May 22, 2024
  25. 6/6 update-ref: add support for 'symref-update' commandKarthik Nayak, May 22, 2024
  26. Junio C HamanoMay 25, 2024
  27. Karthik NayakMay 29, 2024
  28. Junio C HamanoMay 23, 2024
  29. Karthik NayakMay 23, 2024
  30. Junio C HamanoMay 23, 2024
  31. Karthik NayakMay 23, 2024
  32. Eric SunshineMay 23, 2024
  33. Junio C HamanoMay 23, 2024
  34. Eric SunshineMay 23, 2024
  35. Junio C HamanoMay 23, 2024
  36. Junio C HamanoMay 23, 2024
  37. Junio C HamanoMay 23, 2024
  38. 0/6 update-ref: add symref support for --stdinKarthik Nayak, May 30, 2024
  39. Patrick SteinhardtJun 5, 2024
  40. 1/6 refs: create and use `ref_update_expects_existing_old_ref()`Karthik Nayak, May 30, 2024
  41. 2/6 update-ref: add support for 'symref-verify' commandKarthik Nayak, May 30, 2024
  42. 3/6 update-ref: add support for 'symref-delete' commandKarthik Nayak, May 30, 2024
  43. Patrick SteinhardtJun 5, 2024
  44. Karthik NayakJun 5, 2024
  45. Junio C HamanoJun 5, 2024
  46. 4/6 update-ref: add support for 'symref-create' commandKarthik Nayak, May 30, 2024
  47. Patrick SteinhardtJun 5, 2024
  48. 5/6 reftable: pick either 'oid' or 'target' for new updatesKarthik Nayak, May 30, 2024
  49. 6/6 update-ref: add support for 'symref-update' commandKarthik Nayak, May 30, 2024
  50. Patrick SteinhardtJun 5, 2024

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.