* [PATCH v2 5/6] refs: alternate reftable ref backend implementation
From: Han-Wen Nienhuys via GitGitGadget @ 2023-09-20 13:02 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys, Han-Wen Nienhuys, Han-Wen Nienhuys
In-Reply-To: <pull.1574.v2.git.git.1695214968.gitgitgadget@gmail.com>
From: Han-Wen Nienhuys <hanwen@google.com>
This introduces the reftable backend as an alternative to the packed
backend.
This is an alternative to the approach which was explored in
https://github.com/git/git/pull/1215/. This alternative is simpler,
because we now longer have to worry about:
- pseudorefs and the HEAD ref
- worktrees
- commands that blur together files and references (cherry-pick, rebase)
This deviates from the spec that in
Documentation/technical/reftable.txt. It might be possible to update the
code such that all writes by default go to reftable directly. Then the
result would be compatible with an implementation that writes only
reftable (the reftable lock would still prevent races), but something
must be done about the HEAD ref (which JGit keeps in reftable too.)
Alternatively, JGit could be adapted to follow this implementation:
despite the code being there for 4 years now, I haven't heard of anyone
using it in production (exactly because CGit does not support it).
For this incremental path, the reftable format is arguably more
complex than necessary, as
- packed-refs doesn't support symrefs
- reflogs aren't moved into reftable
on the other hand, the code is already there, and it's well-structured
and well-tested.
refs/reftable-backend.c was created by cannibalizing the first version
of reftable support (github PR 1215 as mentioned above). It supports
reflogs and symrefs, even though those features are never exercised.
This implementation is a prototype, for the following reasons:
- no considerations of backward compatibility and configuring an
extension
- no support for converting between packed-refs and reftable
- no documentation
- test failures when setting GIT_TEST_REFTABLE=1.
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
Makefile | 1 +
config.mak.uname | 2 +-
contrib/workdir/git-new-workdir | 2 +-
refs/files-backend.c | 18 +-
refs/refs-internal.h | 1 +
refs/reftable-backend.c | 1658 +++++++++++++++++++++++++++++++
refs/reftable-backend.h | 8 +
7 files changed, 1686 insertions(+), 4 deletions(-)
create mode 100644 refs/reftable-backend.c
create mode 100644 refs/reftable-backend.h
diff --git a/Makefile b/Makefile
index 57763093653..272d3f7f1e9 100644
--- a/Makefile
+++ b/Makefile
@@ -1118,6 +1118,7 @@ LIB_OBJS += reflog.o
LIB_OBJS += refs.o
LIB_OBJS += refs/debug.o
LIB_OBJS += refs/files-backend.o
+LIB_OBJS += refs/reftable-backend.o
LIB_OBJS += refs/iterator.o
LIB_OBJS += refs/packed-backend.o
LIB_OBJS += refs/ref-cache.o
diff --git a/config.mak.uname b/config.mak.uname
index 3bb03f423a0..843829c02fd 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -743,7 +743,7 @@ vcxproj:
# Make .vcxproj files and add them
perl contrib/buildsystems/generate -g Vcxproj
- git add -f git.sln {*,*/lib,t/helper/*}/*.vcxproj
+ git add -f git.sln {*,*/lib,*/libreftable,t/helper/*}/*.vcxproj
# Generate the LinkOrCopyBuiltins.targets and LinkOrCopyRemoteHttp.targets file
(echo '<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">' && \
diff --git a/contrib/workdir/git-new-workdir b/contrib/workdir/git-new-workdir
index 888c34a5215..989197aace0 100755
--- a/contrib/workdir/git-new-workdir
+++ b/contrib/workdir/git-new-workdir
@@ -79,7 +79,7 @@ trap cleanup $siglist
# create the links to the original repo. explicitly exclude index, HEAD and
# logs/HEAD from the list since they are purely related to the current working
# directory, and should not be shared.
-for x in config refs logs/refs objects info hooks packed-refs remotes rr-cache svn
+for x in config refs logs/refs objects info hooks packed-refs remotes rr-cache svn reftable
do
# create a containing directory if needed
case $x in
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 5d288bf38bb..2cd596bbeba 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1,4 +1,5 @@
#include "../git-compat-util.h"
+#include "../abspath.h"
#include "../config.h"
#include "../copy.h"
#include "../environment.h"
@@ -9,6 +10,7 @@
#include "refs-internal.h"
#include "ref-cache.h"
#include "packed-backend.h"
+#include "reftable-backend.h"
#include "../ident.h"
#include "../iterator.h"
#include "../dir-iterator.h"
@@ -98,13 +100,25 @@ static struct ref_store *files_ref_store_create(struct repository *repo,
struct files_ref_store *refs = xcalloc(1, sizeof(*refs));
struct ref_store *ref_store = (struct ref_store *)refs;
struct strbuf sb = STRBUF_INIT;
+ int has_reftable, has_packed;
base_ref_store_init(ref_store, repo, gitdir, &refs_be_files);
refs->store_flags = flags;
get_common_dir_noenv(&sb, gitdir);
refs->gitcommondir = strbuf_detach(&sb, NULL);
- refs->packed_ref_store =
- packed_ref_store_create(repo, refs->gitcommondir, flags);
+
+ strbuf_addf(&sb, "%s/reftable", refs->gitcommondir);
+ has_reftable = is_directory(sb.buf);
+ strbuf_reset(&sb);
+ strbuf_addf(&sb, "%s/packed-refs", refs->gitcommondir);
+ has_packed = file_exists(sb.buf);
+
+ if (!has_packed && !has_reftable)
+ has_reftable = git_env_bool("GIT_TEST_REFTABLE", 0);
+
+ refs->packed_ref_store = has_reftable
+ ? git_reftable_ref_store_create(repo, refs->gitcommondir, flags)
+ : packed_ref_store_create(repo, refs->gitcommondir, flags);
chdir_notify_reparent("files-backend $GIT_DIR", &refs->base.gitdir);
chdir_notify_reparent("files-backend $GIT_COMMONDIR",
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 0a15e8a2ac8..13bd9c79f3c 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -699,6 +699,7 @@ struct ref_storage_be {
};
extern struct ref_storage_be refs_be_files;
+extern struct ref_storage_be refs_be_reftable;
extern struct ref_storage_be refs_be_packed;
/*
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
new file mode 100644
index 00000000000..16064fff198
--- /dev/null
+++ b/refs/reftable-backend.c
@@ -0,0 +1,1658 @@
+#include "../git-compat-util.h"
+#include "../abspath.h"
+#include "../chdir-notify.h"
+#include "../config.h"
+#include "../environment.h"
+#include "../hash.h"
+#include "../hex.h"
+#include "../iterator.h"
+#include "../ident.h"
+#include "../lockfile.h"
+#include "../object.h"
+#include "../path.h"
+#include "../refs.h"
+#include "../reftable/reftable-stack.h"
+#include "../reftable/reftable-record.h"
+#include "../reftable/reftable-error.h"
+#include "../reftable/reftable-blocksource.h"
+#include "../reftable/reftable-reader.h"
+#include "../reftable/reftable-iterator.h"
+#include "../reftable/reftable-merged.h"
+#include "../reftable/reftable-generic.h"
+#include "../worktree.h"
+#include "refs-internal.h"
+#include "reftable-backend.h"
+#include "../repository.h"
+
+extern struct ref_storage_be refs_be_reftable;
+
+struct git_reftable_ref_store {
+ struct ref_store base;
+ unsigned int store_flags;
+
+ int err;
+ char *repo_dir;
+ char *reftable_dir;
+
+ struct reftable_stack *main_stack;
+
+ struct reftable_write_options write_options;
+};
+
+static struct reftable_stack *stack_for(struct git_reftable_ref_store *store,
+ const char *refname)
+{
+ return store->main_stack;
+}
+
+static int should_log(const char *refname)
+{
+ return log_all_ref_updates != LOG_REFS_NONE &&
+ (log_all_ref_updates == LOG_REFS_ALWAYS ||
+ log_all_ref_updates == LOG_REFS_UNSET ||
+ should_autocreate_reflog(refname));
+}
+
+static const char *bare_ref_name(const char *ref)
+{
+ const char *stripped;
+ parse_worktree_ref(ref, NULL, NULL, &stripped);
+ return stripped;
+}
+
+static int git_reftable_read_raw_ref(struct ref_store *ref_store,
+ const char *refname, struct object_id *oid,
+ struct strbuf *referent,
+ unsigned int *type, int *failure_errno);
+
+static void clear_reftable_log_record(struct reftable_log_record *log)
+{
+ switch (log->value_type) {
+ case REFTABLE_LOG_UPDATE:
+ /* when we write log records, the hashes are owned by a struct
+ * oid */
+ log->value.update.old_hash = NULL;
+ log->value.update.new_hash = NULL;
+ break;
+ case REFTABLE_LOG_DELETION:
+ break;
+ }
+ reftable_log_record_release(log);
+}
+
+static void fill_reftable_log_record(struct reftable_log_record *log)
+{
+ const char *info = git_committer_info(0);
+ struct ident_split split = { NULL };
+ int result = split_ident_line(&split, info, strlen(info));
+ int sign = 1;
+ assert(0 == result);
+
+ reftable_log_record_release(log);
+ log->value_type = REFTABLE_LOG_UPDATE;
+ log->value.update.name =
+ xstrndup(split.name_begin, split.name_end - split.name_begin);
+ log->value.update.email =
+ xstrndup(split.mail_begin, split.mail_end - split.mail_begin);
+ log->value.update.time = atol(split.date_begin);
+ if (*split.tz_begin == '-') {
+ sign = -1;
+ split.tz_begin++;
+ }
+ if (*split.tz_begin == '+') {
+ sign = 1;
+ split.tz_begin++;
+ }
+
+ log->value.update.tz_offset = sign * atoi(split.tz_begin);
+}
+
+struct ref_store *git_reftable_ref_store_create(struct repository *repo,
+ const char *gitdir,
+ unsigned int store_flags)
+{
+ struct git_reftable_ref_store *refs = xcalloc(1, sizeof(*refs));
+ struct ref_store *ref_store = (struct ref_store *)refs;
+ struct strbuf sb = STRBUF_INIT;
+ int shared = get_shared_repository();
+ if (shared < 0)
+ shared = -shared;
+
+ refs->write_options.block_size = 4096;
+ refs->write_options.hash_id = the_hash_algo->format_id;
+ if (shared && (shared & 0600))
+ refs->write_options.default_permissions = shared;
+
+ /* XXX should this use `path` or `gitdir.buf` ? */
+ base_ref_store_init(ref_store, repo, gitdir, &refs_be_reftable);
+ refs->store_flags = store_flags;
+ strbuf_addf(&sb, "%s/reftable", gitdir);
+ refs->reftable_dir = xstrdup(sb.buf);
+ safe_create_dir(refs->reftable_dir, 1);
+
+ refs->base.repo = repo;
+ strbuf_reset(&sb);
+
+ refs->err = reftable_new_stack(&refs->main_stack, refs->reftable_dir,
+ refs->write_options);
+ assert(refs->err != REFTABLE_API_ERROR);
+
+ strbuf_release(&sb);
+
+ /* TODO something with chdir_notify_reparent() ? */
+
+ return ref_store;
+}
+
+static int git_reftable_init_db(struct ref_store *ref_store, struct strbuf *err)
+{
+ struct git_reftable_ref_store *refs =
+ (struct git_reftable_ref_store *)ref_store;
+ safe_create_dir(refs->reftable_dir, 1);
+ return 0;
+}
+
+struct git_reftable_iterator {
+ struct ref_iterator base;
+ struct reftable_iterator iter;
+ struct reftable_ref_record ref;
+ struct object_id oid;
+ struct ref_store *ref_store;
+
+ unsigned int flags;
+ int err;
+ const char *prefix;
+};
+
+static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator)
+{
+ struct git_reftable_iterator *ri =
+ (struct git_reftable_iterator *)ref_iterator;
+ struct git_reftable_ref_store *refs =
+ (struct git_reftable_ref_store *)ri->ref_store;
+
+ while (ri->err == 0) {
+ int signed_flags = 0;
+ ri->err = reftable_iterator_next_ref(&ri->iter, &ri->ref);
+ if (ri->err) {
+ break;
+ }
+
+ ri->base.flags = 0;
+
+ if (!strcmp(ri->ref.refname, "HEAD")) {
+ /*
+ HEAD should not be produced by default.Other
+ pseudorefs (FETCH_HEAD etc.) shouldn't be
+ stored in reftables at all.
+ */
+ continue;
+ }
+ ri->base.refname = ri->ref.refname;
+ if (ri->prefix &&
+ strncmp(ri->prefix, ri->ref.refname, strlen(ri->prefix))) {
+ ri->err = 1;
+ break;
+ }
+ if (ri->flags & DO_FOR_EACH_PER_WORKTREE_ONLY &&
+ parse_worktree_ref(ri->base.refname, NULL, NULL, NULL) !=
+ REF_WORKTREE_CURRENT)
+ continue;
+
+ if (ri->flags & DO_FOR_EACH_INCLUDE_BROKEN &&
+ check_refname_format(ri->base.refname,
+ REFNAME_ALLOW_ONELEVEL)) {
+ /* This is odd, as REF_BAD_NAME and REF_ISBROKEN are
+ orthogonal, but it's what the spec says and the
+ files-backend does. */
+ ri->base.flags |= REF_BAD_NAME | REF_ISBROKEN;
+ break;
+ }
+
+ switch (ri->ref.value_type) {
+ case REFTABLE_REF_VAL1:
+ oidread(&ri->oid, ri->ref.value.val1);
+ break;
+ case REFTABLE_REF_VAL2:
+ oidread(&ri->oid, ri->ref.value.val2.value);
+ break;
+ case REFTABLE_REF_SYMREF:
+ ri->base.flags = REF_ISSYMREF;
+ break;
+ default:
+ abort();
+ }
+
+ ri->base.oid = &ri->oid;
+ if (!(ri->flags & DO_FOR_EACH_INCLUDE_BROKEN) &&
+ !ref_resolves_to_object(ri->base.refname, refs->base.repo,
+ ri->base.oid, ri->base.flags)) {
+ continue;
+ }
+
+ /* Arguably, resolving recursively following symlinks should be
+ * lifted to refs.c because it is shared between reftable and
+ * the files backend, but it's here now.
+ */
+ if (!refs_resolve_ref_unsafe(ri->ref_store, ri->ref.refname,
+ RESOLVE_REF_READING, &ri->oid,
+ &signed_flags)) {
+ ri->base.flags = signed_flags;
+ if (ri->ref.value_type == REFTABLE_REF_SYMREF &&
+ ri->flags & DO_FOR_EACH_OMIT_DANGLING_SYMREFS)
+ continue;
+
+ if (ri->ref.value_type == REFTABLE_REF_SYMREF &&
+ !(ri->flags & DO_FOR_EACH_INCLUDE_BROKEN) &&
+ (ri->base.flags & REF_ISBROKEN)) {
+ continue;
+ }
+
+ if (is_null_oid(&ri->oid)) {
+ oidclr(&ri->oid);
+ ri->base.flags |= REF_ISBROKEN;
+ }
+ }
+ break;
+ }
+
+ if (ri->err > 0) {
+ return ITER_DONE;
+ }
+ if (ri->err < 0) {
+ return ITER_ERROR;
+ }
+
+ return ITER_OK;
+}
+
+static int reftable_ref_iterator_peel(struct ref_iterator *ref_iterator,
+ struct object_id *peeled)
+{
+ struct git_reftable_iterator *ri =
+ (struct git_reftable_iterator *)ref_iterator;
+ if (ri->ref.value_type == REFTABLE_REF_VAL2) {
+ oidread(peeled, ri->ref.value.val2.target_value);
+ return 0;
+ }
+
+ return -1;
+}
+
+static int reftable_ref_iterator_abort(struct ref_iterator *ref_iterator)
+{
+ struct git_reftable_iterator *ri =
+ (struct git_reftable_iterator *)ref_iterator;
+ reftable_ref_record_release(&ri->ref);
+ reftable_iterator_destroy(&ri->iter);
+ return 0;
+}
+
+static struct ref_iterator_vtable reftable_ref_iterator_vtable = {
+ reftable_ref_iterator_advance, reftable_ref_iterator_peel,
+ reftable_ref_iterator_abort
+};
+
+static struct ref_iterator *
+git_reftable_ref_iterator_begin(struct ref_store *ref_store, const char *prefix,
+ const char **exclude_patterns_TODO,
+ unsigned int flags)
+{
+ struct git_reftable_ref_store *refs =
+ (struct git_reftable_ref_store *)ref_store;
+ struct git_reftable_iterator *ri = xcalloc(1, sizeof(*ri));
+
+ if (refs->err < 0) {
+ ri->err = refs->err;
+ } else {
+ struct reftable_merged_table *mt =
+ reftable_stack_merged_table(refs->main_stack);
+ ri->err = reftable_merged_table_seek_ref(mt, &ri->iter, prefix);
+ }
+
+ base_ref_iterator_init(&ri->base, &reftable_ref_iterator_vtable, 1);
+ ri->prefix = prefix;
+ ri->base.oid = &ri->oid;
+ ri->flags = flags;
+ ri->ref_store = ref_store;
+ return &ri->base;
+}
+
+static struct ref_transaction *git_reftable_transaction_begin(struct ref_store *ref_store,
+ struct strbuf *errbuf)
+{
+ struct git_reftable_ref_store *refs =
+ (struct git_reftable_ref_store *)ref_store;
+ struct reftable_addition *add = NULL;
+ struct ref_transaction *transaction = NULL;
+ struct reftable_stack *stack = refs->main_stack;
+
+ int err = refs->err;
+ if (err < 0) {
+ goto done;
+ }
+
+ err = reftable_stack_reload(stack);
+ if (err) {
+ goto done;
+ }
+
+ err = reftable_stack_new_addition(&add, stack);
+ if (err) {
+ strbuf_addf(errbuf, "reftable: transaction begin: %s",
+ reftable_error_str(err));
+ goto done;
+ }
+
+ CALLOC_ARRAY(transaction, 1);
+ transaction->backend_data = add;
+done:
+ return transaction;
+}
+
+static int git_reftable_transaction_prepare(struct ref_store *ref_store,
+ struct ref_transaction *transaction,
+ struct strbuf *errbuf)
+{
+ transaction->state = REF_TRANSACTION_PREPARED;
+ return 0;
+}
+
+static int git_reftable_transaction_abort(struct ref_store *ref_store,
+ struct ref_transaction *transaction,
+ struct strbuf *err)
+{
+ struct reftable_addition *add =
+ (struct reftable_addition *)transaction->backend_data;
+ reftable_addition_destroy(add);
+ transaction->backend_data = NULL;
+
+ /* XXX. Shouldn't this be handled generically in refs.c? */
+ transaction->state = REF_TRANSACTION_CLOSED;
+ return 0;
+}
+
+static int reftable_check_old_oid(struct ref_store *refs, const char *refname,
+ struct object_id *want_oid)
+{
+ struct object_id out_oid;
+ int out_flags = 0;
+ const char *resolved = refs_resolve_ref_unsafe(
+ refs, refname, RESOLVE_REF_READING, &out_oid, &out_flags);
+ if (is_null_oid(want_oid) != !resolved) {
+ return REFTABLE_LOCK_ERROR;
+ }
+
+ if (resolved && !oideq(&out_oid, want_oid)) {
+ return REFTABLE_LOCK_ERROR;
+ }
+
+ return 0;
+}
+
+static int ref_update_cmp(const void *a, const void *b)
+{
+ return strcmp((*(struct ref_update **)a)->refname,
+ (*(struct ref_update **)b)->refname);
+}
+
+static int write_transaction_table(struct reftable_writer *writer, void *arg)
+{
+ struct ref_transaction *transaction = (struct ref_transaction *)arg;
+ struct git_reftable_ref_store *refs =
+ (struct git_reftable_ref_store *)transaction->ref_store;
+ struct reftable_stack *stack =
+ stack_for(refs, transaction->updates[0]->refname);
+ uint64_t ts = reftable_stack_next_update_index(stack);
+ int err = 0;
+ int i = 0;
+ int log_count = 0;
+ struct reftable_log_record *logs =
+ calloc(transaction->nr, sizeof(*logs));
+ struct ref_update **sorted =
+ malloc(transaction->nr * sizeof(struct ref_update *));
+ struct reftable_merged_table *mt = reftable_stack_merged_table(stack);
+ struct reftable_table tab = { NULL };
+ struct reftable_ref_record ref = { NULL };
+ reftable_table_from_merged_table(&tab, mt);
+ COPY_ARRAY(sorted, transaction->updates, transaction->nr);
+ QSORT(sorted, transaction->nr, ref_update_cmp);
+ reftable_writer_set_limits(writer, ts, ts);
+
+ for (i = 0; i < transaction->nr; i++) {
+ struct ref_update *u = sorted[i];
+ struct reftable_log_record *log = &logs[log_count];
+ struct object_id old_id = *null_oid();
+
+ log->value.update.new_hash = NULL;
+ log->value.update.old_hash = NULL;
+ if ((u->flags & REF_FORCE_CREATE_REFLOG) ||
+ should_log(u->refname))
+ log_count++;
+ fill_reftable_log_record(log);
+
+ log->update_index = ts;
+ log->value_type = REFTABLE_LOG_UPDATE;
+ log->refname = xstrdup(u->refname);
+ log->value.update.new_hash = u->new_oid.hash;
+ log->value.update.message =
+ xstrndup(u->msg, refs->write_options.block_size / 2);
+
+ err = reftable_table_read_ref(&tab, u->refname, &ref);
+ if (err < 0)
+ goto done;
+ else if (err > 0) {
+ err = 0;
+ }
+
+ /* XXX if this is a symref (say, HEAD), should we deref the
+ * symref and check the update.old_hash against the referent? */
+ if (ref.value_type == REFTABLE_REF_VAL2 ||
+ ref.value_type == REFTABLE_REF_VAL1)
+ oidread(&old_id, ref.value.val1);
+
+ /* XXX fold together with the old_id check below? */
+ log->value.update.old_hash = old_id.hash;
+ if (u->flags & REF_LOG_ONLY) {
+ continue;
+ }
+
+ if (u->flags & REF_HAVE_NEW) {
+ struct reftable_ref_record ref = { NULL };
+ struct object_id peeled;
+
+ int peel_error = peel_object(&u->new_oid, &peeled);
+ ref.refname = (char *)u->refname;
+ ref.update_index = ts;
+
+ if (!peel_error) {
+ ref.value_type = REFTABLE_REF_VAL2;
+ ref.value.val2.target_value = peeled.hash;
+ ref.value.val2.value = u->new_oid.hash;
+ } else if (!is_null_oid(&u->new_oid)) {
+ ref.value_type = REFTABLE_REF_VAL1;
+ ref.value.val1 = u->new_oid.hash;
+ }
+
+ err = reftable_writer_add_ref(writer, &ref);
+ if (err < 0) {
+ goto done;
+ }
+ }
+ }
+
+ for (i = 0; i < log_count; i++) {
+ err = reftable_writer_add_log(writer, &logs[i]);
+ logs[i].value.update.new_hash = NULL;
+ logs[i].value.update.old_hash = NULL;
+ clear_reftable_log_record(&logs[i]);
+ if (err < 0) {
+ goto done;
+ }
+ }
+
+done:
+ assert(err != REFTABLE_API_ERROR);
+ reftable_ref_record_release(&ref);
+ free(logs);
+ free(sorted);
+ return err;
+}
+
+static int git_reftable_transaction_finish(struct ref_store *ref_store,
+ struct ref_transaction *transaction,
+ struct strbuf *errmsg)
+{
+ struct git_reftable_ref_store *refs =
+ (struct git_reftable_ref_store *)transaction->ref_store;
+ struct reftable_addition *add =
+ (struct reftable_addition *)transaction->backend_data;
+ int err = 0;
+ int i;
+
+ for (i = 0; i < transaction->nr; i++) {
+ struct ref_update *u = transaction->updates[i];
+ if (u->flags & REF_HAVE_OLD) {
+ err = reftable_check_old_oid(transaction->ref_store,
+ u->refname, &u->old_oid);
+ if (err < 0) {
+ goto done;
+ }
+ }
+ }
+ if (transaction->nr) {
+ err = reftable_addition_add(add, &write_transaction_table,
+ transaction);
+ if (err < 0) {
+ goto done;
+ }
+ }
+
+ err = reftable_addition_commit(add);
+
+ if (!err)
+ err = reftable_stack_auto_compact(refs->main_stack);
+done:
+ assert(err != REFTABLE_API_ERROR);
+ reftable_addition_destroy(add);
+ transaction->state = REF_TRANSACTION_CLOSED;
+ transaction->backend_data = NULL;
+ if (err) {
+ strbuf_addf(errmsg, "reftable: transaction failure: %s",
+ reftable_error_str(err));
+ return -1;
+ }
+ return err;
+}
+
+static int
+git_reftable_transaction_initial_commit(struct ref_store *ref_store,
+ struct ref_transaction *transaction,
+ struct strbuf *errmsg)
+{
+ int err = git_reftable_transaction_prepare(ref_store, transaction,
+ errmsg);
+ if (err)
+ return err;
+
+ return git_reftable_transaction_finish(ref_store, transaction, errmsg);
+}
+
+struct write_delete_refs_arg {
+ struct git_reftable_ref_store *refs;
+ struct reftable_stack *stack;
+ struct string_list *refnames;
+ const char *logmsg;
+ unsigned int flags;
+};
+
+static int write_delete_refs_table(struct reftable_writer *writer, void *argv)
+{
+ struct write_delete_refs_arg *arg =
+ (struct write_delete_refs_arg *)argv;
+ uint64_t ts = reftable_stack_next_update_index(arg->stack);
+ int err = 0;
+ int i = 0;
+
+ reftable_writer_set_limits(writer, ts, ts);
+ for (i = 0; i < arg->refnames->nr; i++) {
+ struct reftable_ref_record ref = {
+ .refname = (char *)arg->refnames->items[i].string,
+ .value_type = REFTABLE_REF_DELETION,
+ .update_index = ts,
+ };
+ err = reftable_writer_add_ref(writer, &ref);
+ if (err < 0) {
+ return err;
+ }
+ }
+
+ for (i = 0; i < arg->refnames->nr; i++) {
+ struct reftable_log_record log = {
+ .update_index = ts,
+ };
+ struct reftable_ref_record current = { NULL };
+ fill_reftable_log_record(&log);
+ log.update_index = ts;
+ log.refname = xstrdup(arg->refnames->items[i].string);
+ if (!should_log(log.refname)) {
+ continue;
+ }
+ log.value.update.message = xstrndup(
+ arg->logmsg, arg->refs->write_options.block_size / 2);
+ log.value.update.new_hash = NULL;
+ log.value.update.old_hash = NULL;
+ if (reftable_stack_read_ref(arg->stack, log.refname,
+ ¤t) == 0) {
+ log.value.update.old_hash =
+ reftable_ref_record_val1(¤t);
+ }
+ err = reftable_writer_add_log(writer, &log);
+ log.value.update.old_hash = NULL;
+ reftable_ref_record_release(¤t);
+
+ clear_reftable_log_record(&log);
+ if (err < 0) {
+ return err;
+ }
+ }
+ return 0;
+}
+
+static int git_reftable_delete_refs(struct ref_store *ref_store,
+ const char *msg,
+ struct string_list *refnames,
+ unsigned int flags)
+{
+ struct git_reftable_ref_store *refs =
+ (struct git_reftable_ref_store *)ref_store;
+ struct reftable_stack *stack = stack_for(
+ refs, refnames->nr ? refnames->items[0].string : NULL);
+ struct write_delete_refs_arg arg = {
+ .refs = refs,
+ .stack = stack,
+ .refnames = refnames,
+ .logmsg = msg,
+ .flags = flags,
+ };
+ int err = refs->err;
+ if (err < 0) {
+ goto done;
+ }
+
+ string_list_sort(refnames);
+ err = reftable_stack_reload(stack);
+ if (err) {
+ goto done;
+ }
+ err = reftable_stack_add(stack, &write_delete_refs_table, &arg);
+done:
+ assert(err != REFTABLE_API_ERROR);
+ return err;
+}
+
+static int git_reftable_pack_refs(struct ref_store *ref_store,
+ struct pack_refs_opts *opts)
+{
+ struct git_reftable_ref_store *refs =
+ (struct git_reftable_ref_store *)ref_store;
+
+ int err = refs->err;
+ if (err < 0) {
+ return err;
+ }
+ err = reftable_stack_compact_all(refs->main_stack, NULL);
+ if (err == 0)
+ err = reftable_stack_clean(refs->main_stack);
+
+ return err;
+}
+
+struct write_create_symref_arg {
+ struct git_reftable_ref_store *refs;
+ struct reftable_stack *stack;
+ const char *refname;
+ const char *target;
+ const char *logmsg;
+};
+
+static int write_create_symref_table(struct reftable_writer *writer, void *arg)
+{
+ struct write_create_symref_arg *create =
+ (struct write_create_symref_arg *)arg;
+ uint64_t ts = reftable_stack_next_update_index(create->stack);
+ int err = 0;
+
+ struct reftable_ref_record ref = {
+ .refname = (char *)create->refname,
+ .value_type = REFTABLE_REF_SYMREF,
+ .value.symref = (char *)create->target,
+ .update_index = ts,
+ };
+ reftable_writer_set_limits(writer, ts, ts);
+ err = reftable_writer_add_ref(writer, &ref);
+ if (err == 0) {
+ struct reftable_log_record log = { NULL };
+ struct object_id new_oid;
+ struct object_id old_oid;
+
+ fill_reftable_log_record(&log);
+ log.refname = xstrdup(create->refname);
+ if (!should_log(log.refname)) {
+ return err;
+ }
+ log.update_index = ts;
+ log.value.update.message =
+ xstrndup(create->logmsg,
+ create->refs->write_options.block_size / 2);
+ if (refs_resolve_ref_unsafe(
+ (struct ref_store *)create->refs, create->refname,
+ RESOLVE_REF_READING, &old_oid, NULL)) {
+ log.value.update.old_hash = old_oid.hash;
+ }
+
+ if (refs_resolve_ref_unsafe((struct ref_store *)create->refs,
+ create->target, RESOLVE_REF_READING,
+ &new_oid, NULL)) {
+ log.value.update.new_hash = new_oid.hash;
+ }
+
+ if (log.value.update.old_hash ||
+ log.value.update.new_hash) {
+ err = reftable_writer_add_log(writer, &log);
+ }
+ log.refname = NULL;
+ log.value.update.message = NULL;
+ log.value.update.old_hash = NULL;
+ log.value.update.new_hash = NULL;
+ clear_reftable_log_record(&log);
+ }
+ return err;
+}
+
+static int git_reftable_create_symref(struct ref_store *ref_store,
+ const char *refname, const char *target,
+ const char *logmsg)
+{
+ struct git_reftable_ref_store *refs =
+ (struct git_reftable_ref_store *)ref_store;
+ struct reftable_stack *stack = stack_for(refs, refname);
+ struct write_create_symref_arg arg = { .refs = refs,
+ .stack = stack,
+ .refname = refname,
+ .target = target,
+ .logmsg = logmsg };
+ int err = refs->err;
+ if (err < 0) {
+ goto done;
+ }
+ err = reftable_stack_reload(stack);
+ if (err) {
+ goto done;
+ }
+ err = reftable_stack_add(stack, &write_create_symref_table, &arg);
+done:
+ assert(err != REFTABLE_API_ERROR);
+ return err;
+}
+
+struct write_rename_arg {
+ struct git_reftable_ref_store *refs;
+ struct reftable_stack *stack;
+ const char *oldname;
+ const char *newname;
+ const char *logmsg;
+};
+
+static int write_rename_table(struct reftable_writer *writer, void *argv)
+{
+ struct write_rename_arg *arg = (struct write_rename_arg *)argv;
+ uint64_t ts = reftable_stack_next_update_index(arg->stack);
+ struct reftable_ref_record old_ref = { NULL };
+ struct reftable_ref_record new_ref = { NULL };
+ int err = reftable_stack_read_ref(arg->stack, arg->oldname, &old_ref);
+ struct reftable_ref_record todo[2] = {
+ {
+ .refname = (char *)arg->oldname,
+ .update_index = ts,
+ .value_type = REFTABLE_REF_DELETION,
+ },
+ old_ref,
+ };
+
+ if (err) {
+ goto done;
+ }
+
+ /* git-branch supports a --force, but the check is not atomic. */
+ if (!reftable_stack_read_ref(arg->stack, arg->newname, &new_ref)) {
+ goto done;
+ }
+
+ reftable_writer_set_limits(writer, ts, ts);
+
+ todo[1].update_index = ts;
+ todo[1].refname = (char *)arg->newname;
+
+ err = reftable_writer_add_refs(writer, todo, 2);
+ if (err < 0) {
+ goto done;
+ }
+
+ if (reftable_ref_record_val1(&old_ref)) {
+ uint8_t *val1 = reftable_ref_record_val1(&old_ref);
+ struct reftable_log_record todo[2] = { { NULL } };
+ int firstlog = 0;
+ int lastlog = 2;
+ char *msg = xstrndup(arg->logmsg,
+ arg->refs->write_options.block_size / 2);
+ fill_reftable_log_record(&todo[0]);
+ fill_reftable_log_record(&todo[1]);
+
+ todo[0].refname = xstrdup(arg->oldname);
+ todo[0].update_index = ts;
+ todo[0].value.update.message = msg;
+ todo[0].value.update.old_hash = val1;
+ todo[0].value.update.new_hash = NULL;
+
+ todo[1].refname = xstrdup(arg->newname);
+ todo[1].update_index = ts;
+ todo[1].value.update.old_hash = NULL;
+ todo[1].value.update.new_hash = val1;
+ todo[1].value.update.message = xstrdup(msg);
+
+ if (!should_log(todo[1].refname)) {
+ lastlog--;
+ }
+ if (!should_log(todo[0].refname)) {
+ firstlog++;
+ }
+ err = reftable_writer_add_logs(writer, &todo[firstlog],
+ lastlog - firstlog);
+
+ clear_reftable_log_record(&todo[0]);
+ clear_reftable_log_record(&todo[1]);
+ if (err < 0) {
+ goto done;
+ }
+
+ } else {
+ /* XXX what should we write into the reflog if we rename a
+ * symref? */
+ }
+
+done:
+ assert(err != REFTABLE_API_ERROR);
+ reftable_ref_record_release(&new_ref);
+ reftable_ref_record_release(&old_ref);
+ return err;
+}
+
+static int write_copy_table(struct reftable_writer *writer, void *argv)
+{
+ struct write_rename_arg *arg = (struct write_rename_arg *)argv;
+ uint64_t ts = reftable_stack_next_update_index(arg->stack);
+ struct reftable_ref_record old_ref = { NULL };
+ struct reftable_ref_record new_ref = { NULL };
+ struct reftable_log_record log = { NULL };
+ struct reftable_iterator it = { NULL };
+ int err = reftable_stack_read_ref(arg->stack, arg->oldname, &old_ref);
+ if (err) {
+ goto done;
+ }
+
+ /* git-branch supports a --force, but the check is not atomic. */
+ if (reftable_stack_read_ref(arg->stack, arg->newname, &new_ref) == 0) {
+ goto done;
+ }
+
+ reftable_writer_set_limits(writer, ts, ts);
+
+ FREE_AND_NULL(old_ref.refname);
+ old_ref.refname = xstrdup(arg->newname);
+ old_ref.update_index = ts;
+ err = reftable_writer_add_ref(writer, &old_ref);
+ if (err < 0) {
+ goto done;
+ }
+
+ /* XXX this copies the entire reflog history. Is this the right
+ * semantics? should clear out existing reflog entries for oldname? */
+ if (!should_log(arg->newname))
+ goto done;
+
+ err = reftable_merged_table_seek_log(
+ reftable_stack_merged_table(arg->stack), &it, arg->oldname);
+ if (err < 0) {
+ goto done;
+ }
+ while (1) {
+ int err = reftable_iterator_next_log(&it, &log);
+ if (err < 0) {
+ goto done;
+ }
+
+ if (err > 0 || strcmp(log.refname, arg->oldname)) {
+ break;
+ }
+ FREE_AND_NULL(log.refname);
+ log.refname = xstrdup(arg->newname);
+ reftable_writer_add_log(writer, &log);
+ reftable_log_record_release(&log);
+ }
+
+done:
+ assert(err != REFTABLE_API_ERROR);
+ reftable_ref_record_release(&new_ref);
+ reftable_ref_record_release(&old_ref);
+ reftable_log_record_release(&log);
+ reftable_iterator_destroy(&it);
+ return err;
+}
+
+static int git_reftable_rename_ref(struct ref_store *ref_store,
+ const char *oldrefname,
+ const char *newrefname, const char *logmsg)
+{
+ struct git_reftable_ref_store *refs =
+ (struct git_reftable_ref_store *)ref_store;
+ struct reftable_stack *stack = stack_for(refs, newrefname);
+ struct write_rename_arg arg = {
+ .refs = refs,
+ .stack = stack,
+ .oldname = oldrefname,
+ .newname = newrefname,
+ .logmsg = logmsg,
+ };
+ int err = refs->err;
+ if (err < 0) {
+ goto done;
+ }
+ err = reftable_stack_reload(stack);
+ if (err) {
+ goto done;
+ }
+
+ err = reftable_stack_add(stack, &write_rename_table, &arg);
+done:
+ assert(err != REFTABLE_API_ERROR);
+ return err;
+}
+
+static int git_reftable_copy_ref(struct ref_store *ref_store,
+ const char *oldrefname, const char *newrefname,
+ const char *logmsg)
+{
+ struct git_reftable_ref_store *refs =
+ (struct git_reftable_ref_store *)ref_store;
+ struct reftable_stack *stack = stack_for(refs, newrefname);
+ struct write_rename_arg arg = {
+ .refs = refs,
+ .stack = stack,
+ .oldname = oldrefname,
+ .newname = newrefname,
+ .logmsg = logmsg,
+ };
+ int err = refs->err;
+ if (err < 0) {
+ goto done;
+ }
+ err = reftable_stack_reload(stack);
+ if (err) {
+ goto done;
+ }
+
+ err = reftable_stack_add(stack, &write_copy_table, &arg);
+done:
+ assert(err != REFTABLE_API_ERROR);
+ return err;
+}
+
+struct git_reftable_reflog_ref_iterator {
+ struct ref_iterator base;
+ struct reftable_iterator iter;
+ struct reftable_log_record log;
+ struct object_id oid;
+ struct git_reftable_ref_store *refs;
+
+ /* Used when iterating over worktree & main */
+ struct reftable_merged_table *merged;
+ char *last_name;
+};
+
+static int
+git_reftable_reflog_ref_iterator_advance(struct ref_iterator *ref_iterator)
+{
+ struct git_reftable_reflog_ref_iterator *ri =
+ (struct git_reftable_reflog_ref_iterator *)ref_iterator;
+
+ while (1) {
+ int flags = 0;
+ int err = reftable_iterator_next_log(&ri->iter, &ri->log);
+
+ if (err > 0) {
+ return ITER_DONE;
+ }
+ if (err < 0) {
+ return ITER_ERROR;
+ }
+
+ ri->base.refname = ri->log.refname;
+ if (ri->last_name &&
+ !strcmp(ri->log.refname, ri->last_name)) {
+ /* we want the refnames that we have reflogs for, so we
+ * skip if we've already produced this name. This could
+ * be faster by seeking directly to
+ * reflog@update_index==0.
+ */
+ continue;
+ }
+
+ if (!refs_resolve_ref_unsafe(&ri->refs->base, ri->log.refname,
+ 0, &ri->oid, &flags)) {
+ error("bad ref for %s", ri->log.refname);
+ continue;
+ }
+
+ free(ri->last_name);
+ ri->last_name = xstrdup(ri->log.refname);
+ ri->base.oid = &ri->oid;
+ ri->base.flags = flags;
+ return ITER_OK;
+ }
+}
+
+static int
+git_reftable_reflog_ref_iterator_peel(struct ref_iterator *ref_iterator,
+ struct object_id *peeled)
+{
+ BUG("not supported.");
+ return -1;
+}
+
+static int
+git_reftable_reflog_ref_iterator_abort(struct ref_iterator *ref_iterator)
+{
+ struct git_reftable_reflog_ref_iterator *ri =
+ (struct git_reftable_reflog_ref_iterator *)ref_iterator;
+ reftable_log_record_release(&ri->log);
+ reftable_iterator_destroy(&ri->iter);
+ if (ri->merged)
+ reftable_merged_table_free(ri->merged);
+ return 0;
+}
+
+static struct ref_iterator_vtable git_reftable_reflog_ref_iterator_vtable = {
+ git_reftable_reflog_ref_iterator_advance,
+ git_reftable_reflog_ref_iterator_peel,
+ git_reftable_reflog_ref_iterator_abort
+};
+
+static struct ref_iterator *
+git_reftable_reflog_iterator_begin(struct ref_store *ref_store)
+{
+ struct git_reftable_ref_store *refs =
+ (struct git_reftable_ref_store *)ref_store;
+ struct git_reftable_reflog_ref_iterator *ri = xcalloc(1, sizeof(*ri));
+ struct reftable_stack *stack = refs->main_stack;
+ struct reftable_merged_table *mt =
+ reftable_stack_merged_table(stack);
+ int err = reftable_merged_table_seek_log(mt, &ri->iter, "");
+
+ ri->refs = refs;
+ if (err < 0) {
+ free(ri);
+ /* XXX how to handle errors in iterator_begin()? */
+ return NULL;
+ }
+ base_ref_iterator_init(&ri->base,
+ &git_reftable_reflog_ref_iterator_vtable, 1);
+ ri->base.oid = &ri->oid;
+
+ return (struct ref_iterator *)ri;
+}
+
+static int git_reftable_for_each_reflog_ent_newest_first(
+ struct ref_store *ref_store, const char *refname, each_reflog_ent_fn fn,
+ void *cb_data)
+{
+ struct reftable_iterator it = { NULL };
+ struct git_reftable_ref_store *refs =
+ (struct git_reftable_ref_store *)ref_store;
+ struct reftable_stack *stack = stack_for(refs, refname);
+ struct reftable_merged_table *mt = NULL;
+ int err = 0;
+ struct reftable_log_record log = { NULL };
+
+ if (refs->err < 0) {
+ return refs->err;
+ }
+ refname = bare_ref_name(refname);
+
+ mt = reftable_stack_merged_table(stack);
+ err = reftable_merged_table_seek_log(mt, &it, refname);
+ while (err == 0) {
+ struct object_id old_oid;
+ struct object_id new_oid;
+ const char *full_committer = "";
+
+ err = reftable_iterator_next_log(&it, &log);
+ if (err > 0) {
+ err = 0;
+ break;
+ }
+ if (err < 0) {
+ break;
+ }
+
+ if (strcmp(log.refname, refname)) {
+ break;
+ }
+
+ oidread(&old_oid, log.value.update.old_hash);
+ oidread(&new_oid, log.value.update.new_hash);
+
+ if (is_null_oid(&old_oid) && is_null_oid(&new_oid)) {
+ /* placeholder for existence. */
+ continue;
+ }
+
+ full_committer = fmt_ident(log.value.update.name,
+ log.value.update.email,
+ WANT_COMMITTER_IDENT,
+ /*date*/ NULL, IDENT_NO_DATE);
+ err = fn(&old_oid, &new_oid, full_committer,
+ log.value.update.time, log.value.update.tz_offset,
+ log.value.update.message, cb_data);
+ if (err)
+ break;
+ }
+
+ reftable_log_record_release(&log);
+ reftable_iterator_destroy(&it);
+ return err;
+}
+
+static int git_reftable_for_each_reflog_ent_oldest_first(
+ struct ref_store *ref_store, const char *refname, each_reflog_ent_fn fn,
+ void *cb_data)
+{
+ struct reftable_iterator it = { NULL };
+ struct git_reftable_ref_store *refs =
+ (struct git_reftable_ref_store *)ref_store;
+ struct reftable_stack *stack = stack_for(refs, refname);
+ struct reftable_merged_table *mt = NULL;
+ struct reftable_log_record *logs = NULL;
+ int cap = 0;
+ int len = 0;
+ int err = 0;
+ int i = 0;
+
+ if (refs->err < 0) {
+ return refs->err;
+ }
+ refname = bare_ref_name(refname);
+ mt = reftable_stack_merged_table(stack);
+ err = reftable_merged_table_seek_log(mt, &it, refname);
+
+ while (err == 0) {
+ struct reftable_log_record log = { NULL };
+ err = reftable_iterator_next_log(&it, &log);
+ if (err > 0) {
+ err = 0;
+ break;
+ }
+ if (err < 0) {
+ break;
+ }
+
+ if (strcmp(log.refname, refname)) {
+ break;
+ }
+
+ if (len == cap) {
+ cap = 2 * cap + 1;
+ logs = realloc(logs, cap * sizeof(*logs));
+ }
+
+ logs[len++] = log;
+ }
+
+ for (i = len; i--;) {
+ struct reftable_log_record *log = &logs[i];
+ struct object_id old_oid;
+ struct object_id new_oid;
+ const char *full_committer = "";
+
+ oidread(&old_oid, log->value.update.old_hash);
+ oidread(&new_oid, log->value.update.new_hash);
+
+ if (is_null_oid(&old_oid) && is_null_oid(&new_oid)) {
+ /* placeholder for existence. */
+ continue;
+ }
+
+ full_committer = fmt_ident(log->value.update.name,
+ log->value.update.email,
+ WANT_COMMITTER_IDENT, NULL,
+ IDENT_NO_DATE);
+ err = fn(&old_oid, &new_oid, full_committer,
+ log->value.update.time, log->value.update.tz_offset,
+ log->value.update.message, cb_data);
+ if (err) {
+ break;
+ }
+ }
+
+ for (i = 0; i < len; i++) {
+ reftable_log_record_release(&logs[i]);
+ }
+ free(logs);
+
+ reftable_iterator_destroy(&it);
+ return err;
+}
+
+static int git_reftable_reflog_exists(struct ref_store *ref_store,
+ const char *refname)
+{
+ struct reftable_iterator it = { NULL };
+ struct git_reftable_ref_store *refs =
+ (struct git_reftable_ref_store *)ref_store;
+ struct reftable_stack *stack = stack_for(refs, refname);
+ struct reftable_merged_table *mt = reftable_stack_merged_table(stack);
+ struct reftable_log_record log = { NULL };
+ int err = refs->err;
+
+ if (err < 0) {
+ goto done;
+ }
+
+ refname = bare_ref_name(refname);
+ err = reftable_merged_table_seek_log(mt, &it, refname);
+ if (err) {
+ goto done;
+ }
+ err = reftable_iterator_next_log(&it, &log);
+ if (err) {
+ goto done;
+ }
+
+ if (strcmp(log.refname, refname)) {
+ err = 1;
+ }
+
+done:
+ reftable_iterator_destroy(&it);
+ reftable_log_record_release(&log);
+ return !err;
+}
+
+struct write_reflog_existence_arg {
+ struct git_reftable_ref_store *refs;
+ const char *refname;
+ struct reftable_stack *stack;
+};
+
+static int write_reflog_existence_table(struct reftable_writer *writer,
+ void *argv)
+{
+ struct write_reflog_existence_arg *arg =
+ (struct write_reflog_existence_arg *)argv;
+ uint64_t ts = reftable_stack_next_update_index(arg->stack);
+ struct reftable_log_record log = { NULL };
+
+ int err = reftable_stack_read_log(arg->stack, arg->refname, &log);
+ if (err <= 0) {
+ goto done;
+ }
+
+ reftable_writer_set_limits(writer, ts, ts);
+
+ log.refname = (char *)arg->refname;
+ log.update_index = ts;
+ log.value_type = REFTABLE_LOG_UPDATE;
+ err = reftable_writer_add_log(writer, &log);
+
+ /* field is not malloced */
+ log.refname = NULL;
+
+done:
+ assert(err != REFTABLE_API_ERROR);
+ reftable_log_record_release(&log);
+ return err;
+}
+
+static int git_reftable_create_reflog(struct ref_store *ref_store,
+ const char *refname,
+ struct strbuf *errmsg)
+{
+ struct git_reftable_ref_store *refs =
+ (struct git_reftable_ref_store *)ref_store;
+ struct reftable_stack *stack = stack_for(refs, refname);
+ struct write_reflog_existence_arg arg = {
+ .refs = refs,
+ .stack = stack,
+ .refname = refname,
+ };
+ int err = refs->err;
+ if (err < 0) {
+ goto done;
+ }
+
+ err = reftable_stack_reload(stack);
+ if (err) {
+ goto done;
+ }
+
+ err = reftable_stack_add(stack, &write_reflog_existence_table, &arg);
+
+done:
+ return err;
+}
+
+struct write_reflog_delete_arg {
+ struct reftable_stack *stack;
+ const char *refname;
+};
+
+static int write_reflog_delete_table(struct reftable_writer *writer, void *argv)
+{
+ struct write_reflog_delete_arg *arg = argv;
+ struct reftable_merged_table *mt =
+ reftable_stack_merged_table(arg->stack);
+ struct reftable_log_record log = { NULL };
+ struct reftable_iterator it = { NULL };
+ uint64_t ts = reftable_stack_next_update_index(arg->stack);
+ int err = reftable_merged_table_seek_log(mt, &it, arg->refname);
+
+ reftable_writer_set_limits(writer, ts, ts);
+ while (err == 0) {
+ struct reftable_log_record tombstone = {
+ .refname = (char *)arg->refname,
+ .update_index = REFTABLE_LOG_DELETION,
+ };
+ err = reftable_iterator_next_log(&it, &log);
+ if (err > 0) {
+ err = 0;
+ break;
+ }
+
+ if (err < 0 || strcmp(log.refname, arg->refname)) {
+ break;
+ }
+ if (log.value_type == REFTABLE_LOG_DELETION)
+ continue;
+
+ tombstone.update_index = log.update_index;
+ err = reftable_writer_add_log(writer, &tombstone);
+ }
+
+ reftable_log_record_release(&log);
+ return err;
+}
+
+static int git_reftable_delete_reflog(struct ref_store *ref_store,
+ const char *refname)
+{
+ struct git_reftable_ref_store *refs =
+ (struct git_reftable_ref_store *)ref_store;
+ struct reftable_stack *stack = stack_for(refs, refname);
+ struct write_reflog_delete_arg arg = {
+ .stack = stack,
+ .refname = refname,
+ };
+ int err = reftable_stack_add(stack, &write_reflog_delete_table, &arg);
+ assert(err != REFTABLE_API_ERROR);
+ return err;
+}
+
+struct reflog_expiry_arg {
+ struct reftable_stack *stack;
+ struct reftable_log_record *records;
+ int len;
+ const char *refname;
+};
+
+static int write_reflog_expiry_table(struct reftable_writer *writer, void *argv)
+{
+ struct reflog_expiry_arg *arg = (struct reflog_expiry_arg *)argv;
+ uint64_t ts = reftable_stack_next_update_index(arg->stack);
+ int i = 0;
+ int live_records = 0;
+ uint64_t max_ts = 0;
+ for (i = 0; i < arg->len; i++) {
+ if (arg->records[i].value_type == REFTABLE_LOG_UPDATE)
+ live_records++;
+
+ if (max_ts < arg->records[i].update_index)
+ max_ts = arg->records[i].update_index;
+ }
+
+ reftable_writer_set_limits(writer, ts, ts);
+ if (live_records == 0) {
+ struct reftable_log_record log = {
+ .refname = (char *)arg->refname,
+ .update_index = max_ts + 1,
+ .value_type = REFTABLE_LOG_UPDATE,
+ /* existence dummy has null new/old oid */
+ };
+ int err;
+ if (log.update_index < ts)
+ log.update_index = ts;
+
+ err = reftable_writer_add_log(writer, &log);
+ if (err) {
+ return err;
+ }
+ }
+
+ for (i = 0; i < arg->len; i++) {
+ int err = reftable_writer_add_log(writer, &arg->records[i]);
+ if (err) {
+ return err;
+ }
+ }
+ return 0;
+}
+
+static int git_reftable_reflog_expire(
+ struct ref_store *ref_store, const char *refname, unsigned int flags,
+ reflog_expiry_prepare_fn prepare_fn,
+ reflog_expiry_should_prune_fn should_prune_fn,
+ reflog_expiry_cleanup_fn cleanup_fn, void *policy_cb_data)
+{
+ /*
+ For log expiry, we write tombstones in place of the expired entries,
+ This means that the entries are still retrievable by delving into the
+ stack, and expiring entries paradoxically takes extra memory.
+
+ This memory is only reclaimed when some operation issues a
+ git_reftable_pack_refs(), which will compact the entire stack and get
+ rid of deletion entries.
+
+ It would be better if the refs backend supported an API that sets a
+ criterion for all refs, passing the criterion to pack_refs().
+
+ On the plus side, because we do the expiration per ref, we can easily
+ insert the reflog existence dummies.
+ */
+ struct git_reftable_ref_store *refs =
+ (struct git_reftable_ref_store *)ref_store;
+ struct reftable_stack *stack = stack_for(refs, refname);
+ struct reftable_merged_table *mt = NULL;
+ struct reflog_expiry_arg arg = {
+ .stack = stack,
+ .refname = refname,
+ };
+ struct reftable_log_record *logs = NULL;
+ struct reftable_log_record *rewritten = NULL;
+ struct reftable_ref_record ref_record = { NULL };
+ int logs_len = 0;
+ int logs_cap = 0;
+ int i = 0;
+ uint8_t *last_hash = NULL;
+ struct reftable_iterator it = { NULL };
+ struct reftable_addition *add = NULL;
+ int err = 0;
+ struct object_id oid = { 0 };
+ if (refs->err < 0) {
+ return refs->err;
+ }
+ err = reftable_stack_reload(stack);
+ if (err) {
+ goto done;
+ }
+
+ mt = reftable_stack_merged_table(stack);
+ err = reftable_merged_table_seek_log(mt, &it, refname);
+ if (err < 0) {
+ goto done;
+ }
+
+ err = reftable_stack_new_addition(&add, stack);
+ if (err) {
+ goto done;
+ }
+ if (!reftable_stack_read_ref(stack, refname, &ref_record)) {
+ uint8_t *hash = reftable_ref_record_val1(&ref_record);
+ if (hash)
+ oidread(&oid, hash);
+ }
+
+ prepare_fn(refname, &oid, policy_cb_data);
+ while (1) {
+ struct reftable_log_record log = { NULL };
+ int err = reftable_iterator_next_log(&it, &log);
+ if (err < 0) {
+ goto done;
+ }
+
+ if (err > 0 || strcmp(log.refname, refname)) {
+ break;
+ }
+
+ if (logs_len >= logs_cap) {
+ int new_cap = logs_cap * 2 + 1;
+ logs = realloc(logs, new_cap * sizeof(*logs));
+ logs_cap = new_cap;
+ }
+ logs[logs_len++] = log;
+ }
+
+ rewritten = calloc(logs_len, sizeof(*rewritten));
+ for (i = logs_len - 1; i >= 0; i--) {
+ struct object_id ooid;
+ struct object_id noid;
+ struct reftable_log_record *dest = &rewritten[i];
+
+ *dest = logs[i];
+ oidread(&ooid, logs[i].value.update.old_hash);
+ oidread(&noid, logs[i].value.update.new_hash);
+
+ if (should_prune_fn(&ooid, &noid, logs[i].value.update.email,
+ (timestamp_t)logs[i].value.update.time,
+ logs[i].value.update.tz_offset,
+ logs[i].value.update.message,
+ policy_cb_data)) {
+ dest->value_type = REFTABLE_LOG_DELETION;
+ } else {
+ if ((flags & EXPIRE_REFLOGS_REWRITE) &&
+ last_hash) {
+ dest->value.update.old_hash = last_hash;
+ }
+ last_hash = logs[i].value.update.new_hash;
+ }
+ }
+
+ arg.records = rewritten;
+ arg.len = logs_len;
+ err = reftable_addition_add(add, &write_reflog_expiry_table, &arg);
+ if (err < 0) {
+ goto done;
+ }
+
+ if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
+ /* future improvement: we could skip writing records that were
+ * not changed. */
+ err = reftable_addition_commit(add);
+ }
+
+done:
+ if (add) {
+ cleanup_fn(policy_cb_data);
+ }
+ assert(err != REFTABLE_API_ERROR);
+ reftable_addition_destroy(add);
+ for (i = 0; i < logs_len; i++)
+ reftable_log_record_release(&logs[i]);
+ free(logs);
+ free(rewritten);
+ reftable_iterator_destroy(&it);
+ return err;
+}
+
+static int git_reftable_read_raw_ref(struct ref_store *ref_store,
+ const char *refname, struct object_id *oid,
+ struct strbuf *referent,
+ unsigned int *type, int *failure_errno)
+{
+ struct git_reftable_ref_store *refs =
+ (struct git_reftable_ref_store *)ref_store;
+ struct reftable_stack *stack = stack_for(refs, refname);
+ struct reftable_ref_record ref = { NULL };
+ int err = 0;
+
+ refname = bare_ref_name(refname); /* XXX - in which other cases should
+ we do this? */
+ if (refs->err < 0) {
+ return refs->err;
+ }
+
+ /* This is usually not needed, but Git doesn't signal to ref backend if
+ a subprocess updated the ref DB. So we always check.
+ */
+ err = reftable_stack_reload(stack);
+ if (err) {
+ goto done;
+ }
+
+ err = reftable_stack_read_ref(stack, refname, &ref);
+ if (err > 0) {
+ *failure_errno = ENOENT;
+ err = -1;
+ goto done;
+ }
+ if (err < 0) {
+ goto done;
+ }
+
+ if (ref.value_type == REFTABLE_REF_SYMREF) {
+ strbuf_reset(referent);
+ strbuf_addstr(referent, ref.value.symref);
+ *type |= REF_ISSYMREF;
+ } else if (reftable_ref_record_val1(&ref)) {
+ oidread(oid, reftable_ref_record_val1(&ref));
+ } else {
+ /* We got a tombstone, which should not happen. */
+ BUG("Got reftable_ref_record with value type %d",
+ ref.value_type);
+ }
+
+done:
+ assert(err != REFTABLE_API_ERROR);
+ reftable_ref_record_release(&ref);
+ return err;
+}
+
+static int git_reftable_read_symbolic_ref(struct ref_store *ref_store,
+ const char *refname,
+ struct strbuf *referent)
+{
+ struct git_reftable_ref_store *refs =
+ (struct git_reftable_ref_store *)ref_store;
+ struct reftable_stack *stack = stack_for(refs, refname);
+ struct reftable_ref_record ref = { NULL };
+ int err = 0;
+
+ err = reftable_stack_read_ref(stack, refname, &ref);
+ if (err == 0 && ref.value_type == REFTABLE_REF_SYMREF) {
+ strbuf_addstr(referent, ref.value.symref);
+ } else {
+ err = -1;
+ }
+
+ reftable_ref_record_release(&ref);
+ return err;
+}
+
+struct ref_storage_be refs_be_reftable = {
+ .next = &refs_be_files,
+ .name = "reftable",
+ .init = git_reftable_ref_store_create,
+ .init_db = git_reftable_init_db,
+ .transaction_begin = git_reftable_transaction_begin,
+ .transaction_prepare = git_reftable_transaction_prepare,
+ .transaction_finish = git_reftable_transaction_finish,
+ .transaction_abort = git_reftable_transaction_abort,
+ .initial_transaction_commit = git_reftable_transaction_initial_commit,
+
+ .pack_refs = git_reftable_pack_refs,
+ .create_symref = git_reftable_create_symref,
+ .delete_refs = git_reftable_delete_refs,
+ .rename_ref = git_reftable_rename_ref,
+ .copy_ref = git_reftable_copy_ref,
+
+ .iterator_begin = git_reftable_ref_iterator_begin,
+ .read_raw_ref = git_reftable_read_raw_ref,
+ .read_symbolic_ref = git_reftable_read_symbolic_ref,
+
+ .reflog_iterator_begin = git_reftable_reflog_iterator_begin,
+ .for_each_reflog_ent = git_reftable_for_each_reflog_ent_oldest_first,
+ .for_each_reflog_ent_reverse =
+ git_reftable_for_each_reflog_ent_newest_first,
+ .reflog_exists = git_reftable_reflog_exists,
+ .create_reflog = git_reftable_create_reflog,
+ .delete_reflog = git_reftable_delete_reflog,
+ .reflog_expire = git_reftable_reflog_expire,
+};
diff --git a/refs/reftable-backend.h b/refs/reftable-backend.h
new file mode 100644
index 00000000000..bf5fd42229a
--- /dev/null
+++ b/refs/reftable-backend.h
@@ -0,0 +1,8 @@
+#ifndef REFS_REFTABLE_BACKEND_H
+#define REFS_REFTABLE_BACKEND_H
+
+struct ref_store *git_reftable_ref_store_create(struct repository *repo,
+ const char *gitdir,
+ unsigned int store_flags);
+
+#endif /* REFS_REFTABLE_BACKEND_H */
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 6/6] refs: always try to do packed transactions for reftable
From: Han-Wen Nienhuys via GitGitGadget @ 2023-09-20 13:02 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys, Han-Wen Nienhuys, Han-Wen Nienhuys
In-Reply-To: <pull.1574.v2.git.git.1695214968.gitgitgadget@gmail.com>
From: Han-Wen Nienhuys <hanwen@google.com>
Reftable updates are cheap (compared to packed-refs updates), so do them
always. This also helps exercise this code more in tests.
This should commit should not be merged currently. It fails 55 tests
when running with GIT_TEST_REFTABLE.
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
refs/files-backend.c | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 2cd596bbeba..0d61a18b4ba 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2800,6 +2800,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,
int head_type;
struct files_transaction_backend_data *backend_data;
struct ref_transaction *packed_transaction = NULL;
+ int is_reftable = !strcmp(refs->packed_ref_store->be->name, "reftable");
assert(err);
@@ -2809,6 +2810,16 @@ static int files_transaction_prepare(struct ref_store *ref_store,
CALLOC_ARRAY(backend_data, 1);
transaction->backend_data = backend_data;
+ if (is_reftable) {
+ packed_transaction = ref_store_transaction_begin(
+ refs->packed_ref_store, err);
+ if (!packed_transaction) {
+ ret = TRANSACTION_GENERIC_ERROR;
+ goto cleanup;
+ }
+ backend_data->packed_transaction = packed_transaction;
+ }
+
/*
* Fail if a refname appears more than once in the
* transaction. (If we end up splitting up any updates using
@@ -2881,9 +2892,9 @@ static int files_transaction_prepare(struct ref_store *ref_store,
if (ret)
goto cleanup;
- if (update->flags & REF_DELETING &&
- !(update->flags & REF_LOG_ONLY) &&
- !(update->flags & REF_IS_PRUNING)) {
+ if (is_reftable || (update->flags & REF_DELETING &&
+ !(update->flags & REF_LOG_ONLY) &&
+ !(update->flags & REF_IS_PRUNING))) {
/*
* This reference has to be deleted from
* packed-refs if it exists there.
@@ -2909,8 +2920,9 @@ static int files_transaction_prepare(struct ref_store *ref_store,
}
if (packed_transaction) {
- backend_data->packed_transaction_needed = is_packed_transaction_needed(refs->packed_ref_store,
- packed_transaction);
+ backend_data->packed_transaction_needed = is_reftable ||
+ is_packed_transaction_needed(refs->packed_ref_store,
+ packed_transaction);
if (backend_data->packed_transaction_needed) {
ret = ref_transaction_prepare(packed_transaction, err);
/*
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 4/6] refs: move is_packed_transaction_needed out of packed-backend.c
From: Han-Wen Nienhuys via GitGitGadget @ 2023-09-20 13:02 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys, Han-Wen Nienhuys, Han-Wen Nienhuys
In-Reply-To: <pull.1574.v2.git.git.1695214968.gitgitgadget@gmail.com>
From: Han-Wen Nienhuys <hanwen@google.com>
It is no longer specific to the packed backend.
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
refs/files-backend.c | 94 ++++++++++++++++++++++++++++++++++++++++++
refs/packed-backend.c | 95 -------------------------------------------
refs/packed-backend.h | 9 ----
3 files changed, 94 insertions(+), 104 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 43c1882be3c..5d288bf38bb 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2630,6 +2630,100 @@ out:
return ret;
}
+/*
+ * Return true if `transaction` really needs to be carried out against
+ * the specified packed_ref_store, or false if it can be skipped
+ * (i.e., because it is an obvious NOOP). `ref_store` must be locked
+ * before calling this function.
+ */
+static int is_packed_transaction_needed(struct ref_store *ref_store,
+ struct ref_transaction *transaction)
+{
+ struct strbuf referent = STRBUF_INIT;
+ size_t i;
+ int ret;
+
+ /*
+ * We're only going to bother returning false for the common,
+ * trivial case that references are only being deleted, their
+ * old values are not being checked, and the old `packed-refs`
+ * file doesn't contain any of those reference(s). This gives
+ * false positives for some other cases that could
+ * theoretically be optimized away:
+ *
+ * 1. It could be that the old value is being verified without
+ * setting a new value. In this case, we could verify the
+ * old value here and skip the update if it agrees. If it
+ * disagrees, we could either let the update go through
+ * (the actual commit would re-detect and report the
+ * problem), or come up with a way of reporting such an
+ * error to *our* caller.
+ *
+ * 2. It could be that a new value is being set, but that it
+ * is identical to the current packed value of the
+ * reference.
+ *
+ * Neither of these cases will come up in the current code,
+ * because the only caller of this function passes to it a
+ * transaction that only includes `delete` updates with no
+ * `old_id`. Even if that ever changes, false positives only
+ * cause an optimization to be missed; they do not affect
+ * correctness.
+ */
+
+ /*
+ * Start with the cheap checks that don't require old
+ * reference values to be read:
+ */
+ for (i = 0; i < transaction->nr; i++) {
+ struct ref_update *update = transaction->updates[i];
+
+ if (update->flags & REF_HAVE_OLD)
+ /* Have to check the old value -> needed. */
+ return 1;
+
+ if ((update->flags & REF_HAVE_NEW) && !is_null_oid(&update->new_oid))
+ /* Have to set a new value -> needed. */
+ return 1;
+ }
+
+ /*
+ * The transaction isn't checking any old values nor is it
+ * setting any nonzero new values, so it still might be able
+ * to be skipped. Now do the more expensive check: the update
+ * is needed if any of the updates is a delete, and the old
+ * `packed-refs` file contains a value for that reference.
+ */
+ ret = 0;
+ for (i = 0; i < transaction->nr; i++) {
+ struct ref_update *update = transaction->updates[i];
+ int failure_errno;
+ unsigned int type;
+ struct object_id oid;
+
+ if (!(update->flags & REF_HAVE_NEW))
+ /*
+ * This reference isn't being deleted -> not
+ * needed.
+ */
+ continue;
+
+ if (!refs_read_raw_ref(ref_store, update->refname, &oid,
+ &referent, &type, &failure_errno) ||
+ failure_errno != ENOENT) {
+ /*
+ * We have to actually delete that reference
+ * -> this transaction is needed.
+ */
+ ret = 1;
+ break;
+ }
+ }
+
+ strbuf_release(&referent);
+ return ret;
+}
+
struct files_transaction_backend_data {
struct ref_transaction *packed_transaction;
int packed_transaction_needed;
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index f8b27f1afe5..2017d117c13 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1445,101 +1445,6 @@ error:
return -1;
}
-int is_packed_transaction_needed(struct ref_store *ref_store,
- struct ref_transaction *transaction)
-{
- struct packed_ref_store *refs = packed_downcast(
- ref_store,
- REF_STORE_READ,
- "is_packed_transaction_needed");
- struct strbuf referent = STRBUF_INIT;
- size_t i;
- int ret;
-
- if (!is_lock_file_locked(&refs->lock))
- BUG("is_packed_transaction_needed() called while unlocked");
-
- /*
- * We're only going to bother returning false for the common,
- * trivial case that references are only being deleted, their
- * old values are not being checked, and the old `packed-refs`
- * file doesn't contain any of those reference(s). This gives
- * false positives for some other cases that could
- * theoretically be optimized away:
- *
- * 1. It could be that the old value is being verified without
- * setting a new value. In this case, we could verify the
- * old value here and skip the update if it agrees. If it
- * disagrees, we could either let the update go through
- * (the actual commit would re-detect and report the
- * problem), or come up with a way of reporting such an
- * error to *our* caller.
- *
- * 2. It could be that a new value is being set, but that it
- * is identical to the current packed value of the
- * reference.
- *
- * Neither of these cases will come up in the current code,
- * because the only caller of this function passes to it a
- * transaction that only includes `delete` updates with no
- * `old_id`. Even if that ever changes, false positives only
- * cause an optimization to be missed; they do not affect
- * correctness.
- */
-
- /*
- * Start with the cheap checks that don't require old
- * reference values to be read:
- */
- for (i = 0; i < transaction->nr; i++) {
- struct ref_update *update = transaction->updates[i];
-
- if (update->flags & REF_HAVE_OLD)
- /* Have to check the old value -> needed. */
- return 1;
-
- if ((update->flags & REF_HAVE_NEW) && !is_null_oid(&update->new_oid))
- /* Have to set a new value -> needed. */
- return 1;
- }
-
- /*
- * The transaction isn't checking any old values nor is it
- * setting any nonzero new values, so it still might be able
- * to be skipped. Now do the more expensive check: the update
- * is needed if any of the updates is a delete, and the old
- * `packed-refs` file contains a value for that reference.
- */
- ret = 0;
- for (i = 0; i < transaction->nr; i++) {
- struct ref_update *update = transaction->updates[i];
- int failure_errno;
- unsigned int type;
- struct object_id oid;
-
- if (!(update->flags & REF_HAVE_NEW))
- /*
- * This reference isn't being deleted -> not
- * needed.
- */
- continue;
-
- if (!refs_read_raw_ref(ref_store, update->refname, &oid,
- &referent, &type, &failure_errno) ||
- failure_errno != ENOENT) {
- /*
- * We have to actually delete that reference
- * -> this transaction is needed.
- */
- ret = 1;
- break;
- }
- }
-
- strbuf_release(&referent);
- return ret;
-}
-
struct packed_transaction_backend_data {
/* True iff the transaction owns the packed-refs lock. */
struct string_list updates;
diff --git a/refs/packed-backend.h b/refs/packed-backend.h
index ade3c8a5ac4..51a3b6a332a 100644
--- a/refs/packed-backend.h
+++ b/refs/packed-backend.h
@@ -17,13 +17,4 @@ struct ref_store *packed_ref_store_create(struct repository *repo,
const char *gitdir,
unsigned int store_flags);
-/*
- * Return true if `transaction` really needs to be carried out against
- * the specified packed_ref_store, or false if it can be skipped
- * (i.e., because it is an obvious NOOP). `ref_store` must be locked
- * before calling this function.
- */
-int is_packed_transaction_needed(struct ref_store *ref_store,
- struct ref_transaction *transaction);
-
#endif /* REFS_PACKED_BACKEND_H */
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 3/6] refs: push lock management into packed backend
From: Han-Wen Nienhuys via GitGitGadget @ 2023-09-20 13:02 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys, Han-Wen Nienhuys, Han-Wen Nienhuys
In-Reply-To: <pull.1574.v2.git.git.1695214968.gitgitgadget@gmail.com>
From: Han-Wen Nienhuys <hanwen@google.com>
Take packed-refs.lock in the transaction_begin of the packed backend.
This decouples the files backend from the packed backend, allowing the
latter to be swapped out by another ref backend.
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
refs.c | 5 ++-
refs/files-backend.c | 78 +++++++++++++++----------------------------
refs/packed-backend.c | 55 ++++++++++++------------------
refs/packed-backend.h | 10 ------
4 files changed, 50 insertions(+), 98 deletions(-)
diff --git a/refs.c b/refs.c
index f7ba3e56284..0611197dc6e 100644
--- a/refs.c
+++ b/refs.c
@@ -2320,10 +2320,9 @@ int ref_transaction_abort(struct ref_transaction *transaction,
int ret = 0;
switch (transaction->state) {
- case REF_TRANSACTION_OPEN:
- /* No need to abort explicitly. */
- break;
case REF_TRANSACTION_PREPARED:
+ case REF_TRANSACTION_OPEN:
+ /* an OPEN transactions may have a lock. */
ret = refs->be->transaction_abort(refs, transaction, err);
break;
case REF_TRANSACTION_CLOSED:
diff --git a/refs/files-backend.c b/refs/files-backend.c
index d83eea8b408..43c1882be3c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1219,10 +1219,10 @@ static int files_pack_refs(struct ref_store *ref_store,
struct ref_transaction *transaction;
transaction = ref_store_transaction_begin(refs->packed_ref_store, &err);
- if (!transaction)
+ if (!transaction) {
+ die("could not start transaction: %s", err.buf);
return -1;
-
- packed_refs_lock(refs->packed_ref_store, LOCK_DIE_ON_ERROR, &err);
+ }
iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL,
the_repository, 0);
@@ -1262,8 +1262,6 @@ static int files_pack_refs(struct ref_store *ref_store,
ref_transaction_free(transaction);
- packed_refs_unlock(refs->packed_ref_store);
-
prune_refs(refs, &refs_to_prune);
strbuf_release(&err);
return 0;
@@ -1280,16 +1278,10 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,
if (!refnames->nr)
return 0;
- if (packed_refs_lock(refs->packed_ref_store, 0, &err))
- goto error;
-
if (refs_delete_refs(refs->packed_ref_store, msg, refnames, flags)) {
- packed_refs_unlock(refs->packed_ref_store);
goto error;
}
- packed_refs_unlock(refs->packed_ref_store);
-
for (i = 0; i < refnames->nr; i++) {
const char *refname = refnames->items[i].string;
@@ -2640,7 +2632,7 @@ out:
struct files_transaction_backend_data {
struct ref_transaction *packed_transaction;
- int packed_refs_locked;
+ int packed_transaction_needed;
};
/*
@@ -2672,10 +2664,8 @@ static void files_transaction_cleanup(struct files_ref_store *refs,
strbuf_release(&err);
}
- if (backend_data->packed_refs_locked)
- packed_refs_unlock(refs->packed_ref_store);
-
free(backend_data);
+ transaction->backend_data = NULL;
}
transaction->state = REF_TRANSACTION_CLOSED;
@@ -2811,14 +2801,9 @@ static int files_transaction_prepare(struct ref_store *ref_store,
}
if (packed_transaction) {
- if (packed_refs_lock(refs->packed_ref_store, 0, err)) {
- ret = TRANSACTION_GENERIC_ERROR;
- goto cleanup;
- }
- backend_data->packed_refs_locked = 1;
-
- if (is_packed_transaction_needed(refs->packed_ref_store,
- packed_transaction)) {
+ backend_data->packed_transaction_needed = is_packed_transaction_needed(refs->packed_ref_store,
+ packed_transaction);
+ if (backend_data->packed_transaction_needed) {
ret = ref_transaction_prepare(packed_transaction, err);
/*
* A failure during the prepare step will abort
@@ -2830,22 +2815,6 @@ static int files_transaction_prepare(struct ref_store *ref_store,
ref_transaction_free(packed_transaction);
backend_data->packed_transaction = NULL;
}
- } else {
- /*
- * We can skip rewriting the `packed-refs`
- * file. But we do need to leave it locked, so
- * that somebody else doesn't pack a reference
- * that we are trying to delete.
- *
- * We need to disconnect our transaction from
- * backend_data, since the abort (whether successful or
- * not) will free it.
- */
- backend_data->packed_transaction = NULL;
- if (ref_transaction_abort(packed_transaction, err)) {
- ret = TRANSACTION_GENERIC_ERROR;
- goto cleanup;
- }
}
}
@@ -2947,13 +2916,24 @@ static int files_transaction_finish(struct ref_store *ref_store,
* First delete any packed versions of the references, while
* retaining the packed-refs lock:
*/
- if (packed_transaction) {
- ret = ref_transaction_commit(packed_transaction, err);
- ref_transaction_free(packed_transaction);
- packed_transaction = NULL;
- backend_data->packed_transaction = NULL;
- if (ret)
- goto cleanup;
+ if (backend_data->packed_transaction) {
+ if (backend_data->packed_transaction_needed) {
+ ret = ref_transaction_commit(packed_transaction, err);
+ if (ret)
+ goto cleanup;
+
+ ref_transaction_free(packed_transaction);
+ packed_transaction = NULL;
+ backend_data->packed_transaction = NULL;
+ } else {
+ ret = ref_transaction_abort(packed_transaction, err);
+ if (ret)
+ goto cleanup;
+
+ /* transaction_commit doesn't free the data, but transaction_abort does. Go figure. */
+ packed_transaction = NULL;
+ backend_data->packed_transaction = NULL;
+ }
}
/* Now delete the loose versions of the references: */
@@ -3094,16 +3074,10 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,
NULL);
}
- if (packed_refs_lock(refs->packed_ref_store, 0, err)) {
- ret = TRANSACTION_GENERIC_ERROR;
- goto cleanup;
- }
-
if (initial_ref_transaction_commit(packed_transaction, err)) {
ret = TRANSACTION_GENERIC_ERROR;
}
- packed_refs_unlock(refs->packed_ref_store);
cleanup:
if (packed_transaction)
ref_transaction_free(packed_transaction);
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 4f5b1025605..f8b27f1afe5 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1153,7 +1153,7 @@ static int write_packed_entry(FILE *fh, const char *refname,
return 0;
}
-int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)
+static int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)
{
struct packed_ref_store *refs =
packed_downcast(ref_store, REF_STORE_WRITE | REF_STORE_MAIN,
@@ -1212,7 +1212,7 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)
return 0;
}
-void packed_refs_unlock(struct ref_store *ref_store)
+static void packed_refs_unlock(struct ref_store *ref_store)
{
struct packed_ref_store *refs = packed_downcast(
ref_store,
@@ -1224,16 +1224,6 @@ void packed_refs_unlock(struct ref_store *ref_store)
rollback_lock_file(&refs->lock);
}
-int packed_refs_is_locked(struct ref_store *ref_store)
-{
- struct packed_ref_store *refs = packed_downcast(
- ref_store,
- REF_STORE_READ | REF_STORE_WRITE,
- "packed_refs_is_locked");
-
- return is_lock_file_locked(&refs->lock);
-}
-
/*
* The packed-refs header line that we write out. Perhaps other traits
* will be added later.
@@ -1552,8 +1542,6 @@ int is_packed_transaction_needed(struct ref_store *ref_store,
struct packed_transaction_backend_data {
/* True iff the transaction owns the packed-refs lock. */
- int own_lock;
-
struct string_list updates;
};
@@ -1568,9 +1556,8 @@ static void packed_transaction_cleanup(struct packed_ref_store *refs,
if (is_tempfile_active(refs->tempfile))
delete_tempfile(&refs->tempfile);
- if (data->own_lock && is_lock_file_locked(&refs->lock)) {
+ if (is_lock_file_locked(&refs->lock)) {
packed_refs_unlock(&refs->base);
- data->own_lock = 0;
}
free(data);
@@ -1580,12 +1567,25 @@ static void packed_transaction_cleanup(struct packed_ref_store *refs,
transaction->state = REF_TRANSACTION_CLOSED;
}
-
static struct ref_transaction *packed_transaction_begin(struct ref_store *ref_store,
- struct strbuf *err) {
- struct ref_transaction *tr;
- CALLOC_ARRAY(tr, 1);
- return tr;
+ struct strbuf *err)
+{
+ struct packed_ref_store *refs = packed_downcast(
+ ref_store,
+ REF_STORE_READ | REF_STORE_WRITE | REF_STORE_ODB,
+ "ref_transaction_begin");
+ struct packed_transaction_backend_data *data;
+ struct ref_transaction *transaction;
+
+ if (!is_lock_file_locked(&refs->lock)) {
+ if (packed_refs_lock(ref_store, 0, err))
+ return NULL;
+ }
+ CALLOC_ARRAY(transaction, 1);
+ CALLOC_ARRAY(data, 1);
+ string_list_init_nodup(&data->updates);
+ transaction->backend_data = data;
+ return transaction;
}
static int packed_transaction_prepare(struct ref_store *ref_store,
@@ -1596,7 +1596,7 @@ static int packed_transaction_prepare(struct ref_store *ref_store,
ref_store,
REF_STORE_READ | REF_STORE_WRITE | REF_STORE_ODB,
"ref_transaction_prepare");
- struct packed_transaction_backend_data *data;
+ struct packed_transaction_backend_data *data = transaction->backend_data;
size_t i;
int ret = TRANSACTION_GENERIC_ERROR;
@@ -1609,11 +1609,6 @@ static int packed_transaction_prepare(struct ref_store *ref_store,
* do so itself.
*/
- CALLOC_ARRAY(data, 1);
- string_list_init_nodup(&data->updates);
-
- transaction->backend_data = data;
-
/*
* Stick the updates in a string list by refname so that we
* can sort them:
@@ -1631,12 +1626,6 @@ static int packed_transaction_prepare(struct ref_store *ref_store,
if (ref_update_reject_duplicates(&data->updates, err))
goto failure;
- if (!is_lock_file_locked(&refs->lock)) {
- if (packed_refs_lock(ref_store, 0, err))
- goto failure;
- data->own_lock = 1;
- }
-
if (write_with_updates(refs, &data->updates, err))
goto failure;
diff --git a/refs/packed-backend.h b/refs/packed-backend.h
index 9dd8a344c34..ade3c8a5ac4 100644
--- a/refs/packed-backend.h
+++ b/refs/packed-backend.h
@@ -17,16 +17,6 @@ struct ref_store *packed_ref_store_create(struct repository *repo,
const char *gitdir,
unsigned int store_flags);
-/*
- * Lock the packed-refs file for writing. Flags is passed to
- * hold_lock_file_for_update(). Return 0 on success. On errors, write
- * an error message to `err` and return a nonzero value.
- */
-int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err);
-
-void packed_refs_unlock(struct ref_store *ref_store);
-int packed_refs_is_locked(struct ref_store *ref_store);
-
/*
* Return true if `transaction` really needs to be carried out against
* the specified packed_ref_store, or false if it can be skipped
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 0/6] RFC: simple reftable backend
From: Han-Wen Nienhuys via GitGitGadget @ 2023-09-20 13:02 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys, Han-Wen Nienhuys
In-Reply-To: <pull.1574.git.git.1695059978.gitgitgadget@gmail.com>
This series comes from a conversation with Patrick Steinhardt at Gitlab, who
have an interest in a more scalable ref storage system.
I unfortunately don't have business reasons to spend much time on this
project anymore, and the original, sweeping migration has too many open
questions.
I thought of an alternate approach, and I wanted to show it as input to
discussions at the contributor summit.
I think the first part (everything before "refs: alternate reftable ref
backend implementation") is a good improvement overall, and could be landed
separately without much tweaking.
The second part ("refs: alternate reftable ref backend implementation" and
follow-on) is something open for discussion: the alternative "packed
storage" is based on reftable, but it could conceivably be a different type
of database/file format too. I think it's attractive to use reftable here,
because over time more data (symrefs, reflog) could be moved into reftable.
I'm offering this series as a RFC. I hope that someone else (Patrick?) can
look into getting the bits and pieces of this merged.
Han-Wen Nienhuys (6):
refs: construct transaction using a _begin callback
refs: wrap transaction in a debug-specific transaction
refs: push lock management into packed backend
refs: move is_packed_transaction_needed out of packed-backend.c
refs: alternate reftable ref backend implementation
refs: always try to do packed transactions for reftable
Makefile | 1 +
config.mak.uname | 2 +-
contrib/workdir/git-new-workdir | 2 +-
refs.c | 13 +-
refs/debug.c | 87 +-
refs/files-backend.c | 216 ++--
refs/packed-backend.c | 149 +--
refs/packed-backend.h | 19 -
refs/refs-internal.h | 5 +
refs/reftable-backend.c | 1658 +++++++++++++++++++++++++++++++
refs/reftable-backend.h | 8 +
11 files changed, 1929 insertions(+), 231 deletions(-)
create mode 100644 refs/reftable-backend.c
create mode 100644 refs/reftable-backend.h
base-commit: bda494f4043963b9ec9a1ecd4b19b7d1cd9a0518
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1574%2Fhanwen%2Fsimple-reftable-backend-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1574/hanwen/simple-reftable-backend-v2
Pull-Request: https://github.com/git/git/pull/1574
Range-diff vs v1:
-: ----------- > 1: e99f3d20056 refs: construct transaction using a _begin callback
-: ----------- > 2: 9a459259330 refs: wrap transaction in a debug-specific transaction
1: dea0fbb139a ! 3: 8dedb23eb69 refs: push lock management into packed backend
@@ Metadata
## Commit message ##
refs: push lock management into packed backend
- Introduces a ref backend method transaction_begin, which for the
- packed backend takes packed-refs.lock.
+ Take packed-refs.lock in the transaction_begin of the packed backend.
This decouples the files backend from the packed backend, allowing the
latter to be swapped out by another ref backend.
@@ Commit message
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
## refs.c ##
-@@ refs.c: struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,
- struct strbuf *err)
- {
- struct ref_transaction *tr;
-+ int ret = 0;
- assert(err);
-
- CALLOC_ARRAY(tr, 1);
- tr->ref_store = refs;
-+
-+ if (refs->be->transaction_begin)
-+ ret = refs->be->transaction_begin(refs, tr, err);
-+ if (ret) {
-+ free(tr);
-+ return NULL;
-+ }
- return tr;
- }
-
-
- ## refs/debug.c ##
-@@ refs/debug.c: static int debug_init_db(struct ref_store *refs, struct strbuf *err)
- return res;
- }
-
-+static int debug_transaction_begin(struct ref_store *refs,
-+ struct ref_transaction *transaction,
-+ struct strbuf *err)
-+{
-+ struct debug_ref_store *drefs = (struct debug_ref_store *)refs;
-+ int res;
-+ transaction->ref_store = drefs->refs;
-+ res = drefs->refs->be->transaction_begin(drefs->refs, transaction,
-+ err);
-+ trace_printf_key(&trace_refs, "transaction_begin: %d \"%s\"\n", res,
-+ err->buf);
-+ return res;
-+}
-+
- static int debug_transaction_prepare(struct ref_store *refs,
- struct ref_transaction *transaction,
- struct strbuf *err)
-@@ refs/debug.c: struct ref_storage_be refs_be_debug = {
- * has a function we should also have a wrapper for it here.
- * Test the output with "GIT_TRACE_REFS=1".
- */
-+ .transaction_begin = debug_transaction_begin,
- .transaction_prepare = debug_transaction_prepare,
- .transaction_finish = debug_transaction_finish,
- .transaction_abort = debug_transaction_abort,
+@@ refs.c: int ref_transaction_abort(struct ref_transaction *transaction,
+ int ret = 0;
+
+ switch (transaction->state) {
+- case REF_TRANSACTION_OPEN:
+- /* No need to abort explicitly. */
+- break;
+ case REF_TRANSACTION_PREPARED:
++ case REF_TRANSACTION_OPEN:
++ /* an OPEN transactions may have a lock. */
+ ret = refs->be->transaction_abort(refs, transaction, err);
+ break;
+ case REF_TRANSACTION_CLOSED:
## refs/files-backend.c ##
@@ refs/files-backend.c: static int files_pack_refs(struct ref_store *ref_store,
@@ refs/files-backend.c: static int files_transaction_finish(struct ref_store *ref_
- backend_data->packed_transaction = NULL;
- if (ret)
- goto cleanup;
-+ if (backend_data->packed_transaction) {
++ if (backend_data->packed_transaction) {
+ if (backend_data->packed_transaction_needed) {
+ ret = ref_transaction_commit(packed_transaction, err);
+ if (ret)
+ goto cleanup;
-+ /* TODO: leaks on error path. */
++
+ ref_transaction_free(packed_transaction);
+ packed_transaction = NULL;
+ backend_data->packed_transaction = NULL;
@@ refs/files-backend.c: static int files_initial_transaction_commit(struct ref_sto
ref_transaction_free(packed_transaction);
## refs/packed-backend.c ##
+@@ refs/packed-backend.c: static int write_packed_entry(FILE *fh, const char *refname,
+ return 0;
+ }
+
+-int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)
++static int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)
+ {
+ struct packed_ref_store *refs =
+ packed_downcast(ref_store, REF_STORE_WRITE | REF_STORE_MAIN,
+@@ refs/packed-backend.c: int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)
+ return 0;
+ }
+
+-void packed_refs_unlock(struct ref_store *ref_store)
++static void packed_refs_unlock(struct ref_store *ref_store)
+ {
+ struct packed_ref_store *refs = packed_downcast(
+ ref_store,
+@@ refs/packed-backend.c: void packed_refs_unlock(struct ref_store *ref_store)
+ rollback_lock_file(&refs->lock);
+ }
+
+-int packed_refs_is_locked(struct ref_store *ref_store)
+-{
+- struct packed_ref_store *refs = packed_downcast(
+- ref_store,
+- REF_STORE_READ | REF_STORE_WRITE,
+- "packed_refs_is_locked");
+-
+- return is_lock_file_locked(&refs->lock);
+-}
+-
+ /*
+ * The packed-refs header line that we write out. Perhaps other traits
+ * will be added later.
@@ refs/packed-backend.c: int is_packed_transaction_needed(struct ref_store *ref_store,
struct packed_transaction_backend_data {
@@ refs/packed-backend.c: static void packed_transaction_cleanup(struct packed_ref_
transaction->state = REF_TRANSACTION_CLOSED;
}
-+static int packed_transaction_begin(struct ref_store *ref_store,
-+ struct ref_transaction *transaction,
+-
+ static struct ref_transaction *packed_transaction_begin(struct ref_store *ref_store,
+- struct strbuf *err) {
+- struct ref_transaction *tr;
+- CALLOC_ARRAY(tr, 1);
+- return tr;
+ struct strbuf *err)
+{
+ struct packed_ref_store *refs = packed_downcast(
@@ refs/packed-backend.c: static void packed_transaction_cleanup(struct packed_ref_
+ REF_STORE_READ | REF_STORE_WRITE | REF_STORE_ODB,
+ "ref_transaction_begin");
+ struct packed_transaction_backend_data *data;
-+
-+ CALLOC_ARRAY(data, 1);
-+ string_list_init_nodup(&data->updates);
++ struct ref_transaction *transaction;
+
+ if (!is_lock_file_locked(&refs->lock)) {
+ if (packed_refs_lock(ref_store, 0, err))
-+ /* todo: leaking data */
-+ return -1;
++ return NULL;
+ }
++ CALLOC_ARRAY(transaction, 1);
++ CALLOC_ARRAY(data, 1);
++ string_list_init_nodup(&data->updates);
+ transaction->backend_data = data;
-+ return 0;
-+}
-+
++ return transaction;
+ }
+
static int packed_transaction_prepare(struct ref_store *ref_store,
- struct ref_transaction *transaction,
- struct strbuf *err)
@@ refs/packed-backend.c: static int packed_transaction_prepare(struct ref_store *ref_store,
ref_store,
REF_STORE_READ | REF_STORE_WRITE | REF_STORE_ODB,
@@ refs/packed-backend.c: static int packed_transaction_prepare(struct ref_store *r
if (write_with_updates(refs, &data->updates, err))
goto failure;
-@@ refs/packed-backend.c: struct ref_storage_be refs_be_packed = {
- .name = "packed",
- .init = packed_ref_store_create,
- .init_db = packed_init_db,
-+ .transaction_begin = packed_transaction_begin,
- .transaction_prepare = packed_transaction_prepare,
- .transaction_finish = packed_transaction_finish,
- .transaction_abort = packed_transaction_abort,
## refs/packed-backend.h ##
@@ refs/packed-backend.h: struct ref_store *packed_ref_store_create(struct repository *repo,
@@ refs/packed-backend.h: struct ref_store *packed_ref_store_create(struct reposito
/*
* Return true if `transaction` really needs to be carried out against
* the specified packed_ref_store, or false if it can be skipped
-
- ## refs/refs-internal.h ##
-@@ refs/refs-internal.h: typedef struct ref_store *ref_store_init_fn(struct repository *repo,
-
- typedef int ref_init_db_fn(struct ref_store *refs, struct strbuf *err);
-
-+typedef int ref_transaction_begin_fn(struct ref_store *refs,
-+ struct ref_transaction *transaction,
-+ struct strbuf *err);
-+
- typedef int ref_transaction_prepare_fn(struct ref_store *refs,
- struct ref_transaction *transaction,
- struct strbuf *err);
-@@ refs/refs-internal.h: struct ref_storage_be {
- ref_store_init_fn *init;
- ref_init_db_fn *init_db;
-
-+ ref_transaction_begin_fn *transaction_begin;
- ref_transaction_prepare_fn *transaction_prepare;
- ref_transaction_finish_fn *transaction_finish;
- ref_transaction_abort_fn *transaction_abort;
2: a5cb3adc662 ! 4: 0b8919b05c4 refs: move is_packed_transaction_needed out of packed-backend.c
@@ refs/files-backend.c: out:
return ret;
}
-+
+/*
+ * Return true if `transaction` really needs to be carried out against
+ * the specified packed_ref_store, or false if it can be skipped
@@ refs/files-backend.c: out:
struct files_transaction_backend_data {
struct ref_transaction *packed_transaction;
int packed_transaction_needed;
-@@ refs/files-backend.c: struct ref_storage_be refs_be_files = {
- .delete_reflog = files_delete_reflog,
- .reflog_expire = files_reflog_expire
- };
-+
## refs/packed-backend.c ##
@@ refs/packed-backend.c: error:
3: 559c04cee99 ! 5: d5669da57e3 refs: alternate reftable ref backend implementation
@@ Commit message
refs: alternate reftable ref backend implementation
This introduces the reftable backend as an alternative to the packed
- backend. This simplifies the code, because we now longer have to worry
- about:
+ backend.
+
+ This is an alternative to the approach which was explored in
+ https://github.com/git/git/pull/1215/. This alternative is simpler,
+ because we now longer have to worry about:
- pseudorefs and the HEAD ref
- worktrees
- commands that blur together files and references (cherry-pick, rebase)
This deviates from the spec that in
- Documentation/technical/reftable.txt. It might be possible to update
- the code such that all writes by default go to reftable directly. Then
- the result would be compatible with an implementation that writes only
- reftable (the reftable lock would still prevent races) relative to an
- implementation that disregards loose refs. Or, JGit could be adapted
- to follow this implementation.
+ Documentation/technical/reftable.txt. It might be possible to update the
+ code such that all writes by default go to reftable directly. Then the
+ result would be compatible with an implementation that writes only
+ reftable (the reftable lock would still prevent races), but something
+ must be done about the HEAD ref (which JGit keeps in reftable too.)
+ Alternatively, JGit could be adapted to follow this implementation:
+ despite the code being there for 4 years now, I haven't heard of anyone
+ using it in production (exactly because CGit does not support it).
For this incremental path, the reftable format is arguably more
complex than necessary, as
- packed-refs doesn't support symrefs
- - reflogs aren't moved into reftable/
+ - reflogs aren't moved into reftable
on the other hand, the code is already there, and it's well-structured
and well-tested.
- This implementation is a prototype. To test, you need to do `export
- GIT_TEST_REFTABLE=1`. Doing so creates a handful of errors in the
- test-suite, most seemingly related to the new behavior of pack-refs
- (which creates a reftable/ dir and not a packed-refs file.), but it
- seems overseeable.
+ refs/reftable-backend.c was created by cannibalizing the first version
+ of reftable support (github PR 1215 as mentioned above). It supports
+ reflogs and symrefs, even though those features are never exercised.
+
+ This implementation is a prototype, for the following reasons:
+
+ - no considerations of backward compatibility and configuring an
+ extension
+ - no support for converting between packed-refs and reftable
+ - no documentation
+ - test failures when setting GIT_TEST_REFTABLE=1.
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
@@ contrib/workdir/git-new-workdir: trap cleanup $siglist
case $x in
## refs/files-backend.c ##
+@@
+ #include "../git-compat-util.h"
++#include "../abspath.h"
+ #include "../config.h"
+ #include "../copy.h"
+ #include "../environment.h"
@@
#include "refs-internal.h"
#include "ref-cache.h"
@@ refs/files-backend.c
#include "../iterator.h"
#include "../dir-iterator.h"
@@ refs/files-backend.c: static struct ref_store *files_ref_store_create(struct repository *repo,
+ struct files_ref_store *refs = xcalloc(1, sizeof(*refs));
+ struct ref_store *ref_store = (struct ref_store *)refs;
+ struct strbuf sb = STRBUF_INIT;
++ int has_reftable, has_packed;
+
+ base_ref_store_init(ref_store, repo, gitdir, &refs_be_files);
refs->store_flags = flags;
get_common_dir_noenv(&sb, gitdir);
refs->gitcommondir = strbuf_detach(&sb, NULL);
-+
-+ /* TODO: should look at the repo to decide whether to use packed-refs or
-+ * reftable. */
- refs->packed_ref_store =
+- refs->packed_ref_store =
- packed_ref_store_create(repo, refs->gitcommondir, flags);
-+ git_env_bool("GIT_TEST_REFTABLE", 0)
++
++ strbuf_addf(&sb, "%s/reftable", refs->gitcommondir);
++ has_reftable = is_directory(sb.buf);
++ strbuf_reset(&sb);
++ strbuf_addf(&sb, "%s/packed-refs", refs->gitcommondir);
++ has_packed = file_exists(sb.buf);
++
++ if (!has_packed && !has_reftable)
++ has_reftable = git_env_bool("GIT_TEST_REFTABLE", 0);
++
++ refs->packed_ref_store = has_reftable
+ ? git_reftable_ref_store_create(repo, refs->gitcommondir, flags)
+ : packed_ref_store_create(repo, refs->gitcommondir, flags);
chdir_notify_reparent("files-backend $GIT_DIR", &refs->base.gitdir);
chdir_notify_reparent("files-backend $GIT_COMMONDIR",
- ## refs/packed-backend.c ##
-@@ refs/packed-backend.c: static int write_packed_entry(FILE *fh, const char *refname,
- return 0;
- }
-
--int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)
-+static int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)
- {
- struct packed_ref_store *refs =
- packed_downcast(ref_store, REF_STORE_WRITE | REF_STORE_MAIN,
-@@ refs/packed-backend.c: int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)
- return 0;
- }
-
--void packed_refs_unlock(struct ref_store *ref_store)
-+static void packed_refs_unlock(struct ref_store *ref_store)
- {
- struct packed_ref_store *refs = packed_downcast(
- ref_store,
-@@ refs/packed-backend.c: void packed_refs_unlock(struct ref_store *ref_store)
- rollback_lock_file(&refs->lock);
- }
-
--int packed_refs_is_locked(struct ref_store *ref_store)
--{
-- struct packed_ref_store *refs = packed_downcast(
-- ref_store,
-- REF_STORE_READ | REF_STORE_WRITE,
-- "packed_refs_is_locked");
--
-- return is_lock_file_locked(&refs->lock);
--}
--
- /*
- * The packed-refs header line that we write out. Perhaps other traits
- * will be added later.
-
## refs/refs-internal.h ##
@@ refs/refs-internal.h: struct ref_storage_be {
};
@@ refs/reftable-backend.c (new)
+#include "../worktree.h"
+#include "refs-internal.h"
+#include "reftable-backend.h"
++#include "../repository.h"
+
+extern struct ref_storage_be refs_be_reftable;
+
@@ refs/reftable-backend.c (new)
+ return &ri->base;
+}
+
-+static int fixup_symrefs(struct ref_store *ref_store,
-+ struct ref_transaction *transaction)
-+{
-+ struct strbuf referent = STRBUF_INIT;
-+ int i = 0;
-+ int err = 0;
-+
-+ for (i = 0; i < transaction->nr; i++) {
-+ struct ref_update *update = transaction->updates[i];
-+ struct object_id old_oid;
-+ int failure_errno;
-+
-+ err = git_reftable_read_raw_ref(ref_store, update->refname,
-+ &old_oid, &referent,
-+ /* mutate input, like
-+ files-backend.c */
-+ &update->type, &failure_errno);
-+ if (err < 0 && failure_errno == ENOENT &&
-+ is_null_oid(&update->old_oid)) {
-+ err = 0;
-+ }
-+ if (err < 0)
-+ goto done;
-+
-+ if (!(update->type & REF_ISSYMREF))
-+ continue;
-+
-+ if (update->flags & REF_NO_DEREF) {
-+ /* what should happen here? See files-backend.c
-+ * lock_ref_for_update. */
-+ } else {
-+ /*
-+ If we are updating a symref (eg. HEAD), we should also
-+ update the branch that the symref points to.
-+
-+ This is generic functionality, and would be better
-+ done in refs.c, but the current implementation is
-+ intertwined with the locking in files-backend.c.
-+ */
-+ int new_flags = update->flags;
-+ struct ref_update *new_update = NULL;
-+
-+ /* if this is an update for HEAD, should also record a
-+ log entry for HEAD? See files-backend.c,
-+ split_head_update()
-+ */
-+ new_update = ref_transaction_add_update(
-+ transaction, referent.buf, new_flags,
-+ &update->new_oid, &update->old_oid,
-+ update->msg);
-+ new_update->parent_update = update;
-+
-+ /* files-backend sets REF_LOG_ONLY here. */
-+ update->flags |= REF_NO_DEREF | REF_LOG_ONLY;
-+ update->flags &= ~REF_HAVE_OLD;
-+ }
-+ }
-+
-+done:
-+ assert(err != REFTABLE_API_ERROR);
-+ strbuf_release(&referent);
-+ return err;
-+}
-+
-+static int git_reftable_transaction_prepare(struct ref_store *ref_store,
-+ struct ref_transaction *transaction,
-+ struct strbuf *errbuf)
++static struct ref_transaction *git_reftable_transaction_begin(struct ref_store *ref_store,
++ struct strbuf *errbuf)
+{
+ struct git_reftable_ref_store *refs =
+ (struct git_reftable_ref_store *)ref_store;
+ struct reftable_addition *add = NULL;
-+ struct reftable_stack *stack = stack_for(
-+ refs,
-+ transaction->nr ? transaction->updates[0]->refname : NULL);
-+ int i;
++ struct ref_transaction *transaction = NULL;
++ struct reftable_stack *stack = refs->main_stack;
+
+ int err = refs->err;
+ if (err < 0) {
@@ refs/reftable-backend.c (new)
+
+ err = reftable_stack_new_addition(&add, stack);
+ if (err) {
++ strbuf_addf(errbuf, "reftable: transaction begin: %s",
++ reftable_error_str(err));
+ goto done;
+ }
+
-+ for (i = 0; i < transaction->nr; i++) {
-+ struct ref_update *u = transaction->updates[i];
-+ if ((u->flags & REF_HAVE_NEW) && !is_null_oid(&u->new_oid) &&
-+ !(u->flags & REF_SKIP_OID_VERIFICATION) &&
-+ !(u->flags & REF_LOG_ONLY)) {
-+ struct object *o =
-+ parse_object(refs->base.repo, &u->new_oid);
-+ if (!o) {
-+ strbuf_addf(
-+ errbuf,
-+ "trying to write ref '%s' with nonexistent object %s",
-+ u->refname, oid_to_hex(&u->new_oid));
-+ err = -1;
-+ goto done;
-+ }
-+ }
-+ }
-+
-+ err = fixup_symrefs(ref_store, transaction);
-+ if (err) {
-+ goto done;
-+ }
-+
++ CALLOC_ARRAY(transaction, 1);
+ transaction->backend_data = add;
-+ transaction->state = REF_TRANSACTION_PREPARED;
-+
+done:
-+ assert(err != REFTABLE_API_ERROR);
-+ if (err < 0) {
-+ if (add) {
-+ reftable_addition_destroy(add);
-+ add = NULL;
-+ }
-+ transaction->state = REF_TRANSACTION_CLOSED;
-+ if (!errbuf->len)
-+ strbuf_addf(errbuf, "reftable: transaction prepare: %s",
-+ reftable_error_str(err));
-+ }
++ return transaction;
++}
+
-+ return err;
++static int git_reftable_transaction_prepare(struct ref_store *ref_store,
++ struct ref_transaction *transaction,
++ struct strbuf *errbuf)
++{
++ transaction->state = REF_TRANSACTION_PREPARED;
++ return 0;
+}
+
+static int git_reftable_transaction_abort(struct ref_store *ref_store,
@@ refs/reftable-backend.c (new)
+ struct ref_transaction *transaction,
+ struct strbuf *errmsg)
+{
++ struct git_reftable_ref_store *refs =
++ (struct git_reftable_ref_store *)transaction->ref_store;
+ struct reftable_addition *add =
+ (struct reftable_addition *)transaction->backend_data;
+ int err = 0;
@@ refs/reftable-backend.c (new)
+
+ err = reftable_addition_commit(add);
+
++ if (!err)
++ err = reftable_stack_auto_compact(refs->main_stack);
+done:
+ assert(err != REFTABLE_API_ERROR);
+ reftable_addition_destroy(add);
@@ refs/reftable-backend.c (new)
+ .name = "reftable",
+ .init = git_reftable_ref_store_create,
+ .init_db = git_reftable_init_db,
++ .transaction_begin = git_reftable_transaction_begin,
+ .transaction_prepare = git_reftable_transaction_prepare,
+ .transaction_finish = git_reftable_transaction_finish,
+ .transaction_abort = git_reftable_transaction_abort,
-: ----------- > 6: 2cf743031ab refs: always try to do packed transactions for reftable
--
gitgitgadget
^ permalink raw reply
* [PATCH v2 2/6] refs: wrap transaction in a debug-specific transaction
From: Han-Wen Nienhuys via GitGitGadget @ 2023-09-20 13:02 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys, Han-Wen Nienhuys, Han-Wen Nienhuys
In-Reply-To: <pull.1574.v2.git.git.1695214968.gitgitgadget@gmail.com>
From: Han-Wen Nienhuys <hanwen@google.com>
This ensures that all transaction methods are properly printed. This is
needed because the callbacks are called as
transaction->ref_store->backend->$CALLBACK.
Originally, there was a hacky workaround which overwrote
transaction->ref_store before passing it into the non-debug prepare()
callback. This would print something for the prepare function, but not
for abort/finish functions.
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
refs/debug.c | 83 +++++++++++++++++++++++++++++++++++++---------------
1 file changed, 59 insertions(+), 24 deletions(-)
diff --git a/refs/debug.c b/refs/debug.c
index 95fb3eb4430..30509da4b89 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -43,25 +43,18 @@ static int debug_init_db(struct ref_store *refs, struct strbuf *err)
static struct ref_transaction *debug_transaction_begin(struct ref_store *ref_store,
struct strbuf *err) {
+ struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
struct ref_transaction *tr;
+ struct ref_transaction *sub_tr = ref_store_transaction_begin(drefs->refs, err);
+ trace_printf_key(&trace_refs, "transaction_begin: %s\n", err->buf);
+ if (!sub_tr) {
+ return NULL;
+ }
CALLOC_ARRAY(tr, 1);
+ tr->backend_data = sub_tr;
return tr;
}
-static int debug_transaction_prepare(struct ref_store *refs,
- struct ref_transaction *transaction,
- struct strbuf *err)
-{
- struct debug_ref_store *drefs = (struct debug_ref_store *)refs;
- int res;
- transaction->ref_store = drefs->refs;
- res = drefs->refs->be->transaction_prepare(drefs->refs, transaction,
- err);
- trace_printf_key(&trace_refs, "transaction_prepare: %d \"%s\"\n", res,
- err->buf);
- return res;
-}
-
static void print_update(int i, const char *refname,
const struct object_id *old_oid,
const struct object_id *new_oid, unsigned int flags,
@@ -77,20 +70,59 @@ static void print_update(int i, const char *refname,
type &= 0xf; /* see refs.h REF_* */
flags &= REF_HAVE_NEW | REF_HAVE_OLD | REF_NO_DEREF |
REF_FORCE_CREATE_REFLOG;
- trace_printf_key(&trace_refs, "%d: %s %s -> %s (F=0x%x, T=0x%x) \"%s\"\n", i, refname,
+ trace_printf_key(&trace_refs, " %d: '%s' %s -> %s (F=0x%x, T=0x%x) \"%s\"\n", i, refname,
o, n, flags, type, msg);
}
-static void print_transaction(struct ref_transaction *transaction)
+static void print_transaction(struct ref_transaction *transaction,
+ const char *action,
+ int res, struct strbuf *err)
{
int i;
- trace_printf_key(&trace_refs, "transaction {\n");
+ trace_printf_key(&trace_refs, "transaction %s {\n", action);
for (i = 0; i < transaction->nr; i++) {
struct ref_update *u = transaction->updates[i];
print_update(i, u->refname, &u->old_oid, &u->new_oid, u->flags,
u->type, u->msg);
}
- trace_printf_key(&trace_refs, "}\n");
+ trace_printf_key(&trace_refs, "}: %d '%s'\n", res, err->buf);
+}
+
+static void copy_update(struct ref_update *dst,
+ struct ref_update *src) {
+ /* dst->refname is const char; can't assign structs */
+ dst->new_oid = src->new_oid;
+ dst->old_oid = src->old_oid;
+ dst->flags = src->flags;
+ dst->backend_data = NULL;
+ dst->type = src->type;
+ dst->msg = xstrdup(src->msg);
+ dst->parent_update = NULL;
+ /* refname is done as part of FLEX_ALLOC_STR. */
+}
+
+static int debug_transaction_prepare(struct ref_store *refs,
+ struct ref_transaction *transaction,
+ struct strbuf *err)
+{
+ struct debug_ref_store *drefs = (struct debug_ref_store *)refs;
+ struct ref_transaction *sub_transaction = transaction->backend_data;
+ int res;
+ int i;
+
+ ALLOC_GROW(sub_transaction->updates, transaction->nr, sub_transaction->alloc);
+ for (i = 0; i < transaction->nr; i++) {
+ struct ref_update *up = transaction->updates[i];
+ struct ref_update *sub_up;
+ FLEX_ALLOC_STR(sub_up, refname, up->refname);
+ copy_update(sub_up, up);
+ sub_transaction->updates[i] = sub_up;
+ }
+ sub_transaction->nr = transaction->nr;
+ res = drefs->refs->be->transaction_prepare(drefs->refs, sub_transaction,
+ err);
+ print_transaction(sub_transaction, "prepare", res, err);
+ return res;
}
static int debug_transaction_finish(struct ref_store *refs,
@@ -98,12 +130,11 @@ static int debug_transaction_finish(struct ref_store *refs,
struct strbuf *err)
{
struct debug_ref_store *drefs = (struct debug_ref_store *)refs;
+ struct ref_transaction *sub_transaction = transaction->backend_data;
int res;
- transaction->ref_store = drefs->refs;
- res = drefs->refs->be->transaction_finish(drefs->refs, transaction,
+ res = drefs->refs->be->transaction_finish(drefs->refs, sub_transaction,
err);
- print_transaction(transaction);
- trace_printf_key(&trace_refs, "finish: %d\n", res);
+ print_transaction(sub_transaction, "finish", res, err);
return res;
}
@@ -112,9 +143,11 @@ static int debug_transaction_abort(struct ref_store *refs,
struct strbuf *err)
{
struct debug_ref_store *drefs = (struct debug_ref_store *)refs;
+ struct ref_transaction *sub_transaction = transaction->backend_data;
int res;
transaction->ref_store = drefs->refs;
- res = drefs->refs->be->transaction_abort(drefs->refs, transaction, err);
+ res = drefs->refs->be->transaction_abort(drefs->refs, sub_transaction, err);
+ print_transaction(sub_transaction, "abort", res, err);
return res;
}
@@ -123,10 +156,12 @@ static int debug_initial_transaction_commit(struct ref_store *refs,
struct strbuf *err)
{
struct debug_ref_store *drefs = (struct debug_ref_store *)refs;
+ struct ref_transaction *sub_transaction = transaction->backend_data;
int res;
transaction->ref_store = drefs->refs;
res = drefs->refs->be->initial_transaction_commit(drefs->refs,
- transaction, err);
+ sub_transaction, err);
+ print_transaction(sub_transaction, "commit", res, err);
return res;
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 1/6] refs: construct transaction using a _begin callback
From: Han-Wen Nienhuys via GitGitGadget @ 2023-09-20 13:02 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys, Han-Wen Nienhuys, Han-Wen Nienhuys
In-Reply-To: <pull.1574.v2.git.git.1695214968.gitgitgadget@gmail.com>
From: Han-Wen Nienhuys <hanwen@google.com>
This prepares for taking locks on constructing the transaction.
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
refs.c | 8 +++-----
refs/debug.c | 8 ++++++++
refs/files-backend.c | 8 ++++++++
refs/packed-backend.c | 9 +++++++++
refs/refs-internal.h | 4 ++++
5 files changed, 32 insertions(+), 5 deletions(-)
diff --git a/refs.c b/refs.c
index fcae5dddc60..f7ba3e56284 100644
--- a/refs.c
+++ b/refs.c
@@ -1129,11 +1129,9 @@ int read_ref_at(struct ref_store *refs, const char *refname,
struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,
struct strbuf *err)
{
- struct ref_transaction *tr;
- assert(err);
-
- CALLOC_ARRAY(tr, 1);
- tr->ref_store = refs;
+ struct ref_transaction *tr = refs->be->transaction_begin(refs, err);
+ if (tr)
+ tr->ref_store = refs;
return tr;
}
diff --git a/refs/debug.c b/refs/debug.c
index b7ffc4ce67e..95fb3eb4430 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -41,6 +41,13 @@ static int debug_init_db(struct ref_store *refs, struct strbuf *err)
return res;
}
+static struct ref_transaction *debug_transaction_begin(struct ref_store *ref_store,
+ struct strbuf *err) {
+ struct ref_transaction *tr;
+ CALLOC_ARRAY(tr, 1);
+ return tr;
+}
+
static int debug_transaction_prepare(struct ref_store *refs,
struct ref_transaction *transaction,
struct strbuf *err)
@@ -451,6 +458,7 @@ struct ref_storage_be refs_be_debug = {
* has a function we should also have a wrapper for it here.
* Test the output with "GIT_TRACE_REFS=1".
*/
+ .transaction_begin = debug_transaction_begin,
.transaction_prepare = debug_transaction_prepare,
.transaction_finish = debug_transaction_finish,
.transaction_abort = debug_transaction_abort,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 341354182bb..d83eea8b408 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2681,6 +2681,13 @@ static void files_transaction_cleanup(struct files_ref_store *refs,
transaction->state = REF_TRANSACTION_CLOSED;
}
+static struct ref_transaction *files_transaction_begin(struct ref_store *ref_store,
+ struct strbuf *err) {
+ struct ref_transaction *tr;
+ CALLOC_ARRAY(tr, 1);
+ return tr;
+}
+
static int files_transaction_prepare(struct ref_store *ref_store,
struct ref_transaction *transaction,
struct strbuf *err)
@@ -3297,6 +3304,7 @@ struct ref_storage_be refs_be_files = {
.name = "files",
.init = files_ref_store_create,
.init_db = files_init_db,
+ .transaction_begin = files_transaction_begin,
.transaction_prepare = files_transaction_prepare,
.transaction_finish = files_transaction_finish,
.transaction_abort = files_transaction_abort,
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 59c78d7941f..4f5b1025605 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1580,6 +1580,14 @@ static void packed_transaction_cleanup(struct packed_ref_store *refs,
transaction->state = REF_TRANSACTION_CLOSED;
}
+
+static struct ref_transaction *packed_transaction_begin(struct ref_store *ref_store,
+ struct strbuf *err) {
+ struct ref_transaction *tr;
+ CALLOC_ARRAY(tr, 1);
+ return tr;
+}
+
static int packed_transaction_prepare(struct ref_store *ref_store,
struct ref_transaction *transaction,
struct strbuf *err)
@@ -1758,6 +1766,7 @@ struct ref_storage_be refs_be_packed = {
.name = "packed",
.init = packed_ref_store_create,
.init_db = packed_init_db,
+ .transaction_begin = packed_transaction_begin,
.transaction_prepare = packed_transaction_prepare,
.transaction_finish = packed_transaction_finish,
.transaction_abort = packed_transaction_abort,
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 9db8aec4da8..0a15e8a2ac8 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -531,6 +531,9 @@ typedef struct ref_store *ref_store_init_fn(struct repository *repo,
typedef int ref_init_db_fn(struct ref_store *refs, struct strbuf *err);
+typedef struct ref_transaction *ref_transaction_begin_fn(struct ref_store *refs,
+ struct strbuf *err);
+
typedef int ref_transaction_prepare_fn(struct ref_store *refs,
struct ref_transaction *transaction,
struct strbuf *err);
@@ -670,6 +673,7 @@ struct ref_storage_be {
ref_store_init_fn *init;
ref_init_db_fn *init_db;
+ ref_transaction_begin_fn *transaction_begin;
ref_transaction_prepare_fn *transaction_prepare;
ref_transaction_finish_fn *transaction_finish;
ref_transaction_abort_fn *transaction_abort;
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v2] bulk-checkin: only support blobs in index_bulk_checkin
From: Eric W. Biederman @ 2023-09-20 12:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: brian m. carlson, git
In-Reply-To: <xmqqr0mtcosy.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> "Eric W. Biederman" <ebiederm@gmail.com> writes:
>
>> As far as I can tell this extra pass defeats most of the purpose of
>> streaming, and it is much easier to implement with in memory buffers.
>
> The purpose of streaming being the ability to hash and compute the
> object name without having to hold the entirety of the object, I am
> not sure the above is a good argument. You can run multiple passes
> by streaming the same data twice if you needed to, and how much
> easier the implementation may become if you can assume that you can
> hold everything in-core, what you cannot fit in-core would not fit
> in-core, so ...
Yes this wording needs to be clarified.
If streaming to handle objects that don't fit in memory is the purpose,
I agree there are slow multi-pass ways to deal with trees, commits and
tags.
If writing directly to the pack is the purpose, using an in-core
buffer for trees, commits, and tags is better.
I will put on the wording on the back burner and see what I come up
with.
>> So if it is needed to write commits, trees, and tags directly to pack
>> files writing a separate function to do the would be needed.
>
> But I am OK with this conclusion. As the way to compute the
> fallback hashes for different types of objects are very different,
> compared to a single-hash world where as long as you come up with a
> serialization you have only a single way to hash and name the
> object. We would end up having separate helper functions per target
> type anyway, even if we kept a single entry point function like
> index_stream(). The single entry point function will only be used
> to just dispatch to type specific ones, so renaming what we have today
> and making it clear they are for "blobs" does make sense.
Good. I am glad I am able to step back and successfully explain the
whys of things.
Eric
^ permalink raw reply
* Re: [PATCH 1/2] parse-options: add int value pointer to struct option
From: René Scharfe @ 2023-09-20 11:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Taylor Blau, Git List, Jeff King
In-Reply-To: <xmqq5y47mp58.fsf@gitster.g>
Am 18.09.23 um 18:17 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> It reduces the memory footprint, but only slightly. Saving a few bytes
>> for objects with less than a hundred instances total doesn't seem worth
>> the downsides.
>
> It makes it impossible to use the both at the same time, which is a
> bigger (than reduced memory) advantage. Otherwise, we would be
> tempted to consider that having "void *value" and "int value_int"
> next to each other and allow them to coexist may be a good solution
> for a narrow corner case (please see at the end of the message you
> are responding to).
Good point. The patch adds a check to parse_options_check() to prevent
double use, which adds some runtime overhead and doesn't fully remove
the temptation.
René
^ permalink raw reply
* Re: [REGRESSION] uninitialized value $address in git send-email
From: Bagas Sanjaya @ 2023-09-20 11:00 UTC (permalink / raw)
To: Michael Strawbridge, Junio C Hamano, Luben Tuikov,
Ævar Arnfjörð Bjarmason, Emily Shaffer,
Doug Anderson
Cc: Git Mailing List
In-Reply-To: <118975ef-c07f-c397-5288-7698e60516a7@amd.com>
[-- Attachment #1: Type: text/plain, Size: 1265 bytes --]
On Tue, Sep 19, 2023 at 10:37:36AM -0400, Michael Strawbridge wrote:
> I suppose I may have mistook what issue was being reported. I had
> originally understood the problem to be that hook related logic was
> failing with correct email addresses, but it seems rather that we are
> trying to fix an error that occurs when an email address that fails
> extract_valid_address_or_die() is given. Feel free to ignore my last
> email if that is all we are trying to solve.
>
Originally, I was intended to report regression on handling multiple
addresses passed in a single --to/--cc/--bcc option. Previously on Git v2.40,
git-send-email(1) accepts `--to="foo <foo@foo.com>,bar <bar@bar.com>"
as two separate --to addresses (with comma as separator). However, on
v2.41 and up, instead I got perl error as I reported in this thread.
Interestingly, that perl error can be reduced into one invalid addresses.
The same thing also happens to --cc and --bcc. I used aforementioned
trick when I was sending patches to LKML to save frin typing the same
option multiple times, each with different address.
If I need to send separate regression report for above use case,
please let me know.
--
An old man doll... just what I always wanted! - Clara
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* [PATCH v4] revision: add `--ignore-missing-links` user option
From: Karthik Nayak @ 2023-09-20 10:45 UTC (permalink / raw)
To: karthik.188; +Cc: git, gitster, me
In-Reply-To: <20230915083415.263187-1-knayak@gitlab.com>
The revision backend is used by multiple porcelain commands such as
git-rev-list(1) and git-log(1). The backend currently supports ignoring
missing links by setting the `ignore_missing_links` bit. This allows the
revision walk to skip any objects links which are missing. Expose this
bit via an `--ignore-missing-links` user option.
A scenario where this option would be used is to find the boundary
objects between different object directories. Consider a repository with
a main object directory (GIT_OBJECT_DIRECTORY) and one or more alternate
object directories (GIT_ALTERNATE_OBJECT_DIRECTORIES). In such a
repository, enabling this option along with the `--boundary` option for
while disabling the alternate object directory allows us to find the
boundary objects between the main and alternate object directory.
Helped-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Changes from v3:
1. Remove hard-coded skipping of finish_object__ma(...). This means that
`--ignore-missing-links` needs to be used with `--missing=...` for missing
non-commit objects, but also now provides the flexibility to the user instead.
Fixes to the tests around this.
2. Fix an incorrect test.
3. Capitalize first character in test's comment.
Range diff from v4
1: a08f3637a0 ! 1: 639a8cc385 revision: add `--ignore-missing-links` user option
@@ Documentation/rev-list-options.txt: explicitly.
--bisect::
Pretend as if the bad bisection ref `refs/bisect/bad`
- ## builtin/rev-list.c ##
-@@ builtin/rev-list.c: static int finish_object(struct object *obj, const char *name UNUSED,
- {
- struct rev_list_info *info = cb_data;
- if (oid_object_info_extended(the_repository, &obj->oid, NULL, 0) < 0) {
-- finish_object__ma(obj);
-+ if (!info->revs->ignore_missing_links)
-+ finish_object__ma(obj);
- return 1;
- }
- if (info->revs->verify_objects && !obj->parsed && obj->type != OBJ_COMMIT)
-
## revision.c ##
@@ revision.c: static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
revs->limited = 1;
@@ t/t6022-rev-list-alternates.sh (new)
+ test_commit_bulk --start=6 -C alt 5
+'
+
-+# when the alternate odb is provided, all commits are listed along with the boundary
++# When the alternate odb is provided, all commits are listed along with the boundary
+# commit.
+test_expect_success 'rev-list passes with alternate object directory' '
+ git -C alt rev-list --all --objects --no-object-names >actual.raw &&
@@ t/t6022-rev-list-alternates.sh (new)
+ hide_alternates &&
+
+ git -C alt rev-list --objects --no-object-names \
-+ --ignore-missing-links HEAD >actual.raw &&
++ --ignore-missing-links --missing=allow-any HEAD >actual.raw &&
+ git -C alt cat-file --batch-check="%(objectname)" \
+ --batch-all-objects >expect.raw &&
+
+ sort actual.raw >actual &&
+ sort expect.raw >expect &&
-+ test_must_fail git -C alt rev-list HEAD
++ test_cmp expect actual
+'
+
+# With `--ignore-missing-links` and `--boundary`, we can even print those boundary
@@ t/t6022-rev-list-alternates.sh (new)
+ mv "$path" "$path.hidden" &&
+ test_when_finished "mv $path.hidden $path" &&
+
-+ git -C alt rev-list --ignore-missing-links --objects HEAD \
++ git -C alt rev-list --ignore-missing-links --missing=allow-any --objects HEAD \
+ >actual &&
+ ! grep $oid actual
+ '
Documentation/rev-list-options.txt | 9 +++
revision.c | 2 +
t/t6022-rev-list-alternates.sh | 93 ++++++++++++++++++++++++++++++
3 files changed, 104 insertions(+)
create mode 100755 t/t6022-rev-list-alternates.sh
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index a4a0cb93b2..8ee713db3d 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -227,6 +227,15 @@ explicitly.
Upon seeing an invalid object name in the input, pretend as if
the bad input was not given.
+--ignore-missing-links::
+ During traversal, if an object that is referenced does not
+ exist, instead of dying of a repository corruption, pretend as
+ if the reference itself does not exist. Running the command
+ with the `--boundary` option makes these missing commits,
+ together with the commits on the edge of revision ranges
+ (i.e. true boundary objects), appear on the output, prefixed
+ with '-'.
+
ifndef::git-rev-list[]
--bisect::
Pretend as if the bad bisection ref `refs/bisect/bad`
diff --git a/revision.c b/revision.c
index 2f4c53ea20..cbfcbf6e28 100644
--- a/revision.c
+++ b/revision.c
@@ -2595,6 +2595,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
revs->limited = 1;
} else if (!strcmp(arg, "--ignore-missing")) {
revs->ignore_missing = 1;
+ } else if (!strcmp(arg, "--ignore-missing-links")) {
+ revs->ignore_missing_links = 1;
} else if (opt && opt->allow_exclude_promisor_objects &&
!strcmp(arg, "--exclude-promisor-objects")) {
if (fetch_if_missing)
diff --git a/t/t6022-rev-list-alternates.sh b/t/t6022-rev-list-alternates.sh
new file mode 100755
index 0000000000..9ba739c830
--- /dev/null
+++ b/t/t6022-rev-list-alternates.sh
@@ -0,0 +1,93 @@
+#!/bin/sh
+
+test_description='handling of alternates in rev-list'
+
+TEST_PASSES_SANITIZE_LEAK=true
+. ./test-lib.sh
+
+# We create 5 commits and move them to the alt directory and
+# create 5 more commits which will stay in the main odb.
+test_expect_success 'create repository and alternate directory' '
+ test_commit_bulk 5 &&
+ git clone --reference=. --shared . alt &&
+ test_commit_bulk --start=6 -C alt 5
+'
+
+# When the alternate odb is provided, all commits are listed along with the boundary
+# commit.
+test_expect_success 'rev-list passes with alternate object directory' '
+ git -C alt rev-list --all --objects --no-object-names >actual.raw &&
+ {
+ git rev-list --all --objects --no-object-names &&
+ git -C alt rev-list --all --objects --no-object-names --not \
+ --alternate-refs
+ } >expect.raw &&
+ sort actual.raw >actual &&
+ sort expect.raw >expect &&
+ test_cmp expect actual
+'
+
+alt=alt/.git/objects/info/alternates
+
+hide_alternates () {
+ test -f "$alt.bak" || mv "$alt" "$alt.bak"
+}
+
+show_alternates () {
+ test -f "$alt" || mv "$alt.bak" "$alt"
+}
+
+# When the alternate odb is not provided, rev-list fails since the 5th commit's
+# parent is not present in the main odb.
+test_expect_success 'rev-list fails without alternate object directory' '
+ hide_alternates &&
+ test_must_fail git -C alt rev-list HEAD
+'
+
+# With `--ignore-missing-links`, we stop the traversal when we encounter a
+# missing link. The boundary commit is not listed as we haven't used the
+# `--boundary` options.
+test_expect_success 'rev-list only prints main odb commits with --ignore-missing-links' '
+ hide_alternates &&
+
+ git -C alt rev-list --objects --no-object-names \
+ --ignore-missing-links --missing=allow-any HEAD >actual.raw &&
+ git -C alt cat-file --batch-check="%(objectname)" \
+ --batch-all-objects >expect.raw &&
+
+ sort actual.raw >actual &&
+ sort expect.raw >expect &&
+ test_cmp expect actual
+'
+
+# With `--ignore-missing-links` and `--boundary`, we can even print those boundary
+# commits.
+test_expect_success 'rev-list prints boundary commit with --ignore-missing-links' '
+ git -C alt rev-list --ignore-missing-links --boundary HEAD >got &&
+ grep "^-$(git rev-parse HEAD)" got
+'
+
+test_expect_success "setup for rev-list --ignore-missing-links with missing objects" '
+ show_alternates &&
+ test_commit -C alt 11
+'
+
+for obj in "HEAD^{tree}" "HEAD:11.t"
+do
+ # The `--ignore-missing-links` option should ensure that git-rev-list(1)
+ # doesn't fail when used alongside `--objects` when a tree/blob is
+ # missing.
+ test_expect_success "rev-list --ignore-missing-links with missing $type" '
+ oid="$(git -C alt rev-parse $obj)" &&
+ path="alt/.git/objects/$(test_oid_to_path $oid)" &&
+
+ mv "$path" "$path.hidden" &&
+ test_when_finished "mv $path.hidden $path" &&
+
+ git -C alt rev-list --ignore-missing-links --missing=allow-any --objects HEAD \
+ >actual &&
+ ! grep $oid actual
+ '
+done
+
+test_done
--
2.41.0
^ permalink raw reply related
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
From: René Scharfe @ 2023-09-20 8:18 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: Git List, Jeff King, Junio C Hamano
In-Reply-To: <ZQlspgfu7yDW0oTN@ugly>
Am 19.09.23 um 11:40 schrieb Oswald Buddenhagen:
> On Sat, Sep 09, 2023 at 11:14:20PM +0200, René Scharfe wrote:
>> Some uses of OPT_CMDMODE provide a pointer to an enum. It is
>> dereferenced as an int pointer in parse-options.c::get_value(). These
>> two types are incompatible, though
>>
> s/are/may be/ - c.f. https://en.cppreference.com/w/c/language/enum
You're right. Citing the relevant part: "Each enumerated type [...] is
compatible with one of: char, a signed integer type, or an unsigned
integer type [...]. It is implementation-defined which type is
compatible with any given enumerated type [...]." So there's a chance
that the underlying type would be compatible by accident.
When we try a few combinations (https://godbolt.org/z/KvKcndY4Y),
Clang warns about incompatible pointers if we use a pointer to an enum
with only positive values as int pointer and about different signs if
use a pointer to an enum with negative values as in unsigned int
pointer and accepts the rest. GCC accepts the same cases, but all its
warnings are about incompatible pointers. This seems to be dependent
on the optimization level, though. MSVC warns about all combinations.
>> -- the storage size of an enum can vary between platforms.
>>
> here's a completely different perspective:
> this is merely a theoretical problem, right? gcc for example won't
> actually use non-ints unless -fshort-enums is supplied. so how about
> simply adding a (configure) test to ensure that there is actually no
> problem, and calling it a day?
That would be an easy, but complex solution. If the check is done
using -Wincompatible-pointer-types or equivalent then MSVC is out. If
we base it on type size then we're making assumptions that I find hard
to justify. Using the same type at both ends of the void and avoiding
compiler warnings that would have been issued if we'd cut out the
middle part is simpler overall.
René
^ permalink raw reply
* Re: [PATCH v2] bulk-checkin: only support blobs in index_bulk_checkin
From: Junio C Hamano @ 2023-09-20 6:59 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: brian m. carlson, git
In-Reply-To: <878r918ps3.fsf@gmail.froward.int.ebiederm.org>
"Eric W. Biederman" <ebiederm@gmail.com> writes:
> As the code is written today index_bulk_checkin only accepts blobs.
> Remove the enum object_type parameter and rename index_bulk_checkin to
> index_blob_bulk_checkin, index_stream to index_blob_stream,
> deflate_to_pack to deflate_blob_to_pack, stream_to_pack to
> stream_blobk_to_pack, to make this explicit.
> Not supporting commits, tags, or trees has no downside as it is not
> currently supported now, and commits, tags, and trees being smaller by
> design do not have the problem that the problem that index_bulk_checkin
> was built to solve.
Exactly. The streaming was primarily to help dealing with huge
blobs that cannot be held in-core. Of course other parts (like
comparing them) of the system would require to hold them in-core
so some things may not work for them, but at least it is a start
to be able to _hash_ them to store them in the object store and to
give them names.
> What is more this is very desiable from the context of the hash function
> transition.
A bit hard to parse; perhaps want a comma before "this"?
> For blob objects it is straight forward to compute multiple hash
> functions during index_bulk_checkin as the object header and content of
> a blob is the same no matter which hash function is being used to
> compute the oid of a blob.
OK.
> For commits, tress, and tags the object header and content that need to
> be hashed ard different for different hashes. Even worse the object
> header can not be known until the size of the content that needs to be
> hashed is known. The size of the content that needs to be hashed can
> not be known until a complete pass is made through all of the variable
> length entries of the original object.
"tress" -> "trees". Also a comma after "worse".
> As far as I can tell this extra pass defeats most of the purpose of
> streaming, and it is much easier to implement with in memory buffers.
The purpose of streaming being the ability to hash and compute the
object name without having to hold the entirety of the object, I am
not sure the above is a good argument. You can run multiple passes
by streaming the same data twice if you needed to, and how much
easier the implementation may become if you can assume that you can
hold everything in-core, what you cannot fit in-core would not fit
in-core, so ...
> So if it is needed to write commits, trees, and tags directly to pack
> files writing a separate function to do the would be needed.
But I am OK with this conclusion. As the way to compute the
fallback hashes for different types of objects are very different,
compared to a single-hash world where as long as you come up with a
serialization you have only a single way to hash and name the
object. We would end up having separate helper functions per target
type anyway, even if we kept a single entry point function like
index_stream(). The single entry point function will only be used
to just dispatch to type specific ones, so renaming what we have today
and making it clear they are for "blobs" does make sense.
^ permalink raw reply
* Re: [PATCH v3 13/13] trailer doc: <token> is a <key> or <keyAlias>, not both
From: Linus Arver @ 2023-09-20 6:48 UTC (permalink / raw)
To: Jonathan Tan, Linus Arver via GitGitGadget
Cc: Jonathan Tan, git, Christian Couder
In-Reply-To: <20230919225926.2189091-1-jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> @@ -248,34 +258,40 @@ With `add`, a new trailer will be added.
>> +
>> With `doNothing`, nothing will be done.
>>
>> -trailer.<token>.key::
>> - This `key` will be used instead of <token> in the trailer. At
>> - the end of this key, a separator can appear and then some
>> - space characters. By default the only valid separator is ':',
>> - but this can be changed using the `trailer.separators` config
>> - variable.
>> +trailer.<keyAlias>.key::
>> + Defines a <keyAlias> for the <key>. The <keyAlias> must be a
>> + prefix (case does not matter) of the <key>. For example, in `git
>> + config trailer.ack.key "Acked-by"` the "Acked-by" is the <key> and
>> + the "ack" is the <keyAlias>. This configuration allows the shorter
>> + `--trailer "ack:..."` invocation on the command line using the "ack"
>> + <keyAlias> instead of the longer `--trailer "Acked-by:..."`.
>> ++
>> +At the end of the <key>, a separator can appear and then some
>> +space characters. By default the only valid separator is ':',
>> +but this can be changed using the `trailer.separators` config
>> +variable.
>
> I think all the other patches will be a great help to the user, but I'm
> on the fence about this one.
Ack. I'm OK with dropping this patch (I assume I don't need to reroll
this series for that?).
> Someone who knows these trailer components
> by their old names might be confused upon seeing tne new ones, so I'm
> inclined to minimize such changes.
True, I had some inclination to do the same kind of change but for the
trailer.c code also (that code uses the "token" language also,
furthering the ambiguity, sadly) but wanted to just do the user-facing
part first because I thought the users shouldn't need to know about the
internals anyway.
If I did revise this patch to include the same changes in trailer.c and
not just the docs, would you be more willing to support the (new) patch?
I'm mainly curious about what will make you more comfortable to accept
this change.
> I do think that the new names make
> more sense, though.
Thanks!
> The documentation doesn't seem to say what happens when trailer.ack.cmd
> and trailer.Acked-by.cmd (replace "cmd" with whatever) are defined, but
> that was true previously too (and knowing this does not really enable
> the user to be able to do something they previously couldn't, so this
> is fine).
Ah interesting point. I'll try to remember this when I revisit this
patch again in the (near?) future.
^ permalink raw reply
* Re: What's cooking in git.git (Sep 2023, #05; Fri, 15)
From: Linus Arver @ 2023-09-20 6:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq34z9g8jz.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Linus Arver <linusa@google.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> [Cooking]
>>>
>>> [...]
>>>
>>> * la/trailer-cleanups (2023-09-11) 6 commits
>>> (merged to 'next' on 2023-09-12 at 779c4a097a)
>>> + trailer: use offsets for trailer_start/trailer_end
>>> + trailer: rename *_DEFAULT enums to *_UNSPECIFIED
>>> + trailer: teach find_patch_start about --no-divider
>>> + trailer: split process_command_line_args into separate functions
>>> + trailer: split process_input_file into separate pieces
>>> + trailer: separate public from internal portion of trailer_iterator
>>>
>>> Code clean-up.
>>>
>>> Will merge to 'master'.
>>> source: <pull.1563.v2.git.1694240177.gitgitgadget@gmail.com>
>>
>> This isn't ready yet (still need to reroll).
>
> Whoa, wait. I wasn't aware of any more comments that needed
> addressing.
Perhaps you missed my comment [1]?
> Whatever improvements you have in mind, if they are
> minor, letting the above graduate (they have been in 'next' for a
> week without anybody complaining) and doing them as a follow-up
> series would be sensible, I would think.
>
> Thanks.
Hmm, I don't think they are minor? See
https://github.com/listx/git/tree/trailer-libification-prep for the
current state of things.
I need to still follow up to your last comment on "trailer: rename
*_DEFAULT enums to *_UNSPECIFIED" [2] (I was going to see if we needed
the "obvious solution" as you described).
If it's too painful to move this out of 'next' now, I'm OK with it
graduating as is and doing a separate follow-up (I expect several more
of these to happen anyway). Up to you.
Sorry for not noticing that this was in 'next' sooner and communicating
accordingly.
[1] https://lore.kernel.org/git/pull.1563.git.1691211879.gitgitgadget@gmail.com/T/#m0131f9829c35d8e0103ffa88f07d8e0e43dd732c
[2] https://lore.kernel.org/git/pull.1563.git.1691211879.gitgitgadget@gmail.com/T/#md4ff2235720e921a2e031665f4da054d82edd506
^ permalink raw reply
* [PATCH v2] bulk-checkin: only support blobs in index_bulk_checkin
From: Eric W. Biederman @ 2023-09-20 3:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: brian m. carlson, git
As the code is written today index_bulk_checkin only accepts blobs.
Remove the enum object_type parameter and rename index_bulk_checkin to
index_blob_bulk_checkin, index_stream to index_blob_stream,
deflate_to_pack to deflate_blob_to_pack, stream_to_pack to
stream_blobk_to_pack, to make this explicit.
Not supporting commits, tags, or trees has no downside as it is not
currently supported now, and commits, tags, and trees being smaller by
design do not have the problem that the problem that index_bulk_checkin
was built to solve.
What is more this is very desiable from the context of the hash function
transition.
For blob objects it is straight forward to compute multiple hash
functions during index_bulk_checkin as the object header and content of
a blob is the same no matter which hash function is being used to
compute the oid of a blob.
For commits, tress, and tags the object header and content that need to
be hashed ard different for different hashes. Even worse the object
header can not be known until the size of the content that needs to be
hashed is known. The size of the content that needs to be hashed can
not be known until a complete pass is made through all of the variable
length entries of the original object.
As far as I can tell this extra pass defeats most of the purpose of
streaming, and it is much easier to implement with in memory buffers.
So if it is needed to write commits, trees, and tags directly to pack
files writing a separate function to do the would be needed.
So let's just simplify the code base for now, simplify the development
needed for the hash function transition and only support blobs with the
existing bulk_checkin code.
Inspired-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
bulk-checkin.c | 35 +++++++++++++++++------------------
bulk-checkin.h | 6 +++---
object-file.c | 12 ++++++------
3 files changed, 26 insertions(+), 27 deletions(-)
This is just a v2 of the description, that addresses Junio's
capitalization concern, and hopefully makes the justification clear to
other people.
I am sending it now mostly because the original version did not
land on the mailing list for some reason. So I have switched
which email account I am using for now.
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 73bff3a23d27..223562b4e748 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -155,10 +155,10 @@ static int already_written(struct bulk_checkin_packfile *state, struct object_id
* status before calling us just in case we ask it to call us again
* with a new pack.
*/
-static int stream_to_pack(struct bulk_checkin_packfile *state,
- git_hash_ctx *ctx, off_t *already_hashed_to,
- int fd, size_t size, enum object_type type,
- const char *path, unsigned flags)
+static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
+ git_hash_ctx *ctx, off_t *already_hashed_to,
+ int fd, size_t size, const char *path,
+ unsigned flags)
{
git_zstream s;
unsigned char ibuf[16384];
@@ -170,7 +170,7 @@ static int stream_to_pack(struct bulk_checkin_packfile *state,
git_deflate_init(&s, pack_compression_level);
- hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), type, size);
+ hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), OBJ_BLOB, size);
s.next_out = obuf + hdrlen;
s.avail_out = sizeof(obuf) - hdrlen;
@@ -247,11 +247,10 @@ static void prepare_to_stream(struct bulk_checkin_packfile *state,
die_errno("unable to write pack header");
}
-static int deflate_to_pack(struct bulk_checkin_packfile *state,
- struct object_id *result_oid,
- int fd, size_t size,
- enum object_type type, const char *path,
- unsigned flags)
+static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
+ struct object_id *result_oid,
+ int fd, size_t size,
+ const char *path, unsigned flags)
{
off_t seekback, already_hashed_to;
git_hash_ctx ctx;
@@ -265,7 +264,7 @@ static int deflate_to_pack(struct bulk_checkin_packfile *state,
return error("cannot find the current offset");
header_len = format_object_header((char *)obuf, sizeof(obuf),
- type, size);
+ OBJ_BLOB, size);
the_hash_algo->init_fn(&ctx);
the_hash_algo->update_fn(&ctx, obuf, header_len);
@@ -282,8 +281,8 @@ static int deflate_to_pack(struct bulk_checkin_packfile *state,
idx->offset = state->offset;
crc32_begin(state->f);
}
- if (!stream_to_pack(state, &ctx, &already_hashed_to,
- fd, size, type, path, flags))
+ if (!stream_blob_to_pack(state, &ctx, &already_hashed_to,
+ fd, size, path, flags))
break;
/*
* Writing this object to the current pack will make
@@ -350,12 +349,12 @@ void fsync_loose_object_bulk_checkin(int fd, const char *filename)
}
}
-int index_bulk_checkin(struct object_id *oid,
- int fd, size_t size, enum object_type type,
- const char *path, unsigned flags)
+int index_blob_bulk_checkin(struct object_id *oid,
+ int fd, size_t size,
+ const char *path, unsigned flags)
{
- int status = deflate_to_pack(&bulk_checkin_packfile, oid, fd, size, type,
- path, flags);
+ int status = deflate_blob_to_pack(&bulk_checkin_packfile, oid, fd, size,
+ path, flags);
if (!odb_transaction_nesting)
flush_bulk_checkin_packfile(&bulk_checkin_packfile);
return status;
diff --git a/bulk-checkin.h b/bulk-checkin.h
index 48fe9a6e9171..aa7286a7b3e1 100644
--- a/bulk-checkin.h
+++ b/bulk-checkin.h
@@ -9,9 +9,9 @@
void prepare_loose_object_bulk_checkin(void);
void fsync_loose_object_bulk_checkin(int fd, const char *filename);
-int index_bulk_checkin(struct object_id *oid,
- int fd, size_t size, enum object_type type,
- const char *path, unsigned flags);
+int index_blob_bulk_checkin(struct object_id *oid,
+ int fd, size_t size,
+ const char *path, unsigned flags);
/*
* Tell the object database to optimize for adding
diff --git a/object-file.c b/object-file.c
index 7dc0c4bfbba8..7c7afe579364 100644
--- a/object-file.c
+++ b/object-file.c
@@ -2446,11 +2446,11 @@ static int index_core(struct index_state *istate,
* binary blobs, they generally do not want to get any conversion, and
* callers should avoid this code path when filters are requested.
*/
-static int index_stream(struct object_id *oid, int fd, size_t size,
- enum object_type type, const char *path,
- unsigned flags)
+static int index_blob_stream(struct object_id *oid, int fd, size_t size,
+ const char *path,
+ unsigned flags)
{
- return index_bulk_checkin(oid, fd, size, type, path, flags);
+ return index_blob_bulk_checkin(oid, fd, size, path, flags);
}
int index_fd(struct index_state *istate, struct object_id *oid,
@@ -2472,8 +2472,8 @@ int index_fd(struct index_state *istate, struct object_id *oid,
ret = index_core(istate, oid, fd, xsize_t(st->st_size),
type, path, flags);
else
- ret = index_stream(oid, fd, xsize_t(st->st_size), type, path,
- flags);
+ ret = index_blob_stream(oid, fd, xsize_t(st->st_size), path,
+ flags);
close(fd);
return ret;
}
--
2.41.0
Eric
^ permalink raw reply related
* What's cooking in git.git (Sep 2023, #06; Tue, 19)
From: Junio C Hamano @ 2023-09-20 0:49 UTC (permalink / raw)
To: git
Here are the topics that have been cooking in my tree. Commits
prefixed with '+' are in 'next' (being in 'next' is a sign that a
topic is stable enough to be used and are candidate to be in a
future release). Commits prefixed with '-' are only in 'seen', and
aren't considered "accepted" at all and may be annotated with an URL
to a message that raises issues but they are no means exhaustive. A
topic without enough support may be discarded after a long period of
no activity (of course they can be resubmit when new interests
arise).
Copies of the source code to Git live in many repositories, and the
following is a list of the ones I push into or their mirrors. Some
repositories have only a subset of branches.
With maint, master, next, seen, todo:
git://git.kernel.org/pub/scm/git/git.git/
git://repo.or.cz/alt-git.git/
https://kernel.googlesource.com/pub/scm/git/git/
https://github.com/git/git/
https://gitlab.com/git-vcs/git/
With all the integration branches and topics broken out:
https://github.com/gitster/git/
Even though the preformatted documentation in HTML and man format
are not sources, they are published in these repositories for
convenience (replace "htmldocs" with "manpages" for the manual
pages):
git://git.kernel.org/pub/scm/git/git-htmldocs.git/
https://github.com/gitster/git-htmldocs.git/
Release tarballs are available at:
https://www.kernel.org/pub/software/scm/git/
--------------------------------------------------
[Graduated to 'master']
* js/complete-checkout-t (2023-09-08) 1 commit
(merged to 'next' on 2023-09-08 at 461bb28fbd)
+ completion(switch/checkout): treat --track and -t the same
The completion script (in contrib/) has been taught to treat the
"-t" option to "git checkout" and "git switch" just like the
"--track" option, to complete remote-tracking branches.
source: <pull.1584.git.1694176123471.gitgitgadget@gmail.com>
* rs/grep-no-no-or (2023-09-07) 1 commit
(merged to 'next' on 2023-09-08 at 33849032bc)
+ grep: reject --no-or
"git grep -e A --no-or -e B" is accepted, even though the negation
of "or" did not mean anything, which has been tightened.
source: <6aeb0ebe-0fea-ccd3-089a-ee0b5b5baf10@web.de>
--------------------------------------------------
[New Topics]
* jc/diff-cached-fsmonitor-fix (2023-09-15) 3 commits
- diff-lib: fix check_removed() when fsmonitor is active
- Merge branch 'jc/fake-lstat' into jc/diff-cached-fsmonitor-fix
- Merge branch 'js/diff-cached-fsmonitor-fix' into jc/diff-cached-fsmonitor-fix
(this branch uses jc/fake-lstat and js/diff-cached-fsmonitor-fix.)
The optimization based on fsmonitor in the "diff --cached"
codepath is resurrected with the "fake-lstat" introduced earlier.
It is unknown if the optimization is worth resurrecting, but in case...
source: <xmqqr0n0h0tw.fsf@gitster.g>
* ml/git-gui-exec-path-fix (2023-09-18) 3 commits
(merged to 'next' on 2023-09-19 at 0565b0b14b)
+ Merge git-gui into ml/git-gui-exec-path-fix
+ git-gui - use git-hook, honor core.hooksPath
+ git-gui - re-enable use of hook scripts
Fix recent regression in Git-GUI that fails to run hook scripts at
all.
Will merge to 'master'.
* ds/stat-name-width-configuration (2023-09-18) 1 commit
- diff --stat: add config option to limit filename width
"git diff" learned diff.statNameWidth configuration variable, to
give the default width for the name part in the "--stat" output.
Will merge to 'next'.
source: <87badb12f040d1c66cd9b89074d3de5015a45983.1694446743.git.dsimic@manjaro.org>
* tb/send-email-extract-valid-address-error-message-fix (2023-09-18) 1 commit
(merged to 'next' on 2023-09-19 at d037282391)
+ git-send-email.perl: avoid printing undef when validating addresses
An error message given by "git send-email" when given a malformed
address did not give correct information, which has been corrected.
Will merge to 'master'.
source: <545729b619308c6f3397b9aa1747f26ddc58f461.1695054945.git.me@ttaylorr.com>
* jk/fsmonitor-unused-parameter (2023-09-18) 8 commits
(merged to 'next' on 2023-09-19 at bd06505f9e)
+ run-command: mark unused parameters in start_bg_wait callbacks
+ fsmonitor: mark unused hashmap callback parameters
+ fsmonitor/darwin: mark unused parameters in system callback
+ fsmonitor: mark unused parameters in stub functions
+ fsmonitor/win32: mark unused parameter in fsm_os__incompatible()
+ fsmonitor: mark some maybe-unused parameters
+ fsmonitor/win32: drop unused parameters
+ fsmonitor: prefer repo_git_path() to git_pathdup()
Unused parameters in fsmonitor related code paths have been marked
as such.
Will merge to 'master'.
source: <20230918222908.GA2659096@coredump.intra.peff.net>
--------------------------------------------------
[Stalled]
* tk/cherry-pick-sequence-requires-clean-worktree (2023-06-01) 1 commit
- cherry-pick: refuse cherry-pick sequence if index is dirty
"git cherry-pick A" that replays a single commit stopped before
clobbering local modification, but "git cherry-pick A..B" did not,
which has been corrected.
Expecting a reroll.
cf. <999f12b2-38d6-f446-e763-4985116ad37d@gmail.com>
source: <pull.1535.v2.git.1685264889088.gitgitgadget@gmail.com>
--------------------------------------------------
[Cooking]
* ob/sequencer-remove-dead-code (2023-09-12) 1 commit
(merged to 'next' on 2023-09-13 at 1f6c2b336b)
+ sequencer: remove unreachable exit condition in pick_commits()
Code clean-up.
Will merge to 'master'.
source: <20230912105541.272917-1-oswald.buddenhagen@gmx.de>
* ob/t3404-typofix (2023-09-12) 1 commit
(merged to 'next' on 2023-09-13 at b17fa78c3f)
+ t3404-rebase-interactive.sh: fix typos in title of a rewording test
Code clean-up.
Will merge to 'master'.
source: <20230912104237.271616-1-oswald.buddenhagen@gmx.de>
* jk/ort-unused-parameter-cleanups (2023-09-16) 5 commits
(merged to 'next' on 2023-09-18 at 25a5dfc38e)
+ merge-ort: lowercase a few error messages
+ merge-ort: drop unused "opt" parameter from merge_check_renames_reusable()
+ merge-ort: drop unused parameters from detect_and_process_renames()
+ merge-ort: stop passing "opt" to read_oid_strbuf()
+ merge-ort: drop custom err() function
Code clean-up.
Will merge to 'master'.
source: <20230914093409.GA2254811@coredump.intra.peff.net>
* jc/fake-lstat (2023-09-15) 1 commit
- cache: add fake_lstat()
(this branch is used by jc/diff-cached-fsmonitor-fix.)
A new helper to let us pretend that we called lstat() when we know
our cache_entry is up-to-date via fsmonitor.
Needs review.
source: <xmqqcyykig1l.fsf@gitster.g>
* jk/redact-h2h3-headers-fix (2023-09-15) 2 commits
(merged to 'next' on 2023-09-19 at d40278ef9e)
+ http: update curl http/2 info matching for curl 8.3.0
+ http: factor out matching of curl http/2 trace lines
HTTP Header redaction code has been adjusted for a newer version of
cURL library that shows its traces differently from earlier
versions.
Will merge to 'master'.
source: <20230915113237.GA3531328@coredump.intra.peff.net>
* ch/clean-docfix (2023-09-15) 1 commit
(merged to 'next' on 2023-09-19 at 1e16b715bb)
+ git-clean doc: fix "without do cleaning" typo
Typofix.
Will merge to 'master'.
source: <pull.1572.git.git.1694800409471.gitgitgadget@gmail.com>
* eg/config-type-path-docfix (2023-09-15) 1 commit
(merged to 'next' on 2023-09-19 at 2c8ad3e731)
+ git-config: fix misworded --type=path explanation
Typofix.
Will merge to 'master'.
source: <20230915202610.21206-1-evan.gates@gmail.com>
* kn/rev-list-ignore-missing-links (2023-09-15) 1 commit
- revision: add `--ignore-missing-links` user option
Surface the .ignore_missing_links bit that stops the revision
traversal from stopping and dying when encountering a missing
object to a new command line option of "git rev-list", so that the
objects that are required but are missing can be enumerated.
Waiting for review response.
source: <20230915083415.263187-1-knayak@gitlab.com>
* js/systemd-timers-wsl-fix (2023-09-11) 1 commit
(merged to 'next' on 2023-09-12 at 71c4cbb1df)
+ maintenance(systemd): support the Windows Subsystem for Linux
Update "git maintainance" timers' implementation based on systemd
timers to work with WSL.
Will merge to 'master'.
source: <pull.1586.git.1694334620260.gitgitgadget@gmail.com>
* kh/range-diff-notes (2023-09-19) 1 commit
- range-diff: treat notes like `log`
"git range-diff --notes=foo" compared "log --notes=foo --notes" of
the two ranges, instead of using just the specified notes tree.
Will merge to 'next'.
source: <6e114271a2e7d2323193bd58bb307f60101942ce.1695154855.git.code@khaugsbakk.name>
* pw/diff-no-index-from-named-pipes (2023-09-11) 1 commit
(merged to 'next' on 2023-09-12 at 135ecd136f)
+ diff --no-index: fix -R with stdin
"git diff --no-index -R <(one) <(two)" did not work correctly,
which has been corrected.
Will merge to 'master'.
source: <22fdfa3b-f90e-afcc-667c-705fb7670245@web.de>
* rs/parse-options-value-int (2023-09-18) 2 commits
- parse-options: use and require int pointer for OPT_CMDMODE
- parse-options: add int value pointer to struct option
A bit of type safety for the "value" pointer used in the
parse-options API.
Comments?
source: <e6d8a291-03de-cfd3-3813-747fc2cad145@web.de>
* so/diff-merges-d (2023-09-11) 2 commits
- diff-merges: introduce '-d' option
- diff-merges: improve --diff-merges documentation
Teach a new "-d" option that shows the patch against the first
parent for merge commits (which is "--diff-merges=first-parent -p").
Letting a less useful combination of options squat on short-and-sweet "-d" feels dubious.
source: <20230909125446.142715-1-sorganov@gmail.com>
* js/diff-cached-fsmonitor-fix (2023-09-11) 1 commit
(merged to 'next' on 2023-09-12 at 7479278da0)
+ diff-lib: fix check_removed when fsmonitor is on
(this branch is used by jc/diff-cached-fsmonitor-fix.)
"git diff --cached" codepath did not fill the necessary stat
information for a file when fsmonitor knows it is clean and ended
up behaving as if it is not clean, which has been corrected.
Will merge to 'master'.
source: <20230911170901.49050-2-sokcevic@google.com>
* pb/completion-aliases-doc (2023-09-12) 1 commit
(merged to 'next' on 2023-09-13 at b248a5bc26)
+ completion: improve doc for complex aliases
Clarify how "alias.foo = : git cmd ; aliased-command-string" should
be spelled with necessary whitespaces around punctuation marks to
work.
Will merge to 'master'.
source: <pull.1585.v2.git.1694538135853.gitgitgadget@gmail.com>
* cc/repack-sift-filtered-objects-to-separate-pack (2023-09-11) 9 commits
. gc: add `gc.repackFilterTo` config option
. repack: implement `--filter-to` for storing filtered out objects
. gc: add `gc.repackFilter` config option
. repack: add `--filter=<filter-spec>` option
. pack-bitmap-write: rebuild using new bitmap when remapping
. repack: refactor finding pack prefix
. repack: refactor finishing pack-objects command
. t/helper: add 'find-pack' test-tool
. pack-objects: allow `--filter` without `--stdout`
"git repack" machinery learns to pay attention to the "--filter="
option.
May need to wait until tb/repack-existing-packs-cleanup stablizes.
source: <20230911150618.129737-1-christian.couder@gmail.com>
* la/trailer-cleanups (2023-09-11) 6 commits
(merged to 'next' on 2023-09-12 at 779c4a097a)
+ trailer: use offsets for trailer_start/trailer_end
+ trailer: rename *_DEFAULT enums to *_UNSPECIFIED
+ trailer: teach find_patch_start about --no-divider
+ trailer: split process_command_line_args into separate functions
+ trailer: split process_input_file into separate pieces
+ trailer: separate public from internal portion of trailer_iterator
Code clean-up.
Will merge to 'master'.
source: <pull.1563.v2.git.1694240177.gitgitgadget@gmail.com>
* tb/repack-existing-packs-cleanup (2023-09-13) 8 commits
(merged to 'next' on 2023-09-14 at bb8065e89c)
+ builtin/repack.c: extract common cruft pack loop
+ builtin/repack.c: avoid directly inspecting "util"
+ builtin/repack.c: store existing cruft packs separately
+ builtin/repack.c: extract `has_existing_non_kept_packs()`
+ builtin/repack.c: extract redundant pack cleanup for existing packs
+ builtin/repack.c: extract redundant pack cleanup for --geometric
+ builtin/repack.c: extract marking packs for deletion
+ builtin/repack.c: extract structure to store existing packs
The code to keep track of existing packs in the repository while
repacking has been refactored.
Will merge to 'master'.
source: <cover.1694632644.git.me@ttaylorr.com>
* pb/complete-commit-trailers (2023-09-12) 2 commits
(merged to 'next' on 2023-09-13 at 9a0ec17606)
+ completion: commit: complete trailers tokens more robustly
(merged to 'next' on 2023-09-08 at 842587016d)
+ completion: commit: complete configured trailer tokens
The command-line complation support (in contrib/) learned to
complete "git commit --trailer=" for possible trailer keys.
Will merge to 'master'.
source: <pull.1583.v2.git.1694539827.gitgitgadget@gmail.com>
* pw/rebase-sigint (2023-09-07) 1 commit
- rebase -i: ignore signals when forking subprocesses
If the commit log editor or other external programs (spawned via
"exec" insn in the todo list) receive internactive signal during
"git rebase -i", it caused not just the spawned program but the
"Git" process that spawned them, which is often not what the end
user intended. "git" learned to ignore SIGINT and SIGQUIT while
waiting for these subprocesses.
Expecting a reroll.
cf. <12c956ea-330d-4441-937f-7885ab519e26@gmail.com>
source: <pull.1581.git.1694080982621.gitgitgadget@gmail.com>
* cw/git-std-lib (2023-09-11) 7 commits
- SQUASH???
- git-std-lib: add test file to call git-std-lib.a functions
- git-std-lib: introduce git standard library
- parse: create new library for parsing strings and env values
- config: correct bad boolean env value error message
- wrapper: remove dependency to Git-specific internal file
- hex-ll: split out functionality from hex
Another libification effort.
Needs more work.
cf. <xmqqy1hfrk6p.fsf@gitster.g>
cf. <20230915183927.1597414-1-jonathantanmy@google.com>
source: <20230908174134.1026823-1-calvinwan@google.com>
* cc/git-replay (2023-09-07) 15 commits
- replay: stop assuming replayed branches do not diverge
- replay: add --contained to rebase contained branches
- replay: add --advance or 'cherry-pick' mode
- replay: disallow revision specific options and pathspecs
- replay: use standard revision ranges
- replay: make it a minimal server side command
- replay: remove HEAD related sanity check
- replay: remove progress and info output
- replay: add an important FIXME comment about gpg signing
- replay: don't simplify history
- replay: introduce pick_regular_commit()
- replay: die() instead of failing assert()
- replay: start using parse_options API
- replay: introduce new builtin
- t6429: remove switching aspects of fast-rebase
Waiting for review response.
cf. <52277471-4ddd-b2e0-62ca-c2a5b59ae418@gmx.de>
cf. <58daa706-7efb-51dd-9061-202ef650b96a@gmx.de>
cf. <f0e75d47-c277-9fbb-7bcd-53e4e5686f3c@gmx.de>
May want to wait until tb/repack-existing-packs-cleanup stabilizes.
source: <20230907092521.733746-1-christian.couder@gmail.com>
* la/trailer-test-and-doc-updates (2023-09-07) 13 commits
- trailer doc: <token> is a <key> or <keyAlias>, not both
- trailer doc: separator within key suppresses default separator
- trailer doc: emphasize the effect of configuration variables
- trailer --unfold help: prefer "reformat" over "join"
- trailer --parse docs: add explanation for its usefulness
- trailer --only-input: prefer "configuration variables" over "rules"
- trailer --parse help: expose aliased options
- trailer --no-divider help: describe usual "---" meaning
- trailer: trailer location is a place, not an action
- trailer doc: narrow down scope of --where and related flags
- trailer: add tests to check defaulting behavior with --no-* flags
- trailer test description: this tests --where=after, not --where=before
- trailer tests: make test cases self-contained
Test coverage for trailers has been improved.
source: <pull.1564.v3.git.1694125209.gitgitgadget@gmail.com>
* js/doc-unit-tests (2023-08-17) 3 commits
- ci: run unit tests in CI
- unit tests: add TAP unit test framework
- unit tests: Add a project plan document
(this branch is used by js/doc-unit-tests-with-cmake.)
Process to add some form of low-level unit tests has started.
Waiting for review response.
cf. <xmqq350hw6n7.fsf@gitster.g>
source: <cover.1692297001.git.steadmon@google.com>
* js/doc-unit-tests-with-cmake (2023-09-18) 7 commits
- cmake: handle also unit tests
- cmake: use test names instead of full paths
- cmake: fix typo in variable name
- artifacts-tar: when including `.dll` files, don't forget the unit-tests
- unit-tests: do show relative file paths
- unit-tests: do not mistake `.pdb` files for being executable
- cmake: also build unit tests
(this branch uses js/doc-unit-tests.)
Update the base topic to work with CMake builds.
Waiting for the base topic to settle.
source: <pull.1579.v2.git.1695070468.gitgitgadget@gmail.com>
* tb/path-filter-fix (2023-08-30) 15 commits
- bloom: introduce `deinit_bloom_filters()`
- commit-graph: reuse existing Bloom filters where possible
- object.h: fix mis-aligned flag bits table
- commit-graph: drop unnecessary `graph_read_bloom_data_context`
- commit-graph.c: unconditionally load Bloom filters
- t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()`
- bloom: prepare to discard incompatible Bloom filters
- bloom: annotate filters with hash version
- commit-graph: new filter ver. that fixes murmur3
- repo-settings: introduce commitgraph.changedPathsVersion
- t4216: test changed path filters with high bit paths
- t/helper/test-read-graph: implement `bloom-filters` mode
- bloom.h: make `load_bloom_filter_from_graph()` public
- t/helper/test-read-graph.c: extract `dump_graph_info()`
- gitformat-commit-graph: describe version 2 of BDAT
The Bloom filter used for path limited history traversal was broken
on systems whose "char" is unsigned; update the implementation and
bump the format version to 2.
Needs more work.
cf. <20230830200218.GA5147@szeder.dev>
source: <cover.1693413637.git.jonathantanmy@google.com>
* js/config-parse (2023-08-23) 4 commits
- config-parse: split library out of config.[c|h]
- config.c: accept config_parse_options in git_config_from_stdin
- config: report config parse errors using cb
- config: split out config_parse_options
The parsing routines for the configuration files have been split
into a separate file.
Needs review.
source: <cover.1692827403.git.steadmon@google.com>
* jc/update-index-show-index-version (2023-09-12) 3 commits
(merged to 'next' on 2023-09-13 at b754554df8)
+ test-tool: retire "index-version"
+ update-index: add --show-index-version
+ update-index doc: v4 is OK with JGit and libgit2
"git update-index" learns "--show-index-version" to inspect
the index format version used by the on-disk index file.
Will merge to 'master'.
source: <20230912193235.776292-1-gitster@pobox.com>
* jc/rerere-cleanup (2023-08-25) 4 commits
- rerere: modernize use of empty strbuf
- rerere: try_merge() should use LL_MERGE_ERROR when it means an error
- rerere: fix comment on handle_file() helper
- rerere: simplify check_one_conflict() helper function
(this branch uses jc/unresolve-removal.)
Code clean-up.
Not ready to be reviewed yet.
source: <20230731224409.4181277-1-gitster@pobox.com>
* jc/unresolve-removal (2023-07-31) 7 commits
- checkout: allow "checkout -m path" to unmerge removed paths
- checkout/restore: add basic tests for --merge
- checkout/restore: refuse unmerging paths unless checking out of the index
- update-index: remove stale fallback code for "--unresolve"
- update-index: use unmerge_index_entry() to support removal
- resolve-undo: allow resurrecting conflicted state that resolved to deletion
- update-index: do not read HEAD and MERGE_HEAD unconditionally
(this branch is used by jc/rerere-cleanup.)
"checkout --merge -- path" and "update-index --unresolve path" did
not resurrect conflicted state that was resolved to remove path,
but now they do.
Needs review.
source: <20230731224409.4181277-1-gitster@pobox.com>
* rj/status-bisect-while-rebase (2023-08-01) 1 commit
- status: fix branch shown when not only bisecting
"git status" is taught to show both the branch being bisected and
being rebased when both are in effect at the same time.
Needs review.
cf. <xmqqtttia3vn.fsf@gitster.g>
source: <48745298-f12b-8efb-4e48-90d2c22a8349@gmail.com>
^ permalink raw reply
* [PATCH] bulk-checkin: Only accept blobs
From: Eric W. Biederman @ 2023-09-20 0:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, brian m. carlson
In-Reply-To: <87a5thrak6.fsf@email.froward.int.ebiederm.org>
Junio C Hamano <gitster@pobox.com> writes:
> "Eric W. Biederman" <ebiederm@xmission.com> writes:
>
>> Subject: Re: [PATCH] bulk-checkin: Only accept blobs
>
> s/Only/only/;
>
>> As the code is written today bulk_checkin only accepts blobs. When
>> dealing with multiple hash algorithms it is necessary to distinguish
>> between blobs and object types that have embedded oids. For object
>> that embed oids a completely new object needs to be generated to
>> compute the compatibility hash on. For blobs however all that is
>> needed is to compute the compatibility hash on the same blob as the
>> storage hash.
>
> All of the above is understandable, but ...
>
>> As the code will need the compatiblity hash from a bulk checkin, remove
>> support for a bulk checkin of anything except blobs.
>
> ... I am not sure what the first half of this sentence is saying.
I mean the code will need to compute both the SHA-1 and the SHA-256
oid of the object being checked in.
What I left implied is that for blobs this is straight forward because
the object header and content are the same in both cases, and we just
need calls from the compatible hash algorithm to allow computing both
SHA-1 and SHA-256 simultaneously.
For commits, trees, and tags the entire objects must be walked before
the size of the compatible object is known. The size of the compatible
object appears in the compatible object header. Which means that for
commits, trees, and tags their compatible object must be computed
before their compatible hash can be computed.
Computing all of that in two passes is not what streaming interface
is about.
> Do you mean something like:
>
> The function takes "enum object_type" and pretends that it could
> stream trees and commits, if we wanted to interoperate with
> multiple hash algorithms, there are a lot more information than
> just the contents and object type needed. A tree object needs
> "compatibility hash" (i.e. the hash computed for the other hash
> functions) for objects the tree contains, a commit object
> likewise needs "compatibility hash" for its tree and its parent
> commits. IOW, the existing interface into the functions this
> patch touches is way too narrow to be useful for object types
> other than blobs.
>
> perhaps? I agree with the conclusion that the functions on the
> callchain involved in the bulk-checkin feature can safely be
> limited to handle blobs in the current code (and I have my reasons
> to think why it is not regressing the interface), but it makes me
> feel uneasy that I am not quite sure what your reasoning is.
The interface is wide enough, as we can safely assume that all
objects a commit, tag, or tree would refer to already have both
their SHA-1 and SHA-256 oid computed.
The problem is that it would take a 2 passes over a tree, commit, or tag
object to compute both it's SHA-1 oid and it's SHA-256 oid.
>>
>> Inspired-by: brian m. carlson <sandals@crustytoothpaste.net>
>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> ---
>>
>> Apologies to anyone who has seen this before. The last time I sent
>> this patch out it seems to have disappeared into a black hole so
>> I am trying again.
>
> I do not know if vger is dropping your messages, but I haven't seen
> this on https://lore.kernel.org/git/ archive, so I'll quote from the
> message I am responding to everything without omitting anything, to
> let others who may find out about this patch via my response.
I don't see the messages at vger either. So I am going to try this
conversation from my gmail address and see how well that works.
Eric
^ permalink raw reply
* Re: [PATCH 1/2] transport-helper: no connection restriction in connect_helper
From: Jiang Xin @ 2023-09-20 0:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Ilari Liusvaara, Brandon Williams
In-Reply-To: <xmqqy1h2f5dv.fsf@gitster.g>
On Wed, Sep 20, 2023 at 1:19 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Jiang Xin <worldhello.net@gmail.com> writes:
>
> > From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> >
> > For protocol-v2, "stateless-connection" can be used to establish a
> > stateless connection between the client and the server, but trying to
> > establish http connection by calling "transport->vtable->connect" will
> > fail. This restriction was first introduced in commit b236752a87
> > (Support remote archive from all smart transports, 2009-12-09) by
> > adding a limitation in the "connect_helper()" function.
>
> The above description may not be technically wrong per-se, but I
> found it confusing. The ".connect method must be defined" you are
> removing was added back when there was no "stateless" variant of the
> connection initiation. Many codepaths added by that patch did "if
> .connect is there, call it, but otherwise die()" and I think the
> code you were removing was added as a safety valve, not a limitation
> or restriction. Later, process_connect_service() learned to handle
> the .stateless_connect bit as a fallback for transports without
> .connect method defined, and the commit added that feature, edc9caf7
> (transport-helper: introduce stateless-connect, 2018-03-15), forgot
> that the caller did not allow this fallback.
>
> When b236752a (Support remote archive from all smart
> transports, 2009-12-09) added "remote archive" support for
> "smart transports", it was for transport that supports the
> .connect method. connect_helper() function protected itself
> from getting called for a transport without the method
> before calling process_connect_service(), which did not work
> wuth such a transport.
>
> Later, edc9caf7 (transport-helper: introduce
> stateless-connect, 2018-03-15) added a way for a transport
> without the .connect method to establish a "stateless"
> connection in protocol-v2, process_connect_service() was
> taught to handle the "stateless" connection, making the old
> safety valve in its caller that insisted that .connect
> method must be defined too strict, and forgot to loosen it.
>
> or something along that line would have been easire to follow, at
> least to me.
>
These explanations are very clear and helpful, thank you.
--
Jiang Xin
^ permalink raw reply
* Re: [PATCH v3 13/13] trailer doc: <token> is a <key> or <keyAlias>, not both
From: Jonathan Tan @ 2023-09-19 22:59 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: Jonathan Tan, git, Christian Couder, Linus Arver
In-Reply-To: <0b9525db5a0787fdc71834abad9151aa03acc497.1694125210.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> @@ -248,34 +258,40 @@ With `add`, a new trailer will be added.
> +
> With `doNothing`, nothing will be done.
>
> -trailer.<token>.key::
> - This `key` will be used instead of <token> in the trailer. At
> - the end of this key, a separator can appear and then some
> - space characters. By default the only valid separator is ':',
> - but this can be changed using the `trailer.separators` config
> - variable.
> +trailer.<keyAlias>.key::
> + Defines a <keyAlias> for the <key>. The <keyAlias> must be a
> + prefix (case does not matter) of the <key>. For example, in `git
> + config trailer.ack.key "Acked-by"` the "Acked-by" is the <key> and
> + the "ack" is the <keyAlias>. This configuration allows the shorter
> + `--trailer "ack:..."` invocation on the command line using the "ack"
> + <keyAlias> instead of the longer `--trailer "Acked-by:..."`.
> ++
> +At the end of the <key>, a separator can appear and then some
> +space characters. By default the only valid separator is ':',
> +but this can be changed using the `trailer.separators` config
> +variable.
I think all the other patches will be a great help to the user, but I'm
on the fence about this one. Someone who knows these trailer components
by their old names might be confused upon seeing tne new ones, so I'm
inclined to minimize such changes. I do think that the new names make
more sense, though.
The documentation doesn't seem to say what happens when trailer.ack.cmd
and trailer.Acked-by.cmd (replace "cmd" with whatever) are defined, but
that was true previously too (and knowing this does not really enable
the user to be able to do something they previously couldn't, so this
is fine).
^ permalink raw reply
* Re: [PATCH] pkt-line: do not chomp EOL for sideband progress info
From: Junio C Hamano @ 2023-09-19 22:38 UTC (permalink / raw)
To: Jiang Xin; +Cc: Git List, Jonathan Tan, Jiang Xin
In-Reply-To: <20230919071956.14015-1-worldhello.net@gmail.com>
Jiang Xin <worldhello.net@gmail.com> writes:
> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
Who knows packet_reader interface well? Jonathan?
Thanks.
> In the protocol negotiation stage, we need to turn on the flag
> "PACKET_READ_CHOMP_NEWLINE" to chomp EOL for each packet line from
> client or server. But when receiving data and progress information
> using sideband, we will turn off the flag "PACKET_READ_CHOMP_NEWLINE"
> to prevent mangling EOLs from data and progress information.
>
> When both the server and the client support "sideband-all" capability,
> we have a dilemma that EOLs in negotiation packets should be trimmed,
> but EOLs in progress infomation should be leaved as is.
>
> Move the logic of chomping EOLs from "packet_read_with_status()" to
> "packet_reader_read()" can resolve this dilemma.
>
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
> pkt-line.c | 19 ++++++++++++++++---
> 1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/pkt-line.c b/pkt-line.c
> index af83a19f4d..d6d08b6aa6 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -597,12 +597,18 @@ void packet_reader_init(struct packet_reader *reader, int fd,
> enum packet_read_status packet_reader_read(struct packet_reader *reader)
> {
> struct strbuf scratch = STRBUF_INIT;
> + int options = reader->options;
>
> if (reader->line_peeked) {
> reader->line_peeked = 0;
> return reader->status;
> }
>
> + /* Do not chomp newlines for sideband progress and error messages */
> + if (reader->use_sideband && options & PACKET_READ_CHOMP_NEWLINE) {
> + options &= ~PACKET_READ_CHOMP_NEWLINE;
> + }
> +
> /*
> * Consume all progress packets until a primary payload packet is
> * received
> @@ -615,7 +621,7 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
> reader->buffer,
> reader->buffer_size,
> &reader->pktlen,
> - reader->options);
> + options);
> if (!reader->use_sideband)
> break;
> if (demultiplex_sideband(reader->me, reader->status,
> @@ -624,12 +630,19 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
> break;
> }
>
> - if (reader->status == PACKET_READ_NORMAL)
> + if (reader->status == PACKET_READ_NORMAL) {
> /* Skip the sideband designator if sideband is used */
> reader->line = reader->use_sideband ?
> reader->buffer + 1 : reader->buffer;
> - else
> +
> + if ((reader->options & PACKET_READ_CHOMP_NEWLINE) &&
> + reader->buffer[reader->pktlen - 1] == '\n') {
> + reader->buffer[reader->pktlen - 1] = 0;
> + reader->pktlen--;
> + }
> + } else {
> reader->line = NULL;
> + }
>
> return reader->status;
> }
^ permalink raw reply
* Re: [PATCH v3 07/13] trailer --parse help: expose aliased options
From: Jonathan Tan @ 2023-09-19 22:16 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: Jonathan Tan, git, Christian Couder, Linus Arver
In-Reply-To: <d1780a0127a749902d60249b531869ee9cd1a5f6.1694125210.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
> index a7623dbfb2e..5f3e1a38eee 100644
> --- a/builtin/interpret-trailers.c
> +++ b/builtin/interpret-trailers.c
> @@ -107,7 +107,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
> OPT_BOOL(0, "only-trailers", &opts.only_trailers, N_("output only the trailers")),
> OPT_BOOL(0, "only-input", &opts.only_input, N_("do not apply config rules")),
> OPT_BOOL(0, "unfold", &opts.unfold, N_("join whitespace-continued values")),
> - OPT_CALLBACK_F(0, "parse", &opts, NULL, N_("set parsing options"),
> + OPT_CALLBACK_F(0, "parse", &opts, NULL, N_("alias for --only-trailers --only-input --unfold"),
> PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_parse),
> OPT_BOOL(0, "no-divider", &opts.no_divider, N_("do not treat \"---\" as the end of input")),
> OPT_CALLBACK(0, "trailer", &trailers, N_("trailer"),
Indeed, parse_opt_parse() sets the 3 options described in the commit
message. Looks good.
^ permalink raw reply
* Re: [PATCH] bulk-checkin: Only accept blobs
From: Junio C Hamano @ 2023-09-19 22:15 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: git, brian m. carlson
In-Reply-To: <87fs39vppo.fsf@email.froward.int.ebiederm.org>
"Eric W. Biederman" <ebiederm@xmission.com> writes:
> Subject: Re: [PATCH] bulk-checkin: Only accept blobs
s/Only/only/;
> As the code is written today bulk_checkin only accepts blobs. When
> dealing with multiple hash algorithms it is necessary to distinguish
> between blobs and object types that have embedded oids. For object
> that embed oids a completely new object needs to be generated to
> compute the compatibility hash on. For blobs however all that is
> needed is to compute the compatibility hash on the same blob as the
> storage hash.
All of the above is understandable, but ...
> As the code will need the compatiblity hash from a bulk checkin, remove
> support for a bulk checkin of anything except blobs.
... I am not sure what the first half of this sentence is saying.
Do you mean something like:
The function takes "enum object_type" and pretends that it could
stream trees and commits, if we wanted to interoperate with
multiple hash algorithms, there are a lot more information than
just the contents and object type needed. A tree object needs
"compatibility hash" (i.e. the hash computed for the other hash
functions) for objects the tree contains, a commit object
likewise needs "compatibility hash" for its tree and its parent
commits. IOW, the existing interface into the functions this
patch touches is way too narrow to be useful for object types
other than blobs.
perhaps? I agree with the conclusion that the functions on the
callchain involved in the bulk-checkin feature can safely be
limited to handle blobs in the current code (and I have my reasons
to think why it is not regressing the interface), but it makes me
feel uneasy that I am not quite sure what your reasoning is.
>
> Inspired-by: brian m. carlson <sandals@crustytoothpaste.net>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>
> Apologies to anyone who has seen this before. The last time I sent
> this patch out it seems to have disappeared into a black hole so
> I am trying again.
I do not know if vger is dropping your messages, but I haven't seen
this on https://lore.kernel.org/git/ archive, so I'll quote from the
message I am responding to everything without omitting anything, to
let others who may find out about this patch via my response.
Thanks.
> bulk-checkin.c | 35 +++++++++++++++++------------------
> bulk-checkin.h | 6 +++---
> object-file.c | 12 ++++++------
> 3 files changed, 26 insertions(+), 27 deletions(-)
>
> diff --git a/bulk-checkin.c b/bulk-checkin.c
> index 73bff3a23d27..223562b4e748 100644
> --- a/bulk-checkin.c
> +++ b/bulk-checkin.c
> @@ -155,10 +155,10 @@ static int already_written(struct bulk_checkin_packfile *state, struct object_id
> * status before calling us just in case we ask it to call us again
> * with a new pack.
> */
> -static int stream_to_pack(struct bulk_checkin_packfile *state,
> - git_hash_ctx *ctx, off_t *already_hashed_to,
> - int fd, size_t size, enum object_type type,
> - const char *path, unsigned flags)
> +static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
> + git_hash_ctx *ctx, off_t *already_hashed_to,
> + int fd, size_t size, const char *path,
> + unsigned flags)
> {
> git_zstream s;
> unsigned char ibuf[16384];
> @@ -170,7 +170,7 @@ static int stream_to_pack(struct bulk_checkin_packfile *state,
>
> git_deflate_init(&s, pack_compression_level);
>
> - hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), type, size);
> + hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), OBJ_BLOB, size);
> s.next_out = obuf + hdrlen;
> s.avail_out = sizeof(obuf) - hdrlen;
>
> @@ -247,11 +247,10 @@ static void prepare_to_stream(struct bulk_checkin_packfile *state,
> die_errno("unable to write pack header");
> }
>
> -static int deflate_to_pack(struct bulk_checkin_packfile *state,
> - struct object_id *result_oid,
> - int fd, size_t size,
> - enum object_type type, const char *path,
> - unsigned flags)
> +static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
> + struct object_id *result_oid,
> + int fd, size_t size,
> + const char *path, unsigned flags)
> {
> off_t seekback, already_hashed_to;
> git_hash_ctx ctx;
> @@ -265,7 +264,7 @@ static int deflate_to_pack(struct bulk_checkin_packfile *state,
> return error("cannot find the current offset");
>
> header_len = format_object_header((char *)obuf, sizeof(obuf),
> - type, size);
> + OBJ_BLOB, size);
> the_hash_algo->init_fn(&ctx);
> the_hash_algo->update_fn(&ctx, obuf, header_len);
>
> @@ -282,8 +281,8 @@ static int deflate_to_pack(struct bulk_checkin_packfile *state,
> idx->offset = state->offset;
> crc32_begin(state->f);
> }
> - if (!stream_to_pack(state, &ctx, &already_hashed_to,
> - fd, size, type, path, flags))
> + if (!stream_blob_to_pack(state, &ctx, &already_hashed_to,
> + fd, size, path, flags))
> break;
> /*
> * Writing this object to the current pack will make
> @@ -350,12 +349,12 @@ void fsync_loose_object_bulk_checkin(int fd, const char *filename)
> }
> }
>
> -int index_bulk_checkin(struct object_id *oid,
> - int fd, size_t size, enum object_type type,
> - const char *path, unsigned flags)
> +int index_blob_bulk_checkin(struct object_id *oid,
> + int fd, size_t size,
> + const char *path, unsigned flags)
> {
> - int status = deflate_to_pack(&bulk_checkin_packfile, oid, fd, size, type,
> - path, flags);
> + int status = deflate_blob_to_pack(&bulk_checkin_packfile, oid, fd, size,
> + path, flags);
> if (!odb_transaction_nesting)
> flush_bulk_checkin_packfile(&bulk_checkin_packfile);
> return status;
> diff --git a/bulk-checkin.h b/bulk-checkin.h
> index 48fe9a6e9171..aa7286a7b3e1 100644
> --- a/bulk-checkin.h
> +++ b/bulk-checkin.h
> @@ -9,9 +9,9 @@
> void prepare_loose_object_bulk_checkin(void);
> void fsync_loose_object_bulk_checkin(int fd, const char *filename);
>
> -int index_bulk_checkin(struct object_id *oid,
> - int fd, size_t size, enum object_type type,
> - const char *path, unsigned flags);
> +int index_blob_bulk_checkin(struct object_id *oid,
> + int fd, size_t size,
> + const char *path, unsigned flags);
>
> /*
> * Tell the object database to optimize for adding
> diff --git a/object-file.c b/object-file.c
> index 7dc0c4bfbba8..7c7afe579364 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -2446,11 +2446,11 @@ static int index_core(struct index_state *istate,
> * binary blobs, they generally do not want to get any conversion, and
> * callers should avoid this code path when filters are requested.
> */
> -static int index_stream(struct object_id *oid, int fd, size_t size,
> - enum object_type type, const char *path,
> - unsigned flags)
> +static int index_blob_stream(struct object_id *oid, int fd, size_t size,
> + const char *path,
> + unsigned flags)
> {
> - return index_bulk_checkin(oid, fd, size, type, path, flags);
> + return index_blob_bulk_checkin(oid, fd, size, path, flags);
> }
>
> int index_fd(struct index_state *istate, struct object_id *oid,
> @@ -2472,8 +2472,8 @@ int index_fd(struct index_state *istate, struct object_id *oid,
> ret = index_core(istate, oid, fd, xsize_t(st->st_size),
> type, path, flags);
> else
> - ret = index_stream(oid, fd, xsize_t(st->st_size), type, path,
> - flags);
> + ret = index_blob_stream(oid, fd, xsize_t(st->st_size), path,
> + flags);
> close(fd);
> return ret;
> }
^ permalink raw reply
* Re: [PATCH v3 05/13] trailer: trailer location is a place, not an action
From: Jonathan Tan @ 2023-09-19 22:13 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: Jonathan Tan, git, Christian Couder, Linus Arver
In-Reply-To: <040766861e21afe5f686299560677e429be11844.1694125210.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Linus Arver <linusa@google.com>
>
> Fix the help text to say "placement" instead of "action" because the
> values are placements, not actions.
>
> While we're at it, tweak the documentation to say "placements" instead
> of "values", similar to how the existing language for "--if-exists" uses
> the word "action" to describe both the syntax (with the phrase
> "--if-exists <action>") and the possible values (with the phrase
> "possible actions").
>
> Signed-off-by: Linus Arver <linusa@google.com>
> ---
> Documentation/git-interpret-trailers.txt | 2 +-
> builtin/interpret-trailers.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
> index 72f5bdb652f..b5284c3d33f 100644
> --- a/Documentation/git-interpret-trailers.txt
> +++ b/Documentation/git-interpret-trailers.txt
> @@ -117,7 +117,7 @@ OPTIONS
> and applies to all '--trailer' options until the next occurrence of
> '--where' or '--no-where'. Upon encountering '--no-where', clear the
> effect of any previous use of '--where', such that the relevant configuration
> - variables are no longer overridden. Possible values are `after`,
> + variables are no longer overridden. Possible placements are `after`,
> `before`, `end` or `start`.
>
> --if-exists <action>::
Not shown in the diff as printed in this email, but this option is
indeed documented with "<placement>". Up to and including this patch
makes sense.
^ permalink raw reply
* Re: What's cooking in git.git (Sep 2023, #05; Fri, 15)
From: Junio C Hamano @ 2023-09-19 21:25 UTC (permalink / raw)
To: Linus Arver; +Cc: git
In-Reply-To: <owly7comj5ll.fsf@fine.c.googlers.com>
Linus Arver <linusa@google.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> [Cooking]
>>
>> [...]
>>
>> * la/trailer-cleanups (2023-09-11) 6 commits
>> (merged to 'next' on 2023-09-12 at 779c4a097a)
>> + trailer: use offsets for trailer_start/trailer_end
>> + trailer: rename *_DEFAULT enums to *_UNSPECIFIED
>> + trailer: teach find_patch_start about --no-divider
>> + trailer: split process_command_line_args into separate functions
>> + trailer: split process_input_file into separate pieces
>> + trailer: separate public from internal portion of trailer_iterator
>>
>> Code clean-up.
>>
>> Will merge to 'master'.
>> source: <pull.1563.v2.git.1694240177.gitgitgadget@gmail.com>
>
> This isn't ready yet (still need to reroll).
Whoa, wait. I wasn't aware of any more comments that needed
addressing. Whatever improvements you have in mind, if they are
minor, letting the above graduate (they have been in 'next' for a
week without anybody complaining) and doing them as a follow-up
series would be sensible, I would think.
Thanks.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox