threads / patch / 28550

patchdiff: resurrect XDF_NEED_MINIMAL with --minimal

Subject: [PATCH] diff: resurrect XDF_NEED_MINIMAL with --minimal

## tl;dr

3 messages between Oct 2, 2011 and Oct 3, 2011. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

Junio C Hamano· Oct 2, 2011, 05:04 UTC · lore

Earlier, 582aa00 (git diff too slow for a file, 2010-05-02) unconditionally dropped XDF_NEED_MINIMAL option from the internal xdiff invocation to help performance on pathological cases, while hinting that a follow-up patch could reintroduce it with "--minimal" option from the command line.

Make it so.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * This together with René's c5aa906 (Revert removal of multi-match
   discard heuristic in 27af01, 2011-09-25) on top of v1.7.7 seems to give
   identical diff output as v1.7.1 (e.g. "git diff-tree -p v2.6.39 v3.0"
   in the kernel repository, with "--minimal").
 diff.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
Show changes to diff.c +4 −0
diff --git a/diff.c b/diff.c
index 93ef9a2..34a88db 100644
--- a/diff.c
+++ b/diff.c
@@ -3511,6 +3511,10 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 	}
 	else if (!strcmp(arg, "--abbrev"))
 		options->abbrev = DEFAULT_ABBREV;
+	else if (!strcmp(arg, "--minimal"))
+		DIFF_XDL_SET(options, NEED_MINIMAL);
+	else if (!strcmp(arg, "--no-minimal"))
+		DIFF_XDL_CLR(options, NEED_MINIMAL);
 	else if (!prefixcmp(arg, "--abbrev=")) {
 		options->abbrev = strtoul(arg + 9, NULL, 10);
 		if (options->abbrev < MINIMUM_ABBREV)

-- 
1.7.7
Jonathan Nieder· Oct 3, 2011, 12:38 UTC · re: Junio C Hamano · lore

Re: [PATCH] diff: resurrect XDF_NEED_MINIMAL with --minimal

Junio C Hamano wrote:
>  * This together with René's c5aa906 (Revert removal of multi-match
>    discard heuristic in 27af01, 2011-09-25) on top of v1.7.7 seems to give
>    identical diff output as v1.7.1 (e.g. "git diff-tree -p v2.6.39 v3.0"
>    in the kernel repository, with "--minimal").
Very neat.
Show 11 quoted lines
> --- a/diff.c
> +++ b/diff.c
> @@ -3511,6 +3511,10 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
>  	}
>  	else if (!strcmp(arg, "--abbrev"))
>  		options->abbrev = DEFAULT_ABBREV;
> +	else if (!strcmp(arg, "--minimal"))
> +		DIFF_XDL_SET(options, NEED_MINIMAL);
> +	else if (!strcmp(arg, "--no-minimal"))
> +		DIFF_XDL_CLR(options, NEED_MINIMAL);
>  	else if (!prefixcmp(arg, "--abbrev=")) {

I think this would fit well near --patience (the "xdiff options" section), instead of hidden between the unsticked and sticked forms of --abbrev. ;-)

Like this:
Show changes to diff +8 −0
diff --git i/Documentation/diff-options.txt w/Documentation/diff-options.txt
index b620b3af..4d87256e 100644
--- i/Documentation/diff-options.txt
+++ w/Documentation/diff-options.txt
@@ -48,6 +48,10 @@ endif::git-format-patch[]
 --patience::
 	Generate a diff using the "patience diff" algorithm.
 
+--minimal::
+	Spend extra time to make sure the smallest possible
+	diff is produced.
+
 --stat[=<width>[,<name-width>[,<count>]]]::
 	Generate a diffstat.  You can override the default
 	output width for 80-column terminal by `--stat=<width>`.
diff --git i/diff.c w/diff.c
index fcc00780..2282f86f 100644
--- i/diff.c
+++ w/diff.c
@@ -3393,6 +3393,10 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		DIFF_XDL_SET(options, IGNORE_WHITESPACE_AT_EOL);
 	else if (!strcmp(arg, "--patience"))
 		DIFF_XDL_SET(options, PATIENCE_DIFF);
+	else if (!strcmp(arg, "--minimal"))
+		DIFF_XDL_SET(options, NEED_MINIMAL);
+	else if (!strcmp(arg, "--no-minimal"))
+		DIFF_XDL_CLR(options, NEED_MINIMAL);
 	else if (!strcmp(arg, "--histogram"))
 		DIFF_XDL_SET(options, HISTOGRAM_DIFF);
Tay Ray Chuan· Oct 3, 2011, 13:04 UTC · re: Jonathan Nieder · lore

Re: [PATCH] diff: resurrect XDF_NEED_MINIMAL with --minimal

On Mon, Oct 3, 2011 at 8:38 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Thanks for the Cc, Johnathan.
Show 8 quoted lines
> Junio C Hamano wrote:
>
>>  * This together with René's c5aa906 (Revert removal of multi-match
>>    discard heuristic in 27af01, 2011-09-25) on top of v1.7.7 seems to give
>>    identical diff output as v1.7.1 (e.g. "git diff-tree -p v2.6.39 v3.0"
>>    in the kernel repository, with "--minimal").
>
> Very neat.

Interesting. Clearly there is more than just the multi-match discard heuristic in (xdl_clean_mmatch() and xdl_cleanup_records()).

Show 16 quoted lines
> diff --git i/diff.c w/diff.c
> index fcc00780..2282f86f 100644
> --- i/diff.c
> +++ w/diff.c
> @@ -3393,6 +3393,10 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
>                DIFF_XDL_SET(options, IGNORE_WHITESPACE_AT_EOL);
>        else if (!strcmp(arg, "--patience"))
>                DIFF_XDL_SET(options, PATIENCE_DIFF);
> +       else if (!strcmp(arg, "--minimal"))
> +               DIFF_XDL_SET(options, NEED_MINIMAL);
> +       else if (!strcmp(arg, "--no-minimal"))
> +               DIFF_XDL_CLR(options, NEED_MINIMAL);
>        else if (!strcmp(arg, "--histogram"))
>                DIFF_XDL_SET(options, HISTOGRAM_DIFF);
>
>

That's an improvement, but it would be even better if it was placed above the --<strategy> options, instead of being placed between them.

-- 
Cheers,
Ray Chuan

← back to recent threads