* [PATCH 5/9] ref-filter.h: add functions for filter/format & format-only
From: Victoria Dye via GitGitGadget @ 2023-11-07 1:25 UTC (permalink / raw)
To: git; +Cc: Victoria Dye, Victoria Dye
In-Reply-To: <pull.1609.git.1699320361.gitgitgadget@gmail.com>
From: Victoria Dye <vdye@github.com>
Add two new public methods to 'ref-filter.h':
* 'print_formatted_ref_array()' which, given a format specification & array
of ref items, formats and prints the items to stdout.
* 'filter_and_format_refs()' which combines 'filter_refs()',
'ref_array_sort()', and 'print_formatted_ref_array()' into a single
function.
This consolidates much of the code used to filter and format refs in
'builtin/for-each-ref.c', 'builtin/tag.c', and 'builtin/branch.c', reducing
duplication and simplifying the future changes needed to optimize the filter
& format process.
Signed-off-by: Victoria Dye <vdye@github.com>
---
builtin/branch.c | 33 +++++++++++++++++----------------
builtin/for-each-ref.c | 27 +--------------------------
builtin/tag.c | 23 +----------------------
ref-filter.c | 35 +++++++++++++++++++++++++++++++++++
ref-filter.h | 14 ++++++++++++++
5 files changed, 68 insertions(+), 64 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 5a1ec1cd04f..2ed59f16f1c 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -437,8 +437,6 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
{
int i;
struct ref_array array;
- struct strbuf out = STRBUF_INIT;
- struct strbuf err = STRBUF_INIT;
int maxwidth = 0;
const char *remote_prefix = "";
char *to_free = NULL;
@@ -468,24 +466,27 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
filter_ahead_behind(the_repository, format, &array);
ref_array_sort(sorting, &array);
- for (i = 0; i < array.nr; i++) {
- strbuf_reset(&err);
- strbuf_reset(&out);
- if (format_ref_array_item(array.items[i], format, &out, &err))
- die("%s", err.buf);
- if (column_active(colopts)) {
- assert(!filter->verbose && "--column and --verbose are incompatible");
- /* format to a string_list to let print_columns() do its job */
+ if (column_active(colopts)) {
+ struct strbuf out = STRBUF_INIT, err = STRBUF_INIT;
+
+ assert(!filter->verbose && "--column and --verbose are incompatible");
+
+ for (i = 0; i < array.nr; i++) {
+ strbuf_reset(&err);
+ strbuf_reset(&out);
+ if (format_ref_array_item(array.items[i], format, &out, &err))
+ die("%s", err.buf);
+
+ /* format to a string_list to let print_columns() do its job */
string_list_append(output, out.buf);
- } else {
- fwrite(out.buf, 1, out.len, stdout);
- if (out.len || !format->array_opts.omit_empty)
- putchar('\n');
}
+
+ strbuf_release(&err);
+ strbuf_release(&out);
+ } else {
+ print_formatted_ref_array(&array, format);
}
- strbuf_release(&err);
- strbuf_release(&out);
ref_array_clear(&array);
free(to_free);
}
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 881c3ee055f..1c19cd5bd34 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -19,15 +19,11 @@ static char const * const for_each_ref_usage[] = {
int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
{
- int i, total;
struct ref_sorting *sorting;
struct string_list sorting_options = STRING_LIST_INIT_DUP;
int icase = 0;
- struct ref_array array;
struct ref_filter filter = REF_FILTER_INIT;
struct ref_format format = REF_FORMAT_INIT;
- struct strbuf output = STRBUF_INIT;
- struct strbuf err = STRBUF_INIT;
int from_stdin = 0;
struct strvec vec = STRVEC_INIT;
@@ -61,8 +57,6 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
OPT_END(),
};
- memset(&array, 0, sizeof(array));
-
format.format = "%(objectname) %(objecttype)\t%(refname)";
git_config(git_default_config, NULL);
@@ -104,27 +98,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
}
filter.match_as_path = 1;
- filter_refs(&array, &filter, FILTER_REFS_ALL);
- filter_ahead_behind(the_repository, &format, &array);
-
- ref_array_sort(sorting, &array);
-
- total = format.array_opts.max_count;
- if (!total || array.nr < total)
- total = array.nr;
- for (i = 0; i < total; i++) {
- strbuf_reset(&err);
- strbuf_reset(&output);
- if (format_ref_array_item(array.items[i], &format, &output, &err))
- die("%s", err.buf);
- fwrite(output.buf, 1, output.len, stdout);
- if (output.len || !format.array_opts.omit_empty)
- putchar('\n');
- }
+ filter_and_format_refs(&filter, FILTER_REFS_ALL, sorting, &format);
- strbuf_release(&err);
- strbuf_release(&output);
- ref_array_clear(&array);
ref_filter_clear(&filter);
ref_sorting_release(sorting);
strvec_clear(&vec);
diff --git a/builtin/tag.c b/builtin/tag.c
index 2d599245d48..2528d499dd8 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -48,13 +48,7 @@ static int config_sign_tag = -1; /* unspecified */
static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,
struct ref_format *format)
{
- struct ref_array array;
- struct strbuf output = STRBUF_INIT;
- struct strbuf err = STRBUF_INIT;
char *to_free = NULL;
- int i;
-
- memset(&array, 0, sizeof(array));
if (filter->lines == -1)
filter->lines = 0;
@@ -72,23 +66,8 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,
if (verify_ref_format(format))
die(_("unable to parse format string"));
filter->with_commit_tag_algo = 1;
- filter_refs(&array, filter, FILTER_REFS_TAGS);
- filter_ahead_behind(the_repository, format, &array);
- ref_array_sort(sorting, &array);
-
- for (i = 0; i < array.nr; i++) {
- strbuf_reset(&output);
- strbuf_reset(&err);
- if (format_ref_array_item(array.items[i], format, &output, &err))
- die("%s", err.buf);
- fwrite(output.buf, 1, output.len, stdout);
- if (output.len || !format->array_opts.omit_empty)
- putchar('\n');
- }
+ filter_and_format_refs(filter, FILTER_REFS_TAGS, sorting, format);
- strbuf_release(&err);
- strbuf_release(&output);
- ref_array_clear(&array);
free(to_free);
return 0;
diff --git a/ref-filter.c b/ref-filter.c
index 5129b6986c9..8992fbf45b1 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -3023,6 +3023,18 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
return ret;
}
+void filter_and_format_refs(struct ref_filter *filter, unsigned int type,
+ struct ref_sorting *sorting,
+ struct ref_format *format)
+{
+ struct ref_array array = { 0 };
+ filter_refs(&array, filter, type);
+ filter_ahead_behind(the_repository, format, &array);
+ ref_array_sort(sorting, &array);
+ print_formatted_ref_array(&array, format);
+ ref_array_clear(&array);
+}
+
static int compare_detached_head(struct ref_array_item *a, struct ref_array_item *b)
{
if (!(a->kind ^ b->kind))
@@ -3212,6 +3224,29 @@ int format_ref_array_item(struct ref_array_item *info,
return 0;
}
+void print_formatted_ref_array(struct ref_array *array, struct ref_format *format)
+{
+ int total;
+ struct strbuf output = STRBUF_INIT, err = STRBUF_INIT;
+
+ total = format->array_opts.max_count;
+ if (!total || array->nr < total)
+ total = array->nr;
+ for (int i = 0; i < total; i++) {
+ strbuf_reset(&err);
+ strbuf_reset(&output);
+ if (format_ref_array_item(array->items[i], format, &output, &err))
+ die("%s", err.buf);
+ if (output.len || !format->array_opts.omit_empty) {
+ fwrite(output.buf, 1, output.len, stdout);
+ putchar('\n');
+ }
+ }
+
+ strbuf_release(&err);
+ strbuf_release(&output);
+}
+
void pretty_print_ref(const char *name, const struct object_id *oid,
struct ref_format *format)
{
diff --git a/ref-filter.h b/ref-filter.h
index 0db3ff52889..0ce5af58ab3 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -137,6 +137,14 @@ struct ref_format {
* filtered refs in the ref_array structure.
*/
int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int type);
+/*
+ * Filter refs using the given ref_filter and type, sort the contents
+ * according to the given ref_sorting, format the filtered refs with the
+ * given ref_format, and print them to stdout.
+ */
+void filter_and_format_refs(struct ref_filter *filter, unsigned int type,
+ struct ref_sorting *sorting,
+ struct ref_format *format);
/* Clear all memory allocated to ref_array */
void ref_array_clear(struct ref_array *array);
/* Used to verify if the given format is correct and to parse out the used atoms */
@@ -161,6 +169,12 @@ char *get_head_description(void);
/* Set up translated strings in the output. */
void setup_ref_filter_porcelain_msg(void);
+/*
+ * Print up to maxcount ref_array elements to stdout using the given
+ * ref_format.
+ */
+void print_formatted_ref_array(struct ref_array *array, struct ref_format *format);
+
/*
* Print a single ref, outside of any ref-filter. Note that the
* name must be a fully qualified refname.
--
gitgitgadget
^ permalink raw reply related
* [PATCH 6/9] ref-filter.c: refactor to create common helper functions
From: Victoria Dye via GitGitGadget @ 2023-11-07 1:25 UTC (permalink / raw)
To: git; +Cc: Victoria Dye, Victoria Dye
In-Reply-To: <pull.1609.git.1699320361.gitgitgadget@gmail.com>
From: Victoria Dye <vdye@github.com>
Factor out parts of 'ref_array_push()', 'ref_filter_handler()', and
'filter_refs()' into new helper functions ('ref_array_append()',
'apply_ref_filter()', and 'do_filter_refs()' respectively), as well as
rename 'ref_filter_handler()' to 'filter_one()'. In this and later
patches, these helpers will be used by new ref-filter API functions. This
patch does not result in any user-facing behavior changes or changes to
callers outside of 'ref-filter.c'.
The changes are as follows:
* The logic to grow a 'struct ref_array' and append a given 'struct
ref_array_item *' to it is extracted from 'ref_array_push()' into
'ref_array_append()'.
* 'ref_filter_handler()' is renamed to 'filter_one()' to more clearly
distinguish it from other ref filtering callbacks that will be added in
later patches. The "*_one()" naming convention is common throughout the
codebase for iteration callbacks.
* The code to filter a given ref by refname & object ID then create a new
'struct ref_array_item' is moved out of 'filter_one()' and into
'apply_ref_filter()'. 'apply_ref_filter()' returns either NULL (if the ref
does not match the given filter) or a 'struct ref_array_item *' created
with 'new_ref_array_item()'; 'filter_one()' appends that item to
its ref array with 'ref_array_append()'.
* The filter pre-processing, contains cache creation, and ref iteration of
'filter_refs()' is extracted into 'do_filter_refs()'. 'do_filter_refs()'
takes its ref iterator function & callback data as an input from the
caller, setting it up to be used with additional filtering callbacks in
later patches.
Signed-off-by: Victoria Dye <vdye@github.com>
---
ref-filter.c | 115 ++++++++++++++++++++++++++++++---------------------
1 file changed, 69 insertions(+), 46 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 8992fbf45b1..ff00ab4b8d8 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2716,15 +2716,18 @@ static struct ref_array_item *new_ref_array_item(const char *refname,
return ref;
}
+static void ref_array_append(struct ref_array *array, struct ref_array_item *ref)
+{
+ ALLOC_GROW(array->items, array->nr + 1, array->alloc);
+ array->items[array->nr++] = ref;
+}
+
struct ref_array_item *ref_array_push(struct ref_array *array,
const char *refname,
const struct object_id *oid)
{
struct ref_array_item *ref = new_ref_array_item(refname, oid);
-
- ALLOC_GROW(array->items, array->nr + 1, array->alloc);
- array->items[array->nr++] = ref;
-
+ ref_array_append(array, ref);
return ref;
}
@@ -2761,46 +2764,36 @@ static int filter_ref_kind(struct ref_filter *filter, const char *refname)
return ref_kind_from_refname(refname);
}
-struct ref_filter_cbdata {
- struct ref_array *array;
- struct ref_filter *filter;
-};
-
-/*
- * A call-back given to for_each_ref(). Filter refs and keep them for
- * later object processing.
- */
-static int ref_filter_handler(const char *refname, const struct object_id *oid, int flag, void *cb_data)
+static struct ref_array_item *apply_ref_filter(const char *refname, const struct object_id *oid,
+ int flag, struct ref_filter *filter)
{
- struct ref_filter_cbdata *ref_cbdata = cb_data;
- struct ref_filter *filter = ref_cbdata->filter;
struct ref_array_item *ref;
struct commit *commit = NULL;
unsigned int kind;
if (flag & REF_BAD_NAME) {
warning(_("ignoring ref with broken name %s"), refname);
- return 0;
+ return NULL;
}
if (flag & REF_ISBROKEN) {
warning(_("ignoring broken ref %s"), refname);
- return 0;
+ return NULL;
}
/* Obtain the current ref kind from filter_ref_kind() and ignore unwanted refs. */
kind = filter_ref_kind(filter, refname);
if (!(kind & filter->kind))
- return 0;
+ return NULL;
if (!filter_pattern_match(filter, refname))
- return 0;
+ return NULL;
if (filter_exclude_match(filter, refname))
- return 0;
+ return NULL;
if (filter->points_at.nr && !match_points_at(&filter->points_at, oid, refname))
- return 0;
+ return NULL;
/*
* A merge filter is applied on refs pointing to commits. Hence
@@ -2811,15 +2804,15 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
filter->with_commit || filter->no_commit || filter->verbose) {
commit = lookup_commit_reference_gently(the_repository, oid, 1);
if (!commit)
- return 0;
+ return NULL;
/* We perform the filtering for the '--contains' option... */
if (filter->with_commit &&
!commit_contains(filter, commit, filter->with_commit, &filter->internal.contains_cache))
- return 0;
+ return NULL;
/* ...or for the `--no-contains' option */
if (filter->no_commit &&
commit_contains(filter, commit, filter->no_commit, &filter->internal.no_contains_cache))
- return 0;
+ return NULL;
}
/*
@@ -2827,11 +2820,32 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
* to do its job and the resulting list may yet to be pruned
* by maxcount logic.
*/
- ref = ref_array_push(ref_cbdata->array, refname, oid);
+ ref = new_ref_array_item(refname, oid);
ref->commit = commit;
ref->flag = flag;
ref->kind = kind;
+ return ref;
+}
+
+struct ref_filter_cbdata {
+ struct ref_array *array;
+ struct ref_filter *filter;
+};
+
+/*
+ * A call-back given to for_each_ref(). Filter refs and keep them for
+ * later object processing.
+ */
+static int filter_one(const char *refname, const struct object_id *oid, int flag, void *cb_data)
+{
+ struct ref_filter_cbdata *ref_cbdata = cb_data;
+ struct ref_array_item *ref;
+
+ ref = apply_ref_filter(refname, oid, flag, ref_cbdata->filter);
+ if (ref)
+ ref_array_append(ref_cbdata->array, ref);
+
return 0;
}
@@ -2967,26 +2981,12 @@ void filter_ahead_behind(struct repository *r,
free(commits);
}
-/*
- * API for filtering a set of refs. Based on the type of refs the user
- * has requested, we iterate through those refs and apply filters
- * as per the given ref_filter structure and finally store the
- * filtered refs in the ref_array structure.
- */
-int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int type)
+static int do_filter_refs(struct ref_filter *filter, unsigned int type, each_ref_fn fn, void *cb_data)
{
- struct ref_filter_cbdata ref_cbdata;
- int save_commit_buffer_orig;
int ret = 0;
- ref_cbdata.array = array;
- ref_cbdata.filter = filter;
-
filter->kind = type & FILTER_REFS_KIND_MASK;
- save_commit_buffer_orig = save_commit_buffer;
- save_commit_buffer = 0;
-
init_contains_cache(&filter->internal.contains_cache);
init_contains_cache(&filter->internal.no_contains_cache);
@@ -3001,20 +3001,43 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
* of filter_ref_kind().
*/
if (filter->kind == FILTER_REFS_BRANCHES)
- ret = for_each_fullref_in("refs/heads/", ref_filter_handler, &ref_cbdata);
+ ret = for_each_fullref_in("refs/heads/", fn, cb_data);
else if (filter->kind == FILTER_REFS_REMOTES)
- ret = for_each_fullref_in("refs/remotes/", ref_filter_handler, &ref_cbdata);
+ ret = for_each_fullref_in("refs/remotes/", fn, cb_data);
else if (filter->kind == FILTER_REFS_TAGS)
- ret = for_each_fullref_in("refs/tags/", ref_filter_handler, &ref_cbdata);
+ ret = for_each_fullref_in("refs/tags/", fn, cb_data);
else if (filter->kind & FILTER_REFS_ALL)
- ret = for_each_fullref_in_pattern(filter, ref_filter_handler, &ref_cbdata);
+ ret = for_each_fullref_in_pattern(filter, fn, cb_data);
if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))
- head_ref(ref_filter_handler, &ref_cbdata);
+ head_ref(fn, cb_data);
}
clear_contains_cache(&filter->internal.contains_cache);
clear_contains_cache(&filter->internal.no_contains_cache);
+ return ret;
+}
+
+/*
+ * API for filtering a set of refs. Based on the type of refs the user
+ * has requested, we iterate through those refs and apply filters
+ * as per the given ref_filter structure and finally store the
+ * filtered refs in the ref_array structure.
+ */
+int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int type)
+{
+ struct ref_filter_cbdata ref_cbdata;
+ int save_commit_buffer_orig;
+ int ret = 0;
+
+ ref_cbdata.array = array;
+ ref_cbdata.filter = filter;
+
+ save_commit_buffer_orig = save_commit_buffer;
+ save_commit_buffer = 0;
+
+ ret = do_filter_refs(filter, type, filter_one, &ref_cbdata);
+
/* Filters that need revision walking */
reach_filter(array, &filter->reachable_from, INCLUDE_REACHED);
reach_filter(array, &filter->unreachable_from, EXCLUDE_REACHED);
--
gitgitgadget
^ permalink raw reply related
* [PATCH 7/9] ref-filter.c: filter & format refs in the same callback
From: Victoria Dye via GitGitGadget @ 2023-11-07 1:25 UTC (permalink / raw)
To: git; +Cc: Victoria Dye, Victoria Dye
In-Reply-To: <pull.1609.git.1699320361.gitgitgadget@gmail.com>
From: Victoria Dye <vdye@github.com>
Update 'filter_and_format_refs()' to try to perform ref filtering &
formatting in a single ref iteration, without an intermediate 'struct
ref_array'. This can only be done if no operations need to be performed on a
pre-filtered array; specifically, if the refs are
- filtered on reachability,
- sorted, or
- formatted with ahead-behind information
they cannot be filtered & formatted in the same iteration. In that case,
fall back on the current filter-then-sort-then-format flow.
This optimization substantially improves memory usage due to no longer
storing a ref array in memory. In some cases, it also dramatically reduces
runtime (e.g. 'git for-each-ref --no-sort --count=1', which no longer loads
all refs into a 'struct ref_array' to printing only the first ref).
Signed-off-by: Victoria Dye <vdye@github.com>
---
ref-filter.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 74 insertions(+), 6 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index ff00ab4b8d8..384cf1595ff 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2863,6 +2863,44 @@ static void free_array_item(struct ref_array_item *item)
free(item);
}
+struct ref_filter_and_format_cbdata {
+ struct ref_filter *filter;
+ struct ref_format *format;
+
+ struct ref_filter_and_format_internal {
+ int count;
+ } internal;
+};
+
+static int filter_and_format_one(const char *refname, const struct object_id *oid, int flag, void *cb_data)
+{
+ struct ref_filter_and_format_cbdata *ref_cbdata = cb_data;
+ struct ref_array_item *ref;
+ struct strbuf output = STRBUF_INIT, err = STRBUF_INIT;
+
+ ref = apply_ref_filter(refname, oid, flag, ref_cbdata->filter);
+ if (!ref)
+ return 0;
+
+ if (format_ref_array_item(ref, ref_cbdata->format, &output, &err))
+ die("%s", err.buf);
+
+ if (output.len || !ref_cbdata->format->array_opts.omit_empty) {
+ fwrite(output.buf, 1, output.len, stdout);
+ putchar('\n');
+ }
+
+ strbuf_release(&output);
+ strbuf_release(&err);
+ free_array_item(ref);
+
+ if (ref_cbdata->format->array_opts.max_count &&
+ ++ref_cbdata->internal.count >= ref_cbdata->format->array_opts.max_count)
+ return -1;
+
+ return 0;
+}
+
/* Free all memory allocated for ref_array */
void ref_array_clear(struct ref_array *array)
{
@@ -3046,16 +3084,46 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
return ret;
}
+static inline int can_do_iterative_format(struct ref_filter *filter,
+ struct ref_sorting *sorting,
+ struct ref_format *format)
+{
+ /*
+ * Refs can be filtered and formatted in the same iteration as long
+ * as we aren't filtering on reachability, sorting the results, or
+ * including ahead-behind information in the formatted output.
+ */
+ return !(filter->reachable_from ||
+ filter->unreachable_from ||
+ sorting ||
+ format->bases.nr);
+}
+
void filter_and_format_refs(struct ref_filter *filter, unsigned int type,
struct ref_sorting *sorting,
struct ref_format *format)
{
- struct ref_array array = { 0 };
- filter_refs(&array, filter, type);
- filter_ahead_behind(the_repository, format, &array);
- ref_array_sort(sorting, &array);
- print_formatted_ref_array(&array, format);
- ref_array_clear(&array);
+ if (can_do_iterative_format(filter, sorting, format)) {
+ int save_commit_buffer_orig;
+ struct ref_filter_and_format_cbdata ref_cbdata = {
+ .filter = filter,
+ .format = format,
+ };
+
+ save_commit_buffer_orig = save_commit_buffer;
+ save_commit_buffer = 0;
+
+ do_filter_refs(filter, type, filter_and_format_one, &ref_cbdata);
+
+ save_commit_buffer = save_commit_buffer_orig;
+ } else {
+ struct ref_array array = { 0 };
+ filter_refs(&array, filter, type);
+ filter_ahead_behind(the_repository, format, &array);
+ ref_array_sort(sorting, &array);
+ print_formatted_ref_array(&array, format);
+ ref_array_clear(&array);
+ }
}
static int compare_detached_head(struct ref_array_item *a, struct ref_array_item *b)
--
gitgitgadget
^ permalink raw reply related
* [PATCH 8/9] for-each-ref: add option to fully dereference tags
From: Victoria Dye via GitGitGadget @ 2023-11-07 1:26 UTC (permalink / raw)
To: git; +Cc: Victoria Dye, Victoria Dye
In-Reply-To: <pull.1609.git.1699320361.gitgitgadget@gmail.com>
From: Victoria Dye <vdye@github.com>
Add a boolean flag '--full-deref' that, when enabled, fills '%(*fieldname)'
format fields using the fully peeled target of tag objects, rather than the
immediate target.
In other builtins ('rev-parse', 'show-ref'), "dereferencing" tags typically
means peeling them down to their non-tag target. Unlike these commands,
'for-each-ref' dereferences only one "level" of tags in '*' format fields
(like "%(*objectname)"). For most annotated tags, one level of dereferencing
is enough, since most tags point to commits or trees. However, nested tags
(annotated tags whose target is another annotated tag) dereferenced once
will point to their target tag, different a full peel to e.g. a commit.
Currently, if a user wants to filter & format refs and include information
about the fully dereferenced tag, they can do so with something like
'cat-file --batch-check':
git for-each-ref --format="%(objectname)^{} %(refname)" <pattern> |
git cat-file --batch-check="%(objectname) %(rest)"
But the combination of commands is inefficient. So, to improve the
efficiency of this use case, add a '--full-deref' option that causes
'for-each-ref' to fully dereference tags when formatting with '*' fields.
Signed-off-by: Victoria Dye <vdye@github.com>
---
Documentation/git-for-each-ref.txt | 9 ++++++++
builtin/for-each-ref.c | 2 ++
ref-filter.c | 26 ++++++++++++++---------
ref-filter.h | 1 +
t/t6300-for-each-ref.sh | 34 ++++++++++++++++++++++++++++++
5 files changed, 62 insertions(+), 10 deletions(-)
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 407f624fbaa..2714a87088e 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -11,6 +11,7 @@ SYNOPSIS
'git for-each-ref' [--count=<count>] [--shell|--perl|--python|--tcl]
[(--sort=<key>)...] [--format=<format>]
[ --stdin | <pattern>... ]
+ [--full-deref]
[--points-at=<object>]
[--merged[=<object>]] [--no-merged[=<object>]]
[--contains[=<object>]] [--no-contains[=<object>]]
@@ -77,6 +78,14 @@ OPTIONS
the specified host language. This is meant to produce
a scriptlet that can directly be `eval`ed.
+--full-deref::
+ Populate dereferenced format fields (indicated with an asterisk (`*`)
+ prefix before the fieldname) with information about the fully-peeled
+ target object of a tag ref, rather than its immediate target object.
+ This only affects the output for nested annotated tags, where the tag's
+ immediate target is another tag but its fully-peeled target is another
+ object type (e.g. a commit).
+
--points-at=<object>::
Only list refs which points at the given object.
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 1c19cd5bd34..7a2127a3bc4 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -43,6 +43,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
OPT_INTEGER( 0 , "count", &format.array_opts.max_count, N_("show only <n> matched refs")),
OPT_STRING( 0 , "format", &format.format, N_("format"), N_("format to use for the output")),
OPT__COLOR(&format.use_color, N_("respect format colors")),
+ OPT_BOOL(0, "full-deref", &format.full_deref,
+ N_("fully dereference tags to populate '*' format fields")),
OPT_REF_FILTER_EXCLUDE(&filter),
OPT_REF_SORT(&sorting_options),
OPT_CALLBACK(0, "points-at", &filter.points_at,
diff --git a/ref-filter.c b/ref-filter.c
index 384cf1595ff..a66ac7921b1 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -237,7 +237,14 @@ static struct used_atom {
char *head;
} u;
} *used_atom;
-static int used_atom_cnt, need_tagged, need_symref;
+static int used_atom_cnt, need_symref;
+
+enum tag_dereference_mode {
+ NO_DEREF = 0,
+ DEREF_ONE,
+ DEREF_ALL
+};
+static enum tag_dereference_mode need_tagged;
/*
* Expand string, append it to strbuf *sb, then return error code ret.
@@ -1066,8 +1073,8 @@ static int parse_ref_filter_atom(struct ref_format *format,
memset(&used_atom[at].u, 0, sizeof(used_atom[at].u));
if (valid_atom[i].parser && valid_atom[i].parser(format, &used_atom[at], arg, err))
return -1;
- if (*atom == '*')
- need_tagged = 1;
+ if (*atom == '*' && !need_tagged)
+ need_tagged = format->full_deref ? DEREF_ALL : DEREF_ONE;
if (i == ATOM_SYMREF)
need_symref = 1;
return at;
@@ -2511,14 +2518,13 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
* If it is a tag object, see if we use a value that derefs
* the object, and if we do grab the object it refers to.
*/
- oi_deref.oid = *get_tagged_oid((struct tag *)obj);
+ if (need_tagged == DEREF_ALL) {
+ if (peel_iterated_oid(&obj->oid, &oi_deref.oid))
+ die("bad tag");
+ } else {
+ oi_deref.oid = *get_tagged_oid((struct tag *)obj);
+ }
- /*
- * NEEDSWORK: This derefs tag only once, which
- * is good to deal with chains of trust, but
- * is not consistent with what deref_tag() does
- * which peels the onion to the core.
- */
return get_object(ref, 1, &obj, &oi_deref, err);
}
diff --git a/ref-filter.h b/ref-filter.h
index 0ce5af58ab3..0caa39ecee5 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -92,6 +92,7 @@ struct ref_format {
const char *rest;
int quote_style;
int use_color;
+ int full_deref;
/* Internal state to ref-filter */
int need_color_reset_at_eol;
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 0613e5e3623..3c2af785cdb 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -1839,6 +1839,40 @@ test_expect_success 'git for-each-ref with non-existing refs' '
test_must_be_empty actual
'
+test_expect_success 'git for-each-ref with nested tags' '
+ git tag -am "Normal tag" nested/base HEAD &&
+ git tag -am "Nested tag" nested/nest1 refs/tags/nested/base &&
+ git tag -am "Double nested tag" nested/nest2 refs/tags/nested/nest1 &&
+
+ head_oid="$(git rev-parse HEAD)" &&
+ base_tag_oid="$(git rev-parse refs/tags/nested/base)" &&
+ nest1_tag_oid="$(git rev-parse refs/tags/nested/nest1)" &&
+ nest2_tag_oid="$(git rev-parse refs/tags/nested/nest2)" &&
+
+ # Without full dereference
+ cat >expect <<-EOF &&
+ refs/tags/nested/base $base_tag_oid tag $head_oid commit
+ refs/tags/nested/nest1 $nest1_tag_oid tag $base_tag_oid tag
+ refs/tags/nested/nest2 $nest2_tag_oid tag $nest1_tag_oid tag
+ EOF
+
+ git for-each-ref --format="%(refname) %(objectname) %(objecttype) %(*objectname) %(*objecttype)" \
+ refs/tags/nested/ >actual &&
+ test_cmp expect actual &&
+
+ # With full dereference
+ cat >expect <<-EOF &&
+ refs/tags/nested/base $base_tag_oid tag $head_oid commit
+ refs/tags/nested/nest1 $nest1_tag_oid tag $head_oid commit
+ refs/tags/nested/nest2 $nest2_tag_oid tag $head_oid commit
+ EOF
+
+ git for-each-ref --full-deref \
+ --format="%(refname) %(objectname) %(objecttype) %(*objectname) %(*objecttype)" \
+ refs/tags/nested/ >actual &&
+ test_cmp expect actual
+'
+
GRADE_FORMAT="%(signature:grade)%0a%(signature:key)%0a%(signature:signer)%0a%(signature:fingerprint)%0a%(signature:primarykeyfingerprint)"
TRUSTLEVEL_FORMAT="%(signature:trustlevel)%0a%(signature:key)%0a%(signature:signer)%0a%(signature:fingerprint)%0a%(signature:primarykeyfingerprint)"
--
gitgitgadget
^ permalink raw reply related
* [PATCH 9/9] t/perf: add perf tests for for-each-ref
From: Victoria Dye via GitGitGadget @ 2023-11-07 1:26 UTC (permalink / raw)
To: git; +Cc: Victoria Dye, Victoria Dye
In-Reply-To: <pull.1609.git.1699320361.gitgitgadget@gmail.com>
From: Victoria Dye <vdye@github.com>
Add performance tests for 'for-each-ref'. The tests exercise different
combinations of filters/formats/options, as well as the overall performance
of 'git for-each-ref | git cat-file --batch-check' to demonstrate the
performance difference vs. 'git for-each-ref --full-deref'.
All tests are run against a repository with 40k loose refs - 10k commits,
each having a unique:
- branch
- custom ref (refs/custom/special_*)
- annotated tag pointing at the commit
- annotated tag pointing at the other annotated tag (i.e., a nested tag)
After those tests are finished, the refs are packed with 'pack-refs --all'
and the same tests are rerun.
Signed-off-by: Victoria Dye <vdye@github.com>
---
t/perf/p6300-for-each-ref.sh | 87 ++++++++++++++++++++++++++++++++++++
1 file changed, 87 insertions(+)
create mode 100755 t/perf/p6300-for-each-ref.sh
diff --git a/t/perf/p6300-for-each-ref.sh b/t/perf/p6300-for-each-ref.sh
new file mode 100755
index 00000000000..172fd68a4e9
--- /dev/null
+++ b/t/perf/p6300-for-each-ref.sh
@@ -0,0 +1,87 @@
+#!/bin/sh
+
+test_description='performance of for-each-ref'
+. ./perf-lib.sh
+
+test_perf_fresh_repo
+
+ref_count_per_type=10000
+test_iteration_count=10
+
+test_expect_success "setup" '
+ test_commit_bulk $(( 1 + $ref_count_per_type )) &&
+
+ # Create refs
+ test_seq $ref_count_per_type |
+ sed "s,.*,update refs/heads/branch_& HEAD~&\nupdate refs/custom/special_& HEAD~&," |
+ git update-ref --stdin &&
+
+ # Create annotated tags
+ for i in $(test_seq $ref_count_per_type)
+ do
+ # Base tags
+ echo "tag tag_$i" &&
+ echo "mark :$i" &&
+ echo "from HEAD~$i" &&
+ printf "tagger %s <%s> %s\n" \
+ "$GIT_COMMITTER_NAME" \
+ "$GIT_COMMITTER_EMAIL" \
+ "$GIT_COMMITTER_DATE" &&
+ echo "data <<EOF" &&
+ echo "tag $i" &&
+ echo "EOF" &&
+
+ # Nested tags
+ echo "tag nested_$i" &&
+ echo "from :$i" &&
+ printf "tagger %s <%s> %s\n" \
+ "$GIT_COMMITTER_NAME" \
+ "$GIT_COMMITTER_EMAIL" \
+ "$GIT_COMMITTER_DATE" &&
+ echo "data <<EOF" &&
+ echo "nested tag $i" &&
+ echo "EOF" || return 1
+ done | git fast-import
+'
+
+test_for_each_ref () {
+ title="for-each-ref"
+ if test $# -gt 0; then
+ title="$title ($1)"
+ shift
+ fi
+ args="$@"
+
+ test_perf "$title" "
+ for i in \$(test_seq $test_iteration_count); do
+ git for-each-ref $args >/dev/null
+ done
+ "
+}
+
+run_tests () {
+ test_for_each_ref "$1"
+ test_for_each_ref "$1, no sort" --no-sort
+ test_for_each_ref "$1, tags" refs/tags/
+ test_for_each_ref "$1, tags, no sort" --no-sort refs/tags/
+ test_for_each_ref "$1, tags, shallow deref" '--format="%(refname) %(objectname) %(*objectname)"' refs/tags/
+ test_for_each_ref "$1, tags, shallow deref, no sort" --no-sort '--format="%(refname) %(objectname) %(*objectname)"' refs/tags/
+ test_for_each_ref "$1, tags, full deref" --full-deref '--format="%(refname) %(objectname) %(*objectname)"' refs/tags/
+ test_for_each_ref "$1, tags, full deref, no sort" --no-sort --full-deref '--format="%(refname) %(objectname) %(*objectname)"' refs/tags/
+
+ test_perf "for-each-ref ($1, tags) + cat-file --batch-check (full deref)" "
+ for i in \$(test_seq $test_iteration_count); do
+ git for-each-ref --format='%(objectname)^{} %(refname) %(objectname)' refs/tags/ | \
+ git cat-file --batch-check='%(objectname) %(rest)' >/dev/null
+ done
+ "
+}
+
+run_tests "loose"
+
+test_expect_success 'pack refs' '
+ git pack-refs --all
+'
+run_tests "packed"
+
+test_done
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 0/2] RelNotes: minor changes in 2.43.0 draft
From: Junio C Hamano @ 2023-11-07 1:32 UTC (permalink / raw)
To: Todd Zullinger; +Cc: git
In-Reply-To: <20231103141759.864875-1-tmz@pobox.com>
Todd Zullinger <tmz@pobox.com> writes:
> These are minor changes from a quick reading of the 2.43.0
> release notes draft.
>
> Todd Zullinger (2):
> RelNotes: minor typo fixes in 2.43.0 draft
> RelNotes: improve wording of credential helper notes
>
> Documentation/RelNotes/2.43.0.txt | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
Thanks. Applied.
^ permalink raw reply
* Re: [PATCH 0/2] revision: exclude all packed objects with `--unpacked`
From: Junio C Hamano @ 2023-11-07 1:42 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Jeff King
In-Reply-To: <cover.1699311386.git.me@ttaylorr.com>
Taylor Blau <me@ttaylorr.com> writes:
> While working on my longer series to enable verbatim pack reuse across
> multiple packs[^1], I noticed a couple of oddities with the `--unpacked`
> rev-walk flag.
>
> While it does exclude packed commits, it does not exclude (all) packed
> trees/blobs/annotated tags. This problem exists in the pack-bitmap
> machinery, too, which will over-count queries like:
>
> $ git rev-list --use-bitmap-index --all --unpacked --objects
>
> , etc.
>
> The fix is relatively straightforward, split across two patches that
> Peff and I worked on together earlier today.
>
> This is technically a backwards-incompatible change, but the existing
> behavior is broken and does not match the documented behavior, so I
> think in this case we are OK to change --unpacked to faithfully
> implement its documentation.
Yeah, it does sound like a straight bugfix to me.
>
> [^1]: Which, I'm very excited to say, is working :-).
>
> Taylor Blau (2):
> list-objects: drop --unpacked non-commit objects from results
> pack-bitmap: drop --unpacked non-commit objects from results
>
> list-objects.c | 3 +++
> pack-bitmap.c | 27 +++++++++++++++++++++++++++
> t/t6000-rev-list-misc.sh | 13 +++++++++++++
> t/t6113-rev-list-bitmap-filters.sh | 13 +++++++++++++
> t/t6115-rev-list-du.sh | 7 +++++++
> 5 files changed, 63 insertions(+)
>
>
> base-commit: bc5204569f7db44d22477485afd52ea410d83743
^ permalink raw reply
* Re: [PATCH 2/2] pack-bitmap: drop --unpacked non-commit objects from results
From: Junio C Hamano @ 2023-11-07 2:20 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Jeff King
In-Reply-To: <7492dc699052a392d2fb394e1dcfabebac82ded0.1699311386.git.me@ttaylorr.com>
Taylor Blau <me@ttaylorr.com> writes:
The same comment to the commit title applies here.
> .... Note that we do not have to
> inspect individual bits in the result bitmap, since we know that the
> first N (where N is the number of objects in the bitmap's pack/MIDX)
> bits correspond to objects which packed by definition.
;-)
Very nice. Since we are omitting any object that appears in some
packfile, this produces an expected result even when some of these
objects also appear in loose form.
> +test_expect_success 'bitmap traversal with --unpacked' '
> + git repack -adb &&
> + test_commit unpacked &&
> +
> + git rev-list --objects --no-object-names unpacked^.. >expect.raw &&
> + sort expect.raw >expect &&
> +
> + git rev-list --use-bitmap-index --objects --all --unpacked >actual.raw &&
> + sort actual.raw >actual &&
> +
> + test_cmp expect actual
> +'
OK.
^ permalink raw reply
* Re: [PATCH 0/4] Memory leak fixes
From: Junio C Hamano @ 2023-11-07 2:20 UTC (permalink / raw)
To: Jeff King; +Cc: Patrick Steinhardt, git
In-Reply-To: <20231106173205.GD10414@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Nov 06, 2023 at 11:45:48AM +0100, Patrick Steinhardt wrote:
>
>> this patch series fixes some memory leaks. All of these leaks have been
>> found while working on the reftable backend.
>
> All four look good to me (and the refactoring in 3/4 is very cleanly
> done).
Thanks, both.
^ permalink raw reply
* Re: [PATCH 1/2] list-objects: drop --unpacked non-commit objects from results
From: Junio C Hamano @ 2023-11-07 2:21 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Jeff King
In-Reply-To: <d3992c98aaa54c3635c249a15d919aa1177324d8.1699311386.git.me@ttaylorr.com>
Taylor Blau <me@ttaylorr.com> writes:
> Subject: Re: [PATCH 1/2] list-objects: drop --unpacked non-commit objects from results
I thought the purpose of this change is to make sure that we drop
packed objects from results when --unpacked is given? This makes
it sound as if we are dropping unpacked ones instead? I dunno.
> In git-rev-list(1), we describe the `--unpacked` option as:
>
> Only useful with `--objects`; print the object IDs that are not in
> packs.
>
> This is true of commits, which we discard via get_commit_action(), but
> not of the objects they reach. So if we ask for an --objects traversal
> with --unpacked, we may get arbitrarily many objects which are indeed
> packed.
Strictly speaking, as long as all the objects that are not in packs
are shown, "print the object IDs that are not in packs" is satisfied.
With this fix, perhaps we would want to tighten the explanation a
little bit while we are at it. Perhaps
print the object names but exclude those that are in packs
or something along that line?
> diff --git a/list-objects.c b/list-objects.c
> index c25c72b32c..c8a5fb998e 100644
> --- a/list-objects.c
> +++ b/list-objects.c
> @@ -39,6 +39,9 @@ static void show_object(struct traversal_context *ctx,
> {
> if (!ctx->show_object)
> return;
> + if (ctx->revs->unpacked && has_object_pack(&object->oid))
> + return;
> +
> ctx->show_object(object, name, ctx->show_data);
> }
The implementation is as straight-forward as it can get. Very
pleasing.
> diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
> index 12def7bcbf..6289a2e8b0 100755
> --- a/t/t6000-rev-list-misc.sh
> +++ b/t/t6000-rev-list-misc.sh
> @@ -169,4 +169,17 @@ test_expect_success 'rev-list --count --objects' '
> test_line_count = $count actual
> '
>
> +test_expect_success 'rev-list --unpacked' '
> + git repack -ad &&
> + test_commit unpacked &&
> +
> + git rev-list --objects --no-object-names unpacked^.. >expect.raw &&
> + sort expect.raw >expect &&
> +
> + git rev-list --all --objects --unpacked --no-object-names >actual.raw &&
> + sort actual.raw >actual &&
> +
> + test_cmp expect actual
> +'
> +
> test_done
^ permalink raw reply
* Re: [PATCH 0/9] for-each-ref optimizations & usability improvements
From: Junio C Hamano @ 2023-11-07 2:36 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget; +Cc: git, Victoria Dye
In-Reply-To: <pull.1609.git.1699320361.gitgitgadget@gmail.com>
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> This series is a bit of an informal follow-up to [1], adding some more
> substantial optimizations and usability fixes around ref
> filtering/formatting. Some of the changes here affect user-facing behavior,
> some are internal-only, but they're all interdependent enough to warrant
> putting them together in one series.
>
> [1]
> https://lore.kernel.org/git/pull.1594.v2.git.1696888736.gitgitgadget@gmail.com/
>
> Patch 1 changes the behavior of the '--no-sort' option in 'for-each-ref',
> 'tag', and 'branch'. Currently, it just removes previous sort keys and, if
> no further keys are specified, falls back on ascending refname sort (which,
> IMO, makes the name '--no-sort' somewhat misleading).
We can read it changes the behaviour and what the current behaviour
is, but I presume that the untold new behaviour with --no-sort is to
show the output in an unspecified order of implementation's
convenience? I think it makes quite a lot of sense if that is what
is done.
> Patch 2 updates the 'for-each-ref' docs to clearly state what happens if you
> use '--omit-empty' and '--count' together. I based the explanation on what
> the current behavior is (i.e., refs omitted with '--omit-empty' do count
> towards the total limited by '--count').
OK.
> Patches 3-7 incrementally refactor various parts of the ref
> filtering/formatting workflows in order to create a
> 'filter_and_format_refs()' function. If certain conditions are met (sorting
> disabled, no reachability filtering or ahead-behind formatting), ref
> filtering & formatting is done within a single 'for_each_fullref_in'
> callback. Especially in large repositories, this makes a huge difference in
> memory usage & runtime for certain usages of 'for-each-ref', since it's no
> longer writing everything to a 'struct ref_array' then repeatedly whittling
> down/updating its contents.
OK. I was wondering if you are going threaded implementation, until
I read into 6th line ;-)
> Patch 8 introduces a new option to 'for-each-ref' called '--full-deref'.
> When provided, any format fields for the dereferenced value of a tag (e.g.
> "%(*objectname)") will be populated with the fully peeled target of the tag;
> right now, those fields are populated with the immediate target of a tag
> (which can be another tag). This avoids the need to pipe 'for-each-ref'
> results to 'cat-file --batch-check' to get fully-peeled tag information. It
> also benefits from the 'filter_and_format_refs()' single-iteration
> optimization, since 'peel_iterated_oid()' may be able to read the
> pre-computed peeled OID from a packed ref. A couple notes on this one:
>
> * I went with a command line option for '--full-deref' rather than another
> format specifier (like ** instead of *) because it seems unlikely that a
> user is going to want to perform a shallow dereference and a full
> dereference in the same 'for-each-ref'. There's also a NEEDSWORK going
> all the way back to the introduction of 'for-each-ref' in 9f613ddd21c
> (Add git-for-each-ref: helper for language bindings, 2006-09-15) that (to
> me) implies different dereferencing behavior corresponds to different use
> cases/user needs.
Makes quite a lot of sense.
> * I'm not attached to '--full-deref' as a name - if someone has an idea for
> a more descriptive name, please suggest it!
Another candidate verb may be "to peel", and I have no strong
opinion between it and "to dereference". But I have a mild aversion
to an abbreviation that is not strongly established.
> Finally, patch 9 adds performance tests for 'for-each-ref', showing the
> effects of optimizations made throughout the series. Here are some sample
> results from my Ubuntu VM (test names shortened for space):
Nice.
^ permalink raw reply
* Re: [PATCH v6 00/14] Introduce new `git replay` command
From: Elijah Newren @ 2023-11-07 2:43 UTC (permalink / raw)
To: Christian Couder
Cc: git, Junio C Hamano, Patrick Steinhardt, Johannes Schindelin,
John Cai, Derrick Stolee, Phillip Wood, Calvin Wan, Toon Claes,
Dragan Simic, Linus Arver
In-Reply-To: <20231102135151.843758-1-christian.couder@gmail.com>
Hi,
Looking good, just one comment on one small hunk...
On Thu, Nov 2, 2023 at 6:52 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
[...]
> @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix
> -
> strvec_pushl(&rev_walk_args, "", argv[2], "--not", argv[1], NULL);
>
> ++ /*
> ++ * TODO: For now, let's warn when we see an option that we are
> ++ * going to override after setup_revisions() below. In the
> ++ * future we might want to either die() or allow them if we
> ++ * think they could be useful though.
> ++ */
> ++ for (i = 0; i < argc; i++) {
> ++ if (!strcmp(argv[i], "--reverse") || !strcmp(argv[i], "--date-order") ||
> ++ !strcmp(argv[i], "--topo-order") || !strcmp(argv[i], "--author-date-order") ||
> ++ !strcmp(argv[i], "--full-history"))
> ++ warning(_("option '%s' will be overridden"), argv[i]);
> ++ }
> ++
Two things:
1) Not sure it makes sense to throw a warning with --topo-order or
--full-history, since they would result in a value matching what we
would be setting anyway.
2) This seems like an inefficient way to provide this warning; could
we avoid parsing the arguments for an extra time? Perhaps instead
a) set the desired values here, before setup_revisions()
b) after setup_revisions, check whether these values differ from the
desired values, if so throw a warning.
c) set the desired values, again
?
^ permalink raw reply
* Re: [PATCH] diff: implement config.diff.renames=copies-harder
From: Elijah Newren @ 2023-11-07 2:45 UTC (permalink / raw)
To: Sam James via GitGitGadget; +Cc: git, Sam James
In-Reply-To: <pull.1606.git.1699010701704.gitgitgadget@gmail.com>
Hi,
On Fri, Nov 3, 2023 at 4:25 AM Sam James via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Sam James <sam@gentoo.org>
>
> This patch adds a config value for 'diff.renames' called 'copies-harder'
> which make it so '-C -C' is in effect always passed for 'git log -p',
> 'git diff', etc.
>
> This allows specifying that 'git log -p', 'git diff', etc should always act
> as if '-C --find-copies-harder' was passed.
>
> I've found this especially useful for certain types of repository (like
> Gentoo's ebuild repositories) because files are often copies of a previous
> version.
These must be very small repositories? --find-copies-harder is really
expensive...
But, if you are willing to pay the price, the idea of making this a
configuration item makes sense.
> Signed-off-by: Sam James <sam@gentoo.org>
> ---
> diff: implement config.diff.renames=copies-harder
>
> This patch adds a config value for 'diff.renames' called 'copies-harder'
> which make it so '-C -C' is in effect always passed for 'git log -p',
> 'git diff', etc.
>
> This allows specifying that 'git log -p', 'git diff', etc should always
> act as if '-C --find-copies-harder' was passed.
>
> I've found this especially useful for certain types of repository (like
> Gentoo's ebuild repositories) because files are often copies of a
> previous version.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1606%2Fthesamesam%2Fconfig-copies-harder-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1606/thesamesam/config-copies-harder-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1606
>
> Documentation/config/diff.txt | 3 ++-
> Documentation/config/status.txt | 3 ++-
> diff.c | 12 +++++++++---
> diff.h | 1 +
> diffcore-rename.c | 4 ++--
> merge-ort.c | 2 +-
> merge-recursive.c | 2 +-
> 7 files changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/config/diff.txt b/Documentation/config/diff.txt
> index bd5ae0c3378..d2ff3c62d41 100644
> --- a/Documentation/config/diff.txt
> +++ b/Documentation/config/diff.txt
> @@ -131,7 +131,8 @@ diff.renames::
> Whether and how Git detects renames. If set to "false",
> rename detection is disabled. If set to "true", basic rename
> detection is enabled. If set to "copies" or "copy", Git will
> - detect copies, as well. Defaults to true. Note that this
> + detect copies, as well. If set to "copies-harder", Git will try harder
> + to detect copies. Defaults to true. Note that this
"try harder to detect copies" feels like an unhelpful explanation. I
understand that a lengthy explanation (like the one found under the
`--find-copies-harder` option in git-diff) may not be wanted here
since we are trying to describe things succinctly, but could we at
least reference the `--find-copies-harder` option so that people know
where to go to get a more detailed explanation?
> affects only 'git diff' Porcelain like linkgit:git-diff[1] and
> linkgit:git-log[1], and not lower level commands such as
> linkgit:git-diff-files[1].
> diff --git a/Documentation/config/status.txt b/Documentation/config/status.txt
> index 2ff8237f8fc..7ca7a4becd7 100644
> --- a/Documentation/config/status.txt
> +++ b/Documentation/config/status.txt
> @@ -33,7 +33,8 @@ status.renames::
> Whether and how Git detects renames in linkgit:git-status[1] and
> linkgit:git-commit[1] . If set to "false", rename detection is
> disabled. If set to "true", basic rename detection is enabled.
> - If set to "copies" or "copy", Git will detect copies, as well.
> + If set to "copies" or "copy", Git will detect copies, as well. If
> + set to "copies-harder", Git will try harder to detect copies.
Same here.
> Defaults to the value of diff.renames.
>
> status.showStash::
> diff --git a/diff.c b/diff.c
> index 2c602df10a3..0ca906611f5 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -206,8 +206,11 @@ int git_config_rename(const char *var, const char *value)
> {
> if (!value)
> return DIFF_DETECT_RENAME;
> + if (!strcasecmp(value, "copies-harder"))
> + return DIFF_DETECT_COPY_HARDER;
> if (!strcasecmp(value, "copies") || !strcasecmp(value, "copy"))
> - return DIFF_DETECT_COPY;
> + return DIFF_DETECT_COPY;
> +
As per CodingGuidelines:
"""
- Fixing style violations while working on a real change as a
preparatory clean-up step is good, but otherwise avoid useless code
churn for the sake of conforming to the style.
"""
So, the fixing of extra space and the extra blank line should be
placed in a separate patch.
> return git_config_bool(var,value) ? DIFF_DETECT_RENAME : 0;
> }
>
> @@ -4832,8 +4835,11 @@ void diff_setup_done(struct diff_options *options)
> else
> options->flags.diff_from_contents = 0;
>
> - if (options->flags.find_copies_harder)
> + /* Just fold this in as it makes the patch-to-git smaller */
> + if (options->flags.find_copies_harder || options->detect_rename == DIFF_DETECT_COPY_HARDER) {
As per CodingGuidelines, this line is too long and should be split
across two lines at the `||`.
> + options->flags.find_copies_harder = 1;
> options->detect_rename = DIFF_DETECT_COPY;
> + }
>
> if (!options->flags.relative_name)
> options->prefix = NULL;
> @@ -5264,7 +5270,7 @@ static int diff_opt_find_copies(const struct option *opt,
> if (*arg != 0)
> return error(_("invalid argument to %s"), opt->long_name);
>
> - if (options->detect_rename == DIFF_DETECT_COPY)
> + if (options->detect_rename == DIFF_DETECT_COPY || options->detect_rename == DIFF_DETECT_COPY_HARDER)
Also too long.
> options->flags.find_copies_harder = 1;
> else
> options->detect_rename = DIFF_DETECT_COPY;
> diff --git a/diff.h b/diff.h
> index 66bd8aeb293..b29e5b777f8 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -555,6 +555,7 @@ int git_config_rename(const char *var, const char *value);
>
> #define DIFF_DETECT_RENAME 1
> #define DIFF_DETECT_COPY 2
> +#define DIFF_DETECT_COPY_HARDER 3
>
> #define DIFF_PICKAXE_ALL 1
> #define DIFF_PICKAXE_REGEX 2
> diff --git a/diffcore-rename.c b/diffcore-rename.c
> index 5a6e2bcac71..856291d66f2 100644
> --- a/diffcore-rename.c
> +++ b/diffcore-rename.c
> @@ -299,7 +299,7 @@ static int find_identical_files(struct hashmap *srcs,
> }
> /* Give higher scores to sources that haven't been used already */
> score = !source->rename_used;
> - if (source->rename_used && options->detect_rename != DIFF_DETECT_COPY)
> + if (source->rename_used && options->detect_rename != DIFF_DETECT_COPY && options->detect_rename != DIFF_DETECT_COPY_HARDER)
This line should also be split.
> continue;
> score += basename_same(source, target);
> if (score > best_score) {
> @@ -1405,7 +1405,7 @@ void diffcore_rename_extended(struct diff_options *options,
> trace2_region_enter("diff", "setup", options->repo);
> info.setup = 0;
> assert(!dir_rename_count || strmap_empty(dir_rename_count));
> - want_copies = (detect_rename == DIFF_DETECT_COPY);
> + want_copies = (detect_rename == DIFF_DETECT_COPY || detect_rename == DIFF_DETECT_COPY_HARDER);
and so should this one.
> if (dirs_removed && (break_idx || want_copies))
> BUG("dirs_removed incompatible with break/copy detection");
> if (break_idx && relevant_sources)
> diff --git a/merge-ort.c b/merge-ort.c
> index 6491070d965..77498354652 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -4782,7 +4782,7 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
> * sanity check them anyway.
> */
> assert(opt->detect_renames >= -1 &&
> - opt->detect_renames <= DIFF_DETECT_COPY);
> + opt->detect_renames <= DIFF_DETECT_COPY_HARDER);
> assert(opt->verbosity >= 0 && opt->verbosity <= 5);
> assert(opt->buffer_output <= 2);
> assert(opt->obuf.len == 0);
> diff --git a/merge-recursive.c b/merge-recursive.c
> index e3beb0801b1..d52dd536606 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -3708,7 +3708,7 @@ static int merge_start(struct merge_options *opt, struct tree *head)
> assert(opt->branch1 && opt->branch2);
>
> assert(opt->detect_renames >= -1 &&
> - opt->detect_renames <= DIFF_DETECT_COPY);
> + opt->detect_renames <= DIFF_DETECT_COPY_HARDER);
> assert(opt->detect_directory_renames >= MERGE_DIRECTORY_RENAMES_NONE &&
> opt->detect_directory_renames <= MERGE_DIRECTORY_RENAMES_TRUE);
> assert(opt->rename_limit >= -1);
>
> base-commit: 692be87cbba55e8488f805d236f2ad50483bd7d5
> --
> gitgitgadget
The overall patch makes sense and looks good, modulo some minor
stylistic things that need cleaning up.
^ permalink raw reply
* Re: [PATCH 0/9] for-each-ref optimizations & usability improvements
From: Victoria Dye @ 2023-11-07 2:48 UTC (permalink / raw)
To: Junio C Hamano, Victoria Dye via GitGitGadget; +Cc: git
In-Reply-To: <xmqqo7g69tmf.fsf@gitster.g>
Junio C Hamano wrote:
> "Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> This series is a bit of an informal follow-up to [1], adding some more
>> substantial optimizations and usability fixes around ref
>> filtering/formatting. Some of the changes here affect user-facing behavior,
>> some are internal-only, but they're all interdependent enough to warrant
>> putting them together in one series.
>>
>> [1]
>> https://lore.kernel.org/git/pull.1594.v2.git.1696888736.gitgitgadget@gmail.com/
>>
>> Patch 1 changes the behavior of the '--no-sort' option in 'for-each-ref',
>> 'tag', and 'branch'. Currently, it just removes previous sort keys and, if
>> no further keys are specified, falls back on ascending refname sort (which,
>> IMO, makes the name '--no-sort' somewhat misleading).
>
> We can read it changes the behaviour and what the current behaviour
> is, but I presume that the untold new behaviour with --no-sort is to
> show the output in an unspecified order of implementation's
> convenience? I think it makes quite a lot of sense if that is what
> is done.
Ah sorry, I over-edited my cover letter and accidentally removed the
explanation of what this patch does! Yes - the new behavior is that
'--no-sort' (assuming there are no subsequent --sort=<something> options)
will completely skip sorting the filtered refs.
>> * I'm not attached to '--full-deref' as a name - if someone has an idea for
>> a more descriptive name, please suggest it!
>
> Another candidate verb may be "to peel", and I have no strong
> opinion between it and "to dereference". But I have a mild aversion
> to an abbreviation that is not strongly established.
>
Makes sense. I got the "deref" abbreviation for 'update-ref --no-deref', but
'show-ref' has a "--dereference" option and protocol v2's "ls-refs" includes
a "peel" arg. "Dereference" is the term already used in the 'for-each-ref'
documentation, though, so if no one comes in with an especially strong
opinion on this I'll change the option to '--full-dereference'. Thanks!
^ permalink raw reply
* Re: [PATCH 0/9] for-each-ref optimizations & usability improvements
From: Junio C Hamano @ 2023-11-07 3:04 UTC (permalink / raw)
To: Victoria Dye; +Cc: Victoria Dye via GitGitGadget, git
In-Reply-To: <dbcbcf0e-aeee-4bb9-9e39-e2e85194d083@github.com>
Victoria Dye <vdye@github.com> writes:
> Ah sorry, I over-edited my cover letter and accidentally removed the
> explanation of what this patch does! Yes - the new behavior is that
> '--no-sort' (assuming there are no subsequent --sort=<something> options)
> will completely skip sorting the filtered refs.
Makes sense.
And the way to countermand "--no-sort" that appears earlier on the
command line to revert to the default sort order is "--sort" that
uses "refname" as the sort key, which is also nice.
^ permalink raw reply
* Re: [PATCH] diff: implement config.diff.renames=copies-harder
From: Junio C Hamano @ 2023-11-07 3:10 UTC (permalink / raw)
To: Elijah Newren; +Cc: Sam James via GitGitGadget, git, Sam James
In-Reply-To: <CABPp-BEuvjduS4JiORJybKtoPWvJd+BbbR_JAvZdj4Px_v8H4A@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
> On Fri, Nov 3, 2023 at 4:25 AM Sam James via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Sam James <sam@gentoo.org>
>>
>> This patch adds a config value for 'diff.renames' called 'copies-harder'
>> which make it so '-C -C' is in effect always passed for 'git log -p',
>> 'git diff', etc.
>>
>> This allows specifying that 'git log -p', 'git diff', etc should always act
>> as if '-C --find-copies-harder' was passed.
>>
>> I've found this especially useful for certain types of repository (like
>> Gentoo's ebuild repositories) because files are often copies of a previous
>> version.
>
> These must be very small repositories? --find-copies-harder is really
> expensive...
True. "often copies of a previous version" means that it is a
directory that has a collection of subdirectories, one for each
version? In a source tree managed in a version control system,
files are often rewritten in place from the previous version,
so I am puzzled by that justification.
It is, in the proposed log message of our commits, a bit unusual to
see "This patch does X" and "I do Y", by the way, which made my
reading hiccup a bit, but perhaps it is just me?
>> diff --git a/Documentation/config/diff.txt b/Documentation/config/diff.txt
>> index bd5ae0c3378..d2ff3c62d41 100644
>> --- a/Documentation/config/diff.txt
>> +++ b/Documentation/config/diff.txt
>> @@ -131,7 +131,8 @@ diff.renames::
>> Whether and how Git detects renames. If set to "false",
>> rename detection is disabled. If set to "true", basic rename
>> detection is enabled. If set to "copies" or "copy", Git will
>> - detect copies, as well. Defaults to true. Note that this
>> + detect copies, as well. If set to "copies-harder", Git will try harder
>> + to detect copies. Defaults to true. Note that this
>
> "try harder to detect copies" feels like an unhelpful explanation.
Yup. "will spend extra cycles to find more copies", perhaps?
^ permalink raw reply
* Re: Bug: magic-less pathspecs that start with ":" not processed as expected.
From: Jeff King @ 2023-11-07 3:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Joanna Wang, git
In-Reply-To: <xmqqfs1icvfl.fsf@gitster.g>
On Tue, Nov 07, 2023 at 08:29:34AM +0900, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > PS It took me a while to figure out where we document pathspec syntax. I
> > wonder if a "gitpathspecs" manpage would make sense, like we have
> > "gitrevisions".
>
> Yeah, I came to the same conclusion (should have saved time by
> scanning the mailing list before I started writing my response) and
> wondered where we wrote it down. The description you found in the
> glossary, as far as I recall, is the authoritative one and looks
> readable, but I agree it is not as discoverable as it should be.
>
> A simpler and more readable workaround than ":::file" is "./:file"
> by the way ;-)
Oh, indeed. That is much less horrible than ":(literal)". :)
-Peff
^ permalink raw reply
* Re: [PATCH v5 0/5] merge-ort: implement support for packing objects together
From: Jeff King @ 2023-11-07 3:42 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Junio C Hamano, Taylor Blau, git, Elijah Newren,
Eric W. Biederman, Patrick Steinhardt
In-Reply-To: <0ac32374-7d52-8f0c-8583-110de678291e@gmx.de>
On Mon, Nov 06, 2023 at 04:46:32PM +0100, Johannes Schindelin wrote:
> I wonder whether a more generic approach would be more desirable, an
> approach that would work for `git replay`, too, for example (where
> streaming objects does not work because they need to be made available
> immediately because subsequent `merge_incore_nonrecursive()` might expect
> the created objects to be present)?
>
> What I have in mind is more along Elijah's suggestion at the Contributor
> Summit to use the `tmp_objdir*()` machinery. But instead of discarding the
> temporary object database, the contained objects would be repacked and the
> `.pack`, (maybe `.rev`) and the `.idx` file would then be moved (in that
> order) before discarding the temporary object database.
>
> This would probably need to be implemented as a new
> `tmp_objdir_pack_and_migrate()` function that basically spawns
> `pack-objects` and feeds it the list of generated objects, writing
> directly into the non-temporary object directory, then discarding the
> `tmp_objdir`.
Perhaps I'm missing some piece of the puzzle, but I'm not sure what
you're trying to accomplish with that approach.
If the goal is to increase performance by avoiding the loose object
writes, then we haven't really helped much. We're still writing them,
and then writing them again for the repack.
If the goal is just to end up with a single nice pack for the long term,
then why do we need to use tmp_objdir at all? That point of that API is
to avoid letting other simultaneous processes see the intermediate state
before we're committed to keeping the objects around. That makes sense
for receiving a fetch or push, since we want to do some quality checks
on the objects before agreeing to keep them. But does it make sense for
a merge? Sure, in some workflows (like GitHub's test merges) we might
end up throwing away the merge result if it's not clean. But there is no
real downside to other processes seeing those objects. They can be
cleaned up at the next pruning repack.
I guess if your scenario requirements include "and we are never allowed
to run a pruning repack", then that could make sense. And I know that
has been a historical issue for GitHub. But I'm not sure it's
necessarily a good driver for an upstream feature.
As an alternative, though, I wonder if you need to have access to the
objects outside of the merge process. If not, then rather than an
alternate object store, what if that single process wrote to a streaming
pack _and_ used its running in-core index of the objects to allow access
via the usual object-retrieval. Then you'd get a single, clean pack as
the outcome _and_ you'd get the performance boost over just "write loose
objects, repack, and prune".
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] pack-bitmap: drop --unpacked non-commit objects from results
From: Jeff King @ 2023-11-07 3:52 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano
In-Reply-To: <7492dc699052a392d2fb394e1dcfabebac82ded0.1699311386.git.me@ttaylorr.com>
On Mon, Nov 06, 2023 at 05:56:33PM -0500, Taylor Blau wrote:
> Before returning a bitmap containing the result of the traversal back to
> the caller, drop any bits from the extended index which appear in one or
> more packs. This implements the correct behavior for rev-list operations
> which use the bitmap index to compute their result.
Nice. I was very happy to see the extra test for "rev-list --disk-usage"
to demonstrate this. The same should apply for "pack-objects --unpacked
--use-bitmap-index" (though in practice I don't think we'd do that, as
"--unpacked" implies on-disk repacking, which we do not generally use
with bitmaps).
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] list-objects: drop --unpacked non-commit objects from results
From: Jeff King @ 2023-11-07 4:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Taylor Blau, git
In-Reply-To: <xmqq7cmub8wm.fsf@gitster.g>
On Tue, Nov 07, 2023 at 11:21:29AM +0900, Junio C Hamano wrote:
> > In git-rev-list(1), we describe the `--unpacked` option as:
> >
> > Only useful with `--objects`; print the object IDs that are not in
> > packs.
> >
> > This is true of commits, which we discard via get_commit_action(), but
> > not of the objects they reach. So if we ask for an --objects traversal
> > with --unpacked, we may get arbitrarily many objects which are indeed
> > packed.
>
> Strictly speaking, as long as all the objects that are not in packs
> are shown, "print the object IDs that are not in packs" is satisfied.
> With this fix, perhaps we would want to tighten the explanation a
> little bit while we are at it. Perhaps
>
> print the object names but exclude those that are in packs
>
> or something along that line?
I think using the word "exclude" is a good idea, as it makes it clear
that we are omitting objects that otherwise would be traversed (as
opposed to just showing unpacked objects, reachable or not).
But I wanted to point out one other subtlety here. The existing code
(before this patch) checks for already-packed commits, and avoids adding
them to the traversal. The problem this patch is fixing is that we may
see objects they point to via other non-packed commits. But the opposite
problem exists, too: we have unpacked objects that are reachable from
those packed commits.
It's probably reasonably rare, since we _tend_ to make packs by rolling
up reachable chunks of history. But that's not a guarantee. One way I
can think of for it to happen in practice is that somebody pushes (or
fetches) a thin pack with commit C as a delta against an unpacked C'. In
that case "index-pack --fix-thin" will create a duplicate of C' in the
new pack, but its trees and blobs may remain unpacked.
I think with the patch in this series we could actually drop that "do
not traverse commits that are unpacked" line of code, and end up "more
correct". But I suspect performance of an incremental "git repack -d"
would suffer. This is kind of analagous to the "we do not traverse every
UNINTERESTING commit just to mark its trees/blobs as UNINTERESTING"
optimization. We know that it is not a true set difference, but it is OK
in practice and it buys us a lot of performance. And just like that
case, bitmaps do let us cheaply compute the true set difference. ;)
-Peff
^ permalink raw reply
* Re: [PATCH 0/2] revision: exclude all packed objects with `--unpacked`
From: Jeff King @ 2023-11-07 4:02 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano
In-Reply-To: <cover.1699311386.git.me@ttaylorr.com>
On Mon, Nov 06, 2023 at 05:56:27PM -0500, Taylor Blau wrote:
> While working on my longer series to enable verbatim pack reuse across
> multiple packs[^1], I noticed a couple of oddities with the `--unpacked`
> rev-walk flag.
>
> While it does exclude packed commits, it does not exclude (all) packed
> trees/blobs/annotated tags. This problem exists in the pack-bitmap
> machinery, too, which will over-count queries like:
>
> $ git rev-list --use-bitmap-index --all --unpacked --objects
>
> , etc.
>
> The fix is relatively straightforward, split across two patches that
> Peff and I worked on together earlier today.
I'm not sure my review is worth anything, but this looks good to me. ;)
I do think it might be worth tightening up the docs as Junio suggested,
but I would be fine to see that as a patch on top.
-Peff
^ permalink raw reply
* Re: Regression: git send-email Message-Id: numbering doesn't start at 1 any more
From: Uwe Kleine-König @ 2023-11-07 7:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Strawbridge, Douglas Anderson, git, entwicklung
In-Reply-To: <xmqqwmuucwi9.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 4696 bytes --]
Hello Junio,
On Tue, Nov 07, 2023 at 08:06:22AM +0900, Junio C Hamano wrote:
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
>
> > Hello,
> >
> > Since commit 3ece9bf0f9e24909b090cf348d89e8920bd4f82f I experience that
> > the generated Message-Ids don't start at ....-1-... any more. I have:
> >
> > $ git send-email w/*
> > ...
> > Subject: [PATCH 0/5] watchdog: Drop platform_driver_probe() and convert to platform remove callback returning void (part II)
> > Date: Mon, 6 Nov 2023 16:10:04 +0100
> > Message-ID: <20231106151003.3844134-7-u.kleine-koenig@pengutronix.de>
> > ...
> >
> > So the cover letter is sent with Message-Id: ...-7-...
>
> The above is consistent with the fact that a 5-patch series with a
> cover letter consists of 6 messages. Dry-run uses message numbers
> 1-6 and forgets to reset the counter, so the next message becomes 7.
> As you identified, the fix in 3ece9bf0 (send-email: clear the
> $message_id after validation, 2023-05-17) for the fallout from an
> even earlier change to process each message twice still had left an
> observable side effect subject to the Hyrum's law, it seems.
>
> > +my ($message_id_stamp, $message_id_serial);
> > if ($validate) {
> > # FIFOs can only be read once, exclude them from validation.
> > my @real_files = ();
> > @@ -821,6 +822,7 @@ sub is_format_patch_arg {
> > }
> > delete $ENV{GIT_SENDEMAIL_FILE_COUNTER};
> > delete $ENV{GIT_SENDEMAIL_FILE_TOTAL};
> > + $message_id_serial = 0;
> > }
>
> This fix looks quite logical to me, but even with this, the side
> effects of the earlier "read message twice" persists in end-user
> observable form, don't they? IIRC, when sending out an N message
> series, we start from the timestamp as of N seconds ago and give
> each message the Date: header that increments by 1 second, which
> would mean the validator will see Date: that is different from what
> will actually be sent out, and more importantly, the messages sent
> out for real will have timestamps from the future, negating the
> point of starting from N seconds ago in the first place.
The series I used as an example here was finally sent out with
git-send-email patched with my suggested change.
The Message-Ids involved are:
20231106154807.3866712-1-u.kleine-koenig@pengutronix.de
20231106154807.3866712-2-u.kleine-koenig@pengutronix.de
20231106154807.3866712-3-u.kleine-koenig@pengutronix.de
20231106154807.3866712-4-u.kleine-koenig@pengutronix.de
20231106154807.3866712-5-u.kleine-koenig@pengutronix.de
20231106154807.3866712-6-u.kleine-koenig@pengutronix.de
So the Ids are are identical apart from the number this report is about.
> Your script may not have been paying attention to it and only noticed
> the difference in id_serial, but somebody else would complain the
> difference coming from calling gen_header more than once for each
> messages since a8022c5f (send-email: expose header information to
> git-send-email's sendemail-validate hook, 2023-04-19).
>
> So, I dunno. Michael, what do you think? It appears to me that a
> more fundamental fix to the fallout from a8022c5f might be needed
> (e.g., we still let gen_header run while validating, but once
> validation is done, save the headers that validator saw and use them
> without calling gen_header again when we send the messages out, or
> something), if we truly want to be regression free.
That sounds sane.
> By the way, out of curiosity, earlier you said your script looks at
> the Message-IDs and counts the number of messages. How does it do
> that? Does it read the output of send-email and pass the messages
> to MTA for sending out for real?
The output of git send-email dumps the messages it sends out and then I
pick the message-id of the last mail by cut-n-paste and call my script
with that as a parameter. It then adds notes to the $commitcount topmost
commits such that I have something like this on the sent out commits:
Notes:
Forwarded: id:20231106154807.3866712-2-u.kleine-koenig@pengutronix.de (v1)
This is very convenient to find the thread to ping if a patch doesn't
make it into the next release.
(By the way, one difficulty in my script is that depending on the series
having a cover letter or not I have to apply the id:....-1-... marker or
not. Would be great if git send-email started with ...-0-... for a
series with a cover letter. Detecting that reliably isn't trivial I
guess.)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: Git Rename Detection Bug
From: Elijah Newren @ 2023-11-07 8:05 UTC (permalink / raw)
To: Jeremy Pridmore; +Cc: git@vger.kernel.org, Paul Baumgartner
In-Reply-To: <LO6P265MB6736043BE8FB607DB671D21EFAAAA@LO6P265MB6736.GBRP265.PROD.OUTLOOK.COM>
Hi,
On Mon, Nov 6, 2023 at 4:01 AM Jeremy Pridmore <jpridmore@rdt.co.uk> wrote:
>
> Thank you for filling out a Git bug report!
> Please answer the following questions to help us understand your issue.
I think it might be worthwhile to point out a few facts about rename
handling in Git, as background information that might clarify a few
things about how Git's mental model seems to differ from yours:
* In git, renames are not tracked; they are detected (based on file
similarity of the commits being compared).
* So, when you run "git mv A B", there is no rename recorded. It's
basically the same as "git rm A", followed by creating B with the same
contents, followed by "git add B".
* The detection happens whenever an operation (diff, log -p, merge,
status, etc.) needs or wants to know about renames.
* In git, directory renames are detected _after_ normal renames, and
via amalgamation of the individual renames.
* As a corollary of the last item, the only way individual renames
can be affected by directory renames, is if the individual rename on
one side of history was into a directory that the other side of
history renamed away; in such a case, we apply an _extra_ rename to
move it into the new directory. But we don't "undo" individual
renames to make them fit the majority-determined directory rename or
anything like that.
(Also, if it matters, all of this is true of both `recursive` and
`ort` merge strategies, i.e. the old default merge backend and the new
one.)
> What did you do before the bug happened? (Steps to reproduce your issue)
> I have two GIT repositories (A and B). Both migrated from the same TFS server using git-tfs tool. I migrated code into A and made lots of changes, including moving 50,000+ files from folder "/Landscape" to "/Landscape/src". B contains the same code but with various other changes made since my original migration from TFS to A. All the files in B are still in the "/Landscape" folder. I recently needed to merge my changes from A to B, so I added A as a remote to B and then performed a number of cherry-picks from A to B, but got stuck when trying to cherry-pick the commit containing the results of moving all files into "/Landscape/src".
In case anyone else wants to dig into this, note that this problem
setup precludes directory rename detection being involved. Directory
rename detection has a rule where if the source directory wasn't
entirely removed on one side, then that directory was not renamed on
that side. Seems obvious, but the upshot of that rule is that a
directory cannot be renamed into a subdirectory of itself, because by
virtue of being a subdirectory that means its parent directory still
exists.
So, this is a problem where only regular rename detection (i.e. rename
detection of individual files) is going to be at play.
> What did you expect to happen? (Expected behavior)
> I expected the git rename detection to match all files in A "/Landscape" to files in B "/Landscape/src".
Are all files under "/Landscape" from the merge base commit
individually more similar to the counterpart under "/Landscape/src"
than to files under any other directory? If not, the expectation goes
against how rename detection has worked in git from the beginning.
> What happened instead? (Actual behavior)
> Although many files were matched successfully, git mismatched over two dozen similarly named files, e.g.
>
> Incorrect path match: Landscape/Services/uiServices/Complaints/Interfaces/IAccountsIntegration.vb -> Landscape/src/Complaints/Rdt.Complaints.UI/Interfaces/IAccountsIntegration.vb
> Incorrect path match: Landscape/Services/uiServices/Complaints/Interfaces/IDocumentIntegration.vb -> Landscape/src/Complaints/Rdt.Complaints.UI/Interfaces/IDocumentIntegration.vb
> Incorrect path match: Landscape/Deployment/PowershellScripts/pre-req/Rdt.BatchProcessingService.Setup/pre-req.ps1 -> Landscape/src/Deployment/PowershellScripts/pre-req/Landscape.Net/pre-req.ps1
> Incorrect path match: Landscape/Deployment/PowershellScripts/pre-req/Workflow/pre-req.ps1 -> Landscape/src/Deployment/PowershellScripts/pre-req/Rdt.BatchProcessingService.Setup/pre-req.ps1
> Incorrect name match: Landscape/Documentation/Rdt.Documentation.UI/Properties/licenses.licx -> Landscape/src/Deployment/PowershellScripts/pre-req/Workflow/pre-req.ps1
> Incorrect path match: Landscape/Documentation/uiDocumentation/licenses.licx -> Landscape/src/Documentation/Rdt.Documentation.UI/Properties/licenses.licx
> Incorrect path match: Landscape/Import/uiImport/My Project/licenses.licx -> Landscape/src/Documentation/uiDocumentation/licenses.licx
> Incorrect path match: Landscape/Main/uiMain.Workflow/My Project/licenses.licx -> Landscape/src/Import/uiImport/My Project/licenses.licx
> Incorrect path match: Landscape/Main/uiMain/My Project/licenses.licx -> Landscape/src/Main/uiMain.Workflow/My Project/licenses.licx
> Incorrect path match: Landscape/LandscapeApiService.Setup/Setup/UIContent/RDT_Logo.ico -> Landscape/src/Main/uiMain.Workflow/Resources/RDT_Logo.ico
> Incorrect path match: Landscape/Policy/Rdt.Policy.UI.Templates/Properties/licenses.licx -> Landscape/src/Main/uiMain/My Project/licenses.licx
> Incorrect path match: Landscape/Main/uiMain.Workflow/Resources/RDT_Logo.ico -> Landscape/src/Main/uiMain/Resources/RDT_Logo.ico
> Incorrect path match: Landscape/Policy/Rdt.Policy.UI/Properties/licenses.licx -> Landscape/src/Policy/Rdt.Policy.UI.Templates/Properties/licenses.licx
> Incorrect path match: Landscape/Rates/uiRates/My Project/licenses.licx -> Landscape/src/Policy/Rdt.Policy.UI/Properties/licenses.licx
> Incorrect path match: Landscape/Rdt.Claim.UI/Properties/licenses.licx -> Landscape/src/Rates/uiRates/My Project/licenses.licx
> Incorrect path match: Landscape/Rdt.Landscape.UI.Templates.Workflow/Properties/licenses.licx -> Landscape/src/Rdt.Claim.UI/Properties/licenses.licx
> Incorrect path match: Landscape/Rdt.Landscape.UI.Templates/Properties/licenses.licx -> Landscape/src/Rdt.Landscape.UI.Templates.Workflow/Properties/licenses.licx
> Incorrect path match: Landscape/Rdt.Landscape.UI.Workflow/Properties/licenses.licx -> Landscape/src/Rdt.Landscape.UI.Templates/Properties/licenses.licx
> Incorrect path match: Landscape/Rdt.Landscape.UI/Properties/licenses.licx -> Landscape/src/Rdt.Landscape.UI.Workflow/Properties/licenses.licx
> Incorrect path match: Landscape/StandardLetters/uiStandardLetters/My Project/licenses.licx -> Landscape/src/Rdt.Landscape.UI/Properties/licenses.licx
> Incorrect path match: Landscape/Complaints/Rdt.Complaints.UI/Interfaces/IDocumentIntegration.vb -> Landscape/src/Services/uiServices/Complaints/Interfaces/IDocumentIntegration.vb
> Incorrect path match: Landscape/SystemEvents/uiSystemEvents/My Project/licenses.licx -> Landscape/src/StandardLetters/uiStandardLetters/My Project/licenses.licx
> Incorrect path match: Landscape/Services/busServices/RDT_Logo.ico -> Landscape/src/Startup/uiStartup.Workflow/Resources/RDT_Logo.ico
> Incorrect path match: Landscape/Startup/uiStartup.Workflow/Resources/RDT_Logo.ico -> Landscape/src/Startup/uiStartup/Resources/RDT_Logo.ico
> Incorrect path match: Landscape/Startup/uiStartup/Resources/RDT_Logo.ico -> Landscape/src/Startup/uiStartup32/RDT_Logo.ico
> Incorrect path match: Landscape/Startup/uiStartup/Resources/newrdlogogradiant48shad.ico -> Landscape/src/Startup/uiStartup32/newrdlogogradiant48shad.ico
> Incorrect path match: Landscape/Templates/uiTemplates.Workflow/My Project/licenses.licx -> Landscape/src/SystemEvents/uiSystemEvents/My Project/licenses.licx
> Incorrect path match: Landscape/Utils/Rdt.Utils.UI/Properties/licenses.licx -> Landscape/src/Templates/uiTemplates.Workflow/My Project/licenses.licx
> Incorrect path match: Landscape/Utils/uiUtils/My Project/licenses.licx -> Landscape/src/Utils/Rdt.Utils.UI/Properties/licenses.licx
> Incorrect name match: Landscape/WebServices/ServiceFabric/Policy/Rdt.Policy.Repository.Service.Fabric.Host/PackageRoot/Data/Swagger/Examples/POST_UKSTasks_Response.json -> Landscape/src/Utils/uiUtils/My Project/licenses.licx
>
>
> What's different between what you expected and what actually happened?
>
> As you can see, although the filenames (and content) are the same,
The content is the same as well? So, these renames that you label as
incorrect are actually _exact_ renames -- and further, in most cases
they also have an identical basename for the file as well.
Exact renames are detected first, before any other method of rename
detection is employed, and other than giving a preference to files
with the same basename, if there are multiple choices it just picks
one (what I'd call at random, though technically based on what the
internal processing order happens to be; see the "Too many identical
alternatives? Pick one" code comment).
And this, too, is true of both the `recursve` and `ort` backends; no
change has been made to how exact renames are handled.
> In some cases, it seems that the catalyst has been git thinking that a file from B has been deleted from A, when in fact it has not actually been deleted at all.
>
> For example, the file Landscape/Deployment/PowershellScripts/pre-req/Landscape.Net/pre-req.ps1 has not been deleted in A or B, therefore git should not have attempted to rename Landscape/Deployment/PowershellScripts/pre-req/Rdt.BatchProcessingService.Setup/pre-req.ps1 to Landscape/Deployment/PowershellScripts/pre-req/Landscape.Net/pre-req.ps1, especially as it then attempts to rename Landscape/Deployment/PowershellScripts/pre-req/Workflow/pre-req.ps1 to Landscape/src/Deployment/PowershellScripts/pre-req/Rdt.BatchProcessingService.Setup/pre-req.ps1 and so on.
Renamed files, from Git's perspective, always involve files that have
been deleted.
> Git status contains, for example:
> deleted by them: Landscape/Deployment/PowershellScripts/pre-req/Landscape.Net/pre-req.ps1
This means that it wasn't sufficiently similar to any of the new
files...or that _other_ deleted files were more similar to the new
files and thus that they were paired up instead of this file, leaving
this file to simply be marked as deleted. (Or that other deleted
files were just as similar; tie-breakers are kinda random in such a
case.)
[...]
> Anything else you want to add:
> I can't help but think that this is related to changes made by Palantir:
> https://blog.palantir.com/optimizing-gits-merge-machinery-1-127ceb0ef2a1
Curious. What makes you think it's related?
If there is some reason you think it's related, there's an easy way to
check -- just repeat the cherry-pick with the "-s recursive" flag to
use the old merge backend and compare the results.
I'll be somewhat surprised if it's related, though.
> I have tried to unstage these renames using "git restore --staged <file_name>" so I can then apply the correct "git mv" commands
Why? Just modify all the files to have the correct end results and then commit.
>, but bizzarely, this then results in "git status" reporting a different, smaller set of mismatched names:
As mentioned earlier, git does _not_ record renames. So, running the
correct "git mv" command doesn't really mean much. If you use
completely "incorrect" git-mv commands, but then manually tweak files
until they have the correct results, then what's recorded is exactly
the same as if you had used the "correct" git-mv commands.
Further, when you run "git status", it can't access any renames you
did because that information isn't recorded anywhere. It instead
recomputes renames on the fly. And it does so each and every time you
run "git status", even if you make no changes between two invocations.
In fact, from this you can probably also deduce that there are other
ways to affect what will be shown as renames, when you have multiple
files similar to any given source file. In particular, you can cause
a different pairing modifying one of the similar files enough that it
becomes the most similar to the source file, or so that it becomes no
longer the most similar to the source file. However, what "git
status" reports for renames is irrelevant, since that info won't be
recorded in the commit. Renames are never recorded. Anywhere.
In fact, you can even run "git status --no-renames" to just see the
old filenames that were removed and the new ones that were added
without having all the files be paired up as renames.
Hope that helps,
Elijah
^ permalink raw reply
* Re: first-class conflicts?
From: Elijah Newren @ 2023-11-07 8:16 UTC (permalink / raw)
To: Sandra Snan; +Cc: git
In-Reply-To: <87cywmintp.fsf@ellen.idiomdrottning.org>
On Mon, Nov 6, 2023 at 1:26 PM Sandra Snan
<sandra.snan@idiomdrottning.org> wrote:
>
> Is this feature from jj also a good idea for git?
> https://martinvonz.github.io/jj/v0.11.0/conflicts/
Martin talked about this and other features at Git Merge 2022, a
little over a year ago. I talked to him in more depth about these
while there. I personally think he has some really interesting
features here, though at the time, I thought that the additional
object type might be too much to ask for in a Git change, and it was
an intrinsic part of the implementation back then.
Martin also gave us an update at the 2023 Git Contributors summit, and
in particular noted a significant implementation change to not have
per-file storage of conflicts, but rather storing at the commit level
the multiple conflicting trees involved. That model might be
something we could implement in Git. And if we did, it'd solve
various issues such as people wanting to be able to stash conflicts,
or wanting to be able to partially resolve conflicts and fix it up
later, or be able to collaboratively resolve conflicts without having
everyone have access to the same checkout.
But we'd also have to be careful and think through usecases, including
in the surrounding community. People would probably want to ensure
that e.g. "Protected" or "Integration" branches don't get accept
fetches or pushes of conflicted commits, git status would probably
need some special warnings or notices, git checkout would probably
benefit from additional warnings/notices checks for those cases, git
log should probably display conflicted commits differently, we'd need
to add special handling for higher order conflicts (e.g. a merge with
conflicts is itself involved in a merge) probably similar to what jj
has done, and audit a lot of other code paths to see what would be
needed.
I think it'd be really interesting to at least investigate, but it'd
also be a lot of work, and I already have several other things I've
been wanting to get back to for over a year and haven't succeeded in
generating more time for Git.
Anyway, just my $0.02.
Elijah
^ permalink raw reply
* Re: first-class conflicts?
From: Dragan Simic @ 2023-11-07 8:21 UTC (permalink / raw)
To: Elijah Newren; +Cc: Sandra Snan, git
In-Reply-To: <CABPp-BH7WBm1j-Ue9oZFjoy6sTcw5B0hz_ndDEtJqvpZF4YF=w@mail.gmail.com>
On 2023-11-07 09:16, Elijah Newren wrote:
> But we'd also have to be careful and think through usecases, including
> in the surrounding community. People would probably want to ensure
> that e.g. "Protected" or "Integration" branches don't get accept
> fetches or pushes of conflicted commits, git status would probably
> need some special warnings or notices, git checkout would probably
> benefit from additional warnings/notices checks for those cases, git
> log should probably display conflicted commits differently, we'd need
> to add special handling for higher order conflicts (e.g. a merge with
> conflicts is itself involved in a merge) probably similar to what jj
> has done, and audit a lot of other code paths to see what would be
> needed.
That would be a truly _massive_ project.
^ 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