Thread: Git repo problem?

Hi,

Today I saw an wired error message when I tried to push:

t-ishii$ t-ishii$ socksify git push
X11 forwarding request failed on channel 0
Counting objects: 8, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (8/8), done.
Writing objects: 100% (8/8), 1.36 KiB | 1.36 MiB/s, done.
Total 8 (delta 6), reused 0 (delta 0)
remote: Traceback (most recent call last):        
remote:   File "hooks/post-receive", line 437, in <module>        
remote:     while parse_commit_log(do_send_mail, lines):        
remote:   File "hooks/post-receive", line 209, in parse_commit_log        
remote:     l = lines.pop().decode('utf8', errors='ignore').strip()        
remote: AttributeError: 'str' object has no attribute 'decode'        
To ssh://git.postgresql.org/pgpool2.git
   9d7c5baf..d1a2e366  V3_7_STABLE -> V3_7_STABLE

It seems the commit was pushed successfully
(https://git.postgresql.org/gitweb/?p=pgpool2.git;a=commit;h=b9ee8417ea74798c108498849d453e693246c3a1)
but the commit mail did not delivered.

Is there anything I need to care?
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

> That sounds a lot like the bug that Stephen bumped into during pgcon, and
> that we haven't fully tracked down yet. It only makes the commit email
> break, everything else should work as usual, so it's not that bad, but it
> should be fixed of course.
> 
> Did that push by any chance include a merge commit? That's our current
> theory on what's triggering the problem...

Yes. I pushed:
https://git.postgresql.org/gitweb/?p=pgpool2.git;a=commit;h=b9ee8417ea74798c108498849d453e693246c3a1
and
https://git.postgresql.org/gitweb/?p=pgpool2.git;a=commit;h=d1a2e366afeee87493db3463baa6660f9671e5d6

The latter is a merge commit.
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

Greetings,

* Tatsuo Ishii (ishii@sraoss.co.jp) wrote:
> > That sounds a lot like the bug that Stephen bumped into during pgcon, and
> > that we haven't fully tracked down yet. It only makes the commit email
> > break, everything else should work as usual, so it's not that bad, but it
> > should be fixed of course.
> >
> > Did that push by any chance include a merge commit? That's our current
> > theory on what's triggering the problem...
>
> Yes. I pushed:
> https://git.postgresql.org/gitweb/?p=pgpool2.git;a=commit;h=b9ee8417ea74798c108498849d453e693246c3a1
> and
> https://git.postgresql.org/gitweb/?p=pgpool2.git;a=commit;h=d1a2e366afeee87493db3463baa6660f9671e5d6
>
> The latter is a merge commit.

It turns out that the issue seems to be when there is no diffstat for a
given commit and the last line read is appended back to the list:

        if not l.startswith(" "):
            # If there is no starting space, it means there were no stats rows,
            # and we're already looking at the next commit. Put this line back
            # on the list and move on
            lines.append(l)
            break

This ends up adding a regular str (and not a byte string) to the list,
which blows up later when we try to do a 'decode' on it, in the next
pass.

Instead, I believe this should be:

        if not l.startswith(" "):
            # If there is no starting space, it means there were no stats rows,
            # and we're already looking at the next commit. Put this line back
            # on the list and move on
            lines.append(l.encode('utf-8'))
            break

Local testing showed that to fix it, but I'd like to give Magnus an
opportunity to review and confirm that this fix makes sense before
changing things on the git server.

Thanks!

Stephen

Greetings,

* Magnus Hagander (magnus@hagander.net) wrote:
> Nice spot. I think you have at least found the issue, but I think it may
> not be the best fix. Given that when we decode the string we do it with
> errors=ignore, we might loose data. Does the attached patch fix it in your
> tests as well? Instead of encoding/recoding, it just sticks the old line
> back on the list (which also matches the comment).

Tested and yes, this patch fixes it, and I agree that it makes more
sense than the approach I was using.

Would be great to get it deployed soon. :)

Thanks!

Stephen