Re: [PATCH] range-diff: add configurable memory limit for cost matrix
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 26, 2025, 19:18 UTC
- Message-ID
- <xmqqzfblj3hq.fsf@gitster.g>
- In-Reply-To
- <pull.1958.git.1756228693233.gitgitgadget@gmail.com>
"Paulo Casaretto via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: pcasaretto <paulo.casaretto@shopify.com>
<administrivia>
It is usual to see a less human readable name embedded in the commit object than the mail header when a mail comes from GGG.
Just in case you want to be known to this community as "Paulo Casaretto", not "pcasaretto", I thought I'd point it out that you may want to redo the commit. I do not mind what name you like to use, as long as it is identifiable, and From: identity matches the identity you add your Signed-off-by: with.
</administrivia>
> Acked-by: Johannes Schindelin johannes.schindelin@gmx.de
It is unusual to lack <> around e-mail address here.
> Signed-off-by: pcasaretto <paulo.casaretto@shopify.com> > --- > range-diff: add configurable memory limit for cost matrix
Show 8 quoted lines
> +static int parse_max_memory(const struct option *opt, const char *arg, int unset)
> +{
> + size_t *max_memory = opt->value;
> + uintmax_t val;
> +
> + if (unset) {
> + return 0;
> + }No unnecessary {braces} around a single statement, please.Show 6 quoted lines
> + if (!git_parse_unsigned(arg, &val, SIZE_MAX))
> + return error(_("invalid max-memory value: %s"), arg);
> +
> + *max_memory = (size_t)val;
> + return 0;
> +}Show 26 quoted lines
> @@ -33,17 +51,21 @@ int cmd_range_diff(int argc,
> OPT_INTEGER(0, "creation-factor",
> &range_diff_opts.creation_factor,
> N_("percentage by which creation is weighted")),
> + OPT_PASSTHRU_ARGV(0, "diff-merges", &diff_merges_arg,
> + N_("style"), N_("passed to 'git log'"), 0),
> + OPT_BOOL(0, "left-only", &left_only,
> + N_("only emit output related to the first range")),
> + OPT_CALLBACK(0, "max-memory", &range_diff_opts.max_memory,
> + N_("size"),
> + N_("maximum memory for cost matrix (default 4G)"),
> + parse_max_memory),
> OPT_BOOL(0, "no-dual-color", &simple_color,
> N_("use simple diff colors")),
> OPT_PASSTHRU_ARGV(0, "notes", &other_arg,
> N_("notes"), N_("passed to 'git log'"),
> PARSE_OPT_OPTARG),
> - OPT_PASSTHRU_ARGV(0, "diff-merges", &diff_merges_arg,
> - N_("style"), N_("passed to 'git log'"), 0),
> OPT_PASSTHRU_ARGV(0, "remerge-diff", &diff_merges_arg, NULL,
> N_("passed to 'git log'"), PARSE_OPT_NOARG),
> - OPT_BOOL(0, "left-only", &left_only,
> - N_("only emit output related to the first range")),
> OPT_BOOL(0, "right-only", &right_only,
> N_("only emit output related to the second range")),
> OPT_END()This seems to mix unrelated changes. Please don't.
Or if the reordering of options do have a reason to exist in _this_ commit, please justify it in your proposed log message. Even if there were a good reason for reordering existing options, I strongly suspect that the change would want to be done in a separate, preparatory-clean-up commit (i.e., making this topic a two-patch series), because it has nothing to do with preventing inefficient cost matrix computation from consuming too much memory, which _is_ the theme of this commit.
Show 9 quoted lines
> diff --git a/range-diff.c b/range-diff.c > index 8a2dcbee322..6e9b6b115e5 100644 > --- a/range-diff.c > +++ b/range-diff.c > @@ -21,6 +21,7 @@ > #include "apply.h" > #include "revision.h" > > +
Unrelated, unexplained, and unnecessary change snuck in? Please proof-read the patch yourself before sending.
Show 8 quoted lines
> @@ -287,8 +288,8 @@ static void find_exact_matches(struct string_list *a, struct string_list *b) > } > > static int diffsize_consume(void *data, > - char *line UNUSED, > - unsigned long len UNUSED) > + char *line UNUSED, > + unsigned long len UNUSED)
What is this change about???
Show 22 quoted lines
> static void get_correspondences(struct string_list *a, struct string_list *b,
> - int creation_factor)
> + int creation_factor, size_t max_memory)
> {
> int n = a->nr + b->nr;
> int *cost, c, *a2b, *b2a;
> int i, j;
> -
> - ALLOC_ARRAY(cost, st_mult(n, n));
> + size_t cost_size = st_mult(n, n);
> + size_t cost_bytes = st_mult(sizeof(int), cost_size);
> + if (cost_bytes >= max_memory) {
> + struct strbuf cost_str = STRBUF_INIT;
> + struct strbuf max_str = STRBUF_INIT;
> + strbuf_humanise_bytes(&cost_str, cost_bytes);
> + strbuf_humanise_bytes(&max_str, max_memory);
> + die(_("range-diff: unable to compute the range-diff, since it "
> + "exceeds the maximum memory for the cost matrix: %s "
> + "(%"PRIuMAX" bytes) needed, %s (%"PRIuMAX" bytes) available"),
> + cost_str.buf, (uintmax_t)cost_bytes, max_str.buf, (uintmax_t)max_memory);
> + }
> + ALLOC_ARRAY(cost, cost_size);Nicely done.
Show 10 quoted lines
> @@ -351,7 +363,8 @@ static void get_correspondences(struct string_list *a, struct string_list *b, > } > > c = a_util->matching < 0 ? > - a_util->diffsize * creation_factor / 100 : COST_MAX; > + a_util->diffsize * creation_factor / 100 : > + COST_MAX; > for (j = b->nr; j < n; j++) > cost[i + n * j] = c; > }
There seem to be other unrelated changes indentation-only changes mixed in to the changes to this file, not just this one.
As a style fix,
c = a_util->matching < 0 ? a_util->diffsize * creation_factor / 100 : COST_MAX;
would be easier to follow and read, but please do not do such a cosmetic clean-up in the same patch. Do them in a separate preliminary clean-up patch before the "real work".
Show 9 quoted lines
> @@ -591,7 +605,8 @@ int show_range_diff(const char *range1, const char *range2,
> if (!res) {
> find_exact_matches(&branch1, &branch2);
> get_correspondences(&branch1, &branch2,
> - range_diff_opts->creation_factor);
> + range_diff_opts->creation_factor,
> + range_diff_opts->max_memory);
> output(&branch1, &branch2, range_diff_opts);
> }OK.