git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Taylor Blau <me@ttaylorr.com>
Cc: Thomas Guyot-Sionnest <tguyot@gmail.com>,
	git@vger.kernel.org, dermoth@aei.ca, peff@peff.net
Subject: Re: [PATCH v2] diff: Fix modified lines stats with --stat and --numstat
Date: Sun, 20 Sep 2020 15:15:28 -0700	[thread overview]
Message-ID: <xmqq8sd4gkn3.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <xmqqd02ggp84.fsf@gitster.c.googlers.com> (Junio C. Hamano's message of "Sun, 20 Sep 2020 13:36:27 -0700")

Junio C Hamano <gitster@pobox.com> writes:

Sorry, this will be the last message from me on this topic for now.

> we'd probably need to factor out the condition used in the above
> into a helper function, e.g.
>
>     static int cannot_be_the_same(struct diff_filespec *one, struct diff_filespec *two)

The naming of this helper is tricky.  In both potential callers,
what we want to see is "one and two may be different, we cannot say
they are the same with certainty", so "cannot be the same" is a
misnomer.  Worse, the negated form is hard to grok.

Perhaps "may_differ()" is a more correct name.  If either side is
NULL oid, we cannot say they are the same, so it is true.  If two
oid that are not NULL oid are the same, there is no possibility that
they are different, so we return false.  And two oid that are not
NULL oid are different, we know they are different, so we return
true.

>     {
> 	if ((oideq(&one->oid, &null_oid) || oideq(&two->oid, &null_oid))
> 		return 1;
> 	else if (oideq(&one->oid, &two->oid))
> 		return 0;
> 	else
> 		return 1;
>     }
>
> and rewrite the conditional in fill_metainfo() to
>
> 	if (one && two && cannot_be_the_same(one, two)) {
> 		...

And this becomes much easier to read and understand, i.e.

	if (one && two && may_differ(one, two)) {
		... create the index one->oid..two->oid header ...

> The "second best fix" could then become a single liner:
>
> 	same_contents = !cannot_be_the_same(one, two);
>
> using the helper.

And this becomes

	same_contents = !may_differ(one, two);

meaning that "there is no possibility that one and two are
different".  That allows us to optimize out the invocation of the
xdl machinery.


  reply	other threads:[~2020-09-20 22:16 UTC|newest]

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-18 11:32 Allow passing pipes to diff --no-index + bugfix Thomas Guyot-Sionnest
2020-09-18 11:32 ` [PATCH 1/2] diff: Fix modified lines stats with --stat and --numstat Thomas Guyot-Sionnest
2020-09-18 14:46   ` Taylor Blau
2020-09-18 15:10     ` Thomas Guyot-Sionnest
2020-09-18 17:37       ` Jeff King
2020-09-18 18:00         ` Thomas Guyot-Sionnest
2020-09-20  4:53       ` Thomas Guyot
2020-09-18 17:27   ` Jeff King
2020-09-18 17:52     ` Thomas Guyot-Sionnest
2020-09-18 18:06       ` Junio C Hamano
2020-09-23 19:16         ` Johannes Schindelin
2020-09-23 19:23           ` Junio C Hamano
2020-09-23 20:44             ` Johannes Schindelin
2020-09-24  4:49               ` Thomas Guyot
2020-09-24  5:24                 ` [PATCH v3] " Thomas Guyot-Sionnest
2020-09-24  7:41                   ` [PATCH v4] " Thomas Guyot-Sionnest
2020-09-24  6:40                 ` [PATCH 1/2] " Junio C Hamano
2020-09-24  7:13                   ` Thomas Guyot
2020-09-24 17:19                     ` Junio C Hamano
2020-09-24 17:38                       ` Junio C Hamano
2020-09-23 15:05     ` Johannes Schindelin
2020-09-20 13:09   ` [PATCH v2] " Thomas Guyot-Sionnest
2020-09-20 15:39     ` Taylor Blau
2020-09-20 16:38       ` Thomas Guyot
2020-09-20 19:11       ` Junio C Hamano
2020-09-20 20:08         ` Junio C Hamano
2020-09-20 20:36         ` Junio C Hamano
2020-09-20 22:15           ` Junio C Hamano [this message]
2020-09-21 19:26         ` Jeff King
2020-09-21 21:51           ` Junio C Hamano
2020-09-21 22:20             ` Jeff King
2020-09-21 22:37               ` Junio C Hamano
2020-09-18 11:32 ` [PATCH 2/2] Allow passing pipes for input pipes to diff --no-index Thomas Guyot-Sionnest
2020-09-18 14:36   ` Taylor Blau
2020-09-18 16:34     ` Thomas Guyot-Sionnest
2020-09-18 17:19       ` Jeff King
2020-09-18 17:21         ` Jeff King
2020-09-18 17:39         ` Thomas Guyot-Sionnest
2020-09-18 17:48         ` Junio C Hamano
2020-09-18 18:02           ` Jeff King
2020-09-20 12:54             ` Thomas Guyot
2020-09-21 19:31               ` Jeff King
2020-09-21 20:14                 ` Junio C Hamano
2020-09-18 17:58       ` Taylor Blau
2020-09-18 18:05         ` Jeff King
2020-09-18 17:20     ` Jeff King
2020-09-18 18:00       ` Taylor Blau
2020-09-18 21:56   ` brian m. carlson
2020-09-18 17:51 ` Allow passing pipes to diff --no-index + bugfix Junio C Hamano
2020-09-18 18:24   ` Thomas Guyot-Sionnest

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=xmqq8sd4gkn3.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=dermoth@aei.ca \
    --cc=git@vger.kernel.org \
    --cc=me@ttaylorr.com \
    --cc=peff@peff.net \
    --cc=tguyot@gmail.com \
    --subject='Re: [PATCH v2] diff: Fix modified lines stats with --stat and --numstat' \
    /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

Code repositories for project(s) associated with this 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).