* [PATCH 2/2 (for GIT-GUI)] git-gui: MERGE_RR lives in .git/ directly with newer Git versions
From: Johannes Schindelin @ 2008-07-12 14:56 UTC (permalink / raw)
To: Kalle Olavi Niemitalo; +Cc: git, gitster, spearce
In-Reply-To: <alpine.DEB.1.00.0807121553120.8950@racer>
Now that MERGE_RR was moved out of .git/rr-cache/, we have to delete
it somewhere else. Just in case somebody wants to use a newer git-gui
with an older Git, the file .git/rr-cache/MERGE_RR is removed, too (if
it exists).
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
git-gui/lib/merge.tcl | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/lib/merge.tcl b/lib/merge.tcl
index cc26b07..5c01875 100644
--- a/lib/merge.tcl
+++ b/lib/merge.tcl
@@ -257,6 +257,7 @@ proc _reset_wait {fd} {
catch {file delete [gitdir MERGE_HEAD]}
catch {file delete [gitdir rr-cache MERGE_RR]}
+ catch {file delete [gitdir MERGE_RR]}
catch {file delete [gitdir SQUASH_MSG]}
catch {file delete [gitdir MERGE_MSG]}
catch {file delete [gitdir GITGUI_MSG]}
--
1.5.6.2.511.ge432a
^ permalink raw reply related
* [PATCH 1/2] Move MERGE_RR from .git/rr-cache/ into .git/
From: Johannes Schindelin @ 2008-07-12 14:56 UTC (permalink / raw)
To: Kalle Olavi Niemitalo; +Cc: git, gitster, spearce
In-Reply-To: <87skufmjg4.fsf@Astalo.kon.iki.fi>
If you want to reuse the rerere cache in another repository, and set
a symbolic link to it, you do not want to have the two repositories
interfer with each other by accessing the _same_ MERGE_RR.
For example, if you use contrib/git-new-workdir to set up a second
working directory, and you have a conflict in one working directory,
but commit in the other working directory first, the wrong "resolution"
will be recorded.
The easy solution is to move MERGE_RR out of the rr-cache/ directory,
which also corresponds with the notion that rr-cache/ contains cached
resolutions, not some intermediate temporary states.
Noticed by Kalle Olavi Niemitalo.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Shawn, I Cc:ed you because of 2/2.
On Sat, 12 Jul 2008, Kalle Olavi Niemitalo wrote:
> I don't see any way to modify the symlinks made by
> git-new-workdir so that new SHA-1 directories in .git/rr-cache
> would be shared but .git/rr-cache/MERGE_RR would not. On IRC,
> "gitte" suggested changing Git to use $GIT_DIR/MERGE_RR instead of
> $GIT_DIR/rr-cache/MERGE_RR.
"gitte" actually expected that a patch would not be that hard.
> I suppose compatibility with people's existing repositories
> would require the modified Git to keep reading
> $GIT_DIR/rr-cache/MERGE_RR too, so that Git could be painlessly
> upgraded during a merge, but it is not obvious to me how lock files
> should then work.
"rerere" is not perfect. Thus, I suspect that we can just leave
an existing MERGE_RR alone, and the user just has to record the
resolution another time. Too bad.
branch.c | 2 +-
builtin-rerere.c | 2 +-
t/t4200-rerere.sh | 6 +++---
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/branch.c b/branch.c
index 56e9492..b1e59f2 100644
--- a/branch.c
+++ b/branch.c
@@ -166,7 +166,7 @@ void create_branch(const char *head,
void remove_branch_state(void)
{
unlink(git_path("MERGE_HEAD"));
- unlink(git_path("rr-cache/MERGE_RR"));
+ unlink(git_path("MERGE_RR"));
unlink(git_path("MERGE_MSG"));
unlink(git_path("SQUASH_MSG"));
}
diff --git a/builtin-rerere.c b/builtin-rerere.c
index 69c3a52..1db2e0c 100644
--- a/builtin-rerere.c
+++ b/builtin-rerere.c
@@ -429,7 +429,7 @@ static int setup_rerere(struct path_list *merge_rr)
if (!is_rerere_enabled())
return -1;
- merge_rr_path = xstrdup(git_path("rr-cache/MERGE_RR"));
+ merge_rr_path = xstrdup(git_path("MERGE_RR"));
fd = hold_lock_file_for_update(&write_lock, merge_rr_path, 1);
read_rr(merge_rr);
return fd;
diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index cf10557..b5a4202 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh
@@ -57,7 +57,7 @@ test_expect_success 'conflicting merge' '
! git merge first
'
-sha1=$(sed -e 's/ .*//' .git/rr-cache/MERGE_RR)
+sha1=$(sed -e 's/ .*//' .git/MERGE_RR)
rr=.git/rr-cache/$sha1
test_expect_success 'recorded preimage' "grep ^=======$ $rr/preimage"
@@ -143,7 +143,7 @@ test_expect_success 'rerere kicked in' "! grep ^=======$ a1"
test_expect_success 'rerere prefers first change' 'test_cmp a1 expect'
rm $rr/postimage
-echo "$sha1 a1" | perl -pe 'y/\012/\000/' > .git/rr-cache/MERGE_RR
+echo "$sha1 a1" | perl -pe 'y/\012/\000/' > .git/MERGE_RR
test_expect_success 'rerere clear' 'git rerere clear'
@@ -190,7 +190,7 @@ test_expect_success 'file2 added differently in two branches' '
git add file2 &&
git commit -m version2 &&
! git merge fourth &&
- sha1=$(sed -e "s/ .*//" .git/rr-cache/MERGE_RR) &&
+ sha1=$(sed -e "s/ .*//" .git/MERGE_RR) &&
rr=.git/rr-cache/$sha1 &&
echo Cello > file2 &&
git add file2 &&
--
1.5.6.2.511.ge432a
^ permalink raw reply related
* Re: tests for git-p4
From: Johannes Schindelin @ 2008-07-12 14:36 UTC (permalink / raw)
To: Matt McClure; +Cc: git
In-Reply-To: <e48c5e540807120524rabdcfa6vd91c16ef1b0f80a0@mail.gmail.com>
Hi,
On Sat, 12 Jul 2008, Matt McClure wrote:
> Would my tests be of interest to git-p4's authors? Is there any
> existing test framework for git-p4 that I missed?
git$ git shortlog -e -s -n -- contrib/fast-import/git-p4* | head -n 5
102 Simon Hausmann <simon@lst.de>
50 Simon Hausmann <shausman@trolltech.com>
31 Han-Wen Nienhuys <hanwen@google.com>
13 Simon Hausmann <hausmann@kde.org>
7 Marius Storm-Olsen <marius@trolltech.com>
Hth,
Dscho
^ permalink raw reply
* Re: [RFC/PATCH (WIP)] Git.pm: Add get_config() method and related subroutines
From: Jakub Narebski @ 2008-07-12 14:31 UTC (permalink / raw)
To: Petr Baudis; +Cc: git, Lea Wiemann
In-Reply-To: <20080712134553.GC10151@machine.or.cz>
On Sat, Jul 12, 2008, Petr Baudis wrote:
> On Sat, Jul 12, 2008 at 02:35:48PM +0200, Jakub Narebski wrote:
>> On Sat, Jul 12, 2008, Petr Baudis wrote:
>> On the other hand if one uses (assuming now Git::Config object API)
>>
>> $conf = $repo->get_config('section');
>>
>> one could access configuration variable values _without_ prefixing
>> them with subsection name, i.e.
>>
>> $conf->get('variable');
>>
>> and not
>>
>> $conf->get('section.variable');
>>
>>
>> I'm not sure if it is much of an improvement.
>
> I would propose not to go with this feature yet and add it only when
> justified by accompanied cleanups of real code using this.
O.K.
>> The former plays better with
>>
>> $r->get_config()->bool('gitweb.blame');
>>
>> but this is nevertheless not recommended workflow; you can use
>>
>> $r->config_bool('gitweb.blame');
>>
>> and it would be faster (unless some Perl/OO trickery with singletons
>> and the like).
>>
>> Recommended workflow (code) is
>>
>> $c = $r->get_config();
>> ...
>> $c->get_bool('gitweb.blame');
>>
>> or something like that.
>
> I thought that the get_config() method would be reusing existing
> Git::Config instances no-matter-what? (Otherwise, you can get into
> rather tricky issues like one part of the code modifying the config and
> the other not noticing the change, etc. And I can se no benefit. If you
> want to reload the config, we might have method like reload(), but I
> don't think that would be very useful except perhaps in some kind of
> mod_perl'd gitweb scenario.)
Somehow I forgot that $repo can remember existing Git::Config, generate
it only once (and remember it) on demand but fully, and then serve it.
It probably is some OO "pattern" like SingletonFactory or sth...
It would probably play better with Git::Repo object interface, but can
I think work as well with Git->repository(...) instance.
As to changing config: I wasn't even thinking about that... and I don't
think that any command written in Perl which uses or can use either
Git or Git::Repo API needs writing config files.
> But we're arguing about nothing, really, except the exact amount of
> coolness of the Git::Config approach. :-)
True. For me the fact that one can write $c->value('diff.renameLimit')
or $c->value('diff.renamelimit'); is teh winner ;-)
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [RFC/PATCH (WIP)] Git.pm: Add get_config() method and related subroutines
From: Petr Baudis @ 2008-07-12 13:45 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git, Lea Wiemann
In-Reply-To: <200807121435.49151.jnareb@gmail.com>
On Sat, Jul 12, 2008 at 02:35:48PM +0200, Jakub Narebski wrote:
> On Sat, Jul 12, 2008, Petr Baudis wrote:
> > Or if you mean foreach use, it's trivial to
> >
> > foreach (grep { /^gitweb\./ } keys %$config)
> >
> > or provide a method of Git::Config that will return this list, but it
> > does not seem appropriate to have specific Git::Config instances for
> > this. (Besides, if the script also needs to access a single variable
> > from a different section, it will need to re-parse the config again.)
> >
> > So I think your approach would be good only if there are multiple
> > methods of Git::Config that would operate on the whole config and needed
> > a way to be restricted; is that the case?
>
> On the other hand if one uses (assuming now Git::Config object API)
>
> $conf = $repo->get_config('section');
>
> one could access configuration variable values _without_ prefixing
> them with subsection name, i.e.
>
> $conf->get('variable');
>
> and not
>
> $conf->get('section.variable');
>
>
> I'm not sure if it is much of an improvement.
I would propose not to go with this feature yet and add it only when
justified by accompanied cleanups of real code using this.
> We can have $c->value(<VAR>), $c->bool(<VAR>), $c->int(<VAR>) and
> (in the future) $c->color(<VAR>) and $c->colorbool(<VAR>)... or we can
> have $c->get(<VAR>), $c->get_bool(<VAR>) etc.
I think I prefer the terser approach of value/bool/int now since it is
shorter and seems to stay pretty unambiguous. Besides, this way we can
easily make mutators by just adding second optional argument to these
methods.
> The former plays better with
>
> $r->get_config()->bool('gitweb.blame');
>
> but this is nevertheless not recommended workflow; you can use
>
> $r->config_bool('gitweb.blame');
>
> and it would be faster (unless some Perl/OO trickery with singletons
> and the like).
>
> Recommended workflow (code) is
>
> $c = $r->get_config();
> ...
> $c->get_bool('gitweb.blame');
>
> or something like that.
I thought that the get_config() method would be reusing existing
Git::Config instances no-matter-what? (Otherwise, you can get into
rather tricky issues like one part of the code modifying the config and
the other not noticing the change, etc. And I can se no benefit. If you
want to reload the config, we might have method like reload(), but I
don't think that would be very useful except perhaps in some kind of
mod_perl'd gitweb scenario.)
> >>> Also, having accessors for special types lets you return undef when
> >>> the type really isn't defined, instead of e.g. true with current
> >>> config_val_bool, which is clearly bogus and requires complicated code
> >>> on the user side.
> >>
> >> I don't follow you. Didn't we agree on casting an error when variable
> >> is not of given type?
> >
> > Sorry, s/type really/variable really/. According to your original code,
> >
> > config_val_bool(undef)
> >
> > would return true, while the undef could be from both non-existent and
> > unassigned variable. (This 'unassigned variables' case is really
> > annoying to handle.)
>
> That is why you should check exists($config{<VAR>}) first.
And that was my point when I talked about complicated code on the user
side.
But we're arguing about nothing, really, except the exact amount of
coolness of the Git::Config approach. :-)
--
Petr "Pasky" Baudis
GNU, n. An animal of South Africa, which in its domesticated state
resembles a horse, a buffalo and a stag. In its wild condition it is
something like a thunderbolt, an earthquake and a cyclone. -- A. Pierce
^ permalink raw reply
* Re: [PATCH 3/3] imap-send.c: Clean up coding style to better match the rest of the git codebase.
From: Jakub Narebski @ 2008-07-12 12:38 UTC (permalink / raw)
To: Rob Shearman; +Cc: Junio C Hamano, git
In-Reply-To: <1096648c0807081621x669a9bc4ie484fd004674918d@mail.gmail.com>
On Wed, 9 July 2008, Rob Shearman wrote:
> 2008/7/7 Jakub Narebski <jnareb@gmail.com>:
> > or use either git-send-email
> > (I think you can send patches also via Gmail) or git-imap-send,
>
> Ok, but git-imap-send only puts the emails into your Drafts folder so
> that you can send them with your normal email client (which is what
> does the linewrapping).
Errr... if you are using normal email client, then it should be
possible to turn off line wrapping in the client, isn't it?
[...]
> > then preferred solution is to have patch inline.
> >
> > If it is not possible, then attach the patch, but preferrably
> > 1) with text/plain mimetype (.txt extension instead of .patch or
> > .diff could be required for that)
> >
> > 2) 8bit (preferred) or quoted-printable (if 8bit is not possible)
> > transfer encoding (base64 is terrible waste of space); text/plain
> > should chose quoted-printable at worst
> >
> > 3) use "inline" attachement (select 'suggest to display attachement'
> > or something like that), so it is possible to select attachement
> > and hit reply.
>
> I think this was the case with the patches originally sent, but I used
> "git format-patch --attach --stdout ... | git imap-send" so if the
> mails didn't meet one of those points then it should be fixed in
> git-format-patch.
"git format-patch --inline --stdout", perhaps?
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [RFC/PATCH (WIP)] Git.pm: Add get_config() method and related subroutines
From: Jakub Narebski @ 2008-07-12 12:35 UTC (permalink / raw)
To: Petr Baudis; +Cc: git, Lea Wiemann
In-Reply-To: <20080712014708.GB10151@machine.or.cz>
On Sat, Jul 12, 2008, Petr Baudis wrote:
> On Thu, Jul 10, 2008 at 01:33:36AM +0200, Jakub Narebski wrote:
>> On Wed, Jul 09, 2008, Petr "Pasky" Baudis wrote:
>>> On Thu, Jul 03, 2008 at 06:24:53PM +0200, Jakub Narebski wrote:
>>>> * What should ->get_config() have as an optional parameter:
>>>> PREFIX (/^$prefix/o), or simply SECTION (/^(?:$section)\./o)?
>>>
>>> Do we even _need_ a parameter like that? I don't understand what is
>>> this interface trying to address.
>>
>> For example if one wants to access _all_ variables in gitweb.* section
>> (or in gitcvs.* section), and _only_ config variables in given section.
>
> But what is the practical benefit? Does anyone use a config file long
> enough that this makes any difference?
Probably not.
> Or if you mean foreach use, it's trivial to
>
> foreach (grep { /^gitweb\./ } keys %$config)
>
> or provide a method of Git::Config that will return this list, but it
> does not seem appropriate to have specific Git::Config instances for
> this. (Besides, if the script also needs to access a single variable
> from a different section, it will need to re-parse the config again.)
>
> So I think your approach would be good only if there are multiple
> methods of Git::Config that would operate on the whole config and needed
> a way to be restricted; is that the case?
On the other hand if one uses (assuming now Git::Config object API)
$conf = $repo->get_config('section');
one could access configuration variable values _without_ prefixing
them with subsection name, i.e.
$conf->get('variable');
and not
$conf->get('section.variable');
I'm not sure if it is much of an improvement.
>> BTW. what should non-typecasting should be named? $c->get(<VAR>),
>> $c->value(<VAR>), $c->param(<VAR>), or something yet different?
>
> I would prefer 'get' since it's the shortest and most clear, but 'value'
> would be fine too, I suppose (and more in line with bool etc.).
Well, that would have to be decided before other programs could use
this API.
We can have $c->value(<VAR>), $c->bool(<VAR>), $c->int(<VAR>) and
(in the future) $c->color(<VAR>) and $c->colorbool(<VAR>)... or we can
have $c->get(<VAR>), $c->get_bool(<VAR>) etc.
The former plays better with
$r->get_config()->bool('gitweb.blame');
but this is nevertheless not recommended workflow; you can use
$r->config_bool('gitweb.blame');
and it would be faster (unless some Perl/OO trickery with singletons
and the like). Recommended workflow (code) is
$c = $r->get_config();
...
$c->get_bool('gitweb.blame');
or something like that.
>>> Also, having accessors for special types lets you return undef when
>>> the type really isn't defined, instead of e.g. true with current
>>> config_val_bool, which is clearly bogus and requires complicated code
>>> on the user side.
>>
>> I don't follow you. Didn't we agree on casting an error when variable
>> is not of given type?
>
> Sorry, s/type really/variable really/. According to your original code,
>
> config_val_bool(undef)
>
> would return true, while the undef could be from both non-existent and
> unassigned variable. (This 'unassigned variables' case is really
> annoying to handle.)
That is why you should check exists($config{<VAR>}) first.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: git pull is slow
From: Stephan Hennig @ 2008-07-12 12:32 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Andreas Ericsson
In-Reply-To: <alpine.DEB.1.00.0807111443280.8950@racer>
Hi,
Johannes Schindelin schrieb:
> On Fri, 11 Jul 2008, Andreas Ericsson wrote:
>
>> Seems like you're being bitten by a bug we had some months back,
>> where the client requested full history for new tag objects.
>
> I do not think so. I think it is a problem with the pack. The slowness
> is already there in the clone, in the resolving phase.
Thanks for having a look at this! What does "problem with the pack"
mean? Do you think it is a Git problem (client or server side?) or just
a misconfiguration?
Best regards,
Stephan Hennig
^ permalink raw reply
* tests for git-p4
From: Matt McClure @ 2008-07-12 12:24 UTC (permalink / raw)
To: git
I'm planning to write a "p4-fast-export" that will facilitate using
Bazaar on projects maintained in Perforce. I'd like to reuse code
from git-p4 since it appears to have much of the functionality I want.
In fact, the main reason I can't just use git-p4 today is that it
does too much: it pushes the Perforce fast-export representation
directly into Git. I'm going to work on covering the existing
functionality in tests before I start modifying the code, and I could
try to write those tests such that they'll be useful to the Git
project as well.
Would my tests be of interest to git-p4's authors? Is there any
existing test framework for git-p4 that I missed?
Matt
^ permalink raw reply
* git-new-workdir should not share .git/rr-cache/MERGE_RR
From: Kalle Olavi Niemitalo @ 2008-07-12 11:27 UTC (permalink / raw)
To: git
The contrib/workdir/git-new-workdir script makes .git/rr-cache in
each workdir a symlink to the shared .git/rr-cache directory.
This lets git-rerere share conflict resolutions between workdirs.
However, that directory also contains .git/rr-cache/MERGE_RR,
which lists the files for which conflict resolutions should be
saved in the next commit. It is thus possible that git-merge in
one workdir records conflicts to the shared MERGE_RR file, and
git-commit in another workdir saves the staged files to rr-cache
as conflict resolutions, even though those files are from a
different branch and never saw the merge. Such invalid conflict
resolutions need then be separately deleted. This happened to
me, with git version 1.5.6.
I don't see any way to modify the symlinks made by
git-new-workdir so that new SHA-1 directories in .git/rr-cache
would be shared but .git/rr-cache/MERGE_RR would not. On IRC,
"gitte" suggested changing Git to use $GIT_DIR/MERGE_RR instead
of $GIT_DIR/rr-cache/MERGE_RR. I suppose compatibility with
people's existing repositories would require the modified Git to
keep reading $GIT_DIR/rr-cache/MERGE_RR too, so that Git could be
painlessly upgraded during a merge, but it is not obvious to me
how lock files should then work.
Alternatively, the SHA-1 directories in .git/rr-cache could be
moved to a subdirectory; then git-new-workdir could be changed to
symlink only that subdirectory.
^ permalink raw reply
* Automatic cherry-pick failure
From: Jelle Geerts @ 2008-07-12 11:55 UTC (permalink / raw)
To: git
Hello,
When I tried to `git rebase -i', I changed the first line in the editor from
'pick' to 'edit'. After the editor exited, git complained:
error: Untracked working tree file 'DESIGN.TXT' would be overwritten by merge.
But this file is tracked, so first I did `git rebase --abort'.
I had to use `git config --bool core.ignorecase true' to get around the error.
It's because I use Git with Cygwin on an NTFS partition. Only thing I is that
'core.ignorecase' is undocumented.
After getting around the Cygwin specific workaround, I reran rebase. Again, I
edited the first line from 'pick' to 'edit'. All went well. Then I made the
amendment to the commit with `git commit --amend --author="me <me@somewhere>"'.
Now, when I typed `git rebase --continue' I got this:
error: Entry 'code/src/sys_main.cc' not uptodate. Cannot merge.
fatal: merging of trees e3249e935c9ccd790c372aac82b8e097044b1ffe and
ac79e613ed0fe912e0da2c5ccf52f6a540533cb8 failed
Automatic cherry-pick failed. After resolving the conflicts,
mark the corrected paths with 'git add <paths>', and
run 'git rebase --continue'
Could not apply 5c03af9... commit test
I'm not sure what I have to do now, can someone help me with this?
-- Jelle
^ permalink raw reply
* [PATCH] t0004: fix timing bug
From: Junio C Hamano @ 2008-07-12 11:14 UTC (permalink / raw)
To: git
The test created an initial commit, made .git/objects unwritable and then
exercised various codepaths to create loose commit, tree and blob objects
to make sure the commands notice failures from these attempts.
However, the initial commit was not preceded with test_tick, which made
its object name depend on the timestamp. The names of all the later tree
and blob objects the test tried to create were static. If the initial
commit's object name happened to begin with the same two hexdigits as the
tree or blob objects the test later attempted to create, the fan-out
directory in which these tree or blob would be created is already created
when the initial commit was made, and the object creation succeeds, and
commands being tested should not notice any failure --- in short, the test
was bogus.
This makes the fan-out directories also unwritable, and adds test_tick
before the commit object creation to make the test repeatable.
The contents of the file to create a blob from "a" to "60" is to force the
name of the blob object to begin with "1b", which shares the fan-out
directory with the initial commit that is created with the test. This was
useful when diagnosing the breakage of this test.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* I initially hoped that I spotted a bug in the Linux filesystem code
that lets a quick operation sneak past chmod that is done immediately
after it or something racy so that I can tease Linus with it, but no
such luck ;-) Doubly embarrasing is that the test-bug is mine.
t/t0004-unwritable.sh | 19 ++++++++++---------
1 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/t/t0004-unwritable.sh b/t/t0004-unwritable.sh
index 9255c63..63e1217 100755
--- a/t/t0004-unwritable.sh
+++ b/t/t0004-unwritable.sh
@@ -8,6 +8,7 @@ test_expect_success setup '
>file &&
git add file &&
+ test_tick &&
git commit -m initial &&
echo >file &&
git add file
@@ -17,11 +18,11 @@ test_expect_success setup '
test_expect_success 'write-tree should notice unwritable repository' '
(
- chmod a-w .git/objects
+ chmod a-w .git/objects .git/objects/?? &&
test_must_fail git write-tree
)
status=$?
- chmod 775 .git/objects
+ chmod 775 .git/objects .git/objects/??
(exit $status)
'
@@ -29,11 +30,11 @@ test_expect_success 'write-tree should notice unwritable repository' '
test_expect_success 'commit should notice unwritable repository' '
(
- chmod a-w .git/objects
+ chmod a-w .git/objects .git/objects/?? &&
test_must_fail git commit -m second
)
status=$?
- chmod 775 .git/objects
+ chmod 775 .git/objects .git/objects/??
(exit $status)
'
@@ -41,12 +42,12 @@ test_expect_success 'commit should notice unwritable repository' '
test_expect_success 'update-index should notice unwritable repository' '
(
- echo a >file &&
- chmod a-w .git/objects
+ echo 6O >file &&
+ chmod a-w .git/objects .git/objects/?? &&
test_must_fail git update-index file
)
status=$?
- chmod 775 .git/objects
+ chmod 775 .git/objects .git/objects/??
(exit $status)
'
@@ -55,11 +56,11 @@ test_expect_success 'add should notice unwritable repository' '
(
echo b >file &&
- chmod a-w .git/objects
+ chmod a-w .git/objects .git/objects/?? &&
test_must_fail git add file
)
status=$?
- chmod 775 .git/objects
+ chmod 775 .git/objects .git/objects/??
(exit $status)
'
^ permalink raw reply related
* Re: [StGit PATCH 2/2] Reuse the same temp index in a transaction
From: Catalin Marinas @ 2008-07-12 10:24 UTC (permalink / raw)
To: Karl Hasselström; +Cc: git
In-Reply-To: <20080702061314.11361.28297.stgit@yoghurt>
2008/7/2 Karl Hasselström <kha@treskal.com>:
> Instead of making a new temp index every time we need one, just keep
> reusing the same one. And keep track of which tree is currently stored
> in it -- if we do several consecutive successful pushes, it's always
> going to be the "right" tree so that we don't have to call read-tree
> before each patch application.
>
> The motivation behind this change is of course that it makes things
> faster.
>
> (The same simple test as in the previous patch -- pushing 250 patches
> in a 32k-file repository, with one file-level merge necessary per push
> -- went from 0.36 to 0.19 seconds per patch with this patch applied.)
That's an impressive improvement (together with the previous patch).
Is this with the new patch log infrastructure?
Thanks.
--
Catalin
^ permalink raw reply
* Re: [StGit PATCH 1/2] Do simple in-index merge with diff+apply instead of read-tree
From: Catalin Marinas @ 2008-07-12 10:22 UTC (permalink / raw)
To: Karl Hasselström; +Cc: git
In-Reply-To: <20080702061243.11361.75544.stgit@yoghurt>
2008/7/2 Karl Hasselström <kha@treskal.com>:
> The advantage is that patch application will resolve some file content
> conflicts for us, so that we'll fall back to merge-recursive less
> often. This is a significant speedup, especially since merge-recursive
> needs to touch the worktree, which means we have to check out the
> index first.
Thanks.
--
Catalin
^ permalink raw reply
* Re: [StGit PATCH 03/14] Write to a stack log when stack is modified
From: Catalin Marinas @ 2008-07-12 10:09 UTC (permalink / raw)
To: Karl Hasselström; +Cc: git
In-Reply-To: <20080623123631.GA18347@diana.vm.bytemark.co.uk>
2008/6/23 Karl Hasselström <kha@treskal.com>:
> On 2008-06-20 10:14:29 +0100, Catalin Marinas wrote:
>
>> 2008/6/19 Karl Hasselström <kha@treskal.com>:
>> > I'd actually say the opposite: until we have a good visualizer
>> > that doesn't need the simplified log, we need to have the
>> > simplified log. If I actually have to look at the diffs in the
>> > log, I find gitk indispensible.
>>
>> And what would the simplified log contain if we decide to go with a
>> new scheme? In your proposal, it points to the tree of main log and
>> you get the diff of diffs (which also means that the diffs must be
>> generated for every modification of a patch). Would this be the
>> same? Again, I worry a bit about the overhead to generate the patch
>> diff for every push (with refresh I'm OK). It can be optimised as in
>> the stable branch where we try git-apply followed by a three-way
>> merge (which, BTW, I'd like added before 0.15). If git-apply
>> succeeds, there is no need to re-generate the diff.
>
> Yes, I was imagining a simplified log precisely like the one I've
> currently implemented (a tree with one blob per patch, which contains
> the message, the diff, and some other odds and ends such as the
> author).
At a first look, I'm OK with a simplified log.
> Two optimizations would hopefully make it fast:
>
> 1. If the patch's sha1 hasn't changed, we don't have to regenerate
> the diff.
>
> 2. If the patch's sha1 has changed, but git apply was sufficient
> during the merge stage, we can just reuse that patch. We do have
> to write it to a blob, but we have already generated the diff and
> don't need to do so again. (I've shamelessly stolen your idea
> here.)
It can be optimised a bit more to actually apply the diff in the blob
directly rather than the current way of generating the diff (since we
don't store the diff).
> In most cases, (1) would make sure that only a small handful of
> patches would need to be considered. In the cases where a lot of
> patches are touched, such as rebase, (2) would provide a good speedup
> (except for the cases where we had to call merge-recursive, and those
> are slow anyway).
I think it should work. Rebase is indeed my worry but it might be even
faster for most of the patches to apply the blob than computing the
diff. In my experience with the Linux kernel, full merge is rarely
needed.
--
Catalin
^ permalink raw reply
* Re: [StGit PATCH] Remove --undo flags from stg commands and docs
From: Catalin Marinas @ 2008-07-12 9:58 UTC (permalink / raw)
To: Karl Hasselström; +Cc: git
In-Reply-To: <20080708042131.GB2247@diana.vm.bytemark.co.uk>
2008/7/8 Karl Hasselström <kha@treskal.com>:
> On 2008-07-07 21:54:01 +0100, Catalin Marinas wrote:
>
>> 2008/7/4 Karl Hasselström <kha@treskal.com>:
>>
>> > In this one, I've just removed the --undo flag from sync without
>> > adding anything back. Still undetermined if that's OK.
>>
>> I think it should be ok (see the other thread). Anyway, have you
>> heard of anyone else using sync apart from me?
>
> No, I haven't.
>
> I've tried to understand what it does, and as far as I can tell it
> doesn't do quite what I want. (What I want is the ability to 3-way
> merge StGit patch stacks, so that I can modify the same patch stack in
> several places and merge back and forth. From what I recall, the sync
> command is more like a 2-way merge -- that is, it doesn't take the
> last common ancestor into account. But it's been a while since I
> studied it.)
No, the sync command is a 3-way merge between the top of a remote
patch, the top of the current patch (current HEAD actually) and the
bottom of the remote patch as ancestor. It also has a mode to
synchronise with a patch file and it applies the patch onto the bottom
of the current patch and does the merging between patch tops
afterwards.
This is not ideal as I have to deal with safe conflicts several time
(maybe adding git-rerere support would help). If you have better ideas
for keeping two stacks in sync, I'm happy to accept them (or maybe
just a different workflow).
--
Catalin
^ permalink raw reply
* Re: [PATCH] git-mailinfo: Fix getting the subject from the body
From: Junio C Hamano @ 2008-07-12 9:36 UTC (permalink / raw)
To: Lukas Sandström; +Cc: Git Mailing List, Don Zickus
In-Reply-To: <4876820D.4070806@etek.chalmers.se>
Lukas Sandström <lukass@etek.chalmers.se> writes:
> "Subject: " isn't in the static array "header", and thus
> memcmp("Subject: ", header[i], 7) will never match.
>
> Signed-off-by: Lukas Sandström <lukass@etek.chalmers.se>
> ---
>
> This has been broken since 2007-03-12, with commit
> 87ab799234639c26ea10de74782fa511cb3ca606
> so it might not be very important.
>
> builtin-mailinfo.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
> index 962aa34..2d1520f 100644
> --- a/builtin-mailinfo.c
> +++ b/builtin-mailinfo.c
> @@ -334,7 +334,7 @@ static int check_header(char *line, unsigned linesize, char **hdr_data, int over
> return 1;
> if (!memcmp("[PATCH]", line, 7) && isspace(line[7])) {
> for (i = 0; header[i]; i++) {
> - if (!memcmp("Subject: ", header[i], 9)) {
> + if (!memcmp("Subject", header[i], 7)) {
> if (! handle_header(line, hdr_data[i], 0)) {
> return 1;
> }
Actually, I do not think your patch alone makes any difference, and the
original code looks somewhat bogus. If there is no "Subject: " in the
same section of the message (either in e-mail header in which case
hdr_data == p_hdr_data[], or in the message body part in which case
hdr_data == s_hdr_data[]), hdr_data[1] will be NULL, because the only
place that allocates the storage for the data is the first loop of this
function that deals with real-RFC2822-header-looking lines.
You'd probably need something like this on top of your patch to actually
activate the code.
Another thing I noticed and found puzzling is the handling of ">From "
line that is shown in the context below. check_header() is supposed to
return true when it handled header (i.e. not part of the commit message)
and return false when line is not part of the header. As ">From " is part
of the commit log message, shouldn't it return zero?
Don, this part was what you introduced. Has this codepath ever been
exercised in the real life?
builtin-mailinfo.c | 2 ++
t/t5100-mailinfo.sh | 2 +-
t/t5100/sample.mbox | 35 +++++++++++++++++++++++++++++++++++
3 files changed, 38 insertions(+), 1 deletions(-)
diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
index 2d1520f..13f0502 100644
--- a/builtin-mailinfo.c
+++ b/builtin-mailinfo.c
@@ -332,12 +332,14 @@ static int check_header(char *line, unsigned linesize, char **hdr_data, int over
/* for inbody stuff */
if (!memcmp(">From", line, 5) && isspace(line[5]))
return 1;
if (!memcmp("[PATCH]", line, 7) && isspace(line[7])) {
for (i = 0; header[i]; i++) {
if (!memcmp("Subject", header[i], 7)) {
+ if (!hdr_data[i])
+ hdr_data[i] = xmalloc(linesize + 20);
if (! handle_header(line, hdr_data[i], 0)) {
return 1;
}
}
}
}
diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
index 2d1520f..13f0502 100644
--- a/builtin-mailinfo.c
+++ b/builtin-mailinfo.c
@@ -335,6 +335,8 @@ static int check_header(char *line, unsigned linesize, char **hdr_data, int over
if (!memcmp("[PATCH]", line, 7) && isspace(line[7])) {
for (i = 0; header[i]; i++) {
if (!memcmp("Subject", header[i], 7)) {
+ if (!hdr_data[i])
+ hdr_data[i] = xmalloc(linesize + 20);
if (! handle_header(line, hdr_data[i], 0)) {
return 1;
}
diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index 577ecc2..e9f3e72 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -11,7 +11,7 @@ test_expect_success 'split sample box' \
'git mailsplit -o. ../t5100/sample.mbox >last &&
last=`cat last` &&
echo total is $last &&
- test `cat last` = 9'
+ test `cat last` = 10'
for mail in `echo 00*`
do
diff --git a/t/t5100/sample.mbox b/t/t5100/sample.mbox
index 0476b96..aba57f9 100644
--- a/t/t5100/sample.mbox
+++ b/t/t5100/sample.mbox
@@ -430,3 +430,38 @@ index b426a14..97756ec 100644
=20
=20
2. When the environment variable 'GIT_EXTERNAL_DIFF' is set, the
+From b9704a518e21158433baa2cc2d591fea687967f6 Mon Sep 17 00:00:00 2001
+From: =?UTF-8?q?Lukas=20Sandstr=C3=B6m?= <lukass@etek.chalmers.se>
+Date: Thu, 10 Jul 2008 23:41:33 +0200
+Subject: Re: discussion that lead to this patch
+MIME-Version: 1.0
+Content-Type: text/plain; charset=UTF-8
+Content-Transfer-Encoding: 8bit
+
+[PATCH] git-mailinfo: Fix getting the subject from the body
+
+"Subject: " isn't in the static array "header", and thus
+memcmp("Subject: ", header[i], 7) will never match.
+
+Signed-off-by: Lukas Sandström <lukass@etek.chalmers.se>
+Signed-off-by: Junio C Hamano <gitster@pobox.com>
+---
+ builtin-mailinfo.c | 2 +-
+ 1 files changed, 1 insertions(+), 1 deletions(-)
+
+diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
+index 962aa34..2d1520f 100644
+--- a/builtin-mailinfo.c
++++ b/builtin-mailinfo.c
+@@ -334,7 +334,7 @@ static int check_header(char *line, unsigned linesize, char **hdr_data, int over
+ return 1;
+ if (!memcmp("[PATCH]", line, 7) && isspace(line[7])) {
+ for (i = 0; header[i]; i++) {
+- if (!memcmp("Subject: ", header[i], 9)) {
++ if (!memcmp("Subject", header[i], 7)) {
+ if (! handle_header(line, hdr_data[i], 0)) {
+ return 1;
+ }
+--
+1.5.6.2.455.g1efb2
+
^ permalink raw reply related
* Re: [PATCH] Add pretty format %aN which gives the author name, respecting .mailmap
From: Sverre Rabbelier @ 2008-07-12 8:44 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: gitster, Git Mailinglist, David Symonds
In-Reply-To: <alpine.DEB.1.00.0807120041300.8950@racer>
On Sat, Jul 12, 2008 at 1:42 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Heh. The price is that my patches are usually more buggy than other
> contributors' patches.
Pick Two: Good, Fast, and Cheap...? ;)
>> I'm not sure what to do though, if I use this new %cN GitStats will
>> only work with the latest git version... :(
>
> Yes, that is correct. But my impression was that GitStats was never meant
> as a pure add-on, but rather some integral part of Git, no? IOW at least
> contrib/ stuff.
Aye, that is true. Currently it should run with pretty much any
version, but if it does end up in git.git there is no reason not to
benefit from the most recent changes.
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: Should we discuss Windows-related changes on git@vger.kernel.org?
From: Steffen Prohaska @ 2008-07-12 8:07 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Linus Torvalds, Johannes Sixt, Junio C Hamano, msysGit,
Git Mailing List
In-Reply-To: <alpine.DEB.1.00.0807112226190.8950@racer>
On Jul 11, 2008, at 11:38 PM, Johannes Schindelin wrote:
>> Another example is the discussion about GIT_EXEC_PATH, see
>>
>> http://thread.gmane.org/gmane.comp.version-control.msysgit/2633
>
> This is a particularly good example that does not matter for Linux,
> MacOSX, Solaris or the BSDs (Git's principal platforms!) at all.
>
> And once this patch hits git@vger, it is still visible to other
> platforms.
Hmm... on Mac OS X, applications can typically be moved freely in the
filesystem. Installing such an application means no more than mounting
a disk image that contains a directory that you can drag and drop to any
location in the filesystem. You do not need administrative rights to do
this.
For command line programs, like git, moving the installation freely is
less common. Installers often place everything in /usr/local/ (they
need administrative rights to do so). But there are other examples too.
For example, I have other vims installed in addition to the system's
vim:
/Applications/vim70/Vim.app
/Applications/MacVim-snapshot-24/mvim
that I can freely move around.
The result of our discussion on GIT_EXEC_PATH might be useful to achieve
something similar for git -- although I think that it is not an
essential
feature.
Steffen
^ permalink raw reply
* Re: [GitStats] Bling bling or some statistics on the git.git repository
From: Sverre Rabbelier @ 2008-07-12 7:39 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Git Mailinglist, David Symonds
In-Reply-To: <alpine.DEB.1.00.0807120028280.8950@racer>
On Sat, Jul 12, 2008 at 1:33 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> If you were suspecting that I would write the patch once the semantics are
> finalized, you would be absolutely correct.
Awesome!
>> Mhhh, such would be beyond the scope of implementing manually indeed,
>> and should be left to the likes of a diff tool instead in order to
>> prevent reinventing the wheel :).
>
> That is why I was suggesting using the diff tool with munged input to find
> out what works best.
This makes sense, put the responsibility where it belongs, that way
someone else may use it as well.
> When that is done, I'll turn it into C.
Very much appreciated. I will start playing around with munged diffs
today and keep you posted of the result.
>> Correct, that's because that is what 'git log' tells me.
>
> I suspect that one big "git log" will not tell you enough. You probably
> need to make your tool aware (at least a little) about merges, just as you
> probably made it aware about parent/child relationships (to track the
> changes along renames)...
Atm it is not aware of parent/child relationships in the activity
analysis. (This is not the case in the 'branch contains' metric, in
which I use 'git rev-list --parents' to extract and analyse that.) I
thought of tracking renames by honoring what "git log -C -C -M" tells
me (e.g., whenever it says {foo/bar.c => bar.c} I move all the metrics
under key "foo/bar.c" to "bar.c").
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: Should we discuss Windows-related changes on git@vger.kernel.org?
From: Steffen Prohaska @ 2008-07-12 7:38 UTC (permalink / raw)
To: Johannes Schindelin, Junio C Hamano
Cc: Johannes Sixt, msysGit, Git Mailing List, Linus Torvalds
In-Reply-To: <alpine.LFD.1.10.0807111653500.2875@woody.linux-foundation.org>
On Jul 12, 2008, at 1:58 AM, Linus Torvalds wrote:
> On Sat, 12 Jul 2008, Johannes Schindelin wrote:
>>
>> But we are talking about 4msysgit.git, no?
>
> I'm not. I'm just talking about Windows-related changes in git in
> general, that are expected to be merged into regular git one way or
> another.
The discussion is not about 4msysgit, because 4msysgit will no longer
be needed after we cleaned up all differences between Junio's master
and 4msysgit's master.
I propose the following steps:
- We review the differences and either prepare patches for official
git
or revert commits in 4msysgit.
-> It will no longer matter if we build the msysgit release from
Junio's master or from 4msysgit's master.
- We change the /git submodule in msysgit to point to Junio's repo.
-> Every developer on Windows will work with the official code base.
- Windows-users can (and should) participate in the official release
cycle. They should test the next branch and the master branch after
the merge window closed. We can easily create installers for
Junio's
release candidates (I will do this).
We should provide a guideline how developers and users should send
comments and improvements related to Windows. Such a guideline could be
included in Junio's "A note from the maintainer" mail. The section on
Windows needs to be changed anyway. Currently it says:
'''
- Johannes Schindelin and Johannes Sixt for their effort to
move things forward on the Windows front. Although my
repository does not have much from the effort of MinGW team,
I expect a merge into mainline will happen so that everybody
can work from the same codebase.
'''
[ Note that the last sentence will become true if we do what I propose
above. ]
We could send more detailed guidelines to the msysgit list and/or modify
the homepage of the msysgit project. I think Junio's rule of thumb in
http://article.gmane.org/gmane.comp.version-control.git/88191
is a good starting point.
It would be good to achieve all this during the 1.6 cycle, because
MinGW is officially part of 1.6. I am however not sure if it is
practical. We have some changes in 4msysgit that might need more
discussion and we did not even start discussing the tests. So maybe
1.6.1 is more realistic.
Steffen
^ permalink raw reply
* Re: [PATCH 3/3 FIXED] help (Windows): Display HTML in default browser using Windows' shell API
From: Junio C Hamano @ 2008-07-12 7:07 UTC (permalink / raw)
To: Steffen Prohaska; +Cc: git, Johannes Schindelin, Johannes Sixt
In-Reply-To: <8D150442-8B57-4025-9110-BC4C23C2310C@zib.de>
Steffen Prohaska <prohaska@zib.de> writes:
>> Do you mean to have that printf() or is it a leftover debugging
>> statement?
>
> I mean to have it.
Ok, I was just checking. Unless other Windows users complain, will apply
as-is. As you might guess, I am completely neutral on this one.
^ permalink raw reply
* Re: [PATCH 3/3 FIXED] help (Windows): Display HTML in default browser using Windows' shell API
From: Steffen Prohaska @ 2008-07-12 6:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Johannes Schindelin, Johannes Sixt
In-Reply-To: <7vtzevhjf8.fsf@gitster.siamese.dyndns.org>
On Jul 12, 2008, at 5:26 AM, Junio C Hamano wrote:
> Steffen Prohaska <prohaska@zib.de> writes:
>
>> diff --git a/compat/mingw.c b/compat/mingw.c
>> index 3a05fe7..0ca73f7 100644
>> --- a/compat/mingw.c
>> +++ b/compat/mingw.c
>> @@ -1017,3 +1017,24 @@ sig_handler_t mingw_signal(int sig,
>> sig_handler_t handler)
>> ...
>> +void mingw_open_html(const char *unixpath)
>> +{
>> + const char *htmlpath = make_backslash_path(unixpath);
>> + printf("Launching default browser to display HTML ...\n");
>> + ShellExecute(NULL, "open", htmlpath, NULL, "\\", 0);
>> +}
>
> Do you mean to have that printf() or is it a leftover debugging
> statement?
I mean to have it. It takes some time until a fresh browser starts up
if no browser has been running before. Impatient people (like me) could
start believing that nothing would happen. But this certainly depends
on your machine. I run Windows inside a virtual machine on a Laptop,
which is probably rather slow compared to a desktop machine running
Windows natively.
Steffen
^ permalink raw reply
* Re:! [PATCH/RFC] git-mailinfo: use strbuf's instead of fixed buffers
From: Junio C Hamano @ 2008-07-12 6:10 UTC (permalink / raw)
To: Lukas Sandström; +Cc: Git Mailing List
In-Reply-To: <48769E91.60205@etek.chalmers.se>
Lukas Sandström <lukass@etek.chalmers.se> writes:
> -static char *sanity_check(char *name, char *email)
> +static void sanity_check(struct strbuf *out, struct strbuf *name, struct strbuf *email)
> {
> - int len = strlen(name);
> - if (len < 3 || len > 60)
> - return email;
> - if (strchr(name, '@') || strchr(name, '<') || strchr(name, '>'))
> - return email;
> - return name;
> + struct strbuf o = STRBUF_INIT;
> + if (name->len < 3 || name->len > 60)
> + strbuf_addbuf(&o, email);
> + if (strchr(name->buf, '@') || strchr(name->buf, '<') ||
> + strchr(name->buf, '>'))
> + strbuf_addbuf(&o, email);
> + strbuf_addbuf(&o, name);
> + strbuf_reset(out);
> + strbuf_addbuf(out, &o);
> + strbuf_release(&o);
This does not look like a correct conversion. When name is too short or
too long, we do not even look at name and return email straight. Perhaps
this would be more faithful conversion:
struct strbuf *src = name;
if (name->len < 3 ||
60 < name->len ||
strchr(name->buf, '@') ||
strchr(name->buf, '<') ||
strchr(name->buf, '>'))
src = email;
else if (name == out)
return;
strbuf_reset(out);
strbuf_addbuf(out, src);
It is not your fault, but sanity_check() is a grave misnomer for this
function. This does "get_sane_name" (i.e. we have name and email but if
name does not look right, use email instead).
> -static int bogus_from(char *line)
> +static int bogus_from(const struct strbuf *line)
> {
> /* John Doe <johndoe> */
> - char *bra, *ket, *dst, *cp;
>
> + char *bra, *ket;
> /* This is fallback, so do not bother if we already have an
> * e-mail address.
> */
> - if (*email)
> + if (email.len)
> return 0;
>
> - bra = strchr(line, '<');
> + bra = strchr(line->buf, '<');
> if (!bra)
> return 0;
> ket = strchr(bra, '>');
> if (!ket)
> return 0;
>
> - for (dst = email, cp = bra+1; cp < ket; )
> - *dst++ = *cp++;
> - *dst = 0;
> - for (cp = line; isspace(*cp); cp++)
> - ;
> - for (bra--; isspace(*bra); bra--)
> - *bra = 0;
> - cp = sanity_check(cp, email);
> - strcpy(name, cp);
> + strbuf_reset(&email);
> + strbuf_add(&email, bra + 1, ket - bra - 1);
> +
> + strbuf_reset(&name);
> + strbuf_add(&name, line->buf, bra - line->buf);
> + strbuf_trim(&name);
> + sanity_check(&name, &name, &email);
> return 1;
> }
Conversion looks correct but its return value does not make much sense
(again, not your fault). bogus_from() is given a bogus looking from line
(it is not about checking if it is bogus), and returns 0 if we already
have e-mail address, if the from line does not have bra-ket for grabbing
e-mail address for, but returns 1 if we managed to get name and email
pairs. The inconsistency does not matter only because its sole caller
handle_from() returns its return value, and its caller discards it. We
may be better off declaring this function and handle_from() as void.
> -static int handle_from(char *in_line)
> +static int handle_from(struct strbuf *from)
> ...
> + el = strcspn(at, " \n\t\r\v\f>");
> + strbuf_reset(&email);
> + strbuf_add(&email, at, el);
> + strbuf_remove(from, at - from->buf, el + 1);
> /* The remainder is name. It could be "John Doe <john.doe@xz>"
> * or "john.doe@xz (John Doe)", but we have whited out the
> * email part, so trim from both ends, possibly removing
> * the () pair at the end.
> */
Now, it should read "but we have removed the email part", I think.
> + strbuf_trim(from);
> + if (*from->buf == '(')
> + strbuf_remove(&name, 0, 1);
> + if (*(from->buf + from->len - 1) == ')')
Can from be empty at this point before this check?
> + strbuf_setlen(from, from->len - 1);
> +
> + sanity_check(&name, from, &email);
> return 1;
> }
We used to copy the data from the argument (in_line) before munging it in
this function, but now we are modifying it in place (from). Does this
upset our caller, or the original code was just doing an extra unnecessary
copy?
> -static int handle_header(char *line, char *data, int ofs)
> +static void handle_header(struct strbuf **out, const struct strbuf *line)
> {
> - if (!line || !data)
> - return 1;
> -
> - strcpy(data, line+ofs);
> + if (!*out) {
> + *out = xmalloc(sizeof(struct strbuf));
> + strbuf_init(*out, line->len);
> + } else
> + strbuf_reset(*out);
>
> - return 0;
> + strbuf_addbuf(*out, (struct strbuf *)line); /* const warning */
> }
I think its second parameter can safely become "const struct strbuf *";
perhaps we should fix the definition of strbuf_addbuf() in your first
patch?
> @@ -173,180 +153,176 @@ static int slurp_attr(const char *line, const char *name, char *attr)
> else
> ends = "; \t";
> sz = strcspn(ap, ends);
> - memcpy(attr, ap, sz);
> - attr[sz] = 0;
> + strbuf_add(attr, ap, sz);
> return 1;
> }
>
> struct content_type {
> - char *boundary;
> - int boundary_len;
> + struct strbuf *boundary;
> };
>
> static struct content_type content[MAX_BOUNDARIES];
Wouldn't it make more sense to get rid of "struct content_type" altogether
and use "struct strbuf *content[MAX_BOUNDARIES]" directly?
I'll review from handle_content_type() til the rest of the file
separately, as my concentration is wearing out..
^ permalink raw reply
* Re: [PATCH] setup.py: fix error message when running with python-2.3
From: Karl Hasselström @ 2008-07-12 5:19 UTC (permalink / raw)
To: Miklos Vajna; +Cc: Catalin Marinas, Thomas Rast, Petr Baudis, Git Mailing List
In-Reply-To: <1215806972-18713-1-git-send-email-vmiklos@frugalware.org>
On 2008-07-11 22:09:31 +0200, Miklos Vajna wrote:
> When setup.py tries to check the python version, the check actually
> won't give a usable error message but it'll raise a SyntaxError. Fix
> this by not using generator expressions.
> - pyver = '.'.join(str(n) for n in sys.version_info)
> + pyver = '.'.join(map(lambda x: str(x), sys.version_info))
Thanks. And by not using a list comprehension, you stay compatible
with pre-2.0 versions as well (though I guess we'd need to test that,
since there may be other 2.0 things we rely on in this code path).
--
Karl Hasselström, kha@treskal.com
www.treskal.com/kalle
^ 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