threads / patch / 42319

patchcrlf: Add test showing double warning on commit

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

## tl;dr

6 messages between May 14, 2016 and May 15, 2016. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Adam Dinwoodie· May 14, 2016, 11:17 UTC · lore

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(-)

Show changes to t/t0020-crlf.sh +15 −1

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

Torsten Bögershausen· May 14, 2016, 16:20 UTC · re: Adam Dinwoodie · lore

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

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

Show changes to diffcore-break.c +9 −0

--- 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 ?

Junio C Hamano· May 14, 2016, 18:45 UTC · re: Torsten Bögershausen · lore

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

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"?

Torsten Bögershausen· May 15, 2016, 06:39 UTC · re: Junio C Hamano · lore

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

On 14.05.16 20:45, Junio C Hamano wrote:

Show 23 quoted lines

> 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 ?

Junio C Hamano· May 15, 2016, 18:55 UTC · re: Torsten Bögershausen · lore

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

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

Show 7 quoted lines

> 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".

Torsten Bögershausen· May 15, 2016, 06:46 UTC · re: Junio C Hamano · lore

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

On 14.05.16 20:45, Junio C Hamano wrote:

Show 16 quoted lines

> 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)