git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Eric Sunshine <sunshine@sunshineco.com>
To: Isabella Stephens <istephens@atlassian.com>
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"SZEDER Gábor" <szeder.dev@gmail.com>,
	"Git List" <git@vger.kernel.org>, "Jeff King" <peff@peff.net>,
	"Bryan Turner" <bturner@atlassian.com>,
	"Jacob Keller" <jacob.keller@gmail.com>
Subject: Re: [PATCH] blame: prevent error if range ends past end of file
Date: Wed, 30 May 2018 04:45:24 -0400	[thread overview]
Message-ID: <CAPig+cTCDP0zJonDFXyv6Kue5y4PMPaveyjfvfTVNoifzYg-Og@mail.gmail.com> (raw)
In-Reply-To: <20180529053037.38015-2-istephens@atlassian.com>

On Tue, May 29, 2018 at 1:30 AM,  <istephens@atlassian.com> wrote:
> If the -L option is used to specify a line range in git blame, and the
> end of the range is past the end of the file, git will fail with a fatal
> error. This commit prevents such behavior - instead we display the blame
> for existing lines within the specified range.

Makes sense; the new behavior is intuitive and more friendly.

> Tests and documentation are ammended accordingly.

s/ammended/amended/

> This commit also fixes two corner cases. Blaming -L n,-(n+1) now blames
> the first n lines of a file rather than from n to the end of the file.
> Blaming -L ,-n will be treated as -L 1,-n and blame the first line of
> the file, rather than blaming the whole file.
>
> Signed-off-by: Isabella Stephens <istephens@atlassian.com>
> ---
> diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt
> @@ -152,6 +152,16 @@ Also you can use a regular expression to specify the line range:
>  which limits the annotation to the body of the `hello` subroutine.
>
> +A range that begins or ends outside the bounds of the file will
> +blame the relevant lines. For example:
> +
> +       git blame -L 10,-20 foo
> +       git blame -L 10,+20 foo
> +
> +will respectively blame the first 10 and last 11 lines of a
> +20 line file. However, blaming a line range that is entirely
> +outside the bounds of the file will fail.

This documentation seems misplaced. Rather than inserting it after the
discussion of -L/regex/, a more natural place would be just above
-L/regex/ where -L<begin>,<end> is discussed.

However, I am not at all convinced that this behavior should be
documented to this level of detail. Doing so assigns too much emphasis
to what should be intuitive, thus wastes readers' time wondering why
it is so heavily emphasized. At _most_, I would think you could say
merely:

    A range that begins or ends outside the bounds of the file
    will be clipped to the file's extent.

and drop the example and discussion of the example results altogether.

In fact, because this new behavior is what most users will intuitively
expect, it might be perfectly reasonable to not say anything about it
at all (that is, don't modify git-blame.txt).

Thanks.

  reply	other threads:[~2018-05-30  8:45 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-10-26  2:19 [PATCH] blame: add --fuzzy-lines command line option Isabella Stephens
2017-10-26  6:15 ` Junio C Hamano
2017-10-26  7:01   ` [PATCH v2] blame: prevent error if range ends past end of file Isabella Stephens
2017-10-26  8:48     ` Jacob Keller
2017-10-26 22:50       ` Isabella Stephens
2017-10-26 15:31     ` SZEDER Gábor
2017-10-26 23:37       ` Isabella Stephens
2017-10-27  0:56       ` [PATCH v3] " Isabella Stephens
2017-10-27  1:58         ` Junio C Hamano
2017-10-27  2:01           ` Junio C Hamano
2017-10-27  6:18           ` Isabella Stephens
2017-10-27  6:56             ` Eric Sunshine
2018-04-26  7:45               ` [PATCH v4 0/2] blame and log: " istephens
2018-04-26  7:45                 ` [PATCH v4 1/2] blame: " istephens
2018-04-27  0:50                   ` Junio C Hamano
2018-04-27  1:42                     ` Isabella Stephens
2018-04-27  2:09                       ` Junio C Hamano
2018-04-27  4:15                         ` Isabella Stephens
2018-05-02  2:47                           ` Junio C Hamano
2018-05-29  5:30                             ` [PATCH v5 0/2] blame and log: " istephens
2018-05-29  5:30                               ` [PATCH] blame: " istephens
2018-05-30  8:45                                 ` Eric Sunshine [this message]
2018-05-31  5:07                                   ` Isabella Stephens
2018-05-29  5:30                               ` [PATCH] log: prevent error if line " istephens
2018-05-30  8:59                                 ` Eric Sunshine
2018-04-26  7:45                 ` [PATCH v4 2/2] " istephens
2017-10-26  7:08   ` [PATCH] blame: add --fuzzy-lines command line option Isabella Stephens

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=CAPig+cTCDP0zJonDFXyv6Kue5y4PMPaveyjfvfTVNoifzYg-Og@mail.gmail.com \
    --to=sunshine@sunshineco.com \
    --cc=bturner@atlassian.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=istephens@atlassian.com \
    --cc=jacob.keller@gmail.com \
    --cc=peff@peff.net \
    --cc=szeder.dev@gmail.com \
    /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).