* [PATCH v2 1/4] refs: introduce `is_pseudoref()` and `is_headref()`
From: Karthik Nayak @ 2024-01-24 15:27 UTC (permalink / raw)
To: git; +Cc: gitster, ps, phillip.wood123, Karthik Nayak
In-Reply-To: <20240124152726.124873-1-karthik.188@gmail.com>
Introduce two new functions `is_pseudoref()` and `is_headref()`. This
provides the necessary functionality for us to add pseudorefs and HEAD
to the loose ref cache in the files backend, allowing us to build
tooling to print these refs.
The `is_pseudoref()` function internally calls `is_pseudoref_syntax()`
but adds onto it by also checking to ensure that the pseudoref either
ends with a "_HEAD" suffix or matches a list of exceptions. After which
we also parse the contents of the pseudoref to ensure that it conforms
to the ref format.
We cannot directly add the new syntax checks to `is_pseudoref_syntax()`
because the function is also used by `is_current_worktree_ref()` and
making it stricter to match only known pseudorefs might have unintended
consequences due to files like 'BISECT_START' which isn't a pseudoref
but sometimes contains object ID.
Keeping this in mind, we leave `is_pseudoref_syntax()` as is and create
`is_pseudoref()` which is stricter. Ideally we'd want to move the new
syntax checks to `is_pseudoref_syntax()` but a prerequisite for this
would be to actually remove the exception list by converting those
pseudorefs to also contain a '_HEAD' suffix and perhaps move bisect
related files like 'BISECT_START' to a new directory similar to the
'rebase-merge' directory.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
refs.c | 32 ++++++++++++++++++++++++++++++++
refs.h | 3 +++
2 files changed, 35 insertions(+)
diff --git a/refs.c b/refs.c
index 20e8f1ff1f..4b6bfc66fb 100644
--- a/refs.c
+++ b/refs.c
@@ -859,6 +859,38 @@ static int is_pseudoref_syntax(const char *refname)
return 1;
}
+int is_pseudoref(struct ref_store *refs, const char *refname)
+{
+ static const char *const irregular_pseudorefs[] = {
+ "AUTO_MERGE",
+ "BISECT_EXPECTED_REV",
+ "NOTES_MERGE_PARTIAL",
+ "NOTES_MERGE_REF",
+ "MERGE_AUTOSTASH"
+ };
+ size_t i;
+
+ if (!is_pseudoref_syntax(refname))
+ return 0;
+
+ if (ends_with(refname, "_HEAD"))
+ return refs_ref_exists(refs, refname);
+
+ for (i = 0; i < ARRAY_SIZE(irregular_pseudorefs); i++)
+ if (!strcmp(refname, irregular_pseudorefs[i]))
+ return refs_ref_exists(refs, refname);
+
+ return 0;
+}
+
+int is_headref(struct ref_store *refs, const char *refname)
+{
+ if (!strcmp(refname, "HEAD"))
+ return refs_ref_exists(refs, refname);
+
+ return 0;
+}
+
static int is_current_worktree_ref(const char *ref) {
return is_pseudoref_syntax(ref) || is_per_worktree_ref(ref);
}
diff --git a/refs.h b/refs.h
index 11b3b6ccea..46b8085d63 100644
--- a/refs.h
+++ b/refs.h
@@ -1021,4 +1021,7 @@ extern struct ref_namespace_info ref_namespace[NAMESPACE__COUNT];
*/
void update_ref_namespace(enum ref_namespace namespace, char *ref);
+int is_pseudoref(struct ref_store *refs, const char *refname);
+int is_headref(struct ref_store *refs, const char *refname);
+
#endif /* REFS_H */
--
2.43.GIT
^ permalink raw reply related
* [PATCH v2 0/4] for-each-ref: print all refs on empty string pattern
From: Karthik Nayak @ 2024-01-24 15:27 UTC (permalink / raw)
To: git; +Cc: gitster, ps, phillip.wood123, Karthik Nayak
In-Reply-To: <20240119142705.139374-1-karthik.188@gmail.com>
This is the second version of my patch series to print refs
when and empty string pattern is used with git-for-each-ref(1).
With the upcoming introduction of the reftable backend, it becomes ever
so important to provide the necessary tooling for printing all refs
associated with a repository.
While regular refs stored within the "refs/" namespace are currently
supported by multiple commands like git-for-each-ref(1),
git-show-ref(1). Neither support printing all the operational refs
within the repository.
This is crucial because with the reftable backend, all these refs will
also move to reftable. It would be necessary to identify all the refs
that are stored within the reftable since there is no easy way to do so
otherwise. This is because the reftable itself is a binary format and
all access will be via git. Unlike the filesystem backend, which allows
access directly via the filesystem.
This patch series is a follow up to the RFC/discussion we had earlier on
the list [1].
The first 4 commits add the required functionality to ensure we can print
all refs (regular, pseudo, HEAD). The 5th commit modifies the
git-for-each-ref(1) command to print all refs when an empty string pattern
is used. This is a deviation from the current situation wherein the empty
string pattern currently matches and prints no refs.
[1]: https://lore.kernel.org/git/20231221170715.110565-1-karthik.188@gmail.com/#t
Changes since v1:
- Introduce `is_pseudoref()` and `is_headref()` and use them instead of
directly using `is_pseudoref_syntax`.
- Rename `add_pseudoref_like_entries()` to `add_pseudoref_and_head_entries()`
since it also adds the HEAD ref.
- Also check for the pseudoref's contents to ensure it conforms to the ref
format.
Karthik Nayak (4):
refs: introduce `is_pseudoref()` and `is_headref()`
refs: extract out `loose_fill_ref_dir_regular_file()`
refs: introduce `refs_for_each_all_refs()`
for-each-ref: avoid filtering on empty pattern
Documentation/git-for-each-ref.txt | 3 +-
builtin/for-each-ref.c | 21 ++++-
ref-filter.c | 13 ++-
ref-filter.h | 4 +-
refs.c | 39 +++++++++
refs.h | 9 ++
refs/files-backend.c | 127 +++++++++++++++++++++--------
refs/refs-internal.h | 7 ++
t/t6302-for-each-ref-filter.sh | 34 ++++++++
9 files changed, 218 insertions(+), 39 deletions(-)
--
2.43.GIT
^ permalink raw reply
* Re: Fwd: Unexpected behavior of ls-files command when using --others --exclude-from, and a .gitignore file which resides in a subdirectory
From: Raúl Núñez de Arenas Coronado @ 2024-01-24 14:22 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20240124010924.GB2603087@coredump.intra.peff.net>
Hi Jeff!
El mié, 24 ene 2024 a las 2:09, Jeff King (<peff@peff.net>) escribió:
> (I think this also gives an interesting use case arguing for continuing
> to support those more-specific exclusion options for ls-files).
>
> If you are mostly just using core.excludesFile (and not
> .git/info/exclude), then I suspect:
>
> git -c core.excludesFile /dev/null ls-files -o --exclude-standard
>
> would work for you, too (but it sounds like you might also be using
> .git/info/exclude).
In my case, since I'm also using .git/info/exclude in some repos, this
won't work as-is, but you gave me an idea, and I think I can get what
I need until the "precious" mechanism is implemented. It requires a
bit of hacking in the backup script but is not a big deal and it will
work for all my current repos.
Thanks for the help!
--
Raúl Núñez de Arenas Coronado
.
^ permalink raw reply
* [PATCH 2/2] FIX memory leak in one branch
From: Md Isfarul Haque via GitGitGadget @ 2024-01-24 14:04 UTC (permalink / raw)
To: git; +Cc: Md Isfarul Haque, Md Isfarul Haque
In-Reply-To: <pull.1653.git.git.1706105064.gitgitgadget@gmail.com>
From: Md Isfarul Haque <isfarul.876@gmail.com>
Signed-off-by: Md Isfarul Haque <isfarul.876@gmail.com>
---
diff.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/diff.c b/diff.c
index e3223b8ce5b..9fa00103a6b 100644
--- a/diff.c
+++ b/diff.c
@@ -2309,6 +2309,7 @@ const struct strbuf *diff_line_prefix_buf(struct diff_options *opt)
msgbuf->alloc = 1;
}
else {
+ free(msgbuf);
msgbuf = opt->output_prefix(opt, opt->output_prefix_data);
}
return msgbuf;
--
gitgitgadget
^ permalink raw reply related
* [PATCH 1/2] FIX: use utf8_strnwidth for line_prefix in diff.c
From: Md Isfarul Haque via GitGitGadget @ 2024-01-24 14:04 UTC (permalink / raw)
To: git; +Cc: Md Isfarul Haque, Md Isfarul Haque
In-Reply-To: <pull.1653.git.git.1706105064.gitgitgadget@gmail.com>
From: Md Isfarul Haque <isfarul.876@gmail.com>
This patch adresses diff.c:2721 and proposes the fix using a new function.
Signed-off-by: Md Isfarul Haque <isfarul.876@gmail.com>
---
diff.c | 18 ++++++++++++++++--
diff.h | 1 +
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/diff.c b/diff.c
index a89a6a6128a..e3223b8ce5b 100644
--- a/diff.c
+++ b/diff.c
@@ -2300,6 +2300,20 @@ const char *diff_line_prefix(struct diff_options *opt)
return msgbuf->buf;
}
+const struct strbuf *diff_line_prefix_buf(struct diff_options *opt)
+{
+ struct strbuf *msgbuf = (struct strbuf *)malloc(sizeof(*msgbuf));
+ if (!opt->output_prefix){
+ msgbuf->buf = "";
+ msgbuf->len = 0;
+ msgbuf->alloc = 1;
+ }
+ else {
+ msgbuf = opt->output_prefix(opt, opt->output_prefix_data);
+ }
+ return msgbuf;
+}
+
static unsigned long sane_truncate_line(char *line, unsigned long len)
{
const char *cp;
@@ -2635,7 +2649,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
int width, name_width, graph_width, number_width = 0, bin_width = 0;
const char *reset, *add_c, *del_c;
int extra_shown = 0;
- const char *line_prefix = diff_line_prefix(options);
+ const struct strbuf *line_prefix = diff_line_prefix_buf(options);
struct strbuf out = STRBUF_INIT;
if (data->nr == 0)
@@ -2718,7 +2732,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
* used to correctly count the display width instead of strlen().
*/
if (options->stat_width == -1)
- width = term_columns() - strlen(line_prefix);
+ width = term_columns() - utf8_strnwidth(line_prefix->buf, line_prefix->len, 1);
else
width = options->stat_width ? options->stat_width : 80;
number_width = decimal_width(max_change) > number_width ?
diff --git a/diff.h b/diff.h
index 66bd8aeb293..6eb8dc9a97e 100644
--- a/diff.h
+++ b/diff.h
@@ -460,6 +460,7 @@ const char *diff_get_color(int diff_use_color, enum color_diff ix);
const char *diff_line_prefix(struct diff_options *);
+const struct strbuf *diff_line_prefix_buf(struct diff_options *);
extern const char mime_boundary_leader[];
--
gitgitgadget
^ permalink raw reply related
* [PATCH 0/2] [GSoC][RFC/Patch] FIX: use utf8_strnwidth for line_prefix in diff.c
From: Md Isfarul Haque via GitGitGadget @ 2024-01-24 14:04 UTC (permalink / raw)
To: git; +Cc: Md Isfarul Haque
This patch adresses diff.c:2721 and proposes the fix using a new function.
Md Isfarul Haque (2):
FIX: use utf8_strnwidth for line_prefix in diff.c
FIX memory leak in one branch
diff.c | 19 +++++++++++++++++--
diff.h | 1 +
2 files changed, 18 insertions(+), 2 deletions(-)
base-commit: e02ecfcc534e2021aae29077a958dd11c3897e4c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1653%2FInnocentZero%2Fdiff_needswork-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1653/InnocentZero/diff_needswork-v1
Pull-Request: https://github.com/git/git/pull/1653
--
gitgitgadget
^ permalink raw reply
* [PATCH v2] reftable/stack: adjust permissions of compacted tables
From: Patrick Steinhardt @ 2024-01-24 12:53 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1706099090.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3062 bytes --]
When creating a new compacted table from a range of preexisting ones we
don't set the default permissions on the resulting table when specified
by the user. This has the effect that the "core.sharedRepository" config
will not be honored correctly.
Fix this bug and add a test to catch this issue.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Ugh, I totally forgot about the fact that fchmod(3P) isn't actually
available on Windows. I've thus evicted the first patch and changed the
second patch to use chmod(3P) instead. Too bad.
reftable/stack.c | 6 ++++++
reftable/stack_test.c | 25 +++++++++++++++++++++++--
2 files changed, 29 insertions(+), 2 deletions(-)
diff --git a/reftable/stack.c b/reftable/stack.c
index 7ffeb3ee10..38e784a8ab 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -731,6 +731,12 @@ static int stack_compact_locked(struct reftable_stack *st, int first, int last,
strbuf_addstr(temp_tab, ".temp.XXXXXX");
tab_fd = mkstemp(temp_tab->buf);
+ if (st->config.default_permissions &&
+ chmod(temp_tab->buf, st->config.default_permissions) < 0) {
+ err = REFTABLE_IO_ERROR;
+ goto done;
+ }
+
wr = reftable_new_writer(reftable_fd_write, &tab_fd, &st->config);
err = stack_write_compact(st, wr, first, last, config);
diff --git a/reftable/stack_test.c b/reftable/stack_test.c
index 289e902146..2e7d1768b7 100644
--- a/reftable/stack_test.c
+++ b/reftable/stack_test.c
@@ -443,15 +443,16 @@ static void test_reftable_stack_add(void)
int err = 0;
struct reftable_write_options cfg = {
.exact_log_message = 1,
+ .default_permissions = 0660,
};
struct reftable_stack *st = NULL;
char *dir = get_tmp_dir(__LINE__);
-
struct reftable_ref_record refs[2] = { { NULL } };
struct reftable_log_record logs[2] = { { NULL } };
+ struct strbuf scratch = STRBUF_INIT;
+ struct stat stat_result;
int N = ARRAY_SIZE(refs);
-
err = reftable_new_stack(&st, dir, cfg);
EXPECT_ERR(err);
st->disable_auto_compact = 1;
@@ -509,12 +510,32 @@ static void test_reftable_stack_add(void)
reftable_log_record_release(&dest);
}
+#ifndef GIT_WINDOWS_NATIVE
+ strbuf_addstr(&scratch, dir);
+ strbuf_addstr(&scratch, "/tables.list");
+ err = stat(scratch.buf, &stat_result);
+ EXPECT(!err);
+ EXPECT((stat_result.st_mode & 0777) == cfg.default_permissions);
+
+ strbuf_reset(&scratch);
+ strbuf_addstr(&scratch, dir);
+ strbuf_addstr(&scratch, "/");
+ /* do not try at home; not an external API for reftable. */
+ strbuf_addstr(&scratch, st->readers[0]->name);
+ err = stat(scratch.buf, &stat_result);
+ EXPECT(!err);
+ EXPECT((stat_result.st_mode & 0777) == cfg.default_permissions);
+#else
+ (void) stat_result;
+#endif
+
/* cleanup */
reftable_stack_destroy(st);
for (i = 0; i < N; i++) {
reftable_ref_record_release(&refs[i]);
reftable_log_record_release(&logs[i]);
}
+ strbuf_release(&scratch);
clear_dir(dir);
}
base-commit: e02ecfcc534e2021aae29077a958dd11c3897e4c
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 2/2] reftable/stack: adjust permissions of compacted tables
From: Patrick Steinhardt @ 2024-01-24 12:31 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1706099090.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2804 bytes --]
When creating a new compacted table from a range of preexisting ones we
don't set the default permissions on the resulting table when specified
by the user. This has the effect that the "core.sharedRepository" config
will not be honored correctly.
Fix this bug and add a test to catch this issue.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/stack.c | 6 ++++++
reftable/stack_test.c | 25 +++++++++++++++++++++++--
2 files changed, 29 insertions(+), 2 deletions(-)
diff --git a/reftable/stack.c b/reftable/stack.c
index c6e4dc4b2b..27cc586460 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -731,6 +731,12 @@ static int stack_compact_locked(struct reftable_stack *st, int first, int last,
strbuf_addstr(temp_tab, ".temp.XXXXXX");
tab_fd = mkstemp(temp_tab->buf);
+ if (st->config.default_permissions &&
+ fchmod(tab_fd, st->config.default_permissions) < 0) {
+ err = REFTABLE_IO_ERROR;
+ goto done;
+ }
+
wr = reftable_new_writer(reftable_fd_write, &tab_fd, &st->config);
err = stack_write_compact(st, wr, first, last, config);
diff --git a/reftable/stack_test.c b/reftable/stack_test.c
index 289e902146..2e7d1768b7 100644
--- a/reftable/stack_test.c
+++ b/reftable/stack_test.c
@@ -443,15 +443,16 @@ static void test_reftable_stack_add(void)
int err = 0;
struct reftable_write_options cfg = {
.exact_log_message = 1,
+ .default_permissions = 0660,
};
struct reftable_stack *st = NULL;
char *dir = get_tmp_dir(__LINE__);
-
struct reftable_ref_record refs[2] = { { NULL } };
struct reftable_log_record logs[2] = { { NULL } };
+ struct strbuf scratch = STRBUF_INIT;
+ struct stat stat_result;
int N = ARRAY_SIZE(refs);
-
err = reftable_new_stack(&st, dir, cfg);
EXPECT_ERR(err);
st->disable_auto_compact = 1;
@@ -509,12 +510,32 @@ static void test_reftable_stack_add(void)
reftable_log_record_release(&dest);
}
+#ifndef GIT_WINDOWS_NATIVE
+ strbuf_addstr(&scratch, dir);
+ strbuf_addstr(&scratch, "/tables.list");
+ err = stat(scratch.buf, &stat_result);
+ EXPECT(!err);
+ EXPECT((stat_result.st_mode & 0777) == cfg.default_permissions);
+
+ strbuf_reset(&scratch);
+ strbuf_addstr(&scratch, dir);
+ strbuf_addstr(&scratch, "/");
+ /* do not try at home; not an external API for reftable. */
+ strbuf_addstr(&scratch, st->readers[0]->name);
+ err = stat(scratch.buf, &stat_result);
+ EXPECT(!err);
+ EXPECT((stat_result.st_mode & 0777) == cfg.default_permissions);
+#else
+ (void) stat_result;
+#endif
+
/* cleanup */
reftable_stack_destroy(st);
for (i = 0; i < N; i++) {
reftable_ref_record_release(&refs[i]);
reftable_log_record_release(&logs[i]);
}
+ strbuf_release(&scratch);
clear_dir(dir);
}
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 1/2] reftable/stack: use fchmod(3P) to set permissions
From: Patrick Steinhardt @ 2024-01-24 12:31 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1706099090.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2406 bytes --]
We use chmod(3P) to modify permissions of "tables.list" locks as well as
temporary new tables we're writing. In all of these cases we do have a
file descriptor readily available though. So instead of using chmod(3P)
we can use fchmod(3P), which should both be more efficient while also
avoiding a potential race where we change permissions of the wrong file
in case it was swapped out after we have created it.
Refactor the code to do so.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/stack.c | 29 ++++++++++++++---------------
1 file changed, 14 insertions(+), 15 deletions(-)
diff --git a/reftable/stack.c b/reftable/stack.c
index 7ffeb3ee10..c6e4dc4b2b 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -467,11 +467,11 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
}
goto done;
}
- if (st->config.default_permissions) {
- if (chmod(add->lock_file->filename.buf, st->config.default_permissions) < 0) {
- err = REFTABLE_IO_ERROR;
- goto done;
- }
+ if (st->config.default_permissions &&
+ fchmod(get_tempfile_fd(add->lock_file),
+ st->config.default_permissions) < 0) {
+ err = REFTABLE_IO_ERROR;
+ goto done;
}
err = stack_uptodate(st);
@@ -633,12 +633,12 @@ int reftable_addition_add(struct reftable_addition *add,
err = REFTABLE_IO_ERROR;
goto done;
}
- if (add->stack->config.default_permissions) {
- if (chmod(temp_tab_file_name.buf, add->stack->config.default_permissions)) {
- err = REFTABLE_IO_ERROR;
- goto done;
- }
+ if (add->stack->config.default_permissions &&
+ fchmod(tab_fd, add->stack->config.default_permissions) < 0) {
+ err = REFTABLE_IO_ERROR;
+ goto done;
}
+
wr = reftable_new_writer(reftable_fd_write, &tab_fd,
&add->stack->config);
err = write_table(wr, arg);
@@ -967,11 +967,10 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last,
goto done;
}
have_lock = 1;
- if (st->config.default_permissions) {
- if (chmod(lock_file_name.buf, st->config.default_permissions) < 0) {
- err = REFTABLE_IO_ERROR;
- goto done;
- }
+ if (st->config.default_permissions &&
+ fchmod(lock_file_fd, st->config.default_permissions) < 0) {
+ err = REFTABLE_IO_ERROR;
+ goto done;
}
format_name(&new_table_name, st->readers[first]->min_update_index,
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 0/2] reftable: adjust permissions of compacted tables
From: Patrick Steinhardt @ 2024-01-24 12:31 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 2486 bytes --]
Hi,
this small patch series fixes a bug in the reftable stack implementation
where the "default_permissions" write option is not honored for newly
written compacted tables. The effect is that the "core.sharedRepository"
config is ignored when performing compaction.
The series runs into two minor conflicts with jc/reftable-core-fsync
that can be fixed like follows. As this series has been merged to next
already I'd also be happy to rebase these patches on top of it. Please
let me know your preference.
Patrick
diff --cc reftable/stack.c
index 27cc586460,ab295341cc..0000000000
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@@ -633,13 -643,13 +643,13 @@@ int reftable_addition_add(struct reftab
err = REFTABLE_IO_ERROR;
goto done;
}
- if (add->stack->config.default_permissions) {
- if (chmod(temp_tab_file_name.buf, add->stack->config.default_permissions)) {
- err = REFTABLE_IO_ERROR;
- goto done;
- }
+ if (add->stack->config.default_permissions &&
+ fchmod(tab_fd, add->stack->config.default_permissions) < 0) {
+ err = REFTABLE_IO_ERROR;
+ goto done;
}
+
- wr = reftable_new_writer(reftable_fd_write, &tab_fd,
+ wr = reftable_new_writer(reftable_fd_write, reftable_fd_flush, &tab_fd,
&add->stack->config);
err = write_table(wr, arg);
if (err < 0)
@@@ -731,13 -741,7 +741,13 @@@ static int stack_compact_locked(struct
strbuf_addstr(temp_tab, ".temp.XXXXXX");
tab_fd = mkstemp(temp_tab->buf);
+ if (st->config.default_permissions &&
+ fchmod(tab_fd, st->config.default_permissions) < 0) {
+ err = REFTABLE_IO_ERROR;
+ goto done;
+ }
+
- wr = reftable_new_writer(reftable_fd_write, &tab_fd, &st->config);
+ wr = reftable_new_writer(reftable_fd_write, reftable_fd_flush, &tab_fd, &st->config);
err = stack_write_compact(st, wr, first, last, config);
if (err < 0)
Patrick Steinhardt (2):
reftable/stack: use fchmod(3P) to set permissions
reftable/stack: adjust permissions of compacted tables
reftable/stack.c | 35 ++++++++++++++++++++---------------
reftable/stack_test.c | 25 +++++++++++++++++++++++--
2 files changed, 43 insertions(+), 17 deletions(-)
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/4] sequencer: Do not require `allow_empty` for redundant commit options
From: Phillip Wood @ 2024-01-24 11:01 UTC (permalink / raw)
To: brianmlyles, git; +Cc: me, newren
In-Reply-To: <7bf5036b-9f55-4451-a13c-8a2c815dfbb7@gmail.com>
Hi Brian
On 23/01/2024 14:23, Phillip Wood wrote:
> rebase always sets "opts->allow_empty = 1" in
> builtin/rebase.c:get_replay_opts() and if the user passes
> --no-keep-empty drops commits that start empty from the list of commits
> to be picked. This is slightly confusing but is more efficient as we
> don't do waste time trying to pick a commit we're going to drop. Can we
> do something similar for "git cherry-pick"? When cherry-picking a
> sequence of commits I think it should just work because the code is
> shared with rebase, for a single commit we'd need to add a test to see
> if it is empty in single_pick() before calling pick_commits().
Having thought about this again I don't think we can reuse the same
approach as rebase because cherry-pick and rebase have different
behaviors. "git rebase --no-keep-empty" drops empty commits whereas "git
cherry-pick" wants to error out if it sees an empty commit. So I think
your approach of reworking allow_empty() in the sequencer is the right
approach.
Sorry for the confusion
Phillip
^ permalink raw reply
* Re: [PATCH 1/4] sequencer: Do not require `allow_empty` for redundant commit options
From: Phillip Wood @ 2024-01-24 11:01 UTC (permalink / raw)
To: brianmlyles, git; +Cc: me, newren
In-Reply-To: <20240119060721.3734775-2-brianmlyles@gmail.com>
Hi Brian
Here are some code comments now I've realized why we need to change it
On 19/01/2024 05:59, brianmlyles@gmail.com wrote:
> From: Brian Lyles <brianmlyles@gmail.com>
> Documentation/git-cherry-pick.txt | 10 +++++++---
> builtin/revert.c | 4 ----
> sequencer.c | 18 ++++++++++--------
> t/t3505-cherry-pick-empty.sh | 5 +++++
> 4 files changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/git-cherry-pick.txt
b/Documentation/git-cherry-pick.txt
> index fdcad3d200..806295a730 100644
> --- a/Documentation/git-cherry-pick.txt
> +++ b/Documentation/git-cherry-pick.txt
> @@ -131,8 +131,8 @@ effect to your index in a row.
> even without this option. Note also, that use of this option only
> keeps commits that were initially empty (i.e. the commit
recorded the
> same tree as its parent). Commits which are made empty due to a
> - previous commit are dropped. To force the inclusion of those
commits
> - use `--keep-redundant-commits`.
> + previous commit will cause the cherry-pick to fail. To force the
> + inclusion of those commits use `--keep-redundant-commits`.
>
> --allow-empty-message::
> By default, cherry-picking a commit with an empty message will
fail.
> @@ -144,7 +144,11 @@ effect to your index in a row.
> current history, it will become empty. By default these
> redundant commits cause `cherry-pick` to stop so the user can
> examine the commit. This option overrides that behavior and
> - creates an empty commit object. Implies `--allow-empty`.
> + creates an empty commit object. Note that use of this option only
> + results in an empty commit when the commit was not initially empty,
> + but rather became empty due to a previous commit. Commits that were
> + initially empty will cause the cherry-pick to fail. To force the
> + inclusion of those commits use `--allow-empty`.
>
> --strategy=<strategy>::
> Use the given merge strategy. Should only be used once.
> diff --git a/builtin/revert.c b/builtin/revert.c
> index e6f9a1ad26..b2cfde7a87 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -136,10 +136,6 @@ static int run_sequencer(int argc, const char
**argv, const char *prefix,
> prepare_repo_settings(the_repository);
> the_repository->settings.command_requires_full_index = 0;
>
> - /* implies allow_empty */
> - if (opts->keep_redundant_commits)
> - opts->allow_empty = 1;
I'm wary of this, especially after Juino's comments in
https://lore.kernel.org/git/xmqqy1cfnca7.fsf@gitster.g/
The documentation changes look if we do decide to change the default.
> if (cleanup_arg) {
> opts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1);
> opts->explicit_cleanup = 1;
> diff --git a/sequencer.c b/sequencer.c
> index d584cac8ed..582bde8d46 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1739,22 +1739,24 @@ static int allow_empty(struct repository *r,
> *
> * (4) we allow both.
> */
The comment above should be updated if we change the behavior of this
function.
> - if (!opts->allow_empty)
> - return 0; /* let "git commit" barf as necessary */
> -
Here we stop returning early if allow_empty is not set - this allows us
to apply allow_empty only to commits that start empty below.
> index_unchanged = is_index_unchanged(r);
> - if (index_unchanged < 0)
> + if (index_unchanged < 0) {
> + if (!opts->allow_empty)
> + return 0;
> return index_unchanged;
> + }
I don't think we want to hide the error here or below from
originally_empty()
> if (!index_unchanged)
> return 0; /* we do not have to say --allow-empty */
>
> - if (opts->keep_redundant_commits)
> - return 1;
> -
We move this check so that we do not unconditionally keep commits that
are initially empty when opts->keep_redundant_commits is set.
> originally_empty = is_original_commit_empty(commit);
> - if (originally_empty < 0)
> + if (originally_empty < 0) {
> + if (!opts->allow_empty)
> + return 0;
> return originally_empty;
> + }
> if (originally_empty)
> + return opts->allow_empty;
Now opts->allow_empty only applies to commits that were originally empty
> + else if (opts->keep_redundant_commits)
> return 1;
This ensures we keep commits that become empty when
opts->redundant_commits is set.
The changes to allow_empty() all looks good apart from hiding errors
from index_unchanged() and originally_empty()
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH 7/7] Documentation: add "special refs" to the glossary
From: Patrick Steinhardt @ 2024-01-24 9:05 UTC (permalink / raw)
To: phillip.wood; +Cc: git
In-Reply-To: <4f60dd83-913d-4c66-989d-282ec1845f4b@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1418 bytes --]
On Tue, Jan 23, 2024 at 04:27:20PM +0000, Phillip Wood wrote:
> Hi Patrick
>
> On 19/01/2024 10:40, Patrick Steinhardt wrote:
> > Add the "special refs" term to our glossary.
>
> Related to this the glossary entry for pseudorefs says
>
> Pseudorefs are a class of files under `$GIT_DIR` which behave
> like refs for the purposes of rev-parse, but which are treated
> specially by git. Pseudorefs both have names that are all-caps,
> and always start with a line consisting of a
> <<def_SHA1,SHA-1>> followed by whitespace. So, HEAD is not a
> pseudoref, because it is sometimes a symbolic ref. They might
> optionally contain some additional data. `MERGE_HEAD` and
> `CHERRY_PICK_HEAD` are examples. Unlike
> <<def_per_worktree_ref,per-worktree refs>>, these files cannot
> be symbolic refs, and never have reflogs. They also cannot be
> updated through the normal ref update machinery. Instead,
> they are updated by directly writing to the files. However,
> they can be read as if they were refs, so `git rev-parse
> MERGE_HEAD` will work.
>
> which is very file-centric. We should probably update that as we're moving
> away from filesystem access except for special refs.
Good point indeed, thanks for the hint! Will update in a future patch
series.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/6] t1300: make tests more robust with non-default ref backends
From: Patrick Steinhardt @ 2024-01-24 8:52 UTC (permalink / raw)
To: Christian Couder; +Cc: git, Taylor Blau, Eric Sunshine
In-Reply-To: <CAP8UFD0p-OqYuTrB5m2uq7BRFko887bKszOLtP5peP-A79g=BA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 5000 bytes --]
On Tue, Jan 23, 2024 at 05:15:48PM +0100, Christian Couder wrote:
> On Wed, Jan 10, 2024 at 10:01 AM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > The t1300 test suite exercises the git-config(1) tool. To do so we
> > overwrite ".git/config" to contain custom contents.
>
> Here "we" means "tests in t1300" I guess.
>
> > While this is easy
> > enough to do, it may create problems when using a non-default repository
> > format because we also overwrite the repository format version as well
> > as any potential extensions.
>
> But I am not sure that "we" in the above sentence is also "tests in
> t1300". I think overwriting the repo format version and potential
> extensions is done by other tests, right? Anyway it would be nice to
> clarify this.
>
> > With the upcoming "reftable" ref backend
> > the result is that we may try to access refs via the "files" backend
> > even though the repository has been initialized with the "reftable"
> > backend.
>
> Not sure here also what "we" is. When could refs be accessed via the
> "files" backend even though the repo was initialized with the
> "reftable" backend?
Yeah, I've rephrased all of these to sey "the tests" or something
similar.
> Does this mean that some of the tests in t1300 try to access refs via
> the "files" backend while we may want to run all the tests using the
> reftable backend?
Exactly. We overwrite the ".git/config", which contains the "refStorage"
extension that tells us to use the "reftable" backend. So the extension
is gone, and thus Git would fall back to use the "files" backend
instead, which will fail.
> > Refactor tests which access the refdb to be more robust by using their
> > own separate repositories, which allows us to be more careful and not
> > discard required extensions.
>
> Not sure what exactly is discarding extensions. Also robust is not
> very clear. It would be better to give at least an example of how
> things could fail.
Hm. I don't really know how to phrase this better. The preceding
paragraph already explains why we're discarding the extension and what
the consequence is. I added a sentence saying ", which will cause
failures when trying to access any refs."
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > t/t1300-config.sh | 74 ++++++++++++++++++++++++++++++-----------------
> > 1 file changed, 48 insertions(+), 26 deletions(-)
> >
> > diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> > index f4e2752134..53c3d65823 100755
> > --- a/t/t1300-config.sh
> > +++ b/t/t1300-config.sh
> > @@ -1099,13 +1099,18 @@ test_expect_success SYMLINKS 'symlink to nonexistent configuration' '
> > '
> >
> > test_expect_success 'check split_cmdline return' "
> > - git config alias.split-cmdline-fix 'echo \"' &&
> > - test_must_fail git split-cmdline-fix &&
> > - echo foo > foo &&
> > - git add foo &&
> > - git commit -m 'initial commit' &&
> > - git config branch.main.mergeoptions 'echo \"' &&
> > - test_must_fail git merge main
> > + test_when_finished 'rm -rf repo' &&
> > + git init repo &&
> > + (
> > + cd repo &&
> > + git config alias.split-cmdline-fix 'echo \"' &&
> > + test_must_fail git split-cmdline-fix &&
> > + echo foo >foo &&
> > + git add foo &&
> > + git commit -m 'initial commit' &&
> > + git config branch.main.mergeoptions 'echo \"' &&
> > + test_must_fail git merge main
> > + )
> > "
>
> Maybe, while at it, this test could be modernized to use single quotes
> around the test code like:
>
> test_expect_success 'check split_cmdline return' '
> ...
> '
>
> or is using double quotes still Ok?
In general single quotes are preferable. This test is using quotes
internally, which I guess is the reason why we didn't. Happy to change
while at it.
[snip]
> > test_expect_success 'git -c does not split values on equals' '
> > @@ -2009,11 +2020,11 @@ test_expect_success '--show-origin getting a single key' '
> > '
> >
> > test_expect_success 'set up custom config file' '
> > - CUSTOM_CONFIG_FILE="custom.conf" &&
> > - cat >"$CUSTOM_CONFIG_FILE" <<-\EOF
> > + cat >"custom.conf" <<-\EOF &&
> > [user]
> > custom = true
> > EOF
> > + CUSTOM_CONFIG_FILE="$(test-tool path-utils real_path custom.conf)"
> > '
>
> This looks like a test modernization, but maybe it's part of making
> the tests more robust. Anyway it might be a good idea to either talk a
> bit about that in the commit message or to move it to a preparatory
> commit if it's a modernization and other modernizations could be made
> in that preparatory commit.
>
> Otherwise this patch looks good to me.
Yup, this has also been pointed out by others. Will mention in the
commit message.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 3/6] t1302: make tests more robust with new extensions
From: Patrick Steinhardt @ 2024-01-24 8:52 UTC (permalink / raw)
To: Christian Couder; +Cc: git, Taylor Blau, Eric Sunshine
In-Reply-To: <CAP8UFD0yh4k4p7zcM+ZF90oWWTUMoGVpd3ajWWOV5pk++YKerw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2623 bytes --]
On Tue, Jan 23, 2024 at 05:15:56PM +0100, Christian Couder wrote:
> On Wed, Jan 10, 2024 at 10:02 AM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > In t1302 we exercise logic around "core.repositoryFormatVersion" and
> > extensions. These tests are not particularly robust against extensions
> > like the newly introduced "refStorage" extension.
>
> Here also it's not very clear what robust means. Some examples of how
> things could fail would perhaps help.
>
> > Refactor the tests to be more robust:
> >
> > - Check the DEFAULT_REPO_FORMAT prereq to determine the expected
> > repository format version. This helps to ensure that we only need to
> > update the prereq in a central place when new extensions are added.
> >
> > - Use a separate repository to rewrite ".git/config" to test
> > combinations of the repository format version and extensions. This
> > ensures that we don't break the main test repository.
> >
> > - Do not rewrite ".git/config" when exercising the "preciousObjects"
> > extension.
>
> It would be nice to talk a bit about the failures that each of the
> above changes could prevent.
They all fail in the same way: we fail to access the repository because
we delete the extension that tells us to use the reftable backend. I'll
add that explanation at the front.
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > t/t1302-repo-version.sh | 19 +++++++++++++++----
> > 1 file changed, 15 insertions(+), 4 deletions(-)
> >
> > diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh
> > index 179474fa65..7c680c91c4 100755
> > --- a/t/t1302-repo-version.sh
> > +++ b/t/t1302-repo-version.sh
> > @@ -28,7 +28,12 @@ test_expect_success 'setup' '
> > '
> >
> > test_expect_success 'gitdir selection on normal repos' '
> > - test_oid version >expect &&
> > + if test_have_prereq DEFAULT_REPO_FORMAT
> > + then
> > + echo 0
> > + else
> > + echo 1
> > + fi >expect &&
> > git config core.repositoryformatversion >actual &&
> > git -C test config core.repositoryformatversion >actual2 &&
> > test_cmp expect actual &&
> > @@ -79,8 +84,13 @@ mkconfig () {
>
> Before that hunk there is:
>
> test_expect_success 'setup' '
> test_oid_cache <<-\EOF &&
> version sha1:0
> version sha256:1
> EOF
>
> and it seems to me that the above test_oid_cache call is not needed
> anymore, if `test_oid version` is not used anymore.
>
> Otherwise this looks good to me.
Good catch.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/5] refs: make `is_pseudoref_syntax()` stricter
From: Patrick Steinhardt @ 2024-01-24 8:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Phillip Wood, Karthik Nayak, git
In-Reply-To: <xmqqwms0ndvu.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 2450 bytes --]
On Tue, Jan 23, 2024 at 09:44:21AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > My first hunch was to convert it so that it indeed always is a proper
> > ref. But thinking about it a bit more I'm less convinced that this is
> > sensible as it is deeply tied to the behaviour of git-bisect(1) and only
> > represents its internal state. I thus came to the conclusion that it is
> > more similar to the sequencer state that we have in ".git/rebase-merge"
> > and ".git/rebase-apply" than anything else.
>
> Fair enough.
>
> > So if we wanted to rectify this, I think the most sensible way to
> > address this would be to introduce a new ".git/bisect-state" directory
> > that contains all of git-bisect(1)'s state:
> >
> > - BISECT_TERMS -> bisect-state/terms
> > - BISECT_LOG -> bisect-state/log
> > - BISECT_START -> bisect-state/start
> > - BISECT_RUN -> bisect-state/run
> > - BISECT_FIRST_PARENT -> bisect-state/first-parent
> > - BISECT_ANCESTORS_OK -> bisect-state/ancestors-ok
> >
> > I think this would make for a much cleaner solution overall as things
> > are neatly contained. Cleaning up after a bisect would thus only require
> > a delete of ".git/bisect-state/" and we're done.
>
> And bisect-state/ needs to be marked as per-worktree hierarchy, I suppose.
Yes, "bisect-state/" would need to be stored in GIT_DIR, not COMMON_DIR.
> > Of course, this would be a backwards-incompatible change.
>
> As long as we ignore folks who switches versions of Git in the
> middle of their "git bisect" session, we should be OK.
>
> If we really cared the backward compatibility, the new version of
> Git that knows and uses this new layout could notice these old-style
> filenames and move them over to the new place under new names. From
> there, everything should work (including things like "git bisect log").
We also have consider that there may be alternate implementations of Git
that would only know to handle the old layout. Those tools would be
broken in case we did such a migration, but they would be broken anyway
if the bisect was started via Git and not via the tool.
Anyway, I'll add this to our growing backlog of issues that we might
want to investigate once the reftable backend has been upstreamed. Which
of course shouldn't preclude anybody else from picking up this topic in
case they are interested.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v3 6/6] t: mark tests regarding git-pack-refs(1) to be backend specific
From: Patrick Steinhardt @ 2024-01-24 8:45 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Eric Sunshine, Toon Claes, Christian Couder,
Justin Tobler
In-Reply-To: <cover.1706085756.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1577 bytes --]
Both t1409 and t3210 exercise parts of git-pack-refs(1). Given that we
must check the on-disk files to verify whether the backend has indeed
packed refs as expected those test suites are deeply tied to the actual
backend that is in use.
Mark the test suites to depend on the REFFILES backend.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1409-avoid-packing-refs.sh | 6 ++++++
t/t3210-pack-refs.sh | 6 ++++++
2 files changed, 12 insertions(+)
diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh
index f23c0152a8..7748973733 100755
--- a/t/t1409-avoid-packing-refs.sh
+++ b/t/t1409-avoid-packing-refs.sh
@@ -5,6 +5,12 @@ test_description='avoid rewriting packed-refs unnecessarily'
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
+if test_have_prereq !REFFILES
+then
+ skip_all='skipping files-backend specific pack-refs tests'
+ test_done
+fi
+
# Add an identifying mark to the packed-refs file header line. This
# shouldn't upset readers, and it should be omitted if the file is
# ever rewritten.
diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index 7f4e98db7d..c0f1f9cfb7 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -15,6 +15,12 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
+if test_have_prereq !REFFILES
+then
+ skip_all='skipping files-backend specific pack-refs tests'
+ test_done
+fi
+
test_expect_success 'enable reflogs' '
git config core.logallrefupdates true
'
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 5/6] t5526: break test submodule differently
From: Patrick Steinhardt @ 2024-01-24 8:45 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Eric Sunshine, Toon Claes, Christian Couder,
Justin Tobler
In-Reply-To: <cover.1706085756.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1873 bytes --]
In 10f5c52656 (submodule: avoid auto-discovery in
prepare_submodule_repo_env(), 2016-09-01) we fixed a bug when doing a
recursive fetch with submodule in the case where the submodule is broken
due to whatever reason. The test to exercise that the fix works breaks
the submodule by deleting its `HEAD` reference, which will cause us to
not detect the directory as a Git repository.
While this is perfectly fine in theory, this way of breaking the repo
becomes problematic with the current efforts to introduce another refdb
backend into Git. The new reftable backend has a stub HEAD file that
always contains "ref: refs/heads/.invalid" so that tools continue to be
able to detect such a repository. But as the reftable backend will never
delete this file even when asked to delete `HEAD` the current way to
delete the `HEAD` reference will stop working.
Adapt the code to instead delete the objects database. Going back with
this new way to cause breakage confirms that it triggers the infinite
recursion just the same, and there are no equivalent ongoing efforts to
replace the object database with an alternate backend.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t5526-fetch-submodules.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 7ab220fa31..5e566205ba 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -771,7 +771,7 @@ test_expect_success 'fetching submodule into a broken repository' '
git -C dst fetch --recurse-submodules &&
# Break the receiving submodule
- test-tool -C dst/sub ref-store main delete-refs REF_NO_DEREF msg HEAD &&
+ rm -r dst/sub/.git/objects &&
# NOTE: without the fix the following tests will recurse forever!
# They should terminate with an error.
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 4/6] t1419: mark test suite as files-backend specific
From: Patrick Steinhardt @ 2024-01-24 8:45 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Eric Sunshine, Toon Claes, Christian Couder,
Justin Tobler
In-Reply-To: <cover.1706085756.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2360 bytes --]
With 59c35fac54 (refs/packed-backend.c: implement jump lists to avoid
excluded pattern(s), 2023-07-10) we have implemented logic to handle
excluded refs more efficiently in the "packed" ref backend. This logic
allows us to skip emitting refs completely which we know to not be of
any interest to the caller, which can avoid quite some allocations and
object lookups.
This was wired up via a new `exclude_patterns` parameter passed to the
backend's ref iterator. The backend only needs to handle them on a best
effort basis though, and in fact we only handle it for the "packed-refs"
file, but not for loose references. Consequently, all callers must still
filter emitted refs with those exclude patterns.
The result is that handling exclude patterns is completely optional in
the ref backend, and any future backends may or may not implement it.
Let's thus mark the test for t1419 to depend on the REFFILES prereq.
An alternative would be to introduce a new prereq that tells us whether
the backend under test supports exclude patterns or not. But this does
feel a bit overblown:
- It would either map to the REFFILES prereq, in which case it feels
overengineered because the prereq is only ever relevant to t1419.
- Otherwise, it could auto-detect whether the backend supports exclude
patterns. But this could lead to silent failures in case the support
for this feature breaks at any point in time.
It should thus be good enough to just use the REFFILES prereq for now.
If future backends ever grow support for exclude patterns we can easily
add their respective prereq as another condition for this test suite to
execute.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1419-exclude-refs.sh | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/t/t1419-exclude-refs.sh b/t/t1419-exclude-refs.sh
index 5d8c86b657..1359574419 100755
--- a/t/t1419-exclude-refs.sh
+++ b/t/t1419-exclude-refs.sh
@@ -8,6 +8,12 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
+if test_have_prereq !REFFILES
+then
+ skip_all='skipping `git for-each-ref --exclude` tests; need files backend'
+ test_done
+fi
+
for_each_ref__exclude () {
GIT_TRACE2_PERF=1 test-tool ref-store main \
for-each-ref--exclude "$@" >actual.raw
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 3/6] t1302: make tests more robust with new extensions
From: Patrick Steinhardt @ 2024-01-24 8:45 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Eric Sunshine, Toon Claes, Christian Couder,
Justin Tobler
In-Reply-To: <cover.1706085756.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2977 bytes --]
In t1302 we exercise logic around "core.repositoryFormatVersion" and
extensions. These tests are not particularly robust against extensions
like the newly introduced "refStorage" extension as we tend to clobber
the repository's config file. We thus overwrite any extensions that were
set, which may render the repository inaccessible in case it has to be
accessed with a non-default ref storage.
Refactor the tests to be more robust:
- Check the DEFAULT_REPO_FORMAT prereq to determine the expected
repository format version. This helps to ensure that we only need to
update the prereq in a central place when new extensions are added.
Furthermore, this allows us to stop seeding the now-unneeded object
ID cache that was only used to figure out the repository version.
- Use a separate repository to rewrite ".git/config" to test
combinations of the repository format version and extensions. This
ensures that we don't break the main test repository. While we could
rewrite these tests to not overwrite preexisting extensions, it
feels cleaner like this so that we can test extensions standalone
without interference from the environment.
- Do not rewrite ".git/config" when exercising the "preciousObjects"
extension.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1302-repo-version.sh | 23 +++++++++++++++--------
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh
index 179474fa65..42caa0d297 100755
--- a/t/t1302-repo-version.sh
+++ b/t/t1302-repo-version.sh
@@ -9,10 +9,6 @@ TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '
- test_oid_cache <<-\EOF &&
- version sha1:0
- version sha256:1
- EOF
cat >test.patch <<-\EOF &&
diff --git a/test.txt b/test.txt
new file mode 100644
@@ -28,7 +24,12 @@ test_expect_success 'setup' '
'
test_expect_success 'gitdir selection on normal repos' '
- test_oid version >expect &&
+ if test_have_prereq DEFAULT_REPO_FORMAT
+ then
+ echo 0
+ else
+ echo 1
+ fi >expect &&
git config core.repositoryformatversion >actual &&
git -C test config core.repositoryformatversion >actual2 &&
test_cmp expect actual &&
@@ -79,8 +80,13 @@ mkconfig () {
while read outcome version extensions; do
test_expect_success "$outcome version=$version $extensions" "
- mkconfig $version $extensions >.git/config &&
- check_${outcome}
+ test_when_finished 'rm -rf extensions' &&
+ git init extensions &&
+ (
+ cd extensions &&
+ mkconfig $version $extensions >.git/config &&
+ check_${outcome}
+ )
"
done <<\EOF
allow 0
@@ -94,7 +100,8 @@ allow 1 noop-v1
EOF
test_expect_success 'precious-objects allowed' '
- mkconfig 1 preciousObjects >.git/config &&
+ git config core.repositoryFormatVersion 1 &&
+ git config extensions.preciousObjects 1 &&
check_allow
'
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 2/6] t1301: mark test for `core.sharedRepository` as reffiles specific
From: Patrick Steinhardt @ 2024-01-24 8:45 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Eric Sunshine, Toon Claes, Christian Couder,
Justin Tobler
In-Reply-To: <cover.1706085756.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]
In t1301 we verify whether reflog files written by the "files" ref
backend correctly honor permissions when "core.sharedRepository" is set.
The test logic is thus specific to the reffiles backend and will not
work with any other backends.
Mark the test accordingly with the REFFILES prereq.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1301-shared-repo.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index e5a0d65caa..8e2c01e760 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -137,7 +137,7 @@ test_expect_success POSIXPERM 'info/refs respects umask in unshared repo' '
test_cmp expect actual
'
-test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '
+test_expect_success REFFILES,POSIXPERM 'git reflog expire honors core.sharedRepository' '
umask 077 &&
git config core.sharedRepository group &&
git reflog expire --all &&
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 1/6] t1300: make tests more robust with non-default ref backends
From: Patrick Steinhardt @ 2024-01-24 8:45 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Eric Sunshine, Toon Claes, Christian Couder,
Justin Tobler
In-Reply-To: <cover.1706085756.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 4893 bytes --]
The t1300 test suite exercises the git-config(1) tool. To do so, the
test overwrites ".git/config" to contain custom contents. While this is
easy enough to do, it may create problems when using a non-default
repository format because this causes us to overwrite the repository
format version as well as any potential extensions. With the upcoming
"reftable" ref backend the result is that Git would try to access refs
via the "files" backend even though the repository has been initialized
with the "reftable" backend, which will cause failures when trying to
access any refs.
Refactor tests which access the refdb to be more robust by using their
own separate repositories, which allows us to be more careful and not
discard required extensions.
Note that we also have to touch up how the CUSTOM_CONFIG_FILE gets
accessed. This environment variable contains the relative path to a
custom config file which we're setting up. But because we are now using
subrepositories, this relative path will not be found anymore because
our working directory changes. This issue is addressed by storing the
absolute path to the file in CUSTOM_CONFIG_FILE instead.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1300-config.sh | 78 ++++++++++++++++++++++++++++++-----------------
1 file changed, 50 insertions(+), 28 deletions(-)
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index f4e2752134..31c3878687 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1098,15 +1098,20 @@ test_expect_success SYMLINKS 'symlink to nonexistent configuration' '
test_must_fail git config --file=linktolinktonada --list
'
-test_expect_success 'check split_cmdline return' "
- git config alias.split-cmdline-fix 'echo \"' &&
- test_must_fail git split-cmdline-fix &&
- echo foo > foo &&
- git add foo &&
- git commit -m 'initial commit' &&
- git config branch.main.mergeoptions 'echo \"' &&
- test_must_fail git merge main
-"
+test_expect_success 'check split_cmdline return' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ git config alias.split-cmdline-fix "echo \"" &&
+ test_must_fail git split-cmdline-fix &&
+ echo foo >foo &&
+ git add foo &&
+ git commit -m "initial commit" &&
+ git config branch.main.mergeoptions "echo \"" &&
+ test_must_fail git merge main
+ )
+'
test_expect_success 'git -c "key=value" support' '
cat >expect <<-\EOF &&
@@ -1157,10 +1162,16 @@ test_expect_success 'git -c works with aliases of builtins' '
'
test_expect_success 'aliases can be CamelCased' '
- test_config alias.CamelCased "rev-parse HEAD" &&
- git CamelCased >out &&
- git rev-parse HEAD >expect &&
- test_cmp expect out
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit A &&
+ git config alias.CamelCased "rev-parse HEAD" &&
+ git CamelCased >out &&
+ git rev-parse HEAD >expect &&
+ test_cmp expect out
+ )
'
test_expect_success 'git -c does not split values on equals' '
@@ -2009,11 +2020,11 @@ test_expect_success '--show-origin getting a single key' '
'
test_expect_success 'set up custom config file' '
- CUSTOM_CONFIG_FILE="custom.conf" &&
- cat >"$CUSTOM_CONFIG_FILE" <<-\EOF
+ cat >"custom.conf" <<-\EOF &&
[user]
custom = true
EOF
+ CUSTOM_CONFIG_FILE="$(test-tool path-utils real_path custom.conf)"
'
test_expect_success !MINGW 'set up custom config file with special name characters' '
@@ -2052,22 +2063,33 @@ test_expect_success '--show-origin stdin with file include' '
'
test_expect_success '--show-origin blob' '
- blob=$(git hash-object -w "$CUSTOM_CONFIG_FILE") &&
- cat >expect <<-EOF &&
- blob:$blob user.custom=true
- EOF
- git config --blob=$blob --show-origin --list >output &&
- test_cmp expect output
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ blob=$(git hash-object -w "$CUSTOM_CONFIG_FILE") &&
+ cat >expect <<-EOF &&
+ blob:$blob user.custom=true
+ EOF
+ git config --blob=$blob --show-origin --list >output &&
+ test_cmp expect output
+ )
'
test_expect_success '--show-origin blob ref' '
- cat >expect <<-\EOF &&
- blob:main:custom.conf user.custom=true
- EOF
- git add "$CUSTOM_CONFIG_FILE" &&
- git commit -m "new config file" &&
- git config --blob=main:"$CUSTOM_CONFIG_FILE" --show-origin --list >output &&
- test_cmp expect output
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ cat >expect <<-\EOF &&
+ blob:main:custom.conf user.custom=true
+ EOF
+ cp "$CUSTOM_CONFIG_FILE" custom.conf &&
+ git add custom.conf &&
+ git commit -m "new config file" &&
+ git config --blob=main:custom.conf --show-origin --list >output &&
+ test_cmp expect output
+ )
'
test_expect_success '--show-origin with --default' '
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 0/6] t: mark "files"-backend specific tests
From: Patrick Steinhardt @ 2024-01-24 8:45 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Eric Sunshine, Toon Claes, Christian Couder,
Justin Tobler
In-Reply-To: <cover.1704802213.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 7632 bytes --]
Hi,
this is the third version of my patch series that addresses tests whihc
are specific to the "files" backend. Changes compared to v2:
- Reworded some commit messages to hopefully explain better what is
going on.
- Refactored a test "while at it" to not use double quotes for the
test body.
- Removed the now-unneeded OID cache for one of our tests.
Thanks all for your comments!
Patrick
Patrick Steinhardt (6):
t1300: make tests more robust with non-default ref backends
t1301: mark test for `core.sharedRepository` as reffiles specific
t1302: make tests more robust with new extensions
t1419: mark test suite as files-backend specific
t5526: break test submodule differently
t: mark tests regarding git-pack-refs(1) to be backend specific
t/t1300-config.sh | 78 ++++++++++++++++++++++-------------
t/t1301-shared-repo.sh | 2 +-
t/t1302-repo-version.sh | 23 +++++++----
t/t1409-avoid-packing-refs.sh | 6 +++
t/t1419-exclude-refs.sh | 6 +++
t/t3210-pack-refs.sh | 6 +++
t/t5526-fetch-submodules.sh | 2 +-
7 files changed, 85 insertions(+), 38 deletions(-)
Range-diff against v2:
1: 0552123fa3 ! 1: a57e57a7c3 t1300: make tests more robust with non-default ref backends
@@ Metadata
## Commit message ##
t1300: make tests more robust with non-default ref backends
- The t1300 test suite exercises the git-config(1) tool. To do so we
- overwrite ".git/config" to contain custom contents. While this is easy
- enough to do, it may create problems when using a non-default repository
- format because we also overwrite the repository format version as well
- as any potential extensions. With the upcoming "reftable" ref backend
- the result is that we may try to access refs via the "files" backend
- even though the repository has been initialized with the "reftable"
- backend.
+ The t1300 test suite exercises the git-config(1) tool. To do so, the
+ test overwrites ".git/config" to contain custom contents. While this is
+ easy enough to do, it may create problems when using a non-default
+ repository format because this causes us to overwrite the repository
+ format version as well as any potential extensions. With the upcoming
+ "reftable" ref backend the result is that Git would try to access refs
+ via the "files" backend even though the repository has been initialized
+ with the "reftable" backend, which will cause failures when trying to
+ access any refs.
Refactor tests which access the refdb to be more robust by using their
own separate repositories, which allows us to be more careful and not
discard required extensions.
+ Note that we also have to touch up how the CUSTOM_CONFIG_FILE gets
+ accessed. This environment variable contains the relative path to a
+ custom config file which we're setting up. But because we are now using
+ subrepositories, this relative path will not be found anymore because
+ our working directory changes. This issue is addressed by storing the
+ absolute path to the file in CUSTOM_CONFIG_FILE instead.
+
Signed-off-by: Patrick Steinhardt <ps@pks.im>
## t/t1300-config.sh ##
@@ t/t1300-config.sh: test_expect_success SYMLINKS 'symlink to nonexistent configuration' '
+ test_must_fail git config --file=linktolinktonada --list
'
- test_expect_success 'check split_cmdline return' "
+-test_expect_success 'check split_cmdline return' "
- git config alias.split-cmdline-fix 'echo \"' &&
- test_must_fail git split-cmdline-fix &&
- echo foo > foo &&
@@ t/t1300-config.sh: test_expect_success SYMLINKS 'symlink to nonexistent configur
- git commit -m 'initial commit' &&
- git config branch.main.mergeoptions 'echo \"' &&
- test_must_fail git merge main
-+ test_when_finished 'rm -rf repo' &&
+-"
++test_expect_success 'check split_cmdline return' '
++ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
-+ git config alias.split-cmdline-fix 'echo \"' &&
++ git config alias.split-cmdline-fix "echo \"" &&
+ test_must_fail git split-cmdline-fix &&
+ echo foo >foo &&
+ git add foo &&
-+ git commit -m 'initial commit' &&
-+ git config branch.main.mergeoptions 'echo \"' &&
++ git commit -m "initial commit" &&
++ git config branch.main.mergeoptions "echo \"" &&
+ test_must_fail git merge main
+ )
- "
++'
test_expect_success 'git -c "key=value" support' '
+ cat >expect <<-\EOF &&
@@ t/t1300-config.sh: test_expect_success 'git -c works with aliases of builtins' '
'
2: 384250fec2 = 2: fd6dd92c23 t1301: mark test for `core.sharedRepository` as reffiles specific
3: 1284b70fab ! 3: ec90320ff1 t1302: make tests more robust with new extensions
@@ Commit message
In t1302 we exercise logic around "core.repositoryFormatVersion" and
extensions. These tests are not particularly robust against extensions
- like the newly introduced "refStorage" extension.
+ like the newly introduced "refStorage" extension as we tend to clobber
+ the repository's config file. We thus overwrite any extensions that were
+ set, which may render the repository inaccessible in case it has to be
+ accessed with a non-default ref storage.
Refactor the tests to be more robust:
- Check the DEFAULT_REPO_FORMAT prereq to determine the expected
repository format version. This helps to ensure that we only need to
update the prereq in a central place when new extensions are added.
+ Furthermore, this allows us to stop seeding the now-unneeded object
+ ID cache that was only used to figure out the repository version.
- Use a separate repository to rewrite ".git/config" to test
combinations of the repository format version and extensions. This
- ensures that we don't break the main test repository.
+ ensures that we don't break the main test repository. While we could
+ rewrite these tests to not overwrite preexisting extensions, it
+ feels cleaner like this so that we can test extensions standalone
+ without interference from the environment.
- Do not rewrite ".git/config" when exercising the "preciousObjects"
extension.
@@ Commit message
Signed-off-by: Patrick Steinhardt <ps@pks.im>
## t/t1302-repo-version.sh ##
+@@ t/t1302-repo-version.sh: TEST_PASSES_SANITIZE_LEAK=true
+ . ./test-lib.sh
+
+ test_expect_success 'setup' '
+- test_oid_cache <<-\EOF &&
+- version sha1:0
+- version sha256:1
+- EOF
+ cat >test.patch <<-\EOF &&
+ diff --git a/test.txt b/test.txt
+ new file mode 100644
@@ t/t1302-repo-version.sh: test_expect_success 'setup' '
'
4: c6062b612c = 4: d0d70c3f18 t1419: mark test suite as files-backend specific
5: ae08afc459 = 5: 066c297189 t5526: break test submodule differently
6: df648be535 = 6: 7b8921817b t: mark tests regarding git-pack-refs(1) to be backend specific
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] reftable: honor core.fsync
From: Patrick Steinhardt @ 2024-01-24 8:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: John Cai via GitGitGadget, git, John Cai
In-Reply-To: <xmqqsf2nlnxv.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 1237 bytes --]
On Tue, Jan 23, 2024 at 01:50:04PM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > A comment and a half.
> >
> > * Can't the new "how to flush" go to the write-option structure?
> > If you represent "no flush" as a NULL pointer in the flush member,
> > most of the changes to the _test files can go, no?
>
> Nah, that was a stupid comment. These are used to populate the
> members of the reftable_writer instance being created, and it does
> make sense to have flush_func immediately next to writer_func.
Agreed (not on the "stupid" part, on having it next to `writer_func`).
> The part about using NULL as the value to say "do not use any flusher"
> still stands, though. You do not have to expose noop_flush into the
> global namespace that way.
One benefit of explicitly using the `noop_flush()` function is that we
make sure that all callsites that should provide a proper flushing
function indeed do. A `noop_flush` in production code may raise some
eyebrows, whereas a `NULL` value could easily be overlooked.
Whether that is a good enough reason for the additional churn might be a
different question. I don't think it's particularly bad though.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: what should "git clean -n -f [-d] [-x] <pattern>" do?
From: Sergey Organov @ 2024-01-24 8:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Elijah Newren, git
In-Reply-To: <xmqqsf2nnbkj.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Sergey Organov <sorganov@gmail.com> writes:
>
>> Then how does one figure what "git clean -f -f" will do without actually
>> doing it?
>
> I think whoever came up with the bright idea of forcing twice
> somehow does a totally different thing from forcing once should be
> shot, twice ;-) It does not mesh well with the idea behind the
> clean.requireForce setting to make you explicitly choose either '-f'
> or '-n' to express your intent.
I agree, yet I see it as another deficiency, in addition to that of -n,
and I used it as an example to emphasize the deficiency of -n.
> I wonder how feasible is it to deprecate that misfeature introduced
> with a0f4afbe (clean: require double -f options to nuke nested git
> repository and work tree, 2009-06-30) and migrate its users (which
> is marked as "This is rarely what the user wants") to a new option,
> say, --nested-repo-too so that the "dry-run" version of the
> invocations become
>
> git clean -n
> git clean -n --nested-repo-too
>
> and you can substitute "-n" with "-f" to actually perform it?
Whereas obsoleting second -f in favor of new --nested-repo might be a
good idea indeed, I believe it's still a mistake for "dry run" to
somehow interfere with -f, sorry.
Thanks,
-- Sergey Organov
^ 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