git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Pierre Habouzit <madcoder@debian.org>
To: Alex Riesen <raa.lkml@gmail.com>
Cc: "Lukas Sandström" <lukass@etek.chalmers.se>,
	"Git Mailing List" <git@vger.kernel.org>,
	"Junio C Hamano" <junkio@cox.net>
Subject: Re: [RFC] Convert builin-mailinfo.c to use The Better String Library.
Date: Wed, 05 Sep 2007 01:01:17 +0200	[thread overview]
Message-ID: <20070904230117.GA12448@olympe.madism.org> (raw)
In-Reply-To: <20070904213857.GA21351@steel.home>

[-- Attachment #1: Type: text/plain, Size: 3505 bytes --]

On mar, sep 04, 2007 at 09:38:57 +0000, Alex Riesen wrote:
> Lukas Sandström, Tue, Sep 04, 2007 22:50:08 +0200:
> > Hi.
> > 
> > This is an attempt to use "The Better String Library"[1] in builtin-mailinfo.c
> > 
> > The patch doesn't pass all the tests in the testsuit yet, but I thought I'd
> > send it out so people can decide if they like how the code looks.
> 
> It looks uglier, but what are measurable merits? Object code size,
> perfomance hit/improvement, valgrind logs?

  Well I honestly believe that putting strbufs/bstrings in mailinfo.c
adds no value. I was going to give it a try to see how strbufs
performed, but it's just useless.

  The main problem mailinfo has, it's according to Junio that it may
sometimes truncate some things in buffers at 1000 octets, without dying
loudly. That is bad.

  _but_ there is no point in using arbitrary long string buffers to
parse a mail. Remember, a mail goes through SMTP, and SMTP is supposed
to limit its lines at 512 characters (without use of extensions at
least). Not to mention that an email address cannot be more than 64+256
chars long (or sth around that). So using variable lengths buffers is
just a waste.

  string buffers are not really (IMHO) supposed to help in parsing
tasks, and when you need to do some serious parsing, either do it by
hand or use lex, but nothing in between makes sense to me.

  OTOH, string buffers can be used in many places where git has (at
least 4 different to my current count, growing) many implementations of
always slightly different kind of buffers. I've some more patches
pending here than the one I already sent, and well, here is the
diffstat:

$ git diff --stat origin/master.. ^strbuf*
 archive-tar.c         |   67 ++++++++++++------------------------------------
 builtin-apply.c       |   29 ++++++---------------
 builtin-blame.c       |   34 ++++++++-----------------
 builtin-commit-tree.c |   59 +++++++++---------------------------------
 builtin-rerere.c      |   53 +++++++++++---------------------------
 cache-tree.c          |   57 ++++++++++++++---------------------------
 diff.c                |   25 ++++++------------
 fast-import.c         |   38 +++++++++++----------------
 mktree.c              |   26 ++++++-------------
 9 files changed, 116 insertions(+), 272 deletions(-)

  I mean, there is not even a need to show the diff to understand what
the gain is. And that was possible, because strbufs are straightforward,
and gives you the kind of controls git needs (tweaking how memory will
be allocated to avoid reallocs is part of the answer).


  A French author once said: “Il semble que la perfection soit atteinte
non quand il n'y a plus rien à ajouter, mais quand il n'y a plus rien à
retrancher.” -- Antoine de St Éxupéry[0]. IMHO git will never need any
of the bstring splits, streaming functions, tokenization or whatever,
and supporting those has necessarily led the bstring library to make
some choices that may not fit git needs. I don't really like reinventing
the wheel, but OTOH buffers and strings are often of the critical path,
and having a nice fitting buffer API is priceless.


  [0] Perfection is achieved, not when there is nothing more to add, but
      when there is nothing left to take away.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

  reply	other threads:[~2007-09-04 23:03 UTC|newest]

Thread overview: 102+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-09-04 20:50 [RFC] Convert builin-mailinfo.c to use The Better String Library Lukas Sandström
2007-09-04 21:38 ` Alex Riesen
2007-09-04 23:01   ` Pierre Habouzit [this message]
2007-09-05 14:54 ` Kristian Høgsberg
2007-09-05 17:29   ` Matthieu Moy
2007-09-06  2:30     ` Miles Bader
2007-09-06  4:48     ` Dmitry Kakurin
2007-09-06  4:59       ` Shawn O. Pearce
2007-09-06  9:12         ` Andreas Ericsson
2007-09-06  9:35           ` Junio C Hamano
2007-09-06 10:21             ` Andreas Ericsson
2007-09-06  9:52           ` David Kastrup
2007-09-06  5:03       ` Miles Bader
2007-09-06 12:08         ` Johannes Schindelin
2007-09-06 17:50       ` Linus Torvalds
2007-09-07  0:21         ` Dmitry Kakurin
2007-09-07  0:38           ` Linus Torvalds
2007-09-07  1:08             ` Dmitry Kakurin
2007-09-07  1:27               ` Linus Torvalds
2007-09-07  3:09                 ` Dmitry Kakurin
2007-09-07  5:48                   ` David Symonds
2007-09-07  6:15                   ` Theodore Tso
2007-09-20 14:06                     ` Steven Burns
2007-09-20 14:56                       ` Andreas Ericsson
2007-09-07  6:31                   ` Andreas Ericsson
2007-09-07 22:17                     ` Dmitry Kakurin
2007-09-07 22:28                       ` David Kastrup
2007-09-08  0:37                         ` Dmitry Kakurin
2007-09-08  6:25                           ` David Kastrup
2007-09-09  0:29                       ` Andreas Ericsson
2007-09-07  6:52                   ` David Kastrup
2007-09-07 10:28                   ` Johannes Schindelin
2007-09-07 10:26                 ` Johannes Schindelin
2007-09-07  6:50               ` David Kastrup
2007-09-07  1:12             ` Linus Torvalds
2007-09-07  1:40               ` alan
2007-09-07  5:09               ` Walter Bright
2007-09-07  7:40                 ` David Kastrup
2007-09-07  8:15                   ` Walter Bright
2007-09-07  8:26                     ` David Kastrup
2007-09-07  9:14                       ` Walter Bright
2007-09-07  9:31                         ` David Kastrup
2007-09-07 20:22                           ` Walter Bright
2007-09-07 20:27                             ` David Kastrup
2007-09-07 23:16                               ` Walter Bright
2007-09-08 23:50                             ` Andreas Ericsson
2007-09-09  0:37                               ` Pierre Habouzit
2007-09-09  1:36                                 ` Andreas Ericsson
2007-09-07 11:36                   ` Wincent Colaiuta
2007-09-07  9:41                 ` Pierre Habouzit
2007-09-07 19:03                   ` Walter Bright
2007-09-07 19:31                     ` David Kastrup
2007-09-07 20:49                       ` Walter Bright
2007-09-07 19:41                     ` Pierre Habouzit
2007-09-07 19:51                       ` David Kastrup
2007-09-07 19:59                         ` Pierre Habouzit
2007-09-07 20:40                       ` Walter Bright
2007-09-07 20:56                         ` Pierre Habouzit
2007-09-07 22:54                           ` Walter Bright
2007-09-08  0:56               ` John 'Z-Bo' Zabroski
2007-09-08  6:36                 ` David Kastrup
2007-09-19 19:56               ` Steven Burns
2007-09-07  3:06           ` Wincent Colaiuta
2007-09-07  4:06             ` Paul Wankadia
2007-09-07  4:30               ` Nicolas Pitre
2007-09-07  9:19               ` Wincent Colaiuta
2007-09-07  6:25             ` Andreas Ericsson
2007-09-07 10:56               ` Johannes Schindelin
2007-09-07 11:54                 ` Andreas Ericsson
2007-09-07 12:33                   ` Wincent Colaiuta
2007-09-07 12:55                     ` Karl Hasselström
2007-09-07 13:58                     ` Andreas Ericsson
2007-09-07 14:13                       ` Wincent Colaiuta
2007-09-09  0:09                         ` Andreas Ericsson
2007-09-07 16:09                 ` David Kastrup
2007-09-07 11:30               ` Wincent Colaiuta
2007-09-07  8:36             ` Walter Bright
2007-09-07  9:41               ` Andreas Ericsson
2007-09-07 19:23                 ` Walter Bright
2007-09-07 19:40                   ` David Kastrup
2007-09-09  0:25                   ` Andreas Ericsson
2009-09-17 16:23                   ` Bernd Jendrissek
2007-09-07 11:52               ` Wincent Colaiuta
2007-09-07 19:25                 ` Walter Bright
2007-09-22 16:52               ` Steven Burns
2007-09-07  6:47           ` David Kastrup
2007-09-07  7:41             ` Andy Parkins
2007-09-07  8:08               ` David Kastrup
2007-09-07 10:21           ` Johannes Schindelin
2007-09-08  0:32             ` Dmitry Kakurin
2007-09-08  6:24               ` David Kastrup
2007-09-08 23:25               ` Alex Riesen
2007-09-24 13:41         ` figo
2007-09-24 13:57           ` David Kastrup
2007-09-25 19:19             ` Steven Burns
2007-09-25 19:55               ` David Kastrup
2012-05-22 18:30         ` Syed M Raihan
2010-06-10 19:12       ` Ian Molton
2010-06-11 12:23         ` Jakub Narebski
2010-06-11 13:33           ` Dario Rodriguez
2007-09-05 15:27 ` Kristian Høgsberg
2007-09-07 10:47 ` Lukas Sandström

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20070904230117.GA12448@olympe.madism.org \
    --to=madcoder@debian.org \
    --cc=git@vger.kernel.org \
    --cc=junkio@cox.net \
    --cc=lukass@etek.chalmers.se \
    --cc=raa.lkml@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).