[3011] in BarnOwl Developers
Re: Realms and outgoing messages
daemon@ATHENA.MIT.EDU (David A Benjamin)
Thu Mar 24 17:29:55 2011
Date: Thu, 24 Mar 2011 17:29:51 -0400 (EDT)
From: David A Benjamin <davidben@MIT.EDU>
To: Anders Kaseorg <andersk@mit.edu>
cc: barnowl-dev@mit.edu
In-Reply-To: <alpine.DEB.2.02.1103240104580.4468@dr-wily.mit.edu>
This message is in MIME format. The first part should be readable text,
while the remaining parts are likely unreadable without MIME-aware tools.
---1257051904-51839019-1301000753=:25741
Content-Type: TEXT/PLAIN; CHARSET=UTF-8; FORMAT=flowed
Content-Transfer-Encoding: QUOTED-PRINTABLE
Content-ID: <alpine.DEB.1.10.1103241706101.25741@dr-wily.mit.edu>
Pushed a new version of the branch to my github. Also added a new commit=20
on top to fix a problem I noticed.
Responses inline.
On Thu, 24 Mar 2011, Anders Kaseorg wrote:
>> When you zephyr multiple people without using -C, show one outgoing =
message per recipient.
>
> Can this commit subject be shortened to at most 76 characters?
>
> This would be more readable if the owl_function_make_outgoing_zephyr =E2=
=86=92
> owl_function_add_outgoing_zephyrs refactoring was split from the behavior
> change.
>
>
> + if (owl_zwrite_get_numrecips(z) > recip_index) {
>
> recip_index < owl_zwrite_get_numrecips(z) would be more consistent.
>
I would normally agree, but I am not going to split this commit. It was=20
mailed out in November and was not written by me; it was the result of an=
=20
attempt to get more people involved in BarnOwl some hackathon ago. I do=20
not wish to keep messing with it. The only reason I didn't ACK it and land=
=20
it ages ago was because
a) I was lame and not sure whether our messed code review setup allowed me=
=20
to sigh off on a patch that was written while I was around.
b) When it was clear no one else was going to review it, I went to sign=20
off and realized we messed up and this commit only fixed half the problem,=
=20
and it would be poor to land it without fc3db8bcaa8a, which I took forever=
=20
to write. (I thought I wrote an email to that effect, but evidently not?)
So, I think I'm going to sign off on this particular commit myself, if the=
=20
only complaints are stylistic. Sorry... my fault for the mess and not=20
mentioning this earlier.
I'll shorten the commit message and switch the order of the comparison,=20
although I honestly don't see the point in the latter. It is consistent=20
with the code that was modified.
>> In duplicated outgoing zephyrs only reply on the relevant recipient
>
> + } else if (recip_index < owl_list_get_size(&(z->recips))) {
> + g_string_append_c(buf, ' ');
> + owl_string_append_quoted_arg(buf, owl_list_get_element(&(z->recips=
), recip_index));
>
> Those two lines are 2 spaces overindented. The parens in &(z->recips)
> could go away.
Oops. Indentation fixed.
I'm not a huge fan of the parens, but we use it everywhere else in that=20
file, and overwhelming so in the codebase in general. Excluding libfaim, I=
=20
note:
% git grep '&([a-z_]\+->' origin/master *.c | wc -l
176
% git grep '&[a-z_]\+->' origin/master *.c | wc -l
48
I know we have an unwritten policy of fixing kretchisms as we modify the=20
code, but I'm not sure we've been too consistent about killing those.=20
There was a small drop in their down from 1.6 to 1.7, but no corresponding=
=20
increase in the non-paren'd version. I think that's more because of the=20
various refactorings that came about (owl_window, punting obarray, using a=
=20
GQueue for the message queue). Not sure it's worth instating one either=20
since they'll naturally go away as we move from init/cleanup to=20
new/delete.
Also I'm working on a branch to replace owl_list with GPtrArray anway. :-)
>> Add owl_zwrite_get_recip_n_with_realm
>
> + if (strcmp(z->realm, "")) {
> + return g_strdup_printf("%s@%s", (const char *) owl_zwrite_get_recip_=
n(z, n), z->realm);
> + } else {
> + return g_strdup_printf("%s", (const char *) owl_zwrite_get_recip_n(z=
, n));
> + }
>
> Using strcmp=E2=80=99s return as bool is confusing; =E2=80=98if (strcmp(z=
->realm, "") !=3D
> 0)=E2=80=99 or =E2=80=98if (realm[0])=E2=80=99 is clearer.
>
> Those casts can die now. Also, g_strdup_printf("%s", =E2=80=A6) is g_str=
dup(=E2=80=A6).
"That came from the code I moved." :-) But sure. Done. (...why did I miss=
=20
all that? I guess I originally did that refactor when I was doing the=20
GPtrArray branch and wasn't paying too much attention.)
(I think in general I'm not too unhappy about using strcmp as a bool;=20
!strcmp(a, b) is a pretty common idiom, but without the ! it is a bit=20
awkward, yes. Really C should have given us a strequal macro and called it=
=20
a day. Anyway, it's realm[0] now.)
>> Correctly compute the realm from zusers and zwrite
>
> This commit does three things: add and use zuser_realm; rewrite
> short_zuser and long_zuser; set the realm in
> owl_message_create_from_zwrite. Again, would have preferred to see it
> split.
Split.
> + /* TODO: We should maybe munge this into the case above, as that com=
parison
> + * is a little overly clever. It's also not clear this codepath ever=
runs
> + * anyway. */
>
> It runs if owl_zwrite_get_numrecips(z) =3D=3D 0 in
> owl_function_add_outgoing_zephyrs, right?
>
> (Does it really make sense to have a message with no recipients? Should
> those be rewritten to have recipient =E2=80=98=E2=80=99 or recipient =E2=
=80=98*=E2=80=99 or something so
> we don=E2=80=99t have weird behavior differences between those cases?)
Yeah, I was in fact going to do just that and then decided there were=20
enough subtle things involved (our code generally does the right thing on=
=20
no recipients, and not recipient ''; * acts like '', but *@REALM and * -r=
=20
REALM don't act like @REALM and '' -r REALM; how should recipient '' USER=
=20
work?, etc.), that are fixable, but would take considerable effort to get=
=20
that right. So I left a TODO instead. I think the two sets of fixes in=20
this branch are comparatively simple (and do not worsen the existing=20
situation) so it wasn't worth making them depend on that more invasive=20
cleanup.
> + owl_message_set_realm(m, strcmp(realm, "") ? realm : owl_zephyr_get_=
realm());
>
> Same strcmp comment.
Done.
---1257051904-51839019-1301000753=:25741--
