Menu link identifier logic is confusing

Background

Features has a 'menu_links' component that allows to export menu links.
In features, menu links are identified by an "identifier" which is generated from data in the menu link.

Long ago, the identifier used to be like "$menu_name:$link_path".
Since Oct 2013, commit afa91335ebc88a7fb04b05d578f9d74cdea453d1, the pattern is like "{$menu_name}_{$clean_title}:$link_path".
This helps to distinguish items which have the same link path and menu, but have different link title. Of course if the link title is also the same, it does not help.

Unfortunately, the commit was tagged with the wrong issue number #1597186: Do not cache included components in features_include_defaults(), which has really nothing to do with menu links.
The correct issue was #927566-74: Add link title to menu link identifiers to make them more unique., patch from #72 (thanks @ciss for pointing this out).
I find some issues that appear to discuss this problem, but I do not find any that contains the changes in afa91335eb as a patch, or as a code change suggestion in a comment.

Problems

Insufficiently explained git commit

As mentioned above, the changes where this was introduced are mixed into another commit which has nothing to do with it.

Insufficient documentation, confusing code

There are some comments, but I still had to dig deep to understand what is going on here.

E.g. the menu_links_features_identifier() has a parameter $old, but it is not documented with a @param doc.
An inline comment says "The old identifier is requested.", but this does not describe the exact behavior in either case.

Looking at the code, the $old can mean two things:
- An identifier that was already stored on the menu link, in $link['options']['identifier']. This could have the old or new format.
- A newly constructed identifier using the old format, IF no other menu links have the same identifier. I am not even sure about the exact condition, because features_menu_link_load() is also confusing.
- A newly constructed identifier using the new format, if the old format would lead to clashes.

In menu_links_features_export(), the $old parameter is provided from empty($export)

Possible bugs

1.
In features_menu_link_load(), the variable $clean_title is initialized in all cases to be always a string.

And yet, there are two cases that check isset($clean_title), probably to check if we are dealing with an old-style or new-style identifier.

2.
In menu_links_features_identifier().
If $old is TRUE, the link has no identifier, and 'menu_name' or 'link_path' are missing, the $identifier variable is set to FALSE, and then this value is passed to features_menu_link_load(). That function is not built to accept FALSE for its parameter.

3.
In menu_links_features_export(), the $old parameter is provided from empty($export).
However, this value is initialized at the top in a way that it can never be empty. So this is at least silly.

As a comparison, in menu_links_features_export_render() it is also called with empty($export) for the $old parameter, but here the $export might actually be empty.

In either case, it is not really clear what empty($export) should have to do with the identifier format. So both cases qualify as "confusing code".

Insufficient tests

This behavior is not sufficiently covered with tests, I think.

Solution

I am going to add a lot of documentation in #3060511: Enhance (function) doc comments in features.. To properly do this, I need to at least understand the current behavior.

This issue is a follow-up, it is meant for further clean-up.