git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Thomas Rast <tr@thomasrast.ch>, Junio C Hamano <gitster@pobox.com>
Subject: [PATCH 0/4] line-log: be more careful when adjusting multiple line ranges
Date: Sat, 04 Aug 2018 15:18:22 -0700 (PDT)	[thread overview]
Message-ID: <pull.15.git.gitgitgadget@gmail.com> (raw)

I am a heavy user of git log -L .... In fact, I use the feature where
multiple ranges can be specified extensively, via a not-exactly-trivial
shell script function that takes the currently staged changes (or if none
are staged, the current unstanged changes) and turns them into the
corresponding commit history:

staged_log () { #
        diff="$(git diff --cached -U1)"
        test -n "$diff" ||
        diff="$(git diff -U1)"
        test -n "$diff" ||
        die "No changes"
        eval "git log $(echo "$diff" |
                sed -ne '/^--- a\//{s/^-* a\/\(.*\)/'\''\1'\''/;x}' -e \
                        '/^@@ -/{s/^@@ -\([^, ]*\),\([^ ]*\).*/-L \1,+\2/;G;s/\n/:/g;p}' |
                        tr '\n' ' ')"
}

This is an extremely useful way to look at the history, especially when
trying to fix up a commit deep in a long branch (or a thicket of branches).

Today, however, this method failed me, by greeting me with an assertion.
When I tried to paper over that assertion by joining line ranges that became
adjacent (or overlapping), it still produced a segmentation fault when the
line-log tried to print lines past the file contents.

So I had no choice but to fix this properly.

I still wanted to keep the optimization where multiple line ranges are
joined into a single one (I am convinced that this also affects the output,
where previously multiple hunks would have been displayed, but I ran out of
time to investigate this). This is the 3rd patch. It is not purely an
optimization, as the assertion would still trigger when line ranges could be
joined.

Now, I am fairly certain that the changes are correct, but given my track
record with off-by-one bugs (and once even an off-by-two bug), I would
really appreciate some thorough review of this code, in particular the
second one that is the actual bug fix. I am specifically interested in
reviews from people who know line-log.c pretty well and can tell me whether
the src[i].end > target[j].end is correct, or whether it should actually
have been a >= (I tried to wrap my head around this, but I would feel more
comfortable if a domain expert would analyze this, whistling, and looking
Eric's way).

Cc: Eric Sunshine sunshine@sunshineco.com [sunshine@sunshineco.com]

Johannes Schindelin (4):
  line-log: demonstrate a bug with nearly-overlapping ranges
  line-log: adjust start/end of ranges individually
  line-log: optimize ranges by joining them when possible
  line-log: convert an assertion to a full BUG() call

 line-log.c          | 18 +++++++++++++++---
 t/t4211-line-log.sh | 17 +++++++++++++++++
 2 files changed, 32 insertions(+), 3 deletions(-)


base-commit: 1d89318c48d233d52f1db230cf622935ac3c69fa
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-15%2Fdscho%2Fline-log-fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-15/dscho/line-log-fix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/15
-- 
gitgitgadget

             reply	other threads:[~2018-08-04 22:18 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-04 22:18 Johannes Schindelin via GitGitGadget [this message]
2018-08-04 22:18 ` [PATCH 1/4] line-log: demonstrate a bug with nearly-overlapping ranges Johannes Schindelin via GitGitGadget
2018-08-05  1:59   ` Jonathan Nieder
2018-08-06 10:27     ` Johannes Schindelin
2018-08-06 14:47       ` Jonathan Nieder
2018-08-06 15:33         ` Jonathan Nieder
2018-08-04 22:18 ` [PATCH 2/4] line-log: adjust start/end of ranges individually Johannes Schindelin via GitGitGadget
2018-08-05 10:14   ` Eric Sunshine
2018-08-05 10:57     ` Eric Sunshine
2018-08-06 12:52     ` Johannes Schindelin
2018-08-04 22:18 ` [PATCH 3/4] line-log: optimize ranges by joining them when possible Johannes Schindelin via GitGitGadget
2018-08-05  6:11   ` Junio C Hamano
2018-08-05  8:45   ` Andrei Rybak
2018-08-05 10:31     ` Eric Sunshine
2018-08-04 22:18 ` [PATCH 4/4] line-log: convert an assertion to a full BUG() call Johannes Schindelin via GitGitGadget
2018-08-05 10:42   ` Eric Sunshine
2018-08-06 13:14     ` Johannes Schindelin
2018-08-07  9:09       ` Eric Sunshine
2018-08-07 22:00         ` Eric Sunshine
2018-08-05 10:39 ` [PATCH 0/4] line-log: be more careful when adjusting multiple line ranges Eric Sunshine

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=pull.15.git.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=tr@thomasrast.ch \
    /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).