* [PATCH 08/12] t: introduce GIT_TEST_DEFAULT_REF_FORMAT envvar
From: Patrick Steinhardt @ 2023-12-20 10:55 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1703067989.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2531 bytes --]
Introduce a new GIT_TEST_DEFAULT_REF_FORMAT environment variable that
lets developers run the test suite with a different default ref format
without impacting the ref format used by non-test Git invocations. This
is modeled after GIT_TEST_DEFAULT_OBJECT_FORMAT, which does the same
thing for the repository's object format.
Adapt the setup of the `REFFILES` test prerequisite to be conditionally
set based on the default ref format.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/README | 3 +++
t/test-lib-functions.sh | 5 +++++
t/test-lib.sh | 11 ++++++++++-
3 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/t/README b/t/README
index 36463d0742..621d3b8c09 100644
--- a/t/README
+++ b/t/README
@@ -479,6 +479,9 @@ GIT_TEST_DEFAULT_HASH=<hash-algo> specifies which hash algorithm to
use in the test scripts. Recognized values for <hash-algo> are "sha1"
and "sha256".
+GIT_TEST_DEFAULT_REF_FORMAT=<format> specifies which ref storage format
+to use in the test scripts. Recognized values for <format> are "files".
+
GIT_TEST_NO_WRITE_REV_INDEX=<boolean>, when true disables the
'pack.writeReverseIndex' setting.
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 03292602fb..61871b2a3b 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1659,6 +1659,11 @@ test_detect_hash () {
test_hash_algo="${GIT_TEST_DEFAULT_HASH:-sha1}"
}
+# Detect the hash algorithm in use.
+test_detect_ref_format () {
+ echo "${GIT_TEST_DEFAULT_REF_FORMAT:-files}"
+}
+
# Load common hash metadata and common placeholder object IDs for use with
# test_oid.
test_oid_init () {
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 4685cc3d48..fc93aa57e6 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -542,6 +542,8 @@ export EDITOR
GIT_DEFAULT_HASH="${GIT_TEST_DEFAULT_HASH:-sha1}"
export GIT_DEFAULT_HASH
+GIT_DEFAULT_REF_FORMAT="${GIT_TEST_DEFAULT_REF_FORMAT:-files}"
+export GIT_DEFAULT_REF_FORMAT
GIT_TEST_MERGE_ALGORITHM="${GIT_TEST_MERGE_ALGORITHM:-ort}"
export GIT_TEST_MERGE_ALGORITHM
@@ -1745,7 +1747,14 @@ parisc* | hppa*)
;;
esac
-test_set_prereq REFFILES
+case "$GIT_DEFAULT_REF_FORMAT" in
+files)
+ test_set_prereq REFFILES;;
+*)
+ echo 2>&1 "error: unknown ref format $GIT_DEFAULT_REF_FORMAT"
+ exit 1
+ ;;
+esac
( COLUMNS=1 && test $COLUMNS = 1 ) && test_set_prereq COLUMNS_CAN_BE_1
test -z "$NO_CURL" && test_set_prereq LIBCURL
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 07/12] setup: introduce GIT_DEFAULT_REF_FORMAT envvar
From: Patrick Steinhardt @ 2023-12-20 10:55 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1703067989.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3130 bytes --]
Introduce a new GIT_DEFAULT_REF_FORMAT environment variable that lets
users control the default ref format used by both git-init(1) and
git-clone(1). This is modeled after GIT_DEFAULT_OBJECT_FORMAT, which
does the same thing for the repository's object format.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Documentation/git.txt | 5 +++++
setup.c | 7 +++++++
t/t0001-init.sh | 18 ++++++++++++++++++
3 files changed, 30 insertions(+)
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 4698d7a42b..0ab19eef3b 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -556,6 +556,11 @@ double-quotes and respecting backslash escapes. E.g., the value
is always used. The default is "sha1".
See `--object-format` in linkgit:git-init[1].
+`GIT_DEFAULT_REF_FORMAT`::
+ If this variable is set, the default reference backend format for new
+ repositories will be set to this value. The default is "files".
+ See `--ref-format` in linkgit:git-init[1].
+
Git Commits
~~~~~~~~~~~
`GIT_AUTHOR_NAME`::
diff --git a/setup.c b/setup.c
index 7eb4ae8ea0..8913e30974 100644
--- a/setup.c
+++ b/setup.c
@@ -2160,12 +2160,19 @@ static void validate_hash_algorithm(struct repository_format *repo_fmt, int hash
static void validate_ref_storage_format(struct repository_format *repo_fmt, int format)
{
+ const char *name = getenv("GIT_DEFAULT_REF_FORMAT");
+
if (repo_fmt->version >= 0 &&
format != REF_STORAGE_FORMAT_UNKNOWN &&
format != repo_fmt->ref_storage_format) {
die(_("attempt to reinitialize repository with different reference storage format"));
} else if (format != REF_STORAGE_FORMAT_UNKNOWN) {
repo_fmt->ref_storage_format = format;
+ } else if (name) {
+ format = ref_storage_format_by_name(name);
+ if (format == REF_STORAGE_FORMAT_UNKNOWN)
+ die(_("unknown ref storage format '%s'"), name);
+ repo_fmt->ref_storage_format = format;
}
}
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 38b3e4c39e..30ce752cc1 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -558,6 +558,24 @@ test_expect_success DEFAULT_REPO_FORMAT 'extensions.refStorage with unknown back
grep "invalid value for ${SQ}extensions.refstorage${SQ}: ${SQ}garbage${SQ}" err
'
+test_expect_success DEFAULT_REPO_FORMAT 'init with GIT_DEFAULT_REF_FORMAT=files' '
+ test_when_finished "rm -rf refformat" &&
+ GIT_DEFAULT_REF_FORMAT=files git init refformat &&
+ echo 0 >expect &&
+ git -C refformat config core.repositoryformatversion >actual &&
+ test_cmp expect actual &&
+ test_must_fail git -C refformat config extensions.refstorage
+'
+
+test_expect_success 'init with GIT_DEFAULT_REF_FORMAT=garbage' '
+ test_when_finished "rm -rf refformat" &&
+ cat >expect <<-EOF &&
+ fatal: unknown ref storage format ${SQ}garbage${SQ}
+ EOF
+ test_must_fail env GIT_DEFAULT_REF_FORMAT=garbage git init refformat 2>err &&
+ test_cmp expect err
+'
+
test_expect_success MINGW 'core.hidedotfiles = false' '
git config --global core.hidedotfiles false &&
rm -rf newdir &&
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 06/12] setup: introduce "extensions.refStorage" extension
From: Patrick Steinhardt @ 2023-12-20 10:55 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1703067989.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 9372 bytes --]
Introduce a new "extensions.refStorage" extension that allows us to
specify the ref storage format used by a repository. For now, the only
supported format is the "files" format, but this list will likely soon
be extended to also support the upcoming "reftable" format.
There have been discussions on the Git mailing list in the past around
how exactly this extension should look like. One alternative [1] that
was discussed was whether it would make sense to model the extension in
such a way that backends are arbitrarily stackable. This would allow for
a combined value of e.g. "loose,packed-refs" or "loose,reftable", which
indicates that new refs would be written via "loose" files backend and
compressed into "packed-refs" or "reftable" backends, respectively.
It is arguable though whether this flexibility and the complexity that
it brings with it is really required for now. It is not foreseeable that
there will be a proliferation of backends in the near-term future, and
the current set of existing formats and formats which are on the horizon
can easily be configured with the much simpler proposal where we have a
single value, only.
Furthermore, if we ever see that we indeed want to gain the ability to
arbitrarily stack the ref formats, then we can adapt the current
extension rather easily. Given that Git clients will refuse any unknown
value for the "extensions.refStorage" extension they would also know to
ignore a stacked "loose,packed-refs" in the future.
So let's stick with the easy proposal for the time being and wire up the
extension.
[1]: <pull.1408.git.1667846164.gitgitgadget@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Documentation/config/extensions.txt | 11 ++++++++
Documentation/ref-storage-format.txt | 1 +
.../technical/repository-version.txt | 5 ++++
builtin/clone.c | 2 +-
setup.c | 23 +++++++++++++---
setup.h | 3 ++-
t/t0001-init.sh | 26 +++++++++++++++++++
t/test-lib.sh | 2 +-
8 files changed, 67 insertions(+), 6 deletions(-)
create mode 100644 Documentation/ref-storage-format.txt
diff --git a/Documentation/config/extensions.txt b/Documentation/config/extensions.txt
index bccaec7a96..66db0e15da 100644
--- a/Documentation/config/extensions.txt
+++ b/Documentation/config/extensions.txt
@@ -7,6 +7,17 @@ Note that this setting should only be set by linkgit:git-init[1] or
linkgit:git-clone[1]. Trying to change it after initialization will not
work and will produce hard-to-diagnose issues.
+extensions.refStorage::
+ Specify the ref storage format to use. The acceptable values are:
++
+include::../ref-storage-format.txt[]
++
+It is an error to specify this key unless `core.repositoryFormatVersion` is 1.
++
+Note that this setting should only be set by linkgit:git-init[1] or
+linkgit:git-clone[1]. Trying to change it after initialization will not
+work and will produce hard-to-diagnose issues.
+
extensions.worktreeConfig::
If enabled, then worktrees will load config settings from the
`$GIT_DIR/config.worktree` file in addition to the
diff --git a/Documentation/ref-storage-format.txt b/Documentation/ref-storage-format.txt
new file mode 100644
index 0000000000..1a65cac468
--- /dev/null
+++ b/Documentation/ref-storage-format.txt
@@ -0,0 +1 @@
+* `files` for loose files with packed-refs. This is the default.
diff --git a/Documentation/technical/repository-version.txt b/Documentation/technical/repository-version.txt
index 045a76756f..27be3741e6 100644
--- a/Documentation/technical/repository-version.txt
+++ b/Documentation/technical/repository-version.txt
@@ -100,3 +100,8 @@ If set, by default "git config" reads from both "config" and
multiple working directory mode, "config" file is shared while
"config.worktree" is per-working directory (i.e., it's in
GIT_COMMON_DIR/worktrees/<id>/config.worktree)
+
+==== `refStorage`
+
+Specifies the file format for the ref database. The only valid value
+is `files` (loose references with a packed-refs file).
diff --git a/builtin/clone.c b/builtin/clone.c
index 6840064b59..20c161e94a 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1289,7 +1289,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
* ours to the same thing.
*/
hash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));
- initialize_repository_version(hash_algo, 1);
+ initialize_repository_version(hash_algo, the_repository->ref_storage_format, 1);
repo_set_hash_algo(the_repository, hash_algo);
create_reference_database(the_repository->ref_storage_format, NULL, 1);
diff --git a/setup.c b/setup.c
index 5dffea6e2f..7eb4ae8ea0 100644
--- a/setup.c
+++ b/setup.c
@@ -590,6 +590,17 @@ static enum extension_result handle_extension(const char *var,
"extensions.objectformat", value);
data->hash_algo = format;
return EXTENSION_OK;
+ } else if (!strcmp(ext, "refstorage")) {
+ int format;
+
+ if (!value)
+ return config_error_nonbool(var);
+ format = ref_storage_format_by_name(value);
+ if (format == REF_STORAGE_FORMAT_UNKNOWN)
+ return error(_("invalid value for '%s': '%s'"),
+ "extensions.refstorage", value);
+ data->ref_storage_format = format;
+ return EXTENSION_OK;
}
return EXTENSION_UNKNOWN;
}
@@ -1869,12 +1880,14 @@ static int needs_work_tree_config(const char *git_dir, const char *work_tree)
return 1;
}
-void initialize_repository_version(int hash_algo, int reinit)
+void initialize_repository_version(int hash_algo, int ref_storage_format,
+ int reinit)
{
char repo_version_string[10];
int repo_version = GIT_REPO_VERSION;
- if (hash_algo != GIT_HASH_SHA1)
+ if (hash_algo != GIT_HASH_SHA1 ||
+ ref_storage_format != REF_STORAGE_FORMAT_FILES)
repo_version = GIT_REPO_VERSION_READ;
/* This forces creation of new config file */
@@ -1887,6 +1900,10 @@ void initialize_repository_version(int hash_algo, int reinit)
hash_algos[hash_algo].name);
else if (reinit)
git_config_set_gently("extensions.objectformat", NULL);
+
+ if (ref_storage_format != REF_STORAGE_FORMAT_FILES)
+ git_config_set("extensions.refstorage",
+ ref_storage_format_to_name(ref_storage_format));
}
static int is_reinit(void)
@@ -2028,7 +2045,7 @@ static int create_default_files(const char *template_path,
adjust_shared_perm(get_git_dir());
}
- initialize_repository_version(fmt->hash_algo, 0);
+ initialize_repository_version(fmt->hash_algo, fmt->ref_storage_format, 0);
/* Check filemode trustability */
path = git_path_buf(&buf, "config");
diff --git a/setup.h b/setup.h
index 4927abf2cf..89033ae1d9 100644
--- a/setup.h
+++ b/setup.h
@@ -180,7 +180,8 @@ int init_db(const char *git_dir, const char *real_git_dir,
int ref_storage_format,
const char *initial_branch, int init_shared_repository,
unsigned int flags);
-void initialize_repository_version(int hash_algo, int reinit);
+void initialize_repository_version(int hash_algo, int ref_storage_format,
+ int reinit);
void create_reference_database(int ref_storage_format,
const char *initial_branch, int quiet);
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 2b78e3be47..38b3e4c39e 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -532,6 +532,32 @@ test_expect_success 'init rejects attempts to initialize with different hash' '
test_must_fail git -C sha256 init --object-format=sha1
'
+test_expect_success DEFAULT_REPO_FORMAT 'extensions.refStorage is not allowed with repo version 0' '
+ test_when_finished "rm -rf refstorage" &&
+ git init refstorage &&
+ git -C refstorage config extensions.refStorage files &&
+ test_must_fail git -C refstorage rev-parse 2>err &&
+ grep "repo version is 0, but v1-only extension found" err
+'
+
+test_expect_success DEFAULT_REPO_FORMAT 'extensions.refStorage with files backend' '
+ test_when_finished "rm -rf refstorage" &&
+ git init refstorage &&
+ git -C refstorage config core.repositoryformatversion 1 &&
+ git -C refstorage config extensions.refStorage files &&
+ test_commit -C refstorage A &&
+ git -C refstorage rev-parse --verify HEAD
+'
+
+test_expect_success DEFAULT_REPO_FORMAT 'extensions.refStorage with unknown backend' '
+ test_when_finished "rm -rf refstorage" &&
+ git init refstorage &&
+ git -C refstorage config core.repositoryformatversion 1 &&
+ git -C refstorage config extensions.refStorage garbage &&
+ test_must_fail git -C refstorage rev-parse 2>err &&
+ grep "invalid value for ${SQ}extensions.refstorage${SQ}: ${SQ}garbage${SQ}" err
+'
+
test_expect_success MINGW 'core.hidedotfiles = false' '
git config --global core.hidedotfiles false &&
rm -rf newdir &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index dc03f06b8e..4685cc3d48 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1937,7 +1937,7 @@ test_lazy_prereq SHA1 '
'
test_lazy_prereq DEFAULT_REPO_FORMAT '
- test_have_prereq SHA1
+ test_have_prereq SHA1,REFFILES
'
# Ensure that no test accidentally triggers a Git command
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 05/12] setup: set repository's formats on init
From: Patrick Steinhardt @ 2023-12-20 10:55 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1703067989.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2228 bytes --]
The proper hash algorithm and ref storage format that will be used for a
newly initialized repository will be figured out in `init_db()` via
`validate_hash_algorithm()` and `validate_ref_storage_format()`. Until
now though, we never set up the hash algorithm or ref storage format of
`the_repository` accordingly.
There are only two callsites of `init_db()`, one in git-init(1) and one
in git-clone(1). The former function doesn't care for the formats to be
set up properly because it never access the repository after calling the
function in the first place.
For git-clone(1) it's a different story though, as we call `init_db()`
before listing remote refs. While we do indeed have the wrong hash
function in `the_repository` when `init_db()` sets up a non-default
object format for the repository, it never mattered because we adjust
the hash after learning about the remote's hash function via the listed
refs.
So the current state is correct for the hash algo, but it's not for the
ref storage format because git-clone(1) wouldn't know to set it up
properly. But instead of adjusting only the `ref_storage_format`, set
both the hash algo and the ref storage format so that `the_repository`
is in the correct state when `init_db()` exits. This is fine as we will
adjust the hash later on anyway and makes it easier to reason about the
end state of `the_repository`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
setup.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/setup.c b/setup.c
index c1bf106379..5dffea6e2f 100644
--- a/setup.c
+++ b/setup.c
@@ -2203,6 +2203,13 @@ int init_db(const char *git_dir, const char *real_git_dir,
&repo_fmt, prev_bare_repository,
init_shared_repository);
+ /*
+ * Now that we have set up both the hash algorithm and the ref storage
+ * format we can update the repository's settings accordingly.
+ */
+ the_repository->hash_algo = &hash_algos[repo_fmt.hash_algo];
+ the_repository->ref_storage_format = repo_fmt.ref_storage_format;
+
if (!(flags & INIT_DB_SKIP_REFDB))
create_reference_database(repo_fmt.ref_storage_format,
initial_branch, flags & INIT_DB_QUIET);
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 04/12] setup: start tracking ref storage format when
From: Patrick Steinhardt @ 2023-12-20 10:55 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1703067989.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 9599 bytes --]
In order to discern which ref storage format a repository is supposed to
use we need to start setting up and/or discovering the format. This
needs to happen in two separate code paths.
- The first path is when we create a repository via `init_db()`. When
we are re-initializing a preexisting repository we need to retain
the previously used ref storage format -- if the user asked for a
different format then this indicates an erorr and we error out.
Otherwise we either initialize the repository with the format asked
for by the user or the default format, which currently is the
"files" backend.
- The second path is when discovering repositories, where we need to
read the config of that repository. There is not yet any way to
configure something other than the "files" backend, so we can just
blindly set the ref storage format to this backend.
Wire up this logic so that we have the ref storage format always readily
available when needed. As there is only a single backend and because it
is not configurable we cannot yet verify that this tracking works as
expected via tests, but tests will be added in subsequent commits. To
countermand this ommission now though, raise a BUG() in case the ref
storage format is not set up properly in `ref_store_init()`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/clone.c | 5 +++--
builtin/init-db.c | 4 +++-
refs.c | 6 +++---
refs.h | 1 +
repository.c | 1 +
repository.h | 3 +++
setup.c | 26 +++++++++++++++++++++++---
setup.h | 6 +++++-
8 files changed, 42 insertions(+), 10 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 339af10c10..6840064b59 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1105,7 +1105,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
* repository, and reference backends may persist that information into
* their on-disk data structures.
*/
- init_db(git_dir, real_git_dir, option_template, GIT_HASH_UNKNOWN, NULL,
+ init_db(git_dir, real_git_dir, option_template, GIT_HASH_UNKNOWN,
+ REF_STORAGE_FORMAT_UNKNOWN, NULL,
do_not_override_repo_unix_permissions, INIT_DB_QUIET | INIT_DB_SKIP_REFDB);
if (real_git_dir) {
@@ -1290,7 +1291,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
hash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));
initialize_repository_version(hash_algo, 1);
repo_set_hash_algo(the_repository, hash_algo);
- create_reference_database(NULL, 1);
+ create_reference_database(the_repository->ref_storage_format, NULL, 1);
/*
* Before fetching from the remote, download and install bundle
diff --git a/builtin/init-db.c b/builtin/init-db.c
index cb727c826f..b6e80feab6 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -11,6 +11,7 @@
#include "object-file.h"
#include "parse-options.h"
#include "path.h"
+#include "refs.h"
#include "setup.h"
#include "strbuf.h"
@@ -236,5 +237,6 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
flags |= INIT_DB_EXIST_OK;
return init_db(git_dir, real_git_dir, template_dir, hash_algo,
- initial_branch, init_shared_repository, flags);
+ REF_STORAGE_FORMAT_UNKNOWN, initial_branch,
+ init_shared_repository, flags);
}
diff --git a/refs.c b/refs.c
index e87c85897d..d289a5e175 100644
--- a/refs.c
+++ b/refs.c
@@ -2045,12 +2045,12 @@ static struct ref_store *ref_store_init(struct repository *repo,
const char *gitdir,
unsigned int flags)
{
- int format = REF_STORAGE_FORMAT_FILES;
- const struct ref_storage_be *be = find_ref_storage_backend(format);
+ const struct ref_storage_be *be;
struct ref_store *refs;
+ be = find_ref_storage_backend(repo->ref_storage_format);
if (!be)
- BUG("reference backend %s is unknown", ref_storage_format_to_name(format));
+ BUG("reference backend is unknown");
refs = be->init(repo, gitdir, flags);
return refs;
diff --git a/refs.h b/refs.h
index c6571bcf6c..944a67ac1b 100644
--- a/refs.h
+++ b/refs.h
@@ -11,6 +11,7 @@ struct string_list;
struct string_list_item;
struct worktree;
+int default_ref_storage_format(void);
int ref_storage_format_by_name(const char *name);
const char *ref_storage_format_to_name(int ref_storage_format);
diff --git a/repository.c b/repository.c
index a7679ceeaa..a75c1598b0 100644
--- a/repository.c
+++ b/repository.c
@@ -184,6 +184,7 @@ int repo_init(struct repository *repo,
goto error;
repo_set_hash_algo(repo, format.hash_algo);
+ repo->ref_storage_format = format.ref_storage_format;
repo->repository_format_worktree_config = format.worktree_config;
/* take ownership of format.partial_clone */
diff --git a/repository.h b/repository.h
index ea4c488b81..22c30cdbc9 100644
--- a/repository.h
+++ b/repository.h
@@ -163,6 +163,9 @@ struct repository {
/* Repository's current hash algorithm, as serialized on disk. */
const struct git_hash_algo *hash_algo;
+ /* Repository's reference storage format, as serialized on disk. */
+ int ref_storage_format;
+
/* A unique-id for tracing purposes. */
int trace2_repo_id;
diff --git a/setup.c b/setup.c
index 155fe13f70..c1bf106379 100644
--- a/setup.c
+++ b/setup.c
@@ -1564,6 +1564,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
}
if (startup_info->have_repository) {
repo_set_hash_algo(the_repository, repo_fmt.hash_algo);
+ the_repository->ref_storage_format =
+ repo_fmt.ref_storage_format;
the_repository->repository_format_worktree_config =
repo_fmt.worktree_config;
/* take ownership of repo_fmt.partial_clone */
@@ -1657,6 +1659,8 @@ void check_repository_format(struct repository_format *fmt)
check_repository_format_gently(get_git_dir(), fmt, NULL);
startup_info->have_repository = 1;
repo_set_hash_algo(the_repository, fmt->hash_algo);
+ the_repository->ref_storage_format =
+ fmt->ref_storage_format;
the_repository->repository_format_worktree_config =
fmt->worktree_config;
the_repository->repository_format_partial_clone =
@@ -1897,7 +1901,8 @@ static int is_reinit(void)
return ret;
}
-void create_reference_database(const char *initial_branch, int quiet)
+void create_reference_database(int ref_storage_format,
+ const char *initial_branch, int quiet)
{
struct strbuf err = STRBUF_INIT;
int reinit = is_reinit();
@@ -1917,6 +1922,7 @@ void create_reference_database(const char *initial_branch, int quiet)
safe_create_dir(git_path("refs"), 1);
adjust_shared_perm(git_path("refs"));
+ the_repository->ref_storage_format = ref_storage_format;
if (refs_init_db(&err))
die("failed to set up refs db: %s", err.buf);
@@ -2135,8 +2141,20 @@ static void validate_hash_algorithm(struct repository_format *repo_fmt, int hash
}
}
+static void validate_ref_storage_format(struct repository_format *repo_fmt, int format)
+{
+ if (repo_fmt->version >= 0 &&
+ format != REF_STORAGE_FORMAT_UNKNOWN &&
+ format != repo_fmt->ref_storage_format) {
+ die(_("attempt to reinitialize repository with different reference storage format"));
+ } else if (format != REF_STORAGE_FORMAT_UNKNOWN) {
+ repo_fmt->ref_storage_format = format;
+ }
+}
+
int init_db(const char *git_dir, const char *real_git_dir,
- const char *template_dir, int hash, const char *initial_branch,
+ const char *template_dir, int hash,
+ int ref_storage_format, const char *initial_branch,
int init_shared_repository, unsigned int flags)
{
int reinit;
@@ -2179,13 +2197,15 @@ int init_db(const char *git_dir, const char *real_git_dir,
check_repository_format(&repo_fmt);
validate_hash_algorithm(&repo_fmt, hash);
+ validate_ref_storage_format(&repo_fmt, ref_storage_format);
reinit = create_default_files(template_dir, original_git_dir,
&repo_fmt, prev_bare_repository,
init_shared_repository);
if (!(flags & INIT_DB_SKIP_REFDB))
- create_reference_database(initial_branch, flags & INIT_DB_QUIET);
+ create_reference_database(repo_fmt.ref_storage_format,
+ initial_branch, flags & INIT_DB_QUIET);
create_object_directory();
if (get_shared_repository()) {
diff --git a/setup.h b/setup.h
index 3f0f17c351..4927abf2cf 100644
--- a/setup.h
+++ b/setup.h
@@ -115,6 +115,7 @@ struct repository_format {
int worktree_config;
int is_bare;
int hash_algo;
+ int ref_storage_format;
int sparse_index;
char *work_tree;
struct string_list unknown_extensions;
@@ -131,6 +132,7 @@ struct repository_format {
.version = -1, \
.is_bare = -1, \
.hash_algo = GIT_HASH_SHA1, \
+ .ref_storage_format = REF_STORAGE_FORMAT_FILES, \
.unknown_extensions = STRING_LIST_INIT_DUP, \
.v1_only_extensions = STRING_LIST_INIT_DUP, \
}
@@ -175,10 +177,12 @@ void check_repository_format(struct repository_format *fmt);
int init_db(const char *git_dir, const char *real_git_dir,
const char *template_dir, int hash_algo,
+ int ref_storage_format,
const char *initial_branch, int init_shared_repository,
unsigned int flags);
void initialize_repository_version(int hash_algo, int reinit);
-void create_reference_database(const char *initial_branch, int quiet);
+void create_reference_database(int ref_storage_format,
+ const char *initial_branch, int quiet);
/*
* NOTE NOTE NOTE!!
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 03/12] refs: refactor logic to look up storage backends
From: Patrick Steinhardt @ 2023-12-20 10:55 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1703067989.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 5190 bytes --]
In order to look up ref storage backends, we're currently using a linked
list of backends, where each backend is expected to set up its `next`
pointer to the next ref storage backend. This is kind of a weird setup
as backends need to be aware of other backends without much of a reason.
Refactor the code so that the array of backends is centrally defined in
"refs.c", where each backend is now identified by an integer constant.
Expose functions to translate from those integer constants to the name
and vice versa, which will be required by subsequent patches.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refs.c | 34 +++++++++++++++++++++++++---------
refs.h | 3 +++
refs/debug.c | 1 -
refs/files-backend.c | 1 -
refs/packed-backend.c | 1 -
refs/refs-internal.h | 1 -
repository.h | 3 +++
7 files changed, 31 insertions(+), 13 deletions(-)
diff --git a/refs.c b/refs.c
index 16bfa21df7..e87c85897d 100644
--- a/refs.c
+++ b/refs.c
@@ -33,17 +33,33 @@
/*
* List of all available backends
*/
-static struct ref_storage_be *refs_backends = &refs_be_files;
+static const struct ref_storage_be *refs_backends[] = {
+ [REF_STORAGE_FORMAT_FILES] = &refs_be_files,
+};
-static struct ref_storage_be *find_ref_storage_backend(const char *name)
+static const struct ref_storage_be *find_ref_storage_backend(int ref_storage_format)
{
- struct ref_storage_be *be;
- for (be = refs_backends; be; be = be->next)
- if (!strcmp(be->name, name))
- return be;
+ if (ref_storage_format && ref_storage_format < ARRAY_SIZE(refs_backends))
+ return refs_backends[ref_storage_format];
return NULL;
}
+int ref_storage_format_by_name(const char *name)
+{
+ for (int i = 0; i < ARRAY_SIZE(refs_backends); i++)
+ if (refs_backends[i] && !strcmp(refs_backends[i]->name, name))
+ return i;
+ return REF_STORAGE_FORMAT_UNKNOWN;
+}
+
+const char *ref_storage_format_to_name(int ref_storage_format)
+{
+ const struct ref_storage_be *be = find_ref_storage_backend(ref_storage_format);
+ if (!be)
+ return "unknown";
+ return be->name;
+}
+
/*
* How to handle various characters in refnames:
* 0: An acceptable character for refs
@@ -2029,12 +2045,12 @@ static struct ref_store *ref_store_init(struct repository *repo,
const char *gitdir,
unsigned int flags)
{
- const char *be_name = "files";
- struct ref_storage_be *be = find_ref_storage_backend(be_name);
+ int format = REF_STORAGE_FORMAT_FILES;
+ const struct ref_storage_be *be = find_ref_storage_backend(format);
struct ref_store *refs;
if (!be)
- BUG("reference backend %s is unknown", be_name);
+ BUG("reference backend %s is unknown", ref_storage_format_to_name(format));
refs = be->init(repo, gitdir, flags);
return refs;
diff --git a/refs.h b/refs.h
index 23211a5ea1..c6571bcf6c 100644
--- a/refs.h
+++ b/refs.h
@@ -11,6 +11,9 @@ struct string_list;
struct string_list_item;
struct worktree;
+int ref_storage_format_by_name(const char *name);
+const char *ref_storage_format_to_name(int ref_storage_format);
+
/*
* Resolve a reference, recursively following symbolic refererences.
*
diff --git a/refs/debug.c b/refs/debug.c
index 83b7a0ba65..b9775f2c37 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -426,7 +426,6 @@ static int debug_reflog_expire(struct ref_store *ref_store, const char *refname,
}
struct ref_storage_be refs_be_debug = {
- .next = NULL,
.name = "debug",
.init = NULL,
.init_db = debug_init_db,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index ad8b1d143f..43fd0ac760 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3241,7 +3241,6 @@ static int files_init_db(struct ref_store *ref_store, struct strbuf *err UNUSED)
}
struct ref_storage_be refs_be_files = {
- .next = NULL,
.name = "files",
.init = files_ref_store_create,
.init_db = files_init_db,
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index b9fa097a29..8d1090e284 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1705,7 +1705,6 @@ static struct ref_iterator *packed_reflog_iterator_begin(struct ref_store *ref_s
}
struct ref_storage_be refs_be_packed = {
- .next = NULL,
.name = "packed",
.init = packed_ref_store_create,
.init_db = packed_init_db,
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 4af83bf9a5..8e9f04cc67 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -663,7 +663,6 @@ typedef int read_symbolic_ref_fn(struct ref_store *ref_store, const char *refnam
struct strbuf *referent);
struct ref_storage_be {
- struct ref_storage_be *next;
const char *name;
ref_store_init_fn *init;
ref_init_db_fn *init_db;
diff --git a/repository.h b/repository.h
index 5f18486f64..ea4c488b81 100644
--- a/repository.h
+++ b/repository.h
@@ -24,6 +24,9 @@ enum fetch_negotiation_setting {
FETCH_NEGOTIATION_NOOP,
};
+#define REF_STORAGE_FORMAT_UNKNOWN 0
+#define REF_STORAGE_FORMAT_FILES 1
+
struct repo_settings {
int initialized;
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 02/12] worktree: skip reading HEAD when repairing worktrees
From: Patrick Steinhardt @ 2023-12-20 10:54 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1703067989.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 4307 bytes --]
When calling `git init --separate-git-dir=<new-path>` on a preexisting
repository, then we move the Git directory of that repository to the new
path specified by the user. If there are worktrees present in the Git
repository, we need to repair the worktrees so that their gitlinks point
to the new location of the repository.
This repair logic will load repositories via `get_worktrees()`, which
will enumerate up and initialize all worktrees. Part of initialization
is logic that we resolve their respective worktree HEADs, even though
that information may not actually be needed in the end by all callers.
In the context of git-init(1) this is about to become a problem, because
we do not have a repository that was set up via `setup_git_directory()`
or friends. Consequentially, it is not yet fully initialized at the time
of calling `repair_worktrees()`, and properly setting up all parts of
the repository in `init_db()` before we repair worktrees is not an easy
thing to do. While this is okay right now where we only have a single
reference backend in Git, once we gain a second one we would be trying
to look up the worktree HEADs before we have figured out the reference
format, which does not work.
We do not require the worktree HEADs at all to repair worktrees. So
let's fix issue this by skipping over the step that reads them.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
worktree.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/worktree.c b/worktree.c
index a56a6c2a3d..9702ed0308 100644
--- a/worktree.c
+++ b/worktree.c
@@ -51,7 +51,7 @@ static void add_head_info(struct worktree *wt)
/**
* get the main worktree
*/
-static struct worktree *get_main_worktree(void)
+static struct worktree *get_main_worktree(int skip_reading_head)
{
struct worktree *worktree = NULL;
struct strbuf worktree_path = STRBUF_INIT;
@@ -70,11 +70,13 @@ static struct worktree *get_main_worktree(void)
*/
worktree->is_bare = (is_bare_repository_cfg == 1) ||
is_bare_repository();
- add_head_info(worktree);
+ if (!skip_reading_head)
+ add_head_info(worktree);
return worktree;
}
-static struct worktree *get_linked_worktree(const char *id)
+static struct worktree *get_linked_worktree(const char *id,
+ int skip_reading_head)
{
struct worktree *worktree = NULL;
struct strbuf path = STRBUF_INIT;
@@ -93,7 +95,8 @@ static struct worktree *get_linked_worktree(const char *id)
CALLOC_ARRAY(worktree, 1);
worktree->path = strbuf_detach(&worktree_path, NULL);
worktree->id = xstrdup(id);
- add_head_info(worktree);
+ if (!skip_reading_head)
+ add_head_info(worktree);
done:
strbuf_release(&path);
@@ -118,7 +121,7 @@ static void mark_current_worktree(struct worktree **worktrees)
free(git_dir);
}
-struct worktree **get_worktrees(void)
+static struct worktree **get_worktrees_internal(int skip_reading_head)
{
struct worktree **list = NULL;
struct strbuf path = STRBUF_INIT;
@@ -128,7 +131,7 @@ struct worktree **get_worktrees(void)
ALLOC_ARRAY(list, alloc);
- list[counter++] = get_main_worktree();
+ list[counter++] = get_main_worktree(skip_reading_head);
strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
dir = opendir(path.buf);
@@ -137,7 +140,7 @@ struct worktree **get_worktrees(void)
while ((d = readdir_skip_dot_and_dotdot(dir)) != NULL) {
struct worktree *linked = NULL;
- if ((linked = get_linked_worktree(d->d_name))) {
+ if ((linked = get_linked_worktree(d->d_name, skip_reading_head))) {
ALLOC_GROW(list, counter + 1, alloc);
list[counter++] = linked;
}
@@ -151,6 +154,11 @@ struct worktree **get_worktrees(void)
return list;
}
+struct worktree **get_worktrees(void)
+{
+ return get_worktrees_internal(0);
+}
+
const char *get_worktree_git_dir(const struct worktree *wt)
{
if (!wt)
@@ -591,7 +599,7 @@ static void repair_noop(int iserr UNUSED,
void repair_worktrees(worktree_repair_fn fn, void *cb_data)
{
- struct worktree **worktrees = get_worktrees();
+ struct worktree **worktrees = get_worktrees_internal(1);
struct worktree **wt = worktrees + 1; /* +1 skips main worktree */
if (!fn)
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 01/12] t: introduce DEFAULT_REPO_FORMAT prereq
From: Patrick Steinhardt @ 2023-12-20 10:54 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1703067989.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1588 bytes --]
A limited number of tests require repositories to have the default
repository format or otherwise they would fail to run, e.g. because they
fail to detect the correct hash function. While the hash function is the
only extension right now that creates problems like this, we are about
to add a second extensions for the ref format.
Introduce a new DEFAULT_REPO_FORMAT prereq that can easily be amended
whenever we add new format extensions. Next to making any such changes
easier on us, the prerequisite's name should also help to clarify the
intent better.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t3200-branch.sh | 2 +-
t/test-lib.sh | 4 ++++
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 6a316f081e..de7d3014e4 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -519,7 +519,7 @@ EOF
mv .git/config .git/config-saved
-test_expect_success SHA1 'git branch -m q q2 without config should succeed' '
+test_expect_success DEFAULT_REPO_FORMAT 'git branch -m q q2 without config should succeed' '
git branch -m q q2 &&
git branch -m q2 q
'
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 876b99562a..dc03f06b8e 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1936,6 +1936,10 @@ test_lazy_prereq SHA1 '
esac
'
+test_lazy_prereq DEFAULT_REPO_FORMAT '
+ test_have_prereq SHA1
+'
+
# Ensure that no test accidentally triggers a Git command
# that runs the actual maintenance scheduler, affecting a user's
# system permanently.
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 00/12] Introduce `refStorage` extension
From: Patrick Steinhardt @ 2023-12-20 10:54 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 4087 bytes --]
Hi,
this patch series introduces a new `refStorage` extension and related
tooling. While there is only a single user-visible ref backend for now,
this extension will eventually allow us to determine which backend shall
be used for a repository. Furthermore, the series introduces tooling so
that the ref storage format becomes more accessible.
The series is structured as follows:
- Patches 1 - 3: preliminary refactorings that prepare for the
introduction of the ref storage format.
- Patches 4 - 6: wire up the ref format when setting up the repository
and creating a new one.
- Patches 7 - 8: introduction of envvars to control the ref format
globally.
- Patches 9 - 11: amend our tooling to know how to access and set the
ref storage format.
- Patch 12: small patch for t9500 to make it handle ref storage
extensions in the future.
I've been kind of torn regarding how to name this in the user-facing
parts. Even though the extension is called "refStorage", I rather opted
to use "ref format" in the user-facing parts, which is similar in nature
to the "object format". Changing the extension to be called "refFormat"
would cause issues for JGit though, so it's likely not an option to
change it now.
Anyway, I'd appreciate your thoughts and proposals and am happy to
rename the user-facing parts if we get to a preferable agreement.
This series depends on 18c9cb7524 (builtin/clone: create the refdb with
the correct object format, 2023-12-12) because it relies on the more
careful setup of the repository's refdb during clones.
Thanks!
Patrick
Patrick Steinhardt (12):
t: introduce DEFAULT_REPO_FORMAT prereq
worktree: skip reading HEAD when repairing worktrees
refs: refactor logic to look up storage backends
setup: start tracking ref storage format when
setup: set repository's formats on init
setup: introduce "extensions.refStorage" extension
setup: introduce GIT_DEFAULT_REF_FORMAT envvar
t: introduce GIT_TEST_DEFAULT_REF_FORMAT envvar
builtin/rev-parse: introduce `--show-ref-format` flag
builtin/init: introduce `--ref-format=` value flag
builtin/clone: introduce `--ref-format=` value flag
t9500: write "extensions.refstorage" into config
Documentation/config/extensions.txt | 11 +++
Documentation/git-clone.txt | 6 ++
Documentation/git-init.txt | 7 ++
Documentation/git-rev-parse.txt | 3 +
Documentation/git.txt | 5 ++
Documentation/ref-storage-format.txt | 1 +
.../technical/repository-version.txt | 5 ++
builtin/clone.c | 17 ++++-
builtin/init-db.c | 15 +++-
builtin/rev-parse.c | 4 ++
refs.c | 34 ++++++---
refs.h | 4 ++
refs/debug.c | 1 -
refs/files-backend.c | 1 -
refs/packed-backend.c | 1 -
refs/refs-internal.h | 1 -
repository.c | 1 +
repository.h | 6 ++
setup.c | 63 +++++++++++++++--
setup.h | 9 ++-
t/README | 3 +
t/t0001-init.sh | 70 +++++++++++++++++++
t/t1500-rev-parse.sh | 17 +++++
t/t3200-branch.sh | 2 +-
t/t5601-clone.sh | 17 +++++
t/t9500-gitweb-standalone-no-errors.sh | 5 ++
t/test-lib-functions.sh | 5 ++
t/test-lib.sh | 15 +++-
worktree.c | 24 ++++---
29 files changed, 318 insertions(+), 35 deletions(-)
create mode 100644 Documentation/ref-storage-format.txt
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v3] Teach git apply to respect core.fileMode settings
From: Chandra Pratap via GitGitGadget @ 2023-12-20 10:08 UTC (permalink / raw)
To: git; +Cc: Torsten Bögershausen, Chandra Pratap, Chandra Pratap
In-Reply-To: <pull.1620.v2.git.1703010646036.gitgitgadget@gmail.com>
From: Chandra Pratap <chandrapratap3519@gmail.com>
When applying a patch that adds an executable file, git apply
ignores the core.fileMode setting (core.fileMode in git config
specifies whether the executable bit on files in the working tree
should be honored or not) resulting in warnings like:
warning: script.sh has type 100644, expected 100755
even when core.fileMode is set to false, which is undesired. This
is extra true for systems like Windows.
Fix this by inferring the correct file mode from the existing
index entry when core.filemode is set to false. Add a test case
that verifies the change and prevents future regression.
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
apply: make git apply respect core.fileMode settings
Regarding this:
> I am wondering if we want to be more strict about hiding error return
> code from "git ls-files" and "git ls-tree" behind pipes like these.
> Usually we encourage using a temporary file, e.g., ... git ls-files -s
> script.sh >ls-files-output && test_grep "^100755" ls-files-output &&
> ...
I have modified the patch so that the output of git ls-files and git
ls-tree are stored in a temporary file instead of being directly piped
to grep but also noticed similar working in other test cases in the same
test file. For example,
test_expect_success FILEMODE 'same mode (index only)' ' .... .... ....
git ls-files -s file | grep "^100755"
and
test_expect_success FILEMODE 'mode update (index only)' ' ... ... ...
git ls-files -s file | grep "^100755"
Would we want to modify these scripts as well so they follow the same
convention as above or is it okay to let them be as is?
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1620%2FChand-ra%2Fdevel-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1620/Chand-ra/devel-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1620
Range-diff vs v2:
1: 29c8c6d120e ! 1: 9a3623edfd2 Teach git apply to respect core.fileMode settings
@@ Commit message
warning: script.sh has type 100644, expected 100755
even when core.fileMode is set to false, which is undesired. This
- is extra true for systems like Windows, which don't rely on "lsat()".
+ is extra true for systems like Windows.
Fix this by inferring the correct file mode from the existing
- index entry when core.filemode is set to false. The added
- test case helps verify the change and prevents future regression.
+ index entry when core.filemode is set to false. Add a test case
+ that verifies the change and prevents future regression.
- Reviewed-by: Johannes Schindelin <johannes.schindelin@gmail.com>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
## apply.c ##
@@ t/t4129-apply-samemode.sh: test_expect_success POSIXPERM 'do not use core.shared
)
'
-+test_expect_success 'ensure git apply respects core.fileMode' '
++test_expect_success 'git apply respects core.fileMode' '
+ test_config core.fileMode false &&
+ echo true >script.sh &&
+ git add --chmod=+x script.sh &&
-+ git ls-files -s script.sh | grep "^100755" &&
++ git ls-files -s script.sh > ls-files-output &&
++ test_grep "^100755" ls-files-output &&
+ test_tick && git commit -m "Add script" &&
-+ git ls-tree -r HEAD script.sh | grep "^100755" &&
++ git ls-tree -r HEAD script.sh > ls-tree-output &&
++ test_grep "^100755" ls-tree-output &&
+
+ echo true >>script.sh &&
+ test_tick && git commit -m "Modify script" script.sh &&
+ git format-patch -1 --stdout >patch &&
-+ grep "index.*100755" patch &&
++ test_grep "^index.*100755$" patch &&
+
+ git switch -c branch HEAD^ &&
+ git apply --index patch 2>err &&
-+ ! grep "has type 100644, expected 100755" err &&
-+ git restore -S script.sh && git restore script.sh &&
++ test_grep ! "has type 100644, expected 100755" err &&
++ git reset --hard &&
+
+ git apply patch 2>err &&
-+ ! grep "has type 100644, expected 100755" err &&
++ test_grep ! "has type 100644, expected 100755" err &&
+
+ git apply --cached patch 2>err &&
-+ ! grep "has type 100644, expected 100755" err
++ test_grep ! "has type 100644, expected 100755" err
+'
+
test_done
apply.c | 8 ++++++--
t/t4129-apply-samemode.sh | 27 +++++++++++++++++++++++++++
2 files changed, 33 insertions(+), 2 deletions(-)
diff --git a/apply.c b/apply.c
index 3d69fec836d..58f26c40413 100644
--- a/apply.c
+++ b/apply.c
@@ -3778,8 +3778,12 @@ static int check_preimage(struct apply_state *state,
return error_errno("%s", old_name);
}
- if (!state->cached && !previous)
- st_mode = ce_mode_from_stat(*ce, st->st_mode);
+ if (!state->cached && !previous) {
+ if (!trust_executable_bit)
+ st_mode = *ce ? (*ce)->ce_mode : patch->old_mode;
+ else
+ st_mode = ce_mode_from_stat(*ce, st->st_mode);
+ }
if (patch->is_new < 0)
patch->is_new = 0;
diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh
index e7a7295f1b6..ff0c1602fd5 100755
--- a/t/t4129-apply-samemode.sh
+++ b/t/t4129-apply-samemode.sh
@@ -101,4 +101,31 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree
)
'
+test_expect_success 'git apply respects core.fileMode' '
+ test_config core.fileMode false &&
+ echo true >script.sh &&
+ git add --chmod=+x script.sh &&
+ git ls-files -s script.sh > ls-files-output &&
+ test_grep "^100755" ls-files-output &&
+ test_tick && git commit -m "Add script" &&
+ git ls-tree -r HEAD script.sh > ls-tree-output &&
+ test_grep "^100755" ls-tree-output &&
+
+ echo true >>script.sh &&
+ test_tick && git commit -m "Modify script" script.sh &&
+ git format-patch -1 --stdout >patch &&
+ test_grep "^index.*100755$" patch &&
+
+ git switch -c branch HEAD^ &&
+ git apply --index patch 2>err &&
+ test_grep ! "has type 100644, expected 100755" err &&
+ git reset --hard &&
+
+ git apply patch 2>err &&
+ test_grep ! "has type 100644, expected 100755" err &&
+
+ git apply --cached patch 2>err &&
+ test_grep ! "has type 100644, expected 100755" err
+'
+
test_done
base-commit: 1a87c842ece327d03d08096395969aca5e0a6996
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 4/7] reftable/record: store "val1" hashes as static arrays
From: Patrick Steinhardt @ 2023-12-20 9:25 UTC (permalink / raw)
To: git
In-Reply-To: <06c9eab67802ba933b44d32f5c8d11fddc216c26.1703063544.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1852 bytes --]
On Wed, Dec 20, 2023 at 10:17:17AM +0100, Patrick Steinhardt wrote:
> When reading ref records of type "val1" we store its object ID in an
> allocated array. This results in an additional allocation for every
> single ref record we read, which is rather inefficient especially when
> iterating over refs.
>
> Refactor the code to instead use a static array of `GIT_MAX_RAWSZ`
> bytes. While this means that `struct ref_record` is bigger now, we
> typically do not store all refs in an array anyway and instead only
> handle a limited number of records at the same point in time.
>
> Using `git show-ref --quiet` in a repository with ~350k refs this leads
> to a significant drop in allocations. Before:
>
> HEAP SUMMARY:
> in use at exit: 21,098 bytes in 192 blocks
> total heap usage: 2,116,683 allocs, 2,116,491 frees, 76,098,060 bytes allocated
>
> After:
>
> HEAP SUMMARY:
> in use at exit: 21,098 bytes in 192 blocks
> total heap usage: 1,419,031 allocs, 1,418,839 frees, 62,145,036 bytes allocated
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
I screwed up the rebase. The following diff is required on top:
diff --git a/reftable/readwrite_test.c b/reftable/readwrite_test.c
index 56c0b4db5d..87b238105c 100644
--- a/reftable/readwrite_test.c
+++ b/reftable/readwrite_test.c
@@ -809,11 +809,10 @@ static void test_write_multiple_indices(void)
writer = reftable_new_writer(&strbuf_add_void, &writer_buf, &opts);
reftable_writer_set_limits(writer, 1, 1);
for (i = 0; i < 100; i++) {
- unsigned char hash[GIT_SHA1_RAWSZ] = {i};
struct reftable_ref_record ref = {
.update_index = 1,
.value_type = REFTABLE_REF_VAL1,
- .value.val1 = hash,
+ .value.val1 = {i},
};
strbuf_reset(&buf);
Will fix in v2 of this series.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 7/7] reftable/merged: transfer ownership of records when iterating
From: Patrick Steinhardt @ 2023-12-20 9:17 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1703063544.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2155 bytes --]
When iterating over recods with the merged iterator we put the records
into a priority queue before yielding them to the caller. This means
that we need to allocate the contents of these records before we can
pass them over to the caller.
The handover to the caller is quite inefficient though because we first
deallocate the record passed in by the caller and then copy over the new
record, which requires us to reallocate memory.
Refactor the code to instead transfer ownership of the new record to the
caller. So instead of reallocating all contents, we now release the old
record and then copy contents of the new record into place.
The following benchmark of `git show-ref --quiet` in a repository with
around 350k refs shows a clear improvement. Before:
HEAP SUMMARY:
in use at exit: 21,163 bytes in 193 blocks
total heap usage: 708,058 allocs, 707,865 frees, 36,783,255 bytes allocated
After:
HEAP SUMMARY:
in use at exit: 21,163 bytes in 193 blocks
total heap usage: 357,007 allocs, 356,814 frees, 24,193,602 bytes allocated
This shows that we now have roundabout a single allocation per record
that we're yielding from the iterator. Ideally, we'd also get rid of
this allocation so that the number of allocations doesn't scale with the
number of refs anymore. This would require some larger surgery though
because the memory is owned by the priority queue before transferring it
over to the caller.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/merged.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/reftable/merged.c b/reftable/merged.c
index a28bb99aaf..a52914d667 100644
--- a/reftable/merged.c
+++ b/reftable/merged.c
@@ -124,10 +124,12 @@ static int merged_iter_next_entry(struct merged_iter *mi,
reftable_record_release(&top.rec);
}
- reftable_record_copy_from(rec, &entry.rec, hash_size(mi->hash_id));
+ reftable_record_release(rec);
+ *rec = entry.rec;
done:
- reftable_record_release(&entry.rec);
+ if (err)
+ reftable_record_release(&entry.rec);
return err;
}
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 6/7] reftable/merged: really reuse buffers to compute record keys
From: Patrick Steinhardt @ 2023-12-20 9:17 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1703063544.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1486 bytes --]
In 829231dc20 (reftable/merged: reuse buffer to compute record keys,
2023-12-11), we have refactored the merged iterator to reuse a set of
buffers that it would otherwise have to reallocate on every single
iteration. Unfortunately, there was a brown-paper-bag-style bug here as
we continue to release these buffers after the iteration, and thus we
have essentially gained nothing.
Fix this performance issue by not releasing those buffers on iteration
anymore, where we instead rely on `merged_iter_close()` to release the
buffers for us.
Using `git show-ref --quiet` in a repository with ~350k refs this leads
to a significant drop in allocations. Before:
HEAP SUMMARY:
in use at exit: 21,163 bytes in 193 blocks
total heap usage: 1,410,148 allocs, 1,409,955 frees, 61,976,068 bytes allocated
After:
HEAP SUMMARY:
in use at exit: 21,163 bytes in 193 blocks
total heap usage: 708,058 allocs, 707,865 frees, 36,783,255 bytes allocated
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/merged.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/reftable/merged.c b/reftable/merged.c
index 556bb5c556..a28bb99aaf 100644
--- a/reftable/merged.c
+++ b/reftable/merged.c
@@ -128,8 +128,6 @@ static int merged_iter_next_entry(struct merged_iter *mi,
done:
reftable_record_release(&entry.rec);
- strbuf_release(&mi->entry_key);
- strbuf_release(&mi->key);
return err;
}
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 5/7] reftable/record: store "val2" hashes as static arrays
From: Patrick Steinhardt @ 2023-12-20 9:17 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1703063544.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 5214 bytes --]
Similar to the preceding commit, convert ref records of type "val2" to
store their object IDs in static arrays instead of allocating them for
every single record.
We're using the same benchmark as in the preceding commit, with `git
show-ref --quiet` in a repository with ~350k refs. This time around
though the effects aren't this huge. Before:
HEAP SUMMARY:
in use at exit: 21,163 bytes in 193 blocks
total heap usage: 1,419,040 allocs, 1,418,847 frees, 62,153,868 bytes allocated
After:
HEAP SUMMARY:
in use at exit: 21,163 bytes in 193 blocks
total heap usage: 1,410,148 allocs, 1,409,955 frees, 61,976,068 bytes allocated
This is because "val2"-type records are typically only stored for peeled
tags, and the number of annotated tags in the benchmark repository is
rather low. Still, it can be seen that this change leads to a reduction
of allocations overall, even if only a small one.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/readwrite_test.c | 12 ++++--------
reftable/record.c | 6 ------
reftable/record_test.c | 4 ----
reftable/reftable-record.h | 4 ++--
4 files changed, 6 insertions(+), 20 deletions(-)
diff --git a/reftable/readwrite_test.c b/reftable/readwrite_test.c
index 56c0b4db5d..900afc1b70 100644
--- a/reftable/readwrite_test.c
+++ b/reftable/readwrite_test.c
@@ -547,8 +547,6 @@ static void test_table_refs_for(int indexed)
uint8_t hash[GIT_SHA1_RAWSZ];
char fill[51] = { 0 };
char name[100];
- uint8_t hash1[GIT_SHA1_RAWSZ];
- uint8_t hash2[GIT_SHA1_RAWSZ];
struct reftable_ref_record ref = { NULL };
memset(hash, i, sizeof(hash));
@@ -558,11 +556,9 @@ static void test_table_refs_for(int indexed)
name[40] = 0;
ref.refname = name;
- set_test_hash(hash1, i / 4);
- set_test_hash(hash2, 3 + i / 4);
ref.value_type = REFTABLE_REF_VAL2;
- ref.value.val2.value = hash1;
- ref.value.val2.target_value = hash2;
+ set_test_hash(ref.value.val2.value, i / 4);
+ set_test_hash(ref.value.val2.target_value, 3 + i / 4);
/* 80 bytes / entry, so 3 entries per block. Yields 17
*/
@@ -570,8 +566,8 @@ static void test_table_refs_for(int indexed)
n = reftable_writer_add_ref(w, &ref);
EXPECT(n == 0);
- if (!memcmp(hash1, want_hash, GIT_SHA1_RAWSZ) ||
- !memcmp(hash2, want_hash, GIT_SHA1_RAWSZ)) {
+ if (!memcmp(ref.value.val2.value, want_hash, GIT_SHA1_RAWSZ) ||
+ !memcmp(ref.value.val2.target_value, want_hash, GIT_SHA1_RAWSZ)) {
want_names[want_names_len++] = xstrdup(name);
}
}
diff --git a/reftable/record.c b/reftable/record.c
index a67a6b4d8a..5c3fbb7b2a 100644
--- a/reftable/record.c
+++ b/reftable/record.c
@@ -222,9 +222,7 @@ static void reftable_ref_record_copy_from(void *rec, const void *src_rec,
memcpy(ref->value.val1, src->value.val1, hash_size);
break;
case REFTABLE_REF_VAL2:
- ref->value.val2.value = reftable_malloc(hash_size);
memcpy(ref->value.val2.value, src->value.val2.value, hash_size);
- ref->value.val2.target_value = reftable_malloc(hash_size);
memcpy(ref->value.val2.target_value,
src->value.val2.target_value, hash_size);
break;
@@ -298,8 +296,6 @@ void reftable_ref_record_release(struct reftable_ref_record *ref)
reftable_free(ref->value.symref);
break;
case REFTABLE_REF_VAL2:
- reftable_free(ref->value.val2.target_value);
- reftable_free(ref->value.val2.value);
break;
case REFTABLE_REF_VAL1:
break;
@@ -401,11 +397,9 @@ static int reftable_ref_record_decode(void *rec, struct strbuf key,
return -1;
}
- r->value.val2.value = reftable_malloc(hash_size);
memcpy(r->value.val2.value, in.buf, hash_size);
string_view_consume(&in, hash_size);
- r->value.val2.target_value = reftable_malloc(hash_size);
memcpy(r->value.val2.target_value, in.buf, hash_size);
string_view_consume(&in, hash_size);
break;
diff --git a/reftable/record_test.c b/reftable/record_test.c
index 5c94d26e35..2876db7d27 100644
--- a/reftable/record_test.c
+++ b/reftable/record_test.c
@@ -122,11 +122,7 @@ static void test_reftable_ref_record_roundtrip(void)
set_hash(in.u.ref.value.val1, 1);
break;
case REFTABLE_REF_VAL2:
- in.u.ref.value.val2.value =
- reftable_malloc(GIT_SHA1_RAWSZ);
set_hash(in.u.ref.value.val2.value, 1);
- in.u.ref.value.val2.target_value =
- reftable_malloc(GIT_SHA1_RAWSZ);
set_hash(in.u.ref.value.val2.target_value, 2);
break;
case REFTABLE_REF_SYMREF:
diff --git a/reftable/reftable-record.h b/reftable/reftable-record.h
index 44b5125445..83d252ec2c 100644
--- a/reftable/reftable-record.h
+++ b/reftable/reftable-record.h
@@ -40,8 +40,8 @@ struct reftable_ref_record {
union {
unsigned char val1[GIT_MAX_RAWSZ];
struct {
- uint8_t *value; /* first value, malloced hash */
- uint8_t *target_value; /* second value, malloced hash */
+ unsigned char value[GIT_MAX_RAWSZ]; /* first hash */
+ unsigned char target_value[GIT_MAX_RAWSZ]; /* second hash */
} val2;
char *symref; /* referent, malloced 0-terminated string */
} value;
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 4/7] reftable/record: store "val1" hashes as static arrays
From: Patrick Steinhardt @ 2023-12-20 9:17 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1703063544.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 7954 bytes --]
When reading ref records of type "val1" we store its object ID in an
allocated array. This results in an additional allocation for every
single ref record we read, which is rather inefficient especially when
iterating over refs.
Refactor the code to instead use a static array of `GIT_MAX_RAWSZ`
bytes. While this means that `struct ref_record` is bigger now, we
typically do not store all refs in an array anyway and instead only
handle a limited number of records at the same point in time.
Using `git show-ref --quiet` in a repository with ~350k refs this leads
to a significant drop in allocations. Before:
HEAP SUMMARY:
in use at exit: 21,098 bytes in 192 blocks
total heap usage: 2,116,683 allocs, 2,116,491 frees, 76,098,060 bytes allocated
After:
HEAP SUMMARY:
in use at exit: 21,098 bytes in 192 blocks
total heap usage: 1,419,031 allocs, 1,418,839 frees, 62,145,036 bytes allocated
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/block_test.c | 4 +---
reftable/merged_test.c | 16 ++++++----------
reftable/readwrite_test.c | 11 +++--------
reftable/record.c | 3 ---
reftable/record_test.c | 1 -
reftable/reftable-record.h | 2 +-
reftable/stack_test.c | 2 --
7 files changed, 11 insertions(+), 28 deletions(-)
diff --git a/reftable/block_test.c b/reftable/block_test.c
index c00bbc8aed..dedb05c7d8 100644
--- a/reftable/block_test.c
+++ b/reftable/block_test.c
@@ -49,13 +49,11 @@ static void test_block_read_write(void)
for (i = 0; i < N; i++) {
char name[100];
- uint8_t hash[GIT_SHA1_RAWSZ];
snprintf(name, sizeof(name), "branch%02d", i);
- memset(hash, i, sizeof(hash));
rec.u.ref.refname = name;
rec.u.ref.value_type = REFTABLE_REF_VAL1;
- rec.u.ref.value.val1 = hash;
+ memset(rec.u.ref.value.val1, i, GIT_SHA1_RAWSZ);
names[i] = xstrdup(name);
n = block_writer_add(&bw, &rec);
diff --git a/reftable/merged_test.c b/reftable/merged_test.c
index d08c16abef..b3927a5d73 100644
--- a/reftable/merged_test.c
+++ b/reftable/merged_test.c
@@ -123,13 +123,11 @@ static void readers_destroy(struct reftable_reader **readers, size_t n)
static void test_merged_between(void)
{
- uint8_t hash1[GIT_SHA1_RAWSZ] = { 1, 2, 3, 0 };
-
struct reftable_ref_record r1[] = { {
.refname = "b",
.update_index = 1,
.value_type = REFTABLE_REF_VAL1,
- .value.val1 = hash1,
+ .value.val1 = { 1, 2, 3, 0 },
} };
struct reftable_ref_record r2[] = { {
.refname = "a",
@@ -165,26 +163,24 @@ static void test_merged_between(void)
static void test_merged(void)
{
- uint8_t hash1[GIT_SHA1_RAWSZ] = { 1 };
- uint8_t hash2[GIT_SHA1_RAWSZ] = { 2 };
struct reftable_ref_record r1[] = {
{
.refname = "a",
.update_index = 1,
.value_type = REFTABLE_REF_VAL1,
- .value.val1 = hash1,
+ .value.val1 = { 1 },
},
{
.refname = "b",
.update_index = 1,
.value_type = REFTABLE_REF_VAL1,
- .value.val1 = hash1,
+ .value.val1 = { 1 },
},
{
.refname = "c",
.update_index = 1,
.value_type = REFTABLE_REF_VAL1,
- .value.val1 = hash1,
+ .value.val1 = { 1 },
}
};
struct reftable_ref_record r2[] = { {
@@ -197,13 +193,13 @@ static void test_merged(void)
.refname = "c",
.update_index = 3,
.value_type = REFTABLE_REF_VAL1,
- .value.val1 = hash2,
+ .value.val1 = { 2 },
},
{
.refname = "d",
.update_index = 3,
.value_type = REFTABLE_REF_VAL1,
- .value.val1 = hash1,
+ .value.val1 = { 1 },
},
};
diff --git a/reftable/readwrite_test.c b/reftable/readwrite_test.c
index 9c16e0504e..56c0b4db5d 100644
--- a/reftable/readwrite_test.c
+++ b/reftable/readwrite_test.c
@@ -60,18 +60,15 @@ static void write_table(char ***names, struct strbuf *buf, int N,
*names = reftable_calloc(sizeof(char *) * (N + 1));
reftable_writer_set_limits(w, update_index, update_index);
for (i = 0; i < N; i++) {
- uint8_t hash[GIT_SHA256_RAWSZ] = { 0 };
char name[100];
int n;
- set_test_hash(hash, i);
-
snprintf(name, sizeof(name), "refs/heads/branch%02d", i);
ref.refname = name;
ref.update_index = update_index;
ref.value_type = REFTABLE_REF_VAL1;
- ref.value.val1 = hash;
+ set_test_hash(ref.value.val1, i);
(*names)[i] = xstrdup(name);
n = reftable_writer_add_ref(w, &ref);
@@ -675,11 +672,10 @@ static void test_write_object_id_min_length(void)
struct strbuf buf = STRBUF_INIT;
struct reftable_writer *w =
reftable_new_writer(&strbuf_add_void, &buf, &opts);
- uint8_t hash[GIT_SHA1_RAWSZ] = {42};
struct reftable_ref_record ref = {
.update_index = 1,
.value_type = REFTABLE_REF_VAL1,
- .value.val1 = hash,
+ .value.val1 = {42},
};
int err;
int i;
@@ -711,11 +707,10 @@ static void test_write_object_id_length(void)
struct strbuf buf = STRBUF_INIT;
struct reftable_writer *w =
reftable_new_writer(&strbuf_add_void, &buf, &opts);
- uint8_t hash[GIT_SHA1_RAWSZ] = {42};
struct reftable_ref_record ref = {
.update_index = 1,
.value_type = REFTABLE_REF_VAL1,
- .value.val1 = hash,
+ .value.val1 = {42},
};
int err;
int i;
diff --git a/reftable/record.c b/reftable/record.c
index 5e258c734b..a67a6b4d8a 100644
--- a/reftable/record.c
+++ b/reftable/record.c
@@ -219,7 +219,6 @@ static void reftable_ref_record_copy_from(void *rec, const void *src_rec,
case REFTABLE_REF_DELETION:
break;
case REFTABLE_REF_VAL1:
- ref->value.val1 = reftable_malloc(hash_size);
memcpy(ref->value.val1, src->value.val1, hash_size);
break;
case REFTABLE_REF_VAL2:
@@ -303,7 +302,6 @@ void reftable_ref_record_release(struct reftable_ref_record *ref)
reftable_free(ref->value.val2.value);
break;
case REFTABLE_REF_VAL1:
- reftable_free(ref->value.val1);
break;
case REFTABLE_REF_DELETION:
break;
@@ -394,7 +392,6 @@ static int reftable_ref_record_decode(void *rec, struct strbuf key,
return -1;
}
- r->value.val1 = reftable_malloc(hash_size);
memcpy(r->value.val1, in.buf, hash_size);
string_view_consume(&in, hash_size);
break;
diff --git a/reftable/record_test.c b/reftable/record_test.c
index 70ae78feca..5c94d26e35 100644
--- a/reftable/record_test.c
+++ b/reftable/record_test.c
@@ -119,7 +119,6 @@ static void test_reftable_ref_record_roundtrip(void)
case REFTABLE_REF_DELETION:
break;
case REFTABLE_REF_VAL1:
- in.u.ref.value.val1 = reftable_malloc(GIT_SHA1_RAWSZ);
set_hash(in.u.ref.value.val1, 1);
break;
case REFTABLE_REF_VAL2:
diff --git a/reftable/reftable-record.h b/reftable/reftable-record.h
index f7eb2d6015..44b5125445 100644
--- a/reftable/reftable-record.h
+++ b/reftable/reftable-record.h
@@ -38,7 +38,7 @@ struct reftable_ref_record {
#define REFTABLE_NR_REF_VALUETYPES 4
} value_type;
union {
- uint8_t *val1; /* malloced hash. */
+ unsigned char val1[GIT_MAX_RAWSZ];
struct {
uint8_t *value; /* first value, malloced hash */
uint8_t *target_value; /* second value, malloced hash */
diff --git a/reftable/stack_test.c b/reftable/stack_test.c
index 14a3fc11ee..feab49d7f7 100644
--- a/reftable/stack_test.c
+++ b/reftable/stack_test.c
@@ -463,7 +463,6 @@ static void test_reftable_stack_add(void)
refs[i].refname = xstrdup(buf);
refs[i].update_index = i + 1;
refs[i].value_type = REFTABLE_REF_VAL1;
- refs[i].value.val1 = reftable_malloc(GIT_SHA1_RAWSZ);
set_test_hash(refs[i].value.val1, i);
logs[i].refname = xstrdup(buf);
@@ -600,7 +599,6 @@ static void test_reftable_stack_tombstone(void)
refs[i].update_index = i + 1;
if (i % 2 == 0) {
refs[i].value_type = REFTABLE_REF_VAL1;
- refs[i].value.val1 = reftable_malloc(GIT_SHA1_RAWSZ);
set_test_hash(refs[i].value.val1, i);
}
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 3/7] reftable/record: constify some parts of the interface
From: Patrick Steinhardt @ 2023-12-20 9:17 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1703063544.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2714 bytes --]
We're about to convert reftable records to stop storing their object IDs
as allocated hashes. Prepare for this refactoring by constifying some
parts of the interface that will be impacted by this.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/record.c | 8 ++++----
reftable/reftable-record.h | 4 ++--
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/reftable/record.c b/reftable/record.c
index fbaa1fbef5..5e258c734b 100644
--- a/reftable/record.c
+++ b/reftable/record.c
@@ -76,7 +76,7 @@ int reftable_is_block_type(uint8_t typ)
return 0;
}
-uint8_t *reftable_ref_record_val1(const struct reftable_ref_record *rec)
+const unsigned char *reftable_ref_record_val1(const struct reftable_ref_record *rec)
{
switch (rec->value_type) {
case REFTABLE_REF_VAL1:
@@ -88,7 +88,7 @@ uint8_t *reftable_ref_record_val1(const struct reftable_ref_record *rec)
}
}
-uint8_t *reftable_ref_record_val2(const struct reftable_ref_record *rec)
+const unsigned char *reftable_ref_record_val2(const struct reftable_ref_record *rec)
{
switch (rec->value_type) {
case REFTABLE_REF_VAL2:
@@ -242,7 +242,7 @@ static char hexdigit(int c)
return 'a' + (c - 10);
}
-static void hex_format(char *dest, uint8_t *src, int hash_size)
+static void hex_format(char *dest, const unsigned char *src, int hash_size)
{
assert(hash_size > 0);
if (src) {
@@ -1164,7 +1164,7 @@ int reftable_record_equal(struct reftable_record *a, struct reftable_record *b,
reftable_record_data(a), reftable_record_data(b), hash_size);
}
-static int hash_equal(uint8_t *a, uint8_t *b, int hash_size)
+static int hash_equal(const unsigned char *a, const unsigned char *b, int hash_size)
{
if (a && b)
return !memcmp(a, b, hash_size);
diff --git a/reftable/reftable-record.h b/reftable/reftable-record.h
index 67104f8fbf..f7eb2d6015 100644
--- a/reftable/reftable-record.h
+++ b/reftable/reftable-record.h
@@ -49,11 +49,11 @@ struct reftable_ref_record {
/* Returns the first hash, or NULL if `rec` is not of type
* REFTABLE_REF_VAL1 or REFTABLE_REF_VAL2. */
-uint8_t *reftable_ref_record_val1(const struct reftable_ref_record *rec);
+const unsigned char *reftable_ref_record_val1(const struct reftable_ref_record *rec);
/* Returns the second hash, or NULL if `rec` is not of type
* REFTABLE_REF_VAL2. */
-uint8_t *reftable_ref_record_val2(const struct reftable_ref_record *rec);
+const unsigned char *reftable_ref_record_val2(const struct reftable_ref_record *rec);
/* returns whether 'ref' represents a deletion */
int reftable_ref_record_is_deletion(const struct reftable_ref_record *ref);
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 2/7] reftable/writer: fix index corruption when writing multiple indices
From: Patrick Steinhardt @ 2023-12-20 9:17 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1703063544.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 5241 bytes --]
Each reftable may contain multiple types of blocks for refs, objects and
reflog records, where each of these may have an index that makes it more
efficient to find the records. It was observed that the index for log
records can become corrupted under certain circumstances, where the
first entry of the index points into the object index instead of to the
log records.
As it turns out, this corruption can occur whenever we write a log index
as well as at least one additional index. Writing records and their index
is basically a two-step process:
1. We write all blocks for the corresponding record. Each block that
gets written is added to a list of blocks to index.
2. Once all blocks were written we finish the section. If at least two
blocks have been added to the list of blocks to index then we will
now write the index for those blocks and flush it, as well.
When we have a very large number of blocks then we may decide to write a
multi-level index, which is why we also keep track of the list of the
index blocks in the same way as we previously kept track of the blocks
to index.
Now when we have finished writing all index blocks we clear the index
and flush the last block to disk. This is done in the wrong order though
because flushing the block to disk will re-add it to the list of blocks
to be indexed. The result is that the next section we are about to write
will have an entry in the list of blocks to index that points to the
last block of the preceding section's index, which will corrupt the log
index.
Fix this corruption by clearing the index after having written the last
block.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/readwrite_test.c | 80 +++++++++++++++++++++++++++++++++++++++
reftable/writer.c | 4 +-
2 files changed, 82 insertions(+), 2 deletions(-)
diff --git a/reftable/readwrite_test.c b/reftable/readwrite_test.c
index 278663f22d..9c16e0504e 100644
--- a/reftable/readwrite_test.c
+++ b/reftable/readwrite_test.c
@@ -798,6 +798,85 @@ static void test_write_key_order(void)
strbuf_release(&buf);
}
+static void test_write_multiple_indices(void)
+{
+ struct reftable_write_options opts = {
+ .block_size = 100,
+ };
+ struct strbuf writer_buf = STRBUF_INIT, buf = STRBUF_INIT;
+ struct reftable_block_source source = { 0 };
+ struct reftable_iterator it = { 0 };
+ const struct reftable_stats *stats;
+ struct reftable_writer *writer;
+ struct reftable_reader *reader;
+ int err, i;
+
+ writer = reftable_new_writer(&strbuf_add_void, &writer_buf, &opts);
+ reftable_writer_set_limits(writer, 1, 1);
+ for (i = 0; i < 100; i++) {
+ unsigned char hash[GIT_SHA1_RAWSZ] = {i};
+ struct reftable_ref_record ref = {
+ .update_index = 1,
+ .value_type = REFTABLE_REF_VAL1,
+ .value.val1 = hash,
+ };
+
+ strbuf_reset(&buf);
+ strbuf_addf(&buf, "refs/heads/%04d", i);
+ ref.refname = buf.buf,
+
+ err = reftable_writer_add_ref(writer, &ref);
+ EXPECT_ERR(err);
+ }
+
+ for (i = 0; i < 100; i++) {
+ unsigned char hash[GIT_SHA1_RAWSZ] = {i};
+ struct reftable_log_record log = {
+ .update_index = 1,
+ .value_type = REFTABLE_LOG_UPDATE,
+ .value.update = {
+ .old_hash = hash,
+ .new_hash = hash,
+ },
+ };
+
+ strbuf_reset(&buf);
+ strbuf_addf(&buf, "refs/heads/%04d", i);
+ log.refname = buf.buf,
+
+ err = reftable_writer_add_log(writer, &log);
+ EXPECT_ERR(err);
+ }
+
+ reftable_writer_close(writer);
+
+ /*
+ * The written data should be sufficiently large to result in indices
+ * for each of the block types.
+ */
+ stats = reftable_writer_stats(writer);
+ EXPECT(stats->ref_stats.index_offset > 0);
+ EXPECT(stats->obj_stats.index_offset > 0);
+ EXPECT(stats->log_stats.index_offset > 0);
+
+ block_source_from_strbuf(&source, &writer_buf);
+ err = reftable_new_reader(&reader, &source, "filename");
+ EXPECT_ERR(err);
+
+ /*
+ * Seeking the log uses the log index now. In case there is any
+ * confusion regarding indices we would notice here.
+ */
+ err = reftable_reader_seek_log(reader, &it, "");
+ EXPECT_ERR(err);
+
+ reftable_iterator_destroy(&it);
+ reftable_writer_free(writer);
+ reftable_reader_free(reader);
+ strbuf_release(&writer_buf);
+ strbuf_release(&buf);
+}
+
static void test_corrupt_table_empty(void)
{
struct strbuf buf = STRBUF_INIT;
@@ -847,5 +926,6 @@ int readwrite_test_main(int argc, const char *argv[])
RUN_TEST(test_log_overflow);
RUN_TEST(test_write_object_id_length);
RUN_TEST(test_write_object_id_min_length);
+ RUN_TEST(test_write_multiple_indices);
return 0;
}
diff --git a/reftable/writer.c b/reftable/writer.c
index 2e322a5683..ee4590e20f 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -432,12 +432,12 @@ static int writer_finish_section(struct reftable_writer *w)
reftable_free(idx);
}
- writer_clear_index(w);
-
err = writer_flush_block(w);
if (err < 0)
return err;
+ writer_clear_index(w);
+
bstats = writer_reftable_block_stats(w, typ);
bstats->index_blocks = w->stats.idx_stats.blocks - before_blocks;
bstats->index_offset = index_start;
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 1/7] reftable/stack: do not overwrite errors when compacting
From: Patrick Steinhardt @ 2023-12-20 9:17 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1703063544.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1854 bytes --]
In order to compact multiple stacks we iterate through the merged ref
and log records. When there is any error either when reading the records
from the old merged table or when writing the records to the new table
then we break out of the respective loops. When breaking out of the loop
for the ref records though the error code will be overwritten, which may
cause us to inadvertently skip over bad ref records. In the worst case,
this can lead to a compacted stack that is missing records.
Fix the code by using `goto done` instead so that any potential error
codes are properly returned to the caller.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/stack.c | 20 ++++++++------------
1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/reftable/stack.c b/reftable/stack.c
index 16bab82063..8729508dc3 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -801,18 +801,16 @@ static int stack_write_compact(struct reftable_stack *st,
err = 0;
break;
}
- if (err < 0) {
- break;
- }
+ if (err < 0)
+ goto done;
if (first == 0 && reftable_ref_record_is_deletion(&ref)) {
continue;
}
err = reftable_writer_add_ref(wr, &ref);
- if (err < 0) {
- break;
- }
+ if (err < 0)
+ goto done;
entries++;
}
reftable_iterator_destroy(&it);
@@ -827,9 +825,8 @@ static int stack_write_compact(struct reftable_stack *st,
err = 0;
break;
}
- if (err < 0) {
- break;
- }
+ if (err < 0)
+ goto done;
if (first == 0 && reftable_log_record_is_deletion(&log)) {
continue;
}
@@ -845,9 +842,8 @@ static int stack_write_compact(struct reftable_stack *st,
}
err = reftable_writer_add_log(wr, &log);
- if (err < 0) {
- break;
- }
+ if (err < 0)
+ goto done;
entries++;
}
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 0/7] reftable: fixes and optimizations (pt.2)
From: Patrick Steinhardt @ 2023-12-20 9:17 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 1611 bytes --]
Hi,
this is the second set of patches that fixes bugs and performs some
slight memory optimizations. The series builds on top of c0cadb0576,
which has been merged to `next`.
The series is structured as follows:
- Patch 1: some hardening to not corrupt the reftable stack on
compaction.
- Patch 2: fix corruption of a reftable when writing multiple indices.
- Patches 3 - 7: various smallish refactorings to optimize memory
usage. Overall these reduce allocations when iterating many refs by
almost 85%.
Thanks in advance for your review!
Patrick
Patrick Steinhardt (7):
reftable/stack: do not overwrite errors when compacting
reftable/writer: fix index corruption when writing multiple indices
reftable/record: constify some parts of the interface
reftable/record: store "val1" hashes as static arrays
reftable/record: store "val2" hashes as static arrays
reftable/merged: really reuse buffers to compute record keys
reftable/merged: transfer ownership of records when iterating
reftable/block_test.c | 4 +-
reftable/merged.c | 8 +--
reftable/merged_test.c | 16 +++---
reftable/readwrite_test.c | 103 +++++++++++++++++++++++++++++++------
reftable/record.c | 17 ++----
reftable/record_test.c | 5 --
reftable/reftable-record.h | 10 ++--
reftable/stack.c | 20 +++----
reftable/stack_test.c | 2 -
reftable/writer.c | 4 +-
10 files changed, 117 insertions(+), 72 deletions(-)
base-commit: c0cadb0576d4920915eb3bd38a7d1abfcbd25f98
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] rebase-interactive: show revert option and add single letter shortcut
From: Michael Lohmann @ 2023-12-20 8:53 UTC (permalink / raw)
To: gitster
Cc: Johannes.Schindelin, git, mi.al.lohmann, mial.lohmann,
phillip.wood123, phillip.wood
In-Reply-To: <xmqqmsu7e4hp.fsf@gitster.g>
Hi Junio,
On 18. Dec 2023, at 19:43, Junio C Hamano <gitster@pobox.com> wrote:
> Michael Lohmann <mial.lohmann@gmail.com> writes:
>
>> A `revert` in an interactive rebase can be useful, e.g. if a faulty
>> commit was pushed to the main branch already, so you can't just drop it.
>
> But wouldn't that be typically done simply by running "git revert",
> totally outside the context of "git rebase -i"?
>
> Interactive rebase is more geared toward rearranging an already
> built history *after* such a "git revert" is made and possibly other
> commits are made either before or after that commit that was created
> by "git revert".
Right - I recently found myself in a situation where a coworker merged a
faulty commit leading to a build failure and (given that only the two of
us actively worked on that project) we coordinated that he would prepare
a proper fix, while I wanted to rebase my current feature branch on
master, but with that commit reverted. For the sake of a clean history I
preferred to have the revert commit right at the merge-base, instead of
somewhere in the middle of all of my commits.
But you are right - if there are more than two people working on a
project, the typical way of properly doing this would be to revert the
offending commit in the master branch until a fix is available.
You can also do the revert you want to create in your feature branch and
directly update the master:
- `git rebase -i origin/master`
- add the following two lines to the top of the todo list:
revert B
exec git push origin @:refs/heads/revert-commit
Instead of
- `git switch origin/master -c revert-commit`
- `git revert B`
- `git push origin HEAD`
- `git switch -`
- `git rebase revert-commit`
So with the interactive revert you can create the "revert-commit” branch
"directly from within your own branch”. But indeed - it really is not
needed...
> A much cleaner way to structure your branch is not to muck with such
> tentative changes *on* the branch you eventually want to store the
> final result on. Fork another branch and rebase B away:
I really like that workflow! I’ll adapt it :)
So in total: I don’t think documenting this is necessary (that is also
why my first message was not directly the patch, but the question why
this is undocumented) and it might even lead to the unclean workflow
that I ended up having, so even from that perspective it might not be a
good thing.
Thank you very much for this very detailed explanation of the workflow!
Michael
P.S. I am sorry - the first reply only went directly to Junio and not the
mailing list
^ permalink raw reply
* Re: [PATCH] rebase: use strvec_pushf() for format-patch revisions
From: René Scharfe @ 2023-12-20 8:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
In-Reply-To: <xmqqmsu6ce0u.fsf@gitster.g>
Am 19.12.23 um 18:12 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> In run_am(), a strbuf is used to create a revision argument that is then
>> added to the argument list for git format-patch using strvec_push().
>> Use strvec_pushf() to add it directly instead, simplifying the code.
>>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>
> Makes sense. Between the location of the original strbuf_addf()
> call and the new strvec_pushf() call, nobody mucks with *opts so
> this change won't affect the correctness. We no longer use the
> extra strbuf, and upon failing to open the rebased-patches file,
> we no longer leak the contents of it. Good.
Ha! I didn't even notice that leak on error. Perhaps the two release
calls at the end gave me the illusion of cleanliness? Or more likely
the opportunity to use strvec_pushf() grabbed my full attention (tunnel
vision).
>
>> @@ -615,34 +614,32 @@ static int run_am(struct rebase_options *opts)
>> return run_command(&am);
>> }
>>
>> - strbuf_addf(&revisions, "%s...%s",
>> - oid_to_hex(opts->root ?
>> - /* this is now equivalent to !opts->upstream */
>> - &opts->onto->object.oid :
>> - &opts->upstream->object.oid),
>> - oid_to_hex(&opts->orig_head->object.oid));
>> -
>> rebased_patches = xstrdup(git_path("rebased-patches"));
>> format_patch.out = open(rebased_patches,
>> O_WRONLY | O_CREAT | O_TRUNC, 0666);
>> if (format_patch.out < 0) {
>> status = error_errno(_("could not open '%s' for writing"),
>> rebased_patches);
>> free(rebased_patches);
>> strvec_clear(&am.args);
>> return status;
>> }
>>
>> format_patch.git_cmd = 1;
>> strvec_pushl(&format_patch.args, "format-patch", "-k", "--stdout",
>> "--full-index", "--cherry-pick", "--right-only",
>> "--default-prefix", "--no-renames",
>> "--no-cover-letter", "--pretty=mboxrd", "--topo-order",
>> "--no-base", NULL);
>> if (opts->git_format_patch_opt.len)
>> strvec_split(&format_patch.args,
>> opts->git_format_patch_opt.buf);
>> - strvec_push(&format_patch.args, revisions.buf);
>> + strvec_pushf(&format_patch.args, "%s...%s",
>> + oid_to_hex(opts->root ?
>> + /* this is now equivalent to !opts->upstream */
>> + &opts->onto->object.oid :
>> + &opts->upstream->object.oid),
>> + oid_to_hex(&opts->orig_head->object.oid));
>> if (opts->restrict_revision)
>> strvec_pushf(&format_patch.args, "^%s",
>> oid_to_hex(&opts->restrict_revision->object.oid));
>> @@ -665,10 +662,8 @@ static int run_am(struct rebase_options *opts)
>> "As a result, git cannot rebase them."),
>> opts->revisions);
>>
>> - strbuf_release(&revisions);
>> return status;
>> }
>> - strbuf_release(&revisions);
>>
>> am.in = open(rebased_patches, O_RDONLY);
>> if (am.in < 0) {
>> --
>> 2.43.0
^ permalink raw reply
* [PATCH] Documentation/git-merge.txt: fix reference to synopsys
From: Michael Lohmann @ 2023-12-20 7:05 UTC (permalink / raw)
To: git; +Cc: Michael Lohmann
437591a9d738 changed the synopsys from two separate lines for `--abort`
and `--continue` to a single line (and it also simultaneously added
`--quit`). That way the "enumeration" of the syntax for `--continue` is
no longer valid. Since `--quit` is now also part of the same syntax
line, a general statement cannot be made any more. Instead of trying to
enumerate the synopsys, be explicit in the limitations of when
respective actions are valid.
This change also groups `--abort` and `--continue` together when
explaining the circumstances under which they can be run in order to
avoid duplication.
Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
---
Documentation/git-merge.txt | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index e8ab340319..d8863cc943 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -46,21 +46,20 @@ a log message from the user describing the changes. Before the operation,
D---E---F---G---H master
------------
-The second syntax ("`git merge --abort`") can only be run after the
-merge has resulted in conflicts. 'git merge --abort' will abort the
-merge process and try to reconstruct the pre-merge state. However,
-if there were uncommitted changes when the merge started (and
-especially if those changes were further modified after the merge
-was started), 'git merge --abort' will in some cases be unable to
-reconstruct the original (pre-merge) changes. Therefore:
+It is possible that a merge failure will prevent this process from being
+completely automatic. "`git merge --continue`" and "`git merge --abort`"
+can only be run after the merge has resulted in conflicts.
+
+'git merge --abort' will abort the merge process and try to reconstruct
+the pre-merge state. However, if there were uncommitted changes when the
+merge started (and especially if those changes were further modified
+after the merge was started), 'git merge --abort' will in some cases be
+unable to reconstruct the original (pre-merge) changes. Therefore:
*Warning*: Running 'git merge' with non-trivial uncommitted changes is
discouraged: while possible, it may leave you in a state that is hard to
back out of in the case of a conflict.
-The third syntax ("`git merge --continue`") can only be run after the
-merge has resulted in conflicts.
-
OPTIONS
-------
:git-merge: 1
--
2.39.3 (Apple Git-145)
^ permalink raw reply related
* Re: Re: [PATCH 0/1] revert/cherry-pick: add --show-current-patch option
From: Michael Lohmann @ 2023-12-20 6:51 UTC (permalink / raw)
To: phillip.wood123; +Cc: git, mi.al.lohmann, mial.lohmann, newren, phillip.wood
In-Reply-To: <42ff6b11-f991-4a6d-ad68-ca8c5a3cd735@gmail.com>
Hi Phillip
On 18/12/2023 16:42, Phillip Wood wrote:
> Thanks for bringing this up I agree it can be very helpful to look at
> the original commit when resolving cherry-pick and revert conflicts.
> I'm in two minds about this change though - I wonder if it'd be better
> to improve the documentation for CHERRY_PICK_HEAD and REVERT_HEAD and
> tell users to run "git show CHERRY_PICK_HEAD" instead. I think the
> main reason we have a "--show-current-patch" option for "rebase" is
> that there are two different implementations of that command and the
> patched-based one of them does not support REBASE_HEAD. That reasoning
> does not apply to "cherry-pick" and "revert" and
> "--show-current-patch" suggests a patch-based implementation which is
> also not the case for these commands.
I appreciate the urge of limiting the interface to the minimum needed
and not to duplicate functionality that already exists. On the other
hand, this would
a) grant the user the same experience, not having to wonder about
implementation details such as different backends for rebase, but not
for revert/cherry-pick and
b) (I know it is more indicative of me, but:) when I am looking for a
feature in software and I look into the respective man page I tend to
focus first on the synopsis instead of reading the whole page (or
sometimes I even just rely on the shell autocompletion for
discoverability).
So yes, mentioning REVERT_HEAD and CHERRY_PICK_HEAD in the respective
docs would technically be sufficient, but I don't think it is as
discoverable to an average user (who does not know about the details of
all the existing pseudo refs) as a toplevel action would be. But an
assessment of the pros and cons is not on me to decide.
I have to be honest: I have troubles distinguishing a "patch" and a
"diff", the latter of which `git show <commit>` shows according to the
documentation ("For commits it shows the log message and textual
diff."), though my understanding was that a patch is a diff + context
lines, which is what `git show` actually shows... I think this is
probably why I don't feel so strong about the potential loose usage of
the word here.
Also the documentation of cherry-pick already uses the word "patch" in a
(according to my understanding from a technical perspective) sloppy (but
from a layman's point of view probably nevertheless helpful) way:
> The following sequence attempts to backport a patch, bails out because
> the code the patch applies to has changed too much, and then tries
> again, this time exercising more care about matching up context lines.
>
> ------------
> $ git cherry-pick topic^ <1>
> $ git diff <2>
> $ git cherry-pick --abort <3>
> $ git cherry-pick -Xpatience topic^ <4>
> ------------
> <1> apply the change that would be shown by `git show topic^`.
> In this example, the patch does not apply cleanly, so
> information about the conflict is written to the index and
> working tree and no new commit results.
Should that also be rephrased?
Out of curiosity: The following from the rebase docs seems to imply that
the apply backend will probably be removed in the future:
> --apply
> Use applying strategies to rebase (calling git-am
> internally). This option may become a no-op in the future
> once the merge backend handles everything the apply one
> does.
But I would expect the `rebase --show-current-patch` still to be
working. Would that only be a legacy compatibility flag and instead also
for rebases the recommended option would be to run
`git show REBASE_HEAD`?
Best Wishes
Michael
^ permalink raw reply
* Re: [PATCH v3 0/2] completion: refactor and support reftables backend
From: Junio C Hamano @ 2023-12-20 0:48 UTC (permalink / raw)
To: Stan Hu; +Cc: git, Patrick Steinhardt, Christian Couder
In-Reply-To: <cover.1703022850.git.stanhu@gmail.com>
Stan Hu <stanhu@gmail.com> writes:
> This patch series addresses the review feedback:
>
> 1. Renames the '__git_ref_exists' helper to '__git_pseudoref_exists' in
> the first refactor patch.
>
> Stan Hu (2):
> completion: refactor existence checks for pseudorefs
> completion: support pseudoref existence checks for reftables
>
> contrib/completion/git-completion.bash | 43 +++++++++++++++++++++++---
> 1 file changed, 38 insertions(+), 5 deletions(-)
Thanks. Will queue.
^ permalink raw reply
* Re: [PATCH v2] Teach git apply to respect core.fileMode settings
From: Junio C Hamano @ 2023-12-19 22:21 UTC (permalink / raw)
To: Chandra Pratap via GitGitGadget; +Cc: git, Chandra Pratap, Chandra Pratap
In-Reply-To: <pull.1620.v2.git.1703010646036.gitgitgadget@gmail.com>
"Chandra Pratap via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Chandra Pratap <chandrapratap3519@gmail.com>
>
> When applying a patch that adds an executable file, git apply
> ignores the core.fileMode setting (core.fileMode in git config
> specifies whether the executable bit on files in the working tree
> should be honored or not) resulting in warnings like:
>
> warning: script.sh has type 100644, expected 100755
>
> even when core.fileMode is set to false, which is undesired. This
> is extra true for systems like Windows, which don't rely on "lsat()".
>
> Fix this by inferring the correct file mode from the existing
> index entry when core.filemode is set to false. The added
> test case helps verify the change and prevents future regression.
Thanks. Has _this_ particular iteration of the patch been reviewed
by Dscho? Otherwise ...
> Reviewed-by: Johannes Schindelin <johannes.schindelin@gmail.com>
... this line is a bit problematic. Just double-checking.
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---
> diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh
> index e7a7295f1b6..73fc472b246 100755
> --- a/t/t4129-apply-samemode.sh
> +++ b/t/t4129-apply-samemode.sh
> @@ -101,4 +101,29 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree
> )
> '
>
> +test_expect_success 'ensure git apply respects core.fileMode' '
> + test_config core.fileMode false &&
> + echo true >script.sh &&
> + git add --chmod=+x script.sh &&
> + git ls-files -s script.sh | grep "^100755" &&
> + test_tick && git commit -m "Add script" &&
> + git ls-tree -r HEAD script.sh | grep "^100755" &&
I am wondering if we want to be more strict about hiding error
return code from "git ls-files" and "git ls-tree" behind pipes
like these. Usually we encourage using a temporary file, e.g.,
...
git ls-files -s script.sh >ls-files-output &&
test_grep "^100755" ls-files-output &&
...
> + echo true >>script.sh &&
> + test_tick && git commit -m "Modify script" script.sh &&
> + git format-patch -1 --stdout >patch &&
> + grep "index.*100755" patch &&
Anchor the pattern when you know where it has to appear and what the
surrounding letters must be, e.g.,
test_grep "^index .* 100755$" patch &&
A test that expects a match with "grep" is silent when it fails, but
if you use test_grep, the output from "sh t4129-*.sh -i -v" gets
easier to view when the test fails, as it is more verbose and say
"we wanted to see X in the output but your output does not have it".
> + git switch -c branch HEAD^ &&
> + git apply --index patch 2>err &&
> + ! grep "has type 100644, expected 100755" err &&
If you wanted to use test_grep here, the way to negate it is a bit
peculiar, i.e.
test_grep ! "has type ..." err &&
It does not have as much value as the positive case, as "! grep"
that expects to fail would show the unexpected match. In any case,
making sure this particular error message does not appear in the
output is a good way to test it, instead of insisting that the
output is empty, since we may add output to the standard error
stream for unrelated reasons to issue warnings, etc.
> + git restore -S script.sh && git restore script.sh &&
Why not "git reset --hard" here? Just being curious why we want to
waste two invocations of "git restore".
> + git apply patch 2>err &&
> + ! grep "has type 100644, expected 100755" err &&
> +
> + git apply --cached patch 2>err &&
> + ! grep "has type 100644, expected 100755" err
> +'
> +
> test_done
>
> base-commit: 1a87c842ece327d03d08096395969aca5e0a6996
^ 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