[PATCH] crlf: Add test showing double warning on commit

[PATCH] crlf: Add test showing double warning on commit

[Adam Dinwoodie]

Add failing test case showing CRLF -> LF rewrite warnings being printed
multiple times when running "git commit".

Signed-off-by: Adam Dinwoodie <adam@dinwoodie.org>
---
 t/t0020-crlf.sh | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
index f94120a..188b1dd 100755
--- a/t/t0020-crlf.sh
+++ b/t/t0020-crlf.sh
@@ -74,7 +74,7 @@ test_expect_success 'safecrlf: autocrlf=true mixed LF/CRLF' '
 	test_must_fail git add mixed
 '
 
-test_expect_success 'safecrlf: print warning only once' '
+test_expect_success 'safecrlf: print warning only once on add' '
 
 	git config core.autocrlf input &&
 	git config core.safecrlf warn &&
@@ -87,6 +87,20 @@ test_expect_success 'safecrlf: print warning only once' '
 '
 
 
+test_expect_failure 'safecrlf: print warning only once on commit' '
+
+	git config core.autocrlf input &&
+	git config core.safecrlf warn &&
+
+	for w in I am all LF; do echo $w; done >doublewarn2 &&
+	git add doublewarn2 &&
+	git commit -m "nowarn" &&
+	for w in Oh here is CRLFQ in text; do echo $w; done | q_to_cr >doublewarn2 &&
+	git add doublewarn2 2>&1 &&
+	test $(git commit -m Message | grep "CRLF will be replaced by LF" | wc -l) = 1
+'
+
+
 test_expect_success 'safecrlf: git diff demotes safecrlf=true to warn' '
 	git config core.autocrlf input &&
 	git config core.safecrlf true &&
-- 
2.8.1

Re: [PATCH] crlf: Add test showing double warning on commit

[Torsten Bögershausen]

On 14.05.16 13:17, Adam Dinwoodie wrote:
> Add failing test case showing CRLF -> LF rewrite warnings being printed
> multiple times when running "git commit".
>

The problem seems to come from this line:

index 5473493..59d4106 100644
--- a/diffcore-break.c
+++ b/diffcore-break.c
@@ -61,9 +61,18 @@ static int should_break(struct diff_filespec *src,
            !hashcmp(src->sha1, dst->sha1))
                return 0; /* they are the same */
 
+       fprintf(stderr, "%s:%d src-path=%s dst-path=%s\n",
+               __FILE__, __LINE__, src->path, dst->path);
+#if 0
        if (diff_populate_filespec(src, 0) || diff_populate_filespec(dst, 0))
                return 0; /* error but caught downstream */
+#else
 
+       if (diff_populate_filespec(src, 0))
+               return 0; /* error but caught downstream */
+       if (strcmp(src->path, dst->path) && diff_populate_filespec(dst, 0))
+               return 0; /* error but caught downstream */
+#endif
        max_size = ((src->size > dst->size) ? src->size : dst->size);

Do we need to run diff_populate_filespec() twice when src==dst ?
If yes, we may need to introduce a flag besides
#define CHECK_SIZE_ONLY 1
#define CHECK_BINARY    2
to suppress the conversion warning ??

If no, the fix from above should do ?

Re: [PATCH] crlf: Add test showing double warning on commit

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> Do we need to run diff_populate_filespec() twice when src==dst ?

Of course we do.

src and dst may have the same path, but are coming from different
places (src may be an indexed blob while dst may be a file in the
working tree).

> If yes, we may need to introduce a flag besides
> #define CHECK_SIZE_ONLY 1
> #define CHECK_BINARY    2
> to suppress the conversion warning ??

I do not think that belongs to diff_populate_filespec() at all.

Why should conversion routine give this warning when called by
diff_populate_filespec() in the first place?  Shouldn't it be silent
by default, and is allowed to talk _ONLY_ when we are attempting to
actually replace the data in the index, e.g. "git add" and "git
commit -a"?

Re: [PATCH] crlf: Add test showing double warning on commit

[Torsten Bögershausen]

On 14.05.16 20:45, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
> 
>> Do we need to run diff_populate_filespec() twice when src==dst ?
> 
> Of course we do.
> 
> src and dst may have the same path, but are coming from different
> places (src may be an indexed blob while dst may be a file in the
> working tree).
> 
>> If yes, we may need to introduce a flag besides
>> #define CHECK_SIZE_ONLY 1
>> #define CHECK_BINARY    2
>> to suppress the conversion warning ??
> 
> I do not think that belongs to diff_populate_filespec() at all.
> 
> Why should conversion routine give this warning when called by
> diff_populate_filespec() in the first place?  Shouldn't it be silent
> by default, and is allowed to talk _ONLY_ when we are attempting to
> actually replace the data in the index, e.g. "git add" and "git
> commit -a"?
> 

Nja, (Or Nyes in English), the old handling tried to be "nice" to the user:
$ git add text # gave warning
#User forgets, does other things, git reset HEAD....
$ git commit # Gave the warning one more time, to remind the user, 
             # what he did, and what is really commited.

But it may be, that diff_populate_filespec() is the wrong place for speaches ?

Re: [PATCH] crlf: Add test showing double warning on commit

[Torsten Bögershausen]

On 14.05.16 20:45, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
> 
>> Do we need to run diff_populate_filespec() twice when src==dst ?
> 
> Of course we do.
> 
> src and dst may have the same path, but are coming from different
> places (src may be an indexed blob while dst may be a file in the
> working tree).
> 
>> If yes, we may need to introduce a flag besides
>> #define CHECK_SIZE_ONLY 1
>> #define CHECK_BINARY    2
>> to suppress the conversion warning ??
> 
> I do not think that belongs to diff_populate_filespec() at all.

Just to remind myself:
sha1_file.c:
The warning should probably triggered from here, depending on the flags ?

int index_fd(unsigned char *sha1, int fd, struct stat *st,
	     enum object_type type, const char *path, unsigned flags)

Re: [PATCH] crlf: Add test showing double warning on commit

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> Nja, (Or Nyes in English), the old handling tried to be "nice" to the user:
> $ git add text # gave warning
> #User forgets, does other things, git reset HEAD....
> $ git commit # Gave the warning one more time, to remind the user, 
>              # what he did, and what is really commited.
>
> But it may be, that diff_populate_filespec() is the wrong place for speaches ?

It is *not* necessarily a wrong place for speeches, but certainly it
is wrong place to talk about whatever convert_to_git() does.

Let's step back and think what the warning is about.

I personally feel that "CRLF will be turned into LF" is pointless
when the user expressed her preference "I want to have LF in the
repository and CRLF in the working tree, when it makes sense".  It
is not even a warning but merely reports "we did what we were told
to do."  But for those who set safecrlf=warn to get that warning, we
should give the warning when we actually convert the the working tree
contents and strip CR from CRLF before recording it as an object.

An as-is "git commit" (nothing on command line affects the index
with working tree contents, e.g. "-a" or pathspec) does not do any
such conversion.  The "damage" the user requested to be warned has
long been done before the command is run.

So the warning from "git add" is good, but it shouldn't appear from
anything that does "diff".