* Re: [PATCH 0/1] completion: send-email: don't complete revs when --no-format-patch
From: Dragan Simic @ 2024-01-08 9:40 UTC (permalink / raw)
To: Britton Leo Kerin; +Cc: git
In-Reply-To: <9627364b-c0c9-4b85-a81a-ba1ef0735c9a@smtp-relay.sendinblue.com>
On 2024-01-08 10:36, Britton Leo Kerin wrote:
> Along the way I taught __git_find_last_on_cmdline to understand '--',
> which
> isn't stricly necessary but I think reads more clearly at the call
> sites.
> __git_find_on_cmdline could be changed to work the same, or this part
> dropped
> if people don't like it.
If I may suggest, there's no need for a cover letter for a single patch.
If you want to include some notes in the patch submission, which aren't
supposed to be part of the commit summary, you can do that in the patch
itself.
> Britton Leo Kerin (1):
> completion: don't comp revs when --no-format-patch
>
> contrib/completion/git-completion.bash | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
>
> base-commit: e79552d19784ee7f4bbce278fe25f93fbda196fa
> --
> 2.43.0
^ permalink raw reply
* [PATCH v2 0/6] worktree: initialize refdb via ref backends
From: Patrick Steinhardt @ 2024-01-08 10:05 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Karthik Nayak, Eric Sunshine
In-Reply-To: <cover.1703754513.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 6346 bytes --]
Hi,
this is the second version of my patch series that refactors the
initialization of worktree refdbs to use the refs API.
Changes compared to v1:
- Improved commit messages.
- This series is now based on `ps/refstorage-extension`, 1b2234079b
(t9500: write "extensions.refstorage" into config, 2023-12-29).
While there is no functional dependency between those series,
merging both topics causes a merge conflict.
Patrick
Patrick Steinhardt (6):
refs: prepare `refs_init_db()` for initializing worktree refs
setup: move creation of "refs/" into the files backend
refs/files: skip creation of "refs/{heads,tags}" for worktrees
builtin/worktree: move setup of commondir file earlier
worktree: expose interface to look up worktree by name
builtin/worktree: create refdb via ref backend
builtin/worktree.c | 53 ++++++++++++++++++++-----------------------
refs.c | 6 ++---
refs.h | 4 +++-
refs/debug.c | 4 ++--
refs/files-backend.c | 37 +++++++++++++++++++++++++-----
refs/packed-backend.c | 1 +
refs/refs-internal.h | 4 +++-
setup.c | 17 +-------------
worktree.c | 27 +++++++++++++---------
worktree.h | 12 ++++++++++
10 files changed, 96 insertions(+), 69 deletions(-)
Range-diff against v1:
1: 6cb4e0a99f ! 1: a4894b3e15 refs: prepare `refs_init_db()` for initializing worktree refs
@@ refs/refs-internal.h: typedef struct ref_store *ref_store_init_fn(struct reposit
struct ref_transaction *transaction,
## setup.c ##
-@@ setup.c: void create_reference_database(const char *initial_branch, int quiet)
- safe_create_dir(git_path("refs"), 1);
+@@ setup.c: void create_reference_database(unsigned int ref_storage_format,
adjust_shared_perm(git_path("refs"));
+ repo_set_ref_storage_format(the_repository, ref_storage_format);
- if (refs_init_db(&err))
+ if (refs_init_db(get_main_ref_store(the_repository), 0, &err))
die("failed to set up refs db: %s", err.buf);
2: ae013eaa4a ! 2: f500db51c2 setup: move creation of "refs/" into the files backend
@@ Commit message
seems a lot more sensible to have it this way round than to require
callers to create the directory themselves.
+ An alternative to this would be to create "refs/" in `refs_init_db()`
+ directly. This feels conceptually unclean though as the creation of the
+ refdb is now cluttered across different callsites. Furthermore, both the
+ "files" and the upcoming "reftable" backend write backend-specific data
+ into the "refs/" directory anyway, so splitting up this logic would only
+ make it harder to reason about.
+
Signed-off-by: Patrick Steinhardt <ps@pks.im>
## refs/files-backend.c ##
@@ refs/files-backend.c: static int files_init_db(struct ref_store *ref_store,
## setup.c ##
-@@ setup.c: void create_reference_database(const char *initial_branch, int quiet)
+@@ setup.c: void create_reference_database(unsigned int ref_storage_format,
struct strbuf err = STRBUF_INIT;
int reinit = is_reinit();
@@ setup.c: void create_reference_database(const char *initial_branch, int quiet)
- safe_create_dir(git_path("refs"), 1);
- adjust_shared_perm(git_path("refs"));
-
+ repo_set_ref_storage_format(the_repository, ref_storage_format);
if (refs_init_db(get_main_ref_store(the_repository), 0, &err))
die("failed to set up refs db: %s", err.buf);
-
3: 3cf6ceb274 ! 3: 9e99efeaa3 refs/files: skip creation of "refs/{heads,tags}" for worktrees
@@ Commit message
The files ref backend will create both "refs/heads" and "refs/tags" in
the Git directory. While this logic makes sense for normal repositories,
- it does not fo worktrees because those refs are "common" refs that would
- always be contained in the main repository's ref database.
+ it does not for worktrees because those refs are "common" refs that
+ would always be contained in the main repository's ref database.
Introduce a new flag telling the backend that it is expected to create a
per-worktree ref database and skip creation of these dirs in the files
4: 1a6337fc51 = 4: f2eb020288 builtin/worktree: move setup of commondir file earlier
5: f344a915e9 ! 5: 5525a9f9c2 worktree: expose interface to look up worktree by name
@@ worktree.c
free (worktrees);
}
-@@ worktree.c: static struct worktree *get_main_worktree(void)
+@@ worktree.c: static struct worktree *get_main_worktree(int skip_reading_head)
return worktree;
}
--static struct worktree *get_linked_worktree(const char *id)
-+struct worktree *get_linked_worktree(const char *id)
+-static struct worktree *get_linked_worktree(const char *id,
+- int skip_reading_head)
++struct worktree *get_linked_worktree(const char *id,
++ int skip_reading_head)
{
struct worktree *worktree = NULL;
struct strbuf path = STRBUF_INIT;
@@ worktree.h: struct worktree *find_worktree(struct worktree **list,
+ * Look up the worktree corresponding to `id`, or NULL of no such worktree
+ * exists.
+ */
-+struct worktree *get_linked_worktree(const char *id);
++struct worktree *get_linked_worktree(const char *id,
++ int skip_reading_head);
+
/*
* Return the worktree corresponding to `path`, or NULL if no such worktree
6: aae969301f ! 6: 240dc40de1 builtin/worktree: create refdb via ref backend
@@ builtin/worktree.c: static int add_worktree(const char *path, const char *refnam
- strbuf_reset(&sb);
- strbuf_addf(&sb, "%s/HEAD", sb_repo.buf);
- write_file(sb.buf, "%s", oid_to_hex(null_oid()));
-+ wt = get_linked_worktree(name);
++ wt = get_linked_worktree(name, 1);
+ if (!wt) {
+ ret = error(_("could not find created worktree '%s'"), name);
+ goto done;
base-commit: 1b2234079b24da99dd78e4ce4bfe338a2a841aed
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v2 1/6] refs: prepare `refs_init_db()` for initializing worktree refs
From: Patrick Steinhardt @ 2024-01-08 10:05 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Karthik Nayak, Eric Sunshine
In-Reply-To: <cover.1704705733.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 4469 bytes --]
The purpose of `refs_init_db()` is to initialize the on-disk files of a
new ref database. The function is quite inflexible right now though, as
callers can neither specify the `struct ref_store` nor can they pass any
flags.
Refactor the interface to accept both of these. This will be required so
that we can start initializing per-worktree ref databases via the ref
backend instead of open-coding the initialization in "worktree.c".
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refs.c | 6 ++----
refs.h | 2 +-
refs/debug.c | 4 ++--
refs/files-backend.c | 4 +++-
refs/packed-backend.c | 1 +
refs/refs-internal.h | 4 +++-
setup.c | 2 +-
7 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/refs.c b/refs.c
index fdbf5f4cb1..254272ba6f 100644
--- a/refs.c
+++ b/refs.c
@@ -1944,11 +1944,9 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
}
/* backend functions */
-int refs_init_db(struct strbuf *err)
+int refs_init_db(struct ref_store *refs, int flags, struct strbuf *err)
{
- struct ref_store *refs = get_main_ref_store(the_repository);
-
- return refs->be->init_db(refs, err);
+ return refs->be->init_db(refs, flags, err);
}
const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
diff --git a/refs.h b/refs.h
index 916b874ae3..114caa272a 100644
--- a/refs.h
+++ b/refs.h
@@ -126,7 +126,7 @@ int should_autocreate_reflog(const char *refname);
int is_branch(const char *refname);
-int refs_init_db(struct strbuf *err);
+int refs_init_db(struct ref_store *refs, int flags, struct strbuf *err);
/*
* Return the peeled value of the oid currently being iterated via
diff --git a/refs/debug.c b/refs/debug.c
index b9775f2c37..634681ca44 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -33,10 +33,10 @@ struct ref_store *maybe_debug_wrap_ref_store(const char *gitdir, struct ref_stor
return (struct ref_store *)res;
}
-static int debug_init_db(struct ref_store *refs, struct strbuf *err)
+static int debug_init_db(struct ref_store *refs, int flags, struct strbuf *err)
{
struct debug_ref_store *drefs = (struct debug_ref_store *)refs;
- int res = drefs->refs->be->init_db(drefs->refs, err);
+ int res = drefs->refs->be->init_db(drefs->refs, flags, err);
trace_printf_key(&trace_refs, "init_db: %d\n", res);
return res;
}
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 43fd0ac760..153efe6662 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3220,7 +3220,9 @@ static int files_reflog_expire(struct ref_store *ref_store,
return -1;
}
-static int files_init_db(struct ref_store *ref_store, struct strbuf *err UNUSED)
+static int files_init_db(struct ref_store *ref_store,
+ int flags UNUSED,
+ struct strbuf *err UNUSED)
{
struct files_ref_store *refs =
files_downcast(ref_store, REF_STORE_WRITE, "init_db");
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 8d1090e284..217f052d34 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1246,6 +1246,7 @@ static const char PACKED_REFS_HEADER[] =
"# pack-refs with: peeled fully-peeled sorted \n";
static int packed_init_db(struct ref_store *ref_store UNUSED,
+ int flags UNUSED,
struct strbuf *err UNUSED)
{
/* Nothing to do. */
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 8e9f04cc67..82219829b0 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -529,7 +529,9 @@ typedef struct ref_store *ref_store_init_fn(struct repository *repo,
const char *gitdir,
unsigned int flags);
-typedef int ref_init_db_fn(struct ref_store *refs, struct strbuf *err);
+typedef int ref_init_db_fn(struct ref_store *refs,
+ int flags,
+ struct strbuf *err);
typedef int ref_transaction_prepare_fn(struct ref_store *refs,
struct ref_transaction *transaction,
diff --git a/setup.c b/setup.c
index 1ab1a66bcb..6c8f656f7c 100644
--- a/setup.c
+++ b/setup.c
@@ -1943,7 +1943,7 @@ void create_reference_database(unsigned int ref_storage_format,
adjust_shared_perm(git_path("refs"));
repo_set_ref_storage_format(the_repository, ref_storage_format);
- if (refs_init_db(&err))
+ if (refs_init_db(get_main_ref_store(the_repository), 0, &err))
die("failed to set up refs db: %s", err.buf);
/*
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 2/6] setup: move creation of "refs/" into the files backend
From: Patrick Steinhardt @ 2024-01-08 10:05 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Karthik Nayak, Eric Sunshine
In-Reply-To: <cover.1704705733.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3859 bytes --]
When creating the ref database we unconditionally create the "refs/"
directory in "setup.c". This is a mandatory prerequisite for all Git
repositories regardless of the ref backend in use, because Git will be
unable to detect the directory as a repository if "refs/" doesn't exist.
We are about to add another new caller that will want to create a ref
database when creating worktrees. We would require the same logic to
create the "refs/" directory even though the caller really should not
care about such low-level details. Ideally, the ref database should be
fully initialized after calling `refs_init_db()`.
Move the code to create the directory into the files backend itself to
make it so. This means that future ref backends will also need to have
equivalent logic around to ensure that the directory exists, but it
seems a lot more sensible to have it this way round than to require
callers to create the directory themselves.
An alternative to this would be to create "refs/" in `refs_init_db()`
directly. This feels conceptually unclean though as the creation of the
refdb is now cluttered across different callsites. Furthermore, both the
"files" and the upcoming "reftable" backend write backend-specific data
into the "refs/" directory anyway, so splitting up this logic would only
make it harder to reason about.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refs/files-backend.c | 17 +++++++++++++++++
setup.c | 15 ---------------
2 files changed, 17 insertions(+), 15 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 153efe6662..054ecdbca3 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3228,9 +3228,26 @@ static int files_init_db(struct ref_store *ref_store,
files_downcast(ref_store, REF_STORE_WRITE, "init_db");
struct strbuf sb = STRBUF_INIT;
+ /*
+ * We need to create a "refs" dir in any case so that older versions of
+ * Git can tell that this is a repository. This serves two main purposes:
+ *
+ * - Clients will know to stop walking the parent-directory chain when
+ * detecting the Git repository. Otherwise they may end up detecting
+ * a Git repository in a parent directory instead.
+ *
+ * - Instead of failing to detect a repository with unknown reference
+ * format altogether, old clients will print an error saying that
+ * they do not understand the reference format extension.
+ */
+ strbuf_addf(&sb, "%s/refs", ref_store->gitdir);
+ safe_create_dir(sb.buf, 1);
+ adjust_shared_perm(sb.buf);
+
/*
* Create .git/refs/{heads,tags}
*/
+ strbuf_reset(&sb);
files_ref_path(refs, &sb, "refs/heads");
safe_create_dir(sb.buf, 1);
diff --git a/setup.c b/setup.c
index 6c8f656f7c..abb271e017 100644
--- a/setup.c
+++ b/setup.c
@@ -1927,21 +1927,6 @@ void create_reference_database(unsigned int ref_storage_format,
struct strbuf err = STRBUF_INIT;
int reinit = is_reinit();
- /*
- * We need to create a "refs" dir in any case so that older versions of
- * Git can tell that this is a repository. This serves two main purposes:
- *
- * - Clients will know to stop walking the parent-directory chain when
- * detecting the Git repository. Otherwise they may end up detecting
- * a Git repository in a parent directory instead.
- *
- * - Instead of failing to detect a repository with unknown reference
- * format altogether, old clients will print an error saying that
- * they do not understand the reference format extension.
- */
- safe_create_dir(git_path("refs"), 1);
- adjust_shared_perm(git_path("refs"));
-
repo_set_ref_storage_format(the_repository, ref_storage_format);
if (refs_init_db(get_main_ref_store(the_repository), 0, &err))
die("failed to set up refs db: %s", err.buf);
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 3/6] refs/files: skip creation of "refs/{heads,tags}" for worktrees
From: Patrick Steinhardt @ 2024-01-08 10:05 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Karthik Nayak, Eric Sunshine
In-Reply-To: <cover.1704705733.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2385 bytes --]
The files ref backend will create both "refs/heads" and "refs/tags" in
the Git directory. While this logic makes sense for normal repositories,
it does not for worktrees because those refs are "common" refs that
would always be contained in the main repository's ref database.
Introduce a new flag telling the backend that it is expected to create a
per-worktree ref database and skip creation of these dirs in the files
backend when the flag is set. No other backends (currently) need
worktree-specific logic, so this is the only required change to start
creating per-worktree ref databases via `refs_init_db()`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refs.h | 2 ++
refs/files-backend.c | 22 ++++++++++++++--------
2 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/refs.h b/refs.h
index 114caa272a..c2dfe451a1 100644
--- a/refs.h
+++ b/refs.h
@@ -126,6 +126,8 @@ int should_autocreate_reflog(const char *refname);
int is_branch(const char *refname);
+#define REFS_INIT_DB_IS_WORKTREE (1 << 0)
+
int refs_init_db(struct ref_store *refs, int flags, struct strbuf *err);
/*
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 054ecdbca3..6dae37e351 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3221,7 +3221,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
}
static int files_init_db(struct ref_store *ref_store,
- int flags UNUSED,
+ int flags,
struct strbuf *err UNUSED)
{
struct files_ref_store *refs =
@@ -3245,15 +3245,21 @@ static int files_init_db(struct ref_store *ref_store,
adjust_shared_perm(sb.buf);
/*
- * Create .git/refs/{heads,tags}
+ * There is no need to create directories for common refs when creating
+ * a worktree ref store.
*/
- strbuf_reset(&sb);
- files_ref_path(refs, &sb, "refs/heads");
- safe_create_dir(sb.buf, 1);
+ if (!(flags & REFS_INIT_DB_IS_WORKTREE)) {
+ /*
+ * Create .git/refs/{heads,tags}
+ */
+ strbuf_reset(&sb);
+ files_ref_path(refs, &sb, "refs/heads");
+ safe_create_dir(sb.buf, 1);
- strbuf_reset(&sb);
- files_ref_path(refs, &sb, "refs/tags");
- safe_create_dir(sb.buf, 1);
+ strbuf_reset(&sb);
+ files_ref_path(refs, &sb, "refs/tags");
+ safe_create_dir(sb.buf, 1);
+ }
strbuf_release(&sb);
return 0;
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 4/6] builtin/worktree: move setup of commondir file earlier
From: Patrick Steinhardt @ 2024-01-08 10:05 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Karthik Nayak, Eric Sunshine
In-Reply-To: <cover.1704705733.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1533 bytes --]
Shuffle around how we create supporting worktree files so that we first
ensure that the worktree has all link files ("gitdir", "commondir")
before we try to initialize the ref database by writing "HEAD". This
will be required by a subsequent commit where we start to initialize the
ref database via `refs_init_db()`, which will require an initialized
`struct worktree *`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/worktree.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 4ac1621541..58937a2a68 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -495,6 +495,10 @@ static int add_worktree(const char *path, const char *refname,
strbuf_realpath(&realpath, get_git_common_dir(), 1);
write_file(sb_git.buf, "gitdir: %s/worktrees/%s",
realpath.buf, name);
+ strbuf_reset(&sb);
+ strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
+ write_file(sb.buf, "../..");
+
/*
* This is to keep resolve_ref() happy. We need a valid HEAD
* or is_git_directory() will reject the directory. Any value which
@@ -505,9 +509,6 @@ static int add_worktree(const char *path, const char *refname,
strbuf_reset(&sb);
strbuf_addf(&sb, "%s/HEAD", sb_repo.buf);
write_file(sb.buf, "%s", oid_to_hex(null_oid()));
- strbuf_reset(&sb);
- strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
- write_file(sb.buf, "../..");
/*
* If the current worktree has sparse-checkout enabled, then copy
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 5/6] worktree: expose interface to look up worktree by name
From: Patrick Steinhardt @ 2024-01-08 10:05 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Karthik Nayak, Eric Sunshine
In-Reply-To: <cover.1704705733.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2518 bytes --]
Our worktree interfaces do not provide a way to look up a worktree by
its name. Expose `get_linked_worktree()` to allow for this usecase. As
callers are responsible for freeing this worktree, introduce a new
function `free_worktree()` that does so.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
worktree.c | 27 ++++++++++++++++-----------
worktree.h | 12 ++++++++++++
2 files changed, 28 insertions(+), 11 deletions(-)
diff --git a/worktree.c b/worktree.c
index cc34a3419b..5d5dd46609 100644
--- a/worktree.c
+++ b/worktree.c
@@ -12,18 +12,23 @@
#include "wt-status.h"
#include "config.h"
+void free_worktree(struct worktree *worktree)
+{
+ if (!worktree)
+ return;
+ free(worktree->path);
+ free(worktree->id);
+ free(worktree->head_ref);
+ free(worktree->lock_reason);
+ free(worktree->prune_reason);
+ free(worktree);
+}
+
void free_worktrees(struct worktree **worktrees)
{
int i = 0;
-
- for (i = 0; worktrees[i]; i++) {
- free(worktrees[i]->path);
- free(worktrees[i]->id);
- free(worktrees[i]->head_ref);
- free(worktrees[i]->lock_reason);
- free(worktrees[i]->prune_reason);
- free(worktrees[i]);
- }
+ for (i = 0; worktrees[i]; i++)
+ free_worktree(worktrees[i]);
free (worktrees);
}
@@ -75,8 +80,8 @@ static struct worktree *get_main_worktree(int skip_reading_head)
return worktree;
}
-static struct worktree *get_linked_worktree(const char *id,
- int skip_reading_head)
+struct worktree *get_linked_worktree(const char *id,
+ int skip_reading_head)
{
struct worktree *worktree = NULL;
struct strbuf path = STRBUF_INIT;
diff --git a/worktree.h b/worktree.h
index ce45b66de9..f14784a2ff 100644
--- a/worktree.h
+++ b/worktree.h
@@ -57,6 +57,13 @@ struct worktree *find_worktree(struct worktree **list,
const char *prefix,
const char *arg);
+/*
+ * Look up the worktree corresponding to `id`, or NULL of no such worktree
+ * exists.
+ */
+struct worktree *get_linked_worktree(const char *id,
+ int skip_reading_head);
+
/*
* Return the worktree corresponding to `path`, or NULL if no such worktree
* exists.
@@ -134,6 +141,11 @@ void repair_worktrees(worktree_repair_fn, void *cb_data);
*/
void repair_worktree_at_path(const char *, worktree_repair_fn, void *cb_data);
+/*
+ * Free up the memory for a worktree.
+ */
+void free_worktree(struct worktree *);
+
/*
* Free up the memory for worktree(s)
*/
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 6/6] builtin/worktree: create refdb via ref backend
From: Patrick Steinhardt @ 2024-01-08 10:05 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Karthik Nayak, Eric Sunshine
In-Reply-To: <cover.1704705733.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 4400 bytes --]
When creating a worktree we create the worktree's ref database manually
by first writing a "HEAD" file so that the directory is recognized as a
Git repository by other commands, and then running git-update-ref(1) or
git-symbolic-ref(1) to write the actual value. But while this is fine
for the files backend, this logic simply assumes too much about how the
ref backend works and will leave behind an invalid ref database once any
other ref backend lands.
Refactor the code to instead use `refs_init_db()` to initialize the ref
database so that git-worktree(1) itself does not need to know about how
to initialize it. This will allow future ref backends to customize how
the per-worktree ref database is set up. Furthermore, as we now already
have a worktree ref store around, we can also avoid spawning external
commands to write the HEAD reference and instead use the refs API to do
so.
Note that we do not have an equivalent to passing the `--quiet` flag to
git-symbolic-ref(1) as we did before. This flag does not have an effect
anyway though, as git-symbolic-ref(1) only honors it when reading a
symref, but never when writing one.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/worktree.c | 48 +++++++++++++++++++++-------------------------
1 file changed, 22 insertions(+), 26 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 58937a2a68..dd10446d81 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -416,7 +416,6 @@ static int add_worktree(const char *path, const char *refname,
struct strbuf sb_git = STRBUF_INIT, sb_repo = STRBUF_INIT;
struct strbuf sb = STRBUF_INIT, realpath = STRBUF_INIT;
const char *name;
- struct child_process cp = CHILD_PROCESS_INIT;
struct strvec child_env = STRVEC_INIT;
unsigned int counter = 0;
int len, ret;
@@ -424,7 +423,8 @@ static int add_worktree(const char *path, const char *refname,
struct commit *commit = NULL;
int is_branch = 0;
struct strbuf sb_name = STRBUF_INIT;
- struct worktree **worktrees;
+ struct worktree **worktrees, *wt = NULL;
+ struct ref_store *wt_refs;
worktrees = get_worktrees();
check_candidate_path(path, opts->force, worktrees, "add");
@@ -500,15 +500,26 @@ static int add_worktree(const char *path, const char *refname,
write_file(sb.buf, "../..");
/*
- * This is to keep resolve_ref() happy. We need a valid HEAD
- * or is_git_directory() will reject the directory. Any value which
- * looks like an object ID will do since it will be immediately
- * replaced by the symbolic-ref or update-ref invocation in the new
- * worktree.
+ * Set up the ref store of the worktree and create the HEAD reference.
*/
- strbuf_reset(&sb);
- strbuf_addf(&sb, "%s/HEAD", sb_repo.buf);
- write_file(sb.buf, "%s", oid_to_hex(null_oid()));
+ wt = get_linked_worktree(name, 1);
+ if (!wt) {
+ ret = error(_("could not find created worktree '%s'"), name);
+ goto done;
+ }
+ wt_refs = get_worktree_ref_store(wt);
+
+ ret = refs_init_db(wt_refs, REFS_INIT_DB_IS_WORKTREE, &sb);
+ if (ret)
+ goto done;
+
+ if (!is_branch && commit)
+ ret = refs_update_ref(wt_refs, NULL, "HEAD", &commit->object.oid,
+ NULL, 0, UPDATE_REFS_MSG_ON_ERR);
+ else
+ ret = refs_create_symref(wt_refs, "HEAD", symref.buf, NULL);
+ if (ret)
+ goto done;
/*
* If the current worktree has sparse-checkout enabled, then copy
@@ -527,22 +538,6 @@ static int add_worktree(const char *path, const char *refname,
strvec_pushf(&child_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf);
strvec_pushf(&child_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path);
- cp.git_cmd = 1;
-
- if (!is_branch && commit) {
- strvec_pushl(&cp.args, "update-ref", "HEAD",
- oid_to_hex(&commit->object.oid), NULL);
- } else {
- strvec_pushl(&cp.args, "symbolic-ref", "HEAD",
- symref.buf, NULL);
- if (opts->quiet)
- strvec_push(&cp.args, "--quiet");
- }
-
- strvec_pushv(&cp.env, child_env.v);
- ret = run_command(&cp);
- if (ret)
- goto done;
if (opts->orphan &&
(ret = make_worktree_orphan(refname, opts, &child_env)))
@@ -588,6 +583,7 @@ static int add_worktree(const char *path, const char *refname,
strbuf_release(&sb_git);
strbuf_release(&sb_name);
strbuf_release(&realpath);
+ free_worktree(wt);
return ret;
}
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 0/4] reftable: optimize I/O patterns
From: Patrick Steinhardt @ 2024-01-08 12:18 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys
[-- Attachment #1: Type: text/plain, Size: 890 bytes --]
Hi,
this patch series optimizes some I/O patterns that the reftable library
uses:
- Establish a stat(3P)-based caching mechanism to avoid re-reading
"tables.list" all the time.
- Convert the block source to use mmap(3P) instead of read(3P) to read
data.
Combined these lead to a ~2.7x speedup when writing many refs.
Patrick
Patrick Steinhardt (4):
reftable/stack: refactor stack reloading to have common exit path
reftable/stack: refactor reloading to use file descriptor
reftable/stack: use stat info to avoid re-reading stack list
reftable/blocksource: use mmap to read tables
reftable/blocksource.c | 48 +++++++++++++-----
reftable/stack.c | 111 ++++++++++++++++++++++++-----------------
reftable/stack.h | 1 +
reftable/system.h | 1 +
4 files changed, 103 insertions(+), 58 deletions(-)
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH 1/4] reftable/stack: refactor stack reloading to have common exit path
From: Patrick Steinhardt @ 2024-01-08 12:18 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys
In-Reply-To: <cover.1704714575.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3322 bytes --]
The `reftable_stack_reload_maybe_reuse()` function is responsible for
reloading the reftable list from disk. The function is quite hard to
follow though because it has a bunch of different exit paths, many of
which have to free the same set of resources.
Refactor the function to have a common exit path. While at it, touch up
the style of this function a bit to match our usual coding style better.
---
reftable/stack.c | 86 +++++++++++++++++++++++-------------------------
1 file changed, 42 insertions(+), 44 deletions(-)
diff --git a/reftable/stack.c b/reftable/stack.c
index 16bab82063..bf869a6772 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -304,69 +304,67 @@ static int tv_cmp(struct timeval *a, struct timeval *b)
static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
int reuse_open)
{
- struct timeval deadline = { 0 };
- int err = gettimeofday(&deadline, NULL);
+ char **names = NULL, **names_after = NULL;
+ struct timeval deadline;
int64_t delay = 0;
- int tries = 0;
- if (err < 0)
- return err;
+ int tries = 0, err;
+ err = gettimeofday(&deadline, NULL);
+ if (err < 0)
+ goto out;
deadline.tv_sec += 3;
+
while (1) {
- char **names = NULL;
- char **names_after = NULL;
- struct timeval now = { 0 };
- int err = gettimeofday(&now, NULL);
- int err2 = 0;
- if (err < 0) {
- return err;
- }
+ struct timeval now;
- /* Only look at deadlines after the first few times. This
- simplifies debugging in GDB */
+ err = gettimeofday(&now, NULL);
+ if (err < 0)
+ goto out;
+
+ /*
+ * Only look at deadlines after the first few times. This
+ * simplifies debugging in GDB.
+ */
tries++;
- if (tries > 3 && tv_cmp(&now, &deadline) >= 0) {
- break;
- }
+ if (tries > 3 && tv_cmp(&now, &deadline) >= 0)
+ goto out;
err = read_lines(st->list_file, &names);
- if (err < 0) {
- free_names(names);
- return err;
- }
+ if (err < 0)
+ goto out;
+
err = reftable_stack_reload_once(st, names, reuse_open);
- if (err == 0) {
- free_names(names);
+ if (!err)
break;
- }
- if (err != REFTABLE_NOT_EXIST_ERROR) {
- free_names(names);
- return err;
- }
-
- /* err == REFTABLE_NOT_EXIST_ERROR can be caused by a concurrent
- writer. Check if there was one by checking if the name list
- changed.
- */
- err2 = read_lines(st->list_file, &names_after);
- if (err2 < 0) {
- free_names(names);
- return err2;
- }
-
+ if (err != REFTABLE_NOT_EXIST_ERROR)
+ goto out;
+
+ /*
+ * REFTABLE_NOT_EXIST_ERROR can be caused by a concurrent
+ * writer. Check if there was one by checking if the name list
+ * changed.
+ */
+ err = read_lines(st->list_file, &names_after);
+ if (err < 0)
+ goto out;
if (names_equal(names_after, names)) {
- free_names(names);
- free_names(names_after);
- return err;
+ err = REFTABLE_NOT_EXIST_ERROR;
+ goto out;
}
+
free_names(names);
+ names = NULL;
free_names(names_after);
+ names_after = NULL;
delay = delay + (delay * rand()) / RAND_MAX + 1;
sleep_millisec(delay);
}
- return 0;
+out:
+ free_names(names);
+ free_names(names_after);
+ return err;
}
/* -1 = error
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 2/4] reftable/stack: refactor reloading to use file descriptor
From: Patrick Steinhardt @ 2024-01-08 12:18 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys
In-Reply-To: <cover.1704714575.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1851 bytes --]
We're about to introduce a stat(3P)-based caching mechanism to reload
the list of stacks only when it has changed. In order to avoid race
conditions this requires us to have a file descriptor available that we
can use to call fstat(3P) on.
Prepare for this by converting the code to use `fd_read_lines()` so that
we have the file descriptor readily available.
---
reftable/stack.c | 21 ++++++++++++++++++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/reftable/stack.c b/reftable/stack.c
index bf869a6772..b1ee247601 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -308,6 +308,7 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
struct timeval deadline;
int64_t delay = 0;
int tries = 0, err;
+ int fd = -1;
err = gettimeofday(&deadline, NULL);
if (err < 0)
@@ -329,9 +330,19 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
if (tries > 3 && tv_cmp(&now, &deadline) >= 0)
goto out;
- err = read_lines(st->list_file, &names);
- if (err < 0)
- goto out;
+ fd = open(st->list_file, O_RDONLY);
+ if (fd < 0) {
+ if (errno != ENOENT) {
+ err = REFTABLE_IO_ERROR;
+ goto out;
+ }
+
+ names = reftable_calloc(sizeof(char *));
+ } else {
+ err = fd_read_lines(fd, &names);
+ if (err < 0)
+ goto out;
+ }
err = reftable_stack_reload_once(st, names, reuse_open);
if (!err)
@@ -356,12 +367,16 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
names = NULL;
free_names(names_after);
names_after = NULL;
+ close(fd);
+ fd = -1;
delay = delay + (delay * rand()) / RAND_MAX + 1;
sleep_millisec(delay);
}
out:
+ if (fd >= 0)
+ close(fd);
free_names(names);
free_names(names_after);
return err;
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 3/4] reftable/stack: use stat info to avoid re-reading stack list
From: Patrick Steinhardt @ 2024-01-08 12:18 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys
In-Reply-To: <cover.1704714575.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 5530 bytes --]
Whenever we call into the refs interfaces we potentially have to reload
refs in case they have been concurrently modified, either in-process or
externally. While this happens somewhat automatically for loose refs
because we simply try to re-read the files, the "packed" backend will
reload its snapshot of the packed-refs file in case its stat info has
changed since last reading it.
In the reftable backend we have a similar mechanism that is provided by
`reftable_stack_reload()`. This function will read the list of stacks
from "tables.list" and, if they have changed from the currently stored
list, reload the stacks. This is heavily inefficient though, as we have
to check whether the stack is up-to-date on basically every read and
thus keep on re-reading the file all the time even if it didn't change
at all.
We can do better and use the same stat(3P)-based mechanism that the
"packed" backend uses. Instead of reading the file, we will only open
the file descriptor, fstat(3P) it, and then compare the info against the
cached value from the last time we have updated the stack. This should
always work alright because "tables.list" is updated atomically via a
rename, so even if the ctime or mtime wasn't granular enough to identify
a change, at least the inode number should have changed.
This change significantly speeds up operations where many refs are read,
like when using git-update-ref(1). The following benchmark creates N
refs in an otherwise-empty repository via `git update-ref --stdin`:
Benchmark 1: update-ref: create many refs (refcount = 1, revision = HEAD~)
Time (mean ± σ): 5.6 ms ± 0.2 ms [User: 2.5 ms, System: 3.0 ms]
Range (min … max): 5.2 ms … 6.0 ms 89 runs
Benchmark 2: update-ref: create many refs (refcount = 100, revision = HEAD~)
Time (mean ± σ): 19.2 ms ± 0.3 ms [User: 8.6 ms, System: 10.4 ms]
Range (min … max): 18.4 ms … 19.8 ms 63 runs
Benchmark 3: update-ref: create many refs (refcount = 10000, revision = HEAD~)
Time (mean ± σ): 1.310 s ± 0.014 s [User: 0.566 s, System: 0.724 s]
Range (min … max): 1.291 s … 1.342 s 10 runs
Benchmark 4: update-ref: create many refs (refcount = 1, revision = HEAD)
Time (mean ± σ): 5.7 ms ± 0.4 ms [User: 2.6 ms, System: 3.1 ms]
Range (min … max): 5.1 ms … 9.5 ms 91 runs
Benchmark 5: update-ref: create many refs (refcount = 100, revision = HEAD)
Time (mean ± σ): 15.2 ms ± 0.3 ms [User: 7.0 ms, System: 8.1 ms]
Range (min … max): 14.3 ms … 17.1 ms 71 runs
Benchmark 6: update-ref: create many refs (refcount = 10000, revision = HEAD)
Time (mean ± σ): 916.1 ms ± 11.0 ms [User: 420.8 ms, System: 495.0 ms]
Range (min … max): 906.9 ms … 943.8 ms 10 runs
Summary
update-ref: create many refs (refcount = 1, revision = HEAD~) ran
1.01 ± 0.09 times faster than update-ref: create many refs (refcount = 1, revision = HEAD)
2.72 ± 0.11 times faster than update-ref: create many refs (refcount = 100, revision = HEAD)
3.42 ± 0.13 times faster than update-ref: create many refs (refcount = 100, revision = HEAD~)
163.59 ± 5.62 times faster than update-ref: create many refs (refcount = 10000, revision = HEAD)
233.91 ± 7.92 times faster than update-ref: create many refs (refcount = 10000, revision = HEAD~)
---
reftable/stack.c | 10 +++++++++-
reftable/stack.h | 1 +
reftable/system.h | 1 +
3 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/reftable/stack.c b/reftable/stack.c
index b1ee247601..a51a2dbf1f 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -175,6 +175,7 @@ void reftable_stack_destroy(struct reftable_stack *st)
st->readers_len = 0;
FREE_AND_NULL(st->readers);
}
+ stat_validity_clear(&st->list_validity);
FREE_AND_NULL(st->list_file);
FREE_AND_NULL(st->reftable_dir);
reftable_free(st);
@@ -374,6 +375,8 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
sleep_millisec(delay);
}
+ stat_validity_update(&st->list_validity, fd);
+
out:
if (fd >= 0)
close(fd);
@@ -388,8 +391,13 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
static int stack_uptodate(struct reftable_stack *st)
{
char **names = NULL;
- int err = read_lines(st->list_file, &names);
+ int err;
int i = 0;
+
+ if (stat_validity_check(&st->list_validity, st->list_file))
+ return 0;
+
+ err = read_lines(st->list_file, &names);
if (err < 0)
return err;
diff --git a/reftable/stack.h b/reftable/stack.h
index f57005846e..3f80cc598a 100644
--- a/reftable/stack.h
+++ b/reftable/stack.h
@@ -14,6 +14,7 @@ license that can be found in the LICENSE file or at
#include "reftable-stack.h"
struct reftable_stack {
+ struct stat_validity list_validity;
char *list_file;
char *reftable_dir;
int disable_auto_compact;
diff --git a/reftable/system.h b/reftable/system.h
index 6b74a81514..2cc7adf271 100644
--- a/reftable/system.h
+++ b/reftable/system.h
@@ -12,6 +12,7 @@ license that can be found in the LICENSE file or at
/* This header glues the reftable library to the rest of Git */
#include "git-compat-util.h"
+#include "statinfo.h"
#include "strbuf.h"
#include "hash-ll.h" /* hash ID, sizes.*/
#include "dir.h" /* remove_dir_recursively, for tests.*/
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 4/4] reftable/blocksource: use mmap to read tables
From: Patrick Steinhardt @ 2024-01-08 12:18 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys
In-Reply-To: <cover.1704714575.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 7265 bytes --]
The blocksource interface provides an interface to read blocks from a
reftable table. This interface is implemented using read(3P) calls on
the underlying file descriptor. While this works alright, this pattern
is very inefficient when repeatedly querying the reftable stack for one
or more refs. This inefficiency can mostly be attributed to the fact
that we often need to re-read the same blocks over and over again, and
every single time we need to call read(3P) again.
A natural fit in this context is to use mmap(3P) instead of read(3P),
which has a bunch of benefits:
- We do not need to come up with a caching strategy for some of the
blocks as this will be handled by the kernel already.
- We can avoid the overhead of having to call into the read(3P)
syscall repeatedly.
- We do not need to allocate returned blocks repeatedly, but can
instead hand out pointers into the mmapped region directly.
Using mmap comes with a significant drawback on Windows though, because
mmapped files cannot be deleted and neither is it possible to rename
files onto an mmapped file. But for one, the reftable library gracefully
handles the case where auto-compaction cannot delete a still-open stack
already and ignores any such errors. Also, `reftable_stack_clean()` will
prune stale tables which are not referenced by "tables.list" anymore so
that those files can eventually be pruned. And second, we never rewrite
already-rewritten stacks, so it does not matter that we cannot rename a
file over an mmaped file, either.
Another unfortunate property of mmap is that it is not supported by all
systems. But given that the size of reftables should typically be rather
limited (megabytes at most in the vast majority of repositories), we can
provide an easy fallback by just reading the complete table into memory
on such platforms. This is the same strategy that the "packed" backend
uses in case the platform does not provide mmap.
While this change doesn't significantly improve performance in the case
where we're seeking through stacks once (like e.g. git-for-each-ref(1)
would). But it does speed up usecases where there is lots of random
access to refs, e.g. when writing. The following benchmark demonstrates
these savings with git-update-ref(1) creating N refs in an otherwise
empty repository:
Benchmark 1: update-ref: create many refs (refcount = 1, revision = HEAD~)
Time (mean ± σ): 5.6 ms ± 0.2 ms [User: 2.7 ms, System: 2.8 ms]
Range (min … max): 5.1 ms … 6.0 ms 90 runs
Benchmark 2: update-ref: create many refs (refcount = 100, revision = HEAD~)
Time (mean ± σ): 15.1 ms ± 0.4 ms [User: 7.1 ms, System: 8.0 ms]
Range (min … max): 14.2 ms … 16.5 ms 71 runs
Benchmark 3: update-ref: create many refs (refcount = 10000, revision = HEAD~)
Time (mean ± σ): 919.4 ms ± 11.2 ms [User: 427.5 ms, System: 491.7 ms]
Range (min … max): 908.1 ms … 936.6 ms 10 runs
Benchmark 4: update-ref: create many refs (refcount = 1, revision = HEAD)
Time (mean ± σ): 5.5 ms ± 0.3 ms [User: 2.4 ms, System: 3.1 ms]
Range (min … max): 5.0 ms … 7.3 ms 89 runs
Benchmark 5: update-ref: create many refs (refcount = 100, revision = HEAD)
Time (mean ± σ): 10.4 ms ± 0.3 ms [User: 5.1 ms, System: 5.3 ms]
Range (min … max): 9.7 ms … 11.0 ms 78 runs
Benchmark 6: update-ref: create many refs (refcount = 10000, revision = HEAD)
Time (mean ± σ): 483.5 ms ± 6.3 ms [User: 227.8 ms, System: 255.6 ms]
Range (min … max): 477.5 ms … 498.8 ms 10 runs
Summary
update-ref: create many refs (refcount = 1, revision = HEAD) ran
1.01 ± 0.06 times faster than update-ref: create many refs (refcount = 1, revision = HEAD~)
1.89 ± 0.11 times faster than update-ref: create many refs (refcount = 100, revision = HEAD)
2.73 ± 0.16 times faster than update-ref: create many refs (refcount = 100, revision = HEAD~)
87.55 ± 4.65 times faster than update-ref: create many refs (refcount = 10000, revision = HEAD)
166.48 ± 8.80 times faster than update-ref: create many refs (refcount = 10000, revision = HEAD~)
Theoretically, we could also replicate the strategy of the "packed"
backend where small tables are read into memory instead of using mmap.
Benchmarks did not confirm that this has a performance benefit though.
---
reftable/blocksource.c | 48 ++++++++++++++++++++++++++++++------------
1 file changed, 35 insertions(+), 13 deletions(-)
diff --git a/reftable/blocksource.c b/reftable/blocksource.c
index a1ea304429..5d3f3d264c 100644
--- a/reftable/blocksource.c
+++ b/reftable/blocksource.c
@@ -13,6 +13,12 @@ license that can be found in the LICENSE file or at
#include "reftable-blocksource.h"
#include "reftable-error.h"
+#if defined(NO_MMAP)
+static int use_mmap = 0;
+#else
+static int use_mmap = 1;
+#endif
+
static void strbuf_return_block(void *b, struct reftable_block *dest)
{
if (dest->len)
@@ -78,6 +84,7 @@ struct reftable_block_source malloc_block_source(void)
struct file_block_source {
int fd;
uint64_t size;
+ unsigned char *data;
};
static uint64_t file_size(void *b)
@@ -87,19 +94,23 @@ static uint64_t file_size(void *b)
static void file_return_block(void *b, struct reftable_block *dest)
{
- if (dest->len)
- memset(dest->data, 0xff, dest->len);
- reftable_free(dest->data);
}
-static void file_close(void *b)
+static void file_close(void *v)
{
- int fd = ((struct file_block_source *)b)->fd;
- if (fd > 0) {
- close(fd);
- ((struct file_block_source *)b)->fd = 0;
+ struct file_block_source *b = v;
+
+ if (b->fd >= 0) {
+ close(b->fd);
+ b->fd = -1;
}
+ if (use_mmap)
+ munmap(b->data, b->size);
+ else
+ reftable_free(b->data);
+ b->data = NULL;
+
reftable_free(b);
}
@@ -108,9 +119,7 @@ static int file_read_block(void *v, struct reftable_block *dest, uint64_t off,
{
struct file_block_source *b = v;
assert(off + size <= b->size);
- dest->data = reftable_malloc(size);
- if (pread_in_full(b->fd, dest->data, size, off) != size)
- return -1;
+ dest->data = b->data + off;
dest->len = size;
return size;
}
@@ -127,8 +136,10 @@ int reftable_block_source_from_file(struct reftable_block_source *bs,
{
struct stat st = { 0 };
int err = 0;
- int fd = open(name, O_RDONLY);
+ int fd;
struct file_block_source *p = NULL;
+
+ fd = open(name, O_RDONLY);
if (fd < 0) {
if (errno == ENOENT) {
return REFTABLE_NOT_EXIST_ERROR;
@@ -144,7 +155,18 @@ int reftable_block_source_from_file(struct reftable_block_source *bs,
p = reftable_calloc(sizeof(struct file_block_source));
p->size = st.st_size;
- p->fd = fd;
+ if (use_mmap) {
+ p->data = xmmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
+ p->fd = fd;
+ } else {
+ p->data = xmalloc(st.st_size);
+ if (read_in_full(fd, p->data, st.st_size) != st.st_size) {
+ close(fd);
+ return -1;
+ }
+ close(fd);
+ p->fd = -1;
+ }
assert(!bs->ops);
bs->ops = &file_vtable;
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [PATCH v3] fetch: add new config option fetch.all
From: Junio C Hamano @ 2024-01-08 17:25 UTC (permalink / raw)
To: Tamino Bauknecht; +Cc: git
In-Reply-To: <20240106202352.7253-2-dev@tb6.eu>
Tamino Bauknecht <dev@tb6.eu> writes:
> This commit introduces the new boolean configuration option fetch.all
"This commit introduces the new boolean" -> "Introduce a boolean"
In general, our log messages are written as if we are giving an
order to explain what should happen to the codebase.
> which allows to fetch all available remotes by default. The config
> option can be overridden by explicitly specifying a remote or by using
> --no-all.
> The behavior for --all is unchanged and calling git-fetch with --all and
> a remote will still result in an error.
Good.
> The config option was also added to the config documentation and new
"The config option was also added to the config" -> "Describe the
configuration variable in the"
> tests cover the expected behavior.
> Additionally, --no-all was added to the command-line documentation of
"--no-all was added to" -> "add --no-all to".
> git-fetch.
>
> Signed-off-by: Tamino Bauknecht <dev@tb6.eu>
> ---
> +fetch.all::
> + If true, fetch will attempt to update all available remotes.
> + This behavior can be overridden by passing `--no-all` or by
> + explicitly specifying one or more remote(s) to fetch from.
> + Defaults to false.
Good.
> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
> index a1d6633a4f..5e70f6d2e4 100644
> --- a/Documentation/fetch-options.txt
> +++ b/Documentation/fetch-options.txt
> @@ -1,5 +1,6 @@
> ---all::
> - Fetch all remotes.
> +--[no-]all::
> + Fetch all remotes. This overrides the configuration option
> + `fetch.all`.
"configuration option" -> "configuration variable".
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index a284b970ef..5a0b249c07 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -102,6 +102,7 @@ static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;
>
> struct fetch_config {
> enum display_format display_format;
> + int all;
> int prune;
> int prune_tags;
> int show_forced_updates;
> @@ -115,6 +116,11 @@ static int git_fetch_config(const char *k, const char *v,
> {
> struct fetch_config *fetch_config = cb;
>
> + if (!strcmp(k, "fetch.all")) {
> + fetch_config->all = git_config_bool(k, v);
> + return 0;
> + }
> +
> if (!strcmp(k, "fetch.prune")) {
> fetch_config->prune = git_config_bool(k, v);
> return 0;
> @@ -2121,6 +2127,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
> {
> struct fetch_config config = {
> .display_format = DISPLAY_FORMAT_FULL,
> + .all = -1,
> .prune = -1,
> .prune_tags = -1,
> .show_forced_updates = 1,
Not incorrect per-se, but I find it misleading, as there is no
reason to initialize it to -1. If left initialized to 0 (which
would be the default), and if we do not see "fetch.all", it will be
left to 0, which is the default value, and that is what you want.
> @@ -2132,7 +2139,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
> const char *bundle_uri;
> struct string_list list = STRING_LIST_INIT_DUP;
> struct remote *remote = NULL;
> - int all = 0, multiple = 0;
> + int all = -1, multiple = 0;
> int result = 0;
> int prune_tags_ok = 1;
> int enable_auto_gc = 1;
> @@ -2148,7 +2155,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>
> struct option builtin_fetch_options[] = {
> OPT__VERBOSITY(&verbosity),
> - OPT_BOOL(0, "all", &all,
> + OPT_COUNTUP(0, "all", &all,
I do not think this change from BOOL to COUNTUP is warranted. Does
it trigger an error if you feed a variable that is initialized to -1
to OPT_BOOL()? If not, "initialize to -1 and let OPT_BOOL() set
either 0 or 1 when seeing a command line option" pattern would be
preferrable.
The idea is to initialize the "all" variable to -1 (unspecified) and
then if there is a command line option set it to either 0 or 1.
After the command line option parsing returns, we can tell:
-1: there was not --all and there was not --no-all. We need to
look at the configuration variable.
0: there was --no-all. Ignore the configuration.
1: there was --all. Ignore the configuration.
Use of COUNTUP can leave the variable set to say 2 or 3. There are
legitimate uses of COUNTUP (e.g., expressing verbosity levels via
multiple use of "-v -v"), but you want "--all" and "--all --all"
behave identically, so COUNTUP is not a good fit here.
> @@ -2337,11 +2344,18 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
> fetch_bundle_uri(the_repository, bundle_uri, NULL))
> warning(_("failed to fetch bundles from '%s'"), bundle_uri);
>
OK. The following has to be a bit unusual and different from the
usual "we initialize a variable to its default, read into it from
the config, and then overwrite it from the command line option"
pattern because we want to ignore configured value upon factors
other than the command line option that corresponds to the
configuration variable (namely, even if --all or --no-all does not
appear on the command line, a remote makes the fetch.all
configuration ignored, without triggering an error).
> - if (all) {
> + if (all > 0) {
Hence, we need to catch only the case where "--all" was given
explicitly (i.e., all == -1 is not handled here). Good.
> if (argc == 1)
> die(_("fetch --all does not take a repository argument"));
> else if (argc > 1)
> die(_("fetch --all does not make sense with refspecs"));
> + }
> +
> + if (all > 0 || (config.all > 0 && !argc && all < 0)) {
Here, you can say config.all (not "config.all > 0") if you
initialized the .all member to 0 (remove the ".all = -1"
initialization, in other words).
> + /*
> + * Only use fetch.all config option if no remotes were
> + * explicitly given and if no --no-all was passed
> + */
> (void) for_each_remote(get_one_remote_for_fetch, &list);
>
> /* do not do fetch_multiple() of one */
But I suspect that it is easier to understand if you added this
if (all < 0)
/* no --[no-]all given. */
all = (!argc) ? config.all : 0;
before all of the above, and leave the rest of the original
untouched. In other words, when there is no command line option
"--[no-]all", we take "fetch.all" into consideration only when there
is no explicit remote. If there is an explicit remote and there is
no "--[no-]all", we can explicitly set all to 0 here.
And the remainder of the existing logic will work fine with all == 0
or all == 1 so we do not have to touch it.
> diff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh
> index a95841dc36..63cd730718 100755
> --- a/t/t5514-fetch-multiple.sh
> +++ b/t/t5514-fetch-multiple.sh
> @@ -24,6 +24,15 @@ setup_repository () {
> )
> }
>
> +setup_test_clone () {
> + test_dir="$1" &&
> + git clone one "$test_dir" &&
> + for r in one two three
> + do
> + git -C "$test_dir" remote add "$r" "../$r" || return 1
> + done
> +}
> +
> test_expect_success setup '
> setup_repository one &&
> setup_repository two &&
> @@ -209,4 +218,156 @@ test_expect_success 'git fetch --multiple --jobs=0 picks a default' '
> git fetch --multiple --jobs=0)
> '
>
> +create_fetch_all_expect () {
> + cat >expect <<-\EOF || return 1
I saw there already was a comment on "|| return 1", which I agree
with.
Thanks.
^ permalink raw reply
* Re: [GSOC][RFC] Heed core.bare from template config file when no command line override given, as a microproject.
From: Junio C Hamano @ 2024-01-08 17:32 UTC (permalink / raw)
To: Ghanshyam Thakkar
Cc: Elijah Newren, Christian Couder, git, johannes.schindelin
In-Reply-To: <CY7M09XT547N.2OOTI5APX9RIX@gmail.com>
"Ghanshyam Thakkar" <shyamthakkar001@gmail.com> writes:
>> Specifically, the commit that introduced the comment never wanted to
>> honor core.bare in the template. I do not think I has core.bare in
>> mind when I wrote the comment, but I would have described it as the
>> same category as the repository format version, i.e. something you
>> would not want to copy, if I were pressed to clarify back then.
>
> Then I suppose this warrants updating the TODO comment in
> create_default_files(), which currently can be interpreted as this
> being a unwanted behavior. And also amending the testcases which
> currently display this as knwon breakage.
I obviously agree with that, after saying that I suspect 0f7443bd
comes from a misunderstanding ;-).
>> If somebody wants to always create a bare repository by having
>> core.bare=true in their template and if we wanted to honor it (which
>> I am dubious of the value of, by the way), I would think the right
>> place to do so would be way before create_default_files() is called.
>> When running "git init [$DIR]", long before calling init_db(), we
>> decide if we are about to create a bare repository and either create
>> $DIR or $DIR/.git. What is in the template, if we really wanted to
>> do so, should be read before that happens, no?
>
> That is what I proposed in my original email, after which I had a
> working solution which passed all the tests. That solution was indeed to
> check for core.bare in the template before we set GIT_DIR_ENVIRONMENT,
> which subsequently creates either $DIR or $DIR/.git as you described
> above.
Yeah, if this were still in soon after 4f629539 was written, then
such a change might have been a useful feature enhancement, but risk
of breaking people (third-party tools) who use the same template to
initialize both bare and non-bare repositories is there, so...
Thanks.
^ permalink raw reply
* [GSoC][RFC] Replace use of atoi() with strtol_i(), as a microproject
From: mohitmarathe @ 2024-01-08 17:34 UTC (permalink / raw)
To: git@vger.kernel.org
Cc: gitster@pobox.com, britton.kerin@gmail.com, peff@peff.net
Hello,
I'm Mohit, an undergrad from India, and I want to start contributing to the Git project. I have already built Git from source and finished `git psuh` tutorial. And I must say the "Hacking Git" documentation is great (very detailed and maybe exhaustive) and easy to follow. So I also read the topic on "microprojects", and while searching for one, I came across this message: https://public-inbox.org/git/xmqqjzpjsbjl.fsf@gitster.g/.
I want to work on this task (if it is not taken up already) as a microproject for GSoC.
Approach:
From what I understood, the idea is to *not* allow non-integer characters in the arguments by printing an error message. So we have to replace `atoi` with `strtol_i`, like it is done in this patch: https://public-inbox.org/git/xmqq5y181fx0.fsf_-_@gitster.g/.
There are some places where we want to continue to parse after the end of the integer (where `strspn` is used as mentioned by Junio), and based on Junio's suggestion, a new helper can be created like this:
> static inline int strtol_i2(char const *s, int base, int *result, char **endp)
> {
> long ul;
> char *dummy = NULL;
>
> if (!endp)
> endp = &dummy;
> errno = 0;
> ul = strtol(s, &endp, base);
> if (errno ||
> /*
> * if we are told to parse to the end of the string by
> * passing NULL to endp, it is an error to have any
> * remaining character after the digits.
> */
> (dummy && *dummy) ||
> endp == s || (int) ul != ul)
> return -1;
> *result = ul;
> return 0;
> }
So I searched for all the occurrences of `atoi(` (as suggested by Junio), and I found only two instances (both in `builtin/patch-id.c`) where it is followed by `strspn`. Is it safe to assume that this is the only place where we cannot directly replace `atoi` with `strtol_i`, or should I keep digging?
Also, this seems like a large change due to the number of files involved, while the documentation about the microproject emphasizes keeping the change small. Does it mean a small change per commit or a small change per Pull Request?
Thanks!
^ permalink raw reply
* [PATCH v6 0/1] mingw: give more details about unsafe directory's
From: Sören Krecker @ 2024-01-08 17:38 UTC (permalink / raw)
To: git; +Cc: sunshine, j6t, Sören Krecker
In-Reply-To: <de9cf40a-1ad6-45fb-8b70-8b0c71a3bfbb@kdbg.org>
I have processed the points raised.
Sören Krecker (1):
mingw: give more details about unsafe directory's ownership
compat/mingw.c | 70 ++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 57 insertions(+), 13 deletions(-)
base-commit: e79552d19784ee7f4bbce278fe25f93fbda196fa
--
2.39.2
^ permalink raw reply
* [PATCH v7 1/1] mingw: give more details about unsafe directory's ownership
From: Sören Krecker @ 2024-01-08 17:38 UTC (permalink / raw)
To: git; +Cc: sunshine, j6t, Sören Krecker
In-Reply-To: <20240108173837.20480-1-soekkle@freenet.de>
Add domain/username in error message, if owner sid of repository and
user sid are not equal on windows systems.
Old error message:
'''
fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
'C:/Users/test/source/repos/git' is owned by:
'S-1-5-21-571067702-4104414259-3379520149-500'
but the current user is:
'S-1-5-21-571067702-4104414259-3379520149-1001'
To add an exception for this directory, call:
git config --global --add safe.directory C:/Users/test/source/repos/git
'''
New error message:
'''
fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
'C:/Users/test/source/repos/git' is owned by:
DESKTOP-L78JVA6/Administrator (S-1-5-21-571067702-4104414259-3379520149-500)
but the current user is:
DESKTOP-L78JVA6/test (S-1-5-21-571067702-4104414259-3379520149-1001)
To add an exception for this directory, call:
git config --global --add safe.directory C:/Users/test/source/repos/git
'''
Signed-off-by: Sören Krecker <soekkle@freenet.de>
---
compat/mingw.c | 70 ++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 57 insertions(+), 13 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index 42053c1f65..d85fae3747 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2684,6 +2684,30 @@ static PSID get_current_user_sid(void)
return result;
}
+static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)
+{
+ SID_NAME_USE pe_use;
+ DWORD len_user = 0, len_domain = 0;
+ BOOL translate_sid_to_user;
+
+ /*
+ * returns only FALSE, because the string pointers are NULL
+ */
+ LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
+ &pe_use);
+ /*
+ * Alloc needed space of the strings
+ */
+ ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user);
+ translate_sid_to_user = LookupAccountSidA(NULL, sid,
+ (*str) + len_domain, &len_user, *str, &len_domain, &pe_use);
+ if (!translate_sid_to_user)
+ FREE_AND_NULL(*str);
+ else
+ (*str)[len_domain] = '/';
+ return translate_sid_to_user;
+}
+
static int acls_supported(const char *path)
{
size_t offset = offset_1st_component(path);
@@ -2765,27 +2789,47 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
strbuf_addf(report, "'%s' is on a file system that does "
"not record ownership\n", path);
} else if (report) {
- LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
+ LPSTR str1, str2, str3, str4, to_free1 = NULL,
+ to_free3 = NULL, to_local_free2 = NULL,
+ to_local_free4 = NULL;
- if (ConvertSidToStringSidA(sid, &str1))
+ if (user_sid_to_user_name(sid, &str1))
to_free1 = str1;
else
str1 = "(inconvertible)";
-
- if (!current_user_sid)
- str2 = "(none)";
- else if (!IsValidSid(current_user_sid))
- str2 = "(invalid)";
- else if (ConvertSidToStringSidA(current_user_sid, &str2))
- to_free2 = str2;
+ if (ConvertSidToStringSidA(sid, &str2))
+ to_local_free2 = str2;
else
str2 = "(inconvertible)";
+
+ if (!current_user_sid) {
+ str3 = "(none)";
+ str4 = "(none)";
+ }
+ else if (!IsValidSid(current_user_sid)) {
+ str3 = "(invalid)";
+ str4 = "(invalid)";
+ } else {
+ if (user_sid_to_user_name(current_user_sid,
+ &str3))
+ to_free3 = str3;
+ else
+ str3 = "(inconvertible)";
+ if (ConvertSidToStringSidA(current_user_sid,
+ &str4))
+ to_local_free4 = str4;
+ else
+ str4 = "(inconvertible)";
+ }
strbuf_addf(report,
"'%s' is owned by:\n"
- "\t'%s'\nbut the current user is:\n"
- "\t'%s'\n", path, str1, str2);
- LocalFree(to_free1);
- LocalFree(to_free2);
+ "\t%s (%s)\nbut the current user is:\n"
+ "\t%s (%s)\n",
+ path, str1, str2, str3, str4);
+ free(to_free1);
+ LocalFree(to_local_free2);
+ free(to_free3);
+ LocalFree(to_local_free4);
}
}
--
2.39.2
^ permalink raw reply related
* Re: [PATCH v6 0/1] mingw: give more details about unsafe directory's
From: Dragan Simic @ 2024-01-08 17:51 UTC (permalink / raw)
To: Sören Krecker; +Cc: git, sunshine, j6t
In-Reply-To: <20240108173837.20480-1-soekkle@freenet.de>
On 2024-01-08 18:38, Sören Krecker wrote:
> I have processed the points raised.
If I may suggest, there's no need for a cover letter for a single patch.
If you want to include some notes in the patch submission, which aren't
supposed to be part of the commit summary, you can do that in the patch
itself.
> Sören Krecker (1):
> mingw: give more details about unsafe directory's ownership
>
> compat/mingw.c | 70 ++++++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 57 insertions(+), 13 deletions(-)
>
>
> base-commit: e79552d19784ee7f4bbce278fe25f93fbda196fa
^ permalink raw reply
* Re: [PATCH] branch: clarify <oldbranch> term
From: Junio C Hamano @ 2024-01-08 18:04 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List
In-Reply-To: <d38e5a18-4d85-48f3-bc8b-8ca02ea683a4@gmail.com>
Rubén Justo <rjusto@gmail.com> writes:
> Since 52d59cc645 (branch: add a --copy (-c) option to go with --move
> (-m), 2017-06-18) <oldbranch> is used in more operations than just -m.
>
> Let's also clarify what we do if <oldbranch> is omitted.
>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
> Documentation/git-branch.txt | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
> index 4395aa9354..233264549c 100644
> --- a/Documentation/git-branch.txt
> +++ b/Documentation/git-branch.txt
> @@ -312,7 +312,8 @@ superproject's "origin/main", but tracks the submodule's "origin/main".
> option is omitted, the current HEAD will be used instead.
>
> <oldbranch>::
> - The name of an existing branch to rename.
> + The name of an existing branch. If this option is omitted,
> + the name of the current branch will be used instead.
Thanks.
Will queue this patch as is for now, but I suspect that in the
longer term it will help readers a lot to revamp the whole page.
The description for the "branch rename" operation, for example, is
split and partly repreated in three places, i.e.
* a paragraph in the DESCRIPTION that talks about <oldbranch>,
where readers will be helped by the same clarification as this
patch gives;
* -m/-M option description which is very sketchy and does not even
hint that they take <oldbranch> and <newbranch>; and
* <oldbranch> description as a separate bullet item in the same
list, which does not even hint that this is used by -m/-M.
which is unmanageable from writers' point of view, and hard to
follow from readers' point of view.
Such a rewrite may look like this:
* Trim down the DESCRIPTION explanation to enumerate "features"
offered, with pointers into the OPTIONS section using the option
names as hints, e.g.,
The command offers many features that work on branches,
depending on the options.
- The default mode of operation is to list (the --list option
explicitly calls for it, or the lack of other options to
trigger different mode).
- ...
- An existing branch can be renamed to a different name with
"-m/-M" options.
- ...
* Enhance the description in the OPTIONS section so that each
bullet item (e.g., "-m") gives everything the user wants to know
about that option, e.g.,
-m [<oldbranch>] <newbranch>::
-M [<oldbranch>] <newbranch>::
Rename <oldbranch> (defaults to the current branch) to
<newbranch>. If <newbranch> already exists, `-m` will error
out and renaming must be forced by using `-M` (in other
words, `-M` works as a short-hand for `-m --force`).
+
The contents reflog of the <oldbranch> will also be renamed to become
the reflog of the <newbranch>, and a reflog entry is added for
the renaming of the branch.
* Remove the non-option entries from the OPTIONS bullet list.
^ permalink raw reply
* Re: Storing private config files in .git directory?
From: Junio C Hamano @ 2024-01-08 18:20 UTC (permalink / raw)
To: Stefan Haller; +Cc: git
In-Reply-To: <8e344dee-f84e-4a2c-835a-406ee72d129b@haller-berlin.de>
Stefan Haller <lists@haller-berlin.de> writes:
> Our git client (lazygit) has a need to store per-repo config files that
> override the global one, much like git itself. The easiest way to do
> that is to store those in a .git/lazygit.cfg file, and I'm wondering if
> there's any reason why this is a bad idea?
An obvious alternative is to have .lazygit directory next to .git directory
which would give you a bigger separation, which can cut both ways.
^ permalink raw reply
* Re: [PATCH 1/1] completion: dir-type optargs for am, format-patch
From: Junio C Hamano @ 2024-01-08 18:37 UTC (permalink / raw)
To: Britton Leo Kerin; +Cc: git
In-Reply-To: <8a8dfcfc-3369-42b8-8387-f1af33202b16@smtp-relay.sendinblue.com>
"Britton Leo Kerin" <britton.kerin@gmail.com> writes:
> + local context_dir=$(__git rev-parse --show-toplevel --show-prefix 2>/dev/null | paste -s -d '/' 2>/dev/null)
Is there a practical difference with the above with
context_dir=$(pwd)
other than that it will give an empty string outside a git working tree?
If not, I suspect
local inside
inside=$(__git rev-parse --is-inside-work-tree) &&
test "$inside" = true || return
local context_dir=$(pwd)
might be clearer on the intent.
> + [ -d "$context_dir" ] || return
> +
> + COMPREPLY=$(cd $context_dir 2>/dev/null && compgen -d -- "$cur_")
Can $context_dir contain $IFS whitespaces here?
> +}
^ permalink raw reply
* Re: [PATCH 1/1] completion: don't comp revs when --no-format-patch
From: Junio C Hamano @ 2024-01-08 18:39 UTC (permalink / raw)
To: Britton Leo Kerin; +Cc: git
In-Reply-To: <ce76a31e-d435-477e-803c-92d0532174eb@smtp-relay.sendinblue.com>
>Subject: Re: [PATCH 1/1] completion: don't comp revs when --no-format-patch
I suspect "comp revs" -> "complete revs" is what you meant, but
if so, please spell it out to help folks who read "git shortlog --no-merges"
output, i.e. without help from the body of the log message.
^ permalink raw reply
* Re: Storing private config files in .git directory?
From: Konstantin Ryabitsev @ 2024-01-08 18:56 UTC (permalink / raw)
To: Stefan Haller; +Cc: git
In-Reply-To: <8e344dee-f84e-4a2c-835a-406ee72d129b@haller-berlin.de>
On Sun, Jan 07, 2024 at 02:03:20PM +0100, Stefan Haller wrote:
> Our git client (lazygit) has a need to store per-repo config files that
> override the global one, much like git itself. The easiest way to do
> that is to store those in a .git/lazygit.cfg file, and I'm wondering if
> there's any reason why this is a bad idea?
I have considered the same question for b4 as well, but I chose to just rely
on git's config file handling instead of any other option. There's a large
number of people who tend to deal with weird repository situations by blowing
away the entire repo and then recloning it. They may remember to back up the
.git/config file, but not really anything else.
So, that would be the only consideration against keeping anything in the .git
directory.
-K
^ permalink raw reply
* Re: [PATCH v7 1/1] mingw: give more details about unsafe directory's ownership
From: Junio C Hamano @ 2024-01-08 19:18 UTC (permalink / raw)
To: Sören Krecker; +Cc: git, sunshine, j6t
In-Reply-To: <20240108173837.20480-2-soekkle@freenet.de>
Sören Krecker <soekkle@freenet.de> writes:
> Add domain/username in error message, if owner sid of repository and
> user sid are not equal on windows systems.
Will queue. "git am --whitespace=fix" noticed numerous whitespace
breakages in the patch, which has been corrected on the receiving
end. Please make sure your future patches will not have such
problems.
Thanks.
$ git am -s3c ./+sk-v7-mingw-ownership-report
Applying: mingw: give more details about unsafe directory's ownership
.git/rebase-apply/patch:23: trailing whitespace.
&pe_use);
.git/rebase-apply/patch:27: trailing whitespace.
ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user);
.git/rebase-apply/patch:45: trailing whitespace.
LPSTR str1, str2, str3, str4, to_free1 = NULL,
warning: 3 lines add whitespace errors.
.git/rebase-apply/patch:23: trailing whitespace.
&pe_use);
.git/rebase-apply/patch:27: trailing whitespace.
ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user);
.git/rebase-apply/patch:45: trailing whitespace.
LPSTR str1, str2, str3, str4, to_free1 = NULL,
warning: 3 lines applied after fixing whitespace errors.
Using index info to reconstruct a base tree...
M compat/mingw.c
Falling back to patching base and 3-way merge...
Auto-merging compat/mingw.c
^ 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