| Message ID | 0cb86b383b9c115c9c6077d47f0c124a96b30acf.1574207673.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | range-diff: learn `--notes` | expand |
Denton Liu <liu.denton@gmail.com> writes: > When a commit being range-diff'd has a note attached to it, the note > will be compared as well. However, if a user has multiple notes refs or > if they want to suppress notes from being printed, there is currently no > way to do this. > > Passthrough `---no--notes` to the `git log` call so that this option is "`--[no-]notes`" or perhaps "`--[no-]notes` and `--notes=<ref>`"? I think the verb phrase is two words, "pass through", by the way. > +--[no-]notes[=<treeish>]:: > + This flag is passed to the `git log` program > + (see linkgit:git-log[1]) that generates the patches. I can see this was taken from "git log --help", and it probably needs fixing for consistency as well, but I think --notes=<ref> would be easier to click users' minds with notes.displayRef configuration variable. > @@ -61,8 +62,11 @@ static int read_patches(const char *range, struct string_list *list) > "--output-indicator-new=>", > "--output-indicator-old=<", > "--output-indicator-context=#", > - "--no-abbrev-commit", range, > + "--no-abbrev-commit", > NULL); > + if (other_arg) > + argv_array_pushv(&cp.args, other_arg->argv); > + argv_array_push(&cp.args, range); Makes sense. > diff --git a/range-diff.h b/range-diff.h > index 08a50b6e98..7d918ab9ed 100644 > --- a/range-diff.h > +++ b/range-diff.h > @@ -2,6 +2,7 @@ > #define RANGE_DIFF_H > > #include "diff.h" > +#include "argv-array.h" > > #define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60 > > @@ -12,6 +13,7 @@ > */ > int show_range_diff(const char *range1, const char *range2, > int creation_factor, int dual_color, > - struct diff_options *diffopt); > + struct diff_options *diffopt, > + struct argv_array *other_arg); > > #endif I thought a mere use of "pointer to a struct" did not have to bring the definition of the struct into the picture? In other words, wouldn't it be fine to leave the other_arg a pointer to an opaque structure by not including "argv-array.h" in this file? Thanks.
Hi Junio, On Wed, Nov 20, 2019 at 01:26:04PM +0900, Junio C Hamano wrote: > Denton Liu <liu.denton@gmail.com> writes: > > > When a commit being range-diff'd has a note attached to it, the note > > will be compared as well. However, if a user has multiple notes refs or > > if they want to suppress notes from being printed, there is currently no > > way to do this. > > > > Passthrough `---no--notes` to the `git log` call so that this option is > > "`--[no-]notes`" or perhaps "`--[no-]notes` and `--notes=<ref>`"? Whoops, I probably typed `ys-` in vim instead of `ys[` so I surrounded the `no-` with the wrong characters. > > I think the verb phrase is two words, "pass through", by the way. Thanks, I didn't know this. > > > +--[no-]notes[=<treeish>]:: > > + This flag is passed to the `git log` program > > + (see linkgit:git-log[1]) that generates the patches. > > I can see this was taken from "git log --help", and it probably > needs fixing for consistency as well, but I think --notes=<ref> > would be easier to click users' minds with notes.displayRef > configuration variable. I'll put in a cleanup patch for this as well. > > > @@ -61,8 +62,11 @@ static int read_patches(const char *range, struct string_list *list) > > "--output-indicator-new=>", > > "--output-indicator-old=<", > > "--output-indicator-context=#", > > - "--no-abbrev-commit", range, > > + "--no-abbrev-commit", > > NULL); > > + if (other_arg) > > + argv_array_pushv(&cp.args, other_arg->argv); > > + argv_array_push(&cp.args, range); > > Makes sense. > > > diff --git a/range-diff.h b/range-diff.h > > index 08a50b6e98..7d918ab9ed 100644 > > --- a/range-diff.h > > +++ b/range-diff.h > > @@ -2,6 +2,7 @@ > > #define RANGE_DIFF_H > > > > #include "diff.h" > > +#include "argv-array.h" > > > > #define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60 > > > > @@ -12,6 +13,7 @@ > > */ > > int show_range_diff(const char *range1, const char *range2, > > int creation_factor, int dual_color, > > - struct diff_options *diffopt); > > + struct diff_options *diffopt, > > + struct argv_array *other_arg); > > > > #endif > > I thought a mere use of "pointer to a struct" did not have to bring > the definition of the struct into the picture? In other words, > wouldn't it be fine to leave the other_arg a pointer to an opaque > structure by not including "argv-array.h" in this file? Without including "argv-array.h", we get the following hdr-check error: $ make range-diff.hco HDR range-diff.h In file included from range-diff.hcc:2: ./range-diff.h:16:14: error: declaration of 'struct argv_array' will not be visible outside of this function [-Werror,-Wvisibility] struct argv_array *other_arg); ^ 1 error generated. make: *** [range-diff.hco] Error 1 I am currently using this compiler for reference: $ clang --version Apple LLVM version 10.0.1 (clang-1001.0.46.4) Target: x86_64-apple-darwin18.7.0 Thread model: posix InstalledDir: /Library/Developer/CommandLineTools/usr/bin Thanks, Denton > > Thanks.
On Wed, Nov 20, 2019 at 2:13 PM Denton Liu <liu.denton@gmail.com> wrote: > On Wed, Nov 20, 2019 at 01:26:04PM +0900, Junio C Hamano wrote: > > Denton Liu <liu.denton@gmail.com> writes: > > > +#include "argv-array.h" > > > > > > int show_range_diff(const char *range1, const char *range2, > > > int creation_factor, int dual_color, > > > - struct diff_options *diffopt); > > > + struct diff_options *diffopt, > > > + struct argv_array *other_arg); > > > > I thought a mere use of "pointer to a struct" did not have to bring > > the definition of the struct into the picture? In other words, > > wouldn't it be fine to leave the other_arg a pointer to an opaque > > structure by not including "argv-array.h" in this file? > > Without including "argv-array.h", we get the following hdr-check error: > > $ make range-diff.hco > In file included from range-diff.hcc:2: > ./range-diff.h:16:14: error: declaration of 'struct argv_array' will not be visible outside of this function [-Werror,-Wvisibility] > struct argv_array *other_arg); Did you forward-declare 'argv_array' in range-diff.h? That is, rather than: #include "argv-array.h" instead say: struct argv_array; which tells the compiler that the type exists, thus allowing you to deal with pointers to the struct, as long as you don't try to access any of the struct's members in code which has seen only the forward declaration. You would still need to #include "argv-array.h" in range-diff.c, though.
Hi Eric, On Thu, Nov 21, 2019 at 07:43:18AM -0500, Eric Sunshine wrote: > On Wed, Nov 20, 2019 at 2:13 PM Denton Liu <liu.denton@gmail.com> wrote: > > On Wed, Nov 20, 2019 at 01:26:04PM +0900, Junio C Hamano wrote: > > > Denton Liu <liu.denton@gmail.com> writes: > > > > +#include "argv-array.h" > > > > > > > > int show_range_diff(const char *range1, const char *range2, > > > > int creation_factor, int dual_color, > > > > - struct diff_options *diffopt); > > > > + struct diff_options *diffopt, > > > > + struct argv_array *other_arg); > > > > > > I thought a mere use of "pointer to a struct" did not have to bring > > > the definition of the struct into the picture? In other words, > > > wouldn't it be fine to leave the other_arg a pointer to an opaque > > > structure by not including "argv-array.h" in this file? > > > > Without including "argv-array.h", we get the following hdr-check error: > > > > $ make range-diff.hco > > In file included from range-diff.hcc:2: > > ./range-diff.h:16:14: error: declaration of 'struct argv_array' will not be visible outside of this function [-Werror,-Wvisibility] > > struct argv_array *other_arg); > > Did you forward-declare 'argv_array' in range-diff.h? That is, rather than: > > #include "argv-array.h" > > instead say: > > struct argv_array; *facepalm* Damn, sorry for the brainfart. I completely forgot to do that. That being said, I'd prefer to just include the header for consistency since we already have the #include "diff.h" up there. Or alternatively, I could write a patch to change both into forward-declarations. Personally, I prefer to avoid forward-declarations whenever possible because it adds duplication. If we have a strong preference for one or the other, should we include it in the CodingGuidelines? Thanks, Denton > > which tells the compiler that the type exists, thus allowing you to > deal with pointers to the struct, as long as you don't try to access > any of the struct's members in code which has seen only the forward > declaration. You would still need to #include "argv-array.h" in > range-diff.c, though.
diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt index 8a6ea2c6c5..a3d69713f7 100644 --- a/Documentation/git-range-diff.txt +++ b/Documentation/git-range-diff.txt @@ -57,6 +57,10 @@ to revert to color all lines according to the outer diff markers See the ``Algorithm`` section below for an explanation why this is needed. +--[no-]notes[=<treeish>]:: + This flag is passed to the `git log` program + (see linkgit:git-log[1]) that generates the patches. + <range1> <range2>:: Compare the commits specified by the two ranges, where `<range1>` is considered an older version of `<range2>`. @@ -75,7 +79,7 @@ to revert to color all lines according to the outer diff markers linkgit:git-diff[1]), most notably the `--color=[<when>]` and `--no-color` options. These options are used when generating the "diff between patches", i.e. to compare the author, commit message and diff of -corresponding old/new commits. There is currently no means to tweak the +corresponding old/new commits. There is currently no means to tweak most of the diff options passed to `git log` when generating those patches. OUTPUT STABILITY diff --git a/builtin/log.c b/builtin/log.c index a26f223ab4..047ac4594d 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1189,7 +1189,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout, diff_setup_done(&opts); fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title); show_range_diff(rev->rdiff1, rev->rdiff2, - rev->creation_factor, 1, &opts); + rev->creation_factor, 1, &opts, NULL); } } diff --git a/builtin/range-diff.c b/builtin/range-diff.c index 9202e75544..98acf3533e 100644 --- a/builtin/range-diff.c +++ b/builtin/range-diff.c @@ -15,12 +15,16 @@ int 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 argv_array other_arg = ARGV_ARRAY_INIT; int simple_color = -1; struct option range_diff_options[] = { OPT_INTEGER(0, "creation-factor", &creation_factor, N_("Percentage by which creation is weighted")), 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_END() }; struct option *options; @@ -78,7 +82,7 @@ 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); + simple_color < 1, &diffopt, &other_arg); strbuf_release(&range1); strbuf_release(&range2); diff --git a/log-tree.c b/log-tree.c index 923a299e70..151e12f415 100644 --- a/log-tree.c +++ b/log-tree.c @@ -770,7 +770,7 @@ void show_log(struct rev_info *opt) opts.use_color = opt->diffopt.use_color; diff_setup_done(&opts); show_range_diff(opt->rdiff1, opt->rdiff2, - opt->creation_factor, 1, &opts); + opt->creation_factor, 1, &opts, NULL); memcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff)); } diff --git a/range-diff.c b/range-diff.c index 623397221d..f56b4012a2 100644 --- a/range-diff.c +++ b/range-diff.c @@ -40,7 +40,8 @@ static size_t find_end_of_line(char *buffer, unsigned long size) * Reads the patches into a string list, with the `util` field being populated * as struct object_id (will need to be free()d). */ -static int read_patches(const char *range, struct string_list *list) +static int read_patches(const char *range, struct string_list *list, + struct argv_array *other_arg) { struct child_process cp = CHILD_PROCESS_INIT; struct strbuf buf = STRBUF_INIT, contents = STRBUF_INIT; @@ -61,8 +62,11 @@ static int read_patches(const char *range, struct string_list *list) "--output-indicator-new=>", "--output-indicator-old=<", "--output-indicator-context=#", - "--no-abbrev-commit", range, + "--no-abbrev-commit", NULL); + if (other_arg) + argv_array_pushv(&cp.args, other_arg->argv); + argv_array_push(&cp.args, range); cp.out = -1; cp.no_stdin = 1; cp.git_cmd = 1; @@ -502,16 +506,17 @@ 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, - struct diff_options *diffopt) + struct diff_options *diffopt, + struct argv_array *other_arg) { int res = 0; struct string_list branch1 = STRING_LIST_INIT_DUP; struct string_list branch2 = STRING_LIST_INIT_DUP; - if (read_patches(range1, &branch1)) + if (read_patches(range1, &branch1, other_arg)) res = error(_("could not parse log for '%s'"), range1); - if (!res && read_patches(range2, &branch2)) + if (!res && read_patches(range2, &branch2, other_arg)) res = error(_("could not parse log for '%s'"), range2); if (!res) { diff --git a/range-diff.h b/range-diff.h index 08a50b6e98..7d918ab9ed 100644 --- a/range-diff.h +++ b/range-diff.h @@ -2,6 +2,7 @@ #define RANGE_DIFF_H #include "diff.h" +#include "argv-array.h" #define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60 @@ -12,6 +13,7 @@ */ int show_range_diff(const char *range1, const char *range2, int creation_factor, int dual_color, - struct diff_options *diffopt); + struct diff_options *diffopt, + struct argv_array *other_arg); #endif diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh index 5bb33efdff..cea1846282 100755 --- a/t/t3206-range-diff.sh +++ b/t/t3206-range-diff.sh @@ -529,6 +529,53 @@ test_expect_success 'range-diff compares notes by default' ' test_cmp expect actual ' +test_expect_success 'range-diff with --no-notes' ' + git notes add -m "topic note" topic && + git notes add -m "unmodified note" unmodified && + test_when_finished git notes remove topic unmodified && + git range-diff --no-color --no-notes master..topic master..unmodified \ + >actual && + cat >expect <<-EOF && + 1: $(test_oid t1) = 1: $(test_oid u1) s/5/A/ + 2: $(test_oid t2) = 2: $(test_oid u2) s/4/A/ + 3: $(test_oid t3) = 3: $(test_oid u3) s/11/B/ + 4: $(test_oid t4) = 4: $(test_oid u4) s/12/B/ + EOF + test_cmp expect actual +' + +test_expect_success 'range-diff with multiple --notes' ' + git notes --ref=note1 add -m "topic note1" topic && + git notes --ref=note1 add -m "unmodified note1" unmodified && + test_when_finished git notes --ref=note1 remove topic unmodified && + git notes --ref=note2 add -m "topic note2" topic && + git notes --ref=note2 add -m "unmodified note2" unmodified && + test_when_finished git notes --ref=note2 remove topic unmodified && + git range-diff --no-color --notes=note1 --notes=note2 master..topic master..unmodified \ + >actual && + sed s/Z/\ /g >expect <<-EOF && + 1: $(test_oid t1) = 1: $(test_oid u1) s/5/A/ + 2: $(test_oid t2) = 2: $(test_oid u2) s/4/A/ + 3: $(test_oid t3) = 3: $(test_oid u3) s/11/B/ + 4: $(test_oid t4) ! 4: $(test_oid u4) s/12/B/ + @@ Commit message + Z + Z + Z ## Notes (note1) ## + - topic note1 + + unmodified note1 + Z + Z + Z ## Notes (note2) ## + - topic note2 + + unmodified note2 + Z + Z ## file ## + Z@@ file: A + EOF + test_cmp expect actual +' + test_expect_success 'format-patch --range-diff compares notes by default' ' git notes add -m "topic note" topic && git notes add -m "unmodified note" unmodified &&
When a commit being range-diff'd has a note attached to it, the note will be compared as well. However, if a user has multiple notes refs or if they want to suppress notes from being printed, there is currently no way to do this. Passthrough `---no--notes` to the `git log` call so that this option is customizable. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- Documentation/git-range-diff.txt | 6 +++- builtin/log.c | 2 +- builtin/range-diff.c | 6 +++- log-tree.c | 2 +- range-diff.c | 15 ++++++---- range-diff.h | 4 ++- t/t3206-range-diff.sh | 47 ++++++++++++++++++++++++++++++++ 7 files changed, 72 insertions(+), 10 deletions(-)