On 05/06/2020 19:03, 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 an object ID or symref directly 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.
I think this patch is looking better now, I've had a couple more
thoughts though
> [...]
> diff --git a/refs.h b/refs.h
> index e010f8aec28..4dad8f24914 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -732,6 +732,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 refs_write_pseudoref(struct ref_store *refs, const char *pseudoref,
> + const struct object_id *oid,
> + const struct object_id *old_oid, struct strbuf *err);
> +int refs_delete_pseudoref(struct ref_store *refs, const char *pseudoref,
> + const struct object_id *old_oid);
> +int delete_pseudoref(const char *pseudoref, const struct object_id *old_oid);
> +
Do we need to expose these functions to the rest of git? In sequencer.c
we write the pseudo ref REBASE_HEAD when a rebase stops due to
conflicts. It is created with update_ref() and deleted with
delete_ref(), is there a reason can't we just use update_ref()/delete
ref() for the other pseudo refs?
> 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..df7553f4cc3 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);
> + 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);
> +
> + 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);
This could fail (especially on windows I think) and there are one or two
places in sequencer.c where we do actually check that the ref has
actually been deleted. Switching to calling delete_pseudoref() as it
stands will break those checks. What are the error semantics of normal
ref deletion where the file cannot be unlinked?
Best Wishes
Phillip
> + rollback_lock_file(&lock);
> + } else {
> + unlink(filename.buf);
> + }
> + ret = 0;
> +done:
> + strbuf_release(&filename);
> + return ret;
> +}
> +
> struct files_ref_iterator {
> struct ref_iterator base;
>
> @@ -3189,6 +3298,9 @@ struct ref_storage_be refs_be_files = {
> files_rename_ref,
> files_copy_ref,
>
> + files_write_pseudoref,
> + files_delete_pseudoref,
> +
> files_ref_iterator_begin,
> files_read_raw_ref,
>
> @@ -3198,5 +3310,5 @@ struct ref_storage_be refs_be_files = {
> files_reflog_exists,
> files_create_reflog,
> files_delete_reflog,
> - files_reflog_expire
> + files_reflog_expire,
> };
> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index 4458a0f69cc..08e8253a893 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -1590,6 +1590,22 @@ static int packed_copy_ref(struct ref_store *ref_store,
> BUG("packed reference store does not support copying references");
> }
>
> +static int packed_write_pseudoref(struct ref_store *ref_store,
> + const char *pseudoref,
> + const struct object_id *oid,
> + const struct object_id *old_oid,
> + struct strbuf *err)
> +{
> + BUG("packed reference store does not support writing pseudo-references");
> +}
> +
> +static int packed_delete_pseudoref(struct ref_store *ref_store,
> + const char *pseudoref,
> + const struct object_id *old_oid)
> +{
> + BUG("packed reference store does not support deleting pseudo-references");
> +}
> +
> static struct ref_iterator *packed_reflog_iterator_begin(struct ref_store *ref_store)
> {
> return empty_ref_iterator_begin();
> @@ -1656,6 +1672,9 @@ struct ref_storage_be refs_be_packed = {
> packed_rename_ref,
> packed_copy_ref,
>
> + packed_write_pseudoref,
> + packed_delete_pseudoref,
> +
> packed_ref_iterator_begin,
> packed_read_raw_ref,
>
> @@ -1665,5 +1684,5 @@ struct ref_storage_be refs_be_packed = {
> packed_reflog_exists,
> packed_create_reflog,
> packed_delete_reflog,
> - packed_reflog_expire
> + packed_reflog_expire,
> };
> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index 4271362d264..59b053d53a2 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -556,6 +556,21 @@ 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);
> +
> +/*
> + * Deletes a pseudoref. Deletion always succeeds (even if the pseudoref doesn't
> + * exist.), except if old_oid is specified. If it is, it can fail due to lock
> + * failure, failure reading the old OID, or an OID mismatch
> + */
> +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
> @@ -655,6 +670,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;
>
>
