Repository: elixir-gettext/gettext
Pull request #283 (2021-08-29 14:56:01)

Preserve fuzzy Flag when merging entries
Fix for #282

josevalim: I *think* this looks good to me. The only flags we support are `fuzzy` and `elixir-format` and it makes sense for us to merge both of them.

The only possible downside is if someone is using gettext for multiple languages and they accidentally mark some `elixir-format` with the invalid format, and now they can't rely on the merger to fix that for them. We could address this by unioning only the `fuzzy` flag, but I am not sure if this is worth it.

Thoughts @whatyouhide?
maennchen: @josevalim I was thinking about the elixir-format flag as well. I wasn't sure on how to deal with it.

If we do not want it included, I would suggest excluding it instead of only including fuzzy.
whatyouhide: @maennchen this makes sense to me conceptually. Can you check how GNU gettext behaves here? That's what I usually do when in doubt. `pomerge` to the rescue :) Let me know if you need help!
whatyouhide: @maennchen this makes sense to me conceptually. Can you check how GNU gettext behaves here? That's what I usually do when in doubt. `pomerge` to the rescue :) Let me know if you need help!
maennchen: @whatyouhide I'm not sure if I have it correct:

https://github.com/winlibs/gettext/blob/master/source/gettext-tools/src/msgmerge.c#L1263

```c
  /* The flags are mixed in a special way.  Some informations come
     from the reference message (such as format/no-format), others
     come from the definition file (fuzzy or not).  */
  result->is_fuzzy = def->is_fuzzy | force_fuzzy;

  /* If ref and def have the same msgid but different msgid_plural, it's
     a reason to mark the result fuzzy.  */
  if (!result->is_fuzzy
      && (ref->msgid_plural != NULL
          ? def->msgid_plural == NULL
            || strcmp (ref->msgid_plural, def->msgid_plural) != 0
          : def->msgid_plural != NULL))
    result->is_fuzzy = true;
```

I would conclude from seeing that, that we should only union the fuzzy flag. Do you agree?
josevalim: Yeah, definitely. But they are treating the fuzzy flag specially, so perhaps we should do the same. :)
maennchen: @josevalim I'll update the PR accordingly.
maennchen: @josevalim I added a commit.

Patch:
diff --git a/lib/gettext/merger.ex b/lib/gettext/merger.ex
index 19ee311..0148aac 100644
--- a/lib/gettext/merger.ex
+++ b/lib/gettext/merger.ex
@@ -141,7 +141,7 @@ defmodule Gettext.Merger do
   # msgstr: new.msgstr should be empty since it comes from a POT file
   # comments: new has no translator comments as it comes from POT
   # extracted_comments: we should take the new most recent ones
-  # flags: we should take the new flags
+  # flags: we should take the new flags and preserve the fuzzy flag
   # references: new contains the updated and most recent references
 
   defp merge_two_translations(%Translation{} = old, %Translation{} = new) do
@@ -151,7 +151,7 @@ defmodule Gettext.Merger do
       msgstr: old.msgstr,
       comments: old.comments,
       extracted_comments: new.extracted_comments,
-      flags: new.flags,
+      flags: merge_flags(old.flags, new.flags),
       references: new.references
     }
   end
@@ -164,11 +164,19 @@ defmodule Gettext.Merger do
       msgstr: old.msgstr,
       comments: old.comments,
       extracted_comments: new.extracted_comments,
-      flags: new.flags,
+      flags: merge_flags(old.flags, new.flags),
       references: new.references
     }
   end
 
+  defp merge_flags(%MapSet{} = old, %MapSet{} = new) do
+    if MapSet.member?(old, "fuzzy") do
+      MapSet.put(new, "fuzzy")
+    else
+      new
+    end
+  end
+
   @doc """
   Returns the contents of a new PO file to be written at `po_file` from the POT
   template in `pot_file`.


Test patch:
diff --git a/test/gettext/merger_test.exs b/test/gettext/merger_test.exs
index aafa455..0acebf2 100644
--- a/test/gettext/merger_test.exs
+++ b/test/gettext/merger_test.exs
@@ -58,6 +58,14 @@ defmodule Gettext.MergerTest do
       assert t.comments == ["# existing comment"]
     end
 
+    test "when translations match, existing translator flags are preserved" do
+      old_po = %PO{translations: [%Translation{msgid: "foo", flags: MapSet.new(["fuzzy"])}]}
+      new_pot = %PO{translations: [%Translation{msgid: "foo"}]}
+
+      assert {%PO{translations: [t]}, _stats} = Merger.merge(old_po, new_pot, "en", @opts)
+      assert MapSet.member?(t.flags, "fuzzy")
+    end
+
     test "when translations match, existing extracted comments are replaced by new ones" do
       old_po = %PO{
         translations: [