Re: [PATCH v2 0/2] Fix interactive rebase when the editor saves with CR/LF

2015-10-27 Thread Johannes Schindelin
Hi Junio,

On Mon, 26 Oct 2015, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > A simple test with CR/LF line endings in a script reveals that it is
> > pretty solid:
> >
> > x=a
> > case "$x" in a) echo b;; esac
> >
> > prints out 'b', as expected.
> 
> I do not see what this has to do with anything.

You probably missed the context. Brian pointed out that my patch said
"printf ''" when it should have said "printf '\n'". I responded that my
commit says the right thing, but somewhere along the lines of transforming
beautiful Git commits into mails it must have been lost.

The explanation you quoted is the essence of the problem of my script to
prepare mails for submission to the mailing list: the script works in
Bash, but fails in Dash. And as my script has the shebang "#!/bin/sh" and
as Ubuntu defaults to using the Dash as its /bin/sh, we now have the full
explanation why my first mail showed an incorrect patch.

Ciao,
Johannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v2 0/2] Fix interactive rebase when the editor saves with CR/LF

2015-10-26 Thread Junio C Hamano
Johannes Schindelin  writes:

> A simple test with CR/LF line endings in a script reveals that it is
> pretty solid:
>
>   x=a
>   case "$x" in a) echo b;; esac
>
> prints out 'b', as expected.

I do not see what this has to do with anything.

The shell language parser when parsing a script may do the right
thing, but the bug I was alluding to was that your 'read' does not
seem to be removing the terminating  (which is CRLF on your
platform) after reading a line before splitting the contents on the
line at IFS boundaries.

> Again. If CR has no place in IFS, why does LF have a place in IFS? It
> makes *no* sense to argue for one and against the other.

See my message to Matthieu.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v2 0/2] Fix interactive rebase when the editor saves with CR/LF

2015-10-26 Thread Johannes Schindelin
Hi Junio,

On Sun, 25 Oct 2015, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > Chad Boles reported that `git rebase -i` recently started producing
> > errors when the editor saves files with DOS line endings. The symptom
> > is:
> >
> > Warning: the command isn't recognized in the following line:
> >  -
> >
> > You can fix this with 'git rebase --edit-todo'.
> > Or you can abort the rebase with 'git rebase --abort'.
> >
> > The real bummer is that simply calling `git rebase --continue` "fixes"
> > it.
> 
> Two questions.
> 
>  * What does the DOS editor do to a file with ^M in the middle of a
>line, e.g. "A^MB^M^J"?

You mean mixed line endings? At least with this Windows 10 WordPad, *all*
line endings are normalized to CR/LF. That is, if you edit a file that
contains a stray CR, it is presented as a line break.

>  * Is your shell ported correctly to the platform?

I would think so. It is an MSys2 program, therefore it uses all the POSIX
niceties, of course.

A simple test with CR/LF line endings in a script reveals that it is
pretty solid:

x=a
case "$x" in a) echo b;; esac

prints out 'b', as expected.

Please also note that things apparently worked alright before the patch
removing the stripspace call was applied.

> The latter may need a bit of elaboration.  "read a b c" is supposed
> to read one line of text (where the definition of line is platform
> dependent, your platform may use CRLF to signal the end of an line),
> split the characters on the line (i.e. LF vs CRLF no longer matters
> at this point) at $IFS characters and give them to $a $b and $c. If
> the platform accepts CRLF as the EOL signal, should the program still
> see CR at the end of $c?
> 
> A solution that mucks with IFS smells like fixing a wrong problem
> without addressing the real cause.

Please note that it is a bit unsettling if you use funny language like
"smells" here and then accuse me of not having an argument when I point
that the same rationale applies to having CR in IFS as applies to having
LF in IFS. Yes, that was an implicit argument, but it is a strong one, so
I do not think you are well served ignoring it.

> Also IFS is used not only by "read", so munging it globally doubly
> feels wrong.
> 
> In addition, you do not want to split at CR; what you want is to
> treat CRLF (i.e. not a lone CR) as the end-of-line separator.
> Adding CR to IFS feels triply wrong.
> 
> By the way, saying "This is right, really" makes you sound as if you
> do not have a real argument.

Again. If CR has no place in IFS, why does LF have a place in IFS? It
makes *no* sense to argue for one and against the other.

In any case, I am not interested in arguing for arguing's sake. Tell me
what you want instead of the patch to IFS, I will implement it and test
whether it fixes the bug and we will move on.

Ciao,
Johannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v2 0/2] Fix interactive rebase when the editor saves with CR/LF

2015-10-25 Thread Junio C Hamano
Johannes Schindelin  writes:

> Chad Boles reported that `git rebase -i` recently started producing
> errors when the editor saves files with DOS line endings. The symptom
> is:
>
>   Warning: the command isn't recognized in the following line:
>-
>
>   You can fix this with 'git rebase --edit-todo'.
>   Or you can abort the rebase with 'git rebase --abort'.
>
> The real bummer is that simply calling `git rebase --continue` "fixes"
> it.

Two questions.

 * What does the DOS editor do to a file with ^M in the middle of a
   line, e.g. "A^MB^M^J"?

 * Is your shell ported correctly to the platform?

The latter may need a bit of elaboration.  "read a b c" is supposed
to read one line of text (where the definition of line is platform
dependent, your platform may use CRLF to signal the end of an line),
split the characters on the line (i.e. LF vs CRLF no longer matters
at this point) at $IFS characters and give them to $a $b and $c. If
the platform accepts CRLF as the EOL signal, should the program still
see CR at the end of $c?

A solution that mucks with IFS smells like fixing a wrong problem
without addressing the real cause.

Also IFS is used not only by "read", so munging it globally doubly
feels wrong.

In addition, you do not want to split at CR; what you want is to
treat CRLF (i.e. not a lone CR) as the end-of-line separator.
Adding CR to IFS feels triply wrong.

By the way, saying "This is right, really" makes you sound as if you
do not have a real argument.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html