Tails

#1 Updated by Anonymous 2016-11-19 18:25:22

• Assignee set to intrigeri
• % Done changed from 0 to 10
• QA Check set to Ready for QA
• Feature Branch set to 451f:tails/feature/11967+refresh_translations

Please review for corner cases and escapes that I might have overlooked.

#2 Updated by intrigeri 2016-11-19 20:40:54

• Assignee deleted (intrigeri)
• % Done changed from 10 to 60
• QA Check changed from Ready for QA to Dev Needed

> Please review for corner cases and escapes that I might have overlooked.

Great! It looks like it does the job. I mostly have comments about the shell programming.

In:

if [ $(diff "${locale}.po" "${locale}.po.new" | grep -c ^@ | wc -l) -eq "1" ]; then

I think that the output of wc will always be “1”. Besides, wc is not needed since you’re doing grep -c. And let’s avoid quoting “1” while we’re doing number comparison (-eq). All in all something like this (untested!) should be enough:

if [ $(diff "${locale}.po" "${locale}.po.new" | grep -c '^@') -eq 1 ]; then

It seems to me that even if ${locale}.po.new does not exist, the code diffs it with {locale}.po.
Using [ -f ${locale}.po.new ] || continue earlier to avoid doing that should save some time.

grep 'POT-Creation-Date' is not enough to ensure “Only header changes in potfile”. E.g. we might use the ‘POT-Creation-Date’ string elsewhere, e.g. in the contribute/ directory. We’ll need a stricter regexp that matches all the lines we want, and nothing else.

In fi; you don’t need ; since a newline follows.

The last rm -f ${locale}.po.new in compare_po_headers() seems unnecessary.

compare_po_headers could have a name that expresses better what it does: its current name suggests it just “compares” stuff, while it renames files and stuff. But actually, forget it: the added code should simply go into intltool_update_po — it basically makes PO files update more clever.

#4 Updated by Anonymous 2016-11-20 09:31:47

• QA Check changed from Dev Needed to Ready for QA

Updated it again.

#5 Updated by intrigeri 2016-11-20 10:48:49

• QA Check changed from Ready for QA to Dev Needed

> Updated it again.

“New PO file for ${locale} does not exist. Skipping.” does not exactly match what the test does.

Let’s use grep -c instead of @ grep | wc -l@.

I’m don’t think that counting occurrences of ^"> " is enough: what if there are two chunks in the diff, one that is about POT-Creation-Date, and the other that only removes lines and adds none. Then it seems that the current code will believe “Only header changes in potfile”, which is untrue. We should make sure that exactly 1 line is removed, exactly 1 line is added, and both those lines match the regexp ^'(?:>|<) "POT-Creation-Date:' (that might need grep -E).

s/Delete/delete/

-f is not needed in rm -f ${locale}.po.new since we already checked that the file exists.

All right!

#6 Updated by Anonymous 2016-11-20 11:39:55

• QA Check changed from Dev Needed to Ready for QA

(Sidenote: tails.pot is changed during the operation.)

#7 Updated by intrigeri 2016-11-20 12:01:52

• QA Check changed from Ready for QA to Dev Needed

Almost there!

On top of what its message says, commit:a41f865fcaad90a658c72df6c91af038f64e1231 seems to add useless parenthesis to a couple regexps. Maybe remove them?

The nested if logic is wrong: the “else” clause won’t be hit in some situations where it should. I suggest using “&&” to express the condition, instead of nesting if’s.

We’re now checking that “exactly 1 line is removed, exactly 1 line is added, and at least one line matches the regexp ^‘(?:>|<) “POT-Creation-Date:‘“, while we should check that ”exactly 1 line is removed, exactly 1 line is added, and both those lines match the regexp ^’(?:>|<) ”POT-Creation-Date:’”.

Also, let’s add the PO files report to language_statistics.sh before we merge this branch.

#8 Updated by Anonymous 2016-11-20 13:17:56

• Assignee set to intrigeri
• QA Check changed from Dev Needed to Ready for QA

#9 Updated by intrigeri 2016-11-20 14:48:17

• Assignee deleted (intrigeri)
• QA Check changed from Ready for QA to Dev Needed

In if [ ! -f ${locale}.po.new -a ! -f ${locale}.po ]; then, I think that we want -o instead of -a: if any of these two files does not exist, then there’s not much useful work the rest of this function can do.

“New PO file for ${locale} does not exist” matches neither what the code currently does, nor what it should IMO do (see above).

How about:

[ -f ${locale}.po ]     || continue
[ -f ${locale}.po.new ] || continue

?

I don’t understand why the question mark is there in ^'?>', ^'?<'. But perhaps it comes from when you had parenthesis in there, and “?” was meant to be “?:”, which would have been a good idea there.

Similarly, instead of ^'(?>|<), I guess you meant ^'(?:>|<). Which would be simpler like this, BTW: '^[<>].

To end with, I prefer the “^” to be inside the single quotes: in some shells, it is a special char that’s interpreted, so it makes it easier to test the code if it’s protected inside single quotes

#10 Updated by Anonymous 2016-11-20 14:59:40

• Assignee set to intrigeri
• QA Check changed from Dev Needed to Ready for QA

Similarly, instead of ^'(?>|<), I guess you meant ^'(?:>|<). Which would be simpler like this, BTW: '^[<>].

Just as a side note: this does not give the same result:

$ diff de.po de.po.new | grep -Ec '^(?:>|<) "POT-Creation-Date:'
1
$ diff de.po de.po.new | grep -Ec '^[<>] "POT-Creation-Date:'
2

Using the second version now.

#13 Updated by intrigeri 2016-11-20 15:52:10

• Status changed from In Progress to Resolved
• % Done changed from 60 to 100

Applied in changeset commit:3e60303b71adb6beee35db8c64f89167a39a96f8.

#14 Updated by intrigeri 2017-03-13 09:23:43

• Assignee deleted (intrigeri)
• QA Check changed from Ready for QA to Pass