* Re: [PATCH v2] ident: check /etc/mailname if email is unknown
From: Ian Jackson @ 2011-10-03 11:32 UTC (permalink / raw)
To: Johannes Sixt
Cc: Jonathan Nieder, Junio C Hamano, git, Matt Kraai, Gerrit Pape,
Linus Torvalds
In-Reply-To: <4E895FBD.8020904@viscovery.net>
Johannes Sixt writes ("Re: [PATCH v2] ident: check /etc/mailname if email is unknown"):
> Am 10/3/2011 8:16, schrieb Jonathan Nieder:
> > +static int add_mailname_host(char *buf, size_t len)
> > +{
> > + FILE *mailname;
> > +
> > + mailname = fopen("/etc/mailname", "r");
> > + if (!mailname) {
> > + if (errno != ENOENT)
> > + warning("cannot open /etc/mailname: %s",
> > + strerror(errno));
>
> This warns on EACCES. Is that OK? (Just asking, I have no opinion.)
I think that's correct. Personally I'm a bit of an error handling
fascist and I would have it crash on EACCES but that's probably a bit
harsh.
Certainly this file ought to be generally readable, if it exists.
Ian.
^ permalink raw reply
* Re: [PATCH] Support ERR in remote archive like in fetch/push
From: René Scharfe @ 2011-10-03 11:45 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Ilari Liusvaara, Jeff King, Nguyễn Thái Ngọc Duy,
git
In-Reply-To: <20111003112649.GA12874@elie>
Am 03.10.2011 13:26, schrieb Jonathan Nieder:
> Ilari Liusvaara wrote:
>
>> Oh, and adding interpretation of ERR packets to git archive is easy
>> (and I even happen to have git:// server that can send those to
>> test against):
>>
>> $ git archive --remote=git://localhost/foobar HEAD
>> fatal: remote error: R access for foobar DENIED to anonymous
>>
>> (I also tested that remote snapshotting of repository that should be
>> readable succeeds, it does).
>
> Sounds like a good idea to me. Let's see what René thinks; also
> changing the subject line to attract other reviewers.
Looks good to me, but I'm not too familiar with the remote protocol.
>> --- >8 ----
>> From: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
>> Date: Mon, 3 Oct 2011 13:55:37 +0300
>> Subject: [PATCH] Support ERR in remote archive like in fetch/push
>>
>> Make ERR as first packet of remote snapshot reply work like it does in
>> fetch/push. Lets servers decline remote snapshot with message the same
>> way as declining fetch/push with a message.
>>
>> Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
>> ---
>> builtin/archive.c | 2 ++
>> 1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/builtin/archive.c b/builtin/archive.c
>> index 883c009..931956d 100644
>> --- a/builtin/archive.c
>> +++ b/builtin/archive.c
>> @@ -61,6 +61,8 @@ static int run_remote_archiver(int argc, const char **argv,
>> if (strcmp(buf, "ACK")) {
>> if (len > 5 && !prefixcmp(buf, "NACK "))
>> die(_("git archive: NACK %s"), buf + 5);
>> + if (len > 4 && !prefixcmp(buf, "ERR "))
>> + die(_("remote error: %s"), buf + 4);
>> die(_("git archive: protocol error"));
>> }
>>
>> --
>> 1.7.7.3.g2791de.dirty
>>
^ permalink raw reply
* [PATCH] Support ERR in remote archive like in fetch/push
From: Jonathan Nieder @ 2011-10-03 11:26 UTC (permalink / raw)
To: Ilari Liusvaara
Cc: Jeff King, Nguyễn Thái Ngọc Duy, git,
René Scharfe
In-Reply-To: <20111003110159.GA13064@LK-Perkele-VI.localdomain>
Ilari Liusvaara wrote:
> Oh, and adding interpretation of ERR packets to git archive is easy
> (and I even happen to have git:// server that can send those to
> test against):
>
> $ git archive --remote=git://localhost/foobar HEAD
> fatal: remote error: R access for foobar DENIED to anonymous
>
> (I also tested that remote snapshotting of repository that should be
> readable succeeds, it does).
Sounds like a good idea to me. Let's see what René thinks; also
changing the subject line to attract other reviewers.
> --- >8 ----
> From: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
> Date: Mon, 3 Oct 2011 13:55:37 +0300
> Subject: [PATCH] Support ERR in remote archive like in fetch/push
>
> Make ERR as first packet of remote snapshot reply work like it does in
> fetch/push. Lets servers decline remote snapshot with message the same
> way as declining fetch/push with a message.
>
> Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
> ---
> builtin/archive.c | 2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/builtin/archive.c b/builtin/archive.c
> index 883c009..931956d 100644
> --- a/builtin/archive.c
> +++ b/builtin/archive.c
> @@ -61,6 +61,8 @@ static int run_remote_archiver(int argc, const char **argv,
> if (strcmp(buf, "ACK")) {
> if (len > 5 && !prefixcmp(buf, "NACK "))
> die(_("git archive: NACK %s"), buf + 5);
> + if (len > 4 && !prefixcmp(buf, "ERR "))
> + die(_("remote error: %s"), buf + 4);
> die(_("git archive: protocol error"));
> }
>
> --
> 1.7.7.3.g2791de.dirty
>
^ permalink raw reply
* Re: [PATCH] transport: do not allow to push over git:// protocol
From: Jonathan Nieder @ 2011-10-03 11:13 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Jeff King, Johannes Sixt, git
In-Reply-To: <CACsJy8B7Z-fT+ED=4F-Ug-bhvCagSxr0X6vZqn5PGRfB7KnUTA@mail.gmail.com>
Nguyen Thai Ngoc Duy wrote:
> To me, just "<service>: access denied" is enough. Not particularly
> friendly but should be a good enough clue.
Yes, I think you're right. It also has the benefit of being easily
parsable, so some day the client might learn to give a friendly
message in the operator's chosen language.
^ permalink raw reply
* Re: [PATCH] transport: do not allow to push over git:// protocol
From: Ilari Liusvaara @ 2011-10-03 11:01 UTC (permalink / raw)
To: Jeff King; +Cc: Nguyễn Thái Ngọc Duy, git
In-Reply-To: <20111003074250.GB9455@sigill.intra.peff.net>
On Mon, Oct 03, 2011 at 03:42:51AM -0400, Jeff King wrote:
> On Sat, Oct 01, 2011 at 11:26:55AM +1000, Nguyen Thai Ngoc Duy wrote:
>
> The real problem here seems to be that instead of communicating "no, we
> don't support that", git-daemon just hangs up. It would be a much nicer
> fix if we could change that. I'm not sure it's possible, though. There's
> not much room in the beginning of the room to make that communication in
> a way that's backwards compatible.
Oh, sure it is possible (except for remote snapshot):
$ /usr/bin/git fetch git://localhost/foobar
fatal: remote error: R access for foobar DENIED to anonymous
$ /usr/bin/git push git://localhost/foobar
fatal: remote error: W access for foobar DENIED to anonymous
$ /usr/bin/git archive --remote=git://localhost/foobar HEAD
fatal: git archive: protocol error
$ /usr/bin/git --version
git version 1.7.6.3
Supported for fetch and push since 1.6.1-rc1 (And 1.6.1 was over
2.5 years ago). Oh, and even before that, but with slightly more
ugly error message.
Oh, and adding interpretation of ERR packets to git archive is easy
(and I even happen to have git:// server that can send those to
test against):
$ git archive --remote=git://localhost/foobar HEAD
fatal: remote error: R access for foobar DENIED to anonymous
(I also tested that remote snapshotting of repository that should be
readable succeeds, it does).
--- >8 ----
From ce3a402e4fa72cf603f92801d6f021ff89d3ac35 Mon Sep 17 00:00:00 2001
From: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
Date: Mon, 3 Oct 2011 13:55:37 +0300
Subject: [PATCH] Support ERR in remote archive like in fetch/push
Make ERR as first packet of remote snapshot reply work like it does in
fetch/push. Lets servers decline remote snapshot with message the same
way as declining fetch/push with a message.
Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
builtin/archive.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/builtin/archive.c b/builtin/archive.c
index 883c009..931956d 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -61,6 +61,8 @@ static int run_remote_archiver(int argc, const char **argv,
if (strcmp(buf, "ACK")) {
if (len > 5 && !prefixcmp(buf, "NACK "))
die(_("git archive: NACK %s"), buf + 5);
+ if (len > 4 && !prefixcmp(buf, "ERR "))
+ die(_("remote error: %s"), buf + 4);
die(_("git archive: protocol error"));
}
--
1.7.7.3.g2791de.dirty
-Ilari
^ permalink raw reply related
* Re: [PATCH] contrib: add a pair of credential helpers for Mac OS X's keychain
From: Jeff King @ 2011-10-03 10:59 UTC (permalink / raw)
To: Jay Soffian; +Cc: git, Junio C Hamano, John Szakmeister
In-Reply-To: <CAG+J_Dww1yOeq1LHQYMiObPKqrWbk4t8Hn=G9WpYWXFBbHiuhQ@mail.gmail.com>
On Fri, Sep 30, 2011 at 06:42:22PM -0400, Jay Soffian wrote:
> Usually it won't. In the default case, the keychain is unlocked and no
> permission is needed to add an entry, nor to retrieve that entry by
> the application which added it. The prompt will only occur if the
> credential helper is not on the entry's ACL, or if the keychain is
> locked.
Yeah. I was thinking the ACL prompt would come up more often, but I
guess most people would hit "allow always", since it would get annoying
pretty quickly otherwise (I didn't, because I was testing).
Side note: do you know how to edit those ACLs? I couldn't find it in the
keychain manager. It would be helpful for testing to be able to tweak it
(as a workaround, I just modified the binary, which apparently the
keychain code cares about).
> > Another example: if you're running gpg-agent, and you run "git tag -s",
> > you'll be prompted for your key passphrase in an out-of-band dialog.
> >
> > Maybe it doesn't make sense for the actual username/password, though.
>
> Personally, it made sense to me do it at the CLI (obviously). But the
> source code in in contrib now. :-)
Let's leave it that way, then. It's open source, after all. If somebody
really wants a dialog, they can add it. They can even make it optional
(you can do "git config credential.helper 'osxkeychain --dialog'" if the
thing actually takes options).
> >> I found it ugly that git's native getpass doesn't echo the username
> >> back, and it seems hackish to me for the credential helper to turn
> >> back around and invoke it in any case. :-(
> >
> > Yes, but I think that's a bug that should be fixed in git. :)
>
> Yes it should. :-)
I think the only thing holding this back is portability. But it's awful
to make reasonable platforms suffer because it can't be done portably.
We should at least do the right thing when we can, and fall back to
using getpass() otherwise. I'll see what I can do.
> I think that actually makes more sense. There's already an existing
> mechanism to customized (2) via GIT_ASKPASS, right? So it overlaps for
> the credential helper to do that doesn't it?
Sort of. The askpass interface was really invented for asking for ssh
passphrases. So:
1. It can only ask for one thing at a time. So you get two dialogs,
not one, and there's no place for anything else, like a "remember
this password" checkbox.
2. Like the getpass() hack in git, it won't show you what you've typed
so far for the username (there are several implementations, so it's
possible one could have an extra feature for this, but the ones
I've seen just always show some opaque image for each character
typed).
3. There's no way to pre-fill the username field, and let the user
override. So you end up just taking whatever username we already
have, ask for the password, and then try the authentication. The
user's only recourse is to restart the command with a different URL
(with the alternative username in the URL).
> Okay, the more I think about this, the more I think the existing
> design is both too much (asking the credential helper to do anything
> other than store/retrieve passwords) and too little (not breaking out
> the fields distinctly).
I remember in some initial research that there may have been some system
which really wanted to do the prompting itself. But now I can't find any
reference to it. I think it may have been the freedesktop Secrets API,
but now that I read their documentation again, I think I may simply have
been mistaking the prompt for unlocking the secure storage itself, not
the actual username and password.
> > That's not an unreasonable attitude. I mostly let the browser store
> > passwords, but sometimes override it for specific sites. But in this
> > case, I think it would be more per-repo. And you can turn off the helper
> > for a particular repo (actually, I'm not sure you can, but you probably
> > should be able to).
>
> If the credential helper becomes no more than a store/retrieve, it's
> git that would do the prompting "Store credentials via
> git-osxkeychain?" after logging in successfully with the credentials.
Maybe. I had assumed we would hand it off to the helper, and it would
make the decision (possibly after consulting with the user). But I
suppose git could do it itself.
-Peff
^ permalink raw reply
* Re: [RFC/PATCH]: reverse bisect v 2.0
From: Jeff King @ 2011-10-03 10:41 UTC (permalink / raw)
To: Junio C Hamano
Cc: Michal Vyskocil, git, Sverre Rabbelier, Johannes Sixt,
Christian Couder
In-Reply-To: <7v62k9q4oq.fsf@alter.siamese.dyndns.org>
On Fri, Sep 30, 2011 at 11:13:25AM -0700, Junio C Hamano wrote:
> I wonder if something along the following line would make the usage more
> pleasant and self-explanatory:
>
> $ git bisect start --used-to='git frotz says xyzzy' v0.99 master
> Bisecting: 171 revisions left to test after this (roughly 8 steps)
> $ ... build and then test ...
> $ git bisect tested
> You are trying to check: git frotz says xyzzy
> Does the result satisify what you are trying to find [y/n]? yes
I like this idea a lot. My "yes/no" thing was a "if I were designing
bisect from scratch today..." suggestion, but having something like
--used-to makes it a natural addition to the regular good/bad interface.
And I really like the prompt to help people remember what it is they're
declaring each time.
However, --used-to feels a bit backwards to me. I think of it as
"--has-property" or something similar. That is, you are looking for when
something appeared (be it a bug, a feature, or whatever). But I guess it
depends on what you are bisecting. In my case, "yes" would be the
current "bad", and "no" would be the current "good".
So maybe provide both --used-to and --has-property (which I really
dislike as a name, but I can't think of anything better at the moment).
And then we can interpret "yes" and "no" accordingly, depending which
one the user used (and while that _sounds_ confusing, it won't be to the
user; we'll be prompting them with "you're looking for ...", so their
answer will be very natural).
But with both of them, the user is free to phrase it in whatever way
feels natural.
> When trying to find regression, you would say:
>
> $ git bisect start --used-to='it works' v0.99 master
Just for completeness, under my proposal this could also be:
$ git bisect start --has-property='git frotz is broken' v0.99 master
Which is probably slightly less natural. But:
> When trying to find a fix, you would say:
>
> $ git bisect start --used-to='checkout $tree $path clobbers $path' v0.99 master
This one would be a bit nicer:
$ git bisect start --has-property='checkout works' v0.99 master
-Peff
PS Side note: do we really need it to be interactive? I would think
you could do:
$ git bisect start --used-to='git frotz says xyzzy'
Bisecting: 171 revisions left to test after this (roughly 8 steps)
Checking whether 'git frotz says xyzzy'.
$ ... build and test ...
$ git bisect yes
Bisecting: 89 revisions left to test after this (roughly 7 steps)
Checking whether 'git frotz says xyzzy'.
It means the user is reminded, then tests, then responds. But I think
it would be enough of a reminder. The important thing is that we keep
mentioning it at each bisection step. Maybe it depends on how long
your build/test step is (I tend to work on things like git, where
that is a 30-second procedure, not a kernel with a long build and a
reboot in between :) ).
^ permalink raw reply
* Re: [RFC/PATCH] git checkout $tree path
From: Jeff King @ 2011-10-03 10:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vk48rq854.fsf@alter.siamese.dyndns.org>
On Thu, Sep 29, 2011 at 03:46:31PM -0700, Junio C Hamano wrote:
> According to that definition, because "master" has dir/file1, and the
> index is unchanged since "next", we would add dir/file1 to the index, and
> then check dir/file1 and dir/file3 out of the index. Hence, we end up
> resurrecting dir/file3 out of the index, even though "master" does not
> have that path.
>
> This is somewhat surprising.
Agreed, it is surprising.
> It may make sense to tweak the semantics a little bit. We can grab the
> paths out of the named tree ("master" in this case), update the index, and
> update the working tree with only with these paths we grabbed from the
> named tree. By doing so, we will keep the local modification to dir/file3
> (in this case, the modification is to "delete", but the above observation
> hold equally true if dir/file3 were modified).
Hmm. I can see that being what the user expects in some cases. For
example, when "master" has nothing to do with dir/file3 in the first
place. But I can also see this:
> An alternative semantics could be to first remove paths that match the
> given pathspec from the index, then update the index with paths taken from
> the named tree, and update the working tree. "git checkout master dir"
> would then mean "replace anything currently in dir with whatever is in dir
> in master". It is more dangerous, and it can easily emulated by doing:
being what the user expects. As in, "master deleted this file; shouldn't
checkout pull the deletion to my new branch when I ask it to?".
But we can't distinguish those two cases without actually having a merge
base. And this isn't a merge; it's not about picking changes from
master, it's about saying "make dir look like it does in master". So
in that sense, the most straightforward thing is your second
alternative: afterwards, we should have only the files in "dir" that
master has.
A related question is what does this do:
git reset master -- dir
My mental model is that it makes the index for "dir" look just like
master:dir. And that seems pretty accurate; it deletes dir/file3 (which
does not exist in "master") from the index.
My mental model of "git checkout master -- dir" is similar. It should
make the index for "dir" look like master:dir, and then check that out.
IOW, I think of it as:
git reset master -- dir &&
git checkout -- dir
Maybe that is not accurate (well, clearly it does not match the current
behavior), but I think it is at least easy to explain and relatively
sane. So it is something to shoot for, and makes "git checkout"
consistent with "git reset".
> * This is a behaviour change, but it may qualify as a bugfix. I dunno.
I think it is a bug. I can see both of the alternatives you outlined
above making some sense, but checking out content that has _nothing_ to
do with master is just confusing. Either make it look like master, or
leave it alone.
-Peff
^ permalink raw reply
* Re: [PATCH] transport: do not allow to push over git:// protocol
From: Jeff King @ 2011-10-03 10:02 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Johannes Sixt, Nguyễn Thái Ngọc Duy, git
In-Reply-To: <m3d3eeo17l.fsf@localhost.localdomain>
On Mon, Oct 03, 2011 at 02:49:15AM -0700, Jakub Narebski wrote:
> I wonder if that is the case... but 48% responders of "Git User's
> Survey 2011" (3424 out of 7100 responders who answered queston
> "23) How do you publish/propagate your changes?") answered that they
> use push via git protocol.
>
> See https://www.survs.com/results/Q5CA9SKQ/P7DE07F0PL
I refuse to believe that 48% of people are using git:// to push. Surely
they are interpreting that response to overlap with "git over ssh" and
"git over http".
-Peff
^ permalink raw reply
* Re: [PATCHv3] git-web--browse: avoid the use of eval
From: Jeff King @ 2011-10-03 9:57 UTC (permalink / raw)
To: Chris Packham; +Cc: git, gitster, chriscool
In-Reply-To: <1317516257-24435-1-git-send-email-judge.packham@gmail.com>
On Sun, Oct 02, 2011 at 01:44:17PM +1300, Chris Packham wrote:
> Using eval causes problems when the URL contains an appropriately
> escaped ampersand (\&). Dropping eval from the built-in browser
> invocation avoids the problem.
>
> Helped-by: Jeff King <peff@peff.net> (test case)
> Signed-off-by: Chris Packham <judge.packham@gmail.com>
>
> ---
> The consensus from the last round of discussion [1] seemed to be to
> remove the eval from the built in browsers but quote custom browser
> commands appropriately.
>
> I've expanded the tests a little. A semi-colon had the same error as
> the ampersand. A hash was another common character that had meaning in
> a shell and in URL.
This looks good to me. I think we may want to squash in the two tests
below, too, which make sure we treat $browser_path and $browser_cmd
appropriately (the former is a filename, and the latter is a shell
snippet).
diff --git a/t/t9901-git-web--browse.sh b/t/t9901-git-web--browse.sh
index c6f48a9..7906e5d 100755
--- a/t/t9901-git-web--browse.sh
+++ b/t/t9901-git-web--browse.sh
@@ -34,4 +34,33 @@ test_expect_success \
test_cmp expect actual
'
+test_expect_success \
+ 'browser paths are properly quoted' '
+ echo fake: http://example.com/foo >expect &&
+ cat >"fake browser" <<-\EOF &&
+ #!/bin/sh
+ echo fake: "$@"
+ EOF
+ chmod +x "fake browser" &&
+ git config browser.w3m.path "`pwd`/fake browser" &&
+ git web--browse --browser=w3m \
+ http://example.com/foo >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success \
+ 'browser command allows arbitrary shell code' '
+ echo "arg: http://example.com/foo" >expect &&
+ git config browser.custom.cmd "
+ f() {
+ for i in \"\$@\"; do
+ echo arg: \$i
+ done
+ }
+ f" &&
+ git web--browse --browser=custom \
+ http://example.com/foo >actual &&
+ test_cmp expect actual
+'
+
test_done
^ permalink raw reply related
* Re: [PATCH] transport: do not allow to push over git:// protocol
From: Nguyen Thai Ngoc Duy @ 2011-10-03 9:52 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Sixt, git, Jonathan Nieder
In-Reply-To: <20111003094730.GA21610@sigill.intra.peff.net>
On Mon, Oct 3, 2011 at 8:47 PM, Jeff King <peff@peff.net> wrote:
>> To me, just "<service>: access denied" is enough. Not particularly
>> friendly but should be a good enough clue.
>
> Yeah, maybe. Certainly it's better than "the remote end hung up
> unexpectedly".
>
> However, the leakage is still there. You would get "the remote hung up"
> for no-such-repo, and "access denied" for this. Or were you just
> proposing that _all_ errors give "access denied". Certainly it's better
> than just hanging up, too, and there is no leakage there.
All of them. At least it's good to know my request has reached (and
rejected by) the server, not dropped on the floor by some random
firewall along the line.
> It might be nice to default to that, and let sites easily enable
> friendlier messages, though.
I'm thinking of passing "verbose" option back to server to get more
helpful messages, the option would be turned off by default. It's up
to admin to decide (would be actually helpful during deployment test,
for example). Or is it possible already?
--
Duy
^ permalink raw reply
* Re: [PATCH] transport: do not allow to push over git:// protocol
From: Jakub Narebski @ 2011-10-03 9:49 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Jeff King, Nguyễn Thái Ngọc Duy, git
In-Reply-To: <4E8975E7.2040804@viscovery.net>
Johannes Sixt <j.sixt@viscovery.net> writes:
> Am 10/3/2011 9:42, schrieb Jeff King:
> > I still think push-over-git:// is a bit insane, and especially now with
> > smart-http, you'd be crazy to run it. And in that sense, I wouldn't mind
> > seeing it deprecated.
>
> You must be kidding ;) It is so much easier to type
>
> git daemon --export-all --enable=receive-pack
>
> for a one-shot, temporary git connection compared to setting up a
> smart-http, ssh, or even a rsh server.
I wonder if that is the case... but 48% responders of "Git User's
Survey 2011" (3424 out of 7100 responders who answered queston
"23) How do you publish/propagate your changes?") answered that they
use push via git protocol.
See https://www.survs.com/results/Q5CA9SKQ/P7DE07F0PL
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH] transport: do not allow to push over git:// protocol
From: Jeff King @ 2011-10-03 9:47 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Johannes Sixt, git, Jonathan Nieder
In-Reply-To: <CACsJy8B7Z-fT+ED=4F-Ug-bhvCagSxr0X6vZqn5PGRfB7KnUTA@mail.gmail.com>
On Mon, Oct 03, 2011 at 08:44:22PM +1100, Nguyen Thai Ngoc Duy wrote:
> > GitHub uses it to make nice messages:
> >
> > $ git push origin
> > fatal: remote error:
> > You can't push to git://github.com/gitster/git.git
> > Use git@github.com:gitster/git.git
> >
> > We should maybe do something like the patch below:
>
> Jonathan also mentions another patch
>
> http://article.gmane.org/gmane.comp.version-control.git/182536
Yeah, I was just reading that. Sorry, I should have read the rest of the
thread more carefully. :)
> > 1. There is some information leakage there. In particular, one can
> > tell the difference now between "repo does not exist" and
> > "receive-pack is not turned on". Personally, I think the tradeoff
> > to have actual error messages is worth it. HTTP has had real error
> > codes for decades, and I don't think anybody is too up-in-arms that
> > I can probe which pages are 404, and which are 401.
>
> To me, just "<service>: access denied" is enough. Not particularly
> friendly but should be a good enough clue.
Yeah, maybe. Certainly it's better than "the remote end hung up
unexpectedly".
However, the leakage is still there. You would get "the remote hung up"
for no-such-repo, and "access denied" for this. Or were you just
proposing that _all_ errors give "access denied". Certainly it's better
than just hanging up, too, and there is no leakage there.
It might be nice to default to that, and let sites easily enable
friendlier messages, though.
-Peff
^ permalink raw reply
* Re: [PATCH] transport: do not allow to push over git:// protocol
From: Nguyen Thai Ngoc Duy @ 2011-10-03 9:44 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Sixt, git, Jonathan Nieder
In-Reply-To: <20111003093912.GA16078@sigill.intra.peff.net>
2011/10/3 Jeff King <peff@peff.net>:
> So yeah, that makes it even worse for the client to start refusing this
> without even contacting the server. I forgot that we added the "ERR"
> response way back in a807328 (connect.c: add a way for git-daemon to
> pass an error back to client, 2008-11-01).
>
> GitHub uses it to make nice messages:
>
> $ git push origin
> fatal: remote error:
> You can't push to git://github.com/gitster/git.git
> Use git@github.com:gitster/git.git
>
> We should maybe do something like the patch below:
Jonathan also mentions another patch
http://article.gmane.org/gmane.comp.version-control.git/182536
> but:
>
> 1. There is some information leakage there. In particular, one can
> tell the difference now between "repo does not exist" and
> "receive-pack is not turned on". Personally, I think the tradeoff
> to have actual error messages is worth it. HTTP has had real error
> codes for decades, and I don't think anybody is too up-in-arms that
> I can probe which pages are 404, and which are 401.
To me, just "<service>: access denied" is enough. Not particularly
friendly but should be a good enough clue.
--
Duy
^ permalink raw reply
* Re: [PATCH] transport: do not allow to push over git:// protocol
From: Jeff King @ 2011-10-03 9:39 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Nguyễn Thái Ngọc Duy, git
In-Reply-To: <4E8975E7.2040804@viscovery.net>
On Mon, Oct 03, 2011 at 10:44:23AM +0200, Johannes Sixt wrote:
> Am 10/3/2011 9:42, schrieb Jeff King:
> > I still think push-over-git:// is a bit insane, and especially now with
> > smart-http, you'd be crazy to run it. And in that sense, I wouldn't mind
> > seeing it deprecated.
>
> You must be kidding ;) It is so much easier to type
>
> git daemon --export-all --enable=receive-pack
>
> for a one-shot, temporary git connection compared to setting up a
> smart-http, ssh, or even a rsh server.
Ah, yeah, I didn't think about one-shot invocations like that (I think
the original motivation was somebody actually running it all the time).
So yeah, that makes it even worse for the client to start refusing this
without even contacting the server. I forgot that we added the "ERR"
response way back in a807328 (connect.c: add a way for git-daemon to
pass an error back to client, 2008-11-01).
GitHub uses it to make nice messages:
$ git push origin
fatal: remote error:
You can't push to git://github.com/gitster/git.git
Use git@github.com:gitster/git.git
We should maybe do something like the patch below:
diff --git a/daemon.c b/daemon.c
index 4c8346d..c1fa55f 100644
--- a/daemon.c
+++ b/daemon.c
@@ -255,6 +255,7 @@ static int run_service(char *dir, struct daemon_service *service)
loginfo("Request %s for '%s'", service->name, dir);
if (!enabled && !service->overridable) {
+ packet_write(1, "ERR %s: service not enabled", service->name);
logerror("'%s': service not enabled.", service->name);
errno = EACCES;
return -1;
@@ -288,6 +289,8 @@ static int run_service(char *dir, struct daemon_service *service)
enabled = service_enabled;
}
if (!enabled) {
+ packet_write(1, "ERR %s: service not enabled for '%s'",
+ service->name, path);
logerror("'%s': service not enabled for '%s'",
service->name, path);
errno = EACCES;
but:
1. There is some information leakage there. In particular, one can
tell the difference now between "repo does not exist" and
"receive-pack is not turned on". Personally, I think the tradeoff
to have actual error messages is worth it. HTTP has had real error
codes for decades, and I don't think anybody is too up-in-arms that
I can probe which pages are 404, and which are 401.
2. It probably makes sense to have a more human-friendly error
message.
3. It may be worth adding error messages for lots of other conditions
(e.g., no such repo). Assuming we accept the information leakage
for (1).
-Peff
^ permalink raw reply related
* Re: [PATCH] transport: do not allow to push over git:// protocol
From: Nguyen Thai Ngoc Duy @ 2011-10-03 9:12 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Ilari Liusvaara, git
In-Reply-To: <20111001052910.GA6502@elie>
2011/10/1 Jonathan Nieder <jrnieder@gmail.com>:
> Ilari Liusvaara wrote:
>
>> What about sticking code to return an error to git daemon instead of this?
>
> The code has even been written:
> http://thread.gmane.org/gmane.comp.version-control.git/145456/focus=145573
>
> Testing and other improvements would be very welcome.
Tests aside, are there any problems with the patch? I don't see any
followup discussions. Personally I don't see much value in adding the
description though.
--
Duy
^ permalink raw reply
* Symbolic references updated by fetch?
From: Mattias Jiderhamn @ 2011-10-03 8:47 UTC (permalink / raw)
To: git
In our current CVS setup, we have two separate builds in Jenkins CI; one for the
latest and greatest (head of "master" branch) and one for the last/current minor
release of the latest major release. The revisions to include in the minor
release build will be tagged with a tag we can call "next-minor-release" here.
Individual files are branched as needed and the "next-minor-release" tag is
moved onto the branch. The Continuous Integration job will fetch and build the
"next-minor-release" tag. As part of a major release, the "next-minor-release"
tag is moved to the head of the main branch again.
When moving to GIT, the natural thing will be a hotfix branch originating from
each major release. In order to have a fixed name for the current hotfix branch,
primarily for the CI but also simplifying for developers working with hotfixes,
it seems git symbolic-ref will do the trick. After the first major release we
can do something like this in our bare repository whereto developers push and
where from Jenkins pulls code to build:
git symbolic-ref refs/heads/current-hotfix-branch refs/heads/release-1-hotfixes
and after the next major release, we simply move the referece pointer, as such
git symbolic-ref refs/heads/current-hotfix-branch refs/heads/release-2-hotfixes
HOWEVER this seems to require that everyone fetching/pulling from that repo -
Jenkins included - delete their local "current-hotfix-branch" tracking branch,
and then refetch it.
Is there an easier way to solve the CI problem, eliminating the need to
explicitly deleting the tracking branch on all remote repos every time the
symbolic ref is moved i.e. at every major release?
How have others solved this? Do you simply reconfigure Jenkins every time...?
Thanks in advance.
^ permalink raw reply
* Re: [PATCH] transport: do not allow to push over git:// protocol
From: Johannes Sixt @ 2011-10-03 8:44 UTC (permalink / raw)
To: Jeff King; +Cc: Nguyễn Thái Ngọc Duy, git
In-Reply-To: <20111003074250.GB9455@sigill.intra.peff.net>
Am 10/3/2011 9:42, schrieb Jeff King:
> I still think push-over-git:// is a bit insane, and especially now with
> smart-http, you'd be crazy to run it. And in that sense, I wouldn't mind
> seeing it deprecated.
You must be kidding ;) It is so much easier to type
git daemon --export-all --enable=receive-pack
for a one-shot, temporary git connection compared to setting up a
smart-http, ssh, or even a rsh server.
-- Hannes
^ permalink raw reply
* Re: Branches & directories
From: Jeff King @ 2011-10-03 7:51 UTC (permalink / raw)
To: Junio C Hamano
Cc: Matthieu Moy, Hilco Wijbenga, Robin Rosenberg, Kyle Moffett,
Michael Witten, Evan Shelhamer, Git Mailing List
In-Reply-To: <7v4nzqikhg.fsf@alter.siamese.dyndns.org>
On Mon, Oct 03, 2011 at 12:48:27AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Yeah, you'd have to maintain your own dependency tree, then. Which is
> > nasty (aside from the work involved), because I don't think you can
> > portably get the header dependencies out of the C compiler.
>
> Heh, but doesn't your Makefile know the header dependencies anyway?
Sort of. It's often wrong (e.g., we are way over-inclusive in the git
Makefile; one of the things I like about ccache is that even when our
Makefile is overly conservative, ccache is fast. Or at least faster than
actually recompiling).
And of course it doesn't generally involve headers outside of your
project, whereas ccache does recompile if those change.
-Peff
^ permalink raw reply
* Re: Branches & directories
From: Junio C Hamano @ 2011-10-03 7:48 UTC (permalink / raw)
To: Jeff King
Cc: Matthieu Moy, Hilco Wijbenga, Robin Rosenberg, Kyle Moffett,
Michael Witten, Evan Shelhamer, Git Mailing List
In-Reply-To: <20111003074412.GC9455@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Yeah, you'd have to maintain your own dependency tree, then. Which is
> nasty (aside from the work involved), because I don't think you can
> portably get the header dependencies out of the C compiler.
Heh, but doesn't your Makefile know the header dependencies anyway?
^ permalink raw reply
* Re: [PATCH v2] ident: check /etc/mailname if email is unknown
From: Jonathan Nieder @ 2011-10-03 7:44 UTC (permalink / raw)
To: Johannes Sixt
Cc: Junio C Hamano, git, Matt Kraai, Gerrit Pape, Ian Jackson,
Linus Torvalds
In-Reply-To: <4E895FBD.8020904@viscovery.net>
Johannes Sixt wrote:
> This warns on EACCES. Is that OK? (Just asking, I have no opinion.)
Good catch. I was worried for a moment: could some command call
copy_email() in a loop, producing an annoyingly redundant stream of
warnings?
Luckily setup_ident() checks if git_default_email has been set to save
the trouble of computing it again. So the behavior is to warn exactly
once when using a command that uses an ident string, which is still
more often than ideal. It would be better to only warn if the
permissions are creating an actual problem, so the user can (1)
complain to the sysadmin and then (2) set an email address in
~/.gitconfig and move on.
We _could_ add parameters to setup_ident() to only warn if
/etc/mailname was going to be used to produce the committer ident (or
whichever ident is checked first). That would be confusing if
GIT_COMMITTER_EMAIL is set and GIT_AUTHOR_EMAIL is not.
In the long term it would be nice to find a way to warn when the
mailname we tried to retrieve was actually going to be used, but short
of that, the least confusing behavior is to just not warn at all.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
ident.c | 10 +---------
1 files changed, 1 insertions(+), 9 deletions(-)
diff --git a/ident.c b/ident.c
index edb43144..6f5c885d 100644
--- a/ident.c
+++ b/ident.c
@@ -55,16 +55,9 @@ static int add_mailname_host(char *buf, size_t len)
FILE *mailname;
mailname = fopen("/etc/mailname", "r");
- if (!mailname) {
- if (errno != ENOENT)
- warning("cannot open /etc/mailname: %s",
- strerror(errno));
+ if (!mailname)
return -1;
- }
if (!fgets(buf, len, mailname)) {
- if (ferror(mailname))
- warning("cannot read /etc/mailname: %s",
- strerror(errno));
fclose(mailname);
return -1;
}
@@ -80,7 +73,6 @@ static void add_domainname(char *buf, size_t len)
const char *domainname;
if (gethostname(buf, len)) {
- warning("cannot get host name: %s", strerror(errno));
strlcpy(buf, "(none)", len);
return;
}
--
1.7.7.rc1
^ permalink raw reply related
* Re: Branches & directories
From: Jeff King @ 2011-10-03 7:44 UTC (permalink / raw)
To: Matthieu Moy
Cc: Hilco Wijbenga, Robin Rosenberg, Kyle Moffett, Michael Witten,
Junio C Hamano, Evan Shelhamer, Git Mailing List
In-Reply-To: <vpqmxdiikt2.fsf@bauges.imag.fr>
On Mon, Oct 03, 2011 at 09:41:29AM +0200, Matthieu Moy wrote:
> Jeff King <peff@peff.net> writes:
>
> > Speaking of which; does anybody know of a git-aware ccache-like tool?
> > We already have a nice index of the sha1 of each file in the repository
> > (along with a stat cache showing us whether it's up-to-date or not).
> > Something like ccache could avoid even looking in the C files at all if
> > it relied on git's index.
>
> It would be a bit harder than that I think. IIRC, ccache hashes the
> preprocessed file, hence it will notice if a .h file changed, even if
> it's outside the project.
Yeah, you'd have to maintain your own dependency tree, then. Which is
nasty (aside from the work involved), because I don't think you can
portably get the header dependencies out of the C compiler.
Oh well.
-Peff
^ permalink raw reply
* Re: [PATCH] transport: do not allow to push over git:// protocol
From: Jeff King @ 2011-10-03 7:42 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1317432415-9459-1-git-send-email-pclouds@gmail.com>
On Sat, Oct 01, 2011 at 11:26:55AM +1000, Nguyen Thai Ngoc Duy wrote:
> This protocol has never been designed for pushing. Attempts to push
> over git:// usually result in
>
> fatal: The remote end hung up unexpectedly
>
> That message does not really point out the reason. With this patch, we get
>
> error: this protocol does not support pushing
> error: failed to push some refs to 'git://some-host.com/my/repo'
I thought pushing over git:// _is_ supported. It's just that most
servers don't have it turned on, for the obvious lack-of-authentication
reasons.
See 4b3b1e1 (git-push through git protocol, 2007-01-21), and the
discussion here:
http://thread.gmane.org/gmane.comp.version-control.git/37325
Your patch shuts it off at the client level, so even with it turned on
for the server, the client can never get to it.
I still think push-over-git:// is a bit insane, and especially now with
smart-http, you'd be crazy to run it. And in that sense, I wouldn't mind
seeing it deprecated. But just shutting it off without a deprecation
period seems unnecessarily harsh.
The real problem here seems to be that instead of communicating "no, we
don't support that", git-daemon just hangs up. It would be a much nicer
fix if we could change that. I'm not sure it's possible, though. There's
not much room in the beginning of the room to make that communication in
a way that's backwards compatible.
-Peff
^ permalink raw reply
* Re: Branches & directories
From: Matthieu Moy @ 2011-10-03 7:41 UTC (permalink / raw)
To: Jeff King
Cc: Hilco Wijbenga, Robin Rosenberg, Kyle Moffett, Michael Witten,
Junio C Hamano, Evan Shelhamer, Git Mailing List
In-Reply-To: <20111003073456.GA10054@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Speaking of which; does anybody know of a git-aware ccache-like tool?
> We already have a nice index of the sha1 of each file in the repository
> (along with a stat cache showing us whether it's up-to-date or not).
> Something like ccache could avoid even looking in the C files at all if
> it relied on git's index.
It would be a bit harder than that I think. IIRC, ccache hashes the
preprocessed file, hence it will notice if a .h file changed, even if
it's outside the project.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: Branches & directories
From: Jeff King @ 2011-10-03 7:34 UTC (permalink / raw)
To: Matthieu Moy
Cc: Hilco Wijbenga, Robin Rosenberg, Kyle Moffett, Michael Witten,
Junio C Hamano, Evan Shelhamer, Git Mailing List
In-Reply-To: <vpqaa9ijzt4.fsf@bauges.imag.fr>
On Mon, Oct 03, 2011 at 09:32:07AM +0200, Matthieu Moy wrote:
> Hilco Wijbenga <hilco.wijbenga@gmail.com> writes:
>
> > Yes, I meant it literally. And, no, Git could not possibly know so it
> > would have to be optional behaviour. But it's probably a lot of work
> > for (for most people) little gain.
>
> Not only little gain, but also important risk: users of this feature
> would be likely to spend hours debugging something just because some
> files weren't recompiled at the right time.
>
> If you want to optimize the number of files compiled by "make", then
> ccache is your friend. This one is safe.
Yes. Despite my previous message showing what _could_ be done, I do
think it's crazy. You should just use ccache.
Speaking of which; does anybody know of a git-aware ccache-like tool?
We already have a nice index of the sha1 of each file in the repository
(along with a stat cache showing us whether it's up-to-date or not).
Something like ccache could avoid even looking in the C files at all if
it relied on git's index.
I don't know how much speedup it would yield in practice, though.
-Peff
^ 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