git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Adam Butcher <dev.lists@jessamine.co.uk>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] Fix 'No newline...' annotation in rewrite diffs.
Date: Thu, 2 Aug 2012 17:33:46 -0400	[thread overview]
Message-ID: <20120802213346.GA575@sigill.intra.peff.net> (raw)
In-Reply-To: <85f291cec03411c61ddf8808e53621ae@imap.force9.net>

On Thu, Aug 02, 2012 at 10:11:02PM +0100, Adam Butcher wrote:

> From 01730a741cc5fd7d0a5d8bd0d3df80d12c81fe48 Mon Sep 17 00:00:00 2001
> From: Adam Butcher <dev.lists@jessamine.co.uk>
> Date: Wed, 1 Aug 2012 22:25:09 +0100
> Subject: [PATCH] Fix 'No newline...' annotation in rewrite diffs.

You can drop these lines from the email body; they are redundant with
what's in your actual header.

> When operating in --break-rewrites (-B) mode on a file with no newline
> terminator (and assuming --break-rewrites determines that the diff
> _is_ a rewrite), git diff previously concatenated the indicator comment
> '\ No newline at end of file' directly to the terminating line rather
> than on a line of its own.  The resulting diff is broken; claiming
> that the last line actually contains the indicator text.  Without -B
> there is no problem with the same files.
> 
> This patch fixes the former case by inserting a newline into the
> output prior to emitting the indicator comment.

Makes sense.

> Potential issue: Currently this emits an ASCII 10 newline character
> only.  I'm not sure whether this will be okay on all platforms; it
> seems to work fine on Windows and GNU at least.

This should not be a problem. Git always outputs newlines; it is stdio
who might munge it into CRLF if need be (and your patch uses putc, so we
should be fine).

> A couple of tests have been added to the rewrite suite to confirm that
> the indicator comment is generated on its own line in both plain diff
> and rewrite mode.  The latter test fails if the functional part of
> this patch (i.e. diff.c) is reverted.

Yay, tests.

> ---
>  diff.c                  |  1 +
>  t/t4022-diff-rewrite.sh | 27 +++++++++++++++++++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/diff.c b/diff.c
> index 95706a5..77d4e84 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -574,6 +574,7 @@ static void emit_rewrite_lines(struct
> emit_callback *ecb,

Your patch is line-wrapped and cannot be applied as-is (try turning off
"flowed text" in your MUA).

>  	if (!endp) {
>  		const char *plain = diff_get_color(ecb->color_diff,
>  						   DIFF_PLAIN);
> +		putc('\n', ecb->opt->file);
>  		emit_line_0(ecb->opt, plain, reset, '\\',
>  			    nneof, strlen(nneof));
>  	}

Looks correct. I was curious how the regular (non-rewrite) code path did
this, and it just sticks the "\n" as part of the nneof string. However,
we would not want that here, because each line should have its own
color markers.

> +# create a file containing numbers with no newline at
> +# the end and modify it such that the starting 10 lines
> +# are unchanged, the next 101 are rewritten and the last
> +# line differs only in that in is terminated by a newline.
> +seq 1 10 > seq
> +seq 100 +1 200 >> seq
> +printf 201 >> seq
> +(git add seq; git commit seq -m seq) >/dev/null
> +seq 1 10 > seq
> +seq 300 -1 200 >> seq

Seq is (unfortunately) not portable. I usually use a perl snippet
instead, like:

  perl -le 'print for (1..10)'

Though I think we are adjusting that to use $PERL_PATH these days.

-Peff

  reply	other threads:[~2012-08-02 21:34 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-08-02 21:11 [PATCH] Fix 'No newline...' annotation in rewrite diffs Adam Butcher
2012-08-02 21:33 ` Jeff King [this message]
2012-08-02 21:52   ` Junio C Hamano
2012-08-02 22:14     ` Jeff King
2012-08-03  7:49       ` Michał Kiedrowicz
2012-08-03 16:02         ` Jeff King
2012-08-03 16:46           ` Junio C Hamano
2012-08-03 17:00             ` Jeff King
2012-08-03 19:57           ` [PATCH] tests: Introduce test_seq Michał Kiedrowicz
2012-08-03 20:02             ` Jeff King
2012-08-03 20:53               ` Junio C Hamano
2012-08-03 22:02                 ` Jeff King
2012-08-03 22:09                 ` Michał Kiedrowicz
2012-08-04 16:38                   ` Johannes Sixt
2012-08-04 23:05                     ` Junio C Hamano
2012-08-06 17:52                     ` Michał Kiedrowicz
2012-08-06 20:16                     ` Jeff King
2012-08-03 22:21                 ` Michał Kiedrowicz
2012-08-03 22:48                   ` Junio C Hamano
2012-08-03 23:08                     ` Jeff King
2012-08-03 23:12                   ` Junio C Hamano
2012-08-04  8:14                     ` Michał Kiedrowicz
2012-08-04 22:10                       ` Adam Butcher
2012-08-03 20:04           ` Michał Kiedrowicz
2012-08-03 20:07             ` Jeff King
2012-08-03 20:12               ` Michał Kiedrowicz
2012-08-03 20:38                 ` Michał Kiedrowicz
2012-08-03 20:41                   ` Jeff King
2012-08-02 22:22   ` [PATCH] Fix 'No newline...' annotation in rewrite diffs Adam Butcher
2012-08-02 22:00 ` Junio C Hamano
2012-08-02 22:58   ` Adam Butcher
2012-08-04 21:07     ` Adam Butcher
2012-08-05  1:26       ` Junio C Hamano
2012-08-05  7:06         ` [PATCH] Fix '\ No " Adam Butcher

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=20120802213346.GA575@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=dev.lists@jessamine.co.uk \
    --cc=git@vger.kernel.org \
    /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).