Opened 9 years ago

Closed 9 years ago

#2374 closed defect (fixed)

or-cvs list doesn't send mail for merges or tags

Reported by: cypherpunks Owned by:
Priority: Medium Milestone:
Component: Archived/Development Progress Version:
Severity: Keywords:
Cc: weasel, Sebastian Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description


Child Tickets

Change History (27)

comment:1 Changed 9 years ago by arma

Summary: Fix or close faked or-cvs mailist.or-cvs list missing some git commits

What he means here is that

commit c9f8a5eebcfbda9d88148e031b43c064f31f20a3
Merge: f12b253... aa45e82...
Author: Nick Mathewson <nickm@torproject.org>
Date:   Mon Jan 10 17:31:11 2011 -0500

    Merge remote branch 'origin/maint-0.2.2'
    
    Conflicts:
        src/or/buffers.c

never got sent to or-cvs.

Why didn't it?

comment:2 Changed 9 years ago by erinn

This seems to happen with merges in general, though we should check more thoroughly. There are several commits of mine missing from or-cvs as well, all of them merges.

comment:3 Changed 9 years ago by nickm

Priority: blockernormal
Summary: or-cvs list missing some git commitsor-cvs list doesn't send mail for merges or tags

Yup; the commit script doesn't send mail for merge commits.

Is our commit script under version control somewhere? If not, getting it into a repository would be a good first step.

comment:4 Changed 9 years ago by nickm

It looks like our script is based on the one in githax, but with a bunch of other stuff added too. I'm not merging it back to githax as-is, since it's scarily monolithic, but you can see it in sebastian's "update_hooks" githax branch.

The relevant incantation is:

               git-format-patch --subject-prefix="$projectname/${refname#refs/heads/}" \
                            --stdout "$oldrev..$newrev" |
                            formail -s /srv/git.torproject.org/git-helpers/copy
headers.pl | formail -I "To: $recipients" -R From: Patch-Author: -I "From: $GL_USER@torproject.org" -I Date: -s /usr/sbin/sendmail -t

FWICT, there doesn't seem to be a way to make git format-patch do with we want. We are probably going to need to do some chicanery with post-processing the results of git log -p, probably with a perl script.

Another question is: which git log -p should we use? By default, git log -p says nothing about merge bodies. Our choices are "git log -p -m", "git log -p -c", and "git log -p --cc". Please see the git log manpage for a description of each, and play with them a little to see how they work for you. Note also that I think the -c and --cc variants do not generate message bodies for sufficiently conflict-free merges: we could safely omit mail for such merges, as it has nothing to add.

I'm going to hack up a quick and dirty postprocessing script, to see if I can.

comment:5 Changed 9 years ago by nickm

I've added a couple of perl scripts to my better_log_script branch in my public githax dir.

comment:6 Changed 9 years ago by nickm

Cc: weasel Sebastian added
Status: newneeds_review

comment:7 Changed 9 years ago by Sebastian

I think -m is not what we want - it would mean huge diffs for large branches (that might not make it to or-cvs because they are huge), and more importantly it actually hides when there *is* a change (unless you scan the big diff carefully). I think going with the --cc option is what we want here, because it seems to give the best info imo.

I'm not sure we'd want to omit empty merges tho; seems like some people might care that the commit added to a branch was not a fast forward and rather a merge. (I don't have a preference either way)

Small typo I noticed: the script is called logs_to_emails.pl, but in the suggested usage section it is log_to_emails.pl

Other then that, this looks ok to me. I didn't test actually emailing using formail, but the rest looks good (tested all options (-m, -c, --cc), empty merges, merges with conflicts, merges of merges with conflicts, etc).

comment:8 Changed 9 years ago by weasel

Sebastian wants me to say that I don't have any preferences here. I defer to the people who actually use the or-cvs list.

comment:9 Changed 9 years ago by nickm

Right; -m is pretty silly. Between -c and -cc, the description makes it sounds like "-c" provides strictly *more* information but "--cc" provides more focus.

If it's down to me to pick, I suggest that we start with "-c" [*], and that we do not yet remove empty merges. This is as verbose as we can go and I figure that it will be easier for us to say "No, we don't need to know about that information, let's take it out" if we start with too much verbosity than it will be for us to notice that we _do_ want more info if we start with too little.

So let's go for it.

[*] Specifically, with "--reverse -p --stat -c"

comment:10 Changed 9 years ago by Sebastian

Sounds good to me, I suggested --cc because I liked the terser form, but the argument for a -c to --cc transition being easier is definitely a good one.

comment:11 Changed 9 years ago by Sebastian

Resolution: fixed
Status: needs_reviewclosed

I believe this is fixed now. Closing the bug for now; please reopen if you notice anything wrong/missing.

comment:12 Changed 9 years ago by rransom

Resolution: fixed
Status: closedreopened

Now it's sending all commits pushed at one time in a single message.

comment:13 Changed 9 years ago by nickm

Ugh. Okay, what exactly does the relevant part of the post-commit script say now vs what it used to say?

comment:14 Changed 9 years ago by Sebastian

                git log --reverse -p --stat -c "$oldrev..$newrev" |
                /srv/git.torproject.org/git-helpers/logs_to_emails.pl "$projectname/${refname#refs/heads/}" |
                formail -I "To: $recipients" \
                  -I "From: $GL_USER@torproject.org" \
                  -s /usr/sbin/sendmail -t

comment:15 Changed 9 years ago by Sebastian

Maybe the issue is a missing newline (there's only one currently) between two commits.

comment:16 Changed 9 years ago by nickm

If so, that's a trivial fix: let's try it! I've commited a new version of the script. Try checking it out and let me know how it looks?

comment:17 Changed 9 years ago by nickm

(Also, please let me know once you've replaced the live version of the script so I know when to expect that maybe stuff will get better.)

comment:18 Changed 9 years ago by Sebastian

Updated cupani

comment:19 Changed 9 years ago by nickm

looks like that didn't work.

comment:20 Changed 9 years ago by arma

I'd vote for --cc rather than -c. nickm's 500KB commit mail is not something I can read.

comment:21 Changed 9 years ago by nickm

okay, pushed another to githax. sebastian has deployed it.

comment:22 Changed 9 years ago by Sebastian

Resolution: fixed
Status: reopenedclosed

Fixed. We should re-evaluate the -c vs --cc choice in a while.

comment:23 Changed 9 years ago by arma

Resolution: fixed
Status: closedreopened

comment:24 Changed 9 years ago by arma

Can I vote again for --cc here?

Does anybody actually try to read the mails that use -c?

Try reading it on http://archives.seul.org/tor/cvs/Feb-2011/msg00435.html
and tell me you would rather stick with that than --cc.

comment:25 Changed 9 years ago by nickm

I am fine with --cc.

comment:26 Changed 9 years ago by arma

Sebastian, it looks like this one is up to you?

Weasel doesn't know what parts to mess with.

comment:27 Changed 9 years ago by Sebastian

Resolution: fixed
Status: reopenedclosed

done

Note: See TracTickets for help on using tickets.