Re: [Tails-dev] [review'n'merge:1.2] feature/7732-i2p-networ…

27/09/14 12:53, Kill Your TV wrote:
> On Fri, 26 Sep 2014 16:25:16 +0000 (UTC)
> anonym <anonym@???> wrote:
>
>> 26/09/14 14:31, Kill Your TV wrote:
>>> On Fri, 26 Sep 2014 02:46:42 +0000 (UTC)
>>> anonym <anonym@???> wrote:
>>>> Similarly we can remove
>>>> `config/chroot_local-includes/usr/share/applications/i2p.desktop*`
>>>> to drop the also obsolete Applications -> Internet -> i2p .desktop
>>>> entry.
>>>
>>> In the I2P browser branch I changewd that desktop entry to point to
>>> the browser. I wasn't sure that I could delete it here without
>>> causing a problem when/if the other branch (the I2P browser one) is
>>> merged in.
>>
>> I see. In general, when working on multiple related branches, it's
>> better if you try to keep the inter-dependencies to a minimum, since
>> it's not necessary that all of them get merged at the same time.
>
> Indeed, in practice I try to make branches 'self-contained' so that
> they can be merged (or not) without affecting any other branch, but with
> these two branches that wasn't possible without introducing merge
> conflicts. Granted, they're *easy* to solve merge conflicts, but
> conflicts nontheless.

TBH, merge-conflicts are a piece of pie in comparison to having to keep
track of exactly which branch another branch is based on, and then
figure out if that blocks merging or not. A branch should rarely be
based on any thing else than one of the following:

* master (wiki changes that wants to go live immediately)
* stable (bugfixes)
* devel (new features, or new tests for the automated test suite)

>> As for this specific issue, I suggest you re-write the history a bit
>> in feature/7725-i2p-browser and create a new i2p-browser.desktop which
>> names it "I2P-enabled Browser" (or "I2P Browser" for consistency with
>> "Tor Browser"?), and then remove i2p.desktop in
>> feature/7732-i2p-network-manager-hook.
>
> Done, other than the rewriting of feature/7725-i2p-browser's history
> since merging everything into a transient branch worked fine.

Thanks!

As to your update to the manual test suite in commit dcc88ff, it again
introduces interdependency to feature/7725-i2p-browser since it talks
about the "I2P Browser". I was merely referring to the changes w.r.t.
notifications when I2P is starting. In fact, I think you should rewrite
the history of feature/7732-i2p-network-manager-hook and *move* commit
dcc88ff into feature/7725-i2p-browser. I'll take care of these tests,
and any potential merge conflicts by merging in commit dcc88ff.

Please have a look at the feature/7732-i2p-network-manager-hook I just
pushed (based on the commit before dcc88ff). It refactors your code
quite heavily, and confines the logic into one place, so it's easier to
follow. As for the new I2P shell library I believe its
i2p_router_console_is_ready() can be handy in your tor-browser script
instead of the custom `netstat ... | grep ...`.

I'm sorry for not being very atomic with the commits, as I sneaked in
some unrelated changes along the way in the "refactor" commit. Any way,
what do you think? Would you merge this? :) BTW, I've personally tested
this branch according to the new manual test steps of this branch.

Cheers!