Re: [PATCH] blame: make sure that the last line ends in an LF

[Sverre Rabbelier <srabbelier@gmail.com>]

Heya,

On Tue, Oct 20, 2009 at 02:00, Junio C Hamano <gitster@pobox.com> wrote:
>  (2) Do the right thing, by coming up with a notation to show that the
>     final line is incomplete, perhaps similar to "\No newline ..."
>     notation used by "diff".

What about 'git blame -p', can we just go about changing the format like that?

For purpose of the discussion below let's assume we squash in the following:

-- <8 --

diff --git a/builtin-blame.c b/builtin-blame.c
index dd16b22..cf492a0 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -1606,7 +1606,7 @@ static void emit_porcelain(struct scoreboard
*sb, struct blame_entry *ent)
 	}

 	if (sb->final_buf_size && cp[-1] != '\n')
-		putchar('\n');
+		printf("\n\\ No newline at end of file\n");
 }

 static void emit_other(struct scoreboard *sb, struct blame_entry *ent, int opt)
@@ -1672,7 +1672,7 @@ static void emit_other(struct scoreboard *sb,
struct blame_entry *ent, int opt)
 	}

 	if (sb->final_buf_size && cp[-1] != '\n')
-		putchar('\n');
+		printf("\n\\ No newline at end of file\n");
 }

 static void output(struct scoreboard *sb, int option)
-- 

-- <8 --

> Does the code assign blame
> correctly around the last line of the original blob?

Yes, it does, when there is no trailing newline an extra "\ No newline
at end of file" is printed, but the last line is still attributed
correctly.

> What if an older
> version ended with an incomplete line and a later version changed the line
> (without adding the terminating LF)?

Nothing changes, the blame on that last line is attributed correctly
and the "\ No newline at end of file" is printed.

> What if a later version changed the
> line and added the terminating LF?

The trailing "\ No newline at end of file" is no longer printed and
the last line is correctly attributed to the commit that added the
trailing LF.

> What if a later version only added the
> terminating LF and did nothing else?  Are these three cases handled
> correctly?

Same as above.

> After thinking issues like the above, I read the patch and I see it does
> not take neither approach.  That makes me feel nervous.

Reading your reply I see that if you care about the presence (or
absence) of a trailing newline the current patch would be problematic,
as it makes it impossible to see in the blame output whether there was
a trailing newline or not.

> By tweaking only the output routine you _might_ be getting correct output,
> but even then it looks to me like the end result is working correctly not
> by design but by accident.  IOW, the patch may be better than nothing, but
> somehow it just feels like it is papering over the real issue than being a
> proper fix.
>
> Or am I worrying too much?

No, I think your concerns are valid, we should go with (2) and DTRT.
Does the updated patch address your concerns? If so I can send a new
version.

-- 
Cheers,

Sverre Rabbelier