Optionally restrict range-diff output to "left" or "right" range only
PATCH 3/6 range-diff: simplify code spawning `git log`
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
Previously, we waited for the child process to be finished in every failing code path as well as at the end of the function show_range_diff().
However, we do not need to wait that long. Directly after reading the output of the child process, we can wrap up the child process.
This also has the advantage that we don't do a bunch of unnecessary work in case finish_command() returns with an error anyway.
--- range-diff.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
range-diff.c +2 -5
@@ -80,6 +80,8 @@ static int read_patches(const char *range, struct string_list *list,finish_command (&cp);return -1 ; } +if (finish_command (&cp)) +return -1 ; line = contents.buf; size = contents.len;
@@ -101,7 +103,6 @@ static int read_patches(const char *range, struct string_list *list,string_list_clear (list,1 );strbuf_release (&buf);strbuf_release (&contents); -finish_command (&cp);return -1 ; } util->matching =-1 ;
@@ -117,7 +118,6 @@ static int read_patches(const char *range, struct string_list *list,string_list_clear (list,1 );strbuf_release (&buf);strbuf_release (&contents); -finish_command (&cp);return -1 ; }
@@ -227,9 +227,6 @@ static int read_patches(const char *range, struct string_list *list,strbuf_release (&buf);free (current_filename); -if (finish_command (&cp)) -return -1 ; -return 0 ; }
Signature · 1 line
gitgitgadget
cover Optionally restrict range-diff output to "left" or "right" range only
One of my quite common workflows is to see whether an ancient topic branch I have lying about has made it into Git. Since my local commit OIDs have nothing to do with the OIDs of the corresponding commits in git/git, my only way is to fire up git range-diff ...upstream/master, but of course that output contains way more commits than I care about.
To help this use case, here is a patch series that teaches git range-diff the --left-only and --right-only options in the end, restricting the output to those commits and commit pairs that correspond to the commits in the first and the second range, respectively.
The first part of the series contains cleanup patches that are not strictly related to the feature I implemented here, but since I already have them, I figured I could just as well contribute them all together.
This patch series is based on js/range-diff-wo-dotdot.
Johannes Schindelin (6):
range-diff: avoid leaking memory in two error code paths
range-diff: libify the read_patches() function again
range-diff: simplify code spawning git log
range-diff: combine all options in a single data structure
range-diff: move the diffopt initialization down one layer
range-diff: offer --left-only/--right-only options
Documentation/git-range-diff.txt | 9 +++ builtin/log.c | 10 ++- builtin/range-diff.c | 21 +++++-- log-tree.c | 8 ++- range-diff.c | 101 +++++++++++++++++-------------- range-diff.h | 12 +++- t/t3206-range-diff.sh | 15 +++++ 7 files changed, 118 insertions(+), 58 deletions(-)
base-commit: 43718f6741a87f87bd400bdf5264394e980583c5
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-869%2Fdscho%2Frange-diff-left-and-right-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-869/dscho/range-diff-left-and-right-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/869
Signature · 1 line
gitgitgadget
PATCH 1/6 range-diff: avoid leaking memory in two error code paths
From: Johannes Schindelin <[email protected]>
In the code paths in question, we already release a lot of memory, but the current_filename variable was missed. Fix that.
--- range-diff.c | 2 ++ 1 file changed, 2 insertions(+)
range-diff.c +2 -0
@@ -97,6 +97,7 @@ static int read_patches(const char *range, struct string_list *list,if (get_oid (p, &util->oid)) {error (_ ("could not parse commit '%s'" ), p);free (util); +free (current_filename);string_list_clear (list,1 );strbuf_release (&buf);strbuf_release (&contents);
@@ -112,6 +113,7 @@ static int read_patches(const char *range, struct string_list *list,error (_ ("could not parse first line of `log` output: " "did not start with 'commit ': '%s'" ), line); +free (current_filename);string_list_clear (list,1 );strbuf_release (&buf);strbuf_release (&contents);
Signature · 1 line
gitgitgadget
PATCH 2/6 range-diff: libify the read_patches() function again
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
In library functions, we do want to avoid the (simple, but rather final) die() calls, instead returning with a value indicating an error.
Let's do exactly that in the code introduced in b66885a30cb8 (range-diff: add section header instead of diff header, 2019-07-11) that wants to error out if a diff header could not be parsed.
--- range-diff.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
range-diff.c +10 -3
@@ -135,9 +135,16 @@ static int read_patches(const char *range, struct string_list *list, orig_len = len; len =parse_git_diff_header (&root, &linenr,0 , line, len, size, &patch); -if (len <0 ) -die (_ ("could not parse git header '%.*s'" ), - orig_len, line); +if (len <0 ) { +error (_ ("could not parse git header '%.*s'" ), + orig_len, line); +free (util); +free (current_filename); +string_list_clear (list,1 ); +strbuf_release (&buf); +strbuf_release (&contents); +return -1 ; + }strbuf_addstr (&buf," ## " );if (patch.is_new >0 )strbuf_addf (&buf,"%s (new)" , patch.new_name);
Signature · 1 line
gitgitgadget
PATCH 4/6 range-diff: combine all options in a single data structure
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
This will make it easier to implement the --left-only and --right-only options.
--- builtin/log.c | 10 ++++++++-- builtin/range-diff.c | 13 +++++++++---- log-tree.c | 8 ++++++-- range-diff.c | 18 +++++++++--------- range-diff.h | 11 ++++++++--- 5 files changed, 40 insertions(+), 20 deletions(-)
builtin/log.c +8 -2
@@ -1231,14 +1231,20 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,*/ struct diff_options opts;struct strvec other_arg =STRVEC_INIT ; +struct range_diff_options range_diff_opts = { + .creation_factor = rev->creation_factor, + .dual_color =1 , + .diffopt = &opts, + .other_arg = &other_arg + }; +diff_setup (&opts); opts.file = rev->diffopt.file; opts.use_color = rev->diffopt.use_color;diff_setup_done (&opts);fprintf_ln (rev->diffopt.file,"%s" , rev->rdiff_title);get_notes_args (&other_arg, rev); -show_range_diff (rev->rdiff1, rev->rdiff2, - rev->creation_factor,1 , &opts, &other_arg); +show_range_diff (rev->rdiff1, rev->rdiff2, &range_diff_opts);strvec_clear (&other_arg); } }
builtin/range-diff.c +9 -4
@@ -14,12 +14,17 @@ NULLint cmd_range_diff (int argc,const char **argv,const char *prefix) { -int creation_factor =RANGE_DIFF_CREATION_FACTOR_DEFAULT ;struct diff_options diffopt = {NULL };struct strvec other_arg =STRVEC_INIT ; +struct range_diff_options range_diff_opts = { + .creation_factor =RANGE_DIFF_CREATION_FACTOR_DEFAULT , + .diffopt = &diffopt, + .other_arg = &other_arg + };int simple_color =-1 ;struct option range_diff_options[] = { -OPT_INTEGER (0 ,"creation-factor" , &creation_factor, +OPT_INTEGER (0 ,"creation-factor" , + &range_diff_opts.creation_factor,N_ ("Percentage by which creation is weighted" )),OPT_BOOL (0 ,"no-dual-color" , &simple_color,N_ ("use simple diff colors" )),
@@ -82,8 +87,8 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix) }FREE_AND_NULL (options); - res =show_range_diff (range1.buf, range2.buf, creation_factor, - simple_color <1 , &diffopt, &other_arg); + range_diff_opts.dual_color = simple_color <1 ; + res =show_range_diff (range1.buf, range2.buf, &range_diff_opts);strvec_clear (&other_arg);strbuf_release (&range1);
log-tree.c +6 -2
@@ -808,6 +808,11 @@ void show_log(struct rev_info *opt)if (cmit_fmt_is_mail (ctx.fmt) && opt->rdiff1) {struct diff_queue_struct dq;struct diff_options opts; +struct range_diff_options range_diff_opts = { + .creation_factor = opt->creation_factor, + .dual_color =1 , + .diffopt = &opts + };memcpy (&dq, &diff_queued_diff,sizeof (diff_queued_diff));DIFF_QUEUE_CLEAR (&diff_queued_diff);
@@ -822,8 +827,7 @@ void show_log(struct rev_info *opt) opts.file = opt->diffopt.file; opts.use_color = opt->diffopt.use_color;diff_setup_done (&opts); -show_range_diff (opt->rdiff1, opt->rdiff2, - opt->creation_factor,1 , &opts,NULL ); +show_range_diff (opt->rdiff1, opt->rdiff2, &range_diff_opts);memcpy (&diff_queued_diff, &dq,sizeof (diff_queued_diff)); }
range-diff.c +9 -9
@@ -525,33 +525,32 @@ static struct strbuf *output_prefix_cb(struct diff_options *opt, void *data) }int show_range_diff (const char *range1,const char *range2, -int creation_factor,int dual_color, -const struct diff_options *diffopt, -const struct strvec *other_arg) +struct range_diff_options *range_diff_opts) {int res =0 ;struct string_list branch1 =STRING_LIST_INIT_DUP ;struct string_list branch2 =STRING_LIST_INIT_DUP ; -if (read_patches (range1, &branch1, other_arg)) +if (read_patches (range1, &branch1, range_diff_opts->other_arg)) res =error (_ ("could not parse log for '%s'" ), range1); -if (!res &&read_patches (range2, &branch2, other_arg)) +if (!res &&read_patches (range2, &branch2, range_diff_opts->other_arg)) res =error (_ ("could not parse log for '%s'" ), range2);if (!res) {struct diff_options opts;struct strbuf indent =STRBUF_INIT ; -if (diffopt) -memcpy (&opts, diffopt,sizeof (opts)); +if (range_diff_opts->diffopt) +memcpy (&opts, range_diff_opts->diffopt,sizeof (opts));else diff_setup (&opts);if (!opts.output_format) opts.output_format =DIFF_FORMAT_PATCH ; opts.flags.suppress_diff_headers =1 ; - opts.flags.dual_color_diffed_diffs = dual_color; + opts.flags.dual_color_diffed_diffs = + range_diff_opts->dual_color; opts.flags.suppress_hunk_header_line_count =1 ; opts.output_prefix = output_prefix_cb;strbuf_addstr (&indent," " );
@@ -559,7 +558,8 @@ int show_range_diff(const char *range1, const char *range2,diff_setup_done (&opts);find_exact_matches (&branch1, &branch2); -get_correspondences (&branch1, &branch2, creation_factor); +get_correspondences (&branch1, &branch2, + range_diff_opts->creation_factor);output (&branch1, &branch2, &opts);strbuf_release (&indent);
range-diff.h +8 -3
@@ -6,15 +6,20 @@#define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60 +struct range_diff_options { +int creation_factor; +unsigned dual_color:1 ; +const struct diff_options *diffopt; +const struct strvec *other_arg; +}; +/* * Compare series of commits in RANGE1 and RANGE2, and emit to the * standard output. NULL can be passed to DIFFOPT to use the built-in * default. */ int show_range_diff (const char *range1,const char *range2, -int creation_factor,int dual_color, -const struct diff_options *diffopt, -const struct strvec *other_arg); +struct range_diff_options *opts);/* * Determine whether the given argument is usable as a range argument of `git
Signature · 1 line
gitgitgadget
PATCH 6/6 range-diff: offer --left-only/--right-only options
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
When comparing commit ranges, one is frequently interested only in one side, such as asking the question "Has this patch that I submitted to the Git mailing list been applied?": one would only care about the part of the output that corresponds to the commits in a local branch.
To make that possible, imitate the git rev-list options --left-only and --right-only.
This addresses https://github.com/gitgitgadget/git/issues/206
--- Documentation/git-range-diff.txt | 9 +++++++++ builtin/range-diff.c | 8 +++++++- range-diff.c | 11 ++++++++--- range-diff.h | 1 + t/t3206-range-diff.sh | 15 +++++++++++++++ 5 files changed, 40 insertions(+), 4 deletions(-)
Documentation/git-range-diff.txt +9 -0
@@ -10,6 +10,7 @@ SYNOPSIS [verse] 'git range-diff' [--color=[<when>]] [--no-color] [<diff-options>] [--no-dual-color] [--creation-factor=<factor>] + [--left-only | --right-only] ( <range1> <range2> | <rev1>...<rev2> | <base> <rev1> <rev2> ) DESCRIPTION
@@ -69,6 +70,14 @@ to revert to color all lines according to the outer diff markers See the ``Algorithm`` section below for an explanation why this is needed. +--left-only:: + Suppress commits that are missing from the first specified range + (or the "left range" when using the `<rev1>...<rev2>` format). + +--right-only:: + Suppress commits that are missing from the second specified range + (or the "right range" when using the `<rev1>...<rev2>` format). + --[no-]notes[=<ref>]:: This flag is passed to the `git log` program (see linkgit:git-log[1]) that generates the patches.
builtin/range-diff.c +7 -1
@@ -21,7 +21,7 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix) .diffopt = &diffopt, .other_arg = &other_arg }; -int simple_color =-1 ; +int simple_color =-1 , left_only =0 , right_only =0 ;struct option range_diff_options[] = { OPT_INTEGER(0, "creation-factor", &range_diff_opts.creation_factor,
@@ -31,6 +31,10 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)OPT_PASSTHRU_ARGV (0 ,"notes" , &other_arg,N_ ("notes" ),N_ ("passed to 'git log'" ),PARSE_OPT_OPTARG ), +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() };struct option *options;
@@ -88,6 +92,8 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)FREE_AND_NULL (options); range_diff_opts.dual_color = simple_color <1 ; + range_diff_opts.left_only = left_only; + range_diff_opts.right_only = right_only; res =show_range_diff (range1.buf, range2.buf, &range_diff_opts);strvec_clear (&other_arg);
range-diff.c +8 -3
@@ -513,7 +513,8 @@ static void output(struct string_list *a, struct string_list *b,/* Show unmatched LHS commit whose predecessors were shown. */ if (i < a->nr && a_util->matching <0 ) { -output_pair_header (&opts, patch_no_width, +if (!range_diff_opts->right_only) +output_pair_header (&opts, patch_no_width, &buf, &dashes, a_util,NULL ); i++;continue ;
@@ -521,7 +522,8 @@ static void output(struct string_list *a, struct string_list *b,/* Show unmatched RHS commits. */ while (j < b->nr && b_util->matching <0 ) { -output_pair_header (&opts, patch_no_width, +if (!range_diff_opts->left_only) +output_pair_header (&opts, patch_no_width, &buf, &dashes,NULL , b_util); b_util = ++j < b->nr ? b->items[j].util :NULL ; }
@@ -551,7 +553,10 @@ int show_range_diff(const char *range1, const char *range2,struct string_list branch1 =STRING_LIST_INIT_DUP ;struct string_list branch2 =STRING_LIST_INIT_DUP ; -if (read_patches (range1, &branch1, range_diff_opts->other_arg)) +if (range_diff_opts->left_only && range_diff_opts->right_only) + res =error (_ ("--left-only and --right-only are mutually exclusive" )); + +if (!res &&read_patches (range1, &branch1, range_diff_opts->other_arg)) res =error (_ ("could not parse log for '%s'" ), range1);if (!res &&read_patches (range2, &branch2, range_diff_opts->other_arg)) res =error (_ ("could not parse log for '%s'" ), range2);
range-diff.h +1 -0
@@ -9,6 +9,7 @@struct range_diff_options {int creation_factor;unsigned dual_color:1 ; +unsigned left_only:1 , right_only:1 ;const struct diff_options *diffopt;const struct strvec *other_arg; };
t/t3206-range-diff.sh +15 -0
@@ -725,4 +725,19 @@ test_expect_success 'format-patch --range-diff with multiple notes' ' test_cmp expect actual ' +test_expect_success '--left-only/--right-only' ' + git switch --orphan left-right && + test_commit first && + test_commit unmatched && + test_commit common && + git switch -C left-right first && + git cherry-pick common && + + git range-diff -s --left-only ...common >actual && + head_oid=$(git rev-parse --short HEAD) && + common_oid=$(git rev-parse --short common) && + echo "1: $head_oid = 2: $common_oid common" >expect && + test_cmp expect actual +' + test_done
Signature · 1 line
gitgitgadget
PATCH 5/6 range-diff: move the diffopt initialization down one layer
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
It is actually only the output() function that uses those diffopts. By moving the diffopt initialization down into that function, it is encapsulated better.
Incidentally, it will also make it easier to implement the --left-only and --right-only options in git range-diff because the output() function is now receiving all range-diff options as a parameter, not just the diffopts.
--- range-diff.c | 64 +++++++++++++++++++++++++--------------------------- 1 file changed, 31 insertions(+), 33 deletions(-)
range-diff.c +31 -33
@@ -464,12 +464,35 @@ static void patch_diff(const char *a, const char *b,diff_flush (diffopt); } +static struct strbuf *output_prefix_cb (struct diff_options *opt,void *data) +{ +return data; +} +static void output (struct string_list *a,struct string_list *b, -struct diff_options *diffopt) +struct range_diff_options *range_diff_opts) {struct strbuf buf =STRBUF_INIT , dashes =STRBUF_INIT ;int patch_no_width =decimal_width (1 + (a->nr > b->nr ? a->nr : b->nr));int i =0 , j =0 ; +struct diff_options opts; +struct strbuf indent =STRBUF_INIT ; + +if (range_diff_opts->diffopt) +memcpy (&opts, range_diff_opts->diffopt,sizeof (opts)); +else +diff_setup (&opts); + +if (!opts.output_format) + opts.output_format =DIFF_FORMAT_PATCH ; + opts.flags.suppress_diff_headers =1 ; + opts.flags.dual_color_diffed_diffs = + range_diff_opts->dual_color; + opts.flags.suppress_hunk_header_line_count =1 ; + opts.output_prefix = output_prefix_cb; +strbuf_addstr (&indent," " ); + opts.output_prefix_data = &indent; +diff_setup_done (&opts);/* * We assume the user is really more interested in the second argument
@@ -490,7 +513,7 @@ static void output(struct string_list *a, struct string_list *b,/* Show unmatched LHS commit whose predecessors were shown. */ if (i < a->nr && a_util->matching <0 ) { -output_pair_header (diffopt, patch_no_width, +output_pair_header (&opts, patch_no_width, &buf, &dashes, a_util,NULL ); i++;continue ;
@@ -498,7 +521,7 @@ static void output(struct string_list *a, struct string_list *b,/* Show unmatched RHS commits. */ while (j < b->nr && b_util->matching <0 ) { -output_pair_header (diffopt, patch_no_width, +output_pair_header (&opts, patch_no_width, &buf, &dashes,NULL , b_util); b_util = ++j < b->nr ? b->items[j].util :NULL ; }
@@ -506,22 +529,18 @@ static void output(struct string_list *a, struct string_list *b,/* Show matching LHS/RHS pair. */ if (j < b->nr) { a_util = a->items[b_util->matching].util; -output_pair_header (diffopt, patch_no_width, +output_pair_header (&opts, patch_no_width, &buf, &dashes, a_util, b_util); -if (!(diffopt->output_format &DIFF_FORMAT_NO_OUTPUT )) +if (!(opts.output_format &DIFF_FORMAT_NO_OUTPUT ))patch_diff (a->items[b_util->matching].string, - b->items[j].string, diffopt); + b->items[j].string, &opts); a_util->shown =1 ; j++; } }strbuf_release (&buf);strbuf_release (&dashes); -} - -static struct strbuf *output_prefix_cb (struct diff_options *opt,void *data) -{ -return data; +strbuf_release (&indent); } int show_range_diff(const char *range1, const char *range2,
@@ -538,31 +557,10 @@ int show_range_diff(const char *range1, const char *range2, res =error (_ ("could not parse log for '%s'" ), range2);if (!res) { -struct diff_options opts; -struct strbuf indent =STRBUF_INIT ; - -if (range_diff_opts->diffopt) -memcpy (&opts, range_diff_opts->diffopt,sizeof (opts)); -else -diff_setup (&opts); - -if (!opts.output_format) - opts.output_format =DIFF_FORMAT_PATCH ; - opts.flags.suppress_diff_headers =1 ; - opts.flags.dual_color_diffed_diffs = - range_diff_opts->dual_color; - opts.flags.suppress_hunk_header_line_count =1 ; - opts.output_prefix = output_prefix_cb; -strbuf_addstr (&indent," " ); - opts.output_prefix_data = &indent; -diff_setup_done (&opts); -find_exact_matches (&branch1, &branch2);get_correspondences (&branch1, &branch2, range_diff_opts->creation_factor); -output (&branch1, &branch2, &opts); - -strbuf_release (&indent); +output (&branch1, &branch2, range_diff_opts); }string_list_clear (&branch1,1 );
Signature · 1 line
gitgitgadget
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 10 lines · writes:
One of my quite common workflows is to see whether an ancient topic branch I have lying about has made it into Git. Since my local commit OIDs have nothing to do with the OIDs of the corresponding commits in git/git, my only way is to fire up git range-diff ...upstream/master, but of course that output contains way more commits than I care about.
To help this use case, here is a patch series that teaches git range-diff the --left-only and --right-only options in the end, restricting the output to those commits and commit pairs that correspond to the commits in the first and the second range, respectively.
Makes sense.
Quoted 9 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 6 lines · writes:
One of my quite common workflows is to see whether an ancient topic branch I have lying about has made it into Git. Since my local commit OIDs have nothing to do with the OIDs of the corresponding commits in git/git, my only way is to fire up git range-diff ...upstream/master, but of course that output contains way more commits than I care about.
Quoted 6 lines
Quoted 4 lines
To help this use case, here is a patch series that teaches git range-diff the --left-only and --right-only options in the end, restricting the output to those commits and commit pairs that correspond to the commits in the first and the second range, respectively.
Makes sense.
I'd add an additional use-case, which is ignoring new commits from upstream when displaying a range-diff in rerolled patch series.
Oftentimes I'll find that the automatically-prepared range diff that 'git format-patch --cover-letter --range-diff' generates will include new commits from upstream, so these new options should help me ignore those in the output.
As an aside: I am curious if I'm missing something when you say the "only way" is to ask for a 'git range-diff ...@{u}'. IIUC what you're describing, I often resort to using 'git cherry' for that exact thing. But, I may not be quite understanding your use-case (and why git-cherry doesn't do what you want already).
My latter question is purely for satisfying my own curiosity; I don't have any problem with a '--{left,right}-only' option in range-diff. From my quick read of the patches, it all looks pretty sane to me.
Thanks,
Taylor
Re: [PATCH 4/6] range-diff: combine all options in a single data structure
In reply to Johannes Schindelin via GitGitGadgetQuoted 14 lines · Johannes Schindelin via GitGitGadget
This will make it easier to implement the
--left-onlyand--right-onlyoptions.Signed-off-by Johannes Schindelin <[email protected]>---
range-diff.h +7
-0@@ -6,15 +6,20 @@ +struct range_diff_options { +int creation_factor; +unsigned dual_color:1 ; +const struct diff_options *diffopt; +const struct strvec *other_arg; +}; +
range-diff.h line 19
Quoted 10 lines
range-diff.h +1
-3/* * Compare series of commits in RANGE1 and RANGE2, and emit to the * standard output. NULL can be passed to DIFFOPT to use the built-in * default. */ int show_range_diff (const char *range1,const char *range2, -int creation_factor,int dual_color, -const struct diff_options *diffopt, -const struct strvec *other_arg); +struct range_diff_options *opts);
The function comment's mention of DIFFOPT becomes outdated with this change. Perhaps update it to say opts.diffopt or something.
Quoted 12 lines · Taylor Blau
Quoted 10 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 6 lines · writes:
One of my quite common workflows is to see whether an ancient topic branch I have lying about has made it into Git. Since my local commit OIDs have nothing to do with the OIDs of the corresponding commits in git/git, my only way is to fire up git range-diff ...upstream/master, but of course that output contains way more commits than I care about.
...Makes sense.
Quoted 7 lines
I'd add an additional use-case, which is ignoring new commits from upstream when displaying a range-diff in rerolled patch series.
Oftentimes I'll find that the automatically-prepared range diff that 'git format-patch --cover-letter --range-diff' generates will include new commits from upstream, so these new options should help me ignore those in the output.
Do you mean that the new round is based on an updated upstream commit, while the old series was based on a bit older upstream? After rebasing your topic, "range-diff @{1}..." would find the updates in the base (made in the upstream) plus the new round of your work on the right hand side of the symmetric range, while the left hand side solely consists of your old round (unless the upstream rewound their work, which should not happen). But that must not be it, I guess, because in such a case, among the commits in @{1}..HEAD, we cannot (eh, at least range-diff cannot) tell which one came from upstream and which one came from our fingers.
So I am a bit puzzled there.
Quoted 9 lines
As an aside: I am curious if I'm missing something when you say the "only way" is to ask for a 'git range-diff ...@{u}'. IIUC what you're describing, I often resort to using 'git cherry' for that exact thing. But, I may not be quite understanding your use-case (and why git-cherry doesn't do what you want already).
My latter question is purely for satisfying my own curiosity; I don't have any problem with a '--{left,right}-only' option in range-diff. From my quick read of the patches, it all looks pretty sane to me.
The question is addressed to Dscho, and I am also somewhat curious. Perhaps the reason would be that the output from cherry is not as easy to read as range-diff, without any post-processing.
I do find "range-diff ...@{u}" a bit too blunt and heavy a hammer for that task, but as they say, when you are familiar with and fond of a hammer, all tasks look like nails ;-).
Quoted 6 lines · Junio C Hamano
Quoted 6 lines
As an aside: I am curious if I'm missing something when you say the "only way" is to ask for a 'git range-diff ...@{u}'. IIUC what you're describing, I often resort to using 'git cherry' for that exact thing. But, I may not be quite understanding your use-case (and why git-cherry doesn't do what you want already).
Quoted 7 lines
Quoted 3 lines
My latter question is purely for satisfying my own curiosity; I don't have any problem with a '--{left,right}-only' option in range-diff. From my quick read of the patches, it all looks pretty sane to me.
The question is addressed to Dscho, and I am also somewhat curious. Perhaps the reason would be that the output from cherry is not as easy to read as range-diff, without any post-processing.
I had the same curiosity; I'd use git-cherry (or rev-list --cherry) for this.
I suspect the big difference is the quality of the matching. git-cherry is purely looking at patch-ids. So it is quite likely to say "this was not applied upstream" when what got applied differed slightly (e.g., fixups upstream, applied to a different base, etc). Whereas range-diff has some cost heuristics for deciding that two patches are basically the same thing. So it would find more cases (and as a bonus, give you the diff to see what tweaks were made upstream).
It does make me wonder if it would be useful for rev-list, etc to have an option to make "--cherry" use the more clever heuristics instead of just a patch-id. It would never show the same diff output as range-diff, but maybe more scripts would find the advanced heuristic useful.
I know it would probably make rebase's "ignore if in upstream" feature less clunky when I rebase topics. But it would also make it more dangerous! E.g., right now I see any upstream tweaks as potential conflicts when I rebase, and I manually review them for sanity.
-Peff
Quoted 19 lines · Junio C Hamano
Quoted 7 lines
I'd add an additional use-case, which is ignoring new commits from upstream when displaying a range-diff in rerolled patch series.
Oftentimes I'll find that the automatically-prepared range diff that 'git format-patch --cover-letter --range-diff' generates will include new commits from upstream, so these new options should help me ignore those in the output.
Do you mean that the new round is based on an updated upstream commit, while the old series was based on a bit older upstream? After rebasing your topic, "range-diff @{1}..." would find the updates in the base (made in the upstream) plus the new round of your work on the right hand side of the symmetric range, while the left hand side solely consists of your old round (unless the upstream rewound their work, which should not happen). But that must not be it, I guess, because in such a case, among the commits in @{1}..HEAD, we cannot (eh, at least range-diff cannot) tell which one came from upstream and which one came from our fingers.
Quoted 1 line
So I am a bit puzzled there.
I'm talking about a situation where a later re-roll is based of of a newer upstream. But your judgement is right: upstream's updates look like "new" commits on the right-hand side.
I have some scripts built around this, but they all boil down to passing '--range-diff=@{1}' (where @{1} is the tip of the previous reroll) to format-patch. See:
https://github.com/ttaylorr/dotfiles/blob/work-gh/bin/git-mail#L8-L10
for details.
IIUC this series, I think I'd also want to start passing '--left-only' to ignore the new commits from upstream in a range-diff, no?
Thanks,
Taylor
PATCH v2 3/6 range-diff: simplify code spawning `git log`
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
Previously, we waited for the child process to be finished in every failing code path as well as at the end of the function show_range_diff().
However, we do not need to wait that long. Directly after reading the output of the child process, we can wrap up the child process.
This also has the advantage that we don't do a bunch of unnecessary work in case finish_command() returns with an error anyway.
--- range-diff.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
range-diff.c +2 -5
@@ -81,6 +81,8 @@ static int read_patches(const char *range, struct string_list *list,finish_command (&cp);return -1 ; } +if (finish_command (&cp)) +return -1 ; line = contents.buf; size = contents.len;
@@ -102,7 +104,6 @@ static int read_patches(const char *range, struct string_list *list,string_list_clear (list,1 );strbuf_release (&buf);strbuf_release (&contents); -finish_command (&cp);return -1 ; } util->matching =-1 ;
@@ -118,7 +119,6 @@ static int read_patches(const char *range, struct string_list *list,string_list_clear (list,1 );strbuf_release (&buf);strbuf_release (&contents); -finish_command (&cp);return -1 ; }
@@ -228,9 +228,6 @@ static int read_patches(const char *range, struct string_list *list,strbuf_release (&buf);free (current_filename); -if (finish_command (&cp)) -return -1 ; -return 0 ; }
Signature · 1 line
gitgitgadget
PATCH v2 5/6 range-diff: move the diffopt initialization down one layer
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
It is actually only the output() function that uses those diffopts. By moving the diffopt initialization down into that function, it is encapsulated better.
Incidentally, it will also make it easier to implement the --left-only and --right-only options in git range-diff because the output() function is now receiving all range-diff options as a parameter, not just the diffopts.
--- range-diff.c | 64 +++++++++++++++++++++++++--------------------------- 1 file changed, 31 insertions(+), 33 deletions(-)
range-diff.c +31 -33
@@ -465,12 +465,35 @@ static void patch_diff(const char *a, const char *b,diff_flush (diffopt); } +static struct strbuf *output_prefix_cb (struct diff_options *opt,void *data) +{ +return data; +} +static void output (struct string_list *a,struct string_list *b, -struct diff_options *diffopt) +struct range_diff_options *range_diff_opts) {struct strbuf buf =STRBUF_INIT , dashes =STRBUF_INIT ;int patch_no_width =decimal_width (1 + (a->nr > b->nr ? a->nr : b->nr));int i =0 , j =0 ; +struct diff_options opts; +struct strbuf indent =STRBUF_INIT ; + +if (range_diff_opts->diffopt) +memcpy (&opts, range_diff_opts->diffopt,sizeof (opts)); +else +diff_setup (&opts); + +if (!opts.output_format) + opts.output_format =DIFF_FORMAT_PATCH ; + opts.flags.suppress_diff_headers =1 ; + opts.flags.dual_color_diffed_diffs = + range_diff_opts->dual_color; + opts.flags.suppress_hunk_header_line_count =1 ; + opts.output_prefix = output_prefix_cb; +strbuf_addstr (&indent," " ); + opts.output_prefix_data = &indent; +diff_setup_done (&opts);/* * We assume the user is really more interested in the second argument
@@ -491,7 +514,7 @@ static void output(struct string_list *a, struct string_list *b,/* Show unmatched LHS commit whose predecessors were shown. */ if (i < a->nr && a_util->matching <0 ) { -output_pair_header (diffopt, patch_no_width, +output_pair_header (&opts, patch_no_width, &buf, &dashes, a_util,NULL ); i++;continue ;
@@ -499,7 +522,7 @@ static void output(struct string_list *a, struct string_list *b,/* Show unmatched RHS commits. */ while (j < b->nr && b_util->matching <0 ) { -output_pair_header (diffopt, patch_no_width, +output_pair_header (&opts, patch_no_width, &buf, &dashes,NULL , b_util); b_util = ++j < b->nr ? b->items[j].util :NULL ; }
@@ -507,22 +530,18 @@ static void output(struct string_list *a, struct string_list *b,/* Show matching LHS/RHS pair. */ if (j < b->nr) { a_util = a->items[b_util->matching].util; -output_pair_header (diffopt, patch_no_width, +output_pair_header (&opts, patch_no_width, &buf, &dashes, a_util, b_util); -if (!(diffopt->output_format &DIFF_FORMAT_NO_OUTPUT )) +if (!(opts.output_format &DIFF_FORMAT_NO_OUTPUT ))patch_diff (a->items[b_util->matching].string, - b->items[j].string, diffopt); + b->items[j].string, &opts); a_util->shown =1 ; j++; } }strbuf_release (&buf);strbuf_release (&dashes); -} - -static struct strbuf *output_prefix_cb (struct diff_options *opt,void *data) -{ -return data; +strbuf_release (&indent); } int show_range_diff(const char *range1, const char *range2,
@@ -539,31 +558,10 @@ int show_range_diff(const char *range1, const char *range2, res =error (_ ("could not parse log for '%s'" ), range2);if (!res) { -struct diff_options opts; -struct strbuf indent =STRBUF_INIT ; - -if (range_diff_opts->diffopt) -memcpy (&opts, range_diff_opts->diffopt,sizeof (opts)); -else -diff_setup (&opts); - -if (!opts.output_format) - opts.output_format =DIFF_FORMAT_PATCH ; - opts.flags.suppress_diff_headers =1 ; - opts.flags.dual_color_diffed_diffs = - range_diff_opts->dual_color; - opts.flags.suppress_hunk_header_line_count =1 ; - opts.output_prefix = output_prefix_cb; -strbuf_addstr (&indent," " ); - opts.output_prefix_data = &indent; -diff_setup_done (&opts); -find_exact_matches (&branch1, &branch2);get_correspondences (&branch1, &branch2, range_diff_opts->creation_factor); -output (&branch1, &branch2, &opts); - -strbuf_release (&indent); +output (&branch1, &branch2, range_diff_opts); }string_list_clear (&branch1,1 );
Signature · 1 line
gitgitgadget
PATCH v2 2/6 range-diff: libify the read_patches() function again
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
In library functions, we do want to avoid the (simple, but rather final) die() calls, instead returning with a value indicating an error.
Let's do exactly that in the code introduced in b66885a30cb8 (range-diff: add section header instead of diff header, 2019-07-11) that wants to error out if a diff header could not be parsed.
--- range-diff.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
range-diff.c +10 -3
@@ -136,9 +136,16 @@ static int read_patches(const char *range, struct string_list *list, orig_len = len; len =parse_git_diff_header (&root, &linenr,0 , line, len, size, &patch); -if (len <0 ) -die (_ ("could not parse git header '%.*s'" ), - orig_len, line); +if (len <0 ) { +error (_ ("could not parse git header '%.*s'" ), + orig_len, line); +free (util); +free (current_filename); +string_list_clear (list,1 ); +strbuf_release (&buf); +strbuf_release (&contents); +return -1 ; + }strbuf_addstr (&buf," ## " );if (patch.is_new >0 )strbuf_addf (&buf,"%s (new)" , patch.new_name);
Signature · 1 line
gitgitgadget
v2 cover Optionally restrict range-diff output to "left" or "right" range only
In reply to Johannes Schindelin via GitGitGadgetOne of my quite common workflows is to see whether an ancient topic branch I have lying about has made it into Git. Since my local commit OIDs have nothing to do with the OIDs of the corresponding commits in git/git, my only way is to fire up git range-diff ...upstream/master, but of course that output contains way more commits than I care about.
To help this use case, here is a patch series that teaches git range-diff the --left-only and --right-only options in the end, restricting the output to those commits and commit pairs that correspond to the commits in the first and the second range, respectively.
The first part of the series contains cleanup patches that are not strictly related to the feature I implemented here, but since I already have them, I figured I could just as well contribute them all together.
This patch series is based on js/range-diff-wo-dotdot.
Changes since v1:
* Adjusted the comment above show_range_diff() to reflect the new signature.
Johannes Schindelin (6):
range-diff: avoid leaking memory in two error code paths
range-diff: libify the read_patches() function again
range-diff: simplify code spawning git log
range-diff: combine all options in a single data structure
range-diff: move the diffopt initialization down one layer
range-diff: offer --left-only/--right-only options
Documentation/git-range-diff.txt | 9 +++ builtin/log.c | 10 ++- builtin/range-diff.c | 21 +++++-- log-tree.c | 8 ++- range-diff.c | 101 +++++++++++++++++-------------- range-diff.h | 17 ++++-- t/t3206-range-diff.sh | 15 +++++ 7 files changed, 120 insertions(+), 61 deletions(-)
base-commit: 35fac18ea5ccab7524501bcb4542473ef8195541
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-869%2Fdscho%2Frange-diff-left-and-right-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-869/dscho/range-diff-left-and-right-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/869
Range-diff vs v1:
1: 5f2f06b79e20 = 1: 15f4f6abdaa2 range-diff: avoid leaking memory in two error code paths
2: 0d25dd2a9c2d = 2: 99e466ef32d7 range-diff: libify the read_patches() function again
3: d6c582c25df0 = 3: a36631b663e2 range-diff: simplify code spawning `git log`
4: b620be042eb3 ! 4: 7367eadfe243 range-diff: combine all options in a single data structure
@@ range-diff.h
+struct range_diff_options {
+ int creation_factor;
+ unsigned dual_color:1;
-+ const struct diff_options *diffopt;
-+ const struct strvec *other_arg;
++ const struct diff_options *diffopt; /* may be NULL */
++ const struct strvec *other_arg; /* may be NULL */
+};
+
/*
- * Compare series of commits in RANGE1 and RANGE2, and emit to the
- * standard output. NULL can be passed to DIFFOPT to use the built-in
- * default.
+- * Compare series of commits in RANGE1 and RANGE2, and emit to the
+- * standard output. NULL can be passed to DIFFOPT to use the built-in
+- * default.
++ * Compare series of commits in `range1` and `range2`, and emit to the
++ * standard output.
*/
int show_range_diff(const char *range1, const char *range2,
- int creation_factor, int dual_color,
5: 9fa945db5f13 = 5: a6285292b4f1 range-diff: move the diffopt initialization down one layer
6: 1c599abdbb6f ! 6: 8357d3c94f17 range-diff: offer --left-only/--right-only options
@@ range-diff.h
int creation_factor;
unsigned dual_color:1;
+ unsigned left_only:1, right_only:1;
- const struct diff_options *diffopt;
- const struct strvec *other_arg;
+ const struct diff_options *diffopt; /* may be NULL */
+ const struct strvec *other_arg; /* may be NULL */
};
## t/t3206-range-diff.sh ##Signature · 1 line
gitgitgadget
PATCH v2 6/6 range-diff: offer --left-only/--right-only options
From: Johannes Schindelin <[email protected]>
When comparing commit ranges, one is frequently interested only in one side, such as asking the question "Has this patch that I submitted to the Git mailing list been applied?": one would only care about the part of the output that corresponds to the commits in a local branch.
To make that possible, imitate the git rev-list options --left-only and --right-only.
This addresses https://github.com/gitgitgadget/git/issues/206
--- Documentation/git-range-diff.txt | 9 +++++++++ builtin/range-diff.c | 8 +++++++- range-diff.c | 11 ++++++++--- range-diff.h | 1 + t/t3206-range-diff.sh | 15 +++++++++++++++ 5 files changed, 40 insertions(+), 4 deletions(-)
Documentation/git-range-diff.txt +9 -0
@@ -10,6 +10,7 @@ SYNOPSIS [verse] 'git range-diff' [--color=[<when>]] [--no-color] [<diff-options>] [--no-dual-color] [--creation-factor=<factor>] + [--left-only | --right-only] ( <range1> <range2> | <rev1>...<rev2> | <base> <rev1> <rev2> ) DESCRIPTION
@@ -68,6 +69,14 @@ to revert to color all lines according to the outer diff markers See the ``Algorithm`` section below for an explanation why this is needed. +--left-only:: + Suppress commits that are missing from the first specified range + (or the "left range" when using the `<rev1>...<rev2>` format). + +--right-only:: + Suppress commits that are missing from the second specified range + (or the "right range" when using the `<rev1>...<rev2>` format). + --[no-]notes[=<ref>]:: This flag is passed to the `git log` program (see linkgit:git-log[1]) that generates the patches.
builtin/range-diff.c +7 -1
@@ -21,7 +21,7 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix) .diffopt = &diffopt, .other_arg = &other_arg }; -int simple_color =-1 ; +int simple_color =-1 , left_only =0 , right_only =0 ;struct option range_diff_options[] = { OPT_INTEGER(0, "creation-factor", &range_diff_opts.creation_factor,
@@ -31,6 +31,10 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)OPT_PASSTHRU_ARGV (0 ,"notes" , &other_arg,N_ ("notes" ),N_ ("passed to 'git log'" ),PARSE_OPT_OPTARG ), +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() };struct option *options;
@@ -88,6 +92,8 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)FREE_AND_NULL (options); range_diff_opts.dual_color = simple_color <1 ; + range_diff_opts.left_only = left_only; + range_diff_opts.right_only = right_only; res =show_range_diff (range1.buf, range2.buf, &range_diff_opts);strvec_clear (&other_arg);
range-diff.c +8 -3
@@ -514,7 +514,8 @@ static void output(struct string_list *a, struct string_list *b,/* Show unmatched LHS commit whose predecessors were shown. */ if (i < a->nr && a_util->matching <0 ) { -output_pair_header (&opts, patch_no_width, +if (!range_diff_opts->right_only) +output_pair_header (&opts, patch_no_width, &buf, &dashes, a_util,NULL ); i++;continue ;
@@ -522,7 +523,8 @@ static void output(struct string_list *a, struct string_list *b,/* Show unmatched RHS commits. */ while (j < b->nr && b_util->matching <0 ) { -output_pair_header (&opts, patch_no_width, +if (!range_diff_opts->left_only) +output_pair_header (&opts, patch_no_width, &buf, &dashes,NULL , b_util); b_util = ++j < b->nr ? b->items[j].util :NULL ; }
@@ -552,7 +554,10 @@ int show_range_diff(const char *range1, const char *range2,struct string_list branch1 =STRING_LIST_INIT_DUP ;struct string_list branch2 =STRING_LIST_INIT_DUP ; -if (read_patches (range1, &branch1, range_diff_opts->other_arg)) +if (range_diff_opts->left_only && range_diff_opts->right_only) + res =error (_ ("--left-only and --right-only are mutually exclusive" )); + +if (!res &&read_patches (range1, &branch1, range_diff_opts->other_arg)) res =error (_ ("could not parse log for '%s'" ), range1);if (!res &&read_patches (range2, &branch2, range_diff_opts->other_arg)) res =error (_ ("could not parse log for '%s'" ), range2);
range-diff.h +1 -0
@@ -9,6 +9,7 @@struct range_diff_options {int creation_factor;unsigned dual_color:1 ; +unsigned left_only:1 , right_only:1 ;const struct diff_options *diffopt;/* may be NULL */ const struct strvec *other_arg;/* may be NULL */ };
t/t3206-range-diff.sh +15 -0
@@ -730,4 +730,19 @@ test_expect_success 'format-patch --range-diff with multiple notes' ' test_cmp expect actual ' +test_expect_success '--left-only/--right-only' ' + git switch --orphan left-right && + test_commit first && + test_commit unmatched && + test_commit common && + git switch -C left-right first && + git cherry-pick common && + + git range-diff -s --left-only ...common >actual && + head_oid=$(git rev-parse --short HEAD) && + common_oid=$(git rev-parse --short common) && + echo "1: $head_oid = 2: $common_oid common" >expect && + test_cmp expect actual +' + test_done
Signature · 1 line
gitgitgadget
PATCH v2 1/6 range-diff: avoid leaking memory in two error code paths
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
In the code paths in question, we already release a lot of memory, but the current_filename variable was missed. Fix that.
--- range-diff.c | 2 ++ 1 file changed, 2 insertions(+)
range-diff.c +2 -0
@@ -98,6 +98,7 @@ static int read_patches(const char *range, struct string_list *list,if (get_oid (p, &util->oid)) {error (_ ("could not parse commit '%s'" ), p);free (util); +free (current_filename);string_list_clear (list,1 );strbuf_release (&buf);strbuf_release (&contents);
@@ -113,6 +114,7 @@ static int read_patches(const char *range, struct string_list *list,error (_ ("could not parse first line of `log` output: " "did not start with 'commit ': '%s'" ), line); +free (current_filename);string_list_clear (list,1 );strbuf_release (&buf);strbuf_release (&contents);
Signature · 1 line
gitgitgadget
Re: [PATCH 4/6] range-diff: combine all options in a single data structure
In reply to Eric SunshineHi Eric,
Quoted 21 lines · Eric Sunshine
Quoted 19 lines · Johannes Schindelin via GitGitGadget
This will make it easier to implement the
--left-onlyand--right-onlyoptions.Signed-off-by Johannes Schindelin <[email protected]>---
range-diff.h +7
-0@@ -6,15 +6,20 @@ +struct range_diff_options { +int creation_factor; +unsigned dual_color:1 ; +const struct diff_options *diffopt; +const struct strvec *other_arg; +}; +/* * Compare series of commits in RANGE1 and RANGE2, and emit to the * standard output. NULL can be passed to DIFFOPT to use the built-in * default. */
Quoted 8 lines
range-diff.h line 22
Quoted 5 lines
range-diff.h +1
-3int show_range_diff (const char *range1,const char *range2, -int creation_factor,int dual_color, -const struct diff_options *diffopt, -const struct strvec *other_arg); +struct range_diff_options *opts);The function comment's mention of DIFFOPT becomes outdated with this change. Perhaps update it to say
opts.diffoptor something.
I actually moved the comment to the new struct.
Thanks,
Dscho
PATCH v2 4/6 range-diff: combine all options in a single data structure
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
This will make it easier to implement the --left-only and --right-only options.
--- builtin/log.c | 10 ++++++++-- builtin/range-diff.c | 13 +++++++++---- log-tree.c | 8 ++++++-- range-diff.c | 18 +++++++++--------- range-diff.h | 16 ++++++++++------ 5 files changed, 42 insertions(+), 23 deletions(-)
builtin/log.c +8 -2
@@ -1231,14 +1231,20 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,*/ struct diff_options opts;struct strvec other_arg =STRVEC_INIT ; +struct range_diff_options range_diff_opts = { + .creation_factor = rev->creation_factor, + .dual_color =1 , + .diffopt = &opts, + .other_arg = &other_arg + }; +diff_setup (&opts); opts.file = rev->diffopt.file; opts.use_color = rev->diffopt.use_color;diff_setup_done (&opts);fprintf_ln (rev->diffopt.file,"%s" , rev->rdiff_title);get_notes_args (&other_arg, rev); -show_range_diff (rev->rdiff1, rev->rdiff2, - rev->creation_factor,1 , &opts, &other_arg); +show_range_diff (rev->rdiff1, rev->rdiff2, &range_diff_opts);strvec_clear (&other_arg); } }
builtin/range-diff.c +9 -4
@@ -14,12 +14,17 @@ NULLint cmd_range_diff (int argc,const char **argv,const char *prefix) { -int creation_factor =RANGE_DIFF_CREATION_FACTOR_DEFAULT ;struct diff_options diffopt = {NULL };struct strvec other_arg =STRVEC_INIT ; +struct range_diff_options range_diff_opts = { + .creation_factor =RANGE_DIFF_CREATION_FACTOR_DEFAULT , + .diffopt = &diffopt, + .other_arg = &other_arg + };int simple_color =-1 ;struct option range_diff_options[] = { -OPT_INTEGER (0 ,"creation-factor" , &creation_factor, +OPT_INTEGER (0 ,"creation-factor" , + &range_diff_opts.creation_factor,N_ ("Percentage by which creation is weighted" )),OPT_BOOL (0 ,"no-dual-color" , &simple_color,N_ ("use simple diff colors" )),
@@ -82,8 +87,8 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix) }FREE_AND_NULL (options); - res =show_range_diff (range1.buf, range2.buf, creation_factor, - simple_color <1 , &diffopt, &other_arg); + range_diff_opts.dual_color = simple_color <1 ; + res =show_range_diff (range1.buf, range2.buf, &range_diff_opts);strvec_clear (&other_arg);strbuf_release (&range1);
log-tree.c +6 -2
@@ -808,6 +808,11 @@ void show_log(struct rev_info *opt)if (cmit_fmt_is_mail (ctx.fmt) && opt->rdiff1) {struct diff_queue_struct dq;struct diff_options opts; +struct range_diff_options range_diff_opts = { + .creation_factor = opt->creation_factor, + .dual_color =1 , + .diffopt = &opts + };memcpy (&dq, &diff_queued_diff,sizeof (diff_queued_diff));DIFF_QUEUE_CLEAR (&diff_queued_diff);
@@ -822,8 +827,7 @@ void show_log(struct rev_info *opt) opts.file = opt->diffopt.file; opts.use_color = opt->diffopt.use_color;diff_setup_done (&opts); -show_range_diff (opt->rdiff1, opt->rdiff2, - opt->creation_factor,1 , &opts,NULL ); +show_range_diff (opt->rdiff1, opt->rdiff2, &range_diff_opts);memcpy (&diff_queued_diff, &dq,sizeof (diff_queued_diff)); }
range-diff.c +9 -9
@@ -526,33 +526,32 @@ static struct strbuf *output_prefix_cb(struct diff_options *opt, void *data) }int show_range_diff (const char *range1,const char *range2, -int creation_factor,int dual_color, -const struct diff_options *diffopt, -const struct strvec *other_arg) +struct range_diff_options *range_diff_opts) {int res =0 ;struct string_list branch1 =STRING_LIST_INIT_DUP ;struct string_list branch2 =STRING_LIST_INIT_DUP ; -if (read_patches (range1, &branch1, other_arg)) +if (read_patches (range1, &branch1, range_diff_opts->other_arg)) res =error (_ ("could not parse log for '%s'" ), range1); -if (!res &&read_patches (range2, &branch2, other_arg)) +if (!res &&read_patches (range2, &branch2, range_diff_opts->other_arg)) res =error (_ ("could not parse log for '%s'" ), range2);if (!res) {struct diff_options opts;struct strbuf indent =STRBUF_INIT ; -if (diffopt) -memcpy (&opts, diffopt,sizeof (opts)); +if (range_diff_opts->diffopt) +memcpy (&opts, range_diff_opts->diffopt,sizeof (opts));else diff_setup (&opts);if (!opts.output_format) opts.output_format =DIFF_FORMAT_PATCH ; opts.flags.suppress_diff_headers =1 ; - opts.flags.dual_color_diffed_diffs = dual_color; + opts.flags.dual_color_diffed_diffs = + range_diff_opts->dual_color; opts.flags.suppress_hunk_header_line_count =1 ; opts.output_prefix = output_prefix_cb;strbuf_addstr (&indent," " );
@@ -560,7 +559,8 @@ int show_range_diff(const char *range1, const char *range2,diff_setup_done (&opts);find_exact_matches (&branch1, &branch2); -get_correspondences (&branch1, &branch2, creation_factor); +get_correspondences (&branch1, &branch2, + range_diff_opts->creation_factor);output (&branch1, &branch2, &opts);strbuf_release (&indent);
range-diff.h +10 -6
@@ -6,15 +6,19 @@#define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60 +struct range_diff_options { +int creation_factor; +unsigned dual_color:1 ; +const struct diff_options *diffopt;/* may be NULL */ +const struct strvec *other_arg;/* may be NULL */ +}; +/* -* Compare series of commits in RANGE1 and RANGE2, and emit to the -* standard output. NULL can be passed to DIFFOPT to use the built-in -* default. +* Compare series of commits in `range1` and `range2`, and emit to the +* standard output. */ int show_range_diff (const char *range1,const char *range2, -int creation_factor,int dual_color, -const struct diff_options *diffopt, -const struct strvec *other_arg); +struct range_diff_options *opts);/* * Determine whether the given argument is usable as a range argument of `git
Signature · 1 line
gitgitgadget
Hi Peff,
Quoted 16 lines · Jeff King
Quoted 6 lines · Junio C Hamano
Quoted 6 lines
As an aside: I am curious if I'm missing something when you say the "only way" is to ask for a 'git range-diff ...@{u}'. IIUC what you're describing, I often resort to using 'git cherry' for that exact thing. But, I may not be quite understanding your use-case (and why git-cherry doesn't do what you want already).
Quoted 7 lines
Quoted 3 lines
My latter question is purely for satisfying my own curiosity; I don't have any problem with a '--{left,right}-only' option in range-diff. From my quick read of the patches, it all looks pretty sane to me.
The question is addressed to Dscho, and I am also somewhat curious. Perhaps the reason would be that the output from cherry is not as easy to read as range-diff, without any post-processing.
Quoted 5 lines
I had the same curiosity; I'd use git-cherry (or rev-list --cherry) for this.
I suspect the big difference is the quality of the matching. git-cherry is purely looking at patch-ids.
Indeed. Whenever I had tried git cherry in the past (which, admittedly, has been with geometrically decreasing frequency given the results), it completely failed to help me. And it's not only its reliance on perfect matches of the diff _with context lines_, it is also that the commit messages are completely ignored.
git cherry's track record with me is so perfect that I want to put this line into all my Bash profiles:
eval "$(set | sed -n '/^__git_main /,/^}$/{s/--list-cmds=list-mainporcelain[^)]*/& | grep -v ^cherry\$/;p}')"Quoted 12 lines
So it is quite likely to say "this was not applied upstream" when what got applied differed slightly (e.g., fixups upstream, applied to a different base, etc). Whereas range-diff has some cost heuristics for deciding that two patches are basically the same thing. So it would find more cases (and as a bonus, give you the diff to see what tweaks were made upstream).
It does make me wonder if it would be useful for rev-list, etc to have an option to make "--cherry" use the more clever heuristics instead of just a patch-id. It would never show the same diff output as range-diff, but maybe more scripts would find the advanced heuristic useful.
Quoted 4 lines
I know it would probably make rebase's "ignore if in upstream" feature less clunky when I rebase topics. But it would also make it more dangerous! E.g., right now I see any upstream tweaks as potential conflicts when I rebase, and I manually review them for sanity.
Yeah, I thought the same when I read the paragraphs before this one. It might sound convenient, but there _are_ false positives in git range-diff's output, therefore I would recommend never using git range-diff --left-only or [...] --right-only with -s. IOW _always_ inspect the differences.
Ciao,
Dscho