git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: markus.heidelberg@web.de, git@vger.kernel.org
Subject: Re: [PATCH] diff --color-words -U0: fix the location of hunk headers
Date: Fri, 30 Oct 2009 11:40:55 -0700	[thread overview]
Message-ID: <7vr5sklo7c.fsf@alter.siamese.dyndns.org> (raw)
In-Reply-To: <alpine.DEB.1.00.0910301831190.5383@felix-maschine> (Johannes Schindelin's message of "Fri\, 30 Oct 2009 18\:32\:21 +0100 \(CET\)")

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> The reason I did not do that was to avoid a full subroutine call, as I 
> expected this code path to be very expensive.

This is only done for the "word diff" mode, and my gut feeling is that it
is not such a big issue.  But you can always inline it if it is
performance critical.

The current structure shows how this code evolved.  fn_out_consume() used
to be "this is called every time a line worth of diff becomes ready from
the lower-level diff routine, and here is what we do to output line
oriented diff using that line."

When we introduced the "word diff" mode, we could have done three things.

 * change fn_out_consume() to "this is called every time a line worth of
   diff becomes ready from the lower-level diff routine.  This function
   knows two sets of helpers (one for line-oriented diff, another for word
   diff), and each set has various functions to be called at certain
   places (e.g. hunk header, context, ...).  The function's role is to
   inspect the incoming line, and dispatch appropriate helpers to produce
   either line- or word- oriented diff output."

 * introduce fn_out_consume_word_diff() that is "this is called every time
   a line worth of diff becomes ready from the lower-level diff routine,
   and here is what we do to prepare word oriented diff using that line."
   without touching fn_out_consume() at all.

 * Do neither of the above, and keep fn_out_consume() to "this is called
   every time a line worth of diff becomes ready from the lower-level diff
   routine, and here is what we do to output line oriented diff using that
   line."  but sprinkle a handful of 'are we in word-diff mode?  if so do
   this totally different thing' at random places.

My clean-up "something like this" patch was to at least abstract the
details of "this totally different thing" out from the main codepath, in
order to improve readability.

We can of course refactor this by introducing fn_out_consume_word_diff(),
i.e. the second route above.  Then we do not have to check "are we in word
diff mode" for every line of diff and that would give you even better
performance.

  reply	other threads:[~2009-10-30 18:41 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-10-28 12:24 [PATCH 0/3] fix "git diff --color-words -U0" Markus Heidelberg
2009-10-28 12:24 ` [PATCH 1/3] t4034-diff-words: add a test for word diff without context Markus Heidelberg
2009-10-28 12:24 ` [PATCH 2/3] diff: move the handling of the hunk header after the changed lines Markus Heidelberg
2009-10-28 12:24 ` [PATCH 3/3] diff: fix the location of hunk headers for "git diff --color-words -U0" Markus Heidelberg
2009-10-29 10:45   ` [PATCH] diff --color-words -U0: fix the location of hunk headers Johannes Schindelin
2009-10-29 11:22     ` Markus Heidelberg
2009-10-29 13:27       ` Johannes Schindelin
2009-10-29 16:29         ` Markus Heidelberg
2009-10-30 17:20           ` Junio C Hamano
2009-10-30 17:32             ` Johannes Schindelin
2009-10-30 18:40               ` Junio C Hamano [this message]
2009-10-31 11:48                 ` Johannes Schindelin
2009-10-28 18:14 ` [PATCH 0/3] fix "git diff --color-words -U0" Junio C Hamano
2009-10-28 22:21   ` Markus Heidelberg

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=7vr5sklo7c.fsf@alter.siamese.dyndns.org \
    --to=gitster@pobox.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=markus.heidelberg@web.de \
    /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.
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 NNTP newsgroup(s).