Latest builtin-commit series

Latest builtin-commit series

[Kristian Høgsberg]

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.

Latest builtin-commit series

[Kristian Høgsberg]

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)

Re: Latest builtin-commit series

[Alex Riesen]

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?).

Re: Latest builtin-commit series

[Johannes Schindelin]

Hi,

On Tue, 18 Sep 2007, Alex Riesen wrote:

> 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?).

No problem.  At least in msysGit.

Ciao,
Dscho

Re: Latest builtin-commit series

[Junio C Hamano]

Kristian Høgsberg <krh@redhat.com> writes:

> 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.

I do not find the "remove trailing newline" so odd.  Didn't it
come because you sometimes needed to _not_ add it?

>       * 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.

We've had a few threads on the list about what to do with:

	GIT_EDITOR='emacs -nw'

I think David's 08874658b450600e72bb7cb0d0747c1ec4b0bfe1
(git-sh-setup.sh: make GIT_EDITOR/core.editor/VISUAL/EDITOR
accept commands) is a sensible to deal with this issue, and
prefer to see the same semantics kept in the C version.

Re: Latest builtin-commit series

[Pierre Habouzit]

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

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

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

Re: Latest builtin-commit series

[Kristian Høgsberg]

On Tue, 2007-09-18 at 15:25 -0700, Junio C Hamano wrote:
> Kristian Høgsberg <krh@redhat.com> writes:
> 
> > 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.
> 
> I do not find the "remove trailing newline" so odd.  Didn't it
> come because you sometimes needed to _not_ add it?

All users add a trailing newline if it's not present after stripping.

> >       * 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.
> 
> We've had a few threads on the list about what to do with:
> 
> 	GIT_EDITOR='emacs -nw'
> 
> I think David's 08874658b450600e72bb7cb0d0747c1ec4b0bfe1
> (git-sh-setup.sh: make GIT_EDITOR/core.editor/VISUAL/EDITOR
> accept commands) is a sensible to deal with this issue, and
> prefer to see the same semantics kept in the C version.

Ok, I'll take a look at that commit.

Kristian

Re: Latest builtin-commit series

[Junio C Hamano]

This series somewhat collided with Pierre's strbuf clean-ups.  I
queued it in 'pu' for now after fixing the conflicts up.

I haven't looked at the code enough to give any meaningful
comments yet, but I agree with most of the comments Dscho and
others raised on the topic already.