* Re: [PATCH 1/3] Add git-send-email-script - tool to send emails from git-format-patch-script
From: Ryan Anderson @ 2005-07-31 23:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vbr4jmhqe.fsf@assigned-by-dhcp.cox.net>
On Sun, Jul 31, 2005 at 02:45:29AM -0700, Junio C Hamano wrote:
> Ryan Anderson <ryan@michonline.com> writes:
>
> > All emails are sent as a reply to the previous email, making it easy to
> > skip a collection of emails that are uninteresting.
>
> I understand why _some_ people consider this preferable, but
> wonder if this should have a knob to be tweaked.
Hmm, fair enough.
I'll send a few more patches in a minute that deal with the things in
this email but for now:
--chain-reply-to (or --no-chain-reply-to)
Will toggle between these two behaviors. (This will not be
prompted for by the ReadLine interface, btw.)
> > +# horrible hack of a script to send off a large number of email messages, one after
> > +# each other, all chained together. This is useful for large numbers of patches.
> > +#
> > +# Use at your own risk!!!!
>
> Well, if it is "Use at your own risk" maybe it should stay
> outside the official distribution for a while until it gets
> safer ;-).
Heh. I missed some comments that I meant to clean up that were in
Greg's original script. One of the patches will clean up the comments.
> > + my @fields = split /\s+/, $data;
> > + my $ident = join(" ", @fields[0...(@fields-3)]);
>
> Wouldn't "s/>.*/>/" be easier than splitting and joining?
Most of GIT_COMMITTER_IDENT (and GIT_AUTHOR_IDENT) is use controllable,
except for the "date" section of it. I know we delimit that with
spaces, so the above is guaranteed to work unless we change the format
that git-var returns.
If I hope that nobody has done something like:
GIT_AUTHOR="Ryan <> Anderson"
GIT_AUTHOR_EMAIL="ryan@michonline.com"
I get more confusing results. (I suddenly have to think about what that
regular expression does in this case - and I'm pretty sure that the one
you gave would do bad things.)
Probably the best fix for this would be to take libgit.a, make a shared
library out of it, and then interface the Perl scripts directly with it
via a .xs module. I was thinking that I'd rather have direct access to
the git_ident* functions than calling out to git-var, anyway. Consider
that a plan for a revamp after the core seems to have settled down a bit
more.
> > +if (!defined $from) {
> > + $from = $author || $committer;
> > + 1 while (!defined ($_ = $term->readline("Who should the emails appear to be from? ",
> > + $from)));
>
> Judging from your past patches, you seem to really like
> statement modifiers[*]. While they _are_ valid Perl constructs,
> it is extremely hard to read when used outside very simple
> idiomatic use. Please consider rewriting the above and the like
> using compound statements[*] (I am using these terms according
> to the definition in perlsyn.pod). Remember, there are people
> Perl is not their native language, but are intelligent enough to
> be of great help fixing problems in programs you write in Perl.
> To most of them, compound statements are more familiar, so try
> to be gentle to them.
I copied this from another program of mine, and I'm *sure* I copied the
style directly from a ReadLine example. But, I can't find a current
example that says this is good, so, I'll fix this, too. It is rather
ugly. (The other uses of this style are... less bad, IMO, than this
abuse here.)
>
> > + opendir(DH,$f)
> > + or die "Failed to opendir $f: $!";
> > + push @files, map { +$f . "/" . $_ } grep !/^\.{1,2}$/,
> > + sort readdir(DH);
>
> Maybe skip potential subdirs while you are at it, something like this?
>
> push @files, sort grep { -f $_ } map { "$f/$_" } readdir(DH)
Good point. One one hand I'd say, "Let it break for people who do
strange things like that", but I'll make it safer anyway.
(Someone is going to reply and ask for it to recurse into subdirectories
now. Maybe Andrew Morton would find that useful with his rather massive
collection of patches in -mm kernels. But that's a feature for next
week.)
> > + my $pseudo_rand = int (rand(4200));
> > + $message_id = "<$date$pseudo_rand\@foobar.com>";
> > + print "new message id = $message_id\n";
>
> I doubt this hardcoded foobar.com is a good idea. Did you mean
> to print it, by the way?
I'll convert this to something that is based off the $from address
instead. It's probably better that way, anyway.
> > + $to{lc(Email::Valid->address($_))}++ for (@to);
> > + my $to = join(",", keys %to);
>
> Is this the culprit that produced this mechanical-looking line?
>
> To: junkio@cox.net,git@vger.kernel.org
No, that line was exactly what I put into the readline entry.
> Interestingly enough, you do not seem to do it for the From:
> line.
>
> From: Ryan Anderson <ryan@michonline.com>
>
> Also you seem to be losing the ordering in @to and @cc by the
> use of uniquefying "keys %to" and "keys %cc". I can not offhand
> tell if it matters, but you probably would care, at least for
> the primary recipients listed in @to array.
Well, it was kind of annoying to see the same email address appear 2-3
times in the email, because of the way I pull in all the relevant emails
from various places. So I really needed a way to cull the duplicates.
I don't believe ordering is really significant in To: or Cc: lines, for
really anyone. I could do soemthing like this, instead, I suppose:
my @clean_to = ();
my %dupe_check_to = ();
foreach my $to_entry (@to) {
if (!$dupe_check_to{Email::Valid->address($to_entry)}++) {
push @clean_to, $to_entry;
}
}
my $to = join(", ", @clean_to);
I just like the first one a little better (though, I can't really pin
down why).
> > + $mail{smtp} = 'localhost';
>
> I suspect this probably need to be configurable. I may be a
> minority, but my outgoing messages are directly handed to my
> local ISP smtp server from my MUA, and the smtp server running
> on the locahost does not talk to the outside world.
Oddly, I think this is even the wrong setting after having read over the
Mail::Sendmail docs to figure out how to change the MIME settings.
> Since there are always 47 different ways to do the same thing in
> Perl, Perl style varies a lot more than Shell style which in
> turn varies a lot more than C. You should be prepared to be
> nitpicked a lot when you post Perl ;-). And I was in nitpicking
> mood tonight.
It's Perl. There does seem to be a lot of nitpicking when people write
Perl.
(If you write it in idiomatic Perl, the non-Perl users hate you. If you
write it like C, the Perl users complain that you write it like C.
*sigh*)
> Thanks for the patch. Overall, very good intent. Slightly
> troublesome details.
Thanks.
I'll have a series of patches out in a few minutes that will update what
I originally sent to fix your issues here.
--
Ryan Anderson
sometimes Pug Majere
^ permalink raw reply
* Re: [PATCH 1/2] Functions for managing the set of packs the library is using
From: Junio C Hamano @ 2005-07-31 23:45 UTC (permalink / raw)
To: barkalow; +Cc: git
In-Reply-To: <Pine.LNX.4.62.0507311609420.23721@iabervon.org>
Daniel, I would really have liked to merge this immediately, but
somehow the patch is whitespace damaged. Depressingly enough,
almost all patches I got from various people today had different
whitespace damages, and I started to suspect if there is
something wrong on my end, but it does not appear to be the
case. The copies found in the usual archive I check show the
same problems as the copies I got directly in my mailbox have.
Could you resend these two if it is not too much trouble for
you?
Thanks for fixing the curl_easy_setopt() screwup. I should have
been more careful. I've already hand-merged it and will be
pushing it out shortly, so there is no need to resend that one.
^ permalink raw reply
* Re: [PATCH] Added hook in git-receive-pack
From: Johannes Schindelin @ 2005-07-31 23:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, Josef Weidendorfer, Git Mailing List
In-Reply-To: <7viryqd0eo.fsf@assigned-by-dhcp.cox.net>
Hi,
On Sun, 31 Jul 2005, Junio C Hamano wrote:
> Let's yank out the update_server_info() call when Josef's patch
> can handle a single hook call at the end of the run, in addition
> to one call per each ref getting updated.
How about executing update_server_info() if no hook was found? That way,
it can be turned off by an empty hook, but is enabled in standard
settings.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] Added hook in git-receive-pack
From: Linus Torvalds @ 2005-07-31 23:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Josef Weidendorfer, Git Mailing List
In-Reply-To: <7viryqd0eo.fsf@assigned-by-dhcp.cox.net>
On Sun, 31 Jul 2005, Junio C Hamano wrote:
> Linus Torvalds <torvalds@osdl.org> writes:
>
> > No I'm not. Try all the machines behind my firewall.
>
> Ah, that's true. Do you push into them?
Yup, I do. I have this thing that I don't do backups, but I end up having
redundancy instead, so I keep archives on my own machines and inside the
private osdl network, for example.
Also, I suspect that anybody who uses the "CVS model" with git - ie a
central repository - is not likely to export that central repository any
way: it's the crown jewels, after all. Open source may not have that
mindset, but I'm thinking of how I was forced to use CVS at Transmeta, for
example: the machine that had the CVS repo was certainly supposed to be
very private indeed.
In the "central repo model" you have another issue - you have potentially
parallell pushes to different branches with no locking what-so-ever (and
that's definitely _supposed_ to work), and I have this suspicion that the
"update for dumb servers" code isn't really safe in that setting anyway. I
haven't checked.
Linus
^ permalink raw reply
* Re: [PATCH] Added hook in git-receive-pack
From: Junio C Hamano @ 2005-07-31 23:24 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Josef Weidendorfer, Git Mailing List
In-Reply-To: <Pine.LNX.4.58.0507311549300.14342@g5.osdl.org>
Linus Torvalds <torvalds@osdl.org> writes:
> No I'm not. Try all the machines behind my firewall.
Ah, that's true. Do you push into them?
Let's yank out the update_server_info() call when Josef's patch
can handle a single hook call at the end of the run, in addition
to one call per each ref getting updated.
^ permalink raw reply
* Re: [PATCH] Added hook in git-receive-pack
From: Josef Weidendorfer @ 2005-07-31 23:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v3bpuenpi.fsf@assigned-by-dhcp.cox.net>
On Sunday 31 July 2005 22:15, Junio C Hamano wrote:
> Josef Weidendorfer <Josef.Weidendorfer@gmx.de> writes:
> > +It is assured that sha1-old is an ancestor of sha1-new (otherwise,
> > +the update would have not been allowed). refname is relative to
> > +$GIT_DIR; e.g. for the master head this is "refs/heads/master".
>
> I think this description is inaccurate;
Thanks for the constructive comments; that patch was only a draft, and
tailored for my needs. I thought it would be better to provide a patch than
requesting for a feature.
I am trying to convert a CVS project with a few developers to GIT; the idea is
that each developer has his public branch in *the* central repository, and
(s)he is allowed to merge to master him/herself; when pushing new features
into the branches or merging to master, there should be send out a mail to a
mailing list.
> the send-pack can be run
> with the --force flag and it is my understanding that receiver
> would happily rewind the branch.
I didn't know this...
> One possibility, if we wanted
> to enforce it on the receiver end,
I actually thought that the ancestor relationship always is given; perhaps we
don't need to enforce this; but before sending out a mail, the hook script
probably would like to check if there is any ancestor relationship; this
would be a potential long lasting task, wouldn't it?
> would be to add another hook
> that is called before the rename happens and tell the
> receive-pack to refuse that update, but that should be done with
> a separate patch, I suppose.
Sorry, I do not understand this.
> > +Using this hook, it is easy to generate mails on updates to
> > +the local repository. This example script sends a mail with
> > +the commits pushed to the repository:
> > +
> > + #!/bin/sh
> > + git-rev-list --pretty "$3" "^$2" |
> > + mail -r $USER -s "New commits on $1" commit-list@mydomain
>
> What is the environment the hook runs in? For example, who
> defines $USER used here?
Good question. I thought it is UNIX/POSIX behavior to set this environemt in
shells (same as "id -u -n"). At least, ssh sets it to "the user logging
in" (see man ssh).
And I supposed "git-receive-pack" to be called in a users SSH environment.
Hmmm... could this be called via CGI script by a web server?
You are right: we should be careful here. Is there any other hook mechanism in
GIT at the moment? Originally, I thought this should be a task for a
porcelain, but this thing is buried too deep in GIT itself...
All the issues about documenting the environment of the hooks, time out
behavior and so on, are general issues for every kind of hook.
> I am not saying this from the security standpoint (the fact that
> you can invoke receive-pack and that you can write into the
> update hooks means you already have control over that
> repository), but to help hook writers to avoid making mistakes.
> For example, I offhand cannot tell what happens if the hook
> tries to read from its standard input. Also what happens if the
> hook does not return but sleeps forever in a loop? Do we want
> to somehow time it out? I think "It is hooks' responsibility to
> time itself out" is an acceptable answer here, but if that is
> the case it had better be documented.
I think that a time out is not needed here: as the hook is called synchronous,
git-receive-pack won't return without until the hook terminated. And that is
visible to the user, i.e. he would see that there is something wrong.
> I think I've seen this "fork -- exec -- for loop with waitpid"
> pattern repeated number of times in the code.
This is no coincidence ;-) I copied it from inside the same file.
But the behavior is another: If the hook goes wrong, I do not want for
git-receive-pack to die.
> Would it be
> feasible to make them into a single library-ish function and
> call it from here and other existing places?
Probably.
> Another thing you may want to consider is to do this hook
> processing before and/or after processing all the refs. A hook
> might want to know what the entire set of refs are that are
> being updated, and may not have enough information if it is
> invoked once per ref.
Do you have a use case? At least, it would make things more complex.
Josef
^ permalink raw reply
* Re: [PATCH] Added hook in git-receive-pack
From: Linus Torvalds @ 2005-07-31 22:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Josef Weidendorfer, Git Mailing List
In-Reply-To: <7vr7ded8ax.fsf@assigned-by-dhcp.cox.net>
On Sun, 31 Jul 2005, Junio C Hamano wrote:
>
> But you are. I can run this just fine:
No I'm not. Try all the machines behind my firewall.
kernel.org is just the place I put things to when I publish them. It
doesn't have any of my working directories on it.
Linus
^ permalink raw reply
* Re: Terminology
From: Johannes Schindelin @ 2005-07-31 22:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vhdeaj05n.fsf@assigned-by-dhcp.cox.net>
Hi,
I tried to avoid the work. But I'll do it.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] Added hook in git-receive-pack
From: Junio C Hamano @ 2005-07-31 20:33 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Josef Weidendorfer, Git Mailing List
In-Reply-To: <Pine.LNX.4.58.0507311305170.29650@g5.osdl.org>
Linus Torvalds <torvalds@osdl.org> writes:
> This looks sane. However, I also get the strong feeling that
> git-update-server-info should be run as part of a hook and not be built
> into receive-pack..
> Personally, I simply don't want to update any dumb server info stuff for
> my own local repositories - it's not like I'm actually serving those out
> anyway.
But you are. I can run this just fine:
$ git clone http://www.kernel.org/pub/scm/linux/kernel/git/torvalds/git.git/ linus
I agree in principle that you should be able to disable the call
to update_server_info() from there, but on the other hand once
we start doing it, we need to explain people which repo is http
capable and which repo is not and why.
I was actually thinking about a call to git-update-server-info
at the end of git-repack-script. Again, great minds think the
opposite way sometimes ;-).
^ permalink raw reply
* Re: [PATCH] Added hook in git-receive-pack
From: Junio C Hamano @ 2005-07-31 20:15 UTC (permalink / raw)
To: Josef Weidendorfer; +Cc: git
In-Reply-To: <200507312117.43957.Josef.Weidendorfer@gmx.de>
Josef Weidendorfer <Josef.Weidendorfer@gmx.de> writes:
> +It is assured that sha1-old is an ancestor of sha1-new (otherwise,
> +the update would have not been allowed). refname is relative to
> +$GIT_DIR; e.g. for the master head this is "refs/heads/master".
I think this description is inaccurate; the send-pack can be run
with the --force flag and it is my understanding that receiver
would happily rewind the branch. One possibility, if we wanted
to enforce it on the receiver end, would be to add another hook
that is called before the rename happens and tell the
receive-pack to refuse that update, but that should be done with
a separate patch, I suppose.
> +Using this hook, it is easy to generate mails on updates to
> +the local repository. This example script sends a mail with
> +the commits pushed to the repository:
> +
> + #!/bin/sh
> + git-rev-list --pretty "$3" "^$2" |
> + mail -r $USER -s "New commits on $1" commit-list@mydomain
What is the environment the hook runs in? For example, who
defines $USER used here?
We might want to describe the environment a bit more tightly
than the current patch does. This includes not just the
environment variables, but $cwd and the set of open file
descriptors among other things.
I am not saying this from the security standpoint (the fact that
you can invoke receive-pack and that you can write into the
update hooks means you already have control over that
repository), but to help hook writers to avoid making mistakes.
For example, I offhand cannot tell what happens if the hook
tries to read from its standard input. Also what happens if the
hook does not return but sleeps forever in a loop? Do we want
to somehow time it out? I think "It is hooks' responsibility to
time itself out" is an acceptable answer here, but if that is
the case it had better be documented.
> +static void updatehook(const char *name, unsigned char *old_sha1, unsigned char *new_sha1)
> +{
> + if (access(update_hook, X_OK) < 0) return;
> + fprintf(stderr, "executing update hook for %s\n", name);
> +...
> +}
I think I've seen this "fork -- exec -- for loop with waitpid"
pattern repeated number of times in the code. Would it be
feasible to make them into a single library-ish function and
call it from here and other existing places?
Another thing you may want to consider is to do this hook
processing before and/or after processing all the refs. A hook
might want to know what the entire set of refs are that are
being updated, and may not have enough information if it is
invoked once per ref.
Thanks for the patch; I agree with what the patch tries to
achieve in general.
-jc
^ permalink raw reply
* Re: [PATCH] Added hook in git-receive-pack
From: Linus Torvalds @ 2005-07-31 20:11 UTC (permalink / raw)
To: Josef Weidendorfer, Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <200507312117.43957.Josef.Weidendorfer@gmx.de>
On Sun, 31 Jul 2005, Josef Weidendorfer wrote:
>
> Added hook in git-receive-pack
>
> After successful update of a ref,
>
> $GIT_DIR/hooks/update refname old-sha1 new-sha2
>
> is called if present. This allows e.g sending of a mail
> with pushed commits on the remote repository.
> Documentation update with example hook included.
This looks sane. However, I also get the strong feeling that
git-update-server-info should be run as part of a hook and not be built
into receive-pack..
Personally, I simply don't want to update any dumb server info stuff for
my own local repositories - it's not like I'm actually serving those out
anyway.
Linus
^ permalink raw reply
* [PATCH 1/2] Functions for managing the set of packs the library is using
From: barkalow @ 2005-07-31 20:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <Pine.LNX.4.62.0507311600040.23721@iabervon.org>
This adds support for reading an uninstalled index, and installing a
pack file that was added while the program was running, as well as
functions for determining where to put the file.
Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---
cache.h | 13 ++++++
sha1_file.c | 123 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 135 insertions(+), 1 deletions(-)
20fcc8f66a6780cf9bbd2fc2ba3b918c33696a67
diff --git a/cache.h b/cache.h
--- a/cache.h
+++ b/cache.h
@@ -172,6 +172,8 @@ extern void rollback_index_file(struct c
extern char *mkpath(const char *fmt, ...);
extern char *git_path(const char *fmt, ...);
extern char *sha1_file_name(const unsigned char *sha1);
+extern char *sha1_pack_name(const unsigned char *sha1);
+extern char *sha1_pack_index_name(const unsigned char *sha1);
int safe_create_leading_directories(char *path);
@@ -200,6 +202,9 @@ extern int write_sha1_to_fd(int fd, cons
extern int has_sha1_pack(const unsigned char *sha1);
extern int has_sha1_file(const unsigned char *sha1);
+extern int has_pack_file(const unsigned char *sha1);
+extern int has_pack_index(const unsigned char *sha1);
+
/* Convert to/from hex/sha1 representation */
extern int get_sha1(const char *str, unsigned char *sha1);
extern int get_sha1_hex(const char *hex, unsigned char *sha1);
@@ -276,6 +281,7 @@ extern struct packed_git {
void *pack_base;
unsigned int pack_last_used;
unsigned int pack_use_cnt;
+ unsigned char sha1[20];
char pack_name[0]; /* something like ".git/objects/pack/xxxxx.pack" */
} *packed_git;
@@ -298,7 +304,14 @@ extern int path_match(const char *path,
extern int get_ack(int fd, unsigned char *result_sha1);
extern struct ref **get_remote_heads(int in, struct ref **list, int nr_match, char **match);
+extern struct packed_git *parse_pack_index(unsigned char *sha1);
+
extern void prepare_packed_git(void);
+extern void install_packed_git(struct packed_git *pack);
+
+extern struct packed_git *find_sha1_pack(const unsigned char *sha1,
+ struct packed_git *packs);
+
extern int use_packed_git(struct packed_git *);
extern void unuse_packed_git(struct packed_git *);
extern struct packed_git *add_packed_git(char *, int);
diff --git a/sha1_file.c b/sha1_file.c
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -200,6 +200,56 @@ char *sha1_file_name(const unsigned char
return base;
}
+char *sha1_pack_name(const unsigned char *sha1)
+{
+ static const char hex[] = "0123456789abcdef";
+ static char *name, *base, *buf;
+ int i;
+
+ if (!base) {
+ const char *sha1_file_directory = get_object_directory();
+ int len = strlen(sha1_file_directory);
+ base = xmalloc(len + 60);
+ sprintf(base, "%s/pack/pack-1234567890123456789012345678901234567890.pack", sha1_file_directory);
+ name = base + len + 11;
+ }
+
+ buf = name;
+
+ for (i = 0; i < 20; i++) {
+ unsigned int val = *sha1++;
+ *buf++ = hex[val >> 4];
+ *buf++ = hex[val & 0xf];
+ }
+
+ return base;
+}
+
+char *sha1_pack_index_name(const unsigned char *sha1)
+{
+ static const char hex[] = "0123456789abcdef";
+ static char *name, *base, *buf;
+ int i;
+
+ if (!base) {
+ const char *sha1_file_directory = get_object_directory();
+ int len = strlen(sha1_file_directory);
+ base = xmalloc(len + 60);
+ sprintf(base, "%s/pack/pack-1234567890123456789012345678901234567890.idx", sha1_file_directory);
+ name = base + len + 11;
+ }
+
+ buf = name;
+
+ for (i = 0; i < 20; i++) {
+ unsigned int val = *sha1++;
+ *buf++ = hex[val >> 4];
+ *buf++ = hex[val & 0xf];
+ }
+
+ return base;
+}
+
struct alternate_object_database *alt_odb;
/*
@@ -360,6 +410,14 @@ void unuse_packed_git(struct packed_git
int use_packed_git(struct packed_git *p)
{
+ if (!p->pack_size) {
+ struct stat st;
+ // We created the struct before we had the pack
+ stat(p->pack_name, &st);
+ if (!S_ISREG(st.st_mode))
+ die("packfile %s not a regular file", p->pack_name);
+ p->pack_size = st.st_size;
+ }
if (!p->pack_base) {
int fd;
struct stat st;
@@ -387,8 +445,10 @@ int use_packed_git(struct packed_git *p)
* this is cheap.
*/
if (memcmp((char*)(p->index_base) + p->index_size - 40,
- p->pack_base + p->pack_size - 20, 20))
+ p->pack_base + p->pack_size - 20, 20)) {
+
die("packfile %s does not match index.", p->pack_name);
+ }
}
p->pack_last_used = pack_used_ctr++;
p->pack_use_cnt++;
@@ -426,6 +486,37 @@ struct packed_git *add_packed_git(char *
return p;
}
+struct packed_git *parse_pack_index(unsigned char *sha1)
+{
+ struct packed_git *p;
+ unsigned long idx_size;
+ void *idx_map;
+ char *path = sha1_pack_index_name(sha1);
+
+ if (check_packed_git_idx(path, &idx_size, &idx_map))
+ return NULL;
+
+ path = sha1_pack_name(sha1);
+
+ p = xmalloc(sizeof(*p) + strlen(path) + 2);
+ strcpy(p->pack_name, path);
+ p->index_size = idx_size;
+ p->pack_size = 0;
+ p->index_base = idx_map;
+ p->next = NULL;
+ p->pack_base = NULL;
+ p->pack_last_used = 0;
+ p->pack_use_cnt = 0;
+ memcpy(p->sha1, sha1, 20);
+ return p;
+}
+
+void install_packed_git(struct packed_git *pack)
+{
+ pack->next = packed_git;
+ packed_git = pack;
+}
+
static void prepare_packed_git_one(char *objdir)
{
char path[PATH_MAX];
@@ -989,6 +1080,20 @@ static int find_pack_entry(const unsigne
return 0;
}
+struct packed_git *find_sha1_pack(const unsigned char *sha1,
+ struct packed_git *packs)
+{
+ struct packed_git *p;
+ struct pack_entry e;
+
+ for (p = packs; p; p = p->next) {
+ if (find_pack_entry_one(sha1, &e, p))
+ return p;
+ }
+ return NULL;
+
+}
+
int sha1_object_info(const unsigned char *sha1, char *type, unsigned long *sizep)
{
int status;
@@ -1335,6 +1440,22 @@ int write_sha1_from_fd(const unsigned ch
return 0;
}
+int has_pack_index(const unsigned char *sha1)
+{
+ struct stat st;
+ if (stat(sha1_pack_index_name(sha1), &st))
+ return 0;
+ return 1;
+}
+
+int has_pack_file(const unsigned char *sha1)
+{
+ struct stat st;
+ if (stat(sha1_pack_name(sha1), &st))
+ return 0;
+ return 1;
+}
+
int has_sha1_pack(const unsigned char *sha1)
{
struct pack_entry e;
^ permalink raw reply
* [PATCH 2/2] Support downloading packs by HTTP
From: barkalow @ 2005-07-31 20:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <Pine.LNX.4.62.0507311600040.23721@iabervon.org>
This adds support to http-pull for finding the list of pack files
available on the server, downloading the index files for those pack
files, and downloading pack files when they contain needed objects not
available individually. It retains the index files even if the pack
files were not needed, but downloads the list of pack files once per
run if an object is not found separately.
Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---
http-pull.c | 181 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
1 files changed, 175 insertions(+), 6 deletions(-)
dff0b76c4a2efbb8407778a1da6dc2ea2ca1458f
diff --git a/http-pull.c b/http-pull.c
--- a/http-pull.c
+++ b/http-pull.c
@@ -33,7 +33,8 @@ struct buffer
};
static size_t fwrite_buffer(void *ptr, size_t eltsize, size_t nmemb,
- struct buffer *buffer) {
+ struct buffer *buffer)
+{
size_t size = eltsize * nmemb;
if (size > buffer->size - buffer->posn)
size = buffer->size - buffer->posn;
@@ -42,8 +43,9 @@ static size_t fwrite_buffer(void *ptr, s
return size;
}
-static size_t fwrite_sha1_file(void *ptr, size_t eltsize, size_t nmemb,
- void *data) {
+static size_t fwrite_sha1_file(void *ptr, size_t eltsize, size_t nmemb,
+ void *data)
+{
unsigned char expn[4096];
size_t size = eltsize * nmemb;
int posn = 0;
@@ -65,6 +67,168 @@ static size_t fwrite_sha1_file(void *ptr
return size;
}
+static int got_indices = 0;
+
+static struct packed_git *packs = NULL;
+
+static int fetch_index(unsigned char *sha1)
+{
+ char *filename;
+ char *url;
+
+ FILE *indexfile;
+
+ if (has_pack_index(sha1))
+ return 0;
+
+ if (get_verbosely)
+ fprintf(stderr, "Getting index for pack %s\n",
+ sha1_to_hex(sha1));
+
+ url = xmalloc(strlen(base) + 64);
+ sprintf(url, "%s/objects/pack/pack-%s.idx",
+ base, sha1_to_hex(sha1));
+
+ filename = sha1_pack_index_name(sha1);
+ indexfile = fopen(filename, "w");
+ if (!indexfile)
+ return error("Unable to open local file %s for pack index",
+ filename);
+
+ curl_easy_setopt(curl, CURLOPT_FILE, indexfile);
+ curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, fwrite);
+ curl_easy_setopt(curl, CURLOPT_URL, url);
+
+ if (curl_easy_perform(curl)) {
+ fclose(indexfile);
+ return error("Unable to get pack index %s", url);
+ }
+
+ fclose(indexfile);
+ return 0;
+}
+
+static int setup_index(unsigned char *sha1)
+{
+ struct packed_git *new_pack;
+ if (has_pack_file(sha1))
+ return 0; // don't list this as something we can get
+
+ if (fetch_index(sha1))
+ return -1;
+
+ new_pack = parse_pack_index(sha1);
+ new_pack->next = packs;
+ packs = new_pack;
+ return 0;
+}
+
+static int fetch_indices(void)
+{
+ unsigned char sha1[20];
+ char *url;
+ struct buffer buffer;
+ char *data;
+ int i = 0;
+
+ if (got_indices)
+ return 0;
+
+ data = xmalloc(4096);
+ buffer.size = 4096;
+ buffer.posn = 0;
+ buffer.buffer = data;
+
+ if (get_verbosely)
+ fprintf(stderr, "Getting pack list\n");
+
+ url = xmalloc(strlen(base) + 21);
+ sprintf(url, "%s/objects/info/packs", base);
+
+ curl_easy_setopt(curl, CURLOPT_FILE, &buffer);
+ curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
+ curl_easy_setopt(curl, CURLOPT_URL, url);
+
+ if (curl_easy_perform(curl)) {
+ return error("Unable to get pack index %s", url);
+ }
+
+ do {
+ switch (data[i]) {
+ case 'P':
+ i++;
+ if (i + 52 < buffer.posn &&
+ !strncmp(data + i, " pack-", 6) &&
+ !strncmp(data + i + 46, ".pack\n", 6)) {
+ get_sha1_hex(data + i + 6, sha1);
+ setup_index(sha1);
+ i += 51;
+ break;
+ }
+ default:
+ while (data[i] != '\n')
+ i++;
+ }
+ i++;
+ } while (i < buffer.posn);
+
+ got_indices = 1;
+ return 0;
+}
+
+static int fetch_pack(unsigned char *sha1)
+{
+ char *url;
+ struct packed_git *target;
+ struct packed_git **lst;
+ FILE *packfile;
+ char *filename;
+
+ if (fetch_indices())
+ return -1;
+ target = find_sha1_pack(sha1, packs);
+ if (!target)
+ return error("Couldn't get %s: not separate or in any pack",
+ sha1_to_hex(sha1));
+
+ if (get_verbosely) {
+ fprintf(stderr, "Getting pack %s\n",
+ sha1_to_hex(target->sha1));
+ fprintf(stderr, " which contains %s\n",
+ sha1_to_hex(sha1));
+ }
+
+ url = xmalloc(strlen(base) + 65);
+ sprintf(url, "%s/objects/pack/pack-%s.pack",
+ base, sha1_to_hex(target->sha1));
+
+ filename = sha1_pack_name(target->sha1);
+ packfile = fopen(filename, "w");
+ if (!packfile)
+ return error("Unable to open local file %s for pack",
+ filename);
+
+ curl_easy_setopt(curl, CURLOPT_FILE, packfile);
+ curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, fwrite);
+ curl_easy_setopt(curl, CURLOPT_URL, url);
+
+ if (curl_easy_perform(curl)) {
+ fclose(packfile);
+ return error("Unable to get pack file %s", url);
+ }
+
+ fclose(packfile);
+
+ lst = &packs;
+ while (*lst != target)
+ lst = &((*lst)->next);
+ *lst = (*lst)->next;
+
+ install_packed_git(target);
+
+ return 0;
+}
+
int fetch(unsigned char *sha1)
{
char *hex = sha1_to_hex(sha1);
@@ -76,7 +240,7 @@ int fetch(unsigned char *sha1)
local = open(filename, O_WRONLY | O_CREAT | O_EXCL, 0666);
if (local < 0)
- return error("Couldn't open %s\n", filename);
+ return error("Couldn't open local object %s\n", filename);
memset(&stream, 0, sizeof(stream));
@@ -84,6 +248,7 @@ int fetch(unsigned char *sha1)
SHA1_Init(&c);
+ curl_easy_setopt(curl, CURLOPT_FAILONERROR, 1);
curl_easy_setopt(curl, CURLOPT_FILE, NULL);
curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, fwrite_sha1_file);
@@ -99,8 +264,12 @@ int fetch(unsigned char *sha1)
curl_easy_setopt(curl, CURLOPT_URL, url);
- if (curl_easy_perform(curl))
- return error("Couldn't get %s for %s\n", url, hex);
+ if (curl_easy_perform(curl)) {
+ unlink(filename);
+ if (fetch_pack(sha1))
+ return error("Tried %s", url);
+ return 0;
+ }
close(local);
inflateEnd(&stream);
^ permalink raw reply
* [PATCH 0/2] Support pack files in http-pull
From: barkalow @ 2005-07-31 20:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
This series adds support for downloading pack files when appropriate in
http-pull. When it finds that a needed object is not available, it
downloads info/packs (into memory), identifies any pack files it doesn't
have from there, downloads indices of any of these that it doesn't have,
and downloads the pack containing the object. If other packs are also
needed, it downloads them when it reaches them.
-Daniel
*This .sig left intentionally blank*
^ permalink raw reply
* [PATCH 1/3] Fix support for old libcurl
From: barkalow @ 2005-07-31 19:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Old libcurl has curl_easy_setopt(), and http-pull requires it; it just
doesn't have one of the options.
Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---
http-pull.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
9c32b0230180d507b4429fb35432bc404a89e637
diff --git a/http-pull.c b/http-pull.c
--- a/http-pull.c
+++ b/http-pull.c
@@ -12,9 +12,6 @@
#if LIBCURL_VERSION_NUM < 0x070800
#define curl_global_init(a) do { /* nothing */ } while(0)
#endif
-#if LIBCURL_VERSION_NUM < 0x070907
-#define curl_easy_setopt(a, b, c) do { /* nothing */ } while(0)
-#endif
static CURL *curl;
@@ -187,7 +184,9 @@ int main(int argc, char **argv)
curl_ssl_verify = gitenv("GIT_SSL_NO_VERIFY") ? 0 : 1;
curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, curl_ssl_verify);
+#if LIBCURL_VERSION_NUM >= 0x070907
curl_easy_setopt(curl, CURLOPT_NETRC, CURL_NETRC_OPTIONAL);
+#endif
base = url;
^ permalink raw reply
* [PATCH] Added hook in git-receive-pack
From: Josef Weidendorfer @ 2005-07-31 19:17 UTC (permalink / raw)
To: git
Added hook in git-receive-pack
After successful update of a ref,
$GIT_DIR/hooks/update refname old-sha1 new-sha2
is called if present. This allows e.g sending of a mail
with pushed commits on the remote repository.
Documentation update with example hook included.
Signed-off-by: Josef Weidendorfer <Josef.Weidendorfer@gmx.de>
------------------------------------------------
diff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt
--- a/Documentation/git-receive-pack.txt
+++ b/Documentation/git-receive-pack.txt
@@ -20,10 +20,25 @@ This command is usually not invoked dire
The UI for the protocol is on the 'git-send-pack' side, and the
program pair is meant to be used to push updates to remote
repository. For pull operations, see 'git-fetch-pack' and
'git-clone-pack'.
+The command allows for creation and fast forwarding of sha1 refs
+(heads/tags) on the local end. After each successful update, the
+following external hook script is called if it is present:
+
+ $GIT_DIR/hooks/update refname sha1-old sha1-new
+
+It is assured that sha1-old is an ancestor of sha1-new (otherwise,
+the update would have not been allowed). refname is relative to
+$GIT_DIR; e.g. for the master head this is "refs/heads/master".
+Using this hook, it is easy to generate mails on updates to
+the local repository. This example script sends a mail with
+the commits pushed to the repository:
+
+ #!/bin/sh
+ git-rev-list --pretty "$3" "^$2" |
+ mail -r $USER -s "New commits on $1" commit-list@mydomain
OPTIONS
-------
<directory>::
The repository to sync into.
diff --git a/receive-pack.c b/receive-pack.c
--- a/receive-pack.c
+++ b/receive-pack.c
@@ -53,10 +53,53 @@ static int verify_old_ref(const char *na
if (memcmp(buffer, hex_contents, 40))
return -1;
return 0;
}
+static const char *update_hook = "hooks/update";
+
+static void updatehook(const char *name, unsigned char *old_sha1, unsigned char *new_sha1)
+{
+ if (access(update_hook, X_OK) < 0) return;
+ fprintf(stderr, "executing update hook for %s\n", name);
+
+ pid_t pid = fork();
+
+ if (pid < 0)
+ die("hook fork failed");
+ if (!pid) {
+ execlp(update_hook, update_hook, name, old_sha1, new_sha1, NULL);
+ die("hook execute failed");
+ }
+
+ for (;;) {
+ int status, code;
+ int retval = waitpid(pid, &status, 0);
+
+ if (retval < 0) {
+ if (errno == EINTR)
+ continue;
+ die("waitpid failed (%s)", strerror(retval));
+ }
+ if (retval != pid)
+ die("waitpid is confused");
+ if (WIFSIGNALED(status)) {
+ fprintf(stderr, "%s died of signal %d",
+ update_hook, WTERMSIG(status));
+ return;
+ }
+ if (!WIFEXITED(status))
+ die("%s died out of really strange complications",
+ update_hook);
+ code = WEXITSTATUS(status);
+ if (code)
+ fprintf(stderr, "%s exited with error code %d",
+ update_hook, code);
+ return;
+ }
+}
+
static void update(const char *name, unsigned char *old_sha1, unsigned char *new_sha1)
{
char new_hex[60], *old_hex, *lock_name;
int newfd, namelen, written;
@@ -93,10 +136,12 @@ static void update(const char *name, uns
if (rename(lock_name, name) < 0) {
unlink(lock_name);
die("unable to replace %s", name);
}
fprintf(stderr, "%s: %s -> %s\n", name, old_hex, new_hex);
+
+ updatehook(name, old_hex, new_hex);
}
/*
* This gets called after(if) we've successfully
^ permalink raw reply
* Re: [PATCH 1/3] Add git-send-email-script - tool to send emails from git-format-patch-script
From: Junio C Hamano @ 2005-07-31 19:13 UTC (permalink / raw)
To: Ryan Anderson; +Cc: git
In-Reply-To: <7vbr4jmhqe.fsf@assigned-by-dhcp.cox.net>
Oh, another thing. Could you refrain from doing
quoted-printable when possible? Thanks.
^ permalink raw reply
* Re: Shipping gitk as part of core git.
From: Junio C Hamano @ 2005-07-31 18:48 UTC (permalink / raw)
To: Paul Mackerras; +Cc: git
In-Reply-To: <17132.48473.276198.835869@cargo.ozlabs.ibm.com>
Paul Mackerras <paulus@samba.org> writes:
> Yes, I agree. I'm happy to send you an email when I have committed
> changes to gitk if that will help.
Please do not waste your time on that doing it regularly, but do
not hesitate to drop me a note if you have something newsworthy.
I always fetch from (not necessarily merge with) a couple of
repositories before starting my git day, in order to sanity
check the status of my private work repository. Your repository
is among them, so I'll usually notice.
> He did ask me first, and I said he could :). It makes things easier
> for me, having gitk in the core git, because it means that I don't
> have to worry about making a proper package out of it. I don't see
> any reason why gitk would grow to be more than just the script.
Understood. So let's keep the current arrangement.
> I am also thinking of doing a "gitool", somewhat like bk citool, to
> make it easier to create commits. I guess we can decide later whether
> to make it part of the core git, although it seems more like porcelain
> than gitk.
Sounds like it.
-jc
^ permalink raw reply
* Re: Terminology
From: Junio C Hamano @ 2005-07-31 18:33 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <Pine.LNX.4.58.0507311541340.29235@wgmdd8.biozentrum.uni-wuerzburg.de>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Maybe we should decide on a common terminology before kicking out 1.0, and
> look through all files in Documentation/ to have a consistent vocabulary.
> And poor me does not get confused no more.
Glad to see you started the discussion on this one. I have a
slight worry and suspicion that this might open a can of worms,
but I agree we need to get this done. We probably would end up
spliting the Terminology section in Documentation/git.txt into a
separate "Glossary" document.
Care to volunteer drafting a strawman, listing the concepts we
need terms for, marking the ones we seem to use the same word
for? You do not have to suggest which candidate term to use
for all of them. Something along these lines...
- The unit of storage in GIT is called "object"; no other word
is used and the word "object" is used only for this purpose
so this one is OK.
- A 20-byte SHA1 to uniquely identify "objects"; README and
early Linus messages call this "object name" so does
tutorial. Many places say "object SHA1" or just "SHA1".
- An "object database" stores a set of "objects", and an
individial object can be retrieved by giving it its object
name.
- Storing a regular file or a symlink in the object database
results in a "blob object" created. You cannot directly
store filesystem directory, but a collection of blob objects
and other tree objects can be recorded as a "tree object"
which corresponds to this notion.
- $GIT_INDEX_FILE is "index file", which is a collection of
"cache entries". The former is sometimes called "cache
file", the latter just "cache".
- the directory which corresponds to the top of the hierarchy
described in the index file; I've seen words like "working
tree", "working directory", "work tree" used.
- When the stat information a cache entry records matches what
is in the work tree, the entry is called "clean" or
"up-to-date". The opposite is "dirty" or "not up-to-date".
- An index file can be in "merged" or "unmerged" state. The
former is when it does not have anything but stage 0 entries,
the latter otherwise.
- An merged index file can be written as a "tree object", which
is technically a set of interconnected tree objects but we
equate it with the toplevel tree object with this set.
- A "tree object" can be recorded as a part of a "commit
object". The tree object is said to be "associated with" the
commit object.
- A "tag object" can be recorded as a pointer to another object
of any type. The act of following the pointer a tag object
holds (this can go recursively) until we get to a non-tag
object is sometimes called "resolving the tag".
- The following objects are collectively called "tree-ish": a
tree object, a commit object, a tag object that resolves to
either a commit or a tree object, and can be given to
commands that expect to work on a tree object.
- The files under $GIT_DIR/refs record object names, and are
called "refs". What is under refs/heads/ are called "heads",
refs/tags/ "tags". Typically, they are either object names
of commit objects or tag objects that resolve to commit
objects, but a tag can point at any object.
- A "head" is always an object name of a commit, and marks the
latest commit in one line of development. A line of
development is often called a "branch". We sometimes use the
word "branch head" to stress the fact that we are talking
about a single commit that is the latest one in a "branch".
- Combining the states from more than one lines of developments
is called "merging" and typically done between two branch
heads. This is called "resolving" in the tutorial and there
is git-resolve-script command for it.
- A set of "refs" with the set of objects reachable from them
constitute a "repository". Although currently there is no
provision for a repository to say that its objects are stored
in this and that object database, multiple repositories can
share the same object database, and there is not a conceptual
limit that a repository must retrive its objects from a
single object database.
- The act of finding out the object names recorded in "refs" a
different repository records, optionally updating a local
"refs" with their values, and retrieving the objects
reachable from them is called "fetching". Fetching immediately
followed by merging is called "pulling".
- The act of updating "refs" in a different repository with new
value and populating the object database(s) associated with
the repository is called "pushing".
- Currently refs/heads records branch heads of both locally
created branches and branches fetched from other
repositories.
- Currently, fetching always happen against a single branch
head on a remote repository, and (a remote repository, name
of the branch) is stored in $GIT_DIR/branches/ as a
short-hand mechanism. A file in this directory identifies
a remote repository by its URL, and the branch to fetch/pull
from is identified with the URL fragment notation, absense of
which makes it default to "master".
-jc
^ permalink raw reply
* Terminology
From: Johannes Schindelin @ 2005-07-31 13:52 UTC (permalink / raw)
To: git
Hi,
the other day I got confused by the terminology. Maybe I'm not the only
one:
The GIT equivalent of a CVS branch is sometimes called a branch
(git-new-branch), sometimes a tree (git-switch-tree), and sometimes a
head (which seems counterintuitive to CVS people: they only have one
HEAD; pun(s) intended).
What is worse: a tree often refers to something different, namely a
directory structure corresponding to a certain commit (which SVN people
would call revision). And in $GIT_DIR/branches, short cuts for remote
addresses are stored (and therefore I would have preferred
$GIT_DIR/remotes).
Maybe we should decide on a common terminology before kicking out 1.0, and
look through all files in Documentation/ to have a consistent vocabulary.
And poor me does not get confused no more.
Ciao,
Dscho
--
Git-R-Done
^ permalink raw reply
* Re: Shipping gitk as part of core git.
From: Paul Mackerras @ 2005-07-31 12:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vk6j8yo0g.fsf_-_@assigned-by-dhcp.cox.net>
Junio C Hamano writes:
> It appears that gitk gets wider test coverage only after it is
> pulled into git.git repository. I think it would be a good idea
> for me to pull from you often.
Yes, I agree. I'm happy to send you an email when I have committed
changes to gitk if that will help.
> Recently there was a discussion with binary packaging folks.
> While I do not mind, and actually I would prefer, shipping gitk
> as part of the core GIT, I have never heard about your
> preference. As long as gitk is just a single file (or even a
> handful files in the future) project that does not have a
> filename that overlaps with core GIT, I can continue pulling
> from you and I think the binary packaging folks can produce
> separate git-core and gitk package out of git.git tree without
> problems. However, once you start wanting to have your own
> Makefile and maybe debian/rules file for packaging, for example,
> I suspect the way currently things are set up would break
> miserably. It's all Linus' fault to have merged with your tree
> in the first place ;-).
He did ask me first, and I said he could :). It makes things easier
for me, having gitk in the core git, because it means that I don't
have to worry about making a proper package out of it. I don't see
any reason why gitk would grow to be more than just the script.
I am also thinking of doing a "gitool", somewhat like bk citool, to
make it easier to create commits. I guess we can decide later whether
to make it part of the core git, although it seems more like porcelain
than gitk.
> Anyhow, I have one bug to report. I selected one rev, and then
> said "diff this -> selected" from right-click menu on an
> adjacent one, and I got this:
Thanks for the patch. I have committed that fix plus fixes for some
other bugs that people have reported, and pushed it to
master.kernel.org. Could you do another pull please?
Regards,
Paul.
^ permalink raw reply
* Re: [PATCH 1/3] Add git-send-email-script - tool to send emails from git-format-patch-script
From: Sergey Vlasov @ 2005-07-31 10:50 UTC (permalink / raw)
To: Ryan Anderson; +Cc: junkio, git
In-Reply-To: <11227978451100@foobar.com>
[-- Attachment #1: Type: text/plain, Size: 1100 bytes --]
On Sun, 31 Jul 2005 04:17:25 -0400 Ryan Anderson wrote:
> This is based off of GregKH's script, send-lots-of-email.pl, and
> strives to do all the nice things a good subsystem maintainer does
> when forwarding a patch or 50 upstream:
>
> All the prior handlers of the patch, as determined by the
> Signed-off-by: lines, and/or the author of the commit, are cc:ed
> on the email.
> All emails are sent as a reply to the previous email, making it
> easy to skip a collection of emails that are uninteresting.
Actually, this is the part of GregKH's script which I hate ;)
50 patches sent this way produce an enormous email thread; if someone
then tries to comment on a 40th patch, this part of the thread ends up
far behind the right edge of the message list window in almost any mail
client (and if someone comments on the first patch, the comments are far
from that patch).
Sending a [PATCH 0/N] message with the overall description of the
patchset and then all patches as replies to that message looks much
better in that respect. However, missing messages in this form are less
obvious.
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: [PATCH 1/3] Add git-send-email-script - tool to send emails from git-format-patch-script
From: Johannes Schindelin @ 2005-07-31 10:25 UTC (permalink / raw)
To: Ryan Anderson; +Cc: junkio, git
In-Reply-To: <11227978451100@foobar.com>
Hi,
wouldn't it be a good idea to make $from and $to required parameters? At
least you could infer a sensible default of $from from GIT_* environment
variables, no? I am not quite comfortable with a hard coded sender in a
script possibly deployed into a multi-user environment.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 1/3] Add git-send-email-script - tool to send emails from git-format-patch-script
From: Junio C Hamano @ 2005-07-31 9:45 UTC (permalink / raw)
To: Ryan Anderson; +Cc: git
In-Reply-To: <11227978451100@foobar.com>
Ryan Anderson <ryan@michonline.com> writes:
> All emails are sent as a reply to the previous email, making it easy to
> skip a collection of emails that are uninteresting.
I understand why _some_ people consider this preferable, but
wonder if this should have a knob to be tweaked.
For example, I myself often find it very hard to read when
a cascading thread goes very deep like this:
[PATCH 0/9] cover
[PATCH 1/9] first one
[PATCH 2/9] second one
[PATCH 3/9] third one in the series
...
and prefer to see this instead (this assumes your MUA is
half-way decent and lets you sort by subject):
[PATCH 0/9] cover
[PATCH 1/9] first one
[PATCH 2/9] second one
[PATCH 3/9] third one in the series
...
> +# horrible hack of a script to send off a large number of email messages, one after
> +# each other, all chained together. This is useful for large numbers of patches.
> +#
> +# Use at your own risk!!!!
Well, if it is "Use at your own risk" maybe it should stay
outside the official distribution for a while until it gets
safer ;-).
> + my @fields = split /\s+/, $data;
> + my $ident = join(" ", @fields[0...(@fields-3)]);
Wouldn't "s/>.*/>/" be easier than splitting and joining?
> +if (!defined $from) {
> + $from = $author || $committer;
> + 1 while (!defined ($_ = $term->readline("Who should the emails appear to be from? ",
> + $from)));
Judging from your past patches, you seem to really like
statement modifiers[*]. While they _are_ valid Perl constructs,
it is extremely hard to read when used outside very simple
idiomatic use. Please consider rewriting the above and the like
using compound statements[*] (I am using these terms according
to the definition in perlsyn.pod). Remember, there are people
Perl is not their native language, but are intelligent enough to
be of great help fixing problems in programs you write in Perl.
To most of them, compound statements are more familiar, so try
to be gentle to them.
> + opendir(DH,$f)
> + or die "Failed to opendir $f: $!";
> + push @files, map { +$f . "/" . $_ } grep !/^\.{1,2}$/,
> + sort readdir(DH);
Maybe skip potential subdirs while you are at it, something like this?
push @files, sort grep { -f $_ } map { "$f/$_" } readdir(DH)
> + my $pseudo_rand = int (rand(4200));
> + $message_id = "<$date$pseudo_rand\@foobar.com>";
> + print "new message id = $message_id\n";
I doubt this hardcoded foobar.com is a good idea. Did you mean
to print it, by the way?
> + $to{lc(Email::Valid->address($_))}++ for (@to);
> + my $to = join(",", keys %to);
Is this the culprit that produced this mechanical-looking line?
To: junkio@cox.net,git@vger.kernel.org
Interestingly enough, you do not seem to do it for the From:
line.
From: Ryan Anderson <ryan@michonline.com>
Also you seem to be losing the ordering in @to and @cc by the
use of uniquefying "keys %to" and "keys %cc". I can not offhand
tell if it matters, but you probably would care, at least for
the primary recipients listed in @to array.
> + $mail{smtp} = 'localhost';
I suspect this probably need to be configurable. I may be a
minority, but my outgoing messages are directly handed to my
local ISP smtp server from my MUA, and the smtp server running
on the locahost does not talk to the outside world.
> + # set up for the next message
> + $reply_to = $message_id;
Making chaining policy configurable would be just one liner
change here, I suppose.
Since there are always 47 different ways to do the same thing in
Perl, Perl style varies a lot more than Shell style which in
turn varies a lot more than C. You should be prepared to be
nitpicked a lot when you post Perl ;-). And I was in nitpicking
mood tonight.
Thanks for the patch. Overall, very good intent. Slightly
troublesome details.
-jc
^ permalink raw reply
* Re: [PATCH 1/3] Add git-send-email-script - tool to send emails from git-format-patch-script
From: Matthias Urlichs @ 2005-07-31 9:36 UTC (permalink / raw)
To: git
In-Reply-To: <20050731082430.GG32263@mythryan2.michonline.com>
Hi, Ryan Anderson wrote:
> And yes, I did generate this thread with this script - so I have proof
> that it works nicely.
It might make sense to create a "Patch 0/N" with a short explanation, and
have the actual patches be replies to that -- or to patch 1/N if that's
not necessary.
As it is, patch N hangs off patch N-1 in my email threading view, which
gets slightly cumbersome if N>10.
--
Matthias Urlichs | {M:U} IT Design @ m-u-it.de | smurf@smurf.noris.de
Disclaimer: The quote was selected randomly. Really. | http://smurf.noris.de
- -
Nothing makes a person more productive than the last minute.
^ 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