From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-Status: No, score=-3.7 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_PASS, SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by dcvr.yhbt.net (Postfix) with ESMTP id 0C0431F66E for ; Mon, 17 Aug 2020 22:38:11 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729566AbgHQWiG (ORCPT ); Mon, 17 Aug 2020 18:38:06 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:44296 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729098AbgHQWiE (ORCPT ); Mon, 17 Aug 2020 18:38:04 -0400 Received: from mail-ua1-x944.google.com (mail-ua1-x944.google.com [IPv6:2607:f8b0:4864:20::944]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6F8CDC061389 for ; Mon, 17 Aug 2020 15:38:04 -0700 (PDT) Received: by mail-ua1-x944.google.com with SMTP id v20so5232527ual.4 for ; Mon, 17 Aug 2020 15:38:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=alPg/3pdwTggWJYbhdxHQJiToOziu9y52iEPf0l6ylE=; b=PEZmrCMC0c9/htDmFIMEbT3qZ4id4eiP2iKdKsy7JdVXjd2fm8ECQM/rccZhjdc+Sz OxMLzJ+rPSxPB3K5lDo3NaDvZ872FuEyoYKMZc0fXU8EzQS3dxkfAynR+iwS8/RM0z17 myggELaLR6YuywmzDOGbeRC9+/n/8FO2aafvBRVOeJmB9pq3PF9+4OnX4OXcgkUDqbwE LUUTi2EX976lfTR7bXxsbB17+U8L2Wribgb9GmS+b172VDjw3Obij25VOqt0DliCjGRC BlNaL5iFe+51HealaPWC+bRBDT/1PKkXY6Gd7VhUIn4MU4TPUQjFsZ3QWh1jRqdV3LH3 WEMQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=alPg/3pdwTggWJYbhdxHQJiToOziu9y52iEPf0l6ylE=; b=jo5Ac0Axkla91mGQ3tRst7GmVecdjgXaEiJan5ilGQn0eLZdGLI9FvCvP3vpP6ETbt TaGfj4YVbjYvQi4Hd99lZcN9jLVo6jnkipsSLPygfM5k4Tas6Ur01PZfmlHo2JJbGknn SaFxxTfQYQTJzhh91c/I3SnUkXXbBdkjK2gucoJynHOiJF+gDYxtsxtDWF4IGze//cwg BF6yroYCk6QASJ21uPGZK+vElhROexBSGvK1JaXtFpYzLpvO7u5PXnriJtLV+2i+F4Xi AA0Q1q5hR1X8A8BjYZofOvIIl8eFqBkKZC8tWHUeVBVPEEBKjeA1rq095Vn4Rt3VVEqo ky4g== X-Gm-Message-State: AOAM531AJMrIKsajzwDiK3loWGuqRpBKj/mpo4QyscW+xO0DhMDlOTwC pu9vNcFVXnnR4880QqkCRMAE+TW8pD/g+uMHLjo= X-Google-Smtp-Source: ABdhPJzg33TfMvO/Fjh8JYmW37QCxYCdGgkLhL35pXgvI/z5HyN37NnUm/A3YXWngPgOWwyHSt9W02sdA/j2RitVkmY= X-Received: by 2002:ab0:2b8b:: with SMTP id q11mr9032337uar.80.1597703883662; Mon, 17 Aug 2020 15:38:03 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Matt Rogers Date: Mon, 17 Aug 2020 18:37:54 -0400 Message-ID: Subject: Re: [PATCH v2] diff: teach --stat to ignore uninteresting modifications To: Junio C Hamano Cc: Matthew Rogers via GitGitGadget , Git Mailing List , Jeff King Content-Type: text/plain; charset="UTF-8" Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org ...As well as updating the comment. (sorry for the double mail I fat fingered ctrl+enter while typing) On Mon, Aug 17, 2020 at 6:37 PM Matt Rogers wrote: > > On Mon, Aug 17, 2020 at 5:39 PM Junio C Hamano wrote: > > > > "Matthew Rogers via GitGitGadget" writes: > > > > > From: Matthew Rogers > > > > > > Sometimes when diffing, files may show as being momdified even when > > > > momdified? mummified? ah, modified. > > > > > there are no interesting diffs to show. This happens naturally when > > > using options such as --ignore-space-change. > > > > Read the next paragraph and notice that it explains the cases where > > the patch does not want not to show, and then read the above again > > to realize that the above does not say anything about what it wants > > to do to cases the next paragraph does not cover. It only says such > > a case often happens when --ignore-space-change is used. > > > > When options like --ignore-space-change is in use, files > > with modification can have no interesting textual changes > > worth showing. In such cases, "git diff --stat" shows 0 > > lines of additions and deletions. Teach "git diff --stat" > > not to show such a path in its output, which would be more > > natural. > > > > perhaps? > > > > > We don't want to prevent > > > the display of all files that have 0 effective diffs since they could > > > be the result of a rename, permission change, or other similar operation > > > that may still be of interest so we special case additions and deletions > > > as they are always interesting. > > > > Yup. That makes sense. > > I'll send a reroll with the message improved as you suggested, as well > as updating > > > > > It would be nice if this does not have to be implemented as a list > > of exceptions, though. Rather, a more targetted "omit output only > > in this narrow case" would be nicer, but the check with the mode > > bits should do at lesat for now. > > > > > diff --git a/diff.c b/diff.c > > > index f9709de7b4..131903fa3a 100644 > > > --- a/diff.c > > > +++ b/diff.c > > > @@ -3153,16 +3153,19 @@ static void show_dirstat_by_line(struct diffstat_t *data, struct diff_options *o > > > gather_dirstat(options, &dir, changed, "", 0); > > > } > > > > > > +static void free_diffstat_file(struct diffstat_file *f) > > > +{ > > > + free(f->print_name); > > > + free(f->name); > > > + free(f->from_name); > > > + free(f); > > > +} > > > + > > > void free_diffstat_info(struct diffstat_t *diffstat) > > > { > > > int i; > > > - for (i = 0; i < diffstat->nr; i++) { > > > - struct diffstat_file *f = diffstat->files[i]; > > > - free(f->print_name); > > > - free(f->name); > > > - free(f->from_name); > > > - free(f); > > > - } > > > + for (i = 0; i < diffstat->nr; i++) > > > + free_diffstat_file(diffstat->files[i]); > > > free(diffstat->files); > > > } > > > > > > @@ -3718,6 +3721,26 @@ static void builtin_diffstat(const char *name_a, const char *name_b, > > > if (xdi_diff_outf(&mf1, &mf2, discard_hunk_line, > > > diffstat_consume, diffstat, &xpp, &xecfg)) > > > die("unable to generate diffstat for %s", one->path); > > > + > > > + if (DIFF_FILE_VALID(one) && DIFF_FILE_VALID(two)) { > > > + struct diffstat_file *file = > > > + diffstat->files[diffstat->nr - 1]; > > > + /* > > > + * Omit diffstats of modified files where nothing changed. > > > + * Even if !same_contents, this might be the case due to > > > + * ignoring whitespace changes, etc. > > > + * > > > + * But note that we special-case additions and deletions, > > > > * renames and mode changes without any content changes, > > > > > + * as adding an empty file, for example is still of interest. > > > + */ > > > + if ((p->status == DIFF_STATUS_MODIFIED) > > > + && !file->added > > > + && !file->deleted > > > + && one->mode == two->mode) { > > > + free_diffstat_file(file); > > > + diffstat->nr--; > > > + } > > > + } > > > } > > > > > > diff_free_filespec_data(one); > > > diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh > > > index 88d3026894..8bdaa0a693 100755 > > > --- a/t/t4015-diff-whitespace.sh > > > +++ b/t/t4015-diff-whitespace.sh > > > @@ -789,7 +789,7 @@ test_expect_success 'checkdiff allows new blank lines' ' > > > git diff --check > > > ' > > > > > > -test_expect_success 'whitespace-only changes not reported' ' > > > +test_expect_success 'whitespace-only changes not reported (diff)' ' > > > git reset --hard && > > > echo >x "hello world" && > > > git add x && > > > @@ -799,10 +799,44 @@ test_expect_success 'whitespace-only changes not reported' ' > > > test_must_be_empty actual > > > ' > > > > > > -test_expect_success 'whitespace-only changes reported across renames' ' > > > +test_expect_success 'whitespace-only changes not reported (diffstat)' ' > > > + # reuse state from previous test > > > + git diff --stat -b >actual && > > > + test_must_be_empty actual > > > +' > > > + > > > +test_expect_success 'whitespace changes with modification reported (diffstat)' ' > > > + git reset --hard && > > > + echo >x "hello world" && > > > + git update-index --chmod=+x x && > > > + git diff --stat --cached -b >actual && > > > + cat <<-EOF >expect && > > > + x | 0 > > > + 1 file changed, 0 insertions(+), 0 deletions(-) > > > + EOF > > > + test_cmp expect actual > > > +' > > > + > > > +test_expect_success 'whitespace-only changes reported across renames (diffstat)' ' > > > git reset --hard && > > > for i in 1 2 3 4 5 6 7 8 9; do echo "$i$i$i$i$i$i"; done >x && > > > git add x && > > > + git commit -m "base" && > > > + sed -e "5s/^/ /" x >z && > > > + git rm x && > > > + git add z && > > > + git diff -w -M --cached --stat >actual && > > > + cat <<-EOF >expect && > > > + x => z | 0 > > > + 1 file changed, 0 insertions(+), 0 deletions(-) > > > + EOF > > > + test_cmp expect actual > > > +' > > > + > > > +test_expect_success 'whitespace-only changes reported across renames' ' > > > + git reset --hard HEAD~1 && > > > + for i in 1 2 3 4 5 6 7 8 9; do echo "$i$i$i$i$i$i"; done >x && > > > + git add x && > > > hash_x=$(git hash-object x) && > > > before=$(git rev-parse --short "$hash_x") && > > > git commit -m "base" && > > > > > > base-commit: 878e727637ec5815ccb3301eb994a54df95b21b8 > > > > -- > Matthew Rogers -- Matthew Rogers