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

Heya,

On Tue, Oct 20, 2009 at 02:00, Junio C Hamano [off-list ref] 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