* Re: [PATCH] Fix git_setup_directory_gently when GIT_DIR is set
From: Junio C Hamano @ 2006-06-06 1:06 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <7vfyij2mo8.fsf@assigned-by-dhcp.cox.net>
Junio C Hamano <junkio@cox.net> writes:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>>> Hmph. Would it be a bug in clone that does not create GIT_DIR
>>> then?
>>
>> I don't think so. The whole point in calling git-init-db is to create
>> that. GIT_DIR is set so that the otherwise nice work-in-a-subdirectory
>> does not kick in. Imagine for example:
>>
>> git-clone ./. victim
>>
>> (taken straight out of t5400). If GIT_DIR was not set, git-init-db (which
>> reads repositoryformat from the config if that exists, right?) would find
>> .git/ in git/t/trash, and _not_ create git/t/trash/victim/.git/.
>
> I know clone currently relies on init-db to create the directory if
> it does not exist (I wrote the code after all).
Ah, I think I see the real problem is. Alias handling is done
too early, and for commands like init-db that does _not_ even
want to look at an existing repository it tries to use GIT_DIR.
So how about this patch instead on top of yours?
-- >8 --
git alias: try alias last.
This disables alias "foo" from being used for git-foo, and when
we do use alias we check the built-in and then existing command
names first and then alias as the fallback. This avoids the
problem of common commands used in scripts getting clobbered by
user specific aliases.
---
diff --git a/git.c b/git.c
index 8854472..6db8f2b 100644
--- a/git.c
+++ b/git.c
@@ -202,6 +202,7 @@ int main(int argc, const char **argv, ch
char *slash = strrchr(cmd, '/');
char git_command[PATH_MAX + 1];
const char *exec_path = NULL;
+ int done_alias = 0;
/*
* Take the basename of argv[0] as the command
@@ -229,7 +230,6 @@ int main(int argc, const char **argv, ch
if (!strncmp(cmd, "git-", 4)) {
cmd += 4;
argv[0] = cmd;
- handle_alias(&argc, &argv);
handle_internal_command(argc, argv, envp);
die("cannot handle %s internally", cmd);
}
@@ -287,13 +287,21 @@ int main(int argc, const char **argv, ch
exec_path = git_exec_path();
prepend_to_path(exec_path, strlen(exec_path));
- handle_alias(&argc, &argv);
+ while (1) {
+ /* See if it's an internal command */
+ handle_internal_command(argc, argv, envp);
- /* See if it's an internal command */
- handle_internal_command(argc, argv, envp);
+ /* .. then try the external ones */
+ execv_git_cmd(argv);
- /* .. then try the external ones */
- execv_git_cmd(argv);
+ /* It could be an alias -- this works around the insanity
+ * of overriding "git log" with "git show" by having
+ * alias.log = show
+ */
+ if (done_alias || !handle_alias(&argc, &argv))
+ break;
+ done_alias = 1;
+ }
if (errno == ENOENT)
cmd_usage(0, exec_path, "'%s' is not a git-command", cmd);
^ permalink raw reply related
* Re: [PATCH] Fix git_setup_directory_gently when GIT_DIR is set
From: Johannes Schindelin @ 2006-06-06 5:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vmzcr14ao.fsf@assigned-by-dhcp.cox.net>
Hi,
On Mon, 5 Jun 2006, Junio C Hamano wrote:
> Junio C Hamano <junkio@cox.net> writes:
>
> > I know clone currently relies on init-db to create the directory if
> > it does not exist (I wrote the code after all).
>
> Ah, I think I see the real problem is. Alias handling is done
> too early, and for commands like init-db that does _not_ even
> want to look at an existing repository it tries to use GIT_DIR.
>
> So how about this patch instead on top of yours?
Yes, I like it. I was so focused on reading the config early to be done
with it, and avoiding reading the config twice (which it can do now, with
your patch, but only if one is so insane to use nested aliases).
Your method of avoiding shadowing proper git commands is elegant, if maybe
incomplete: you have no way of warning the user that the alias is not
used. But then, I do not think that matters.
Ciao,
Dscho
^ permalink raw reply
* Re: What's in git.git (part #2)
From: Shawn Pearce @ 2006-06-06 5:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v3beodpqs.fsf@assigned-by-dhcp.cox.net>
Junio C Hamano <junkio@cox.net> wrote:
> Shawn Pearce <spearce@spearce.org> writes:
>
> > I find it useful to track what I've sent to you just in case I
> > screw up some ref somewhere. I like knowing that if I perform a
> > bad update-ref call (which I'm prone to do sometimes) that I can
> > recover quickly as the log exists.
>
> I find it interesting to be able to say:
>
> $ git log next@{yesterday}..next
>
> I often find myself getting curious to see:
>
> $ git reflog next
> Wed May 31 14:23:58 2006 -0700
> 62b693a... Merge branch 'master' into next
> ...
Hmm, looks like nobody has actually implemented that - at least not
in 'next'. :-)
Is that a serious feature request? If so I'll do it. BTW: we're
also still lacking reflog during receive-pack. Much of the update()
function in receive-pack.c can be replaced with the new locking
functions, except that if reflog is enabled on the target ref then
GIT_COMMITTER_NAME and GIT_COMMITTER_EMAIL need to be set for the
update to succeed.
I've been busy at work but I really have been wanting to put
some time into this GIT Eclipse plugin that I've been neglecting.
Some folks at work are starting to become more interested in it.
I have gotten the really core functionality done; all that is
left is the hard stuff (directory synchronization, push, fetch,
non-delta pack generation for push[1], tree diff).
*1* Generating a simple pack with only deflate compression on the
objects should be trivial. Since this is 100% pure Java code nobody
should be expecting great performance out of it from day 1 anyway.
Sure its not an optimal transport but if you were that worried about
the transfer byte costs to push then you probably would just prefer
to use the native tools code tools anyway.
--
Shawn.
^ permalink raw reply
* Re: Importing Mozilla CVS into git
From: Martin Langhoff @ 2006-06-06 5:55 UTC (permalink / raw)
To: Jon Smirl; +Cc: git
In-Reply-To: <9e4733910606022128h23ff94fbg3fcb4fa191254b5a@mail.gmail.com>
On 6/3/06, Jon Smirl <jonsmirl@gmail.com> wrote:
> On 6/1/06, Jon Smirl <jonsmirl@gmail.com> wrote:
> > With the attached patch you can parse the entire Mozilla tree. The
> > tree has over 100,000 files in it and about 300 branches.
>
> I was a little low with these counts, more like 110,000 files and some
> parts of the tree have 1,000 branches. Total tree size is 3GB.
I don't think it really has that many branches. If I am to believe
cvsps (which took 3GB to walk the history), it has some branches with
recursive loops in their ancestry (MANG_MATH_BRANCH and
SpiderMonkey140_BRANCH have eachother as ancestors!?), 197969 commits
and 796 branches.
This repository has been mangled quite badly. Don't know what you guys
did with it, but it sure isn't pretty. I'm working on getting
git-cvsimport to get through a complete import.
cheers,
martin
^ permalink raw reply
* New release?
From: Junio C Hamano @ 2006-06-06 6:02 UTC (permalink / raw)
To: git; +Cc: Linus Torvalds
In-Reply-To: <Pine.LNX.4.64.0606052002530.5498@g5.osdl.org>
The "next" queue has been shrinking, and nothing is going to
"maint" branch, which can mean only one thing ;-).
The last issue of "What's in" talked about 1.4.0-rc1 but somehow
vger threw it on the bitbucket. The message started like this:
Subject: What's in git.git
To: git@vger.kernel.org
Date: Wed, 31 May 2006 23:40:26 -0700
It's been a while since the last feature release, and I
think with the recent "many things built-in" (including the
busybox style integration) we are nearing a good time to do the
next feature release 1.4.0.
Before doing a 1.4.0-rc1, I would like to see the following
topics in the "next" branch graduate to "master":
- re-add missing flags to format-patch. I have resurrected
"--signoff"; if people care about something else we dropped
when we went built-in, please raise hand and submit patches.
- tree-parser updates from Linus seems to be fine in the sense
I haven't seen breakage from it; I'll push it out to "master"
before the end of the week. I'd like to do another round of
update to introduce a unified tree/index/directory walker, so
settling this down is sort of urgent.
- http-fetch fixes from Nick, which looked obviously correct.
I would appreciate test reports from people who saw breakages
on this one.
- reflog from Shawn. Do people find this useful? I've enabled
reflog on "next" branch in my development repository to see
how useful it would be for myself a few days ago, and also in
a linux-2.6 repository I use for testing (I do not hack on
kernel myself).
All of the above have been cleared with the recent fixes to
miscellaneous minor breakages around the tree-parser updates, so
I think what we have on the "master" branch tonight is worthy of
1.4.0-rc1 status.
Other topics in "next" includes:
- read/write-tree --prefix. This is remnant of now-vetoed
subproject support using "bind commit". I kept both of them
because they could be useful independent of "bind commit",
but I do not know how much. I think read-tree --prefix might
probably be more useful than write-tree --prefix, since the
latter can be writing out the whole tree and run rev-parse
$tree:/path/name to extract that part, but the former does
not have an easy equivalent (you could pipe ls-tree output to
sed and pipe that to update-cache --index-info, but that is
crumsy).
I'd like to do "gitlink" based subproject support but most
likely that needs to come after tree/index/directory walker.
I am inclined to drop "read/write-tree --prefix" even from
"next"; we can resurrect it later if it proves to be useful in
order to do something else, such as "gitlink".
- fetch-pack client-side hack. When your repository has more
roots than the repository you are fetching from, the common
commit discovery exchange between fetch-pack and upload-pack
ends up traversing down the ancestry chain of the history the
other end do not have. The hack in the "next" branch is to
give up the common commit discovery early on the client side,
which Ralf Baechle who originally reported the problem says
to fix the problem (<20060526154239.GA20839@linux-mips.org>);
but the proper fix involves a bit smarter upload-pack.
I've posted a hacky upload-pack patch:
<7vfyiwi4xl.fsf@assigned-by-dhcp.cox.net>
but I think it should really needs to be cleaned up properly.
I think this can happen after 1.4.0.
Things that we might want to have in 1.4.0 but not even in "next"
yet include:
- p4 importer (Sean Estabrooks) -- are people interested?
I am in favour of doing this before 1.4.0 (not in contrib/ but
as an importer next to other git-*import).
- letting fetch-pack ask for an arbitrary commit object the
user obtained out of band (Eric W Biederman) -- waiting for
updated patch. We would need a corresponding one-liner patch
to upload-pack when we do this.
This can wait.
- using ~/.gitrc to give a fall-back default when
$GIT_DIR/config does not have values.
I suspect this would be more involved than Pasky's initial
patch; but it can wait.
- command aliases and possibly default arguments via the
configuration file.
This we have in "next" tonight and we should be able to have it
in "master" by the end of the week. I took what Johannes did,
but I think that is pretty much the same as what Pasky did
initially, but forgot to mention that in the commit log.
I am expected to be mostly offline next week (among other
things, http://osdl.jp/seminar0613/ if you can read Japanese),
so I am planning to do 1.4.0-rc1 by my next git day (Wednesday
7th), and perhaps the real 1.4.0 on Saturday 10th if things go
smoothly.
Now v1.4.0-rc1 seems to have mirrored out. Please have fun.
^ permalink raw reply
* Re: What's in git.git (part #2)
From: Junio C Hamano @ 2006-06-06 6:16 UTC (permalink / raw)
To: Shawn Pearce; +Cc: git
In-Reply-To: <20060606053905.GA9797@spearce.org>
Shawn Pearce <spearce@spearce.org> writes:
>> I find it interesting to be able to say:
>>
>> $ git log next@{yesterday}..next
>>
>> I often find myself getting curious to see:
>>
>> $ git reflog next
>> Wed May 31 14:23:58 2006 -0700
>> 62b693a... Merge branch 'master' into next
>> ...
>
> Hmm, looks like nobody has actually implemented that - at least not
> in 'next'. :-)
>
> Is that a serious feature request?
I've written it but it was so trivial I threw it away after
writing the e-mail you are responding to with it.
As I said, I _think_ I was interested in seeing it primarily
because reflog was a new curiosity to me. It is more like
wanting to know how the new tool works more than using the new
tool effectively to improve my productivity. In a "serious"
environment, a tool is just something you would use to get the
real job done, not to toy around to see how _it_ works, so I
suspect the above would not be so useful in practice, as I wrote
in the message.
^ permalink raw reply
* Re: [PATCH] Cleanup git-send-email.perl:extract_valid_email
From: Junio C Hamano @ 2006-06-06 6:59 UTC (permalink / raw)
To: Horst von Brand, Ryan Anderson, Eric Wong; +Cc: git
In-Reply-To: <200606040010.k540ANa4015204@laptop11.inf.utfsm.cl>
Horst von Brand <vonbrand@inf.utfsm.cl> writes:
>> > # check for a local address:
>> > - return $address if ($address =~ /^([\w\-]+)$/);
>> > + return $address if ($address =~ /^([\w\-.]+)$/);
>>
>> I keep forgetting this, '+' is a valid (and useful) setup, too.
>
> Oops...
>>
>> Actually, I'm retracting my earlier ack on this. This is way too
>> restrictive. I'd rather allow an occasional invalid email address than
>> to reject valid ones. I generally trust git users to know what they're
>> doing when entering email addresses[1].
>>
>> *, $, ^, +, = are all valid characters in the username portion (not sure
>> about local accounts, though), and I'm sure there are more that I don't
>> know about.
>
> As a general principle, I prefer to check what is legal instead of trying
> to filter out what isn't.
If we start doing addr-spec in RFC2822 (page 17) ourselves, we
should rather be using Email::Valid. A permissive sanity check
to catch obvious mistakes would be more appropriate here than
being RFC police.
I think something like the attached, on top of your patch, would
be appropriate for upcoming 1.4.0.
-- >8 --
send-email: be more lenient and just catch obvious mistakes.
This cleans up the pattern matching subroutine by introducing
two variables to hold regexp to approximately match local-part
and domain in the e-mail address. It is meant to catch obvious
mistakes with a cheap check.
The patch also moves "scalar" to force Email::Valid->address()
to work in !wantarray environment to extract_valid_address;
earlier it was in the caller of the subroutine, which was way
too error prone.
---
diff --git a/git-send-email.perl b/git-send-email.perl
index a7a7797..700d0c3 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -312,16 +312,18 @@ our ($message_id, $cc, %mail, $subject,
sub extract_valid_address {
my $address = shift;
+ my $local_part_regexp = '[^<>"\s@]+';
+ my $domain_regexp = '[^.<>"\s@]+\.[^<>"\s@]+';
# check for a local address:
- return $address if ($address =~ /^([\w\-.]+)$/);
+ return $address if ($address =~ /^($local_part_regexp)$/);
if ($have_email_valid) {
- return Email::Valid->address($address);
+ return scalar Email::Valid->address($address);
} else {
# less robust/correct than the monster regexp in Email::Valid,
# but still does a 99% job, and one less dependency
- $address =~ /([\w\-.]+@[\w\-.]+)/;
+ $address =~ /($local_part_regexp\@$domain_regexp)/;
return $1;
}
}
@@ -384,7 +386,7 @@ X-Mailer: git-send-email $gitversion
defined $pid or die $!;
if (!$pid) {
exec($smtp_server,'-i',
- map { scalar extract_valid_address($_) }
+ map { extract_valid_address($_) }
@recipients) or die $!;
}
print $sm "$header\n$message";
^ permalink raw reply related
* peek_remote return forever zero
From: Paolo Teti @ 2006-06-06 8:18 UTC (permalink / raw)
To: git
A question..
Why in peek-remote.c the peek_remote func.
return an int (and forever zero) ?
peek_remote return an int for future use
(and just for now zero) or we can replace it with a void?
^ permalink raw reply
* Re: What's in git.git (part #2)
From: Johannes Schindelin @ 2006-06-06 8:19 UTC (permalink / raw)
To: Shawn Pearce; +Cc: Junio C Hamano, git
In-Reply-To: <20060606053905.GA9797@spearce.org>
Hi,
On Tue, 6 Jun 2006, Shawn Pearce wrote:
> *1* Generating a simple pack with only deflate compression on the
> objects should be trivial. Since this is 100% pure Java code nobody
> should be expecting great performance out of it from day 1 anyway.
If you use jzlib (http://www.jcraft.com/jzlib/) it should not be slow.
IMHO the I/O will be the bottleneck, but I would be happy to be
contradicted by the facts.
Ciao,
Dscho
^ permalink raw reply
* Re: New release?
From: Johannes Schindelin @ 2006-06-06 8:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Linus Torvalds
In-Reply-To: <7vodx6zus2.fsf@assigned-by-dhcp.cox.net>
Hi,
On Mon, 5 Jun 2006, Junio C Hamano wrote:
> - letting fetch-pack ask for an arbitrary commit object the
> user obtained out of band (Eric W Biederman) -- waiting for
> updated patch. We would need a corresponding one-liner patch
> to upload-pack when we do this.
>
> This can wait.
I think that this could be an important step towards a sensible "shallow
clone": we could introduce "remote alternates", where sha1_file
transparently loads (and stores) single commit objects and tree objects
together with their subtrees and blobs from a remote, if they are not
present in the shallow repo yet.
This will be blazingly _slow_, I guess, if one does any such stupid thing
as git-fsck-objects... However, it should be fixable (expensive commands,
i.e. commands traversing extensive history/object lists, could just barf
if the repo is shallow).
> - using ~/.gitrc to give a fall-back default when
> $GIT_DIR/config does not have values.
>
> I suspect this would be more involved than Pasky's initial
> patch; but it can wait.
I think that this is quite important for the aliases to be useful.
However, it does not blend well with the mmap()ifying of the config
reading.
Also, there is the problem of unique keys. You want unique keys to be only
unique _per config file_, i.e. the local config can override the user's
config. This is probably only relevant for git-repo-config, though.
Ciao,
Dscho
^ permalink raw reply
* [RFC] revision limiter in git-rev-list
From: Marco Costalba @ 2006-06-06 8:36 UTC (permalink / raw)
To: git; +Cc: junkio
We currently can run something like
git-rev-list --topo-order --parents HEAD -- foo1.c foo2.c foo3.c
And have in output a list of revisions that modify paths foo1.c,
foo2.c and foo.3
For each revision we have also the corresponding pseudo-parents,
i.e. the proper parents chosen among the revisions in output list.
The idea is to extend this behaviour to also commit objects sha.
As example, given the following revisions history:
a
b-
| c
| d
e
f
We could add a new option --filtered so that
git-rev-list --topo-order --filtered HEAD -- a d e
Gives the following
a
b-
| d
e
Note that the merge point b has been added implicitly as in path limiter case.
This is a powerful and quite general option that can be use for a large
and interesting number of cases.
1) Get the branch a given <sha> belongs
git-rev-list --topo-order --parents --filtered
HEAD -- <sha> branchList[0] branchList[1]... branchList[k-1]
where branchList[] is the vector of branches of lenght k
Searched branch is the one where sha appears as one of his parents.
2) Get nearest previous tag of a given <sha>
git-rev-list --topo-order --parents -n1 --filtered
<sha> -- <sha> tagList[0] tagList[1]... tagList[n-1]
where tagList[] is the vector of tags of lenght n
In output we have only one revision (see option -n1) that is <sha>,
his parent(s) are the nearest tag(s).
3) Get tag/branch history with a sensible graph (using a GUI frontend)
git-rev-list --topo-order --parents --filtered HEAD -- tagList[0] ...
branchList[n-1]
git-rev-list --topo-order --parents --filtered HEAD -- branchList[0]
... branchList[n-1]
4) Filter/find revisions according to a given search criteria (using a
GUI frontend)
git-rev-list --topo-order --parents --filtered HEAD -- matchList[0]
... matchList[n-1]
where matchList[] is the vector of sha's matching a filter criteria and could
be provided by a GUI frontend that already normally have filter capabilities.
The plus here is to let frontend to draw a sensible graph of the
resulting revisions.
Probably there are other uses of this option that is very powerful
because essentially
it adds topological information to a given set of revisions.
Of course I really ignore any implementation difficult/feasibility issues ;-)
Marco
^ permalink raw reply
* Re: New release?
From: Junio C Hamano @ 2006-06-06 10:27 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Linus Torvalds
In-Reply-To: <Pine.LNX.4.63.0606061019440.11478@wbgn013.biozentrum.uni-wuerzburg.de>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> On Mon, 5 Jun 2006, Junio C Hamano wrote:
>
>> - letting fetch-pack ask for an arbitrary commit object the
>> user obtained out of band (Eric W Biederman) -- waiting for
>> updated patch. We would need a corresponding one-liner patch
>> to upload-pack when we do this.
>>
>> This can wait.
>
> I think that this could be an important step towards a sensible "shallow
> clone"...
I did not say we are not interested in doing this ever. The
"arbitrary commit" thing is easy but I do not think it is all
that important to hold all the good stuff back that happened
since 1.3.0 and delay 1.4.0.
Also, what you talk about the "lazy clone" is a lot more
involved than what Eric wanted to have. It is _never_ shallow
clones that normal users would want -- making it shallow to
explicitly say certain operations break is a cop-out for
implementors. What the users really want is to be in total
control -- ranging from completely on-demand ala CVS and SVN to
"down to this commit in the history I would want to be cached on
the local machine so that I can go offline and still do useful
things with the history", with new failure modes for history
traversing commands to exit gracefully when offline. That _is_
the ideal but I know it won't be within reach anytime soon.
>> - using ~/.gitrc to give a fall-back default when
>> $GIT_DIR/config does not have values.
>>
>> I suspect this would be more involved than Pasky's initial
>> patch; but it can wait.
>
> I think that this is quite important for the aliases to be useful.
I am not saying this is unimportant. Again, however, it is not
that important to hold other good stuff.
^ permalink raw reply
* Re: [RFC] revision limiter in git-rev-list
From: Junio C Hamano @ 2006-06-06 10:29 UTC (permalink / raw)
To: Marco Costalba; +Cc: git, junkio
In-Reply-To: <e5bfff550606060136l59143ef2mdb9dc68ab78e9ff1@mail.gmail.com>
"Marco Costalba" <mcostalba@gmail.com> writes:
> Of course I really ignore any implementation difficult/feasibility issues ;-)
I do not think your problem is ignoring difficulty/feasibility.
The bigger problem is ignoring to describe the semantics of what
you are proposing.
For example:
> As example, given the following revisions history:
>
> a
> b-
> | c
> | d
> e
> f
>
> We could add a new option --filtered so that
>
> git-rev-list --topo-order --filtered HEAD -- a d e
>
> Gives the following
>
> a
> b-
> | d
> e
Why does it give that? Where is the HEAD in the example, and
how does it affect the result? I presume when you wrote the
unfiltered graph, you meant time flows from top to bottom. In
other words, I would have written the original graph like this,
time flowing from left to right:
c---d
/
a---b---e---f
Does your rev-list given above give different results depending
on HEAD being d or f? If so how, and for which case is the
example output produced? If not, what significance does the
HEAD argument have in the algorithm that generates the result
you are proposing (maybe your example had time flowing from
bottom to top, then the HEAD might be at a, and you did not have
to think about this question when you wrote the message)? These
are rhetorical questions, so do not waste time thinking about
answers to them before reading to the end of this message.
> Note that the merge point b has been added implicitly as in
> path limiter case.
I do not think path limiter case adds anything. A merge is
shown if it touches the path in an nontrivial way, but otherwise
it isn't. Also b is not a merge unless time is flowing from
bottom to top in your picture -- it is a fork point.
While I really do not think this belongs to rev-list, I suspect
what you want is a command that takes a set of commits you are
interested in and gives you an abbreviated topology across them.
I think it might be a good thing to have in our toolset (didn't
I say that already?).
So your example would become:
Given this graph (and there may be other nodes before a or after
d or f):
c---d
/
a---b---e---f
the user is interested in A, D, and E. Show an
abbreviated topology containing them.
which would give you
D
/
A---B---E
BTW, interestingly enough if the time flowed from right to left,
the answer is the same here.
The command line would not involve HEAD unless it is also one of
the commits you are interested in. So it would be something
like:
git graph-reduce A D E
which would give you:
E B
D B
B A
or even (since you are interested in reachability):
E B A
D B A
B A
Unfortunately, your description is a bit too fuzzy to me, so I
am making guesses as to what you really want. For example,
although you said "b is included because it is a merge", I
strongly suspect you have cases where you would want to and not
want to include a fork point or a merge point in the result,
depending on the commits you are interested in. If you are
reducing this graph for A, H, and J:
f---g
/ \
c---d---e---h
/
a---b---i---j
I think you would want to see this as the result of graph
reduction:
H
/
A---B---J
instead of:
C---E---H
/
A---B---J
That is, although e is a merge and c is a fork point, they do
not bring anything interesting in the bird's eye view picture,
and are filtered out. However, b is a point where lines of
development leading to H and J (two of the commits the user is
interested in) forks, and it is interesting.
Is this kind of graph reduction what you are after?
^ permalink raw reply
* Re: [RFC] revision limiter in git-rev-list
From: Marco Costalba @ 2006-06-06 11:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v7j3uvapa.fsf@assigned-by-dhcp.cox.net>
On 6/6/06, Junio C Hamano <junkio@cox.net> wrote:
> "Marco Costalba" <mcostalba@gmail.com> writes:
>
> > Of course I really ignore any implementation difficult/feasibility issues ;-)
>
> I do not think your problem is ignoring difficulty/feasibility.
> The bigger problem is ignoring to describe the semantics of what
> you are proposing.
>
Sadly I have to agree, sorry.
> For example:
>
> > As example, given the following revisions history:
> >
> > a
> > b-
> > | c
> > | d
> > e
> > f
> >
> > We could add a new option --filtered so that
> >
> > git-rev-list --topo-order --filtered HEAD -- a d e
> >
> > Gives the following
> >
> > a
> > b-
> > | d
> > e
>
> Why does it give that? Where is the HEAD in the example,
HEAD is "a", just to try to be more clear that's the graph you would see running
gitk HEAD: HEAD is at top ("a") initial revision is at bottom ("e").
>
> > Note that the merge point b has been added implicitly as in
> > path limiter case.
>
> I do not think path limiter case adds anything. A merge is
> shown if it touches the path in an nontrivial way, but otherwise
> it isn't.
Yes.
>Also b is not a merge unless time is flowing from
> bottom to top in your picture -- it is a fork point.
>
I meant a graph as shown by gitk HEAD, so I really meant
"b" is a merge.
> While I really do not think this belongs to rev-list, I suspect
> what you want is a command that takes a set of commits you are
> interested in and gives you an abbreviated topology across them.
> I think it might be a good thing to have in our toolset (didn't
> I say that already?).
>
> So your example would become:
>
> Given this graph (and there may be other nodes before a or after
> d or f):
>
> c---d
> /
> a---b---e---f
>
> the user is interested in A, D, and E. Show an
> abbreviated topology containing them.
>
> which would give you
>
> D
> /
> A---B---E
>
Yes.
>
> Unfortunately, your description is a bit too fuzzy to me, so I
> am making guesses as to what you really want. For example,
> although you said "b is included because it is a merge", I
> strongly suspect you have cases where you would want to and not
> want to include a fork point or a merge point in the result,
> depending on the commits you are interested in. If you are
> reducing this graph for A, H, and J:
>
> f---g
> / \
> c---d---e---h
> /
> a---b---i---j
>
> I think you would want to see this as the result of graph
> reduction:
>
> H
> /
> A---B---J
>
> instead of:
>
> C---E---H
> /
> A---B---J
>
> That is, although e is a merge and c is a fork point, they do
> not bring anything interesting in the bird's eye view picture,
> and are filtered out. However, b is a point where lines of
> development leading to H and J (two of the commits the user is
> interested in) forks, and it is interesting.
>
> Is this kind of graph reduction what you are after?
>
>
Practically speaking it's the kind of reduction for whom examples I
gave _do_ work.
Marco
^ permalink raw reply
* Re: [RFC] revision limiter in git-rev-list
From: Marco Costalba @ 2006-06-06 11:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v7j3uvapa.fsf@assigned-by-dhcp.cox.net>
>
> While I really do not think this belongs to rev-list, I suspect
> what you want is a command that takes a set of commits you are
> interested in and gives you an abbreviated topology across them.
I was thinking at an extension of git-rev-list because
1) Current git-rev-list options are quite orthogonal with rev limiter.
As example in previous given examples -n and --parents options are
used and I think more could be used with
interesting results.
Current git-rev-list options are a lot and are very powerful, it's a
pity if this new feature do not inherit them.
2) This feature could be seen as a generalization of path limiting.
>From today:
git-rev-list HEAD -- <path 1> <path 2> ... <path n>
To possible:
git-rev-list HEAD -- <obj 1> <obj 2> ... <obj n>
Where obj == <path> || obj == <commit sha> || obj == <something else
I didn't think of>
Of course we need a (syntactic) way to disambiguate the arguments
after '--' but the results are very powerful and general, as example
we could mix commit objects _and_ paths in git-rev-list command line
(git-rev-list HEAD -- foo.c tag1) and also be able to use the full
set of git-rev-list options before the '--'
Marco
^ permalink raw reply
* [ANNOUNCE] tig 0.4
From: Jonas Fonseca @ 2006-06-06 12:18 UTC (permalink / raw)
To: git
Hello,
Too many improvements to keep me longer from releasing version 0.4 of
tig, the text-mode interface for git. First off, a big thanks to Aneesh
Kumar for helping with improving the basic user interface. Among the
highlights, colors are now configurable in ~/.tigrc and the diff and
main views are better integrated. For more info, changes I feel are
most important can be found below.
I hope to focus more on fixing some of the accumulated bugs for tig-0.5.
A particular embarrassing one, is the problem with text wrapping for
long lines which can end up corrupting the screen. But there should also
be room for a first shot at displaying revision graph and maybe
configurable keybindings.
A list of tig resources:
- Homepage: http://jonas.nitro.dk/tig/
- Tarballs: http://jonas.nitro.dk/tig/releases/
- Git URL: http://jonas.nitro.dk/tig/tig.git
- Gitweb: http://pasky.or.cz/gitweb.cgi?p=tig.git;a=summary
Tarballs contain all the generated documentation, as do the #release
branch of the git repository. Thanks to pasky for the gitweb hosting.
---
Jonas Fonseca:
Redraw the whole display after toggling line number
Bind quit to 'Q'
Add close view request; bound to 'q' by default
Add preliminary support for UTF-8 handling in the main view
Make Enter in the main view switch to the split diff view
Pressing Enter in the diff view will now scroll it one line down
Bind '-' to PageUp; raises Mutt compatibility
Bind 'j'/'k' to moving up/down; add next/previous requests bound to Down/Up
Make -h and --help options ouput a help message
Record builds with dirty working tree by appending -dirty to the version
Make Enter in the pager view always scroll
Add simple window stack, tracking view relation ship via parent member
Move git directory assertion to main; don't require .git repo in pager mode
Add -O2 to CFLAGS to get more warnings
Make update reporting less verbose
Improve title updating and remove flickering
Introduce struct line and use it for view->line
Add support for loading repo config
Make UTF-8 handling optional but still default
Never close backgrounded loads; only clear window when starting to update
Make window switching smother; fix blurring of previous view when switching
When updating the title window, move the cursor to the end of line
Tab size short option changes from -t to -b
Notify that the prompt is unusable while loading
Make the stop all loading request stop all loading
Document the loading time displayed in the title window after 3 seconds
Various usability improvements
Add support for setting color options in ~/.tigrc
Fix segfault where current_view would become >= sizeof(display)
Rename documentation build rules using s/docs/doc/; more like git
Make 'h' and '?' show built-in key binding quick reference
Add support for showing tags and other repo refs in the diff and log view
Move documentation out of tig.c to tig.1.txt, tigrc.5.txt, and manual.txt
Add COPYING, SITES, TODO, BUGS, and INSTALL files
Make the view title show percentage shown like less
Junio C Hamano:
Makefile: make customization of installation locations easier
Kristian Høgsberg:
Bind 'b' to Page Up, and Space to Page Down
Sir Raorn:
Fix linking with --as-needed ld(1) option.
Fix warnings
Timo Hirvonen:
Make some strings "const"
Mark quit() and die() __noreturn
--
Jonas Fonseca
^ permalink raw reply
* Re: New release?
From: Johannes Schindelin @ 2006-06-06 12:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7virnevath.fsf@assigned-by-dhcp.cox.net>
Hi,
On Tue, 6 Jun 2006, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > On Mon, 5 Jun 2006, Junio C Hamano wrote:
> >
> >> - letting fetch-pack ask for an arbitrary commit object the
> >> user obtained out of band (Eric W Biederman) -- waiting for
> >> updated patch. We would need a corresponding one-liner patch
> >> to upload-pack when we do this.
> >>
> >> This can wait.
> >
> > I think that this could be an important step towards a sensible "shallow
> > clone"...
>
> I did not say we are not interested in doing this ever.
I was not trying to make a case to wait with 1.4.0...
> Also, what you talk about the "lazy clone" is a lot more involved than
> what Eric wanted to have.
A little more involved. And I wanted to know what people think about this
way to tackle shallow clones.
Ciao,
Dscho
^ permalink raw reply
* [PATCH] git-format-patch: add --output-directory long option again
From: Dennis Stosberg @ 2006-06-06 14:19 UTC (permalink / raw)
To: git
Additionally this fixes two minor issues:
(1) git-format-patch left an empty directory behind if an invalid
option was given on the command line.
(2) mkdir() was called with a NULL argument (argv[argc]), if -o was
the last option on the command line.
Signed-off-by: Dennis Stosberg <dennis@stosberg.net>
---
builtin-log.c | 18 +++++++++++-------
1 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/builtin-log.c b/builtin-log.c
index 6612f4c..0018d13 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -206,14 +206,12 @@ int cmd_format_patch(int argc, const cha
keep_subject = 1;
rev.total = -1;
}
- else if (!strcmp(argv[i], "-o")) {
- if (argc < 3)
- die ("Which directory?");
- if (mkdir(argv[i + 1], 0777) < 0 && errno != EEXIST)
- die("Could not create directory %s",
- argv[i + 1]);
- output_directory = strdup(argv[i + 1]);
+ else if (!strcmp(argv[i], "-o") ||
+ !strcmp(argv[i], "--output-directory")) {
i++;
+ if (i == argc)
+ die ("Which directory?");
+ output_directory = strdup(argv[i]);
}
else if (!strcmp(argv[i], "--signoff") ||
!strcmp(argv[i], "-s")) {
@@ -243,6 +241,12 @@ int cmd_format_patch(int argc, const cha
if (argc > 1)
die ("unrecognized argument: %s", argv[1]);
+ if (output_directory && !stdout) {
+ if (mkdir(output_directory, 0777) < 0 && errno != EEXIST)
+ die("Could not create directory %s",
+ output_directory);
+ }
+
if (rev.pending_objects && rev.pending_objects->next == NULL) {
rev.pending_objects->item->flags |= UNINTERESTING;
add_head(&rev);
--
1.3.3+git20060602-dest0
^ permalink raw reply related
* Re: [PATCH] git-format-patch: add --output-directory long option again
From: Johannes Schindelin @ 2006-06-06 14:53 UTC (permalink / raw)
To: Dennis Stosberg; +Cc: git
In-Reply-To: <20060606141946.G59b8a1fd@leonov.stosberg.net>
Hi,
what idiot wrote that part of the code originally? ;-)
On Tue, 6 Jun 2006, Dennis Stosberg wrote:
> @@ -243,6 +241,12 @@ int cmd_format_patch(int argc, const cha
> if (argc > 1)
> die ("unrecognized argument: %s", argv[1]);
>
> + if (output_directory && !stdout) {
> + if (mkdir(output_directory, 0777) < 0 && errno != EEXIST)
> + die("Could not create directory %s",
> + output_directory);
> + }
> +
> if (rev.pending_objects && rev.pending_objects->next == NULL) {
> rev.pending_objects->item->flags |= UNINTERESTING;
> add_head(&rev);
Would it not be better to
if (output_directory && stdout)
die("What do you want: stdout or a directory?");
Ciao,
Dscho
^ permalink raw reply
* Re: Importing Mozilla CVS into git
From: Jon Smirl @ 2006-06-06 15:13 UTC (permalink / raw)
To: Martin Langhoff; +Cc: git
In-Reply-To: <46a038f90606052255s62cda81bt62d7442beb26658a@mail.gmail.com>
On 6/6/06, Martin Langhoff <martin.langhoff@gmail.com> wrote:
> On 6/3/06, Jon Smirl <jonsmirl@gmail.com> wrote:
> > On 6/1/06, Jon Smirl <jonsmirl@gmail.com> wrote:
> > > With the attached patch you can parse the entire Mozilla tree. The
> > > tree has over 100,000 files in it and about 300 branches.
> >
> > I was a little low with these counts, more like 110,000 files and some
> > parts of the tree have 1,000 branches. Total tree size is 3GB.
>
> I don't think it really has that many branches. If I am to believe
> cvsps (which took 3GB to walk the history), it has some branches with
> recursive loops in their ancestry (MANG_MATH_BRANCH and
> SpiderMonkey140_BRANCH have eachother as ancestors!?), 197969 commits
> and 796 branches.
It probably is 796 and not a 1,000. The branch names were scrolling
across my screen and I just estimated.
> This repository has been mangled quite badly. Don't know what you guys
> did with it, but it sure isn't pretty. I'm working on getting
> git-cvsimport to get through a complete import.
The repository is close to 10 years old and it has gone through a
number of corporate reorgs. Who knows what has happened to it over
that length of time.
Have you looked at the SVN CVS import tool? It imported Mozilla on the
first try. If you download the source they have built about 40 test
repositories with various errors. Those would make a good test suite
for cvsps. http://cvs2svn.tigris.org
I have been working on converting the svn tool to do git commands but
my git knowledge is limited so it has been slow going. The last stage,
pass 8, is very similar to what the git tools do. The svn commands
just need to be swapped for git ones.
If you get git-cvsimport working I'll use it instead. Will the cvsps
process stay small enough to run on a 32b machine? The svn tools are
very RAM efficient since they use an external db. Can cvsps read from
a local copy of the repository without using a CVS server?
We are going to have to develop some kind of incremental mechanism for
updating the new git tree. It can take up to two days to convert the
repository, Mozilla development can't be shut down that long for a
transition. Git will also need to mirror the CVS repository (check-in
still going to CVS) for a long time while we convince everyone on the
merits of switching.
My imported svn version of Mozilla has a lot of performance problems.
One of the directories has over 200,000 files in it slowing downing
the filesystem. The repository went from 3GB CVS to 8GB svn, probably
due to svn using 1000s of tiny files. I'll look around and see if svn
has a pack feature like git.
--
Jon Smirl
jonsmirl@gmail.com
^ permalink raw reply
* Re: New release?
From: Junio C Hamano @ 2006-06-06 15:20 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <Pine.LNX.4.63.0606061439500.28953@wbgn013.biozentrum.uni-wuerzburg.de>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> Also, what you talk about the "lazy clone" is a lot more involved than
>> what Eric wanted to have.
>
> A little more involved. And I wanted to know what people think about this
> way to tackle shallow clones.
This truly is a lot more involved, not because "lazy clone" is
hard (which it is) but because what Eric wants to have is not to
produce a shallow clone situation. The issue is about being
able to fetch an object that is not listed as one of the refs
the server side has, and it still will result in a fully
connected repository.
^ permalink raw reply
* Re: [PATCH] Cleanup git-send-email.perl:extract_valid_email
From: Horst von Brand @ 2006-06-06 15:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Horst von Brand, Ryan Anderson, Eric Wong, git
In-Reply-To: <7v1wu2zs5n.fsf@assigned-by-dhcp.cox.net>
Junio C Hamano <junkio@cox.net> wrote:
> Horst von Brand <vonbrand@inf.utfsm.cl> writes:
>
> >> > # check for a local address:
> >> > - return $address if ($address =~ /^([\w\-]+)$/);
> >> > + return $address if ($address =~ /^([\w\-.]+)$/);
> >>
> >> I keep forgetting this, '+' is a valid (and useful) setup, too.
> >
> > Oops...
> >>
> >> Actually, I'm retracting my earlier ack on this. This is way too
> >> restrictive. I'd rather allow an occasional invalid email address than
> >> to reject valid ones. I generally trust git users to know what they're
> >> doing when entering email addresses[1].
> >>
> >> *, $, ^, +, = are all valid characters in the username portion (not sure
> >> about local accounts, though), and I'm sure there are more that I don't
> >> know about.
> >
> > As a general principle, I prefer to check what is legal instead of trying
> > to filter out what isn't.
> If we start doing addr-spec in RFC2822 (page 17) ourselves, we
> should rather be using Email::Valid. A permissive sanity check
> to catch obvious mistakes would be more appropriate here than
> being RFC police.
OK.
> I think something like the attached, on top of your patch, would
> be appropriate for upcoming 1.4.0.
>
> -- >8 --
> send-email: be more lenient and just catch obvious mistakes.
>
> This cleans up the pattern matching subroutine by introducing
> two variables to hold regexp to approximately match local-part
> and domain in the e-mail address. It is meant to catch obvious
> mistakes with a cheap check.
>
> The patch also moves "scalar" to force Email::Valid->address()
> to work in !wantarray environment to extract_valid_address;
> earlier it was in the caller of the subroutine, which was way
> too error prone.
Right.
> ---
> diff --git a/git-send-email.perl b/git-send-email.perl
> index a7a7797..700d0c3 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -312,16 +312,18 @@ our ($message_id, $cc, %mail, $subject,
>
> sub extract_valid_address {
> my $address = shift;
> + my $local_part_regexp = '[^<>"\s@]+';
> + my $domain_regexp = '[^.<>"\s@]+\.[^<>"\s@]+';
This forces a '.' in the domain, while vonbrand@localhost is perfectly
reasonable. Plus it doesn't disallow adyacent '.'s. What about:
my $domain_regexp = '[^.<>"\s@]+(\.[^<>"\s@]+)*';
(but this is probably nitpicking...)
--
Dr. Horst H. von Brand User #22616 counter.li.org
Departamento de Informatica Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria +56 32 654239
Casilla 110-V, Valparaiso, Chile Fax: +56 32 797513
^ permalink raw reply
* Re: [PATCH] git-format-patch: add --output-directory long option again
From: Junio C Hamano @ 2006-06-06 15:46 UTC (permalink / raw)
To: Dennis Stosberg, Johannes Schindelin; +Cc: git
In-Reply-To: <20060606141946.G59b8a1fd@leonov.stosberg.net>
Dennis Stosberg <dennis@stosberg.net> writes:
> Additionally this fixes two minor issues:
Thanks.
> - else if (!strcmp(argv[i], "-o")) {
> - if (argc < 3)
> - die ("Which directory?");
Oops.
> + if (output_directory && !stdout) {
> + if (mkdir(output_directory, 0777) < 0 && errno != EEXIST)
> + die("Could not create directory %s",
> + output_directory);
> + }
> +
This code couldn't have been tested -- you meant to say
"use_stdout" here, not "stdout".
How about this instead?
-- >8 --
[PATCH] git-format-patch: add --output-directory long option again
Additionally notices and complains to an -o option without
directory or a duplicated -o option, -o and --stdout given
together. Also delays the creation of directory until all
arguments are parsed, so that the command does not leave an
empty directory behind when it exits after seeing an unrelated
invalid option.
[Originally from Dennis Stosberg but with minor fixes.]
Signed-off-by: Junio C Hamano <junkio@cox.net>
---
builtin-log.c | 26 ++++++++++++++++----------
1 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/builtin-log.c b/builtin-log.c
index 6612f4c..29a8851 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -103,7 +103,7 @@ static int git_format_config(const char
static FILE *realstdout = NULL;
-static char *output_directory = NULL;
+static const char *output_directory = NULL;
static void reopen_stdout(struct commit *commit, int nr, int keep_subject)
{
@@ -206,14 +206,14 @@ int cmd_format_patch(int argc, const cha
keep_subject = 1;
rev.total = -1;
}
- else if (!strcmp(argv[i], "-o")) {
- if (argc < 3)
- die ("Which directory?");
- if (mkdir(argv[i + 1], 0777) < 0 && errno != EEXIST)
- die("Could not create directory %s",
- argv[i + 1]);
- output_directory = strdup(argv[i + 1]);
+ else if (!strcmp(argv[i], "--output-directory") ||
+ !strcmp(argv[i], "-o")) {
i++;
+ if (argc <= i)
+ die("Which directory?");
+ if (output_directory)
+ die("Two output directories?");
+ output_directory = argv[i];
}
else if (!strcmp(argv[i], "--signoff") ||
!strcmp(argv[i], "-s")) {
@@ -243,6 +243,14 @@ int cmd_format_patch(int argc, const cha
if (argc > 1)
die ("unrecognized argument: %s", argv[1]);
+ if (output_directory) {
+ if (use_stdout)
+ die("standard output, or directory, which one?");
+ if (mkdir(output_directory, 0777) < 0 && errno != EEXIST)
+ die("Could not create directory %s",
+ output_directory);
+ }
+
if (rev.pending_objects && rev.pending_objects->next == NULL) {
rev.pending_objects->item->flags |= UNINTERESTING;
add_head(&rev);
@@ -293,8 +301,6 @@ int cmd_format_patch(int argc, const cha
if (!use_stdout)
fclose(stdout);
}
- if (output_directory)
- free(output_directory);
free(list);
return 0;
}
--
1.4.0.rc1-dirty
^ permalink raw reply related
* Re: [PATCH] Cleanup git-send-email.perl:extract_valid_email
From: Junio C Hamano @ 2006-06-06 15:54 UTC (permalink / raw)
To: Horst von Brand; +Cc: git
In-Reply-To: <200606061542.k56Fg9Cm006226@laptop11.inf.utfsm.cl>
Horst von Brand <vonbrand@inf.utfsm.cl> writes:
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index a7a7797..700d0c3 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -312,16 +312,18 @@ our ($message_id, $cc, %mail, $subject,
>>
>> sub extract_valid_address {
>> my $address = shift;
>> + my $local_part_regexp = '[^<>"\s@]+';
>> + my $domain_regexp = '[^.<>"\s@]+\.[^<>"\s@]+';
>
> This forces a '.' in the domain, while vonbrand@localhost is perfectly
> reasonable. Plus it doesn't disallow adyacent '.'s. What about:
>
> my $domain_regexp = '[^.<>"\s@]+(\.[^<>"\s@]+)*';
>
> (but this is probably nitpicking...)
I do not have preference either way about allowing an address
like tld-administrator@net myself, but Email::Valid->address
does not seem to allow it, and I just copied that behaviour for
consistency between two alternative implementations.
I think you meant to say:
> my $domain_regexp = '[^.<>"\s@]+(\.[^.<>"\s@]+)*';
(i.e. exclude dot from the latter character class), but I am
inclined to do this instead:
my $domain_regexp = '[^.<>"\s@]+(?:\.[^.<>"\s@]+)+';
(i.e. still require at least two levels).
^ permalink raw reply
* Re: [PATCH] Cleanup git-send-email.perl:extract_valid_email
From: Horst von Brand @ 2006-06-06 16:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Horst von Brand, git
In-Reply-To: <7vpshmth3q.fsf@assigned-by-dhcp.cox.net>
Junio C Hamano <junkio@cox.net> wrote:
> Horst von Brand <vonbrand@inf.utfsm.cl> writes:
>
> >> diff --git a/git-send-email.perl b/git-send-email.perl
> >> index a7a7797..700d0c3 100755
> >> --- a/git-send-email.perl
> >> +++ b/git-send-email.perl
> >> @@ -312,16 +312,18 @@ our ($message_id, $cc, %mail, $subject,
> >>
> >> sub extract_valid_address {
> >> my $address = shift;
> >> + my $local_part_regexp = '[^<>"\s@]+';
> >> + my $domain_regexp = '[^.<>"\s@]+\.[^<>"\s@]+';
> >
> > This forces a '.' in the domain, while vonbrand@localhost is perfectly
> > reasonable. Plus it doesn't disallow adyacent '.'s. What about:
> >
> > my $domain_regexp = '[^.<>"\s@]+(\.[^<>"\s@]+)*';
> >
> > (but this is probably nitpicking...)
>
> I do not have preference either way about allowing an address
> like tld-administrator@net myself, but Email::Valid->address
> does not seem to allow it, and I just copied that behaviour for
> consistency between two alternative implementations.
Reasonable.
> I think you meant to say:
>
> > my $domain_regexp = '[^.<>"\s@]+(\.[^.<>"\s@]+)*';
>
> (i.e. exclude dot from the latter character class),
Right, my bad.
> but I am
> inclined to do this instead:
>
> my $domain_regexp = '[^.<>"\s@]+(?:\.[^.<>"\s@]+)+';
>
> (i.e. still require at least two levels).
OK, but be careful as this (?:...) is an extended regexp (needs /x on
match). I'd just leave it plain (the performance impact shouldn't be
noticeable). I don't see any use except for $1, so the extra parenthesis
should be safe.
--
Dr. Horst H. von Brand User #22616 counter.li.org
Departamento de Informatica Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria +56 32 654239
Casilla 110-V, Valparaiso, Chile Fax: +56 32 797513
^ 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