git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
* Fw: [PATCH] Documentation: networking: device drivers: Remove stray asterisks
@ 2019-12-01  1:03 Jonathan Corbet
  2019-12-01  1:20 ` Linus Torvalds
  0 siblings, 1 reply; 8+ messages in thread
From: Jonathan Corbet @ 2019-12-01  1:03 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Git List Mailing, Junio Hamano C, LKML, open list:DOCUMENTATION,
	Jonathan Neuschäfer

[-- Attachment #1: Type: text/plain, Size: 1087 bytes --]

> On Sat, Nov 30, 2019 at 4:14 PM Jonathan Corbet <corbet@lwn.net> wrote:
> >
> > So my tooling is "git am", nothing special.
> >
> > All of the afflicted files arrived in that state as the result of a pair
> > of patches from Jonathan (copied); I have verified that the original
> > patches also had the DOS line endings.
> >
> > The problem repeats if I apply those patches now, even if I add an
> > explicit "--no-keep-cr" to the "git am" command line.  It seems like maybe
> > my version of git is somehow broken?  I have git-2.21.0-1.fc30.x86_64,
> > FWIW.  
> 
> Hmm. I wonder if the CRLF removal is broken in general, or if the
> emails are somehow unusual (patches in attachments or MIME-encoded or
> something)? Maybe the CRLF was removed from the envelope email lines,
> but if the patch is then decoded from an attachment or something it's
> not removed again from there?
> 
> Can you attach (not forward) one of the (raw) emails that shows the
> problem and keep the git mailing list cc'd?

Attached.  The patch itself was not an attachment, but it was
base64-encoded. 

jon

[-- Attachment #2: Type: message/rfc822, Size: 26796 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Fw: [PATCH] Documentation: networking: device drivers: Remove stray asterisks
  2019-12-01  1:03 Fw: [PATCH] Documentation: networking: device drivers: Remove stray asterisks Jonathan Corbet
@ 2019-12-01  1:20 ` Linus Torvalds
  2019-12-01  1:45   ` Jonathan Corbet
  0 siblings, 1 reply; 8+ messages in thread
From: Linus Torvalds @ 2019-12-01  1:20 UTC (permalink / raw)
  To: Jonathan Corbet
  Cc: Git List Mailing, Junio Hamano C, LKML, open list:DOCUMENTATION,
	Jonathan Neuschäfer

On Sat, Nov 30, 2019 at 5:03 PM Jonathan Corbet <corbet@lwn.net> wrote:
>
> Attached.  The patch itself was not an attachment, but it was
> base64-encoded.

Ok, so presumably git removed the CRLF from the email, but then the
base64 encoded part had another set of CRLF.

And when I try to apply that patch (in a test-tree reset to commit
facd86390be2, so I think the patch should apply) I see the CRLF in
.git/rebase-apply/patch, but then I get

  error: patch failed: Documentation/networking/device_drivers/intel/e100.rst:1
  error: Documentation/networking/device_drivers/intel/e100.rst: patch
does not apply

for every hunk. I assume that's because the CR part doesn't match the old code.

But my git version is d9f6f3b619 ("The first batch post 2.24 cycle")
and some private patches.

So the problem might be limited to only some versions. I'm surprised,
though - when git applies patches, it really wants the surrounding
lines to match exactly. The extra CR at the end of the lines should
have made that test fail.

Do you use some special options for git? Like --whitespace=nowarn or
--3way or something like that?

            Linus

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Documentation: networking: device drivers: Remove stray asterisks
  2019-12-01  1:20 ` Linus Torvalds
@ 2019-12-01  1:45   ` Jonathan Corbet
  2019-12-01  6:35     ` Junio C Hamano
  0 siblings, 1 reply; 8+ messages in thread
From: Jonathan Corbet @ 2019-12-01  1:45 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Git List Mailing, Junio Hamano C, LKML, open list:DOCUMENTATION,
	Jonathan Neuschäfer

On Sat, 30 Nov 2019 17:20:10 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> Do you use some special options for git? Like --whitespace=nowarn or
> --3way or something like that?

Sigh, that has to be it.  I have --ignore-whitespace in my script. When I
take that option out, the patch in question no longer applies.

Docs patches often come from relatively new folks, and I've found that I
needed that to apply a lot of their patches.  But clearly that was not a
good choice; among other things, I've lost the opportunity to tell
people when their patches have the types of whitespace issues that this
option covers over.  I've taken it out of the script and will only use it
by hand in cases where I'm sure that it won't cause problems.

Sorry for the screwup...

jon

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Documentation: networking: device drivers: Remove stray asterisks
  2019-12-01  1:45   ` Jonathan Corbet
@ 2019-12-01  6:35     ` Junio C Hamano
  2019-12-01 14:28       ` Linus Torvalds
  0 siblings, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2019-12-01  6:35 UTC (permalink / raw)
  To: Jonathan Corbet
  Cc: Linus Torvalds, Git List Mailing, Junio Hamano C, LKML,
	open list\:DOCUMENTATION, Jonathan Neuschäfer

Jonathan Corbet <corbet@lwn.net> writes:

> On Sat, 30 Nov 2019 17:20:10 -0800
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
>> Do you use some special options for git? Like --whitespace=nowarn or
>> --3way or something like that?
>
> Sigh, that has to be it.  I have --ignore-whitespace in my script. When I
> take that option out, the patch in question no longer applies.

OK, so it appears that the tool is working as documented.  The
"ignore" stuff kicks in to fuzz the whitespace difference for the '
' lines and '-' lines, but the option itself does not give "git
apply" enough information to decide what to do with the extra
whitespace that is CR at the end of the line on the '+' lines.

I would also say it is doubtful that it is working as expected.
Perhaps --ignore-whitespace and --whitespace=fix ought to work well
together to allow matching preimage (i.e. ' ' and '-') lines, but
still fix whitespace-broken material in postimage (i.e.  ' ' and
'+') lines before replacing the preimage with the postimage, or
something along that line?




^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Documentation: networking: device drivers: Remove stray asterisks
  2019-12-01  6:35     ` Junio C Hamano
@ 2019-12-01 14:28       ` Linus Torvalds
  2019-12-01 15:27         ` Junio C Hamano
  0 siblings, 1 reply; 8+ messages in thread
From: Linus Torvalds @ 2019-12-01 14:28 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Jonathan Corbet, Git List Mailing, LKML, open list:DOCUMENTATION,
	Jonathan Neuschäfer

On Sat, Nov 30, 2019 at 10:35 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> OK, so it appears that the tool is working as documented.

Well, yes and no.

I think it's a mistake that --no-keep-cr (which is the default) only
acts on the outer envelope.

Now, *originally* the outer envelope was all that existed so it makes
sense in a historical context of "CR removal happens when splitting
emails in an mbox". And that's the behavior we have.

But then git learnt to do MIME decoding and extracting things from
base64 etc, and the CR removal wasn't updated to that change.

I guess "documented" is arguable in the sense that the git
documentation does talk about "git-mailsplit" as an implementation
detail, but still..

             Linus

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Documentation: networking: device drivers: Remove stray asterisks
  2019-12-01 14:28       ` Linus Torvalds
@ 2019-12-01 15:27         ` Junio C Hamano
  2019-12-02  9:50           ` Amit Choudhary
  0 siblings, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2019-12-01 15:27 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Jonathan Corbet, Git List Mailing, LKML,
	open list\:DOCUMENTATION, Jonathan Neuschäfer

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Sat, Nov 30, 2019 at 10:35 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> OK, so it appears that the tool is working as documented.
>
> Well, yes and no.
>
> I think it's a mistake that --no-keep-cr (which is the default) only
> acts on the outer envelope.
>
> Now, *originally* the outer envelope was all that existed so it makes
> sense in a historical context of "CR removal happens when splitting
> emails in an mbox". And that's the behavior we have.

Hmph, first of all, the one I was referring to as "documented" was
about --ignore-whitespace, and not --no-keep-cr.

And I am not as sure as you seem to be about "--no-keep-cr" either.

What was the reason why "--no-keep-cr" was invented and made
default?  Wasn't it because RFC says that each line of plaintext
transfer of an e-mail is terminated with CRLF?  It would mean that,
whether the payload originally had CRLF terminated or LF terminated,
we would not be able to tell---the CR may have been there from the
beginning, or it could have been added in transit.  And because we
(the projects Git was originally designed to serve well) wanted our
patches with LF terminated lines most of the time, it made sense to
strip CR from CRLF (i.e. assuming that it would be rare that the
sender wants to transmit CRLF terminated lines).

If the contents were base64 protected from getting munged during
transit, we _know_ CRLF in the payload after we decode MIME is what
the sender _meant_ to give us, no?  Which leads me to say ...

>
> But then git learnt to do MIME decoding and extracting things from
> base64 etc, and the CR removal wasn't updated to that change.

... I do not think it was a wrong decision (well, I do not think we
made the conscious decision to do so, though) not to do that update.

I dunno.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Documentation: networking: device drivers: Remove stray asterisks
  2019-12-01 15:27         ` Junio C Hamano
@ 2019-12-02  9:50           ` Amit Choudhary
  2019-12-02 14:22             ` Junio C Hamano
  0 siblings, 1 reply; 8+ messages in thread
From: Amit Choudhary @ 2019-12-02  9:50 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Linus Torvalds, Jonathan Corbet, Git List Mailing, LKML,
	open list:DOCUMENTATION, Jonathan Neuschäfer

Is it possible that git complains about everything that has ^M in it
and rejects it (that is without trying to fix it, etc.)

Regards,
Amit

On Sun, Dec 1, 2019 at 8:58 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
> > On Sat, Nov 30, 2019 at 10:35 PM Junio C Hamano <gitster@pobox.com> wrote:
> >>
> >> OK, so it appears that the tool is working as documented.
> >
> > Well, yes and no.
> >
> > I think it's a mistake that --no-keep-cr (which is the default) only
> > acts on the outer envelope.
> >
> > Now, *originally* the outer envelope was all that existed so it makes
> > sense in a historical context of "CR removal happens when splitting
> > emails in an mbox". And that's the behavior we have.
>
> Hmph, first of all, the one I was referring to as "documented" was
> about --ignore-whitespace, and not --no-keep-cr.
>
> And I am not as sure as you seem to be about "--no-keep-cr" either.
>
> What was the reason why "--no-keep-cr" was invented and made
> default?  Wasn't it because RFC says that each line of plaintext
> transfer of an e-mail is terminated with CRLF?  It would mean that,
> whether the payload originally had CRLF terminated or LF terminated,
> we would not be able to tell---the CR may have been there from the
> beginning, or it could have been added in transit.  And because we
> (the projects Git was originally designed to serve well) wanted our
> patches with LF terminated lines most of the time, it made sense to
> strip CR from CRLF (i.e. assuming that it would be rare that the
> sender wants to transmit CRLF terminated lines).
>
> If the contents were base64 protected from getting munged during
> transit, we _know_ CRLF in the payload after we decode MIME is what
> the sender _meant_ to give us, no?  Which leads me to say ...
>
> >
> > But then git learnt to do MIME decoding and extracting things from
> > base64 etc, and the CR removal wasn't updated to that change.
>
> ... I do not think it was a wrong decision (well, I do not think we
> made the conscious decision to do so, though) not to do that update.
>
> I dunno.
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Documentation: networking: device drivers: Remove stray asterisks
  2019-12-02  9:50           ` Amit Choudhary
@ 2019-12-02 14:22             ` Junio C Hamano
  0 siblings, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2019-12-02 14:22 UTC (permalink / raw)
  To: Amit Choudhary
  Cc: Linus Torvalds, Jonathan Corbet, Git List Mailing, LKML,
	open list\:DOCUMENTATION, Jonathan Neuschäfer

Amit Choudhary <amitchoudhary2305@gmail.com> writes:

> Is it possible that git complains about everything that has ^M in it
> and rejects it (that is without trying to fix it, etc.)

I am not sure what you mean.  Are you asking if there is a mode
(e.g. command line switch) to tell "git am" to reject any input with
CR in it?  I do not think there is, and I do not think it would help
all that much.  But perhaps the pre-applypatch hook can be used to
inspect the current working tree files (and it can compare them with
HEAD to learn what are the proposed changes) and reject the patch---
an advantage of such an approach is that the "inspect" step does not
have to be limited to "does it contain a carriage-return?"

Or are you asking if the patch (mis)application that triggered this
discussion thread was somehow caused by Git that complains a payload
with CR in it?  I do not think so.



^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-01  1:03 Fw: [PATCH] Documentation: networking: device drivers: Remove stray asterisks Jonathan Corbet
2019-12-01  1:20 ` Linus Torvalds
2019-12-01  1:45   ` Jonathan Corbet
2019-12-01  6:35     ` Junio C Hamano
2019-12-01 14:28       ` Linus Torvalds
2019-12-01 15:27         ` Junio C Hamano
2019-12-02  9:50           ` Amit Choudhary
2019-12-02 14:22             ` Junio C Hamano

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git