Hi,
I sent out a new builtin-commit patch series last night, and figured I
should have written a cover letter to describe the changes there.
Better late than never:
* rebase to Pierres strbuf changes. Note, there is still some
strbuf tweaking required, to let stripspace work on a strbuf.
Also, I changed the semantics of stripspace to always add a
newline if the last line doesn't have one. I believe the
current odd semantics (always remove the last newline) comes
from not being able to easily add a newline, but now that it's a
strbuf, that's easy.
* Fixing the last bug that caused trouble in the test suite: even
if run_status says there's nothing to commit, proceed if we're
doing a merge.
* Set the test suite default editor to '/bin/true' instead of ':'.
Since we're not exec'ing the editor from shell anymore, ':'
won't work. Maybe we should special case ':' in launch_editor
or perhaps make launch_editor use system(3). Not sure.
* The first few patches are good to go, and if we can get them
committed to next, we can focus on the builtin-commit patch.
Specifically:
0001-Enable-wt-status-output-to-a-given-FILE-pointer.patch
0002-Enable-wt-status-to-run-against-non-standard-index-f.patch
0003-Introduce-entry-point-for-launching-add-interactive.patch
0004-Clean-up-stripspace-a-bit-use-strbuf-even-more.patch
0005-Add-strbuf_read_file.patch
0006-Export-rerere-and-launch_editor.patch
(edit our "and launch_editor" from the title)
should all be fine and easy to review, whereas
0007-Implement-git-commit-as-a-builtin-command.patch
will probably need some careful reviewing. That said, it is
feature complete, the code is nice enough and it passes the test
suite. It's just a very important part of git :)
cheers,
Kristian
(sorry about the duplicate mail)
Kristian Høgsberg, Tue, Sep 18, 2007 17:23:29 +0200:
* Set the test suite default editor to '/bin/true' instead of ':'.
Since we're not exec'ing the editor from shell anymore, ':'
won't work. Maybe we should special case ':' in launch_editor
or perhaps make launch_editor use system(3). Not sure.
Special case "" (empty string)? MinGW may have problems with
/bin/true, any future exotic ports notwithstanding (OS/2, anyone?).
Hi,
On Tue, 18 Sep 2007, Alex Riesen wrote:
Kristian H?gsberg, Tue, Sep 18, 2007 17:23:29 +0200:
quoted
* Set the test suite default editor to '/bin/true' instead of ':'.
Since we're not exec'ing the editor from shell anymore, ':'
won't work. Maybe we should special case ':' in launch_editor
or perhaps make launch_editor use system(3). Not sure.
Special case "" (empty string)? MinGW may have problems with
/bin/true, any future exotic ports notwithstanding (OS/2, anyone?).
No problem. At least in msysGit.
Ciao,
Dscho
On Tue, Sep 18, 2007 at 03:23:29PM +0000, Kristian Høgsberg wrote:
* rebase to Pierres strbuf changes. Note, there is still some
strbuf tweaking required, to let stripspace work on a strbuf.
Yeah I wondered if that would be a gain to migrate stripspace as a
first class citizen strbuf function. Note though, that if you always
want to stripspace in place, changing it is an overkill. I mean, there
isn't a lot of gain, as making it work on a strbuf is just a matter of:
strbuf_setlen(&buf, stripspaces(buf.buf, buf.len));
If you want to do sth like:
stripspaces(&buf, some_other_string);
And see the stripped version of "some_other_string" appended to the
strbuf "buf", then yes, it's not trivial to use the current stripspaces
as is.
General Note:
~~~~~~~~~~~~
As a general rule, and I say this to the list, not only to you,
strbufs should not be used everywhere. It may _look_ like I'm peeing
over all the code putting strbufs anywhere I can, it's not true.
Strbuf's can help for two things:
* the obvious: dealing with variable length strings, it's the least
they can do.
* be used as reused, variable length buffer, instead of loops that do:
for (;;) {
char *foo = xmalloc(...);
[...]
free(foo);
}
here, you can have a strbuf outside of the loop, and just reset it
at the begining of the loop. You'll then work on a buffer that will
stop beeing reallocated at some point. This make allocation patterns
better.
Strbuf's are not especially convenient when it comes to parsing.
Unlike the bstring's that have been discussed here recently, I don't
mean strbuf's to supersede all the standard C string API, because when
it comes to parsing, as soon as your buffers are properly NUL
terminated, C functions are _not_ usafe, no matter what people say.
strchr/memchr, strc?spn, strn?cmp/memcmp, ... are very efficient, and
there is little point to hide them behind stupid strbuf's APIs. Hence,
when it comes to parsing, you just fallback to a string, whose length is
known[0].
That's the reason why I won't probably be the one converting
builtin-mailinfo.c to strbuf's because there is very little point to do
so in the current state of the art.
Cheers,
[0] that allows to fallback to memchr[1] instead of strchr, which is
slightly faster I'm told.
[1] Note that you must be sure you don't have embedded NULs or memchr
and strchr won't have the same result then ;)
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org