Re: [Bug] git branch -v has problems with carriage returns

[Junio C Hamano]

Atousa Duprat  writes:

> Here is my first attempt at fixing the issue.
>
> There are two problems in ref-filter.c:
>
> First, copy_subject() has been modified to turn '\n' into a space and
> every other ascii control character to be ignored.
>
> Second, find_subpos() doesn't realize that a line that only contains a
> '\r\n' is a blank line – at least when using crlf convention.
> I have changed things so that a sequence of either '\n' or "\r\n"
> separate the subject from the body of the commit message.
> I am not looking at the crlf setting because it doesn't seem like a
> useful distinction – when one would we ever care for \r\n not to be a
> blank line?  But it could be done...
>
> Both fixes are minimal, but it feels like they are a issues with the
> specific encoding.  Does git mandate ascii or utf-8 commit messages?
> If not, there may be a larger issue here with encodings and line-end
> conventions at the very least in ref-filter.c
> Guidance would be appreciated for how to deal with this issue...
>
> Patch attached.

Doesn't this share the problem I pointed out in the other attempt in

  https://public-inbox.org/git/xmqqy3tppu13@gitster.mtv.corp.google.com/

In short, any patch that special cases CR will not get the original
behaviour back correctly.  The original never special cased CR; it
stripped isspace() at the end of lines, and turning of CRLF into LF
is merely just one effect of it.

You can apply the attached patch and see what "git branch -v"
produces.  We should only see "one", not "one line3_long line4", for
branch-two in the output.  Then replace the SP between %s and \n in
the printf thing with \r and repeat the experiment.

Thanks.

 t/t3203-branch-output.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index a428ae6703..79b80a2d3f 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -13,7 +13,8 @@ test_expect_success 'make commits' '
 
 test_expect_success 'make branches' '
git branch branch-one &&
-   git branch branch-two HEAD^
+   git branch branch-two $(printf "%s \n" one "" line3_long line4 |
+git commit-tree HEAD:)
 '
 
 test_expect_success 'make remote branches' '

Re: [Bug] git branch -v has problems with carriage returns

[Stefan Beller]

On Tue, May 30, 2017 at 10:32 PM, Atousa Duprat  wrote:
> Here is my first attempt at fixing the issue.

Cool you're looking into this. :)

>
> There are two problems in ref-filter.c:
>
> First, copy_subject() has been modified to turn '\n' into a space and
> every other ascii control character to be ignored.
>
> Second, find_subpos() doesn't realize that a line that only contains a
> '\r\n' is a blank line – at least when using crlf convention.
> I have changed things so that a sequence of either '\n' or "\r\n"
> separate the subject from the body of the commit message.
> I am not looking at the crlf setting because it doesn't seem like a
> useful distinction – when one would we ever care for \r\n not to be a
> blank line?  But it could be done...
>
> Both fixes are minimal, but it feels like they are a issues with the
> specific encoding.  Does git mandate ascii or utf-8 commit messages?
> If not, there may be a larger issue here with encodings and line-end
> conventions at the very least in ref-filter.c
> Guidance would be appreciated for how to deal with this issue...
>
> Patch attached.
>

Please read Documentation/SubmittingPatches
(tl;dr:
(a) please sign your patch, read https://developercertificate.org/
(b) if possible please send patches inline instead of attached)

> diff --git a/ref-filter.c b/ref-filter.c
> index 3a640448f..bc573f481 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -836,11 +836,15 @@ static const char *copy_email(const char *buf)
>  static char *copy_subject(const char *buf, unsigned long len)
>  {
>  char *r = xmemdupz(buf, len);
> -int i;
> +int i, j;
>
> -for (i = 0; i < len; i++)
> +for (i = 0, j = 0; i < len; i++, j++)
>  if (r[i] == '\n')
> -r[i] = ' ';
> +r[j] = ' ';
> +else if (r[i] < 32)
> +j--; // skip ascii control characters that are not '\n'

/*
 * Our comment style uses the other way,
 * as it is compatible with more compilers, still.
 */

This seems to solve a different problem than the carriage return
discussed? So it could go into a separate patch.


> +else r[j] = r[i];
> +r[j]=0;
>
>  return r;
>  }
> @@ -956,9 +960,12 @@ static void find_subpos(const char *buf, unsigned long 
> sz,
>  eol++;
>  buf = eol;
>  }
> +

stray new line?

>  /* skip any empty lines */
>  while (*buf == '\n')
>  buf++;
> +while (*buf == '\r' && *(buf+1) == '\n')
> +buf += 2;

This first skips LF empty lines and then skips CRLF empty
lines. What if they are mixed? I'd think if we extend the
empty line detection we'd want to robust to such as well,
so maybe

while (*buf == '\r' || *buf == '\n')
buf++;

Maybe this is a bit too greedy?

Re: [Bug] git branch -v has problems with carriage returns

[Atousa Duprat]

Here is my first attempt at fixing the issue.

There are two problems in ref-filter.c:

First, copy_subject() has been modified to turn '\n' into a space and
every other ascii control character to be ignored.

Second, find_subpos() doesn't realize that a line that only contains a
'\r\n' is a blank line – at least when using crlf convention.
I have changed things so that a sequence of either '\n' or "\r\n"
separate the subject from the body of the commit message.
I am not looking at the crlf setting because it doesn't seem like a
useful distinction – when one would we ever care for \r\n not to be a
blank line?  But it could be done...

Both fixes are minimal, but it feels like they are a issues with the
specific encoding.  Does git mandate ascii or utf-8 commit messages?
If not, there may be a larger issue here with encodings and line-end
conventions at the very least in ref-filter.c
Guidance would be appreciated for how to deal with this issue...

Patch attached.


Atousa


On Fri, May 19, 2017 at 11:48 PM, Johannes Sixt  wrote:
> Am 19.05.2017 um 23:55 schrieb Atousa Duprat:
>>
>> I have tried to repro this issue but git goes out of its way to store
>> the commit messages using unix end-of-line format.
>> I think that git itself cannot create a repo exhibiting this problem.
>
>
> Here is a recipe to reproduce the error:
>
>   git init
>   git commit --allow-empty -m initial
>   git branch crlf $(printf '%s\r\n' subject '' line3_long line4 |
>git commit-tree HEAD:)
>
> The reason for the "bug" is obviously that a line having CR in addition to
> LF is not "an empty line". Consequently, the second line is not treated as a
> separator between subject and body, whereupon Git concatenates all lines
> into one large subject line. This strips the LFs but leaves the CRs in tact,
> which, when printed on a terminal move the cursor to the beginning of the
> line, so that text after the CRs overwrites what is already in the terminal.
>
> This is just to give you a head start. I'm not going to look into this.
>
> -- Hannes
>
>
>>> If I do `git branch -v` with such a subject line somehow the third and
>>> second line get combined before the hash. Example:
>>>
>>> $ git branch -v
>>> See merge request ! temp space 84e18d22fd Merge branch
>>> 'feature-XXX' into 'develop'
>>> #  >> third)>  
>>>
>>> Before git v2.13.0 `git branch -v` worked completely normal.


branch-crlf.patch
Description: Binary data

Re: [Bug] git branch -v has problems with carriage returns

[Johannes Sixt]

Am 19.05.2017 um 23:55 schrieb Atousa Duprat:

I have tried to repro this issue but git goes out of its way to store
the commit messages using unix end-of-line format.
I think that git itself cannot create a repo exhibiting this problem.


Here is a recipe to reproduce the error:

  git init
  git commit --allow-empty -m initial
  git branch crlf $(printf '%s\r\n' subject '' line3_long line4 |
   git commit-tree HEAD:)

The reason for the "bug" is obviously that a line having CR in addition 
to LF is not "an empty line". Consequently, the second line is not 
treated as a separator between subject and body, whereupon Git 
concatenates all lines into one large subject line. This strips the LFs 
but leaves the CRs in tact, which, when printed on a terminal move the 
cursor to the beginning of the line, so that text after the CRs 
overwrites what is already in the terminal.


This is just to give you a head start. I'm not going to look into this.

-- Hannes


If I do `git branch -v` with such a subject line somehow the third and
second line get combined before the hash. Example:

$ git branch -v
See merge request ! temp space 84e18d22fd Merge branch
'feature-XXX' into 'develop'
#

Before git v2.13.0 `git branch -v` worked completely normal.

Re: [Bug] git branch -v has problems with carriage returns

[Animi Vulpis]

No problem, thanks for taking the time to help me.

I managed to create a minimal repository that shows the bug.
(I was able to deploy gitlab-ce-v8.15.8-ce.0 from docker locally and
create the repo, create the merge request and merge it)

I created a github repository so everybody interested can use it:
https://github.com/AnimiVulpis/git-bug
A few additional informations are in the README.md inside the repository.

FYI: I also tried a lot of things to create commit messages with \r\n
but without success. git does a good job preventing this.

Based on the history of the homebrew git formula
(https://github.com/Homebrew/homebrew-core/commits/master/Formula/git.rb)
and the fact that I `brew udpate` at least once a week I am pretty
sure that this bug does not exist in
git v2.12.2

Hope that helps
Have a nice weekend
David

2017-05-19 23:55 GMT+02:00 Atousa Duprat :
> Sorry for the noise with previous response...
>
> I have tried to repro this issue but git goes out of its way to store
> the commit messages using unix end-of-line format.
> I think that git itself cannot create a repo exhibiting this problem.
>
> Most helpful would be if you could create a mini repo using gitlab.
> All it would need is one file, two branches, and a merge.
> With that in hand, it should be pretty easy to track down the problem
> and fix git.
>
> You mentioned that the previous version you were using was working
> fine, can you tell me which version that was?
> It'll help to narrow down the changes that could have affected the issue.
>
> Thanks,
>
> Atousa
>
> On Tue, May 16, 2017 at 4:22 PM, Animi Vulpis  wrote:
>> Hi,
>>
>> I upgraded to git v2.13.0 and since then git branch -v has problems
>> with carriage returns in subject lines.
>>
>> We are using gitlab (not the newest version). So this bug (It's about
>> carriage returns in auto-generated merge messages (\r\n)) is not yet
>> fixed in our version:
>> https://gitlab.com/gitlab-org/gitlab-ce/issues/31671
>> That's were the carriage returns are coming from.
>>
>> In my specific case the auto-generated merge message has three lines
>> with empty lines in between.
>> So every line ends with `\r\n\r\n`
>>
>> If I do `git branch -v` with such a subject line somehow the third and
>> second line get combined before the hash. Example:
>>
>> $ git branch -v
>> See merge request ! temp space 84e18d22fd Merge branch
>> 'feature-XXX' into 'develop'
>> #  > third)>  
>>
>> Before git v2.13.0 `git branch -v` worked completely normal.
>>
>> I was not able to create a minimal local example, because my manually
>> created \r\n in commit messages were transformed into \n\n
>>
>> Please let me know if I can provide any more information that would be 
>> helpful.
>>
>> Cheers

Re: [Bug] git branch -v has problems with carriage returns

[Atousa Duprat]

Sorry for the noise with previous response...

I have tried to repro this issue but git goes out of its way to store
the commit messages using unix end-of-line format.
I think that git itself cannot create a repo exhibiting this problem.

Most helpful would be if you could create a mini repo using gitlab.
All it would need is one file, two branches, and a merge.
With that in hand, it should be pretty easy to track down the problem
and fix git.

You mentioned that the previous version you were using was working
fine, can you tell me which version that was?
It'll help to narrow down the changes that could have affected the issue.

Thanks,

Atousa

On Tue, May 16, 2017 at 4:22 PM, Animi Vulpis  wrote:
> Hi,
>
> I upgraded to git v2.13.0 and since then git branch -v has problems
> with carriage returns in subject lines.
>
> We are using gitlab (not the newest version). So this bug (It's about
> carriage returns in auto-generated merge messages (\r\n)) is not yet
> fixed in our version:
> https://gitlab.com/gitlab-org/gitlab-ce/issues/31671
> That's were the carriage returns are coming from.
>
> In my specific case the auto-generated merge message has three lines
> with empty lines in between.
> So every line ends with `\r\n\r\n`
>
> If I do `git branch -v` with such a subject line somehow the third and
> second line get combined before the hash. Example:
>
> $ git branch -v
> See merge request ! temp space 84e18d22fd Merge branch
> 'feature-XXX' into 'develop'
> #   third)>  
>
> Before git v2.13.0 `git branch -v` worked completely normal.
>
> I was not able to create a minimal local example, because my manually
> created \r\n in commit messages were transformed into \n\n
>
> Please let me know if I can provide any more information that would be 
> helpful.
>
> Cheers

[Bug] git branch -v has problems with carriage returns

[Animi Vulpis]

Hi,

I upgraded to git v2.13.0 and since then git branch -v has problems
with carriage returns in subject lines.

We are using gitlab (not the newest version). So this bug (It's about
carriage returns in auto-generated merge messages (\r\n)) is not yet
fixed in our version:
https://gitlab.com/gitlab-org/gitlab-ce/issues/31671
That's were the carriage returns are coming from.

In my specific case the auto-generated merge message has three lines
with empty lines in between.
So every line ends with `\r\n\r\n`

If I do `git branch -v` with such a subject line somehow the third and
second line get combined before the hash. Example:

$ git branch -v
See merge request ! temp space 84e18d22fd Merge branch
'feature-XXX' into 'develop'
#

Before git v2.13.0 `git branch -v` worked completely normal.

I was not able to create a minimal local example, because my manually
created \r\n in commit messages were transformed into \n\n

Please let me know if I can provide any more information that would be helpful.

Cheers