* [PATCH 3/8] fsmonitor: mark some maybe-unused parameters
From: Jeff King @ 2023-09-18 22:31 UTC (permalink / raw)
To: git; +Cc: Jeff Hostetler, Eric DeCosta
In-Reply-To: <20230918222908.GA2659096@coredump.intra.peff.net>
There's a bit of conditionally-compiled code in fsmonitor, so some
function parameters may be unused depending on the build options:
- in fsmonitor--daemon.c's try_to_run_foreground_daemon(), we take a
detach_console argument, but it's only used on Windows. This seems
intentional (and not mistakenly missing other platforms) based on
the discussion in c284e27ba7 (fsmonitor--daemon: implement 'start'
command, 2022-03-25), which introduced it.
- in fsmonitor-setting.c's check_for_incompatible(), we pass the "ipc"
flag down to the system-specific fsm_os__incompatible() helper. But
we can only do so if our platform has such a helper.
In both cases we can mark the argument as MAYBE_UNUSED. That annotates
it enough to suppress the compiler's -Wunused-parameter warning, but
without making it impossible to use the variable, as a regular UNUSED
annotation would.
Signed-off-by: Jeff King <peff@peff.net>
---
For a similar case in 2c3c3d88fc (imap-send: mark unused parameters with
NO_OPENSSL, 2023-08-29), I used the old:
(void)some_parameter_that_might_not_be_used;
trick. But I realized while writing this one that MAYBE_UNUSED fits the
bill a little more nicely, and I don't see any reason we would have
portability problems with it.
builtin/fsmonitor--daemon.c | 2 +-
fsmonitor-settings.c | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
index 7e99c4d61b..7c4130c981 100644
--- a/builtin/fsmonitor--daemon.c
+++ b/builtin/fsmonitor--daemon.c
@@ -1412,7 +1412,7 @@ static int fsmonitor_run_daemon(void)
return err;
}
-static int try_to_run_foreground_daemon(int detach_console)
+static int try_to_run_foreground_daemon(int detach_console MAYBE_UNUSED)
{
/*
* Technically, we don't need to probe for an existing daemon
diff --git a/fsmonitor-settings.c b/fsmonitor-settings.c
index b62acf44ae..a6a9e6bc19 100644
--- a/fsmonitor-settings.c
+++ b/fsmonitor-settings.c
@@ -62,7 +62,8 @@ static enum fsmonitor_reason check_remote(struct repository *r)
}
#endif
-static enum fsmonitor_reason check_for_incompatible(struct repository *r, int ipc)
+static enum fsmonitor_reason check_for_incompatible(struct repository *r,
+ int ipc MAYBE_UNUSED)
{
if (!r->worktree) {
/*
--
2.42.0.671.g43fbf3903a
^ permalink raw reply related
* [PATCH 2/8] fsmonitor/win32: drop unused parameters
From: Jeff King @ 2023-09-18 22:30 UTC (permalink / raw)
To: git; +Cc: Jeff Hostetler, Eric DeCosta
In-Reply-To: <20230918222908.GA2659096@coredump.intra.peff.net>
A few helper functions (centered around file-watch events) take extra
fsmonitor state parameters that they don't use. These are static helpers
local to the win32 implementation, and don't need to conform to any
particular interface. We can just drop the extra parameters, which
simplifies the code and silences -Wunused-parameter.
Signed-off-by: Jeff King <peff@peff.net>
---
compat/fsmonitor/fsm-listen-win32.c | 24 ++++++++++--------------
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/compat/fsmonitor/fsm-listen-win32.c b/compat/fsmonitor/fsm-listen-win32.c
index a361a7db20..90a2412284 100644
--- a/compat/fsmonitor/fsm-listen-win32.c
+++ b/compat/fsmonitor/fsm-listen-win32.c
@@ -289,8 +289,7 @@ void fsm_listen__stop_async(struct fsmonitor_daemon_state *state)
SetEvent(state->listen_data->hListener[LISTENER_SHUTDOWN]);
}
-static struct one_watch *create_watch(struct fsmonitor_daemon_state *state,
- const char *path)
+static struct one_watch *create_watch(const char *path)
{
struct one_watch *watch = NULL;
DWORD desired_access = FILE_LIST_DIRECTORY;
@@ -361,8 +360,7 @@ static void destroy_watch(struct one_watch *watch)
free(watch);
}
-static int start_rdcw_watch(struct fsm_listen_data *data,
- struct one_watch *watch)
+static int start_rdcw_watch(struct one_watch *watch)
{
DWORD dwNotifyFilter =
FILE_NOTIFY_CHANGE_FILE_NAME |
@@ -735,11 +733,11 @@ void fsm_listen__loop(struct fsmonitor_daemon_state *state)
state->listen_error_code = 0;
- if (start_rdcw_watch(data, data->watch_worktree) == -1)
+ if (start_rdcw_watch(data->watch_worktree) == -1)
goto force_error_stop;
if (data->watch_gitdir &&
- start_rdcw_watch(data, data->watch_gitdir) == -1)
+ start_rdcw_watch(data->watch_gitdir) == -1)
goto force_error_stop;
for (;;) {
@@ -755,15 +753,15 @@ void fsm_listen__loop(struct fsmonitor_daemon_state *state)
}
if (result == -2) {
/* retryable error */
- if (start_rdcw_watch(data, data->watch_worktree) == -1)
+ if (start_rdcw_watch(data->watch_worktree) == -1)
goto force_error_stop;
continue;
}
/* have data */
if (process_worktree_events(state) == LISTENER_SHUTDOWN)
goto force_shutdown;
- if (start_rdcw_watch(data, data->watch_worktree) == -1)
+ if (start_rdcw_watch(data->watch_worktree) == -1)
goto force_error_stop;
continue;
}
@@ -776,15 +774,15 @@ void fsm_listen__loop(struct fsmonitor_daemon_state *state)
}
if (result == -2) {
/* retryable error */
- if (start_rdcw_watch(data, data->watch_gitdir) == -1)
+ if (start_rdcw_watch(data->watch_gitdir) == -1)
goto force_error_stop;
continue;
}
/* have data */
if (process_gitdir_events(state) == LISTENER_SHUTDOWN)
goto force_shutdown;
- if (start_rdcw_watch(data, data->watch_gitdir) == -1)
+ if (start_rdcw_watch(data->watch_gitdir) == -1)
goto force_error_stop;
continue;
}
@@ -821,16 +819,14 @@ int fsm_listen__ctor(struct fsmonitor_daemon_state *state)
data->hEventShutdown = CreateEvent(NULL, TRUE, FALSE, NULL);
- data->watch_worktree = create_watch(state,
- state->path_worktree_watch.buf);
+ data->watch_worktree = create_watch(state->path_worktree_watch.buf);
if (!data->watch_worktree)
goto failed;
check_for_shortnames(data->watch_worktree);
if (state->nr_paths_watching > 1) {
- data->watch_gitdir = create_watch(state,
- state->path_gitdir_watch.buf);
+ data->watch_gitdir = create_watch(state->path_gitdir_watch.buf);
if (!data->watch_gitdir)
goto failed;
}
--
2.42.0.671.g43fbf3903a
^ permalink raw reply related
* [PATCH 1/8] fsmonitor: prefer repo_git_path() to git_pathdup()
From: Jeff King @ 2023-09-18 22:29 UTC (permalink / raw)
To: git; +Cc: Jeff Hostetler, Eric DeCosta
In-Reply-To: <20230918222908.GA2659096@coredump.intra.peff.net>
The fsmonitor_ipc__get_path() function ignores its repository argument.
It should use it when constructing repo paths (though in practice, it is
unlikely anything but the_repository is ever passed, so this is cleanup
and future proofing, not a bug fix).
Note that despite the lack of "dup" in the name, repo_git_path() behaves
like git_pathdup() and returns an allocated string.
Signed-off-by: Jeff King <peff@peff.net>
---
compat/fsmonitor/fsm-ipc-win32.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/compat/fsmonitor/fsm-ipc-win32.c b/compat/fsmonitor/fsm-ipc-win32.c
index 8928fa93ce..41984ea48e 100644
--- a/compat/fsmonitor/fsm-ipc-win32.c
+++ b/compat/fsmonitor/fsm-ipc-win32.c
@@ -6,6 +6,6 @@
const char *fsmonitor_ipc__get_path(struct repository *r) {
static char *ret;
if (!ret)
- ret = git_pathdup("fsmonitor--daemon.ipc");
+ ret = repo_git_path(r, "fsmonitor--daemon.ipc");
return ret;
}
--
2.42.0.671.g43fbf3903a
^ permalink raw reply related
* [PATCH 0/8] fsmonitor unused parameter cleanups
From: Jeff King @ 2023-09-18 22:29 UTC (permalink / raw)
To: git; +Cc: Jeff Hostetler, Eric DeCosta
Here are a few cleanups of the fsmonitor code to remove or annotate
unused parameters (working towards my goal of making us compile clean
with -Wunused-parameter). I think they should all be pretty
non-controversial, but I'm cc-ing folks active in the area in case patch
2 steps on the toes of any unpublished works in progress.
[1/8]: fsmonitor: prefer repo_git_path() to git_pathdup()
[2/8]: fsmonitor/win32: drop unused parameters
[3/8]: fsmonitor: mark some maybe-unused parameters
[4/8]: fsmonitor/win32: mark unused parameter in fsm_os__incompatible()
[5/8]: fsmonitor: mark unused parameters in stub functions
[6/8]: fsmonitor/darwin: mark unused parameters in system callback
[7/8]: fsmonitor: mark unused hashmap callback parameters
[8/8]: run-command: mark unused parameters in start_bg_wait callbacks
builtin/fsmonitor--daemon.c | 10 ++++++----
compat/fsmonitor/fsm-health-darwin.c | 8 ++++----
compat/fsmonitor/fsm-ipc-win32.c | 2 +-
compat/fsmonitor/fsm-listen-darwin.c | 4 ++--
compat/fsmonitor/fsm-listen-win32.c | 24 ++++++++++--------------
compat/fsmonitor/fsm-path-utils-win32.c | 7 ++++---
compat/fsmonitor/fsm-settings-win32.c | 2 +-
fsmonitor-ipc.c | 10 +++++-----
fsmonitor-settings.c | 3 ++-
t/helper/test-simple-ipc.c | 3 ++-
10 files changed, 37 insertions(+), 36 deletions(-)
-Peff
^ permalink raw reply
* Re: [PATCH] git-send-email.perl: avoid printing undef when validating addresses
From: Jeff King @ 2023-09-18 21:20 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano, Bagas Sanjaya
In-Reply-To: <545729b619308c6f3397b9aa1747f26ddc58f461.1695054945.git.me@ttaylorr.com>
On Mon, Sep 18, 2023 at 12:35:53PM -0400, Taylor Blau wrote:
> When validating email addresses with `extract_valid_address_or_die()`,
> we print out a helpful error message when the given input does not
> contain a valid email address.
>
> However, the pre-image of this patch looks something like:
>
> my $address = shift;
> $address = extract_valid_address($address):
> die sprintf(__("..."), $address) if !$address;
>
> which fails when given a bogus email address by trying to use $address
> (which is undef) in a sprintf() expansion, like so:
>
> $ git.compile send-email --to="pi <pi@pi>" /tmp/x/*.patch --force
> Use of uninitialized value $address in sprintf at /home/ttaylorr/src/git/git-send-email line 1175.
> error: unable to extract a valid address from:
Yeah, we overwrite the variable we're reporting on, so I don't think the
original could possibly work. Your fix makes sense.
> This regression dates back to e431225569 (git-send-email: remove invalid
> addresses earlier, 2012-11-22), but became more noticeable in a8022c5f7b
> (send-email: expose header information to git-send-email's
> sendemail-validate hook, 2023-04-19), which validates SMTP headers in
> the sendemail-validate hook.
I didn't quite understand how a8022c5f7b made this worse, but I guess we
just call it the bad function in more instances. The bug is definitely
from e431225569, though.
-Peff
^ permalink raw reply
* Re: [PATCH 3/4] unit-tests: do show relative file paths
From: Johannes Schindelin @ 2023-09-18 21:00 UTC (permalink / raw)
To: phillip.wood; +Cc: Johannes Schindelin via GitGitGadget, git
In-Reply-To: <69f6f263-06e1-4fef-abd9-d6c03ae0c148@gmail.com>
Hi Phillip,
On Mon, 11 Sep 2023, Phillip Wood wrote:
> On 31/08/2023 07:15, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > Visual C interpolates `__FILE__` with the absolute _Windows_ path of
> > the source file. GCC interpolates it with the relative path, and the
> > tests even verify that.
>
> Oh, that's a pain
>
> > So let's make sure that the unit tests only emit such paths.
>
> Makes sense
>
> > +#ifndef _MSC_VER
> > +#define make_relative(location) location
> > +#else
> > +/*
> > + * Visual C interpolates the absolute Windows path for `__FILE__`,
> > + * but we want to see relative paths, as verified by t0080.
> > + */
> > +#include "strbuf.h"
> > +#include "dir.h"
> > +
> > +static const char *make_relative(const char *location)
> > +{
> > + static const char *prefix;
> > + static size_t prefix_len;
> > + static struct strbuf buf = STRBUF_INIT;
>
> So far test-lib.c avoids using things like struct strbuf that it will be used
> to test. In this instance we're only using it on one particular compiler so it
> may not matter so much. We could avoid it but I'm not sure it is worth the
> extra complexity. One thing I noted in this patch is that prefix is leaked but
> I'm not sure if you run any leak checkers on the msvc build.
I changed the code not to use a strbuf, and I'm now working exclusively on
static buffers instead of `malloc()`ing anything.
Thank you,
Johannes
^ permalink raw reply
* Re: [PATCH 1/4] cmake: also build unit tests
From: Johannes Schindelin @ 2023-09-18 20:58 UTC (permalink / raw)
To: phillip.wood; +Cc: Johannes Schindelin via GitGitGadget, git
In-Reply-To: <d7d1505c-8d65-4b64-8814-3e3b1e46f8ac@gmail.com>
Hi Phillip,
On Mon, 11 Sep 2023, Phillip Wood wrote:
> On 31/08/2023 07:15, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > A new, better way to run unit tests was just added to Git. This adds
> > support for building those unit tests via CMake.
>
> This patch builds the unit tests but does not add them to the list of tests
> run by CTest - how are the tests typically run on the CMake build?
You're right, I missed that the unit tests are run as part not of t0080,
but as a separate target in `t/Makefile`.
I've added a couple of patches to clean up the CTest part and then run the
unit test (t-strbuf, and whatever is added to the `UNIT_TEST_PROGRAMS`
variable in `Makefile`).
Thank you,
Johannes
^ permalink raw reply
* [PATCH v2 7/7] cmake: handle also unit tests
From: Johannes Schindelin via GitGitGadget @ 2023-09-18 20:54 UTC (permalink / raw)
To: git; +Cc: Phillip Wood, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1579.v2.git.1695070468.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The unit tests should also be available e.g. in Visual Studio's Test
Explorer when configuring Git's source code via CMake.
Suggested-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
contrib/buildsystems/CMakeLists.txt | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index ff1a8cc348f..35d451856a0 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -981,6 +981,17 @@ foreach(unit_test ${unit_test_PROGRAMS})
PROPERTIES RUNTIME_OUTPUT_DIRECTORY_RELEASE ${CMAKE_BINARY_DIR}/t/unit-tests)
endif()
list(APPEND PROGRAMS_BUILT "${unit_test}")
+
+ # t-basic intentionally fails tests, to validate the unit-test infrastructure.
+ # Therefore, it should only be run as part of t0080, which verifies that it
+ # fails only in the expected ways.
+ #
+ # All other unit tests should be run.
+ if(NOT ${unit_test} STREQUAL "t-basic")
+ add_test(NAME "unit-tests.${unit_test}"
+ COMMAND "./${unit_test}"
+ WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t/unit-tests)
+ endif()
endforeach()
#test-tool
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 6/7] cmake: use test names instead of full paths
From: Johannes Schindelin via GitGitGadget @ 2023-09-18 20:54 UTC (permalink / raw)
To: git; +Cc: Phillip Wood, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1579.v2.git.1695070468.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The primary purpose of Git's CMake definition is to allow developing Git
in Visual Studio. As part of that, the CTest feature allows running
individual test scripts conveniently in Visual Studio's Test Explorer.
However, this Test Explorer's design targets object-oriented languages
and therefore expects the test names in the form
`<namespace>.<class>.<testname>`. And since we specify the full path
of the test scripts instead, including the ugly `/.././t/` part, these
dots confuse the Test Explorer and it uses a large part of the path as
"namespace".
Let's just use `t.<name>` instead. This still adds an ugly "Empty
Namespace" layer by default, but at least the ugly absolute path is now
gone.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
contrib/buildsystems/CMakeLists.txt | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index ad197ea433f..ff1a8cc348f 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -1106,13 +1106,14 @@ file(GLOB test_scripts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
#test
foreach(tsh ${test_scripts})
- add_test(NAME ${tsh}
+ string(REGEX REPLACE ".*/(.*)\\.sh" "\\1" test_name ${tsh})
+ add_test(NAME "t.${test_name}"
COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
endforeach()
# This test script takes an extremely long time and is known to time out even
# on fast machines because it requires in excess of one hour to run
-set_tests_properties("${CMAKE_SOURCE_DIR}/t/t7112-reset-submodule.sh" PROPERTIES TIMEOUT 4000)
+set_tests_properties("t.t7112-reset-submodule" PROPERTIES TIMEOUT 4000)
endif()#BUILD_TESTING
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 5/7] cmake: fix typo in variable name
From: Johannes Schindelin via GitGitGadget @ 2023-09-18 20:54 UTC (permalink / raw)
To: git; +Cc: Phillip Wood, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1579.v2.git.1695070468.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
contrib/buildsystems/CMakeLists.txt | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 45016213358..ad197ea433f 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -1102,10 +1102,10 @@ if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
file(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-completion.bash DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)
endif()
-file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
+file(GLOB test_scripts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
#test
-foreach(tsh ${test_scipts})
+foreach(tsh ${test_scripts})
add_test(NAME ${tsh}
COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 4/7] artifacts-tar: when including `.dll` files, don't forget the unit-tests
From: Johannes Schindelin via GitGitGadget @ 2023-09-18 20:54 UTC (permalink / raw)
To: git; +Cc: Phillip Wood, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1579.v2.git.1695070468.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
As of recent, Git also builds executables in `t/unit-tests/`. For
technical reasons, when building with CMake and Visual C, the
dependencies (".dll files") need to be copied there, too, otherwise
running the executable will fail "due to missing dependencies".
The CMake definition already contains the directives to copy those
`.dll` files, but we also need to adjust the `artifacts-tar` rule in
the `Makefile` accordingly to let the `vs-test` job in the CI runs
pass successfully.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index 4016da6e39c..d95a7b19b50 100644
--- a/Makefile
+++ b/Makefile
@@ -3596,7 +3596,7 @@ rpm::
.PHONY: rpm
ifneq ($(INCLUDE_DLLS_IN_ARTIFACTS),)
-OTHER_PROGRAMS += $(shell echo *.dll t/helper/*.dll)
+OTHER_PROGRAMS += $(shell echo *.dll t/helper/*.dll t/unit-tests/*.dll)
endif
artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 3/7] unit-tests: do show relative file paths
From: Johannes Schindelin via GitGitGadget @ 2023-09-18 20:54 UTC (permalink / raw)
To: git; +Cc: Phillip Wood, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1579.v2.git.1695070468.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Visual C interpolates `__FILE__` with the absolute _Windows_ path of
the source file. GCC interpolates it with the relative path, and the
tests even verify that.
So let's make sure that the unit tests only emit such paths.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/unit-tests/test-lib.c | 52 +++++++++++++++++++++++++++++++++++++----
1 file changed, 48 insertions(+), 4 deletions(-)
diff --git a/t/unit-tests/test-lib.c b/t/unit-tests/test-lib.c
index 70030d587ff..744e223ee98 100644
--- a/t/unit-tests/test-lib.c
+++ b/t/unit-tests/test-lib.c
@@ -21,6 +21,46 @@ static struct {
.result = RESULT_NONE,
};
+#ifndef _MSC_VER
+#define make_relative(location) location
+#else
+/*
+ * Visual C interpolates the absolute Windows path for `__FILE__`,
+ * but we want to see relative paths, as verified by t0080.
+ */
+#include "dir.h"
+
+static const char *make_relative(const char *location)
+{
+ static char prefix[] = __FILE__, buf[PATH_MAX], *p;
+ static size_t prefix_len;
+
+ if (!prefix_len) {
+ size_t len = strlen(prefix);
+ const char *needle = "\\t\\unit-tests\\test-lib.c";
+ size_t needle_len = strlen(needle);
+
+ if (len < needle_len || strcmp(needle, prefix + len - needle_len))
+ die("unexpected suffix of '%s'", prefix);
+
+ /* let it end in a directory separator */
+ prefix_len = len - needle_len + 1;
+ }
+
+ /* Does it not start with the expected prefix? */
+ if (fspathncmp(location, prefix, prefix_len))
+ return location;
+
+ strlcpy(buf, location + prefix_len, sizeof(buf));
+ /* convert backslashes to forward slashes */
+ for (p = buf; *p; p++)
+ if (*p == '\\')
+ *p = '/';
+
+ return buf;
+}
+#endif
+
static void msg_with_prefix(const char *prefix, const char *format, va_list ap)
{
fflush(stderr);
@@ -147,7 +187,8 @@ int test__run_end(int was_run UNUSED, const char *location, const char *format,
break;
case RESULT_NONE:
- test_msg("BUG: test has no checks at %s", location);
+ test_msg("BUG: test has no checks at %s",
+ make_relative(location));
printf("not ok %d", ctx.count);
print_description(format, ap);
ctx.result = RESULT_FAILURE;
@@ -193,13 +234,15 @@ int test_assert(const char *location, const char *check, int ok)
assert(ctx.running);
if (ctx.result == RESULT_SKIP) {
- test_msg("skipping check '%s' at %s", check, location);
+ test_msg("skipping check '%s' at %s", check,
+ make_relative(location));
return 0;
} else if (!ctx.todo) {
if (ok) {
test_pass();
} else {
- test_msg("check \"%s\" failed at %s", check, location);
+ test_msg("check \"%s\" failed at %s", check,
+ make_relative(location));
test_fail();
}
}
@@ -224,7 +267,8 @@ int test__todo_end(const char *location, const char *check, int res)
if (ctx.result == RESULT_SKIP)
return 0;
if (!res) {
- test_msg("todo check '%s' succeeded at %s", check, location);
+ test_msg("todo check '%s' succeeded at %s", check,
+ make_relative(location));
test_fail();
} else {
test_todo();
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 2/7] unit-tests: do not mistake `.pdb` files for being executable
From: Johannes Schindelin via GitGitGadget @ 2023-09-18 20:54 UTC (permalink / raw)
To: git; +Cc: Phillip Wood, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1579.v2.git.1695070468.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When building the unit tests via CMake, the `.pdb` files are built.
Those are, essentially, files containing the debug information
separately from the executables.
Let's not confuse them with the executables we actually want to run.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/Makefile b/t/Makefile
index 095334bfdec..38fe0ded5bd 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -42,7 +42,7 @@ TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))
TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))
CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
-UNIT_TESTS = $(sort $(filter-out %.h %.c %.o unit-tests/t-basic%,$(wildcard unit-tests/t-*)))
+UNIT_TESTS = $(sort $(filter-out %.h %.c %.o %.pdb unit-tests/t-basic%,$(wildcard unit-tests/t-*)))
# `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)
# checks all tests in all scripts via a single invocation, so tell individual
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 0/7] CMake(Visual C) support for js/doc-unit-tests
From: Johannes Schindelin via GitGitGadget @ 2023-09-18 20:54 UTC (permalink / raw)
To: git; +Cc: Phillip Wood, Johannes Schindelin
In-Reply-To: <pull.1579.git.1693462532.gitgitgadget@gmail.com>
The recent patch series that adds proper unit testing to Git requires a
couple of add-on patches to make it work with the CMake build on Windows
(Visual C). This patch series aims to provide that support.
This patch series is based on js/doc-unit-tests.
Changes since v1:
* The code added to test-lib.c now avoids using a strbuf.
* The unit tests are now also handled via CTest.
* While at it, I cleaned up a little in the CTest-related definitions.
Johannes Schindelin (7):
cmake: also build unit tests
unit-tests: do not mistake `.pdb` files for being executable
unit-tests: do show relative file paths
artifacts-tar: when including `.dll` files, don't forget the
unit-tests
cmake: fix typo in variable name
cmake: use test names instead of full paths
cmake: handle also unit tests
Makefile | 2 +-
contrib/buildsystems/CMakeLists.txt | 38 ++++++++++++++++++---
t/Makefile | 2 +-
t/unit-tests/test-lib.c | 52 ++++++++++++++++++++++++++---
4 files changed, 84 insertions(+), 10 deletions(-)
base-commit: 03f9bc407975bba86d1d1807519d76e1693ff66f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1579%2Fdscho%2Fdoc-unit-tests-and-cmake-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1579/dscho/doc-unit-tests-and-cmake-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1579
Range-diff vs v1:
1: 2cc1c03d851 = 1: 2cc1c03d851 cmake: also build unit tests
2: 90db3d5d41f = 2: 90db3d5d41f unit-tests: do not mistake `.pdb` files for being executable
3: 2b4e36c05c9 ! 3: f0b804129e8 unit-tests: do show relative file paths
@@ t/unit-tests/test-lib.c: static struct {
+ * Visual C interpolates the absolute Windows path for `__FILE__`,
+ * but we want to see relative paths, as verified by t0080.
+ */
-+#include "strbuf.h"
+#include "dir.h"
+
+static const char *make_relative(const char *location)
+{
-+ static const char *prefix;
++ static char prefix[] = __FILE__, buf[PATH_MAX], *p;
+ static size_t prefix_len;
-+ static struct strbuf buf = STRBUF_INIT;
+
-+ if (!prefix) {
-+ strbuf_addstr(&buf, __FILE__);
-+ if (!strbuf_strip_suffix(&buf, "\\t\\unit-tests\\test-lib.c"))
-+ die("unexpected suffix of '%s'");
-+ strbuf_complete(&buf, '\\');
-+ prefix = strbuf_detach(&buf, &prefix_len);
++ if (!prefix_len) {
++ size_t len = strlen(prefix);
++ const char *needle = "\\t\\unit-tests\\test-lib.c";
++ size_t needle_len = strlen(needle);
++
++ if (len < needle_len || strcmp(needle, prefix + len - needle_len))
++ die("unexpected suffix of '%s'", prefix);
++
++ /* let it end in a directory separator */
++ prefix_len = len - needle_len + 1;
+ }
+
+ /* Does it not start with the expected prefix? */
+ if (fspathncmp(location, prefix, prefix_len))
+ return location;
+
-+ strbuf_reset(&buf);
-+ strbuf_addstr(&buf, location + prefix_len);
-+ convert_slashes(buf.buf);
++ strlcpy(buf, location + prefix_len, sizeof(buf));
++ /* convert backslashes to forward slashes */
++ for (p = buf; *p; p++)
++ if (*p == '\\')
++ *p = '/';
+
-+ return buf.buf;
++ return buf;
+}
+#endif
+
4: fb03f5aa6e5 = 4: a70339f57a7 artifacts-tar: when including `.dll` files, don't forget the unit-tests
-: ----------- > 5: 75a74571fbe cmake: fix typo in variable name
-: ----------- > 6: 41228df1b46 cmake: use test names instead of full paths
-: ----------- > 7: 003d44e9f0d cmake: handle also unit tests
--
gitgitgadget
^ permalink raw reply
* [PATCH v2 1/7] cmake: also build unit tests
From: Johannes Schindelin via GitGitGadget @ 2023-09-18 20:54 UTC (permalink / raw)
To: git; +Cc: Phillip Wood, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1579.v2.git.1695070468.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
A new, better way to run unit tests was just added to Git. This adds
support for building those unit tests via CMake.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
contrib/buildsystems/CMakeLists.txt | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 2f6e0197ffa..45016213358 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -965,6 +965,24 @@ target_link_libraries(test-fake-ssh common-main)
parse_makefile_for_sources(test-reftable_SOURCES "REFTABLE_TEST_OBJS")
list(TRANSFORM test-reftable_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
+#unit-tests
+add_library(unit-test-lib OBJECT ${CMAKE_SOURCE_DIR}/t/unit-tests/test-lib.c)
+
+parse_makefile_for_scripts(unit_test_PROGRAMS "UNIT_TEST_PROGRAMS" "")
+foreach(unit_test ${unit_test_PROGRAMS})
+ add_executable("${unit_test}" "${CMAKE_SOURCE_DIR}/t/unit-tests/${unit_test}.c")
+ target_link_libraries("${unit_test}" unit-test-lib common-main)
+ set_target_properties("${unit_test}"
+ PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/t/unit-tests)
+ if(MSVC)
+ set_target_properties("${unit_test}"
+ PROPERTIES RUNTIME_OUTPUT_DIRECTORY_DEBUG ${CMAKE_BINARY_DIR}/t/unit-tests)
+ set_target_properties("${unit_test}"
+ PROPERTIES RUNTIME_OUTPUT_DIRECTORY_RELEASE ${CMAKE_BINARY_DIR}/t/unit-tests)
+ endif()
+ list(APPEND PROGRAMS_BUILT "${unit_test}")
+endforeach()
+
#test-tool
parse_makefile_for_sources(test-tool_SOURCES "TEST_BUILTINS_OBJS")
--
gitgitgadget
^ permalink raw reply related
* Re: [REGRESSION] uninitialized value $address in git send-email
From: Michael Strawbridge @ 2023-09-18 20:26 UTC (permalink / raw)
To: Bagas Sanjaya, Junio C Hamano, Luben Tuikov,
Ævar Arnfjörð Bjarmason, Emily Shaffer,
Doug Anderson
Cc: Git Mailing List
In-Reply-To: <ZQhI5fMhDE82awpE@debian.me>
Hi,
Author of a8022c5f7b67 (send-email: expose header information to
git-send-email's sendemail-validate hook, 2023-04-19) here.
On 2023-09-18 08:56, Bagas Sanjaya wrote:
> Hi,
>
> Recently when I was submitting doc fixes to linux-doc mailing list [1]
> using git-send-email(1), I got perl-related error:
>
> ```
> Use of uninitialized value $address in sprintf at /home/bagas/.app/git/dist/v2.42.0/libexec/git-core/git-send-email line 1172.
> error: unable to extract a valid address from:
> ```
>
> My linux.git clone has sendemail-validate hook which uses patatt (from b4
> package). The hook is:
>
> ```
> #!/bin/sh
> # installed by patatt install-hook
> patatt sign --hook "${1}"
> ```
>
> This issue occurs on Git v2.41.0 but not in v2.40.0. Bisecting, the culprit is
> commit a8022c5f7b67 (send-email: expose header information to git-send-email's
> sendemail-validate hook, 2023-04-19). Emily's earlier report [2] also points to
> the same culprit, but with different bug.
>
> I triggered this issue on patch series with cover letter. To reproduce:
>
> 1. Clone git.git repo, then branch off:
>
> ```
> $ git clone https://github.com/git/git.git && cd git
> $ git checkout -b test
> ```
>
> 2. Make two dummy signed-off commits:
>
> ```
> $ echo test > test && git add test && git commit -s -m "test"
> $ echo "test test" >> test && git commit -a -s -m "test test"
> ```
>
> 3. Generate patch series:
>
> ```
> $ mkdir /tmp/test
> $ git format-patch -o /tmp/test --cover-letter main
> ```
>
> 4. Send the series to dummy address:
>
> ```
> $ git send-email --to="pi <pi@pi>" /tmp/test/*.patch
> ```
I tried to repro this today on my side. I can repro the error when
using the address "pi <pi@pi>" but that's not a valid email address and
so one would expect it to fail in the extract_valid_address_or_die
function with the error that you mention. As soon as I make the address
valid like "pi <pi@pi.com>", git send-email no longer complains.
In your original case, are you trying to send email to an invalid email
address? Is it an alias by chance?
Thanks.
> git-send-email(1) trips on the cover letter since there is no recipient
> addresses detected. It also errored out on patches without Signed-off-by
> trailer. When the command should have been succeeded, I expected that it
> asked me whether to send each patch or not.
>
> My system runs Debian testing (trixie/sid) with perl 5.36.0.
>
> Thanks.
>
> [1]: https://lore.kernel.org/linux-doc/20230918093240.29824-1-bagasdotme@gmail.com/
> [2]: https://lore.kernel.org/git/CAJoAoZ=GGgjGOeaeo6RFBO7=6msdRf-Ze6XcnL04K5ugupLUJA@mail.gmail.com/
>
^ permalink raw reply
* [PATCH] subtree: fix split processing with multiple subtrees present
From: Zach FettersMoore via GitGitGadget @ 2023-09-18 20:05 UTC (permalink / raw)
To: git; +Cc: Zach FettersMoore, Zach FettersMoore
From: Zach FettersMoore <zach.fetters@apollographql.com>
When there are multiple subtrees present in a repository and they are
all using 'git subtree split', the 'split' command can take a
significant (and constantly growing) amount of time to run even when
using the '--rejoin' flag. This is due to the fact that when processing
commits to determine the last known split to start from when looking
for changes, if there has been a split/merge done from another subtree
there will be 2 split commits, one mainline and one subtree, for the
second subtree that are part of the processing. The non-mainline
subtree split commit will cause the processing to always need to search
the entire history of the given subtree as part of its processing even
though those commits are totally irrelevant to the current subtree
split being run.
In the diagram below, 'M' represents the mainline repo branch, 'A'
represents one subtree, and 'B' represents another. M3 and B1 represent
a split commit for subtree B that was created from commit M4. M2 and A1
represent a split commit made from subtree A that was also created
based on changes back to and including M4. M1 represents new changes to
the repo, in this scenario if you try to run a 'git subtree split
--rejoin' for subtree B, commits M1, M2, and A1, will be included in
the processing of changes for the new split commit since the last
split/rejoin for subtree B was at M3. The issue is that by having A1
included in this processing the command ends up needing to processing
every commit down tree A even though none of that is needed or relevant
to the current command and result.
M1
| \ \
M2 | |
| A1 |
M3 | |
| | B1
M4 | |
So this commit makes a change to the processing of commits for the split
command in order to ignore non-mainline commits from other subtrees such
as A1 in the diagram by adding a new function
'should_ignore_subtree_commit' which is called during
'process_split_commit'. This allows the split/rejoin processing to still
function as expected but removes all of the unnecessary processing that
takes place currently which greatly inflates the processing time.
Signed-off-by: Zach FettersMoore <zach.fetters@apollographql.com>
---
subtree: fix split processing with multiple subtrees present
When there are multiple subtrees in a repo and git subtree split
--rejoin is being used for the subtrees, the processing of commits for a
new split can take a significant (and constantly growing) amount of time
because the split commits from other subtrees cause the processing to
have to scan the entire history of the other subtree(s). This patch
filters out the other subtree split commits that are unnecessary for the
split commit processing.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1587%2FBobaFetters%2Fzf%2Fmulti-subtree-processing-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1587/BobaFetters/zf/multi-subtree-processing-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1587
contrib/subtree/git-subtree.sh | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index e0c5d3b0de6..e9250dfb019 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -778,12 +778,29 @@ ensure_valid_ref_format () {
die "fatal: '$1' does not look like a ref"
}
+# Usage: check if a commit from another subtree should be ignored from processing for splits
+should_ignore_subtree_commit () {
+ if [ "$(git log -1 --grep="git-subtree-dir:" $1)" ]
+ then
+ if [[ -z "$(git log -1 --grep="git-subtree-mainline:" $1)" && -z "$(git log -1 --grep="git-subtree-dir: $dir$" $1)" ]]
+ then
+ return 0
+ fi
+ fi
+ return 1
+}
+
# Usage: process_split_commit REV PARENTS
process_split_commit () {
assert test $# = 2
local rev="$1"
local parents="$2"
+ if should_ignore_subtree_commit $rev
+ then
+ return
+ fi
+
if test $indent -eq 0
then
revcount=$(($revcount + 1))
base-commit: bda494f4043963b9ec9a1ecd4b19b7d1cd9a0518
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
From: Phillip Wood @ 2023-09-18 19:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: René Scharfe, Jeff King, Oswald Buddenhagen, Git List
In-Reply-To: <xmqqedivl832.fsf@gitster.g>
On 18/09/2023 18:11, Junio C Hamano wrote:
>> Then in builtin/am.c at the top level we'd add
>>
>> MAKE_CMDMODE_SETTER(resume_type)
>>
>> and change the option definitions to look like
>>
>> OPT_CMDMODE(0, "continue", resume_type, &resume.mode, ...)
>
> Yup, that is ergonomic and corrects "The shape of a particular enum
> may not match 'int'" issue nicely. I do not know how severe the
> problem is that it is not quite type safe that we cannot enforce
> resume_type is the same as typeof(resume.mode) here, though.
We could use gcc's __builtin_types_compatible_p() if we're prepared to
have two definitions of OPT_CMDMODE_F
#if defined(__GNUC__)
#define OPT_CMDMODE_F(s, l, n, v, h, i, f) { \
...
.defval (i) + \
BUILD_ASSERT_OR_ZERO(__builtin_types_compatible_p(enum n,
__typeof__(v))), \
}
#else
#define OPT_CMDMODE_F(s, l, n, v, h, i, f) { \
...
.defval (i), \
}
#endif
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH] git-send-email.perl: avoid printing undef when validating addresses
From: Junio C Hamano @ 2023-09-18 19:04 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Jeff King, Bagas Sanjaya
In-Reply-To: <545729b619308c6f3397b9aa1747f26ddc58f461.1695054945.git.me@ttaylorr.com>
Taylor Blau <me@ttaylorr.com> writes:
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 897cea6564..288ea1ae80 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1166,10 +1166,10 @@ sub extract_valid_address {
>
> sub extract_valid_address_or_die {
> my $address = shift;
> + my $valid_address = extract_valid_address($address);
> die sprintf(__("error: unable to extract a valid address from: %s\n"), $address)
> + if !$valid_address;
> + return $valid_address;
This will still use undef if the incoming $address is already undef,
but the caller deserves what it gets in such a case. The message
reports that the %s is the source from which the code tried to
extract the address from, not the result of failed extraction, so
the rewrite is absolutely the right thing to do.
Will queue. Thanks.
> }
>
> sub validate_address {
^ permalink raw reply
* [PATCH 3/3] refs: alternate reftable ref backend implementation
From: Han-Wen Nienhuys via GitGitGadget @ 2023-09-18 17:59 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys, Han-Wen Nienhuys
In-Reply-To: <pull.1574.git.git.1695059978.gitgitgadget@gmail.com>
From: Han-Wen Nienhuys <hanwen@google.com>
This introduces the reftable backend as an alternative to the packed
backend. This simplifies the code, 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.
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.
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.
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 | 8 +-
refs/packed-backend.c | 14 +-
refs/refs-internal.h | 1 +
refs/reftable-backend.c | 1745 +++++++++++++++++++++++++++++++
refs/reftable-backend.h | 8 +
8 files changed, 1766 insertions(+), 15 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 c0a7e3d375b..725321a229e 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -9,6 +9,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"
@@ -103,8 +104,13 @@ static struct ref_store *files_ref_store_create(struct repository *repo,
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 =
- packed_ref_store_create(repo, refs->gitcommondir, flags);
+ git_env_bool("GIT_TEST_REFTABLE", 0)
+ ? 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/packed-backend.c b/refs/packed-backend.c
index ba895c845c0..ad72cad8024 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.
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 87f8e8b51d6..172ae23534b 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -700,6 +700,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..367bcae2857
--- /dev/null
+++ b/refs/reftable-backend.c
@@ -0,0 +1,1745 @@
+#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"
+
+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 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)
+{
+ 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;
+
+ 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) {
+ 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;
+ }
+
+ 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 err;
+}
+
+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 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);
+
+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_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 2/3] refs: move is_packed_transaction_needed out of packed-backend.c
From: Han-Wen Nienhuys via GitGitGadget @ 2023-09-18 17:59 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys, Han-Wen Nienhuys
In-Reply-To: <pull.1574.git.git.1695059978.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 | 96 +++++++++++++++++++++++++++++++++++++++++++
refs/packed-backend.c | 95 ------------------------------------------
refs/packed-backend.h | 9 ----
3 files changed, 96 insertions(+), 104 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4a6781af783..c0a7e3d375b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2630,6 +2630,101 @@ 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;
@@ -3294,3 +3389,4 @@ struct ref_storage_be refs_be_files = {
.delete_reflog = files_delete_reflog,
.reflog_expire = files_reflog_expire
};
+
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 5df7fa8004f..ba895c845c0 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1455,101 +1455,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 1/3] refs: push lock management into packed backend
From: Han-Wen Nienhuys via GitGitGadget @ 2023-09-18 17:59 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys, Han-Wen Nienhuys
In-Reply-To: <pull.1574.git.git.1695059978.gitgitgadget@gmail.com>
From: Han-Wen Nienhuys <hanwen@google.com>
Introduces a ref backend method transaction_begin, which for the
packed backend takes packed-refs.lock.
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 | 8 +++++
refs/debug.c | 15 +++++++++
refs/files-backend.c | 78 +++++++++++++++----------------------------
refs/packed-backend.c | 41 ++++++++++++++---------
refs/packed-backend.h | 10 ------
refs/refs-internal.h | 5 +++
6 files changed, 79 insertions(+), 78 deletions(-)
diff --git a/refs.c b/refs.c
index fcae5dddc60..a2d1012520e 100644
--- a/refs.c
+++ b/refs.c
@@ -1130,10 +1130,18 @@ 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;
}
diff --git a/refs/debug.c b/refs/debug.c
index b7ffc4ce67e..964806e9301 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -41,6 +41,20 @@ 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)
@@ -451,6 +465,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..4a6781af783 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;
@@ -2804,14 +2794,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
@@ -2823,22 +2808,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;
- }
}
}
@@ -2940,13 +2909,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;
+ /* TODO: leaks on error path. */
+ 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: */
@@ -3087,16 +3067,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 59c78d7941f..5df7fa8004f 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1552,8 +1552,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 +1566,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,6 +1577,28 @@ static void packed_transaction_cleanup(struct packed_ref_store *refs,
transaction->state = REF_TRANSACTION_CLOSED;
}
+static int packed_transaction_begin(struct ref_store *ref_store,
+ struct ref_transaction *transaction,
+ 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;
+
+ CALLOC_ARRAY(data, 1);
+ string_list_init_nodup(&data->updates);
+
+ if (!is_lock_file_locked(&refs->lock)) {
+ if (packed_refs_lock(ref_store, 0, err))
+ /* todo: leaking data */
+ return -1;
+ }
+ transaction->backend_data = data;
+ return 0;
+}
+
static int packed_transaction_prepare(struct ref_store *ref_store,
struct ref_transaction *transaction,
struct strbuf *err)
@@ -1588,7 +1607,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;
@@ -1601,11 +1620,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:
@@ -1623,12 +1637,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;
@@ -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/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
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 9db8aec4da8..87f8e8b51d6 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -531,6 +531,10 @@ 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);
@@ -670,6 +674,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
* [PATCH 0/3] Simple reftable backend
From: Han-Wen Nienhuys via GitGitGadget @ 2023-09-18 17:59 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys
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 ("refs: push lock management into packed backend") is
a good improvement overall, and could be landed separately without much
tweaking.
The second part ("refs: alternate reftable ref backend implementation") 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.
Han-Wen Nienhuys (3):
refs: push lock management into packed backend
refs: move is_packed_transaction_needed out of packed-backend.c
refs: alternate reftable ref backend implementation
Makefile | 1 +
config.mak.uname | 2 +-
contrib/workdir/git-new-workdir | 2 +-
refs.c | 8 +
refs/debug.c | 15 +
refs/files-backend.c | 182 +++-
refs/packed-backend.c | 150 +--
refs/packed-backend.h | 19 -
refs/refs-internal.h | 6 +
refs/reftable-backend.c | 1745 +++++++++++++++++++++++++++++++
refs/reftable-backend.h | 8 +
11 files changed, 1941 insertions(+), 197 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-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1574/hanwen/simple-reftable-backend-v1
Pull-Request: https://github.com/git/git/pull/1574
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH] git-gui - use git-hook, honor core.hooksPath
From: Junio C Hamano @ 2023-09-18 17:53 UTC (permalink / raw)
To: Mark Levedahl; +Cc: Johannes Schindelin, me, git
In-Reply-To: <a4765b59-1953-695b-4f5e-686bef0a3a50@gmail.com>
Mark Levedahl <mlevedahl@gmail.com> writes:
> On 9/18/23 11:58, Junio C Hamano wrote:
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>>>> + set cmd [concat git hook run --ignore-missing $hook_name -- $args 2>@1]
>>>> + return [_open_stdout_stderr $cmd]
>>> This looks so much nicer than the original code.
>>>
>>> Thank you,
>>> Johannes
>> Yup, looking good.
>
> Thanks. BTW, my commit message at "Furthermore, since v2.36 git
> exposes its hook exection machinery via" needs
>
> s/exection/execution/
>
> Should I resend?
Nah, I'll just fix it up locally before merging.
^ permalink raw reply
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
From: Junio C Hamano @ 2023-09-18 17:11 UTC (permalink / raw)
To: Phillip Wood; +Cc: René Scharfe, Jeff King, Oswald Buddenhagen, Git List
In-Reply-To: <0bf56c65-e59f-4290-8160-cce141f692d5@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
>> - resume->mode = RESUME_SHOW_PATCH;
>> + resume->mode_int = RESUME_SHOW_PATCH;
>> resume->sub_mode = new_value;
>> return 0;
>> }
>
> Having "mode" and "mode_int" feels a bit fragile as only "mode_int" is
> valid while parsing the options but then we want to use "mode". I
> wonder if we could get Oswald's idea of using callbacks working in a
> reasonably ergonomic way with a couple of macros. We could add an new
> OPTION_SET_ENUM member to "enum parse_opt_type" that would take a
> setter function as well as the usual void *value. To set the value it
> would pass the value pointer and an integer value to the setter
> function. We could change OPT_CMDMODE to use OPTION_SET_ENUM and take
> the name of the enum as well as the integer value we want to set for
> that option. The name of the enum would be used to generate the name
> of the setter callback which would be defined with another macro. The
> macro to generate the setter would look like
>
> #define MAKE_CMDMODE_SETTER(name) \
> static void parse_cmdmode_ ## name (void * var, int value) {
> enum name *p = var;
> *p = value;
> }
Ah, OK. So that's how you defeat "how the size and alignment of an
enum mixes well with int is not known and depends on particular enum
type". It is a tad sad that this relies on "void *", which means
that the caller of parse_cmdmode_resume_type cannot be forced by the
compilers to pass "enum resume_type *" to the function, though. And
that is probably inevitable with the design as .enum_setter needs to
be of a single type, and the member in the "struct option" that
points at the destination variable must be "void *" as it has to
be capable of pointing at various different enum types.
> ...
> Then in builtin/am.c at the top level we'd add
>
> MAKE_CMDMODE_SETTER(resume_type)
>
> and change the option definitions to look like
>
> OPT_CMDMODE(0, "continue", resume_type, &resume.mode, ...)
Yup, that is ergonomic and corrects "The shape of a particular enum
may not match 'int'" issue nicely. I do not know how severe the
problem is that it is not quite type safe that we cannot enforce
resume_type is the same as typeof(resume.mode) here, though.
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