Pull Requests via Git Push | Hacker News Reader

Pull Requests via Git Push

blog.sesse.net

16 pointsstargrave8 comments

Nice! A couple of thoughts re. your "It's not perfect" which wouldn't add real extra complexity:

Using --from with git-format-patch will add the From: header to the commit message body in the standard way when emailing a patch written by someone else, which would allow you to preserve this easily without risking excitement from injection attacks into email headers.

Maybe the right behaviour for all-zero $oldsha is to quietly turn format-patch $oldsha..$newsha into format-patch --root $newsha?

By default, it seems git-send-email adds the in-body From: with the committer. (I don't know if git am picks this up, but I suppose it does.) If I add --from _after_ $oldsha..$newsha, it seems that's the git-format-patch --from, which _also_ adds something based on the headers sent by git-http-backend, so I get two From: in the body, where the first is:

From: anonymous <anonymous@http.2001:<…my IP address here…>>

I guess what I really want is Reply-To: being set based on the commit message, but I don't know if git can do that easily.

Ah interesting. I use git-format-patch instead of git-send-email myself, so I can re-read the resulting mbox before sending it. I always thought of git-send-email as a wrapper that just passed most options straight through to format-patch, but sounds like it does something more with --from.

I haven't read builtin/log.c (edit: and pretty.c) to check how risky the From: field actually is. It might be that it is guaranteed to be properly/safely quoted by git - but it might not be too! When I embedded a " in the author name by hand, the From: line was correctly quoted as

  From: "first\"last" <email@email>

but that's no guarantee that it's done for every malicious author/committer without inspecting the code.

    echo 'The push will now exit with an error. No commits have actually been pushed.'

Is this part so that the temporary git objects are cleaned up? Or is there some other reason that it has to exit with error?

It is so that the push doesn't actually become a push. :-) If the pre-receive hook exits with success, the push goes through and random people can write whatever they want to my repository. If it fails, the repository is left exactly as it was (including, as you point out, the quarantined temporary objects going away).

One could in theory use git namespaces to keep the new branches but not make them overwrite existing ones, but then people could store porn or whatever for free on your server and have it be served to others.

(I wrote the blog post)

Neat! Thanks for explaining :)

> keep the new branches but not make them overwrite existing ones, but then people could store porn or whatever for free on your server and have it be served to others

When your program sends the mail, do you use any external services for that (GMail, etc) or do you run a local SMTP server? How about the receiving address, is that one self-hosted or powered by a third-party?

I deliver and read mail locally. I don't think that matters a whole lot in this case, though? As long as you can send email from your server to yourself somehow, I guess you're fine.

Mainly thinking because if you use a third-party you could potentially trigger some spam metric if someone were to submit PRs full of spam, and thereby getting your domain landed on some blacklist which in turn would make more of the real mail you yourself send at risk of getting trapped in spam.

So using a local-only / fully self-hosted mail delivery solution is definitely the best way to do it IMO.

Reply on news.ycombinator.com