* [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
* [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 2/4] refs: extract out `loose_fill_ref_dir_regular_file()`
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>
Extract out the code for adding a single file to the loose ref dir as
`loose_fill_ref_dir_regular_file()` from `loose_fill_ref_dir()` in
`refs/files-backend.c`.
This allows us to use this function independently in the following
commits where we add code to also add pseudorefs to the ref dir.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
refs/files-backend.c | 62 +++++++++++++++++++++++---------------------
1 file changed, 33 insertions(+), 29 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index b288fc97db..22495a4807 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -229,6 +229,38 @@ static void add_per_worktree_entries_to_dir(struct ref_dir *dir, const char *dir
}
}
+static void loose_fill_ref_dir_regular_file(struct files_ref_store *refs,
+ const char *refname,
+ struct ref_dir *dir)
+{
+ struct object_id oid;
+ int flag;
+
+ if (!refs_resolve_ref_unsafe(&refs->base, refname, RESOLVE_REF_READING,
+ &oid, &flag)) {
+ oidclr(&oid);
+ flag |= REF_ISBROKEN;
+ } else if (is_null_oid(&oid)) {
+ /*
+ * It is so astronomically unlikely
+ * that null_oid is the OID of an
+ * actual object that we consider its
+ * appearance in a loose reference
+ * file to be repo corruption
+ * (probably due to a software bug).
+ */
+ flag |= REF_ISBROKEN;
+ }
+
+ if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+ if (!refname_is_safe(refname))
+ die("loose refname is dangerous: %s", refname);
+ oidclr(&oid);
+ flag |= REF_BAD_NAME | REF_ISBROKEN;
+ }
+ add_entry_to_dir(dir, create_ref_entry(refname, &oid, flag));
+}
+
/*
* Read the loose references from the namespace dirname into dir
* (without recursing). dirname must end with '/'. dir must be the
@@ -257,8 +289,6 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,
strbuf_add(&refname, dirname, dirnamelen);
while ((de = readdir(d)) != NULL) {
- struct object_id oid;
- int flag;
unsigned char dtype;
if (de->d_name[0] == '.')
@@ -274,33 +304,7 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,
create_dir_entry(dir->cache, refname.buf,
refname.len));
} else if (dtype == DT_REG) {
- if (!refs_resolve_ref_unsafe(&refs->base,
- refname.buf,
- RESOLVE_REF_READING,
- &oid, &flag)) {
- oidclr(&oid);
- flag |= REF_ISBROKEN;
- } else if (is_null_oid(&oid)) {
- /*
- * It is so astronomically unlikely
- * that null_oid is the OID of an
- * actual object that we consider its
- * appearance in a loose reference
- * file to be repo corruption
- * (probably due to a software bug).
- */
- flag |= REF_ISBROKEN;
- }
-
- if (check_refname_format(refname.buf,
- REFNAME_ALLOW_ONELEVEL)) {
- if (!refname_is_safe(refname.buf))
- die("loose refname is dangerous: %s", refname.buf);
- oidclr(&oid);
- flag |= REF_BAD_NAME | REF_ISBROKEN;
- }
- add_entry_to_dir(dir,
- create_ref_entry(refname.buf, &oid, flag));
+ loose_fill_ref_dir_regular_file(refs, refname.buf, dir);
}
strbuf_setlen(&refname, dirnamelen);
}
--
2.43.GIT
^ permalink raw reply related
* [PATCH v2 3/4] refs: introduce `refs_for_each_all_refs()`
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 a new ref iteration flag `DO_FOR_EACH_INCLUDE_ALL_REFS`, which
will be used to iterate over all refs. In the files backend this is
limited to regular refs, pseudorefs and HEAD. For other backends like
the reftable this is the universal set of all refs stored in the
backend.
Refs which fall outside the `refs/` and aren't either pseudorefs or HEAD
are more of a grey area. This is because we don't block the users from
creating such refs but they are not officially supported. In the files
backend, we can isolate such files from other files.
Introduce `refs_for_each_all_refs()` which calls `do_for_each_ref()`
with this newly introduced flag.
In `refs/files-backend.c`, introduce a new function
`add_pseudoref_and_head_entries()` to add pseudorefs and HEAD to the
`ref_dir`. We then finally call `add_pseudoref_and_head_entries()`
whenever the `DO_FOR_EACH_INCLUDE_ALL_REFS` flag is set. Any new ref
backend will also have to implement similar changes on its end.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
refs.c | 7 +++++
refs.h | 6 ++++
refs/files-backend.c | 65 ++++++++++++++++++++++++++++++++++++++++----
refs/refs-internal.h | 7 +++++
4 files changed, 80 insertions(+), 5 deletions(-)
diff --git a/refs.c b/refs.c
index 4b6bfc66fb..b5e63f133a 100644
--- a/refs.c
+++ b/refs.c
@@ -1755,6 +1755,13 @@ int for_each_rawref(each_ref_fn fn, void *cb_data)
return refs_for_each_rawref(get_main_ref_store(the_repository), fn, cb_data);
}
+int refs_for_each_all_refs(struct ref_store *refs, each_ref_fn fn,
+ void *cb_data)
+{
+ return do_for_each_ref(refs, "", NULL, fn, 0,
+ DO_FOR_EACH_INCLUDE_ALL_REFS, cb_data);
+}
+
static int qsort_strcmp(const void *va, const void *vb)
{
const char *a = *(const char **)va;
diff --git a/refs.h b/refs.h
index 46b8085d63..77ecb820f9 100644
--- a/refs.h
+++ b/refs.h
@@ -396,6 +396,12 @@ int for_each_namespaced_ref(const char **exclude_patterns,
int refs_for_each_rawref(struct ref_store *refs, each_ref_fn fn, void *cb_data);
int for_each_rawref(each_ref_fn fn, void *cb_data);
+/*
+ * Iterates over all ref types, regular, pseudorefs and HEAD.
+ */
+int refs_for_each_all_refs(struct ref_store *refs, each_ref_fn fn,
+ void *cb_data);
+
/*
* Normalizes partial refs to their fully qualified form.
* Will prepend <prefix> to the <pattern> if it doesn't start with 'refs/'.
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 22495a4807..104f2e1ac7 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -315,9 +315,59 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,
add_per_worktree_entries_to_dir(dir, dirname);
}
-static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)
+/*
+ * Add pseudorefs to the ref dir by parsing the directory for any files
+ * which follow the pseudoref syntax.
+ */
+static void add_pseudoref_and_head_entries(struct ref_store *ref_store,
+ struct ref_dir *dir,
+ const char *dirname)
+{
+ struct files_ref_store *refs =
+ files_downcast(ref_store, REF_STORE_READ, "fill_ref_dir");
+ struct strbuf path = STRBUF_INIT, refname = STRBUF_INIT;
+ struct dirent *de;
+ size_t dirnamelen;
+ DIR *d;
+
+ files_ref_path(refs, &path, dirname);
+
+ d = opendir(path.buf);
+ if (!d) {
+ strbuf_release(&path);
+ return;
+ }
+
+ strbuf_addstr(&refname, dirname);
+ dirnamelen = refname.len;
+
+ while ((de = readdir(d)) != NULL) {
+ unsigned char dtype;
+
+ if (de->d_name[0] == '.')
+ continue;
+ if (ends_with(de->d_name, ".lock"))
+ continue;
+ strbuf_addstr(&refname, de->d_name);
+
+ dtype = get_dtype(de, &path, 1);
+ if (dtype == DT_REG && (is_pseudoref(ref_store, de->d_name) ||
+ is_headref(ref_store, de->d_name)))
+ loose_fill_ref_dir_regular_file(refs, refname.buf, dir);
+
+ strbuf_setlen(&refname, dirnamelen);
+ }
+ strbuf_release(&refname);
+ strbuf_release(&path);
+ closedir(d);
+}
+
+static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs,
+ unsigned int flags)
{
if (!refs->loose) {
+ struct ref_dir *dir;
+
/*
* Mark the top-level directory complete because we
* are about to read the only subdirectory that can
@@ -328,12 +378,17 @@ static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)
/* We're going to fill the top level ourselves: */
refs->loose->root->flag &= ~REF_INCOMPLETE;
+ dir = get_ref_dir(refs->loose->root);
+
+ if (flags & DO_FOR_EACH_INCLUDE_ALL_REFS)
+ add_pseudoref_and_head_entries(dir->cache->ref_store, dir,
+ refs->loose->root->name);
+
/*
* Add an incomplete entry for "refs/" (to be filled
* lazily):
*/
- add_entry_to_dir(get_ref_dir(refs->loose->root),
- create_dir_entry(refs->loose, "refs/", 5));
+ add_entry_to_dir(dir, create_dir_entry(refs->loose, "refs/", 5));
}
return refs->loose;
}
@@ -861,7 +916,7 @@ static struct ref_iterator *files_ref_iterator_begin(
* disk, and re-reads it if not.
*/
- loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs),
+ loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs, flags),
prefix, ref_store->repo, 1);
/*
@@ -1222,7 +1277,7 @@ static int files_pack_refs(struct ref_store *ref_store,
packed_refs_lock(refs->packed_ref_store, LOCK_DIE_ON_ERROR, &err);
- iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL,
+ iter = cache_ref_iterator_begin(get_loose_ref_cache(refs, 0), NULL,
the_repository, 0);
while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
/*
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 8e9f04cc67..1cf7506435 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -260,6 +260,13 @@ enum do_for_each_ref_flags {
* INCLUDE_BROKEN, since they are otherwise not included at all.
*/
DO_FOR_EACH_OMIT_DANGLING_SYMREFS = (1 << 2),
+
+ /*
+ * Include all refs in the $GIT_DIR in contrast to generally only listing
+ * references having the "refs/" prefix. In the files-backend this is
+ * limited to regular refs, pseudorefs and HEAD.
+ */
+ DO_FOR_EACH_INCLUDE_ALL_REFS = (1 << 3),
};
/*
--
2.43.GIT
^ permalink raw reply related
* [PATCH v2 4/4] for-each-ref: avoid filtering on empty pattern
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>
When the user uses an empty string pattern (""), we don't match any refs
in git-for-each-ref(1). This is because in git-for-each-ref(1), we use
path based matching and an empty string doesn't match any path.
In this commit we change this behavior by making empty string pattern
match all references. This is done by introducing a new flag
`FILTER_REFS_NO_FILTER` in `ref-filter.c`, which uses the newly
introduced `refs_for_each_all_refs()` function to iterate over all the
refs in the repository.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Documentation/git-for-each-ref.txt | 3 ++-
builtin/for-each-ref.c | 21 +++++++++++++++++-
ref-filter.c | 13 ++++++++++--
ref-filter.h | 4 +++-
t/t6302-for-each-ref-filter.sh | 34 ++++++++++++++++++++++++++++++
5 files changed, 70 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index be9543f684..b1cb482bf5 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -32,7 +32,8 @@ OPTIONS
If one or more patterns are given, only refs are shown that
match against at least one pattern, either using fnmatch(3) or
literally, in the latter case matching completely or from the
- beginning up to a slash.
+ beginning up to a slash. If an empty string is provided all refs
+ are printed, including HEAD and pseudorefs.
--stdin::
If `--stdin` is supplied, then the list of patterns is read from
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 3885a9c28e..5aa879e8be 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -25,6 +25,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
struct ref_format format = REF_FORMAT_INIT;
int from_stdin = 0;
struct strvec vec = STRVEC_INIT;
+ unsigned int flags = FILTER_REFS_ALL;
struct option opts[] = {
OPT_BIT('s', "shell", &format.quote_style,
@@ -93,11 +94,29 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
/* vec.v is NULL-terminated, just like 'argv'. */
filter.name_patterns = vec.v;
} else {
+ size_t i;
+
filter.name_patterns = argv;
+
+ /*
+ * Search for any empty string pattern, if it exists then we
+ * print all refs without any filtering.
+ */
+ i = 0;
+ while (argv[i]) {
+ if (!argv[i][0]) {
+ flags = FILTER_REFS_NO_FILTER;
+ /* doing this removes any pattern from being matched */
+ filter.name_patterns[0] = NULL;
+ break;
+ }
+
+ i++;
+ }
}
filter.match_as_path = 1;
- filter_and_format_refs(&filter, FILTER_REFS_ALL, sorting, &format);
+ filter_and_format_refs(&filter, flags, sorting, &format);
ref_filter_clear(&filter);
ref_sorting_release(sorting);
diff --git a/ref-filter.c b/ref-filter.c
index 35b989e1df..6dac133b87 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2622,6 +2622,11 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter,
each_ref_fn cb,
void *cb_data)
{
+ if (filter->kind & FILTER_REFS_NO_FILTER) {
+ return refs_for_each_all_refs(
+ get_main_ref_store(the_repository), cb, cb_data);
+ }
+
if (!filter->match_as_path) {
/*
* in this case, the patterns are applied after
@@ -2775,8 +2780,12 @@ static struct ref_array_item *apply_ref_filter(const char *refname, const struct
/* Obtain the current ref kind from filter_ref_kind() and ignore unwanted refs. */
kind = filter_ref_kind(filter, refname);
- if (!(kind & filter->kind))
+ if (filter->kind & FILTER_REFS_NO_FILTER) {
+ if (kind == FILTER_REFS_DETACHED_HEAD)
+ kind = FILTER_REFS_OTHERS;
+ } else if (!(kind & filter->kind)) {
return NULL;
+ }
if (!filter_pattern_match(filter, refname))
return NULL;
@@ -3041,7 +3050,7 @@ static int do_filter_refs(struct ref_filter *filter, unsigned int type, each_ref
ret = for_each_fullref_in("refs/remotes/", fn, cb_data);
else if (filter->kind == FILTER_REFS_TAGS)
ret = for_each_fullref_in("refs/tags/", fn, cb_data);
- else if (filter->kind & FILTER_REFS_ALL)
+ else if (filter->kind & FILTER_REFS_ALL || filter->kind & FILTER_REFS_NO_FILTER)
ret = for_each_fullref_in_pattern(filter, fn, cb_data);
if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))
head_ref(fn, cb_data);
diff --git a/ref-filter.h b/ref-filter.h
index 07cd6f6da3..1eab325ce0 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -22,7 +22,9 @@
#define FILTER_REFS_ALL (FILTER_REFS_TAGS | FILTER_REFS_BRANCHES | \
FILTER_REFS_REMOTES | FILTER_REFS_OTHERS)
#define FILTER_REFS_DETACHED_HEAD 0x0020
-#define FILTER_REFS_KIND_MASK (FILTER_REFS_ALL | FILTER_REFS_DETACHED_HEAD)
+#define FILTER_REFS_NO_FILTER 0x0040
+#define FILTER_REFS_KIND_MASK (FILTER_REFS_ALL | FILTER_REFS_DETACHED_HEAD | \
+ FILTER_REFS_NO_FILTER)
struct atom_value;
struct ref_sorting;
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 82f3d1ea0f..3922326cab 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -31,6 +31,40 @@ test_expect_success 'setup some history and refs' '
git update-ref refs/odd/spot main
'
+cat >expect <<-\EOF
+ HEAD
+ ORIG_HEAD
+ refs/heads/main
+ refs/heads/side
+ refs/odd/spot
+ refs/tags/annotated-tag
+ refs/tags/doubly-annotated-tag
+ refs/tags/doubly-signed-tag
+ refs/tags/four
+ refs/tags/one
+ refs/tags/signed-tag
+ refs/tags/three
+ refs/tags/two
+EOF
+
+test_expect_success 'empty pattern prints pseudorefs' '
+ git update-ref ORIG_HEAD main &&
+ git for-each-ref --format="%(refname)" "" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'empty pattern with other patterns' '
+ git update-ref ORIG_HEAD main &&
+ git for-each-ref --format="%(refname)" "" "refs/" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'empty pattern towards the end' '
+ git update-ref ORIG_HEAD main &&
+ git for-each-ref --format="%(refname)" "refs/" "" >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'filtering with --points-at' '
cat >expect <<-\EOF &&
refs/heads/main
--
2.43.GIT
^ permalink raw reply related
* Re: Fwd: Unexpected behavior of ls-files command when using --others --exclude-from, and a .gitignore file which resides in a subdirectory
From: Junio C Hamano @ 2024-01-24 17:06 UTC (permalink / raw)
To: Elijah Newren
Cc: Jeff King, Raúl Núñez de Arenas Coronado, git
In-Reply-To: <CABPp-BHcdn3+mbPJk8LZvMjPWZ4s-xdUyevrMbgbT4yQpJ_umA@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
> It can also imply that a feature, design, or practice will be removed
> or discontinued entirely in the future.
> ```
>
> Since "can also imply" != "does also imply", and based on the commit
> message of 491a7575f (particularly the part that quotes dcf0c16ef1,
> both of which were prior work that informed the under discussion
> e750951e74), I thought the use of deprecated was perfectly applicable
> here.
I think we've seen confusions before, more than just once in the
past, caused by the verb "deprecate" in our docs. People, after
reading "X is deprecated; use Y instead", always assumed that X will
eventually be removed, even in contexts where we did not mean it.
------ >8 ----------- >8 ----------- >8 ----------- >8 ------
Subject: ls-files: avoid the verb "deprecate" for individual options
When e750951e74f updated the documentation to give greater
visibility to the --exclude-standard option, it marked the
`--exclude-per-directory` option as "deprecated". While it
technically is correct that being deprecated does not necessarily
mean it is planned to be removed later, it seems to cause confusion
to readers, especially when we merely mean
The option Y can be used to achieve the same thing as the option
X, so to those of you who aren't familiar with either X or Y, we
would recommend to use Y.
This is especially true for `--exclude-standard` vs the combination
of more granular `--exclude-from` and `--exclude-per-directory`
options. It is true that one common combination of the granular
options can be obtained by just giving the former, but that does not
necessarily mean a more granular control is not necessary.
State why we recommend looking at `--exclude-standard` when we do so
in the description of `--exclude-per-directory`, instead of saying
that the option is deprecated. Also, spell out the recipe to emulate
what `--exclude-standard` does, so that the users can give it minute
tweaks (like "not reading the global exclusion file from core.excludes").
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/git-ls-files.txt | 24 +++++++++++++++++++-----
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git c/Documentation/git-ls-files.txt w/Documentation/git-ls-files.txt
index f65a8cd91d..93a0111cfb 100644
--- c/Documentation/git-ls-files.txt
+++ w/Documentation/git-ls-files.txt
@@ -119,8 +119,10 @@ OPTIONS
--exclude-per-directory=<file>::
Read additional exclude patterns that apply only to the
- directory and its subdirectories in <file>. Deprecated; use
- --exclude-standard instead.
+ directory and its subdirectories in <file>. If you are
+ trying to emulate the way how Porcelain commands work,
+ using the `--exclude-standard` instead is easier and more
+ thorough.
--exclude-standard::
Add the standard Git exclusions: .git/info/exclude, .gitignore
@@ -298,9 +300,8 @@ traversing the directory tree and finding files to show when the
flags --others or --ignored are specified. linkgit:gitignore[5]
specifies the format of exclude patterns.
-Generally, you should just use --exclude-standard, but for historical
-reasons the exclude patterns can be specified from the following
-places, in order:
+These exclude patterns can be specified from the following places,
+in order:
1. The command-line flag --exclude=<pattern> specifies a
single pattern. Patterns are ordered in the same order
@@ -322,6 +323,19 @@ top of the directory tree. A pattern read from a file specified
by --exclude-per-directory is relative to the directory that the
pattern file appears in.
+Generally, you should be able to use `--exclude-standard` when you
+want the exclude rules applied the same way as what Porcelain
+commands do. To emulate what `--exclude-standard` specifies, you
+can give `--exclude-per-directory=.gitignore`, and then specify:
+
+ 1. The file specified by the `core.excludesfile` configuration
+ variable, if exists, or the `$XDG_CONFIG_HOME/git/ignore` file.
+
+ 2. The `$GIT_DIR/info/exclude` file
+
+via the `--exclude-from=` option.
+
+
SEE ALSO
--------
linkgit:git-read-tree[1], linkgit:gitignore[5]
^ permalink raw reply related
* Re: what should "git clean -n -f [-d] [-x] <pattern>" do?
From: Junio C Hamano @ 2024-01-24 17:21 UTC (permalink / raw)
To: Sergey Organov; +Cc: Elijah Newren, git
In-Reply-To: <87plxr3zsr.fsf@osv.gnss.ru>
Sergey Organov <sorganov@gmail.com> writes:
> 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.
No need to be sorry ;-)
I actually think the true culprit of making this an odd-man-out is
that the use of "-f" in "git clean", especially with its use of the
configuration variable clean.requireForce that defaults to true, is
utterly non-standard.
The usual pattern of defining what "-f" does is that the "git foo"
command without any options does its common thing but refuses to
perform undesirable operations (e.g. "git add ." adds everything
but refrains from adding ignored paths). And "git foo -f" is a way
to also perform what it commonly skips.
In contrast, with clean.requireForce that defaults to true, "git
clean" does not do anything useful by default. Without such a
safety, "git clean" would be a way to clean expendable paths, and
"git clean -f" might be to also clean precious paths. But it does
not work that way. It always requires "-f" to do anything. Worse
yet, it is not even "by default it acts as if -n is given and -f is
a way to countermand that implicit -n". It is "you must give me
either -f (i.e. please do work) or -n (i.e. please show what you
would do) before I do anything".
$ git clean
fatal: clean.requireForce defaults to true and neither -i, -n, nor -f given; refusing to clean
Given that, it is hard to argue that it would be a natural end-user
expectation that the command does something useful (i.e. show what
would be done) when it is given "-f" and "-n" at the same time.
What makes this a rather nonsense UI is the fact that "-f" does not
work the way we would expect for this command.
^ permalink raw reply
* Re: [PATCH] reftable: honor core.fsync
From: Junio C Hamano @ 2024-01-24 17:22 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: John Cai via GitGitGadget, git, John Cai
In-Reply-To: <ZbDNVouHgr-J2ptC@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
>> 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.
Very true. Another benefit is that at runtime we do not need any
conditional deep inside the logic that calls the .flush method of
the writer object.
^ permalink raw reply
* Re: [PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert
From: Junio C Hamano @ 2024-01-24 17:34 UTC (permalink / raw)
To: Elijah Newren; +Cc: Michael Lohmann, git, phillip.wood123, Johannes Sixt
In-Reply-To: <CABPp-BE4zqRX=wd5EBj96hzCS8V73QpdN-2pCpv7qMdkpkX93w@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
> I had to go look up previous versions to see the discussion of why
> this was useful for things other than merge. I agree with Phillip
> from https://lore.kernel.org/git/648774b5-5208-42d3-95c7-e0cba4d6a159@gmail.com/,
> that the commit message _needs_ to explain this, likely using some of
> Junio's explanation.
Please note that I am not very happy with that "explanation" myself.
The only thing I can still agree to in that message myself is that
it is sensible for "log --merge" to go down all the way to the root
of the histories leading to MERGE_HEAD and HEAD in the "merging two
unrelated histories" scenario. Treating CHERRY_PICK_HEAD and others
the same way, to me, almost sounds as if we are saying "all the
commits behind the commits involved in the conflicted operation are
worth looking at", which is not a very useful or productive thing.
> Also, what about cases where users do a cherry-pick in the middle of a
> rebase, so that both REBASE_HEAD and CHERRY_PICK_HEAD exist? What
> then?
;-)
^ permalink raw reply
* Re: [PATCH 0/2] [GSoC][RFC/Patch] FIX: use utf8_strnwidth for line_prefix in diff.c
From: Junio C Hamano @ 2024-01-24 17:42 UTC (permalink / raw)
To: Md Isfarul Haque via GitGitGadget; +Cc: git, Md Isfarul Haque
In-Reply-To: <pull.1653.git.git.1706105064.gitgitgadget@gmail.com>
"Md Isfarul Haque via GitGitGadget" <gitgitgadget@gmail.com> writes:
> This patch adresses diff.c:2721 and proposes the fix using a new function.
Yay. My favorite long-time pet-peeve topic.
>
> Md Isfarul Haque (2):
> FIX: use utf8_strnwidth for line_prefix in diff.c
> FIX memory leak in one branch
Our convention does not use "FIX" and other prefix on the subject.
Please check Documentation/SubmittingPatches.
Thanks.
^ permalink raw reply
* Re: [PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert
From: Johannes Sixt @ 2024-01-24 17:19 UTC (permalink / raw)
To: Elijah Newren; +Cc: Michael Lohmann, gitster, git, phillip.wood123
In-Reply-To: <CABPp-BE4zqRX=wd5EBj96hzCS8V73QpdN-2pCpv7qMdkpkX93w@mail.gmail.com>
Am 24.01.24 um 08:06 schrieb Elijah Newren:
> On Wed, Jan 17, 2024 at 12:14 AM Michael Lohmann
>> +static const char *lookup_other_head(struct object_id *oid)
>> +{
>> + int i;
>> + static const char *const other_head[] = {
>> + "MERGE_HEAD", "REBASE_HEAD", "CHERRY_PICK_HEAD", "REVERT_HEAD"
>> + };
>> +
>> + for (i = 0; i < ARRAY_SIZE(other_head); i++)
>> + if (!read_ref_full(other_head[i],
>> + RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
>> + oid, NULL)) {
>> + if (is_null_oid(oid))
>> + die("%s is a symbolic ref???", other_head[i]);
>> + return other_head[i];
>> + }
>> +
>> + die("--merge without MERGE_HEAD, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_HEAD?");
>> +}
...
> Also, what about cases where users do a cherry-pick in the middle of a
> rebase, so that both REBASE_HEAD and CHERRY_PICK_HEAD exist? What
> then?
Good point! IMO, REBASE_HEAD should have lower precedence than all the
other *_HEADs. It would mean to reorder the entries:
static const char *const other_head[] = {
"MERGE_HEAD", "CHERRY_PICK_HEAD", "REVERT_HEAD", "REBASE_HEAD"
};
(and perhaps adjust the error message, too).
-- Hannes
^ 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: Jeff King @ 2024-01-24 18:57 UTC (permalink / raw)
To: Junio C Hamano
Cc: Elijah Newren, Raúl Núñez de Arenas Coronado, git
In-Reply-To: <xmqqjznyhd90.fsf@gitster.g>
On Wed, Jan 24, 2024 at 09:06:51AM -0800, Junio C Hamano wrote:
> ------ >8 ----------- >8 ----------- >8 ----------- >8 ------
> Subject: ls-files: avoid the verb "deprecate" for individual options
>
> When e750951e74f updated the documentation to give greater
> visibility to the --exclude-standard option, it marked the
> `--exclude-per-directory` option as "deprecated". While it
> technically is correct that being deprecated does not necessarily
> mean it is planned to be removed later, it seems to cause confusion
> to readers, especially when we merely mean
>
> The option Y can be used to achieve the same thing as the option
> X, so to those of you who aren't familiar with either X or Y, we
> would recommend to use Y.
>
> This is especially true for `--exclude-standard` vs the combination
> of more granular `--exclude-from` and `--exclude-per-directory`
> options. It is true that one common combination of the granular
> options can be obtained by just giving the former, but that does not
> necessarily mean a more granular control is not necessary.
>
> State why we recommend looking at `--exclude-standard` when we do so
> in the description of `--exclude-per-directory`, instead of saying
> that the option is deprecated. Also, spell out the recipe to emulate
> what `--exclude-standard` does, so that the users can give it minute
> tweaks (like "not reading the global exclusion file from core.excludes").
This perfectly sums up the situation from my viewpoint. And I think the
changes you suggest not only remove the confusion over the term, they
lay out the possible options for the user much better.
The patch looks good to me, modulo one minor comment:
> diff --git c/Documentation/git-ls-files.txt w/Documentation/git-ls-files.txt
> index f65a8cd91d..93a0111cfb 100644
> --- c/Documentation/git-ls-files.txt
> +++ w/Documentation/git-ls-files.txt
> @@ -119,8 +119,10 @@ OPTIONS
>
> --exclude-per-directory=<file>::
> Read additional exclude patterns that apply only to the
> - directory and its subdirectories in <file>. Deprecated; use
> - --exclude-standard instead.
> + directory and its subdirectories in <file>. If you are
> + trying to emulate the way how Porcelain commands work,
> + using the `--exclude-standard` instead is easier and more
> + thorough.
As a native English speaker, the phrasing here is awkward to me. I'd
probably say "...emulate the way that Porcelain commands work". Or
alternatively, "emulate how Porcelain commands work".
-Peff
^ permalink raw reply
* Re: [PATCH v2 1/4] refs: introduce `is_pseudoref()` and `is_headref()`
From: Junio C Hamano @ 2024-01-24 19:09 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, ps, phillip.wood123
In-Reply-To: <20240124152726.124873-2-karthik.188@gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> 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.
Well described.
> 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"
Let's end an array's initializer with a trailing comma, to help
future patches to add entries to this array without unnecessary
patch noise.
> + };
> + 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;
> +}
The above uses refs_ref_exists() because we want these to
successfully resolve for reading.
> +int is_headref(struct ref_store *refs, const char *refname)
> +{
> + if (!strcmp(refname, "HEAD"))
> + return refs_ref_exists(refs, refname);
Given that "git for-each-ref refs/remotes" does not show
"refs/remotes/origin/HEAD" in the output when we do not have the
remote-tracking branch that symref points at, we probably do want
to omit "HEAD" from the output when the HEAD symref points at an
unborn branch. If refs_ref_exists() says "no, it does not exist"
in such a case, we are perfectly fine with this code.
We do not have to worry about the unborn state for pseudorefs
because they would never be symbolic. But that in turn makes me
suspect that the check done with refs_ref_exists() in the
is_pseudoref() helper is a bit too lenient by allowing it to be a
symbolic ref. Shouldn't we be using a check based on
read_ref_full(), like we did in another topic recently [*]?
[Reference]
* https://lore.kernel.org/git/xmqqzfxa9usx.fsf@gitster.g/
^ permalink raw reply
* Re: [PATCH] transport-helper: re-examine object dir after fetching
From: Junio C Hamano @ 2024-01-24 19:31 UTC (permalink / raw)
To: Jeff King; +Cc: Josh Steadmon, git, bcmills
In-Reply-To: <20240124010056.GA2603087@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Thanks, I was able to reproduce it. Besides using the v0 protocol, two
> key elements are that the server is http and the use of --depth.
>
> The patch below explains what's going on and should fix it. I prepared
> the patch on top of 'master', but it can also be applied directly on
> 61c7711cfe or on v2.21.0, modulo some minor textual conflicts in the
> test script (modern t5551 has some more tests, and no longer calls
> stop_httpd manually).
Thanks.
The usual "one liner fix that requires two-page explanation"
that only you can produce ;-).
> ...
> So everything works, but mostly due to luck. Whereas in a fetch
> with --depth, we skip step 2 entirely, and thus the out-of-date
> cache is still in place for step 3, giving us the wrong answer.
>
> So the test works with a small "--depth 1" fetch, which makes sure that
> we don't store the pack from the other side, and that we don't trigger
> the accidental cache invalidation. And of course it forces the use of
> v0 along with using the http protocol.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> t/t5551-http-fetch-smart.sh | 18 ++++++++++++++++++
> transport-helper.c | 3 +++
> 2 files changed, 21 insertions(+)
>
> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index e069737b80..a623a1058c 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -733,4 +733,22 @@ test_expect_success 'no empty path components' '
> ! grep "//" log
> '
>
> +test_expect_success 'tag following always works over v0 http' '
> + upstream=$HTTPD_DOCUMENT_ROOT_PATH/tags &&
> + git init "$upstream" &&
> + (
> + cd "$upstream" &&
> + git commit --allow-empty -m base &&
> + git tag not-annotated &&
> + git tag -m foo annotated
> + ) &&
> + git init tags &&
> + git -C tags -c protocol.version=0 \
> + fetch --depth 1 $HTTPD_URL/smart/tags \
> + refs/tags/annotated:refs/tags/annotated &&
> + git -C "$upstream" for-each-ref refs/tags >expect &&
> + git -C tags for-each-ref >actual &&
> + test_cmp expect actual
> +'
> +
> test_done
> diff --git a/transport-helper.c b/transport-helper.c
> index e34a8f47cf..07e42e239a 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -17,6 +17,7 @@
> #include "refspec.h"
> #include "transport-internal.h"
> #include "protocol.h"
> +#include "packfile.h"
>
> static int debug;
>
> @@ -432,6 +433,8 @@ static int fetch_with_fetch(struct transport *transport,
> warning(_("%s unexpectedly said: '%s'"), data->name, buf.buf);
> }
> strbuf_release(&buf);
> +
> + reprepare_packed_git(the_repository);
> return 0;
> }
^ permalink raw reply
* Re: [PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert
From: Junio C Hamano @ 2024-01-24 19:46 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Elijah Newren, Michael Lohmann, git, phillip.wood123
In-Reply-To: <dfb582cf-b1e4-414d-bfe1-0f93d910ec54@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Good point! IMO, REBASE_HEAD should have lower precedence than all the
> other *_HEADs. It would mean to reorder the entries:
>
> static const char *const other_head[] = {
> "MERGE_HEAD", "CHERRY_PICK_HEAD", "REVERT_HEAD", "REBASE_HEAD"
> };
>
> (and perhaps adjust the error message, too).
And probably give a warning saying that we noticed you are rebasing
and cherry-picking and we chose to show the --merge based on the
relationship between cherry-pick-head and head, ignoring your rebase
status, or something.
^ permalink raw reply
* Re: [PATCH 1/2] FIX: use utf8_strnwidth for line_prefix in diff.c
From: Christian Couder @ 2024-01-24 20:01 UTC (permalink / raw)
To: Md Isfarul Haque via GitGitGadget; +Cc: git, Md Isfarul Haque
In-Reply-To: <ac9338533c9096c090d1463c1b29505bde019731.1706105064.git.gitgitgadget@gmail.com>
On Wed, Jan 24, 2024 at 3:04 PM Md Isfarul Haque via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Md Isfarul Haque <isfarul.876@gmail.com>
>
> This patch adresses diff.c:2721 and proposes the fix using a new function.
Please give more details here about what is currently at diff.c:2721
and what the patch is fixing, as lines at diff.c:2721 could move to a
different location if some changes are made to diff.c before your
patches get merged or after they get merged.
Also if the patch is addressing an issue in a code comment I would
expect the corresponding code comment to be removed by the patch.
About the subject, maybe "diff: use utf8_strnwidth() for line_prefix"
is already better.
> 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)
This function seems to be used only in diff.c, so it could be static.
^ permalink raw reply
* Re: [PATCH 1/2] FIX: use utf8_strnwidth for line_prefix in diff.c
From: Junio C Hamano @ 2024-01-24 20:08 UTC (permalink / raw)
To: Md Isfarul Haque via GitGitGadget; +Cc: git, Md Isfarul Haque
In-Reply-To: <ac9338533c9096c090d1463c1b29505bde019731.1706105064.git.gitgitgadget@gmail.com>
"Md Isfarul Haque via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Md Isfarul Haque <isfarul.876@gmail.com>
>
> This patch adresses diff.c:2721 and proposes the fix using a new function.
Once the issue has fully been addressed, it is expected that the
NEEDSWORK comment there would be removed, making this proposed log
message useless. Make it a habit to write a log message that is
self-contained enough to help readers (including yourself in the
future when you have forgotten the details of what you did in this
commit).
> +const struct strbuf *diff_line_prefix_buf(struct diff_options *opt)
> +{
Given that there is only one caller of this function in the same
file, I do not see a reason why this needs to be extern and exported
in diff.h (actually I do not see a need for this helper at all).
When dealing with a string buffer, it is much more common in this
codebase for the caller to prepare a strbuf (often on its stack) and
pass a pointer to it to helper functions. I.e.
static void prepare_diff_line_prefix_buf(struct strbuf *buf,
struct diff_options *opt)
{
... stuff whatever you need into the string buffer ...
strbuf_add(buf, ...);
}
/* in show_stats() */
struct strbuf line_prefix = STRBUF_INIT;
...
prepare_diff_line_prefix_buf(&line_prefix, options);
... use line_prefix and ...
... release the resource before returning ...
strbuf_release(&line_prefix);
is more common and less prone to resource leak over time.
> @@ -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);
I do not see the need for any of the diff_line_prefix_buf() related
changes, only to do this change. You have a const char *line_prefix
at this point, and utf8_strnwidth() wants to know its length, so
what you need is to massage the parameter to match what it wants.
Perhaps even something simple and dumb like
utf8_strnwidth(line_prefix, strlen(line_prefix), 1);
might be sufficient to replace strlen(line_prefix) in the original?
This patch hopefully will change the behaviour of the command. A
patch needs to also protect the change from future breakages by
adding a test or two to demonstrate the desired behaviour. Such a
test should pass with the code change in the patch, and should fail
when the code change in the patch gets reverted.
Thanks.
^ permalink raw reply
* [PATCH v4] merge-ll: expose revision names to custom drivers
From: Antonin Delpeuch via GitGitGadget @ 2024-01-24 20:09 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Phillip Wood, Antonin Delpeuch, Antonin Delpeuch
In-Reply-To: <pull.1648.v3.git.git.1705615794307.gitgitgadget@gmail.com>
From: Antonin Delpeuch <antonin@delpeuch.eu>
Custom merge drivers need access to the names of the revisions they
are working on, so that the merge conflict markers they introduce
can refer to those revisions. The placeholders '%S', '%X' and '%Y'
are introduced to this end.
Signed-off-by: Antonin Delpeuch <antonin@delpeuch.eu>
---
merge-ll: expose revision names to custom drivers
Changes since v3:
* simplify the documentation as suggested by Phillip, keeping
single-letter placeholders as suggested by Junio
* add fallback to empty string when the collision marker names are
NULL, as suggested by Phillip
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1648%2Fwetneb%2Fmerge_driver_pathnames-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1648/wetneb/merge_driver_pathnames-v4
Pull-Request: https://github.com/git/git/pull/1648
Range-diff vs v3:
1: aebd26711fe ! 1: 47aca4fc8f6 merge-ll: expose revision names to custom drivers
@@ Documentation/gitattributes.txt: When left unspecified, the driver itself is use
The merge driver can learn the pathname in which the merged result
-will be stored via placeholder `%P`.
-
-+will be stored via placeholder `%P`. Additionally, the names of the
-+common ancestor revision (`%S`), of the current revision (`%X`) and
-+of the other branch (`%Y`) can also be supplied. Those are short
-+revision names, optionally joined with the paths of the file in each
-+revision. Those paths are only present if they differ and are separated
-+from the revision by a colon.
++will be stored via placeholder `%P`. The conflict labels to be used
++for the common ancestor, local head and other head can be passed by
++using '%S', '%X' and '%Y` respectively.
`conflict-marker-size`
^^^^^^^^^^^^^^^^^^^^^^
@@ merge-ll.c: static enum ll_merge_result ll_ext_merge(const struct ll_merge_drive
else if (skip_prefix(format, "P", &format))
sq_quote_buf(&cmd, path);
+ else if (skip_prefix(format, "S", &format))
-+ sq_quote_buf(&cmd, orig_name);
++ sq_quote_buf(&cmd, orig_name ? orig_name : "");
+ else if (skip_prefix(format, "X", &format))
-+ sq_quote_buf(&cmd, name1);
++ sq_quote_buf(&cmd, name1 ? name1 : "");
+ else if (skip_prefix(format, "Y", &format))
-+ sq_quote_buf(&cmd, name2);
++ sq_quote_buf(&cmd, name2 ? name2 : "");
else
strbuf_addch(&cmd, '%');
}
Documentation/gitattributes.txt | 9 +++++----
merge-ll.c | 17 ++++++++++++++---
t/t6406-merge-attr.sh | 16 +++++++++++-----
3 files changed, 30 insertions(+), 12 deletions(-)
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 201bdf5edbd..4338d023d9a 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -1137,11 +1137,11 @@ The `merge.*.name` variable gives the driver a human-readable
name.
The `merge.*.driver` variable's value is used to construct a
-command to run to merge ancestor's version (`%O`), current
+command to run to common ancestor's version (`%O`), current
version (`%A`) and the other branches' version (`%B`). These
three tokens are replaced with the names of temporary files that
hold the contents of these versions when the command line is
-built. Additionally, %L will be replaced with the conflict marker
+built. Additionally, `%L` will be replaced with the conflict marker
size (see below).
The merge driver is expected to leave the result of the merge in
@@ -1159,8 +1159,9 @@ When left unspecified, the driver itself is used for both
internal merge and the final merge.
The merge driver can learn the pathname in which the merged result
-will be stored via placeholder `%P`.
-
+will be stored via placeholder `%P`. The conflict labels to be used
+for the common ancestor, local head and other head can be passed by
+using '%S', '%X' and '%Y` respectively.
`conflict-marker-size`
^^^^^^^^^^^^^^^^^^^^^^
diff --git a/merge-ll.c b/merge-ll.c
index 1df58ebaac0..5ffb045efb9 100644
--- a/merge-ll.c
+++ b/merge-ll.c
@@ -185,9 +185,9 @@ static void create_temp(mmfile_t *src, char *path, size_t len)
static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
mmbuffer_t *result,
const char *path,
- mmfile_t *orig, const char *orig_name UNUSED,
- mmfile_t *src1, const char *name1 UNUSED,
- mmfile_t *src2, const char *name2 UNUSED,
+ mmfile_t *orig, const char *orig_name,
+ mmfile_t *src1, const char *name1,
+ mmfile_t *src2, const char *name2,
const struct ll_merge_options *opts,
int marker_size)
{
@@ -222,6 +222,12 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
strbuf_addf(&cmd, "%d", marker_size);
else if (skip_prefix(format, "P", &format))
sq_quote_buf(&cmd, path);
+ else if (skip_prefix(format, "S", &format))
+ sq_quote_buf(&cmd, orig_name ? orig_name : "");
+ else if (skip_prefix(format, "X", &format))
+ sq_quote_buf(&cmd, name1 ? name1 : "");
+ else if (skip_prefix(format, "Y", &format))
+ sq_quote_buf(&cmd, name2 ? name2 : "");
else
strbuf_addch(&cmd, '%');
}
@@ -315,7 +321,12 @@ static int read_merge_config(const char *var, const char *value,
* %B - temporary file name for the other branches' version.
* %L - conflict marker length
* %P - the original path (safely quoted for the shell)
+ * %S - the revision for the merge base
+ * %X - the revision for our version
+ * %Y - the revision for their version
*
+ * If the file is not named indentically in all versions, then each
+ * revision is joined with the corresponding path, separated by a colon.
* The external merge driver should write the results in the
* file named by %A, and signal that it has done with zero exit
* status.
diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh
index 72f8c1722ff..156a1efacfe 100755
--- a/t/t6406-merge-attr.sh
+++ b/t/t6406-merge-attr.sh
@@ -42,11 +42,15 @@ test_expect_success setup '
#!/bin/sh
orig="$1" ours="$2" theirs="$3" exit="$4" path=$5
+ orig_name="$6" our_name="$7" their_name="$8"
(
echo "orig is $orig"
echo "ours is $ours"
echo "theirs is $theirs"
echo "path is $path"
+ echo "orig_name is $orig_name"
+ echo "our_name is $our_name"
+ echo "their_name is $their_name"
echo "=== orig ==="
cat "$orig"
echo "=== ours ==="
@@ -121,7 +125,7 @@ test_expect_success 'custom merge backend' '
git reset --hard anchor &&
git config --replace-all \
- merge.custom.driver "./custom-merge %O %A %B 0 %P" &&
+ merge.custom.driver "./custom-merge %O %A %B 0 %P %S %X %Y" &&
git config --replace-all \
merge.custom.name "custom merge driver for testing" &&
@@ -132,7 +136,8 @@ test_expect_success 'custom merge backend' '
o=$(git unpack-file main^:text) &&
a=$(git unpack-file side^:text) &&
b=$(git unpack-file main:text) &&
- sh -c "./custom-merge $o $a $b 0 text" &&
+ base_revid=$(git rev-parse --short main^) &&
+ sh -c "./custom-merge $o $a $b 0 text $base_revid HEAD main" &&
sed -e 1,3d $a >check-2 &&
cmp check-1 check-2 &&
rm -f $o $a $b
@@ -142,7 +147,7 @@ test_expect_success 'custom merge backend' '
git reset --hard anchor &&
git config --replace-all \
- merge.custom.driver "./custom-merge %O %A %B 1 %P" &&
+ merge.custom.driver "./custom-merge %O %A %B 1 %P %S %X %Y" &&
git config --replace-all \
merge.custom.name "custom merge driver for testing" &&
@@ -159,7 +164,8 @@ test_expect_success 'custom merge backend' '
o=$(git unpack-file main^:text) &&
a=$(git unpack-file anchor:text) &&
b=$(git unpack-file main:text) &&
- sh -c "./custom-merge $o $a $b 0 text" &&
+ base_revid=$(git rev-parse --short main^) &&
+ sh -c "./custom-merge $o $a $b 0 text $base_revid HEAD main" &&
sed -e 1,3d $a >check-2 &&
cmp check-1 check-2 &&
sed -e 1,3d -e 4q $a >check-3 &&
@@ -173,7 +179,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'
git reset --hard anchor &&
git config --replace-all \
- merge.custom.driver "./custom-merge %O %A %B 0 %P" &&
+ merge.custom.driver "./custom-merge %O %A %B 0 %P %S %X %Y" &&
git config --replace-all \
merge.custom.name "custom merge driver for testing" &&
base-commit: 186b115d3062e6230ee296d1ddaa0c4b72a464b5
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 2/2] FIX memory leak in one branch
From: Junio C Hamano @ 2024-01-24 20:11 UTC (permalink / raw)
To: Md Isfarul Haque via GitGitGadget; +Cc: git, Md Isfarul Haque
In-Reply-To: <a7c81f7e114fc8854436e2ca1fccb4c968653317.1706105064.git.gitgitgadget@gmail.com>
"Md Isfarul Haque via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 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(+)
We do not need to see that you are just as human as other developers
and are prone to make mistakes that you need to fix it in a
follow-up. In other words, please do not introduce a bug in [1/2]
only to be fixed in [2/2]. By squashing these two patches into one,
you can pretend as if you are a perfect developer and never leaked
memory ;-).
> 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;
But as I said, I do not see a need for this helper function in the
first place, so...
^ permalink raw reply
* Re: [PATCH v4 1/2] git-compat-util: add strtol_i_updated()
From: Junio C Hamano @ 2024-01-24 20:20 UTC (permalink / raw)
To: Mohit Marathe via GitGitGadget; +Cc: git, Mohit Marathe, Mohit Marathe
In-Reply-To: <60ea85a701a05831b0adf1e3f9a7a97fd31ef43f.1706079304.git.gitgitgadget@gmail.com>
"Mohit Marathe via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Mohit Marathe <mohitmarathe23@gmail.com>
>
> This function is an updated version of strtol_i() function. It will
> give more control to handle parsing of the characters after the
> integer and better error handling while parsing numbers.
i2 was horrible but this is worse. What would you call an even
newer variant when you need to add one? strtol_i_updated_twice?
To readers who are reading the code in 6 months, it is totally
uninteresting that strtol_i() is an older function and the new thing
was invented later as its update. What they want to learn is how
these two are different, what additional things this new one lets
them do compared to the old one, namely: we can optionally learn
where the run of the digits has ended.
Perhaps call it "strtoi_with_tail" or something, unless others
suggest even better names?
Thanks.
^ permalink raw reply
* Re: [PATCH v4 2/2] patch-id: replace `atoi()` with `strtol_i_updated()`
From: Junio C Hamano @ 2024-01-24 21:02 UTC (permalink / raw)
To: Mohit Marathe via GitGitGadget; +Cc: git, Mohit Marathe, Mohit Marathe
In-Reply-To: <17f2dda4907ec03b0783160c53c4896fd76cb053.1706079304.git.gitgitgadget@gmail.com>
"Mohit Marathe via GitGitGadget" <gitgitgadget@gmail.com> writes:
> q = p + 4;
> n = strspn(q, digits);
> if (q[n] == ',') {
> q += n + 1;
So, we saw "@@ -" and skipped over these four bytes, skipped the
digits from there, and found a comma.
For "@@ -29,14 +30,18 @@", for example, our q is now "14 +30,18 @@"
as we have skipped over that comma after 29.
> - *p_before = atoi(q);
> + if (strtol_i_updated(q, 10, p_before, &endp) != 0)
> + return 0;
We parse out 14 and store it to *p_before. endp points at " +30..."
now.
> n = strspn(q, digits);
> + if (endp != q + n)
> + return 0;
Is this necessary? By asking strtol_i_updated() where the number ended,
we already know endp without skipping the digits in q with strspn().
Shouldn't these three lines become more like
n = endp - q;
instead?
After all, we are not trying to find a bug in strtol_i_updated(),
which would be the only reason how this "return 0" would trigger.
> } else {
> *p_before = 1;
> }
> @@ -48,8 +53,11 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)
> n = strspn(r, digits);
> if (r[n] == ',') {
> r += n + 1;
> - *p_after = atoi(r);
> + if (strtol_i_updated(r, 10, p_after, &endp) != 0)
> + return 0;
> n = strspn(r, digits);
> + if (endp != r + n)
> + return 0;
Likewise.
> } else {
> *p_after = 1;
> }
^ permalink raw reply
* [PATCH] ls-files: avoid the verb "deprecate" for individual options
From: Junio C Hamano @ 2024-01-24 21:10 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Jeff King,
Raúl Núñez de Arenas Coronado
When e750951e (ls-files: guide folks to --exclude-standard over
other --exclude* options, 2023-01-13) updated the documentation to
give greater visibility to the `--exclude-standard` option, it marked
the `--exclude-per-directory` option as "deprecated".
While it is technically correct that being deprecated does not
necessarily mean it is planned to be removed later, it seems to
cause confusion to readers, especially when we merely mean
The option Y can be used to achieve the same thing as the option
X much simpler. To those of you who aren't familiar with either
X or Y, we would recommend to use Y when appropriate.
This is especially true for `--exclude-standard` vs the combination
of more granular `--exclude-from` and `--exclude-per-directory`
options. It is true that one common combination of the granular
options can be obtained by just giving the former, but that does not
necessarily mean a more granular control is not necessary.
State the reason why we recommend readers `--exclude-standard` in
the description of `--exclude-per-directory`, instead of saying that
the option is deprecated. Also, spell out the recipe to emulate
what `--exclude-standard` does, so that the users can give it minute
tweaks (like "do the same as Git Porcelain, except I do not want to
read the global exclusion file from core.excludes").
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* With the grammofix by Peff squashed in and with minor tweaks to
the proposed log message.
Documentation/git-ls-files.txt | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index f65a8cd91d..9447f2d8f4 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -119,8 +119,10 @@ OPTIONS
--exclude-per-directory=<file>::
Read additional exclude patterns that apply only to the
- directory and its subdirectories in <file>. Deprecated; use
- --exclude-standard instead.
+ directory and its subdirectories in <file>. If you are
+ trying to emulate the way Porcelain commands work, using
+ the `--exclude-standard` instead is easier and more
+ thorough.
--exclude-standard::
Add the standard Git exclusions: .git/info/exclude, .gitignore
@@ -298,9 +300,8 @@ traversing the directory tree and finding files to show when the
flags --others or --ignored are specified. linkgit:gitignore[5]
specifies the format of exclude patterns.
-Generally, you should just use --exclude-standard, but for historical
-reasons the exclude patterns can be specified from the following
-places, in order:
+These exclude patterns can be specified from the following places,
+in order:
1. The command-line flag --exclude=<pattern> specifies a
single pattern. Patterns are ordered in the same order
@@ -322,6 +323,18 @@ top of the directory tree. A pattern read from a file specified
by --exclude-per-directory is relative to the directory that the
pattern file appears in.
+Generally, you should be able to use `--exclude-standard` when you
+want the exclude rules applied the same way as what Porcelain
+commands do. To emulate what `--exclude-standard` specifies, you
+can give `--exclude-per-directory=.gitignore`, and then specify:
+
+ 1. The file specified by the `core.excludesfile` configuration
+ variable, if exists, or the `$XDG_CONFIG_HOME/git/ignore` file.
+
+ 2. The `$GIT_DIR/info/exclude` file.
+
+via the `--exclude-from=` option.
+
SEE ALSO
--------
linkgit:git-read-tree[1], linkgit:gitignore[5]
--
2.43.0-386-ge02ecfcc53
^ permalink raw reply related
* Re: [PATCH v4] merge-ll: expose revision names to custom drivers
From: Junio C Hamano @ 2024-01-24 21:17 UTC (permalink / raw)
To: Antonin Delpeuch via GitGitGadget; +Cc: git, Phillip Wood, Antonin Delpeuch
In-Reply-To: <pull.1648.v4.git.git.1706126951879.gitgitgadget@gmail.com>
"Antonin Delpeuch via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Antonin Delpeuch <antonin@delpeuch.eu>
>
> Custom merge drivers need access to the names of the revisions they
> are working on, so that the merge conflict markers they introduce
> can refer to those revisions. The placeholders '%S', '%X' and '%Y'
> are introduced to this end.
>
> Signed-off-by: Antonin Delpeuch <antonin@delpeuch.eu>
> ---
This looks all good.
Let's declare a victory and merge it down to 'next' soonish.
Thanks for sticking to the topic.
^ permalink raw reply
* Re: [PATCH v2 00/12] Group reffiles tests
From: John Cai @ 2024-01-24 21:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Patrick Steinhardt, John Cai via GitGitGadget, git
In-Reply-To: <xmqqplxsrk8n.fsf@gitster.g>
Hi Junio,
On 22 Jan 2024, at 19:01, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
>> I've got two minor nits, but other than that this looks good to me. I've
>> also verified that all tests continue to pass with the current version
>> of the reftable backend.
>
> OK. I've squashed all the nits from you and Karthik into the copy
> in my tree. If there is nothing else, let's declare a victory and
> merge the topic down to 'next' soonish.
Thank you for doing these tedious corrections!
>
>> There's a minor merge conflict with db4192c364 (t: mark tests regarding
>> git-pack-refs(1) to be backend specific, 2024-01-10). This conflict
>> comes from the fact that both patch series add the REFFILES prereq to
>> t3210, semantically the changes are the same. So it doesn't quite matter
>> which of both versions we retain as they both do the same.
>
> Yup, that is what I've been resolving them.
>
> Thanks.
thanks
John
^ permalink raw reply
* Re: [PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert
From: Johannes Sixt @ 2024-01-24 22:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Elijah Newren, Michael Lohmann, git, phillip.wood123
In-Reply-To: <xmqq1qa6ecqr.fsf@gitster.g>
Am 24.01.24 um 20:46 schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
>
>> Good point! IMO, REBASE_HEAD should have lower precedence than all the
>> other *_HEADs. It would mean to reorder the entries:
>>
>> static const char *const other_head[] = {
>> "MERGE_HEAD", "CHERRY_PICK_HEAD", "REVERT_HEAD", "REBASE_HEAD"
>> };
>>
>> (and perhaps adjust the error message, too).
>
> And probably give a warning saying that we noticed you are rebasing
> and cherry-picking and we chose to show the --merge based on the
> relationship between cherry-pick-head and head, ignoring your rebase
> status, or something.
I don't think that's necessary. When rebase stopped with a merge
conflict, neither of the other commands can begin their work until the
conflicted state is removed. That should be a concious act, just like
when thereafter one of these other commands is used and leads to a
conflict. At least I would certainly not need a reminder.
-- Hannes
^ 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