Manage incorrect/mismatched string encoding gracefully

From inside your project directory, if you type 'git config -l' does the value of user.email match a confirmed e-mail address in your drupal.org account? See http://drupal.org/node/636570 for details.

Thanks for checking the e-mail address and for the additional information. I'll see if @sdboyer can take a look.

By the way, the equivalent of http://drupal.org/cvs is, I believe, http://drupal.org/commitlog

Sounds like some problem in the workers, I mean the parsing not triggered, or triggered and failed.

Fetching logs manually should solve the problem(breaking lock?), but that's a consequence solver, not really the problem solution, but should help here.

I do not have enough privileges on d.o to check this, so yeah, please take a look at this Sam.

@pkiraly - that definitely indicates a critical issue. I'll be looking into this as soon as I can.

Looks like a worker died in the middle of parsing your push, somehow; the repo was locked. Generally speaking this is a known problem, though it happens quite rarely - based on current push stats (around 6k since launch) and the number of locked repos (2 that I cleared), ~0.03% of the time. Something goes wrong with a worker, the parse job crashes, and our unsophisticated locking system never clears the lock. Though we have a bunch of issues open for means of resolving this in a more automated way, we haven't gotten to them yet - they fell to the back burner since this is a pretty rare occurrence.

In any case, I've cleared the lock, so the next push you make should fix everything up. When I have some time, I'll try to investigate what could have caused the problem.

Yeah, parse jobs on your project (the things that read the incoming pushes you make) keep blowing up somewhere midway through parsing your data. Something in it is causing an error. Hopefully I can set marvil07 on your repo and figure out what it is...in the meantime, sorry, please bear with us :/

moving & reassigning

Interesting. No reason the commas would cause a problem, though.

The fact that marvil07 could parse your repo from clean indicates something probably got messed up partway through one parse job that's causing problems in all the subsequent ones. Not a big problem to fix - we purge it and re-parse from scratch - but I'm not 100% confident in the logic path that does that work just yet, so I'd rather wait until we fix it properly before I try it out on your stuff. It's the current thing I'm working on, but it's got several stages, so probably by the end of the week. Given that this isn't actually preventing you from using Git (even though it is preventing you from rolling releases), I'm thinking that's an acceptable delay.

Yep, I'm working on it. Not a lot anyone can do but me atm, unfortunately, but you're not the only one waiting. I'm still trying for the end of the day today.

tagging so i remember to reparse when i've finished this

Now that I finally got the changes in and can reparse from scratch...this repo still fails. Tried it locally, and it fails. And here's the error:

WD php: PDOException: SQLSTATE[HY000]: General error: 1366 Incorrect string value: '\xE1ly P\xE9...' for column 'author_name' at row 1: INSERT INTO {versioncontrol_git_operations} (vc_op_id,        [error]
author_name, committer_name, parent_commit, merge) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4); Array
(
    [:db_insert_placeholder_0] => 1722
    [:db_insert_placeholder_1] => Kir?ly P?ter
    [:db_insert_placeholder_2] => Kir?ly P?ter
    [:db_insert_placeholder_3] => 81afa8ca70104125b081b8c1170e662a8d1a0b3e
    [:db_insert_placeholder_4] => 0
)
 in VersioncontrolGitOperation->backendInsert() (line 22 of /Users/sdboyer/ws/dd/do/sites/all/modules/versioncontrol_git/includes/VersioncontrolGitOperation.php).

So, well, there's yer problem. Now we gotta figure out how to take care of it.

For the record, the problem commit is 7c78114258d837147129527052e2b9547ac102d7:

 commit 7c78114258d837147129527052e2b9547ac102d7
 Merge: 9dbd264 347e129
 Author: Kir<E1>ly P<E9>ter <pkiraly@tesuji.eu>
 Date:   Tue Mar 1 19:59:41 2011 +0100
 
     Merge branch '6.x-1.x' of git.drupal.org:project/xc into 6.x-1.x
 

That's UTF-8 incorrectly encoded with ISO-8859-1. Ugh.

Well, calling utf8_encode() on the offending values solves *this* problem, but not necessarily the whole problem. It highlights that we clearly CLEARLY need to be more robust in our input filtering. We're pretty insulated with this case (since it can only kill workers), but this is actually an attack vector. I don't understand string encoding well enough to know what the right solution is here, but we need to figure it out quick and apply it to everything that can is a potential entry point for these encoding mismatches.

I have been trying to solve this, but I actually could not find the general way Sam mentioned.

Let me explain a little more.

Git is encoding agnostic by design(mentioned in git manuals), so several encodings are accepted if the user decide to use them. Naturally by default Git uses utf-8.

Several git sub-commands have a --encoding option that try to re-encode as the encoding passed in the parameter, but sadly it only applies to the commit message, not to the other values included on the custom formats. It is exactly the same for the %e placeholder available from several git sub-commands, since it refers to the encoding of the commit message.

In this specific case where this bug has been found, the encoding used for user.name seems to be ISO-8859-1, then using utf8_encode() works fine converting it to utf-8, but I did not tried with other encodings, so I am not completely sure if we can trust completely on utf8_encode() since I do not really know how that behaves.

In the other side I also investigated a little about drupal functions dealing with these problems, but the only candidate I found could be useful is drupal_convert_to_utf8(), but it requires that we pass it the encoding of the string we want to convert, and we really do not have a way to detect the encoding, so we can not use it(again %e placeholder only refers to commit message).

So, I am attaching the patch that fix the original bug here, but probably do not solve it if a non ISO-8859-1 and non UTF-8 string is passed.

Any suggestion is welcome.

Another option, that do not really do the job, but can prevent us to attempt to insert unreadable characters on the database is using transliteration_get() from transliteration module, that will leave any character we decide instead of the non-utf8-encoded character, by default a ?.

Please note that transliteration_get() expects a UTF-8 string as the first parameter, so probably this is a bad idea, but not completely sure, so I just wanted to mention it.

re: detecting encoding: mb_detect_encoding() is the sort of thing we want...but it isn't a by-default enabled extension. iconv is enabled by default, but doesn't do detection. Sorry, I should have linked to that in the first place. I'm ok with making us just require it tbh, though if we do that we need to add a hook_requirements() thing to note the requirement.

If we solve the problem the way you've proposed in the patch, we end up stacking in thousands, even millions of additional function calls in large repos since we have to call it per line of output. A very good reason to switch over to stream-based output from our commands, instead of this rather icky line-by-line approach. That doesn't need to be a blocker to an immediate fix necessarily (especially given upcoming improvements to the sync system), but it is really a good reason to want to improve it.

Just doing that alone won't help; you have to excise the offending commit. The proper order would be:

1) delete the branch on d.o
2) excise the commits
3) re-push the fixed branch to d.o

You have to do it that way because we don't allow non-ff updates. I could help you with that at some point in IRC, but I'm pretty swamped right now, it'd be better if someone else could do it.

Note that currently, the 6.x-1.x dev release is not associated with *any* branch. Yuck.

But that means that we could create a new dev release; also means that we could possibly delete and recreate 6.x-1.x

Better title

Un-assigning this until I actually start working on it. Please feel free to continue it.

tagging, see #484382-8: Get 6.x-2.x branch of vcgit to a beta release

Just drop non-utf8 characters. The way utf-8 is built you can always recover from non utf-8 sequences.

Edit: iconv("UTF-8", "UTF-8//IGNORE", $text)

Committed a fix that incorporated iconv. It'll work at least for now, we can improve it more later.

tagging for deployment

This is now deployed.