From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Barret Rhoden <brho@google.com>
Cc: git@vger.kernel.org, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
"David Kastrup" <dak@gnu.org>, "Jeff King" <peff@peff.net>,
"Jeff Smith" <whydoubt@gmail.com>,
"Junio C Hamano" <gitster@pobox.com>,
"René Scharfe" <l.s.r@web.de>,
"Stefan Beller" <stefanbeller@gmail.com>
Subject: Re: [PATCH v2 2/3] blame: add the ability to ignore commits and their changes
Date: Fri, 18 Jan 2019 10:47:36 +0100 (STD) [thread overview]
Message-ID: <nycvar.QRO.7.76.6.1901181038540.41@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20190117202919.157326-3-brho@google.com>
Hi Barret,
On Thu, 17 Jan 2019, Barret Rhoden wrote:
> [...]
>
> Users can ignore a revision with --ignore-rev=rev, which may be
> repeated. They can specify a file of full SHA-1 hashes of revs (one per
> line) with the blame.ignoreRevFile config option. They can also specify
> a file with --ignore-rev-file=file, which overrides the config option.
This sounds like that is already the case in Git, but the truth is: this
patch *introduces* that feature. Maybe start the paragraph with "With this
patch, ..."?
I cannot speak for the correctness of the changes to blame.c, others on
the Cc: list are already much more familiar with that code, so I'll leave
it to them to comment on that.
However, I am missing a regression test for this behavior, the best idea
would be likely to add t/t8013-blame-ignore-revs.sh (copy-edit it from
t/t8009-blame-vs-topicbranches.sh, maybe).
And there is another thing:
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 6d798f99392e..2f9183fb5fbd 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -52,6 +52,7 @@ static int no_whole_file_rename;
> static int show_progress;
> static char repeated_meta_color[COLOR_MAXLEN];
> static int coloring_mode;
> +static const char *ignore_revs_file;
... this...
>
> static struct date_mode blame_date_mode = { DATE_ISO8601 };
> static size_t blame_date_width;
> @@ -695,6 +696,8 @@ static int git_blame_config(const char *var, const char *value, void *cb)
> parse_date_format(value, &blame_date_mode);
> return 0;
> }
> + if (!strcmp(var, "blame.ignorerevsfile"))
> + return git_config_pathname(&ignore_revs_file, var, value);
... this...
> if (!strcmp(var, "color.blame.repeatedlines")) {
> if (color_parse_mem(value, strlen(value), repeated_meta_color))
> warning(_("invalid color '%s' in color.blame.repeatedLines"),
> @@ -806,6 +826,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
> OPT_BIT('s', NULL, &output_option, N_("Suppress author name and timestamp (Default: off)"), OUTPUT_NO_AUTHOR),
> OPT_BIT('e', "show-email", &output_option, N_("Show author email instead of name (Default: off)"), OUTPUT_SHOW_EMAIL),
> OPT_BIT('w', NULL, &xdl_opts, N_("Ignore whitespace differences"), XDF_IGNORE_WHITESPACE),
> + OPT_STRING_LIST(0, "ignore-rev", &ignore_rev_list, N_("rev"), N_("Ignore <rev> when blaming")),
> + OPT_STRING(0, "ignore-revs-file", &ignore_revs_file, N_("file"), N_("Ignore revisions from <file>")),
... and this change limit the user to specifying a single file, for no
good reason. Worse: specifying two different files via two
`--ignore-revs-file` parameters will only heed the latter and skip the
former without any warning.
A better idea IMHO would be to use an OPT_STRING_LIST() for
`--ignore-revs-file`, too, and to allow for multiple
`blame.ignoreRevsFile` config entries (with our usual trick of handling an
empty setting by resetting the list of paths that were accumulated so
far, see e.g. how `credential.helper` is handled).
Ciao,
Johannes
> OPT_BIT(0, "color-lines", &output_option, N_("color redundant metadata from previous line differently"), OUTPUT_COLOR_LINE),
> OPT_BIT(0, "color-by-age", &output_option, N_("color lines by age"), OUTPUT_SHOW_AGE_WITH_COLOR),
>
> @@ -995,6 +1017,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
> sb.contents_from = contents_from;
> sb.reverse = reverse;
> sb.repo = the_repository;
> + build_ignorelist(&sb, &ignore_rev_list);
> + string_list_clear(&ignore_rev_list, 0);
> setup_scoreboard(&sb, path, &o);
> lno = sb.num_lines;
>
> --
> 2.20.1.321.g9e740568ce-goog
>
>
next prev parent reply other threads:[~2019-01-18 9:48 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-07 21:30 [PATCH] blame: add the ability to ignore commits Barret Rhoden
2019-01-07 23:13 ` Junio C Hamano
2019-01-08 16:27 ` Barret Rhoden
2019-01-08 18:26 ` Junio C Hamano
2019-01-09 20:48 ` Barret Rhoden
2019-01-10 22:29 ` Junio C Hamano
2019-01-14 15:19 ` Barret Rhoden
2019-01-14 17:45 ` Junio C Hamano
2019-01-14 18:26 ` Randall S. Becker
2019-01-14 19:03 ` Barret Rhoden
2019-01-15 6:08 ` Junio C Hamano
2019-01-09 21:21 ` Stefan Beller
2019-01-08 13:12 ` Ævar Arnfjörð Bjarmason
2019-01-08 16:41 ` Barret Rhoden
2019-01-08 18:04 ` Barret Rhoden
2019-01-17 20:29 ` [PATCH v2 0/3] " Barret Rhoden
2019-01-17 20:29 ` [PATCH v2 1/3] Move init_skiplist() outside of fsck Barret Rhoden
2019-01-18 9:25 ` Johannes Schindelin
2019-01-18 9:45 ` Ævar Arnfjörð Bjarmason
2019-01-18 17:36 ` Junio C Hamano
2019-01-18 20:59 ` Johannes Schindelin
2019-01-18 21:30 ` Jeff King
2019-01-18 22:26 ` Ævar Arnfjörð Bjarmason
2019-01-22 7:12 ` Jeff King
2019-01-22 9:46 ` Ævar Arnfjörð Bjarmason
2019-01-22 17:54 ` Junio C Hamano
2019-01-22 18:28 ` Jeff King
2019-01-17 20:29 ` [PATCH v2 2/3] blame: add the ability to ignore commits and their changes Barret Rhoden
2019-01-18 9:47 ` Johannes Schindelin [this message]
2019-01-18 17:33 ` Barret Rhoden
2019-01-20 18:19 ` René Scharfe
2019-01-22 15:26 ` Barret Rhoden
2019-01-22 18:14 ` Junio C Hamano
2019-01-22 19:35 ` Barret Rhoden
2019-01-23 19:26 ` Junio C Hamano
2019-02-01 20:37 ` Barret Rhoden
2019-01-17 20:29 ` [PATCH v2 3/3] blame: add a config option to mark ignored lines Barret Rhoden
2019-01-18 10:03 ` Johannes Schindelin
2019-01-18 9:52 ` [PATCH v2 0/3] blame: add the ability to ignore commits Ævar Arnfjörð Bjarmason
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=nycvar.QRO.7.76.6.1901181038540.41@tvgsbejvaqbjf.bet \
--to=johannes.schindelin@gmx.de \
--cc=avarab@gmail.com \
--cc=brho@google.com \
--cc=dak@gnu.org \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=l.s.r@web.de \
--cc=peff@peff.net \
--cc=stefanbeller@gmail.com \
--cc=whydoubt@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).