Re: [RFC/PATCH 1/4] builtin: add git-check-mailmap command
On Thu, Jul 11, 2013 at 12:50 PM, Eric Sunshine sunsh...@sunshineco.com wrote: + maybe_flush_or_die(stdout, contact to stdout); On error this function will print write failure on 'contact to stdout' maybe maybe_flush_or_die(stdout, write contact to stdout) or something? From i18n point of view, maybe_flush_or_die should not compose a sentence this way. Let the second argument be a complete sentence so that translators have more freedom. But that's a different issue. Indeed, it's not ideal. I chose contact to stdout for consistency with other callers, not because of a fondness for it. % git grep maybe_flush_or_die builtin/blame.c: maybe_flush_or_die(stdout, stdout); builtin/check-attr.c: maybe_flush_or_die(stdout, attribute to stdout); builtin/check-attr.c: maybe_flush_or_die(stdout, attribute to stdout); builtin/check-ignore.c: maybe_flush_or_die(stdout, check-ignore to stdout); builtin/check-ignore.c: maybe_flush_or_die(stdout, ignore to stdout); builtin/hash-object.c: maybe_flush_or_die(stdout, hash to stdout); builtin/rev-list.c: maybe_flush_or_die(stdout, stdout); log-tree.c: maybe_flush_or_die(stdout, stdout); They seem pretty evenly split between just stdout and foo to stdout. (I actually prefer plain stdout and will happily change it to that.) Then probably stick to the convention. If this i18n thing is fixed, it has to be fixed everywhere anyway. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 1/4] builtin: add git-check-mailmap command
On Wed, Jul 10, 2013 at 9:03 PM, Eric Sunshine sunsh...@sunshineco.com wrote:
+static void check_mailmap(struct string_list *mailmap, const char *contact)
+{
+ const char *name, *mail;
+ size_t namelen, maillen;
+ struct ident_split ident;
+ char term = null_out ? '\0' : '\n';
+
+ if (split_ident_line(ident, contact, strlen(contact)))
+ die(_(unable to parse contact: %s), contact);
+
+ name = ident.name_begin;
+ namelen = ident.name_end - ident.name_begin;
+ mail = ident.mail_begin;
+ maillen = ident.mail_end - ident.mail_begin;
+
+ map_user(mailmap, mail, maillen, name, namelen);
Would it be useful to check the return value of this function, to
display a message when the name can't mapped ?
+ if (namelen)
+ printf(%.*s %.*s%c,
+ (int)namelen, name, (int)maillen, mail, term);
+ else
+ printf(%.*s%c, (int)maillen, mail, term);
+}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 1/4] builtin: add git-check-mailmap command
On Thu, Jul 11, 2013 at 2:45 AM, Antoine Pelisse apeli...@gmail.com wrote:
On Wed, Jul 10, 2013 at 9:03 PM, Eric Sunshine sunsh...@sunshineco.com
wrote:
+static void check_mailmap(struct string_list *mailmap, const char *contact)
+{
+ const char *name, *mail;
+ size_t namelen, maillen;
+ struct ident_split ident;
+ char term = null_out ? '\0' : '\n';
+
+ if (split_ident_line(ident, contact, strlen(contact)))
+ die(_(unable to parse contact: %s), contact);
+
+ name = ident.name_begin;
+ namelen = ident.name_end - ident.name_begin;
+ mail = ident.mail_begin;
+ maillen = ident.mail_end - ident.mail_begin;
+
+ map_user(mailmap, mail, maillen, name, namelen);
Would it be useful to check the return value of this function, to
display a message when the name can't mapped ?
I thought about it but decided against the added complexity (at least
initially) for a few reasons.
There are only two callers in the existing code which even look at the
return value:
% git grep 'map_user('
builtin/blame.c: map_user(mailmap, mailbuf, maillen,
builtin/shortlog.c: map_user(log-mailmap, mailbuf, maillen,
namebuf, namelen);
pretty.c: map_user(pp-mailmap, mailbuf, maillen, namebuf, namelen);
pretty.c: return mail_map-nr map_user(mail_map, email,
email_len, name, name_len);
revision.c: if (map_user(mailmap, mail, maillen, name, namelen)) {
Of those, mailmap_name() in pretty.c merely passes the return value
along to its callers, but its callers don't care about it:
% git grep 'mailmap_name('
pretty.c: mailmap_name(mail, maillen, name, namelen);
commit_rewrite_person() in revision.c uses the return value apparently
only as an optimization to decide if it should go through the effort
of replacing the old person with the re-mapped person, and then
passes the return value along to its callers, none of which care:
% git grep 'commit_rewrite_person('
revision.c: commit_rewrite_person(buf, \nauthor , opt-mailmap);
revision.c: commit_rewrite_person(buf, \ncommitter , opt-mailmap);
The sort of optimization taken by commit_rewrite_person() is
effectively lost to script and porcelain clients of check-mailmap
since the cost of executing the command probably swamps any savings
the client might otherwise achieve by knowing whether the person was
remapped. Also, modulo possible whitespace changes, a client can
compare what it passed in to what is received back to determine if
remapping occurred.
As plumbing, the expectation is that most clients will pass a value in
and use the output without further interpretation. If check-mailmap
unconditionally adds some mapped or not mapped indicator to each
returned value, then that places extra burden on all clients by
forcing them to parse the result. Alternately, if the indicator is
controlled by a command-line option, then (at the least) that
increases documentation costs.
For people using check-mailmap as a .mailmap debugging aid, such an
indicator might indeed be useful, however, it seems prudent to keep
things simple initially by omitting the bells and whistles until
practical experience shows that more features (and complexity) are
warranted.
+ if (namelen)
+ printf(%.*s %.*s%c,
+ (int)namelen, name, (int)maillen, mail, term);
+ else
+ printf(%.*s%c, (int)maillen, mail, term);
+}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 1/4] builtin: add git-check-mailmap command
On Thu, Jul 11, 2013 at 2:03 AM, Eric Sunshine sunsh...@sunshineco.com wrote:
+static const struct option check_mailmap_options[] = {
+ OPT_BOOLEAN(0, stdin, use_stdin,
+ N_(also read contacts from stdin)),
+ OPT_BOOLEAN('z', NULL, null_out,
+ N_(null-terminate output lines)),
I think OPT_BOOLEAN is deprecated in favor of OPT_BOOL (or OPT_COUNTUP
if you really want -z -z -z to mean differently than -z)
+ maybe_flush_or_die(stdout, contact to stdout);
On error this function will print
write failure on 'contact to stdout'
maybe maybe_flush_or_die(stdout, write contact to stdout) or
something? From i18n point of view, maybe_flush_or_die should not
compose a sentence this way. Let the second argument be a complete
sentence so that translators have more freedom. But that's a different
issue.
--
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
