bugQuilt - Bugs: bug #54295, Portability - At least series and...

 
 

bug #54295: Portability - At least series and push do not always deal with newlines in series files properly

Submitter:  Filipus Klutiero <chealer>
Submitted:  Thu 12 Jul 2018 05:04:41 PM UTC
   
 
Category:  None Severity:  2 - Minor
Item Group:  None Status:  Wont Fix
Privacy:  Public Assigned to:  khali
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 05 Nov 2018 04:05:15 PM UTC, comment #10: 

The fix for the second issue was committed:

http://git.savannah.nongnu.org/cgit/quilt.git/commit/?id=cbde26f240a64bf6b4bd58951113e3524fc8db88

Nothing will be done for the issue originally reported here. It is up to the user to ensure that the text files he or she feeds quilt with match the standard line terminator format for the platform.

Jean Delvare <khali>
Group administrator
Thu 04 Oct 2018 09:55:48 AM UTC, comment #9: 

Candidate fix attached, please test.

Jean Delvare <khali>
Group administrator
Thu 04 Oct 2018 09:36:51 AM UTC, comment #8: 

The issue mentioned in comment #6 is different from the issue reported originally. It should have been reported in a separate ticket.

I can reproduce that problem artificially, but otherwise it should not happen. The series file is managed by quilt itself, which will never create a file without a trailing newline. Editing the series file manually is supported but if you break it then you are on your own. There are a lot of different ways to break the series file and we can't (and don't want to) have quilt detect and fix all of them.

That being, said, we fixed a similar issue for the "header" command last year:

http://git.savannah.nongnu.org/cgit/quilt.git/commit/?id=102ab72dc48297be33d62dbe73536a714bcbf464

Doing something similar for the "new" command would be pretty straightforward.

Jean Delvare <khali>
Group administrator
Mon 20 Aug 2018 08:56:35 PM UTC, comment #7: 

Oh, and this happens whether or not series has CR characters.

Filipus Klutiero <chealer>
Mon 20 Aug 2018 08:54:31 PM UTC, comment #6: 

Thank you Jean

I have reproduced my issue very simply: run quilt new with a series file which doesn't end with a newline (i.e. which has a patch name on its last line). This adds a newline, but at the end of the file rather than before "foo.diff".

But, I couldn't reproduce this with Ubuntu's Quilt 0.61. So, unless you think this is a bug fixed since 0.63, I will report this against Cygwin Quilt.

Filipus Klutiero <chealer>
Wed 18 Jul 2018 08:35:39 AM UTC, comment #5: 

Cygwin must be pretty tricky and a beast of its own when it comes to newline handling. Operating on top of Windows, it should consider CRLF as a valid newline control character sequence. But at the same time, it emulates a Unix-like environment in which most tools will certainly expect LF to be the newline control character sequence.

There are so many separate system commands, did the Cygwin folks really modify all of them to support both CRLF and LF transparently? Hard to believe, but I no longer have any Cygwin system at hand, so I can't test anyway.

On the other hand, at least grep would behave properly with both CRLF and LF text files on Windows (and presumably Cygwin) according to its manual page. But it does not on Linux (as demonstrated in previous comment.)

We are in 2018 and we still don't have a standard format for something as simple as plain text files. How sad...

At this point, it doesn't seem realistic to expect quilt to handle both text file formats (CRLF and LF) transparently on all operating systems. The only sane thing to do is to ensure that the format of the series file matches the expectations of the operating system.

For now it has to be done manually, as you have painfully experienced. Maybe it would make sense to add a safety check for that in quilt itself. However I don't want to slowdown all commands for all users to check for a condition which is not supposed to happen in the first place, so this is only going to happen if we manage to find a fast, reliable and non-intrusive way to perform that check.

Jean Delvare <khali>
Group administrator
Wed 18 Jul 2018 07:54:01 AM UTC, comment #4: 

Quilt is a set of bash scripts, it heavily relies on system commands (cat, awk, grep, head, tail, etc.) to manipulate the series file. On Linux, these commands will appear to support CRLF simply because CR is not displayed. As such, the output of these commands will typically be a "valid" CRLF text file, in the sense that the useless CRs are preserved:


$ file dos.txt
dos.txt: ASCII text, with CRLF line terminators
$ cat dos.txt
first
second
$ head -n 1 dos.txt | hexdump -C
00000000  66 69 72 73 74 0d 0a                              |first..|
00000007
$


"0d" above is CR.

The important detail is that the CR is not considered to be part of the line terminator, as demonstrated by the following examples:


$ grep 'first' dos.txt
first
$ grep 'first$' dos.txt
$
$ perl -wn -e 'print if /first/' dos.txt
first
$ perl -wn -e 'print if /first$/' dos.txt
$


So you can end up with unexpected behavior. While it is possible to convince perl that a newline is CRLF instead of LF, I am not aware of any way to convince other commands like grep of the same.

Additionally, inserting new lines in the series file, as done by "quilt new", will typically be achieved with bash's "echo" builtin command, which will terminate the line with the native newline sequence for the operating system, which is LF on Linux. If the series file was originally in CRLF (DOS) format, this will result in an invalid mixed line terminator text file.

Your broken series file is different from that scenario however. I have to admit that I don't know how such a corruption could happen. What exact quilt commands did you run to get there?

Jean Delvare <khali>
Group administrator
Tue 17 Jul 2018 07:02:40 PM UTC, comment #3: 

It turns out the culprit is Git, which is playing smartass and converting LF to CRLF automatically in text files, even though our Windows software deals with LF just fine. I've worked around that in our repository by adding a .gitattributes file.

However, it may be that Cygwin Quilt 0.63 does not behave properly with CRLF files, failing to add a newline before the new line's content.

Filipus Klutiero <chealer>
Tue 17 Jul 2018 02:44:32 PM UTC, comment #2: 

I am sorry, my report was quite wrong. Thank you for warning me Jean.

Attached is the actual problematic file. I just realized that this file has no newline whatsoever between its previous last line and the (partial) line added by quilt new.

You have a good point Jean. I cannot really explain why our series file uses CRLF. I will try identifying the operation which converts to CRLF. I probably failed to do so so far because TortoiseGit seems to be misleading with regard to newlines.

(file #44570)

Filipus Klutiero <chealer>
Mon 16 Jul 2018 09:42:56 AM UTC, comment #1: 

You attached one of the patches, instead of the series file.

Why are you using CR+LF for your series file in the first place, if your two target environments (Cygwin and Ubuntu) use LF?

Text files using mixed newline control characters are invalid in the first place, so I have no plan to make quilt support them. If you are importing your series file from a different system, it is your responsibility to ensure that its text format matches the convention of the target system.

Jean Delvare <khali>
Group administrator
Thu 12 Jul 2018 05:04:41 PM UTC, original submission:  

Quilt generally works with newlines in series files fine no matter the operating system, but it seems that mixing several newline sequences in the same series file is not supported. In fact, at least subcommands series and push may quietly fail.

Our series file uses CRLF, but adding a new patch with Cygwin Quilt 0.63 adds a LF only, resulting in a mixed file. This works fine on Windows, but Ubuntu's Quilt 0.61 does not work correctly with such a file. Running series simply does not show the new patch (and does not show any kind of warning). quilt push reports (in our case) "File series fully applied, ends at patch Search_6104_TruncatedSearch.diff".

A problematic series file is attached.

I cannot see anything in the NEWS file about this problem.

Filipus Klutiero <chealer>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #45140:  new-deal-with-missing-newline.patch added by khali (2KiB - text/x-patch - [PATCH] new: Fix series file corruption when trailing newline is missing)
file #44570:  series added by chealer (1KiB - application/octet-stream - Problematic series file)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by khali (Posted a comment)
  • -email is unavailable- added by chealer (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-11-05 khali StatusNeed Info Wont Fix
        Open/ClosedOpen Closed
    2018-10-04 khali StatusConfirmed Need Info
    2018-10-04 khali Attached File- Added new-deal-with-missing-newline.patch, #45140
    2018-10-04 khali Severity3 - Normal 2 - Minor
        StatusNeed Info Confirmed
    2018-07-18 khali Attached File#44540 Removed
    2018-07-17 chealer Attached File- Added series, #44570
    2018-07-16 khali StatusNone Need Info
        Assigned toNone khali
    2018-07-12 chealer Attached File- Added Parser_TOC_HTML_fix.diff, #44540

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code