{"thread":{"id":"9927","subject":"Latest builtin-commit series","startedAt":"2007-09-18T15:23:29Z","lastAt":"2007-09-19T23:16:55Z","messageCount":7,"participants":["Kristian Høgsberg","Alex Riesen","Johannes Schindelin","Junio C Hamano","Pierre Habouzit"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"53492","messageId":"1190129009.23692.24.camel@hinata.boston.redhat.com","threadId":"9927","inReplyTo":null,"subject":"Latest builtin-commit series","fromName":"Kristian Høgsberg","fromEmail":"krh@redhat.com","sentAt":"2007-09-18T15:23:29Z","receivedAt":"2007-09-18T15:23:29Z","isPatch":false,"sender":{"key":"krh@redhat.com","avatar":"https://gravatar.com/avatar/763dee6f9594ac474f725b137a39565792928e583ddf59b32befc2907409027e?d=mp&s=160"},"body":"Hi,\n\nI sent out a new builtin-commit patch series last night, and figured I\nshould have written a cover letter to describe the changes there.\nBetter late than never:\n\n * rebase to Pierres strbuf changes. Note, there is still some\n strbuf tweaking required, to let stripspace work on a strbuf.\n Also, I changed the semantics of stripspace to always add a\n newline if the last line doesn't have one. I believe the\n current odd semantics (always remove the last newline) comes\n from not being able to easily add a newline, but now that it's a\n strbuf, that's easy.\n\n * Fixing the last bug that caused trouble in the test suite: even\n if run_status says there's nothing to commit, proceed if we're\n doing a merge.\n\n * Set the test suite default editor to '/bin/true' instead of ':'.\n Since we're not exec'ing the editor from shell anymore, ':'\n won't work. Maybe we should special case ':' in launch_editor\n or perhaps make launch_editor use system(3). Not sure.\n\n * The first few patches are good to go, and if we can get them\n committed to next, we can focus on the builtin-commit patch.\n Specifically:\n\n 0001-Enable-wt-status-output-to-a-given-FILE-pointer.patch\n 0002-Enable-wt-status-to-run-against-non-standard-index-f.patch\n 0003-Introduce-entry-point-for-launching-add-interactive.patch\n 0004-Clean-up-stripspace-a-bit-use-strbuf-even-more.patch\n 0005-Add-strbuf_read_file.patch\n 0006-Export-rerere-and-launch_editor.patch\n (edit our \"and launch_editor\" from the title)\n \n should all be fine and easy to review, whereas \n \n 0007-Implement-git-commit-as-a-builtin-command.patch\n \n will probably need some careful reviewing. That said, it is\n feature complete, the code is nice enough and it passes the test\n suite. It's just a very important part of git :)\n\ncheers,\nKristian\n\n(sorry about the duplicate mail)\n"},{"id":"53505","messageId":"20070918213903.GA14488@steel.home","threadId":"9927","inReplyTo":"1190129009.23692.24.camel@hinata.boston.redhat.com","subject":"Re: Latest builtin-commit series","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-09-18T21:39:03Z","receivedAt":"2007-09-18T21:39:03Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Kristian Høgsberg, Tue, Sep 18, 2007 17:23:29 +0200:\n> * Set the test suite default editor to '/bin/true' instead of ':'.\n> Since we're not exec'ing the editor from shell anymore, ':'\n> won't work. Maybe we should special case ':' in launch_editor\n> or perhaps make launch_editor use system(3). Not sure.\n\nSpecial case \"\" (empty string)? MinGW may have problems with\n/bin/true, any future exotic ports notwithstanding (OS/2, anyone?).\n"},{"id":"53510","messageId":"Pine.LNX.4.64.0709182324230.28395@racer.site","threadId":"9927","inReplyTo":"20070918213903.GA14488@steel.home","subject":"Re: Latest builtin-commit series","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-09-18T22:24:36Z","receivedAt":"2007-09-18T22:24:36Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 18 Sep 2007, Alex Riesen wrote:\n\n> Kristian H?gsberg, Tue, Sep 18, 2007 17:23:29 +0200:\n> > * Set the test suite default editor to '/bin/true' instead of ':'.\n> > Since we're not exec'ing the editor from shell anymore, ':'\n> > won't work. Maybe we should special case ':' in launch_editor\n> > or perhaps make launch_editor use system(3). Not sure.\n> \n> Special case \"\" (empty string)? MinGW may have problems with\n> /bin/true, any future exotic ports notwithstanding (OS/2, anyone?).\n\nNo problem. At least in msysGit.\n\nCiao,\nDscho\n"},{"id":"53511","messageId":"7vbqbzvcf4.fsf@gitster.siamese.dyndns.org","threadId":"9927","inReplyTo":"1190129009.23692.24.camel@hinata.boston.redhat.com","subject":"Re: Latest builtin-commit series","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-09-18T22:25:35Z","receivedAt":"2007-09-18T22:25:35Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kristian Høgsberg writes:\n\n> Better late than never:\n>\n> * rebase to Pierres strbuf changes. Note, there is still some\n> strbuf tweaking required, to let stripspace work on a strbuf.\n> Also, I changed the semantics of stripspace to always add a\n> newline if the last line doesn't have one. I believe the\n> current odd semantics (always remove the last newline) comes\n> from not being able to easily add a newline, but now that it's a\n> strbuf, that's easy.\n\nI do not find the \"remove trailing newline\" so odd. Didn't it\ncome because you sometimes needed to _not_ add it?\n\n> * Set the test suite default editor to '/bin/true' instead of ':'.\n> Since we're not exec'ing the editor from shell anymore, ':'\n> won't work. Maybe we should special case ':' in launch_editor\n> or perhaps make launch_editor use system(3). Not sure.\n\nWe've had a few threads on the list about what to do with:\n\n\tGIT_EDITOR='emacs -nw'\n\nI think David's 08874658b450600e72bb7cb0d0747c1ec4b0bfe1\n(git-sh-setup.sh: make GIT_EDITOR/core.editor/VISUAL/EDITOR\naccept commands) is a sensible to deal with this issue, and\nprefer to see the same semantics kept in the C version.\n"},{"id":"53513","messageId":"20070918223119.GA4535@artemis.corp","threadId":"9927","inReplyTo":"1190129009.23692.24.camel@hinata.boston.redhat.com","subject":"Re: Latest builtin-commit series","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-09-18T22:31:19Z","receivedAt":"2007-09-18T22:31:19Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Tue, Sep 18, 2007 at 03:23:29PM +0000, Kristian Høgsberg wrote:\n> * rebase to Pierres strbuf changes. Note, there is still some\n> strbuf tweaking required, to let stripspace work on a strbuf.\n\n Yeah I wondered if that would be a gain to migrate stripspace as a\nfirst class citizen strbuf function. Note though, that if you always\nwant to stripspace in place, changing it is an overkill. I mean, there\nisn't a lot of gain, as making it work on a strbuf is just a matter of:\n\n strbuf_setlen(&buf, stripspaces(buf.buf, buf.len));\n\nIf you want to do sth like:\n\n stripspaces(&buf, some_other_string);\n\nAnd see the stripped version of \"some_other_string\" appended to the\nstrbuf \"buf\", then yes, it's not trivial to use the current stripspaces\nas is.\n\n\nGeneral Note:\n~~~~~~~~~~~~\n\n As a general rule, and I say this to the list, not only to you,\nstrbufs should not be used everywhere. It may _look_ like I'm peeing\nover all the code putting strbufs anywhere I can, it's not true.\nStrbuf's can help for two things:\n * the obvious: dealing with variable length strings, it's the least\n they can do.\n * be used as reused, variable length buffer, instead of loops that do:\n for (;;) {\n char *foo = xmalloc(...);\n [...]\n free(foo);\n }\n here, you can have a strbuf outside of the loop, and just reset it\n at the begining of the loop. You'll then work on a buffer that will\n stop beeing reallocated at some point. This make allocation patterns\n better.\n\n Strbuf's are not especially convenient when it comes to parsing.\nUnlike the bstring's that have been discussed here recently, I don't\nmean strbuf's to supersede all the standard C string API, because when\nit comes to parsing, as soon as your buffers are properly NUL\nterminated, C functions are _not_ usafe, no matter what people say.\nstrchr/memchr, strc?spn, strn?cmp/memcmp, ... are very efficient, and\nthere is little point to hide them behind stupid strbuf's APIs. Hence,\nwhen it comes to parsing, you just fallback to a string, whose length is\nknown[0].\n\n That's the reason why I won't probably be the one converting\nbuiltin-mailinfo.c to strbuf's because there is very little point to do\nso in the current state of the art.\n\n\nCheers,\n\n\n [0] that allows to fallback to memchr[1] instead of strchr, which is\n slightly faster I'm told.\n\n [1] Note that you must be sure you don't have embedded NULs or memchr\n and strchr won't have the same result then ;)\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"53526","messageId":"1190157301.7157.1.camel@hinata.boston.redhat.com","threadId":"9927","inReplyTo":"7vbqbzvcf4.fsf@gitster.siamese.dyndns.org","subject":"Re: Latest builtin-commit series","fromName":"Kristian Høgsberg","fromEmail":"krh@redhat.com","sentAt":"2007-09-18T23:15:01Z","receivedAt":"2007-09-18T23:15:01Z","isPatch":false,"sender":{"key":"krh@redhat.com","avatar":"https://gravatar.com/avatar/763dee6f9594ac474f725b137a39565792928e583ddf59b32befc2907409027e?d=mp&s=160"},"body":"\nOn Tue, 2007-09-18 at 15:25 -0700, Junio C Hamano wrote:\n> Kristian Høgsberg writes:\n> \n> > Better late than never:\n> >\n> > * rebase to Pierres strbuf changes. Note, there is still some\n> > strbuf tweaking required, to let stripspace work on a strbuf.\n> > Also, I changed the semantics of stripspace to always add a\n> > newline if the last line doesn't have one. I believe the\n> > current odd semantics (always remove the last newline) comes\n> > from not being able to easily add a newline, but now that it's a\n> > strbuf, that's easy.\n> \n> I do not find the \"remove trailing newline\" so odd. Didn't it\n> come because you sometimes needed to _not_ add it?\n\nAll users add a trailing newline if it's not present after stripping.\n\n> > * Set the test suite default editor to '/bin/true' instead of ':'.\n> > Since we're not exec'ing the editor from shell anymore, ':'\n> > won't work. Maybe we should special case ':' in launch_editor\n> > or perhaps make launch_editor use system(3). Not sure.\n> \n> We've had a few threads on the list about what to do with:\n> \n> \tGIT_EDITOR='emacs -nw'\n> \n> I think David's 08874658b450600e72bb7cb0d0747c1ec4b0bfe1\n> (git-sh-setup.sh: make GIT_EDITOR/core.editor/VISUAL/EDITOR\n> accept commands) is a sensible to deal with this issue, and\n> prefer to see the same semantics kept in the C version.\n\nOk, I'll take a look at that commit.\n\nKristian\n"},{"id":"53612","messageId":"7v1wcup7o8.fsf@gitster.siamese.dyndns.org","threadId":"9927","inReplyTo":"1190129009.23692.24.camel@hinata.boston.redhat.com","subject":"Re: Latest builtin-commit series","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-09-19T23:16:55Z","receivedAt":"2007-09-19T23:16:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This series somewhat collided with Pierre's strbuf clean-ups. I\nqueued it in 'pu' for now after fixing the conflicts up.\n\nI haven't looked at the code enough to give any meaningful\ncomments yet, but I agree with most of the comments Dscho and\nothers raised on the topic already.\n"}]}