From: Michael J Gruber <git@drmicha•warpmail.net>
To: Junio C Hamano <gitster@pobox•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCHv3 1/3] diff.c: omit hidden entries from namelen calculation with --stat
Date: Fri, 27 May 2011 22:19:33 +0200 [thread overview]
Message-ID: <4DE00755.1020404@drmicha.warpmail.net> (raw)
In-Reply-To: <7vvcwwrqd7.fsf@alter.siamese.dyndns.org>
Junio C Hamano venit, vidit, dixit 27.05.2011 19:43:
> Michael J Gruber <git@drmicha•warpmail.net> writes:
>
>> Currently, --stat calculates the longest name from all items but then
>> drops some (mode changes) from the output later on.
>>
>> Instead, drop them from the namelen generation and calculation.
>>
>> Signed-off-by: Michael J Gruber <git@drmicha•warpmail.net>
>> ---
>> This optimizes (tightens) the display potentially, but we never had tests
>> which are sensitive to that.
>
> More importantly I think this is a good first step in the right direction
> to reduce the duplicated code that need to implement the same logic.
>
> However, you missed another instance in show_shortstats() no?
I'm sorry I don't understand this remark. For all different formats, the
respective functions show_stat(), show_shortstats() etc. do their own
formatting, and I did not change this at all. So, what did I miss?
Also, show_shortstats() does not need the names, so what similar change
should one have made there?
> I am wondering if it would make it easier to both read and maintain if you
> add a boolean field "changed" to "struct diffstat_file", and set it at the
> end of builtin_diffstat(), and have these places all check that field.
Sounds fine.
> A possible alternative is not to put such an unchanged entry in the struct
> diffstat_t in the first place so that nobody has to worry about it. Right
> now, show_numstat() does show 0/0 entries (i.e. only mode change), while
> shortstat and normal stat do not, but I somehow have a feeling that this
> difference is not by design but by accident. Besides, --numstat that only
> says 0/0 is not useful in practice without --summary.
>
> $ edit diff.c
> $ chmod +x Makefile
> $ git diff --stat
> diff.c | 26 ++++++++++++++------------
> $ git diff --numstat
> 0 0 Makefile
> 14 12 diff.c
> $ git diff --numstat --summary
> 0 0 Makefile
> 14 12 diff.c
> mode change 100644 => 100755 Makefile
>
> Getting rid of an unchanged entry from the diffstat array upfront will
> change the behaviour of numstat, but I suspect that it change things in a
> better way, making the result consistent with the textual version.
That sounds good to me.
> The patch below, diff.c shows that approach, while ndiff.c shows the extra
> boolean "changed" approach.
>
> What do you think?
>
> diff.c | 7 +++++++
> diff.c => ndiff.c | 26 ++++++++++++++------------
> 2 files changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index 8f4815b..e2ee70a 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -2267,6 +2267,13 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
> &xpp, &xecfg);
> }
>
> + if (!data->is_renamed && !data->added && !data->deleted) {
> + diffstat->nr--;
> + free(data->name);
> + free(data->from_name);
> + free(data);
> + }
> +
I don't know that codepath very well, but hasn't that diffstat been
added earlier in builtin_diffstat() already? But maybe the
diffstat->nr-- takes care of that.
Also, I have no idea how builtin_diffstat() influences show_stat() which
my patch was about, sorry.
> diff_free_filespec_data(one);
> diff_free_filespec_data(two);
> }
> diff --git a/diff.c b/ndiff.c
> similarity index 99%
> copy from diff.c
> copy to ndiff.c
> index 8f4815b..1620053 100644
> --- a/diff.c
> +++ b/ndiff.c
> @@ -1222,6 +1222,7 @@ struct diffstat_t {
> unsigned is_unmerged:1;
> unsigned is_binary:1;
> unsigned is_renamed:1;
> + unsigned changed:1;
> uintmax_t added, deleted;
> } **files;
> };
> @@ -1350,6 +1351,9 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
> for (i = 0; i < data->nr; i++) {
> struct diffstat_file *file = data->files[i];
> uintmax_t change = file->added + file->deleted;
> +
> + if (!file->changed)
> + continue;
> fill_print_name(file);
> len = strlen(file->print_name);
> if (max_len < len)
> @@ -1381,6 +1385,11 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
> uintmax_t deleted = data->files[i]->deleted;
> int name_len;
>
> + if (!data->files[i]->changed) {
> + total_files--;
> + continue;
> + }
> +
> /*
> * "scale" the filename
> */
> @@ -1415,11 +1424,6 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
> fprintf(options->file, " Unmerged\n");
> continue;
> }
> - else if (!data->files[i]->is_renamed &&
> - (added + deleted == 0)) {
> - total_files--;
> - continue;
> - }
or do this dance only here (with ...->changed) instead of in the first
loop, because in 2/3 (when limited by stat_count), the second loop would
be done all the way up to nr (as 2 separate loops), but the first one
only up to count.
>
> /*
> * scale the add/delete
> @@ -1457,15 +1461,12 @@ static void show_shortstats(struct diffstat_t *data, struct diff_options *option
> for (i = 0; i < data->nr; i++) {
> if (!data->files[i]->is_binary &&
> !data->files[i]->is_unmerged) {
> - int added = data->files[i]->added;
> - int deleted= data->files[i]->deleted;
> - if (!data->files[i]->is_renamed &&
> - (added + deleted == 0)) {
> + if (!data->files[i]->changed) {
> total_files--;
> - } else {
> - adds += added;
> - dels += deleted;
> + continue;
> }
> + adds += data->files[i]->added;
> + dels += data->files[i]->deleted;
> }
> }
> if (options->output_prefix) {
> @@ -2266,6 +2267,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
> xdi_diff_outf(&mf1, &mf2, diffstat_consume, diffstat,
> &xpp, &xecfg);
> }
> + data->changed = data->is_renamed || data->added || data->deleted;
>
> diff_free_filespec_data(one);
> diff_free_filespec_data(two);
Does show_stats() get the data as modified by builtin_diffstat()? Then
this is fine.
Again, I'm sorry I'm not seeing through the call chain here.
Michael
next prev parent reply other threads:[~2011-05-27 20:19 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-04-29 14:57 [PATCH 1/2] diff: introduce --stat-lines to limit the stat lines Michael J Gruber
2011-04-29 14:57 ` [PATCH 2/2] diff-options.txt: describe --stat-{width,name-width,lines} Michael J Gruber
2011-04-29 15:12 ` [PATCH 1/2] diff: introduce --stat-lines to limit the stat lines Michael J Gruber
2011-04-29 16:15 ` Junio C Hamano
2011-05-01 8:27 ` Michael J Gruber
2011-05-01 18:33 ` Junio C Hamano
2011-05-03 10:46 ` [PATCHv2 " Michael J Gruber
2011-05-03 10:46 ` [PATCHv2 2/2] diff-options.txt: describe --stat-{width,name-width,count} Michael J Gruber
2011-05-03 18:47 ` [PATCHv2 1/2] diff: introduce --stat-lines to limit the stat lines Junio C Hamano
2011-05-04 7:16 ` Michael J Gruber
2011-05-27 12:36 ` [PATCHv3 0/3] diff.c: --stat-count=<n> Michael J Gruber
2011-05-27 12:36 ` [PATCHv3 1/3] diff.c: omit hidden entries from namelen calculation with --stat Michael J Gruber
2011-05-27 17:43 ` Junio C Hamano
2011-05-27 20:19 ` Michael J Gruber [this message]
2011-05-27 22:45 ` Junio C Hamano
2011-05-27 12:36 ` [PATCHv3 2/3] diff: introduce --stat-lines to limit the stat lines Michael J Gruber
2011-05-28 4:46 ` Junio C Hamano
2011-05-30 6:40 ` Michael J Gruber
2011-05-30 7:36 ` Junio C Hamano
2011-05-27 12:36 ` [PATCHv3 3/3] diff-options.txt: describe --stat-{width,name-width,count} Michael J Gruber
2011-05-04 4:37 ` [PATCHv2 1/2] diff: introduce --stat-lines to limit the stat lines Junio C Hamano
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4DE00755.1020404@drmicha.warpmail.net \
--to=git@drmicha$(echo .)warpmail.net \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox