* Re: [PATCH] setup/rev-parse: allow HEAD to be spelled 'head'
From: Johannes Schindelin @ 2007-10-07 16:49 UTC (permalink / raw)
To: Sam Vilain; +Cc: Alex Riesen, git
In-Reply-To: <47080AC4.3040902@catalyst.net.nz>
Hi,
On Sun, 7 Oct 2007, Sam Vilain wrote:
> Alex Riesen wrote:
> > Sam Vilain, Fri, Oct 05, 2007 05:09:10 +0200:
> >> If the repository got mangled by FAT capitalization rules, then a ref
> >> such as "HEAD" will become "head" once it is back on a non-FAT FS.
> >> Check for this condition in resolve_refs and in the setup code.
> >>
> >> Suggested-by: Francois Marier <francois@debian.org>
> >> Signed-off-by: Sam Vilain <sam.vilain@catalyst.net.nz>
> >> ---
> >> This should probably help people putting their git repos on
> >> FAT USB sticks.
> >
> > Can the people just mount FAT partitions with shortname=mixed?
>
> Of course, that is probably a solution to the problem, whereas my patch
> is a workaround.
>
> Now, I realise that this might open a can of worms ... would we also
> want to go looking for files called "pack-ab~1.pac" ?
Hell, no.
> Almost certainly not - but this solves the most immediate problem
> experienced by people putting their git repositories onto FAT
> filesystems mounted with the default options, which will say "FATAL: not
> a git repository" otherwise.
You certainly mean the issue of capitalization; yes, that is my
experience, too. Somehow, "HEAD" is the culprit.
Ciao,
Dscho
P.S.: seems you have quite cute coworkers.
^ permalink raw reply
* Re: [PATCH] Make git-clean a builtin
From: rae l @ 2007-10-07 16:42 UTC (permalink / raw)
To: Shawn Bohrer; +Cc: Linus Torvalds, git, frank, gitster
In-Reply-To: <20071007154126.GD5642@mediacenter.austin.rr.com>
On 10/7/07, Shawn Bohrer <shawn.bohrer@gmail.com> wrote:
> Thanks for the input.
>
> On Sat, Oct 06, 2007 at 06:31:36PM -0700, Linus Torvalds wrote:
> > This looks better, but I think you'd be even better off actually using the
> > "read_directory()" interface directly, instead of exec'ing off "git
> > ls-files" and parsing the line output.
>
> Perhaps, I'll take a look at how git-ls-files does it and see if I can
> do that directly. Since I'm new to git (and C) it will probably take me
> a while to re-implement though.
>
> > I also would still worry a bit about 'chdir(x)' and 'chdir("..")', because
> > quite frankly, they are *not* mirrors of each other (think symlinks, but
> > also error behaviour due to directories that might be non-executable).
> > Now, admittedly, if a directory isn't executable, I can imagine other git
> > things having problems (anybody want to test?), but that whole pattern is
> > just very fragile and not very reliable.
>
> Yes it does seem fragile, but 'chdir("-")' doesn't work in C and I
> couldn't find any equivalents. I actually did think about symlinks, and
> my code does do the right thing since I test if it is a directory before
> doing the 'chdir(x)'. Symlinks are therefore treated as normal files and
> removed.
'chdir -' is just supported by the shell, and the C interface could
use chdir(OLDPWD).
^ permalink raw reply
* Re: git-push [--all] and tags
From: Linus Torvalds @ 2007-10-07 16:39 UTC (permalink / raw)
To: martin f krafft; +Cc: git discussion list, Sam Vilain
In-Reply-To: <20071007093636.GA3568@lapse.madduck.net>
On Sun, 7 Oct 2007, martin f krafft wrote:
>
> So am I right if I say that all the logic should really be happening
> in send-pack and that push is really just an interface when it comes
> to selecting the refs to push, so it should basically feed through
> the options and refs, meaning that send-pack should get --tags and
> --shared as well?
I really don't have any strong personal opinions. I don't think anybody is
ever supposed to use send-pack directly, so I don't think it really much
matters whether send-pack is taught to do all the helper options too, or
whether it should just get the list of refs..
But yes, I suspect it would be cleanest to just remove the "expand --tags"
logic from builtin-push, and make the actual sending side do that.
> Or should push enumerate all refs needed and pass them directly to
> send-pack, effectively making send-pack's --all option obsolete?
I don't think this works very well - builtin-push doesn't even know what
the remote branches _are_, so it cannot list "these branches are shared".
So we'd always have to have that shared behaviour embedded in send-pack
anyway, so I think the most logical thing is to also do the logic for
--all and --tags there.
IOW, builtin-push would just be a wrapper around send-pack, doing the
"for each remote" thing, but just passing down all/tags/shared.
Linus
^ permalink raw reply
* Re: [PATCH] Make git-clean a builtin
From: Johannes Schindelin @ 2007-10-07 16:38 UTC (permalink / raw)
To: Shawn Bohrer; +Cc: git, frank, gitster
In-Reply-To: <1191719841666-git-send-email-shawn.bohrer@gmail.com>
Hi,
On Sat, 6 Oct 2007, Shawn Bohrer wrote:
> +static int remove_directory(const char *path)
Please use remove_directory_recursively(), introduced in commit
7155b727c9baae9ef6d7829370aefc09c4ab64e2 to 'next'.
I have not looked at the rest of your patch yet.
Ciao,
Dscho
^ permalink raw reply
* Re: git fetch -- double fetch
From: Johannes Schindelin @ 2007-10-07 16:29 UTC (permalink / raw)
To: Andy Whitcroft; +Cc: git
In-Reply-To: <20071006185759.GE28610@shadowen.org>
Hi,
On Sat, 6 Oct 2007, Andy Whitcroft wrote:
> I have recently been seeing repeated fetching of some branches. I feel
> this has happened in at least three of my repos on three distinct
> projects:
>
> apw@pinky$ git fetch origin
> remote: Generating pack...
> remote: Done counting 5 objects.
> remote: Deltifying 5 objects...
> remote: 100% (5/5) done
> Unpacking 5 objects...
> remote: Total 5 (delta 0), reused 0 (delta 0)
> 100% (5/5) done
> * refs/remotes/origin/master: fast forward to branch 'master' of ssh://git@abat-dev/var/www/git/abat
> old..new: ce046f0..41c9dde
> * refs/remotes/origin/master: fast forward to branch 'master' of ssh://git@abat-dev/var/www/git/abat
> old..new: ce046f0..41c9dde
What does "git config --get-all remote.origin.fetch" say?
Ciao,
Dscho
^ permalink raw reply
* Re: Question about "git commit -a"
From: Johannes Schindelin @ 2007-10-07 16:26 UTC (permalink / raw)
To: Marko Macek
Cc: Dmitry Potapov, Kristian Høgsberg, Andreas Ericsson,
Paolo Ciarrocchi, Nguyen Thai Ngoc Duy, Wincent Colaiuta,
Git Mailing List, andyparkins, torvalds
In-Reply-To: <470878CB.2010609@gmx.net>
Hi,
On Sun, 7 Oct 2007, Marko Macek wrote:
> Dmitry Potapov wrote:
> > You don't. Even with 'commit -a' there is no guarantee that the
> > result will compile, because you can forget to add a new file.
>
> Actually, it would be a good idea for commit to report an error if there
> are any new files that have not been 'added' or 'ignored' (or even if
> there are missing files that have not been 'deleted'.
It is no error.
And it is reported. That is the whole _point_ of having git status output
both changed-but-not-staged and untracked files.
Of course, you only see that when you do not provide the message within
the editor, but from the command line. But then chances are that your
message is too short anyway.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] Make strbuf_cmp inline, constify its arguments and optimize it a bit
From: David Kastrup @ 2007-10-07 16:27 UTC (permalink / raw)
To: Alex Riesen; +Cc: git
In-Reply-To: <20071007161012.GB3270@steel.home>
Alex Riesen <raa.lkml@gmail.com> writes:
> David Kastrup, Sun, Oct 07, 2007 16:24:57 +0200:
>> Alex Riesen <raa.lkml@gmail.com> writes:
>>
>> > It is definitely less code (also object code). It is not always
>> > measurably faster (but mostly is).
>>
>> > -int strbuf_cmp(struct strbuf *a, struct strbuf *b)
>> > -{
>> > - int cmp;
>> > - if (a->len < b->len) {
>> > - cmp = memcmp(a->buf, b->buf, a->len);
>> > - return cmp ? cmp : -1;
>> > - } else {
>> > - cmp = memcmp(a->buf, b->buf, b->len);
>> > - return cmp ? cmp : a->len != b->len;
>> > - }
>> > -}
>> > -
>>
>> > +static inline int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)
>> > +{
>> > + int len = a->len < b->len ? a->len: b->len;
>> > + int cmp = memcmp(a->buf, b->buf, len);
>> > + if (cmp)
>> > + return cmp;
>> > + return a->len < b->len ? -1: a->len != b->len;
>> > +}
>>
>> My guess is that you are conflating two issues about speed here: the
>> inlining will like speed the stuff up. But having to evaluate the
>> (a->len < b->len) comparison twice will likely slow it down.
>
> Can't the result of the expression be reused in compiled?
> Isn't it a common expression?
No, since the call to memcmp might change a->len or b->len. A
standard-compliant C compiler can't make assumptions about what memcmp
might or might not touch unless both a and b can be shown to refer to
variables with an address never passed out of the scope of the
compilation unit.
>> So if you do any profiling, you should do it on both separate
>> angles of this patch.
>
> I compared the inlined versions of both.
Interesting.
--
David Kastrup, Kriemhildstr. 15, 44793 Bochum
^ permalink raw reply
* Re: [PATCH] Make strbuf_cmp inline, constify its arguments and optimize it a bit
From: Timo Hirvonen @ 2007-10-07 16:18 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Pierre Habouzit, Alex Riesen, git, Junio C Hamano
In-Reply-To: <Pine.LNX.4.64.0710071710190.4174@racer.site>
Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Sun, 7 Oct 2007, Pierre Habouzit wrote:
>
> > On Sun, Oct 07, 2007 at 02:24:25PM +0000, Timo Hirvonen wrote:
> >
> > > strbuf->buf is always non-NULL and NUL-terminated so you could just do
> > >
> > > static inline int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)
> > > {
> > > int len = a->len < b->len ? a->len : b->len;
> > > return memcmp(a->buf, b->buf, len + 1);
> > > }
> >
> > doesn't work, because a buffer can have (in some very specific cases)
> > an embeded NUL.
>
> But it should work. The function memcmp() could not care less if there is
> a NUL or not, it just compares until it finds a difference.
Almost. If a is "hello\0world" and b is "hello" then it would compare 6
characters from both and think the strings are equal.
^ permalink raw reply
* Re: [PATCH] Make strbuf_cmp inline, constify its arguments and optimize it a bit
From: Johannes Schindelin @ 2007-10-07 16:11 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: Timo Hirvonen, Alex Riesen, git, Junio C Hamano
In-Reply-To: <20071007143912.GB10024@artemis.corp>
Hi,
On Sun, 7 Oct 2007, Pierre Habouzit wrote:
> On Sun, Oct 07, 2007 at 02:24:25PM +0000, Timo Hirvonen wrote:
>
> > strbuf->buf is always non-NULL and NUL-terminated so you could just do
> >
> > static inline int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)
> > {
> > int len = a->len < b->len ? a->len : b->len;
> > return memcmp(a->buf, b->buf, len + 1);
> > }
>
> doesn't work, because a buffer can have (in some very specific cases)
> an embeded NUL.
But it should work. The function memcmp() could not care less if there is
a NUL or not, it just compares until it finds a difference.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] Make strbuf_cmp inline, constify its arguments and optimize it a bit
From: Alex Riesen @ 2007-10-07 16:10 UTC (permalink / raw)
To: David Kastrup; +Cc: git
In-Reply-To: <85fy0nknnq.fsf@lola.goethe.zz>
David Kastrup, Sun, Oct 07, 2007 16:24:57 +0200:
> Alex Riesen <raa.lkml@gmail.com> writes:
>
> > It is definitely less code (also object code). It is not always
> > measurably faster (but mostly is).
>
> > -int strbuf_cmp(struct strbuf *a, struct strbuf *b)
> > -{
> > - int cmp;
> > - if (a->len < b->len) {
> > - cmp = memcmp(a->buf, b->buf, a->len);
> > - return cmp ? cmp : -1;
> > - } else {
> > - cmp = memcmp(a->buf, b->buf, b->len);
> > - return cmp ? cmp : a->len != b->len;
> > - }
> > -}
> > -
>
> > +static inline int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)
> > +{
> > + int len = a->len < b->len ? a->len: b->len;
> > + int cmp = memcmp(a->buf, b->buf, len);
> > + if (cmp)
> > + return cmp;
> > + return a->len < b->len ? -1: a->len != b->len;
> > +}
>
> My guess is that you are conflating two issues about speed here: the
> inlining will like speed the stuff up. But having to evaluate the
> (a->len < b->len) comparison twice will likely slow it down.
Can't the result of the expression be reused in compiled?
Isn't it a common expression?
> So if you do any profiling, you should do it on both separate angles
> of this patch.
>
I compared the inlined versions of both.
^ permalink raw reply
* Re: [PATCH] Make strbuf_cmp inline, constify its arguments and optimize it a bit
From: David Kastrup @ 2007-10-07 16:07 UTC (permalink / raw)
To: Miles Bader
Cc: Pierre Habouzit, Timo Hirvonen, Alex Riesen, git, Junio C Hamano
In-Reply-To: <87sl4nlyg0.fsf@catnip.gol.com>
Miles Bader <miles@gnu.org> writes:
> Pierre Habouzit <madcoder@debian.org> writes:
>>> strbuf->buf is always non-NULL and NUL-terminated so you could just do
>>>
>>> static inline int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)
>>> {
>>> int len = a->len < b->len ? a->len : b->len;
>>> return memcmp(a->buf, b->buf, len + 1);
>>> }
>>
>> doesn't work, because a buffer can have (in some very specific cases)
>> an embeded NUL.
>
> Couldn't you then just do:
>
> int len = a->len < b->len ? a->len : b->len;
> int cmp = memcmp(a->buf, b->buf, len);
> if (cmp == 0)
> cmp = b->len - a->len;
> return cmp;
>
> [In the case where one string is a prefix of the other, then the longer
> one is "greater".]
>
> ?
I fail to see where this variant is simpler than what we started the
journey of simplification from.
The only change I consider worth checking from the whole series in
this thread is making the function inline. All the rest pretty much
was worse than what we started from in that it needed to reevaluate
more conditions and turned out more complicated and obfuscate even to
the human reader.
--
David Kastrup, Kriemhildstr. 15, 44793 Bochum
^ permalink raw reply
* Re: [PATCH 1/2] Have a filter_start/filter_end API.
From: Alex Riesen @ 2007-10-07 16:07 UTC (permalink / raw)
To: Pierre Habouzit, Linus Torvalds, Junio C Hamano, git
In-Reply-To: <20071007145355.GC10024@artemis.corp>
Pierre Habouzit, Sun, Oct 07, 2007 16:53:55 +0200:
> On Sat, Oct 06, 2007 at 09:06:21AM +0000, Alex Riesen wrote:
> > If you continue to insist the code is generic enough to justify its
> > residence in strbuf.c, continue reading.
> >
> > First off, what was wrong with dumb
> >
> > void strbuf_make_room(struct strbuf *, size_t newsize);
> >
> > again?
>
> If newsize is >= sb->alloc then the area is reallocated, the pointer
> may move, and the "src" pointer would then be invalidated.
So what? You already _have_ to know it points inside the strbuf, so
you can't expect it to be valid after any serious strbuf operation.
> > what is that for? Why can't the caller just use strbuf_detach? (He
> > already has to pass negative hint somehow, which should be a concious
> > action).
>
> The idea is to have a unified API to deal with both the cases where
> the filtering is known not to work in place by the caller, or for the
> cases where it could if enough space is allocated but that a realloc is
> needed.
this just makes it convoluted and opaque (as in "not transparent")
> > > + if ((size_t)hint >= sb->alloc) {
> > > + void *tmp = strbuf_detach(sb, NULL);
> > > + strbuf_grow(sb, hint);
> > > + return tmp;
> > > + }
> > > +
> > > + return NULL;
> > > +}
> >
> > How can one know when it sb is safe to use after strbuf_end_filter?
>
> We could document it, that's not an issue.
The fact that you _have_to_ document is.
> > It is for the first "if", for example. free() wont free the buf in sb.
> > Oh, right, one can check if returned pointer !NULL. Which just adds
> > more code to handle your API.
>
> I don't get that part. free(NULL) is totally ok.
Not that. One have to store the result of start_filter and check it
> > What actually happens to sb? Is it detached? Is it reallocated?
> > When it is detached and when it is reallocated?
>
> It is detached if the filter does not works in place (caller says that
> with '-1' as a hint) or if it works in place but needs a buffer
> reallocation.
Too many if's. Ugly
> Note that I did not created this semantics, it was how convert.c
> worked already, in a even more convoluted way before.
And why shouldn't these semantics kept to convert.c?
^ permalink raw reply
* Re: Repository specific config file
From: Johannes Schindelin @ 2007-10-07 16:06 UTC (permalink / raw)
To: Pekka Riikonen; +Cc: git
In-Reply-To: <Pine.NEB.4.64.0710070956340.12867@otaku.Xtrmntr.org>
Hi,
On Sun, 7 Oct 2007, Pekka Riikonen wrote:
> Has there been any discussion or considerations adding a repository
> specific config file that would be delivered to all cloned repositories
> automatically? This would allow the publisher of the repository to set
> up some default settings to all developers cloning the repository.
Sure! Just check in the file "gitconfig" and tell the good users that
they can append it to their config with "cat gitconfig >> .git/config".
Or have a script "gitconfig.sh" containing calls to "git config" which the
users can execute.
But you should not _force_ people to have a certain config. It's their
choice.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] Make strbuf_cmp inline, constify its arguments and optimize it a bit
From: Miles Bader @ 2007-10-07 15:46 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: Timo Hirvonen, Alex Riesen, git, Junio C Hamano
In-Reply-To: <20071007143912.GB10024@artemis.corp>
Pierre Habouzit <madcoder@debian.org> writes:
>> strbuf->buf is always non-NULL and NUL-terminated so you could just do
>>
>> static inline int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)
>> {
>> int len = a->len < b->len ? a->len : b->len;
>> return memcmp(a->buf, b->buf, len + 1);
>> }
>
> doesn't work, because a buffer can have (in some very specific cases)
> an embeded NUL.
Couldn't you then just do:
int len = a->len < b->len ? a->len : b->len;
int cmp = memcmp(a->buf, b->buf, len);
if (cmp == 0)
cmp = b->len - a->len;
return cmp;
[In the case where one string is a prefix of the other, then the longer
one is "greater".]
?
-Miles
--
"Suppose He doesn't give a shit? Suppose there is a God but He
just doesn't give a shit?" [George Carlin]
^ permalink raw reply
* Re: [PATCH] Make git-clean a builtin
From: Shawn Bohrer @ 2007-10-07 15:41 UTC (permalink / raw)
To: Linus Torvalds; +Cc: git, frank, gitster
In-Reply-To: <alpine.LFD.0.999.0710061827010.23684@woody.linux-foundation.org>
Thanks for the input.
On Sat, Oct 06, 2007 at 06:31:36PM -0700, Linus Torvalds wrote:
> This looks better, but I think you'd be even better off actually using the
> "read_directory()" interface directly, instead of exec'ing off "git
> ls-files" and parsing the line output.
Perhaps, I'll take a look at how git-ls-files does it and see if I can
do that directly. Since I'm new to git (and C) it will probably take me
a while to re-implement though.
> I also would still worry a bit about 'chdir(x)' and 'chdir("..")', because
> quite frankly, they are *not* mirrors of each other (think symlinks, but
> also error behaviour due to directories that might be non-executable).
> Now, admittedly, if a directory isn't executable, I can imagine other git
> things having problems (anybody want to test?), but that whole pattern is
> just very fragile and not very reliable.
Yes it does seem fragile, but 'chdir("-")' doesn't work in C and I
couldn't find any equivalents. I actually did think about symlinks, and
my code does do the right thing since I test if it is a directory before
doing the 'chdir(x)'. Symlinks are therefore treated as normal files and
removed.
I did not think about non-executable directories, and you are correct
that my code will fail to remove a directory if it is non-executable. I
also tested a git-ls-files with non-executable directories, and it will
fail to show you any files that are more than one level deep for
example:
|-- docs
| |-- contributing
| | `-- patches.txt
| `-- manual.txt
If docs is non-executable it will only return 'docs/manual.txt'
--
Shawn
^ permalink raw reply
* Re: [StGIT PATCH] Better diagnostic for wrong branch configuration.
From: Karl Hasselström @ 2007-10-07 15:25 UTC (permalink / raw)
To: Yann Dirson; +Cc: Catalin Marinas, git
In-Reply-To: <20071007141744.GY26436@nan92-1-81-57-214-146.fbx.proxad.net>
On 2007-10-07 16:17:44 +0200, Yann Dirson wrote:
> Oops, sorry - "Signed-off-by: Yann Dirson <ydirson@altern.org>"
Thanks; I'll include that when I push out my branches tonight.
--
Karl Hasselström, kha@treskal.com
www.treskal.com/kalle
^ permalink raw reply
* Re: [PATCH 1/2] Have a filter_start/filter_end API.
From: Pierre Habouzit @ 2007-10-07 14:53 UTC (permalink / raw)
To: Alex Riesen; +Cc: Linus Torvalds, Junio C Hamano, git
In-Reply-To: <20071006090621.GB2711@steel.home>
[-- Attachment #1: Type: text/plain, Size: 5549 bytes --]
On Sat, Oct 06, 2007 at 09:06:21AM +0000, Alex Riesen wrote:
> Pierre Habouzit, Fri, Oct 05, 2007 22:19:30 +0200:
> > Those are helpers to build functions that transform a buffer into a
> > strbuf, allowing the "buffer" to be taken from the destination buffer.
>
> They are horrible. And very specialized for these "filter" routines.
> To the point where I would move them into the file where they are used
> (convert.c only, isn't it?)
For now they are, but moving them into convert.c is at the very least
awkward.
> If you continue to insist the code is generic enough to justify its
> residence in strbuf.c, continue reading.
>
> First off, what was wrong with dumb
>
> void strbuf_make_room(struct strbuf *, size_t newsize);
>
> again?
If newsize is >= sb->alloc then the area is reallocated, the pointer
may move, and the "src" pointer would then be invalidated.
> > +void *strbuf_start_filter(struct strbuf *sb, const char *src, ssize_t hint)
> > +{
> > + if (src < sb->buf || src >= sb->buf + sb->alloc) {
> > + if (hint > 0 && (size_t)hint >= sb->alloc)
> > + strbuf_grow(sb, hint - sb->len);
> > + return NULL;
> > + }
> > +
> > + if (hint < 0)
> > + return strbuf_detach(sb, NULL);
> what is that for? Why can't the caller just use strbuf_detach? (He
> already has to pass negative hint somehow, which should be a concious
> action).
The idea is to have a unified API to deal with both the cases where
the filtering is known not to work in place by the caller, or for the
cases where it could if enough space is allocated but that a realloc is
needed.
> > + if ((size_t)hint >= sb->alloc) {
> > + void *tmp = strbuf_detach(sb, NULL);
> > + strbuf_grow(sb, hint);
> > + return tmp;
> > + }
> > +
> > + return NULL;
> > +}
>
> How can one know when it sb is safe to use after strbuf_end_filter?
We could document it, that's not an issue.
> It is for the first "if", for example. free() wont free the buf in sb.
> Oh, right, one can check if returned pointer !NULL. Which just adds
> more code to handle your API.
I don't get that part. free(NULL) is totally ok.
> What actually happens to sb? Is it detached? Is it reallocated?
> When it is detached and when it is reallocated?
It is detached if the filter does not works in place (caller says that
with '-1' as a hint) or if it works in place but needs a buffer
reallocation.
> Why is the returned pointer useful only for giving it to
> strbuf_end_filter?
Because the filter works on a {src, len} buffer, that points into the
buffer that we must free later in strbuf_end_filter.
> Take for example your change in crlf_to_git:
> @@ -85,6 +85,7 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
> {
> struct text_stat stats;
> char *dst;
> + void *tmp;
>
> if ((action == CRLF_BINARY) || !auto_crlf || !len)
> return 0;
> @@ -110,9 +111,7 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
> return 0;
> }
>
> - /* only grow if not in place */
> - if (strbuf_avail(buf) + buf->len < len)
> - strbuf_grow(buf, len - buf->len);
> + tmp = strbuf_start_filter(buf, src, len);
> dst = buf->buf;
> if (action == CRLF_GUESS) {
> /*
> @@ -133,13 +132,14 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
> } while (--len);
> }
> strbuf_setlen(buf, dst - buf->buf);
> + strbuf_end_filter(tmp);
> return 1;
> }
>
> And try to rewrite it with the strbuf_make_room:
>
> @@ -110,9 +111,7 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
> return 0;
> }
>
> - /* only grow if not in place */
> - if (strbuf_avail(buf) + buf->len < len)
> - strbuf_grow(buf, len - buf->len);
> + strbuf_make_room(buf, len);
> dst = buf->buf;
> if (action == CRLF_GUESS) {
> /*
>
> The change looks rather simple
but is wrong because you may just break "src". It's not the case here
because len always fits, but it's IMHO the kind of knowledge of the
internals of strbufs we should not rely on.
> > +/*----- filter API -----*/
> > +extern void *strbuf_start_filter(struct strbuf *, const char *, ssize_t);
> > +extern void strbuf_end_filter(void *p);
>
> I find the naming very confusing: what filtering takes place where?
> If strbuf_end_filter is just free, why is it needed at all? For the
> sake of wrapping free()?
Those are not filters, but help writing filters that would have this
prototype:
int some_filter(const char *src, size_t len, struct strbuf *dst);
Allowing [src..src+len[ to be a subpart of `dst'. The usual semantics
of such filters is that it returns 0 if it did nothing, 1 if it wrote in
dst.
This way, you can write filter_all being filter1, filter2, filter3
done one after the other this way:
int filter_all(const char *src, size_t len, struct strbuf *dst) {
int res = 0;
res |= filter1(src, len, dst);
if (res) {
src = dst->buf;
len = dst->len;
}
res |= filter2(src, len, dst);
if (res) {
src = dst->buf;
len = dst->len;
}
return res | filter3(src, len, dst);
}
Note that I did not created this semantics, it was how convert.c
worked already, in a even more convoluted way before.
--
·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: Dmitry Potapov @ 2007-10-07 14:50 UTC (permalink / raw)
To: Marko Macek
Cc: Kristian Høgsberg, Andreas Ericsson, Paolo Ciarrocchi,
Johannes Schindelin, Nguyen Thai Ngoc Duy, Wincent Colaiuta,
Git Mailing List, andyparkins, torvalds
In-Reply-To: <470878CB.2010609@gmx.net>
On 10/7/07, Marko Macek <Marko.Macek@gmx.net> wrote:
> Dmitry Potapov wrote:
> > You don't. Even with 'commit -a' there is no guarantee that the
> > result will compile, because you can forget to add a new file.
>
> Actually, it would be a good idea for commit to report an error if there
> are any new files that have not been 'added' or 'ignored'
If it is good for you, you can add this check to your pre-commit hook.
However, I don't like your idea at all. Sometimes, you do want to have
some file that is not 'added' or 'ignored' as a reminder that you have
something else to do. IMHO, git acts in the most reasonable way in this
respect. When you say 'commit -a' and you have some files not added,
it will show you all untracked files in your editor when you type a
commit message. So, it is more difficult to forget to add a new file
with git than with many other version control systems.
> (or even
> if there are missing files that have not been 'deleted'.
Actually, 'commit -a' will automatically delete all missing files.
IMHO, it is the right thing to do.
Dmitry
^ permalink raw reply
* Re: [PATCH] Make strbuf_cmp inline, constify its arguments and optimize it a bit
From: Pierre Habouzit @ 2007-10-07 14:39 UTC (permalink / raw)
To: Timo Hirvonen; +Cc: Alex Riesen, git, Junio C Hamano
In-Reply-To: <20071007172425.bb691da9.tihirvon@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 933 bytes --]
On Sun, Oct 07, 2007 at 02:24:25PM +0000, Timo Hirvonen wrote:
> Alex Riesen <raa.lkml@gmail.com> wrote:
>
> > +static inline int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)
> > +{
> > + int len = a->len < b->len ? a->len: b->len;
> > + int cmp = memcmp(a->buf, b->buf, len);
> > + if (cmp)
> > + return cmp;
> > + return a->len < b->len ? -1: a->len != b->len;
> > +}
>
> strbuf->buf is always non-NULL and NUL-terminated so you could just do
>
> static inline int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)
> {
> int len = a->len < b->len ? a->len : b->len;
> return memcmp(a->buf, b->buf, len + 1);
> }
doesn't work, because a buffer can have (in some very specific cases)
an embeded NUL.
--
·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: [PATCH] Make strbuf_cmp inline, constify its arguments and optimize it a bit
From: David Kastrup @ 2007-10-07 14:24 UTC (permalink / raw)
To: git
In-Reply-To: <20071007140052.GA3260@steel.home>
Alex Riesen <raa.lkml@gmail.com> writes:
> It is definitely less code (also object code). It is not always
> measurably faster (but mostly is).
> -int strbuf_cmp(struct strbuf *a, struct strbuf *b)
> -{
> - int cmp;
> - if (a->len < b->len) {
> - cmp = memcmp(a->buf, b->buf, a->len);
> - return cmp ? cmp : -1;
> - } else {
> - cmp = memcmp(a->buf, b->buf, b->len);
> - return cmp ? cmp : a->len != b->len;
> - }
> -}
> -
> +static inline int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)
> +{
> + int len = a->len < b->len ? a->len: b->len;
> + int cmp = memcmp(a->buf, b->buf, len);
> + if (cmp)
> + return cmp;
> + return a->len < b->len ? -1: a->len != b->len;
> +}
My guess is that you are conflating two issues about speed here: the
inlining will like speed the stuff up. But having to evaluate the
(a->len < b->len) comparison twice will likely slow it down.
So if you do any profiling, you should do it on both separate angles
of this patch.
--
David Kastrup, Kriemhildstr. 15, 44793 Bochum
^ permalink raw reply
* Re: [PATCH] Make strbuf_cmp inline, constify its arguments and optimize it a bit
From: Timo Hirvonen @ 2007-10-07 14:24 UTC (permalink / raw)
To: Alex Riesen; +Cc: git, Junio C Hamano, Pierre Habouzit
In-Reply-To: <20071007140052.GA3260@steel.home>
Alex Riesen <raa.lkml@gmail.com> wrote:
> +static inline int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)
> +{
> + int len = a->len < b->len ? a->len: b->len;
> + int cmp = memcmp(a->buf, b->buf, len);
> + if (cmp)
> + return cmp;
> + return a->len < b->len ? -1: a->len != b->len;
> +}
strbuf->buf is always non-NULL and NUL-terminated so you could just do
static inline int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)
{
int len = a->len < b->len ? a->len : b->len;
return memcmp(a->buf, b->buf, len + 1);
}
^ permalink raw reply
* Re: [StGIT PATCH] Better diagnostic for wrong branch configuration.
From: Yann Dirson @ 2007-10-07 14:17 UTC (permalink / raw)
To: Karl Hasselström; +Cc: Catalin Marinas, git
In-Reply-To: <20071007131417.GA28492@diana.vm.bytemark.co.uk>
On Sun, Oct 07, 2007 at 03:14:17PM +0200, Karl Hasselström wrote:
> On 2007-10-05 22:44:52 +0200, Yann Dirson wrote:
>
> > If the branch.*.merge parameter does not name a valid remote head,
> > stgit would not rebase after a fetch, and would write instead
> > 'Rebasing to "None" ... done'.
> >
> > This patch makes this situation an error and tells the user what to
> > fix in his repo configuration.
>
> Good. Sign-off?
Oops, sorry - "Signed-off-by: Yann Dirson <ydirson@altern.org>"
> > - raise GitException, "StGit does not support multiple FETCH_HEAD"
> > + raise GitException, 'StGit does not support multiple FETCH_HEAD'
>
> Unrelated quote fixup. No big deal, though.
Right - did not seem to warrant a patch of its own :)
Best regards,
--
Yann
^ permalink raw reply
* [PATCH] Make strbuf_cmp inline, constify its arguments and optimize it a bit
From: Alex Riesen @ 2007-10-07 14:00 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Pierre Habouzit
In-Reply-To: <1190625904-22808-2-git-send-email-madcoder@debian.org>
It is definitely less code (also object code). It is not always
measurably faster (but mostly is).
Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
strbuf.c | 12 ------------
strbuf.h | 9 ++++++++-
2 files changed, 8 insertions(+), 13 deletions(-)
diff --git a/strbuf.c b/strbuf.c
index f4201e1..215837b 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -58,18 +58,6 @@ void strbuf_rtrim(struct strbuf *sb)
sb->buf[sb->len] = '\0';
}
-int strbuf_cmp(struct strbuf *a, struct strbuf *b)
-{
- int cmp;
- if (a->len < b->len) {
- cmp = memcmp(a->buf, b->buf, a->len);
- return cmp ? cmp : -1;
- } else {
- cmp = memcmp(a->buf, b->buf, b->len);
- return cmp ? cmp : a->len != b->len;
- }
-}
-
void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,
const void *data, size_t dlen)
{
diff --git a/strbuf.h b/strbuf.h
index 9b9e861..3116387 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -78,7 +78,14 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len) {
/*----- content related -----*/
extern void strbuf_rtrim(struct strbuf *);
-extern int strbuf_cmp(struct strbuf *, struct strbuf *);
+static inline int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)
+{
+ int len = a->len < b->len ? a->len: b->len;
+ int cmp = memcmp(a->buf, b->buf, len);
+ if (cmp)
+ return cmp;
+ return a->len < b->len ? -1: a->len != b->len;
+}
/*----- add data in your buffer -----*/
static inline void strbuf_addch(struct strbuf *sb, int c) {
--
1.5.3.4.223.g78587
^ permalink raw reply related
* Re: [StGIT PATCH] Better diagnostic for wrong branch configuration.
From: Karl Hasselström @ 2007-10-07 13:14 UTC (permalink / raw)
To: Yann Dirson; +Cc: Catalin Marinas, git
In-Reply-To: <20071005204452.30902.60246.stgit@gandelf.nowhere.earth>
On 2007-10-05 22:44:52 +0200, Yann Dirson wrote:
> If the branch.*.merge parameter does not name a valid remote head,
> stgit would not rebase after a fetch, and would write instead
> 'Rebasing to "None" ... done'.
>
> This patch makes this situation an error and tells the user what to
> fix in his repo configuration.
Good. Sign-off?
> - raise GitException, "StGit does not support multiple FETCH_HEAD"
> + raise GitException, 'StGit does not support multiple FETCH_HEAD'
Unrelated quote fixup. No big deal, though.
--
Karl Hasselström, kha@treskal.com
www.treskal.com/kalle
^ permalink raw reply
* Re: Question about "git commit -a"
From: Wincent Colaiuta @ 2007-10-07 12:26 UTC (permalink / raw)
To: Marko Macek
Cc: Kristian Høgsberg, Andreas Ericsson, Paolo Ciarrocchi,
Johannes Schindelin, Nguyen Thai Ngoc Duy, Git Mailing List
In-Reply-To: <47067F68.2080709@gmx.net>
El 5/10/2007, a las 20:16, Marko Macek escribió:
> In CVS and subversion (which has nicer working-copy command line
> interface IMHO),
> I simply make a copy of the working copy, revert the non-commitable
> parts, build,
> commit the minor changes, and then update the first copy. For
> larger projects,
> where this can be slow, I use diff/revert/patch.
This sounds painful compared to Dmitry's method (pasted below) if you
care about all published changes being buildable and passing all the
tests...
El 5/10/2007, a las 23:10, Dmitry Potapov escribió:
> IMHO, the best practice is to recompile everything step-wise in
> a clean directory before you are going to publish your changes.
> It can be done automatically by script, while you do something
> useful, like reading this mailing-list :)
Cheers,
Wincent
^ 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