* Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c
From: Linus Torvalds @ 2007-10-05 16:43 UTC (permalink / raw)
To: Sam Ravnborg; +Cc: Pierre Habouzit, Junio C Hamano, git, Bernt Hansen
In-Reply-To: <20071005162139.GC31413@uranus.ravnborg.org>
On Fri, 5 Oct 2007, Sam Ravnborg wrote:
>
> Took a short look at strbuf.h after seeing the above code.
> And I was suprised to see that all strbuf users were exposed to
> the strbuf structure.
Well, they *have* to. We want people to declare their strbufs as automatic
or static structures, and using a opaque struct pointer is *not* an option
(like "FILE" is doing in stdio.h).
> Following patch would at least make sure noone fiddle with strbuf internals.
No, following patch is fundamentally broken - it's not even a good
starting point. It's bad, bad, bad.
It's also broken in another way: we want it to be really easy to use
strbuf's as normal C strings.
Yes, many (totally idiotic and broken) interfaces think it's so important
to "protect" their internal data structures that you have a
"string_to_c()" helper function for that. That may be "good abstraction",
but it's totally idiotic, because it results in horrible source code!
Tell me which is more readable:
printf("Hello %s\n", sb->buf);
or
printf("Hello %s\n", strbuf_to_c(sb));
and I claim that anybody who claims that the latter is "more readable" is
full of shit, and has an agenda to push, so it's "more agenda-friendly"
rather than readable!
So having "sb->buf" and "sb->len" be visible to users is a *good* thing.
Otherwise you end up having to create millions of idiotic small helper
functions, rather than just use the standard ones.
Linus
^ permalink raw reply
* RE: [ALTERNATE PATCH] Add a simple option parser.
From: Medve Emilian-EMMEDVE1 @ 2007-10-05 16:41 UTC (permalink / raw)
To: Linus Torvalds
Cc: Pierre Habouzit, Mike Hommey, Kristian H?gsberg, git,
Junio C Hamano
In-Reply-To: <alpine.LFD.0.999.0710050924530.23684@woody.linux-foundation.org>
Hello Linus,
> On Fri, 5 Oct 2007, Medve Emilian-EMMEDVE1 wrote:
> > >
> > > Because it's GNU and that it's a heavy dependency to begin with.
> >
> > So it's more of a political decision then a technical one?
>
> I'd *strongly* argue against new dependencies unless they buy us
> something major.
>
> We've been good at cutting them down, including any required
> libraries
> internally. We shouldn't add new ones.
>
> So we'd have to include GNU getopt sources with the git tree,
> at which
> point any advantage would be gone. Might as well include a
> private and
> simpler version of our own.
>From what I understand argp is part of glibc.
Cheers,
Emil.
^ permalink raw reply
* Re: [ALTERNATE PATCH] Add a simple option parser.
From: Pierre Habouzit @ 2007-10-05 16:38 UTC (permalink / raw)
To: David Kastrup; +Cc: git
In-Reply-To: <86wsu1v8ha.fsf@lola.quinscape.zz>
[-- Attachment #1: Type: text/plain, Size: 1692 bytes --]
On Fri, Oct 05, 2007 at 04:20:33PM +0000, David Kastrup wrote:
> "Medve Emilian-EMMEDVE1" <Emilian.Medve@freescale.com> writes:
>
> > Hi Pierre,
> >
> >> -----Original Message-----
> >> From: Pierre Habouzit [mailto:madcoder@debian.org]
> >> Sent: Friday, October 05, 2007 10:57 AM
> >> To: Medve Emilian-EMMEDVE1
> >> Cc: Mike Hommey; Kristian Høgsberg; git@vger.kernel.org;
> >> Junio C Hamano
> >> Subject: Re: [ALTERNATE PATCH] Add a simple option parser.
> >>
> >> On ven, oct 05, 2007 at 03:45:36 +0000, Medve Emilian-EMMEDVE1 wrote:
> >> > You probably already considered and rejected the GNU argp parser. I
> >> > used it before and I'd like to know reasons I should stay away from
> >> > it.
> >>
> >> Because it's GNU and that it's a heavy dependency to begin with.
> >
> > So it's more of a political decision then a technical one?
>
> Well, if it is GNU then it is likely to mean GPLv3 (or GPLv3+) at some
> point of time, though it should certainly be possible for now to still
> secure a v2-licensed version (either GPL or LGPL).
That is an issue indeed.
> And the typical git developer AFAICT prefers to consider themselves as
> unaligned with GNU and the FSF as much as possible.
And is nothing near reality in my case.
The real issue is dependency and bloat. getopt_long would need the GNU
implementation, That I believe depends upon gettext, and argp is just
bloated, and I'm not even sure it's distributed outside from the glibc
anyways.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c
From: Pierre Habouzit @ 2007-10-05 16:35 UTC (permalink / raw)
To: Sam Ravnborg; +Cc: Linus Torvalds, Junio C Hamano, git, Bernt Hansen
In-Reply-To: <20071005162139.GC31413@uranus.ravnborg.org>
[-- Attachment #1: Type: text/plain, Size: 1820 bytes --]
On Fri, Oct 05, 2007 at 04:21:39PM +0000, Sam Ravnborg wrote:
> On Fri, Oct 05, 2007 at 08:26:44AM -0700, Linus Torvalds wrote:
> >
> >
> > On Fri, 5 Oct 2007, Pierre Habouzit wrote:
> > >
> > > - strbuf_grow(buf, len);
> > > + /* only grow if not in place */
> > > + if (strbuf_avail(buf) + buf->len < len)
> > > + strbuf_grow(buf, len - buf->len);
> >
> > Umm. This is really ugly.
> >
> > The whole point of strbuf's was that you shouldn't be doing your own
> > allocation decisions etc. So why do it?
> >
> > Wouldn't it be much better to have a strbuf_make_room() interface that
> > just guarantees that there is enough room fo "len"?
> >
> > Otherwise, code like the above would seem to make the whole point of a
> > safer string interface rather pointless. The above code only makes sense
> > if you know how the strbuf's are internally done, so it should not exists
> > except as internal strbuf code. No?
>
> Took a short look at strbuf.h after seeing the above code.
> And I was suprised to see that all strbuf users were exposed to
> the strbuf structure.
> Following patch would at least make sure noone fiddle with strbuf internals.
> Cut'n'paste - only for the example of it.
> It simply moves strbuf declaration to the .c file where it rightfully belongs.
you're looking at an antiquated version, please look at the one in
current master on current next. In this one, what you can do or not do
with the struct is explained
> git did not build with this change....
Of course it doesn't! people want to have direct access to ->buf and
->len, and it's definitely OK.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: Question about "git commit -a"
From: Matthieu Moy @ 2007-10-05 16:33 UTC (permalink / raw)
To: Kristian Høgsberg
Cc: Andreas Ericsson, Paolo Ciarrocchi, Johannes Schindelin,
Nguyen Thai Ngoc Duy, Wincent Colaiuta, Git Mailing List
In-Reply-To: <1191599763.7117.18.camel@hinata.boston.redhat.com>
Kristian Høgsberg <krh@redhat.com> writes:
> I understand why people like staging and commit without -a, seeing how
> it's faster and all,
Actually, commit without -a is not much faster, since it runs "status"
internally, to show it to the user when launching the editor. So, it
still checks for changes in the working tree.
> but I have a serious problem with this practice that I haven't seen
> brought up on the list. How do you know what you commit actually
> works or even compiles?
That's a general problem with partial commits, and that's why I
personnaly don't like partial commits in general ($scm commit file1
file2 has the same problem, \forall $scm).
To me, the right approach to partial commit is to stash the unwanted
changes, test, commit, and unstash.
(side note: it would be cool to have a "git stash --unstaged" command,
to put the unstaged changes aside, and match the tree to the index. A
good approximation for that is:
$ git stash # put all aside
$ git reset --mixed stash^2 # get back what the index used to be
$ git add -u # and put it back into the index.
)
*But*, the cool thing with git is that you can view rather easily not
only what you're about to commit (git diff --cached), but also what
you're about _not_ to commit (git diff). So, if the unstaged changes
are trivial enough, it can be OK (for example, Linus changes the linux
version in a Makefile a few commits before the release, and doesn't
add it to the index, to keep it as a reminder).
But I agree with your that splitting a huge patch into smaller ones
using just the index is bad practice, except if you intend to come
back to each commit and test it later.
--
Matthieu
^ permalink raw reply
* RE: [ALTERNATE PATCH] Add a simple option parser.
From: Linus Torvalds @ 2007-10-05 16:28 UTC (permalink / raw)
To: Medve Emilian-EMMEDVE1
Cc: Pierre Habouzit, Mike Hommey, Kristian H?gsberg, git,
Junio C Hamano
In-Reply-To: <598D5675D34BE349929AF5EDE9B03E2701624FF2@az33exm24.fsl.freescale.net>
On Fri, 5 Oct 2007, Medve Emilian-EMMEDVE1 wrote:
> >
> > Because it's GNU and that it's a heavy dependency to begin with.
>
> So it's more of a political decision then a technical one?
I'd *strongly* argue against new dependencies unless they buy us
something major.
We've been good at cutting them down, including any required libraries
internally. We shouldn't add new ones.
So we'd have to include GNU getopt sources with the git tree, at which
point any advantage would be gone. Might as well include a private and
simpler version of our own.
Linus
^ permalink raw reply
* Re: [ALTERNATE PATCH] Add a simple option parser.
From: David Kastrup @ 2007-10-05 16:20 UTC (permalink / raw)
To: git
In-Reply-To: <598D5675D34BE349929AF5EDE9B03E2701624FF2@az33exm24.fsl.freescale.net>
"Medve Emilian-EMMEDVE1" <Emilian.Medve@freescale.com> writes:
> Hi Pierre,
>
>> -----Original Message-----
>> From: Pierre Habouzit [mailto:madcoder@debian.org]
>> Sent: Friday, October 05, 2007 10:57 AM
>> To: Medve Emilian-EMMEDVE1
>> Cc: Mike Hommey; Kristian Høgsberg; git@vger.kernel.org;
>> Junio C Hamano
>> Subject: Re: [ALTERNATE PATCH] Add a simple option parser.
>>
>> On ven, oct 05, 2007 at 03:45:36 +0000, Medve Emilian-EMMEDVE1 wrote:
>> > You probably already considered and rejected the GNU argp parser. I
>> > used it before and I'd like to know reasons I should stay away from
>> > it.
>>
>> Because it's GNU and that it's a heavy dependency to begin with.
>
> So it's more of a political decision then a technical one?
Well, if it is GNU then it is likely to mean GPLv3 (or GPLv3+) at some
point of time, though it should certainly be possible for now to still
secure a v2-licensed version (either GPL or LGPL).
GNU also means a different coding and indentation style.
Personally, I couldn't care less about both points (I prefer the GNU
coding style anyway), but that's for the maintainer to decide, and one
also has to take into account the effect on developer motivation.
And the typical git developer AFAICT prefers to consider themselves as
unaligned with GNU and the FSF as much as possible.
--
David Kastrup
^ permalink raw reply
* Re: Many gits are offline this week
From: David Brown @ 2007-10-05 16:20 UTC (permalink / raw)
To: Dmitry Potapov; +Cc: Randal L. Schwartz, git
In-Reply-To: <37fcd2780710050741x1760a585yeaa9023a8d8cdccf@mail.gmail.com>
On Fri, Oct 05, 2007 at 06:41:43PM +0400, Dmitry Potapov wrote:
>I believe I have found one mistake in your slides. Slide 18 reads:
>"git-commit -a" is like "git-add .; git-commit"
>
>But it is incorrect, because "git-commit -a" does not add new files, so
>it works like "git-add -u .; git-commit".
"git-commit -a" also detects files that have been deleted, whereas
"git-add" does not. In fact, I haven't found an easy way to detect this
other than "git-commit -a", and it is the only reason I ever run
"git-commit -a"
Dave
^ permalink raw reply
* Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c
From: Sam Ravnborg @ 2007-10-05 16:21 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Pierre Habouzit, Junio C Hamano, git, Bernt Hansen
In-Reply-To: <alpine.LFD.0.999.0710050819540.23684@woody.linux-foundation.org>
On Fri, Oct 05, 2007 at 08:26:44AM -0700, Linus Torvalds wrote:
>
>
> On Fri, 5 Oct 2007, Pierre Habouzit wrote:
> >
> > - strbuf_grow(buf, len);
> > + /* only grow if not in place */
> > + if (strbuf_avail(buf) + buf->len < len)
> > + strbuf_grow(buf, len - buf->len);
>
> Umm. This is really ugly.
>
> The whole point of strbuf's was that you shouldn't be doing your own
> allocation decisions etc. So why do it?
>
> Wouldn't it be much better to have a strbuf_make_room() interface that
> just guarantees that there is enough room fo "len"?
>
> Otherwise, code like the above would seem to make the whole point of a
> safer string interface rather pointless. The above code only makes sense
> if you know how the strbuf's are internally done, so it should not exists
> except as internal strbuf code. No?
Took a short look at strbuf.h after seeing the above code.
And I was suprised to see that all strbuf users were exposed to
the strbuf structure.
Following patch would at least make sure noone fiddle with strbuf internals.
Cut'n'paste - only for the example of it.
It simply moves strbuf declaration to the .c file where it rightfully belongs.
git did not build with this change....
Sam
diff --git a/strbuf.c b/strbuf.c
index e33d06b..0d2d578 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -1,6 +1,14 @@
#include "cache.h"
#include "strbuf.h"
+struct strbuf {
+ int alloc;
+ int len;
+ int eof;
+ char *buf;
+};
+
+
void strbuf_init(struct strbuf *sb) {
sb->buf = NULL;
sb->eof = sb->alloc = sb->len = 0;
diff --git a/strbuf.h b/strbuf.h
index 74cc012..c057be3 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -1,11 +1,6 @@
#ifndef STRBUF_H
#define STRBUF_H
-struct strbuf {
- int alloc;
- int len;
- int eof;
- char *buf;
-};
+struct strbuf;
extern void strbuf_init(struct strbuf *);
extern void read_line(struct strbuf *, FILE *, int);
^ permalink raw reply related
* [PATCH 3/2] Document the fact that git-svn now runs git-gc
From: Steven Grimm @ 2007-10-05 16:15 UTC (permalink / raw)
To: git
In-Reply-To: <47066255.6080500@midwinter.com>
Signed-off-by: Steven Grimm <koreth@midwinter.com>
---
Documentation/git-svn.txt | 10 +++++++++-
1 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/Documentation/git-svn.txt b/Documentation/git-svn.txt
index e157c6a..26f0f39 100644
--- a/Documentation/git-svn.txt
+++ b/Documentation/git-svn.txt
@@ -125,7 +125,15 @@ and have no uncommitted changes.
alternative to HEAD.
This is advantageous over 'set-tree' (below) because it produces
cleaner, more linear history.
-+
+
+When the commit is finished, gitlink:git-gc[1] is run with the
+`--prune` and `--auto` options to clean up the git object database,
+including removing old unreachable objects (some of which are
+created by the process of committing to SVN.) Set the `gc.auto`
+config option to 0 if you don't want your repository to be cleaned,
+e.g., because you are intentionally keeping unreachable objects in
+your repository.
+
--no-rebase;;
After committing, do not rebase or reset.
--
--
1.5.3.4.203.gcc61a
^ permalink raw reply related
* Re: [PATCH 2/2] Run garbage collection with loose object pruning after svn dcommit
From: Steven Grimm @ 2007-10-05 16:12 UTC (permalink / raw)
To: Peter Baumann; +Cc: git
In-Reply-To: <20071005082110.GA4797@xp.machine.xx>
Peter Baumann wrote:
> I don't like the automatic prune. What if someone has other objects in
> there which shouldn't be pruned? Making git svn dcommit doing the prune
> would be at least suprising, because how is one supposed to know that
> doing a commit into svn will prune all your precious objects?
>
"git commit" already does garbage collection, so we've already set a
precedent for a commit operation also doing some cleanup at the end.
However, you're correct that this cleanup behavior (and the way to turn
it off) should be documented so that there's some way to know about it.
Doc patch forthcoming.
> Sure, I can unterstand from where you are coming from, but I'd prefere
> if this could be specified on a case by case basis, e.g. from the
> cmdline or as a config option.
>
This code (by virtue of only doing the prune if the "too many loose
objects" test succeeds) will obey the existing gc.auto config option. So
it's already possible to turn off as is. I'll note that in the doc patch.
-Steve
^ permalink raw reply
* RE: [ALTERNATE PATCH] Add a simple option parser.
From: Medve Emilian-EMMEDVE1 @ 2007-10-05 16:10 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: Mike Hommey, Kristian Høgsberg, git, Junio C Hamano
In-Reply-To: <20071005155647.GC20305@artemis.corp>
Hi Pierre,
> -----Original Message-----
> From: Pierre Habouzit [mailto:madcoder@debian.org]
> Sent: Friday, October 05, 2007 10:57 AM
> To: Medve Emilian-EMMEDVE1
> Cc: Mike Hommey; Kristian Høgsberg; git@vger.kernel.org;
> Junio C Hamano
> Subject: Re: [ALTERNATE PATCH] Add a simple option parser.
>
> On ven, oct 05, 2007 at 03:45:36 +0000, Medve Emilian-EMMEDVE1 wrote:
> > You probably already considered and rejected the GNU argp parser. I
> > used it before and I'd like to know reasons I should stay away from
> > it.
>
> Because it's GNU and that it's a heavy dependency to begin with.
So it's more of a political decision then a technical one?
> Moreover, getopt_long doesn't deal with argument types (like
> integers).
AFAIK, getopt_long in not argp.
Cheers,
Emil.
^ permalink raw reply
* Re: [ALTERNATE PATCH] Add a simple option parser.
From: Pierre Habouzit @ 2007-10-05 15:56 UTC (permalink / raw)
To: Medve Emilian-EMMEDVE1
Cc: Mike Hommey, Kristian Høgsberg, git, Junio C Hamano
In-Reply-To: <598D5675D34BE349929AF5EDE9B03E2701624FD6@az33exm24.fsl.freescale.net>
[-- Attachment #1: Type: text/plain, Size: 542 bytes --]
On ven, oct 05, 2007 at 03:45:36 +0000, Medve Emilian-EMMEDVE1 wrote:
> You probably already considered and rejected the GNU argp parser. I
> used it before and I'd like to know reasons I should stay away from
> it.
Because it's GNU and that it's a heavy dependency to begin with.
Moreover, getopt_long doesn't deal with argument types (like integers).
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: Question about "git commit -a"
From: Kristian Høgsberg @ 2007-10-05 15:56 UTC (permalink / raw)
To: Andreas Ericsson
Cc: Paolo Ciarrocchi, Johannes Schindelin, Nguyen Thai Ngoc Duy,
Wincent Colaiuta, Git Mailing List
In-Reply-To: <4705FB52.3030208@op5.se>
> I just don't do 'git commit -a' for the same reason I don't do
> 'git commit -m', really. It tends to be habit-forming, and bisect
> has saved my arse enough times for me to *want* my changes to be
> small and isolated. Debugging a 5-line patch is so much more pleasant
> than debugging a 30k-lines one that spans over several different files.
I understand why people like staging and commit without -a, seeing how
it's faster and all, but I have a serious problem with this practice
that I haven't seen brought up on the list. How do you know what you
commit actually works or even compiles? The reason that I almost
exclusively use -a with commit is that I want to know that what I just
compiled and tested is what I will be committing. I don't want to just
commit half the files in my working copy, I want to make sure that the
exact state of my project that I just compiled and tested is what gets
into version controlled history.
git commit -a isn't sloppy to me - eye balling some subset of your
working copy and committing that under the assumption that you don't
make mistakes and don't need to compile what you commit... that is
sloppy.
Kristian
^ permalink raw reply
* Re: [ALTERNATE PATCH] Add a simple option parser.
From: Pierre Habouzit @ 2007-10-05 15:54 UTC (permalink / raw)
To: Kristian Høgsberg; +Cc: git, Junio C Hamano
In-Reply-To: <1191598424.7117.10.camel@hinata.boston.redhat.com>
[-- Attachment #1: Type: text/plain, Size: 1890 bytes --]
On Fri, Oct 05, 2007 at 03:33:44PM +0000, Kristian Høgsberg wrote:
> On Fri, 2007-10-05 at 16:25 +0200, Pierre Habouzit wrote:
> > The option parser takes argc, argv, an array of struct option
> > and a usage string. Each of the struct option elements in the array
> > describes a valid option, its type and a pointer to the location where the
> > value is written. The entry point is parse_options(), which scans through
> > the given argv, and matches each option there against the list of valid
> > options. During the scan, argv is rewritten to only contain the
> > non-option command line arguments and the number of these is returned.
> >
> > Aggregation of single switches is allowed:
> > -rC0 is the same as -r -C 0 (supposing that -C wants an arg).
> >
> > Boolean switches automatically support the option with the same name,
> > prefixed with 'no-' to disable the switch:
> > --no-color / --color only need to have an entry for "color".
> >
> > Long options are supported either with '=' or without:
> > --some-option=foo is the same as --some-option foo
>
> That looks great, works for me. One comment, though: it looks like
> you're not sure whether to call these things "options" or "switches".
> We should choose one and stick with it.
I use the word "switch" when it's a short_option, and "option" when
it's a long one. But maybe the distinction doesn't make sense, and it's
a non-native speaker glitch. I don't care that much btw.
> > oh and I don't grok what OPTION_LAST is for, so I left it apart, but
> > it seems unused ?
>
> Oh, kill that. I used that as the option array terminator before we
> switched to ARRAY_SIZE().
Okay :)
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c
From: Pierre Habouzit @ 2007-10-05 15:50 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Junio C Hamano, git, Bernt Hansen
In-Reply-To: <alpine.LFD.0.999.0710050819540.23684@woody.linux-foundation.org>
[-- Attachment #1: Type: text/plain, Size: 3385 bytes --]
On Fri, Oct 05, 2007 at 03:26:44PM +0000, Linus Torvalds wrote:
>
>
> On Fri, 5 Oct 2007, Pierre Habouzit wrote:
> >
> > - strbuf_grow(buf, len);
> > + /* only grow if not in place */
> > + if (strbuf_avail(buf) + buf->len < len)
> > + strbuf_grow(buf, len - buf->len);
>
> Umm. This is really ugly.
I agree.
> The whole point of strbuf's was that you shouldn't be doing your own
> allocation decisions etc. So why do it?
the point here is that it's in a "filter" that is called like this:
some_filter(buf->buf, buf->len, buf);
src len dst
You can call the filter with src/len being data from anywere,
including the current content of the destination buffer.
Then there is two cases, either the filter is known to be done in
place, either we can't know or we know it wont.
In the latter case, we have a bit of code like that:
char *to_free = NULL;
if (buf->buf == src)
to_free = strbuf_detach(&buf);
.. hack ..
free(to_free);
In the former case, then there is a small glitch, being that if we are
doing in place editing, we should not touch buffer at all (or it would
invalidate "src"). If we are not in the in-place editing code though,
then we have to make the resulting buffer be big enough...
> Wouldn't it be much better to have a strbuf_make_room() interface that
> just guarantees that there is enough room fo "len"?
Right, that would do the same btw ;)
> Otherwise, code like the above would seem to make the whole point of a
> safer string interface rather pointless. The above code only makes sense
> if you know how the strbuf's are internally done, so it should not exists
> except as internal strbuf code. No?
Well, the above code is used in filters to spare reallocations. So if
we want to "blackbox" such a think, strbuf_make_room isn't the proper
API. We should rather use
void *strbuf_begin_filter(struct strbuf *sb, const char *src, size_t reslen);
strbuf_end_filter(void *);
`strbuf_begin_filter` would decide upon the hint `reslen` argument if
we know if we can work in place or not (has a meaning iff src points
into the strbuf buffer). If not, it could stash the strbuf buffer in the
returned void * to be freed at the end of the filter. It seems like a
better alternative than a strbuf_make_room.
Of course, strbuf_begin_filter() would really be simple and basically
be:
char *tmp;
if (src points into sb->buf && reslen > sb->alloc - 1) {
// in place editing is OK
return NULL;
}
tmp = strbuf_release(&sb);
strbuf_grow(&sb, len);
return tmp;
and strbuf_end_filter would just be "free" :)
We could even make "reslen" be a ssize_t so that -1 would mean "I've
absolutely no idea how much space I'll need (or just in place editing is
not supported). This way, both hacks I described in this mail could be
hidden in the strbuf module, and be properly documented _and_ safe _and_
efficient.
What do you think ?
[Though if we do that, I still think it's more important to fix the
bug in master, and have a new patch implementing this approach]
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: [ALTERNATE PATCH] Add a simple option parser.
From: Kristian Høgsberg @ 2007-10-05 15:33 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: git, Junio C Hamano
In-Reply-To: <20071005142507.GL19879@artemis.corp>
On Fri, 2007-10-05 at 16:25 +0200, Pierre Habouzit wrote:
> The option parser takes argc, argv, an array of struct option
> and a usage string. Each of the struct option elements in the array
> describes a valid option, its type and a pointer to the location where the
> value is written. The entry point is parse_options(), which scans through
> the given argv, and matches each option there against the list of valid
> options. During the scan, argv is rewritten to only contain the
> non-option command line arguments and the number of these is returned.
>
> Aggregation of single switches is allowed:
> -rC0 is the same as -r -C 0 (supposing that -C wants an arg).
>
> Boolean switches automatically support the option with the same name,
> prefixed with 'no-' to disable the switch:
> --no-color / --color only need to have an entry for "color".
>
> Long options are supported either with '=' or without:
> --some-option=foo is the same as --some-option foo
That looks great, works for me. One comment, though: it looks like
you're not sure whether to call these things "options" or "switches".
We should choose one and stick with it.
Acked-by: Kristian Høgsberg <krh@redhat.com>
> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> ---
>
> I'm sorry about the "From" I don't intend to "steal" the patch in any
> sense, it's just an alternate proposal.
No worries, I'm glad to see this move forward.
> oh and I don't grok what OPTION_LAST is for, so I left it apart, but
> it seems unused ?
Oh, kill that. I used that as the option array terminator before we
switched to ARRAY_SIZE().
>
> Makefile | 2 +-
> parse-options.c | 154 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> parse-options.h | 29 ++++++++++
> 3 files changed, 184 insertions(+), 1 deletions(-)
> create mode 100644 parse-options.c
> create mode 100644 parse-options.h
>
> diff --git a/Makefile b/Makefile
> index 62bdac6..d90e959 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -310,7 +310,7 @@ LIB_OBJS = \
> alloc.o merge-file.o path-list.o help.o unpack-trees.o $(DIFF_OBJS) \
> color.o wt-status.o archive-zip.o archive-tar.o shallow.o utf8.o \
> convert.o attr.o decorate.o progress.o mailmap.o symlinks.o remote.o \
> - transport.o bundle.o
> + transport.o bundle.o parse-options.o
>
> BUILTIN_OBJS = \
> builtin-add.o \
> diff --git a/parse-options.c b/parse-options.c
> new file mode 100644
> index 0000000..eb3ff40
> --- /dev/null
> +++ b/parse-options.c
> @@ -0,0 +1,154 @@
> +#include "git-compat-util.h"
> +#include "parse-options.h"
> +
> +struct optparse_t {
> + const char **argv;
> + int argc;
> + const char *opt;
> +};
> +
> +static inline const char *skippfx(const char *str, const char *prefix)
> +{
> + size_t len = strlen(prefix);
> + return strncmp(str, prefix, len) ? NULL : str + len;
> +}
> +
> +static int opterror(struct option *opt, const char *reason, int shorterr)
> +{
> + if (shorterr) {
> + return error("switch `%c' %s", opt->short_name, reason);
> + } else {
> + return error("option `%s' %s", opt->long_name, reason);
> + }
> +}
option/switch?
> +static int get_value(struct optparse_t *p, struct option *opt,
> + int boolean, int shorterr)
> +{
> + switch (opt->type) {
> + const char *s;
> + int v;
> +
> + case OPTION_BOOLEAN:
> + *(int *)opt->value = boolean;
> + return 0;
> +
> + case OPTION_STRING:
> + if (p->opt && *p->opt) {
> + *(const char **)opt->value = p->opt;
> + p->opt = NULL;
> + } else {
> + if (p->argc < 1)
> + return opterror(opt, "requires a value", shorterr);
> + *(const char **)opt->value = *++p->argv;
> + p->argc--;
> + }
> + return 0;
> +
> + case OPTION_INTEGER:
> + if (p->opt && *p->opt) {
> + v = strtol(p->opt, (char **)&s, 10);
> + p->opt = NULL;
> + } else {
> + if (p->argc < 1)
> + return opterror(opt, "requires a value", shorterr);
> + v = strtol(*++p->argv, (char **)&s, 10);
> + p->argc--;
> + }
> + if (*s)
> + return opterror(opt, "expects a numerical value", shorterr);
> + *(int *)opt->value = v;
> + return 0;
> + }
> +
> + abort();
> +}
> +
> +static int parse_short_opt(struct optparse_t *p, struct option *options, int count)
> +{
> + int i;
> +
> + for (i = 0; i < count; i++) {
> + if (options[i].short_name == *p->opt) {
> + p->opt++;
> + return get_value(p, options + i, 1, 1);
> + }
> + }
> + return error("unknown switch `%c'", *p->opt);
> +}
> +
> +static int parse_long_opt(struct optparse_t *p, const char *arg,
> + struct option *options, int count)
> +{
> + int boolean = 1;
> + int i;
> +
> + for (i = 0; i < count; i++) {
> + const char *rest;
> +
> + if (!options[i].long_name)
> + continue;
> +
> + rest = skippfx(arg, options[i].long_name);
> + if (!rest && options[i].type == OPTION_BOOLEAN) {
> + if (!rest && skippfx(arg, "no-")) {
> + rest = skippfx(arg + 3, options[i].long_name);
> + boolean = 0;
> + }
> + if (rest && *rest == '=')
> + return opterror(options + i, "takes no value", 0);
> + }
> + if (!rest || (*rest && *rest != '='))
> + continue;
> + if (*rest) {
> + p->opt = rest;
> + }
> + return get_value(p, options + i, boolean, 0);
> + }
> + return error("unknown option `%s'", arg);
> +}
> +
> +int parse_options(int argc, const char **argv,
> + struct option *options, int count,
> + const char *usage_string)
> +{
> + struct optparse_t optp = { argv + 1, argc - 1, NULL };
> + int j = 0;
> +
> + while (optp.argc) {
> + const char *arg = optp.argv[0];
> +
> + if (*arg != '-' || !arg[1]) {
> + argv[j++] = *optp.argv++;
> + optp.argc--;
> + continue;
> + }
> +
> + if (arg[1] != '-') {
> + optp.opt = arg + 1;
> + while (*optp.opt) {
> + if (parse_short_opt(&optp, options, count) < 0) {
> + usage(usage_string);
> + return -1;
> + }
> + }
> + optp.argc--;
> + optp.argv++;
> + continue;
> + }
> +
> + if (!arg[2]) /* "--" */
> + break;
> +
> + if (parse_long_opt(&optp, arg + 2, options, count)) {
> + usage(usage_string);
> + return -1;
> + }
> + optp.argc--;
> + optp.argv++;
> + }
> +
> + memmove(argv + j, optp.argv, optp.argc * sizeof(argv));
> + argv[j + optp.argc] = NULL;
> + return j + optp.argc;
> +}
> diff --git a/parse-options.h b/parse-options.h
> new file mode 100644
> index 0000000..e4749d0
> --- /dev/null
> +++ b/parse-options.h
> @@ -0,0 +1,29 @@
> +#ifndef PARSE_OPTIONS_H
> +#define PARSE_OPTIONS_H
> +
> +enum option_type {
> + OPTION_BOOLEAN,
> + OPTION_STRING,
> + OPTION_INTEGER,
> +#if 0
> + OPTION_LAST,
> +#endif
> +};
> +
> +struct option {
> + enum option_type type;
> + const char *long_name;
> + char short_name;
> + void *value;
> +};
> +
> +/* parse_options() will filter out the processed options and leave the
> + * non-option argments in argv[]. The return value is the number of
> + * arguments left in argv[].
> + */
> +
> +extern int parse_options(int argc, const char **argv,
> + struct option *options, int count,
> + const char *usage_string);
> +
> +#endif
^ permalink raw reply
* RE: [ALTERNATE PATCH] Add a simple option parser.
From: Medve Emilian-EMMEDVE1 @ 2007-10-05 15:45 UTC (permalink / raw)
To: Pierre Habouzit, Mike Hommey; +Cc: Kristian Høgsberg, git, Junio C Hamano
In-Reply-To: <20071005144540.GM19879@artemis.corp>
Hi,
You probably already considered and rejected the GNU argp parser. I used it before and I'd like to know reasons I should stay away from it.
Cheers,
Emil.
> -----Original Message-----
> From: git-owner@vger.kernel.org
> [mailto:git-owner@vger.kernel.org] On Behalf Of Pierre Habouzit
> Sent: Friday, October 05, 2007 9:46 AM
> To: Mike Hommey
> Cc: Kristian Høgsberg; git@vger.kernel.org; Junio C Hamano
> Subject: Re: [ALTERNATE PATCH] Add a simple option parser.
>
> On Fri, Oct 05, 2007 at 02:30:14PM +0000, Mike Hommey wrote:
> > On Fri, Oct 05, 2007 at 04:25:07PM +0200, Pierre Habouzit
> <madcoder@debian.org> wrote:
> > > The option parser takes argc, argv, an array of struct option
> > > and a usage string. Each of the struct option elements
> in the array
> > > describes a valid option, its type and a pointer to the
> location where the
> > > value is written. The entry point is parse_options(),
> which scans through
> > > the given argv, and matches each option there against the
> list of valid
> > > options. During the scan, argv is rewritten to only contain the
> > > non-option command line arguments and the number of these
> is returned.
> > >
> > > Aggregation of single switches is allowed:
> > > -rC0 is the same as -r -C 0 (supposing that -C wants an arg).
> >
> > I like options aggregation, but I'm not sure aggregating
> option arguments
> > is a good idea... I can't even think of an application that does it.
>
> You mean like `grep -A1` or `diff -u3` or `ls -w10` ?
>
> getopt does that by default as well, so you may not have aware of it,
> but it's how things work in your system already.
>
> btw `ls -rw10` works, though `ls -w10r` drops the 'r'
> silently. FWIW I
> don't, in that case, the alternate patch I propose complains
> about "10r"
> not being a valid integer, and that's because unlike getopt, the patch
> krh proposed knows what an integer is ;)
> --
> ·O· Pierre Habouzit
> ··O madcoder@debian.org
> OOO
> http://www.madism.org
^ permalink raw reply
* Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c
From: Linus Torvalds @ 2007-10-05 15:26 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: Junio C Hamano, git, Bernt Hansen
In-Reply-To: <20071005085522.32EFF1E16E@madism.org>
On Fri, 5 Oct 2007, Pierre Habouzit wrote:
>
> - strbuf_grow(buf, len);
> + /* only grow if not in place */
> + if (strbuf_avail(buf) + buf->len < len)
> + strbuf_grow(buf, len - buf->len);
Umm. This is really ugly.
The whole point of strbuf's was that you shouldn't be doing your own
allocation decisions etc. So why do it?
Wouldn't it be much better to have a strbuf_make_room() interface that
just guarantees that there is enough room fo "len"?
Otherwise, code like the above would seem to make the whole point of a
safer string interface rather pointless. The above code only makes sense
if you know how the strbuf's are internally done, so it should not exists
except as internal strbuf code. No?
Linus
^ permalink raw reply
* Re: [ALTERNATE PATCH] Add a simple option parser.
From: David Kastrup @ 2007-10-05 14:59 UTC (permalink / raw)
To: git
In-Reply-To: <20071005143014.GA18176@glandium.org>
Mike Hommey <mh@glandium.org> writes:
> On Fri, Oct 05, 2007 at 04:25:07PM +0200, Pierre Habouzit <madcoder@debian.org> wrote:
>> The option parser takes argc, argv, an array of struct option
>> and a usage string. Each of the struct option elements in the array
>> describes a valid option, its type and a pointer to the location where the
>> value is written. The entry point is parse_options(), which scans through
>> the given argv, and matches each option there against the list of valid
>> options. During the scan, argv is rewritten to only contain the
>> non-option command line arguments and the number of these is returned.
>>
>> Aggregation of single switches is allowed:
>> -rC0 is the same as -r -C 0 (supposing that -C wants an arg).
>
> I like options aggregation, but I'm not sure aggregating option arguments
> is a good idea... I can't even think of an application that does it.
I think most allow this for the last option in a row. Tar is somewhat
more perverse with its non-option command string:
tar xfzbv filename.tgz 40
uses filename.tgz as the option argument for "f" and 40 for "b".
Note that while tar accepts options instead of the initial command
string,
tar -xfzbv filename.tgz 40
will _not_ work, while
tar -xffilename.tgz -z -b40 -v
presumably would (have no time to test this right now).
--
David Kastrup
^ permalink raw reply
* Re: Many gits are offline this week
From: Frank Lichtenheld @ 2007-10-05 15:13 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: git
In-Reply-To: <20071005010448.GQ2137@spearce.org>
On Thu, Oct 04, 2007 at 09:04:48PM -0400, Shawn O. Pearce wrote:
> With three gits offline for at least the next few days perhaps
> someone would be willing to step up and collect patches so that Junio
> has a reasonable place to pick up from when he can get back online?
While I have neither the time nor the abilities to do this I
will at least accumulate the various cvsserver patches floating
around the last few days so that I can submit them to Junio in one
batch.
Gruesse,
--
Frank Lichtenheld <frank@lichtenheld.de>
www: http://www.djpig.de/
^ permalink raw reply
* Re: [PATCH] git-shell and git-cvsserver
From: Frank Lichtenheld @ 2007-10-05 15:07 UTC (permalink / raw)
To: Jan Wielemaker; +Cc: Git Mailing List
In-Reply-To: <200710051453.47622.wielemak@science.uva.nl>
On Fri, Oct 05, 2007 at 02:53:47PM +0200, Jan Wielemaker wrote:
> +#define EXEC_PATH "/usr/local/bin"
> +
> +static int do_cvs_cmd(const char *me, char *arg)
> +{
> + const char *my_argv[4];
> + const char *oldpath;
> +
> + if ( !arg )
> + die("no argument");
> + if ( strcmp(arg, "server") )
> + die("only allows git-cvsserver server: %s", arg);
> +
> + my_argv[0] = "cvsserver";
> + my_argv[1] = "server";
> + my_argv[2] = NULL;
> +
> + if ( (oldpath=getenv("PATH")) ) {
> + char *newpath = malloc(strlen(oldpath)+strlen(EXEC_PATH)+5+1+1);
> +
> + sprintf(newpath, "PATH=%s:%s", EXEC_PATH, oldpath);
> + putenv(newpath);
> + } else {
> + char *newpath = malloc(strlen(EXEC_PATH)+5+1);
> +
> + sprintf(newpath, "PATH=%s", EXEC_PATH);
> + putenv(newpath);
> + }
> +
> + return execv_git_cmd(my_argv);
> +}
This seems to be mostly a duplication of prepend_to_path from git.c
Gruesse,
--
Frank Lichtenheld <frank@lichtenheld.de>
www: http://www.djpig.de/
^ permalink raw reply
* Re: [ALTERNATE PATCH] Add a simple option parser.
From: Pierre Habouzit @ 2007-10-05 14:45 UTC (permalink / raw)
To: Mike Hommey; +Cc: Kristian Høgsberg, git, Junio C Hamano
In-Reply-To: <20071005143014.GA18176@glandium.org>
[-- Attachment #1: Type: text/plain, Size: 1575 bytes --]
On Fri, Oct 05, 2007 at 02:30:14PM +0000, Mike Hommey wrote:
> On Fri, Oct 05, 2007 at 04:25:07PM +0200, Pierre Habouzit <madcoder@debian.org> wrote:
> > The option parser takes argc, argv, an array of struct option
> > and a usage string. Each of the struct option elements in the array
> > describes a valid option, its type and a pointer to the location where the
> > value is written. The entry point is parse_options(), which scans through
> > the given argv, and matches each option there against the list of valid
> > options. During the scan, argv is rewritten to only contain the
> > non-option command line arguments and the number of these is returned.
> >
> > Aggregation of single switches is allowed:
> > -rC0 is the same as -r -C 0 (supposing that -C wants an arg).
>
> I like options aggregation, but I'm not sure aggregating option arguments
> is a good idea... I can't even think of an application that does it.
You mean like `grep -A1` or `diff -u3` or `ls -w10` ?
getopt does that by default as well, so you may not have aware of it,
but it's how things work in your system already.
btw `ls -rw10` works, though `ls -w10r` drops the 'r' silently. FWIW I
don't, in that case, the alternate patch I propose complains about "10r"
not being a valid integer, and that's because unlike getopt, the patch
krh proposed knows what an integer is ;)
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: Many gits are offline this week
From: Dmitry Potapov @ 2007-10-05 14:41 UTC (permalink / raw)
To: Randal L. Schwartz; +Cc: git
In-Reply-To: <86tzp54sez.fsf@blue.stonehenge.com>
Hi Randal,
> I've had the slides reviewed by Smarter People Than Me on #git already, so
> hopefully most of it is accurate. :) They're temporarily at
>
> http://www.stonehenge.com/pic/Git-2.0.3-to-be.pdf
I believe I have found one mistake in your slides. Slide 18 reads:
"git-commit -a" is like "git-add .; git-commit"
But it is incorrect, because "git-commit -a" does not add new files, so
it works like "git-add -u .; git-commit".
Dmitry
^ permalink raw reply
* Re: [ALTERNATE PATCH] Add a simple option parser.
From: Mike Hommey @ 2007-10-05 14:30 UTC (permalink / raw)
To: Pierre Habouzit, Kristian Høgsberg, git, Junio C Hamano
In-Reply-To: <20071005142507.GL19879@artemis.corp>
On Fri, Oct 05, 2007 at 04:25:07PM +0200, Pierre Habouzit <madcoder@debian.org> wrote:
> The option parser takes argc, argv, an array of struct option
> and a usage string. Each of the struct option elements in the array
> describes a valid option, its type and a pointer to the location where the
> value is written. The entry point is parse_options(), which scans through
> the given argv, and matches each option there against the list of valid
> options. During the scan, argv is rewritten to only contain the
> non-option command line arguments and the number of these is returned.
>
> Aggregation of single switches is allowed:
> -rC0 is the same as -r -C 0 (supposing that -C wants an arg).
I like options aggregation, but I'm not sure aggregating option arguments
is a good idea... I can't even think of an application that does it.
Mike
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox