Hi Han-Wen
Thanks for working on the reftable implementation. I've had a look at
this patch but bear in mind I'm not familiar with the refs code
On 11/05/2020 20:46, Han-Wen Nienhuys via GitGitGadget wrote:
> From: Han-Wen Nienhuys <hanwen@xxxxxxxxxx>
>
> Pseudorefs store transient data in in the repository. Examples are HEAD,
> CHERRY_PICK_HEAD, etc.
>
> These refs have always been read through the ref backends, but they were written
> in a one-off routine that wrote a object ID or symref directly wrote into
> .git/<pseudo_ref_name>.
>
> This causes problems when introducing a new ref storage backend. To remedy this,
> extend the ref backend implementation with a write_pseudoref_fn and
> update_pseudoref_fn.
>
> Signed-off-by: Han-Wen Nienhuys <hanwen@xxxxxxxxxx>
> ---
> refs.c | 115 +++++------------------------
> refs.h | 11 +++
> refs/files-backend.c | 164 +++++++++++++++++++++++++++++++++++-------
> refs/packed-backend.c | 40 ++++-------
> refs/refs-internal.h | 12 ++++
> 5 files changed, 192 insertions(+), 150 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 224ff66c7bb..fc72211a36f 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -739,101 +739,6 @@ long get_files_ref_lock_timeout_ms(void)
> return timeout_ms;
> }
>
> -static int write_pseudoref(const char *pseudoref, const struct object_id *oid,
> - const struct object_id *old_oid, struct strbuf *err)
> -{
> - const char *filename;
> - int fd;
> - struct lock_file lock = LOCK_INIT;
> - struct strbuf buf = STRBUF_INIT;
> - int ret = -1;
> -
> - if (!oid)
> - return 0;
> -
> - strbuf_addf(&buf, "%s\n", oid_to_hex(oid));
> -
> - filename = git_path("%s", pseudoref);
> - fd = hold_lock_file_for_update_timeout(&lock, filename, 0,
> - get_files_ref_lock_timeout_ms());
> - if (fd < 0) {
> - strbuf_addf(err, _("could not open '%s' for writing: %s"),
> - filename, strerror(errno));
> - goto done;
> - }
> -
> - if (old_oid) {
> - struct object_id actual_old_oid;
> -
> - if (read_ref(pseudoref, &actual_old_oid)) {
> - if (!is_null_oid(old_oid)) {
> - strbuf_addf(err, _("could not read ref '%s'"),
> - pseudoref);
> - rollback_lock_file(&lock);
> - goto done;
> - }
> - } else if (is_null_oid(old_oid)) {
> - strbuf_addf(err, _("ref '%s' already exists"),
> - pseudoref);
> - rollback_lock_file(&lock);
> - goto done;
> - } else if (!oideq(&actual_old_oid, old_oid)) {
> - strbuf_addf(err, _("unexpected object ID when writing '%s'"),
> - pseudoref);
> - rollback_lock_file(&lock);
> - goto done;
> - }
> - }
> -
> - if (write_in_full(fd, buf.buf, buf.len) < 0) {
> - strbuf_addf(err, _("could not write to '%s'"), filename);
> - rollback_lock_file(&lock);
> - goto done;
> - }
> -
> - commit_lock_file(&lock);
> - ret = 0;
> -done:
> - strbuf_release(&buf);
> - return ret;
> -}
> -
> -static int delete_pseudoref(const char *pseudoref, const struct object_id *old_oid)
> -{
> - const char *filename;
> -
> - filename = git_path("%s", pseudoref);
> -
> - if (old_oid && !is_null_oid(old_oid)) {
> - struct lock_file lock = LOCK_INIT;
> - int fd;
> - struct object_id actual_old_oid;
> -
> - fd = hold_lock_file_for_update_timeout(
> - &lock, filename, 0,
> - get_files_ref_lock_timeout_ms());
> - if (fd < 0) {
> - error_errno(_("could not open '%s' for writing"),
> - filename);
> - return -1;
> - }
> - if (read_ref(pseudoref, &actual_old_oid))
> - die(_("could not read ref '%s'"), pseudoref);
> - if (!oideq(&actual_old_oid, old_oid)) {
> - error(_("unexpected object ID when deleting '%s'"),
> - pseudoref);
> - rollback_lock_file(&lock);
> - return -1;
> - }
> -
> - unlink(filename);
> - rollback_lock_file(&lock);
> - } else {
> - unlink(filename);
> - }
> -
> - return 0;
> -}
>
> int refs_delete_ref(struct ref_store *refs, const char *msg,
> const char *refname,
> @@ -844,8 +749,7 @@ int refs_delete_ref(struct ref_store *refs, const char *msg,
> struct strbuf err = STRBUF_INIT;
>
> if (ref_type(refname) == REF_TYPE_PSEUDOREF) {
> - assert(refs == get_main_ref_store(the_repository));
I don't think you meant to delete this line, the equivalent line is left
untouched in the next hunk and there are comments in refs.h saying this
should only me called on the main repository
> - return delete_pseudoref(refname, old_oid);
> + return ref_store_delete_pseudoref(refs, refname, old_oid);
> }
>
> transaction = ref_store_transaction_begin(refs, &err);
> @@ -1172,7 +1076,8 @@ int refs_update_ref(struct ref_store *refs, const char *msg,
>
> if (ref_type(refname) == REF_TYPE_PSEUDOREF) {
> assert(refs == get_main_ref_store(the_repository));
> - ret = write_pseudoref(refname, new_oid, old_oid, &err);
> + ret = ref_store_write_pseudoref(refs, refname, new_oid, old_oid,
> + &err);
> } else {
> t = ref_store_transaction_begin(refs, &err);
> if (!t ||
> @@ -1441,6 +1346,20 @@ int head_ref(each_ref_fn fn, void *cb_data)
> return refs_head_ref(get_main_ref_store(the_repository), fn, cb_data);
> }
>
> +int ref_store_write_pseudoref(struct ref_store *refs, const char *pseudoref,
> + const struct object_id *oid,
> + const struct object_id *old_oid,
> + struct strbuf *err)
> +{
> + return refs->be->write_pseudoref(refs, pseudoref, oid, old_oid, err);
> +}
> +
> +int ref_store_delete_pseudoref(struct ref_store *refs, const char *pseudoref,
> + const struct object_id *old_oid)
> +{
> + return refs->be->delete_pseudoref(refs, pseudoref, old_oid);
> +}
> +
> struct ref_iterator *refs_ref_iterator_begin(
> struct ref_store *refs,
> const char *prefix, int trim, int flags)
> diff --git a/refs.h b/refs.h
> index 99ba9e331e5..d1d9361441b 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -728,6 +728,17 @@ int update_ref(const char *msg, const char *refname,
> const struct object_id *new_oid, const struct object_id *old_oid,
> unsigned int flags, enum action_on_err onerr);
>
> +/* Pseudorefs (eg. HEAD, CHERRY_PICK_HEAD) have a separate routines for updating
> + and deletion as they cannot take part in normal transactional updates.
> + Pseudorefs should only be written for the main repository.
> +*/
> +int ref_store_write_pseudoref(struct ref_store *refs, const char *pseudoref,
> + const struct object_id *oid,
> + const struct object_id *old_oid,
> + struct strbuf *err);
> +int ref_store_delete_pseudoref(struct ref_store *refs, const char *pseudoref,
> + const struct object_id *old_oid);
> +
> int parse_hide_refs_config(const char *var, const char *value, const char *);
>
> /*
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 6516c7bc8c8..53b891190be 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -731,6 +731,115 @@ static int lock_raw_ref(struct files_ref_store *refs,
> return ret;
> }
>
> +static int files_write_pseudoref(struct ref_store *ref_store,
> + const char *pseudoref,
> + const struct object_id *oid,
> + const struct object_id *old_oid,
> + struct strbuf *err)
> +{
> + struct files_ref_store *refs =
> + files_downcast(ref_store, REF_STORE_READ, "write_pseudoref");
> + int fd;
> + struct lock_file lock = LOCK_INIT;
> + struct strbuf filename = STRBUF_INIT;
> + struct strbuf buf = STRBUF_INIT;
> + int ret = -1;
> +
> + if (!oid)
> + return 0;
> +
> + strbuf_addf(&filename, "%s/%s", refs->gitdir, pseudoref);
The original used git_path(), this change looks correct to an uninformed
reviewer. As far as I can see the only changes to this function are to
accommodate the strbuf which is cleanup up correctly.
> + fd = hold_lock_file_for_update_timeout(&lock, filename.buf, 0,
> + get_files_ref_lock_timeout_ms());
> + if (fd < 0) {
> + strbuf_addf(err, _("could not open '%s' for writing: %s"),
> + buf.buf, strerror(errno));
> + goto done;
> + }
> +
> + if (old_oid) {
> + struct object_id actual_old_oid;
> +
> + if (read_ref(pseudoref, &actual_old_oid)) {
> + if (!is_null_oid(old_oid)) {
> + strbuf_addf(err, _("could not read ref '%s'"),
> + pseudoref);
> + rollback_lock_file(&lock);
> + goto done;
> + }
> + } else if (is_null_oid(old_oid)) {
> + strbuf_addf(err, _("ref '%s' already exists"),
> + pseudoref);
> + rollback_lock_file(&lock);
> + goto done;
> + } else if (!oideq(&actual_old_oid, old_oid)) {
> + strbuf_addf(err,
> + _("unexpected object ID when writing '%s'"),
> + pseudoref);
> + rollback_lock_file(&lock);
> + goto done;
> + }
> + }
> +
> + strbuf_addf(&buf, "%s\n", oid_to_hex(oid));
> + if (write_in_full(fd, buf.buf, buf.len) < 0) {
> + strbuf_addf(err, _("could not write to '%s'"), filename.buf);
> + rollback_lock_file(&lock);
> + goto done;
> + }
> +
> + commit_lock_file(&lock);
> + ret = 0;
> +done:
> + strbuf_release(&buf);
> + strbuf_release(&filename);
> + return ret;
> +}
> +
> +static int files_delete_pseudoref(struct ref_store *ref_store,
> + const char *pseudoref,
> + const struct object_id *old_oid)
> +{
> + struct files_ref_store *refs =
> + files_downcast(ref_store, REF_STORE_READ, "delete_pseudoref");
> + struct strbuf filename = STRBUF_INIT;
> + int ret = -1;
> +
> + strbuf_addf(&filename, "%s/%s", refs->gitdir, pseudoref);
Same comment as above, the memory management looks good.
> +
> + if (old_oid && !is_null_oid(old_oid)) {
> + struct lock_file lock = LOCK_INIT;
> + int fd;
> + struct object_id actual_old_oid;
> +
> + fd = hold_lock_file_for_update_timeout(
> + &lock, filename.buf, 0,
> + get_files_ref_lock_timeout_ms());
> + if (fd < 0) {
> + error_errno(_("could not open '%s' for writing"),
> + filename.buf);
> + goto done;
> + }
> + if (read_ref(pseudoref, &actual_old_oid))
> + die(_("could not read ref '%s'"), pseudoref);
> + if (!oideq(&actual_old_oid, old_oid)) {
> + error(_("unexpected object ID when deleting '%s'"),
> + pseudoref);
> + rollback_lock_file(&lock);
> + goto done;
> + }
> +
> + unlink(filename.buf);
> + rollback_lock_file(&lock);
> + } else {
> + unlink(filename.buf);
> + }
> + ret = 0;
> +done:
> + strbuf_release(&filename);
> + return ret;
> +}
> +
> struct files_ref_iterator {
> struct ref_iterator base;
>
> @@ -3173,30 +3282,31 @@ static int files_init_db(struct ref_store *ref_store, struct strbuf *err)
> return 0;
> }
>
> -struct ref_storage_be refs_be_files = {
> - NULL,
> - "files",
> - files_ref_store_create,
> - files_init_db,
> - files_transaction_prepare,
> - files_transaction_finish,
> - files_transaction_abort,
> - files_initial_transaction_commit,
> -
> - files_pack_refs,
> - files_create_symref,
> - files_delete_refs,
> - files_rename_ref,
> - files_copy_ref,
> -
> - files_ref_iterator_begin,
> - files_read_raw_ref,
> -
> - files_reflog_iterator_begin,
> - files_for_each_reflog_ent,
> - files_for_each_reflog_ent_reverse,
> - files_reflog_exists,
> - files_create_reflog,
> - files_delete_reflog,
> - files_reflog_expire
> -};
> +struct ref_storage_be refs_be_files = { NULL,
> + "files",
> + files_ref_store_create,
> + files_init_db,
> + files_transaction_prepare,
> + files_transaction_finish,
> + files_transaction_abort,
> + files_initial_transaction_commit,
> +
> + files_pack_refs,
> + files_create_symref,
> + files_delete_refs,
> + files_rename_ref,
> + files_copy_ref,
> +
> + files_write_pseudoref,
> + files_delete_pseudoref,
> +
> + files_ref_iterator_begin,
> + files_read_raw_ref,
> +
> + files_reflog_iterator_begin,
> + files_for_each_reflog_ent,
> + files_for_each_reflog_ent_reverse,
> + files_reflog_exists,
> + files_create_reflog,
> + files_delete_reflog,
> + files_reflog_expire };
The formatting has gone haywire
> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index 4458a0f69cc..8f9b85f0b0c 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -1641,29 +1641,19 @@ static int packed_reflog_expire(struct ref_store *ref_store,
> }
>
> struct ref_storage_be refs_be_packed = {
> - NULL,
> - "packed",
> - packed_ref_store_create,
> - packed_init_db,
> - packed_transaction_prepare,
> - packed_transaction_finish,
> - packed_transaction_abort,
> - packed_initial_transaction_commit,
> -
> - packed_pack_refs,
> - packed_create_symref,
> - packed_delete_refs,
> - packed_rename_ref,
> - packed_copy_ref,
> -
> - packed_ref_iterator_begin,
> - packed_read_raw_ref,
> -
> - packed_reflog_iterator_begin,
> - packed_for_each_reflog_ent,
> - packed_for_each_reflog_ent_reverse,
> - packed_reflog_exists,
> - packed_create_reflog,
> - packed_delete_reflog,
> - packed_reflog_expire
> + NULL, "packed", packed_ref_store_create, packed_init_db,
> + packed_transaction_prepare, packed_transaction_finish,
> + packed_transaction_abort, packed_initial_transaction_commit,
> +
> + packed_pack_refs, packed_create_symref, packed_delete_refs,
> + packed_rename_ref, packed_copy_ref,
> +
> + /* XXX */
> + NULL, NULL,
Should the wrappers above that invoke these virtual functions check they
are non-null before dereferencing them? It would be better to die with
BUG() than segfault.
> +
> + packed_ref_iterator_begin, packed_read_raw_ref,
> +
> + packed_reflog_iterator_begin, packed_for_each_reflog_ent,
> + packed_for_each_reflog_ent_reverse, packed_reflog_exists,
> + packed_create_reflog, packed_delete_reflog, packed_reflog_expire
Formatting again
I think this patch basically works but I'm concerned by the potential
NULL pointer dereference. While it's unfair to judge a patch by it's
formatting the changes to the formatting of existing code and the
dropped assertion rightly or wrongly gave me the impression lack of
attention which does make me concerned that there are other more serious
unintentional changes in the rest of the series.
Best Wishes
Phillip
> };
> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index 3490aac3a40..dabe18baea1 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -549,6 +549,15 @@ typedef int copy_ref_fn(struct ref_store *ref_store,
> const char *oldref, const char *newref,
> const char *logmsg);
>
> +typedef int write_pseudoref_fn(struct ref_store *ref_store,
> + const char *pseudoref,
> + const struct object_id *oid,
> + const struct object_id *old_oid,
> + struct strbuf *err);
> +typedef int delete_pseudoref_fn(struct ref_store *ref_store,
> + const char *pseudoref,
> + const struct object_id *old_oid);
> +
> /*
> * Iterate over the references in `ref_store` whose names start with
> * `prefix`. `prefix` is matched as a literal string, without regard
> @@ -648,6 +657,9 @@ struct ref_storage_be {
> rename_ref_fn *rename_ref;
> copy_ref_fn *copy_ref;
>
> + write_pseudoref_fn *write_pseudoref;
> + delete_pseudoref_fn *delete_pseudoref;
> +
> ref_iterator_begin_fn *iterator_begin;
> read_raw_ref_fn *read_raw_ref;
>
>
