* Re: [PATCH v2 09/15] user-manual: Use 'git config --global user.*' for setup
From: Junio C Hamano @ 2013-02-10 22:48 UTC (permalink / raw)
To: W. Trevor King; +Cc: Git
In-Reply-To: <20130210222508.GC8377@odin.tremily.us>
"W. Trevor King" <wking@tremily.us> writes:
> On Sun, Feb 10, 2013 at 02:12:05PM -0800, Junio C Hamano wrote:
>> > +Which will add the following stanza to a file named `.gitconfig` in
>> > +your home directory:
>>
>> Looks good, even though I do not think we would want/need to confuse
>> the readers with an unfamiliar word "stanza".
>
> Aw. I suppose "section" is better? (It's what Python's ConfigParser
> uses anyway). I saw "stanza" in another part of the manual and
> thought it was quite poetic ;).
How about not saying anything and say "adds the following"?
^ permalink raw reply
* Re: [PATCH v2 09/15] user-manual: Use 'git config --global user.*' for setup
From: W. Trevor King @ 2013-02-10 22:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git
In-Reply-To: <7vobfrrdjy.fsf@alter.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 303 bytes --]
On Sun, Feb 10, 2013 at 02:48:01PM -0800, Junio C Hamano wrote:
> How about not saying anything and say "adds the following"?
Will do in v3.
--
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v2 15/15] user-manual: Use --format=tar.gz to create a gzipped tarball
From: Junio C Hamano @ 2013-02-10 22:52 UTC (permalink / raw)
To: W. Trevor King; +Cc: Git
In-Reply-To: <20130210223239.GE8377@odin.tremily.us>
"W. Trevor King" <wking@tremily.us> writes:
> On Sun, Feb 10, 2013 at 02:27:08PM -0800, Junio C Hamano wrote:
>> This is a "meh" at least to me. Unless it uses something like
>>
>> git archive -o latest.tar.gz --prefix=project/ HEAD
>>
>> it is not all that interesting.
>
> Great suggestion. My original goal was to remove the pipe, but if we
> can remove the redirection too, that's one less POSIX-ism for newbies
> to wonder about ;).
I was referring to more about the implicit format recognition not
pipe.
^ permalink raw reply
* Re: Git prompt
From: Junio C Hamano @ 2013-02-10 22:54 UTC (permalink / raw)
To: Jeff King
Cc: Jonathan Nieder, Ethan Reesor, git, Ramkumar Ramachandra,
Greg Brockman
In-Reply-To: <20130210224345.GA32318@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Sun, Feb 10, 2013 at 01:25:38PM -0800, Jonathan Nieder wrote:
>
>> Ethan Reesor wrote:
>>
>> > I have a git user set up on my server. It's prompt is set to
>> > git-prompt and it's git-shell-commands is empty.
>> [...]
>> > How do I make the git user work like github where, upon attempting to
>> > get a prompt, the connection is closed?
>>
>> I assume you mean that the user's login shell is git-shell.
>>
>> You can disable interactive logins by removing the
>> ~/git-shell-commands/ directory. Unfortunately that doesn't let you
>> customize the message. Perhaps it would make sense to teach shell.c
>> to look for a
>>
>> [shell]
>> greeting = 'Hi %(username)! You've successfully authenticated, but I do not provide interactive shell access.'
>>
>> setting in git's config file. What do you think?
>
> I think something like that makes sense. To my knowledge there is no way
> with stock git to customize git-shell's output (at GitHub, that message
> comes from our front-end routing process before you even hit git-shell
> on our backend machines).
>
> The "username" in our version of the message comes from a database
> mapping public keys to GitHub users, not the Unix username. But I
> suspect sites running stock Git would be happy enough to have
> %(username) map to the actual Unix username.
Yeah, that greeting is cute---I like it ;-)
^ permalink raw reply
* Re: [PATCH v2 03/15] user-manual: Use 'remote add' to setup push URLs
From: W. Trevor King @ 2013-02-10 22:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git
In-Reply-To: <7vsj53rdnc.fsf@alter.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 1692 bytes --]
On Sun, Feb 10, 2013 at 02:45:59PM -0800, Junio C Hamano wrote:
> "W. Trevor King" <wking@tremily.us> writes:
> > I think for new users, `git config …`'s opacity may a good thing
>
> No. Reducing the fear factor from new users by not hiding simple
> things is one of the design of these tutorials, and showing that the
> contents of .git/config is a simple, human-readable plain text file
> is a part of it.
>
> > I'm fine with this too, but if this is the suggested route, why bother
> > with `git config` at all? Is it just for ease of scripting?
>
> I would actually say why bother with `git remote` at all, when you
> can do that with `git config`, but arguably when a Porcelain that is
> type-specific exists, use of it would make the life of end users
> easier, and that is why we ended up having `git remote add`. So if
> you know how the remote configuration is implemented, you can still
> use `config` to do the same, but `remote add` that futzes with many
> variables at once would be easier to use for most people.
Hmm. It sounds like maybe it would be better to skip both `git
config` and `git remote` and focus on the config file text itself.
Explain what each line does. When we talk about identifing yourself
(which is, I think, the first point configuration comes up), we can
mention `git config` as an alternative interface. When we talk about
remotes, we can mention `git remote` as an alternative interface.
Both references would just point you to the man pages.
Thoughts?
--
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v2 15/15] user-manual: Use --format=tar.gz to create a gzipped tarball
From: W. Trevor King @ 2013-02-10 23:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git
In-Reply-To: <7vk3qfrdco.fsf@alter.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 1081 bytes --]
On Sun, Feb 10, 2013 at 02:52:23PM -0800, Junio C Hamano wrote:
> "W. Trevor King" <wking@tremily.us> writes:
> > On Sun, Feb 10, 2013 at 02:27:08PM -0800, Junio C Hamano wrote:
> >> This is a "meh" at least to me. Unless it uses something like
> >>
> >> git archive -o latest.tar.gz --prefix=project/ HEAD
> >>
> >> it is not all that interesting.
> >
> > Great suggestion. My original goal was to remove the pipe, but if we
> > can remove the redirection too, that's one less POSIX-ism for newbies
> > to wonder about ;).
>
> I was referring to more about the implicit format recognition not
> pipe.
I'll mention it when I'm linking them to git-archive(1) ;). I think
the goal in the manual should be to show people that the functionality
exists and is not scary. Explaining the actual implementation details
and assorted usage details is best left to command-specific
documentation.
--
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v2 06/15] user-manual: Give 'git push -f' as an alternative to +master
From: Junio C Hamano @ 2013-02-10 23:19 UTC (permalink / raw)
To: W. Trevor King; +Cc: Git
In-Reply-To: <7vsj53sub1.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
>> +Note the addition of the `+` sign. Alternatively, you can use the
>> +`-f` flag to force the remote update, as in:
>> +
>> +-------------------------------------------------
>> +$ git push -f ssh://yourserver.com/~you/proj.git master
>> +-------------------------------------------------
>> +
>
> I didn't check the surrounding examples but would it make it a bit
> too advanced to make the example flow push out more than one
> branches here (perhaps he is also updating the 'maint' branch)?
> Then use of "--force" can be explained as "Instead of adding + to
> each and every refs to be pushed, you can use a single -f to force
> everything."
Now I have. This is after a "git push" failed, saying that 'master'
does not fast forward, and attempting to push the 'master' out
forcibly. There is little risk the user would get confused and say
"push -f $there master some-other-branch" in the context of this
flow.
So I think the patch is good, but others may have comments, so I'll
let the patch floating on the list for now.
^ permalink raw reply
* Re: [PATCH v2 07/15] user-manual: Mention 'git remote add' for remote branch config
From: W. Trevor King @ 2013-02-10 22:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git
In-Reply-To: <7vmwvbstw1.fsf@alter.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 531 bytes --]
On Sun, Feb 10, 2013 at 02:09:50PM -0800, Junio C Hamano wrote:
> I'd suggest rewriting everything after "If there are other
> repositories that you also use frequently,..." of this section.
>
> Replace the first example after "for example, after" that uses
> "config remote.example.url" with the new text in your patch,
> [snip additional advice]
Will do in v3.
--
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v3 06/11] sequencer.c: always separate "(cherry picked from" from commit body
From: Brandon Casey @ 2013-02-10 23:25 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, pclouds, gitster, Brandon Casey
In-Reply-To: <20130128023409.GH8206@elie.Belkin>
On Sun, Jan 27, 2013 at 6:34 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Brandon Casey wrote:
>
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -497,6 +558,8 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
>> }
>>
>> if (opts->record_origin) {
>> + if (!has_conforming_footer(&msgbuf, 0))
>> + strbuf_addch(&msgbuf, '\n');
>
> What should this do in the case of an entirely blank message?
> (Maybe that's too insane a case to worry about.)
Yeah, I think there are a number of insane cases that I think we
should just say "behavior is undefined".
- Entirely blank message
- Message lacking trailing newline.
- Oneline message that is actually a signed-off-by (equal or not to
the committer's sob).
- Message body that is entirely 1 or more newlines, or one which has
more than one trailing newline.
It's probably not worth going through contortions to handle these cases.
-Brandon
^ permalink raw reply
* Re: [PATCH v3 07/11] sequencer.c: teach append_signoff how to detect duplicate s-o-b
From: Brandon Casey @ 2013-02-10 23:36 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, pclouds, gitster, Brandon Casey
In-Reply-To: <20130128031424.GK8206@elie.Belkin>
On Sun, Jan 27, 2013 at 7:14 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Brandon Casey wrote:
>
>> --- a/sequencer.c
>> +++ b/sequencer.c
> [...]
>> @@ -1096,10 +1117,16 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer)
>> strbuf_addch(&sob, '\n');
>> for (i = msgbuf->len - 1 - ignore_footer; i > 0 && msgbuf->buf[i - 1] != '\n'; i--)
>> ; /* do nothing */
>> - if (prefixcmp(msgbuf->buf + i, sob.buf)) {
>> - if (!i || !has_conforming_footer(msgbuf, ignore_footer))
>> - strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, "\n", 1);
>> - strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, sob.buf, sob.len);
>> - }
>> +
>> + if (i)
>> + has_footer = has_conforming_footer(msgbuf, &sob, ignore_footer);
>
> Is this "if (i)" test needed? I'd expect that if the message has no newlines,
> has_conforming_footer() would notice that and return 0.
Thanks for pointing out this awkwardness. It should be pushed into
has_conforming_footer().
-Brandon
^ permalink raw reply
* Re: [PATCH v3 11/11] Unify appending signoff in format-patch, commit and sequencer
From: Brandon Casey @ 2013-02-10 23:55 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, pclouds, gitster, Brandon Casey
In-Reply-To: <20130128033921.GP8206@elie.Belkin>
On Sun, Jan 27, 2013 at 7:39 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Brandon Casey wrote:
>
>> --- a/log-tree.c
>> +++ b/log-tree.c
> [...]
>> @@ -208,94 +207,6 @@ void show_decorations(struct rev_info *opt, struct commit *commit)
>> putchar(')');
>> }
>>
>> -/*
>> - * Search for "^[-A-Za-z]+: [^@]+@" pattern. It usually matches
>> - * Signed-off-by: and Acked-by: lines.
>> - */
>
> That's stricter than the test from sequencer.c. Maybe it's worth
> stealing to avoid false positives?
No, we don't want this stricter test because it assumes that the
right-hand side of "[-A-Za-z]+:" will be an email address, so it
requires an '@' to exist. We want to be able to support lines that do
not have email addresses on the right-hand side like:
Bug: XXX
Change-Id: XXX
and perhaps eventually
Cherry-picked-from: XXX
The current series has retained the same basic test for an
rfc2822-like line that existed in sequencer.c and would interpret a
line that contains only a colon as conforming. A follow-on patch
could require that at least one character precede the colon, but that
would be a change in behavior that is not the goal of this series.
-Brandon
^ permalink raw reply
* Re: Git prompt
From: Sitaram Chamarty @ 2013-02-11 0:43 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Jonathan Nieder, Ethan Reesor, git,
Ramkumar Ramachandra, Greg Brockman
In-Reply-To: <7vfw13rd9x.fsf@alter.siamese.dyndns.org>
On Mon, Feb 11, 2013 at 4:24 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> On Sun, Feb 10, 2013 at 01:25:38PM -0800, Jonathan Nieder wrote:
>>
>>> Ethan Reesor wrote:
>>>
>>> > I have a git user set up on my server. It's prompt is set to
>>> > git-prompt and it's git-shell-commands is empty.
>>> [...]
>>> > How do I make the git user work like github where, upon attempting to
>>> > get a prompt, the connection is closed?
>>>
>>> I assume you mean that the user's login shell is git-shell.
>>>
>>> You can disable interactive logins by removing the
>>> ~/git-shell-commands/ directory. Unfortunately that doesn't let you
>>> customize the message. Perhaps it would make sense to teach shell.c
>>> to look for a
>>>
>>> [shell]
>>> greeting = 'Hi %(username)! You've successfully authenticated, but I do not provide interactive shell access.'
>>>
>>> setting in git's config file. What do you think?
>>
>> I think something like that makes sense. To my knowledge there is no way
>> with stock git to customize git-shell's output (at GitHub, that message
>> comes from our front-end routing process before you even hit git-shell
>> on our backend machines).
>>
>> The "username" in our version of the message comes from a database
>> mapping public keys to GitHub users, not the Unix username. But I
>> suspect sites running stock Git would be happy enough to have
>> %(username) map to the actual Unix username.
>
> Yeah, that greeting is cute---I like it ;-)
Indeed! In gitolite, I borrowed that idea added to it by making it
print a list of repos you have access to, along with what permissions
(R or RW) you have :-)
I'm not suggesting git should do that, but instead of a fixed string,
a default command to be executed would be better. That command could
do anything the local site wanted to make it do, including something
eqvt to what I just said.
This of course now means that the ~/git-shell-commands should not be
empty, since that is where this default command also will be present.
^ permalink raw reply
* [RFC/PATCH] shell: allow 'help' command to disable interactive shell
From: Jonathan Nieder @ 2013-02-11 1:20 UTC (permalink / raw)
To: Sitaram Chamarty
Cc: Junio C Hamano, Jeff King, Ethan Reesor, git,
Ramkumar Ramachandra, Greg Brockman
In-Reply-To: <CAMK1S_jFUXiHM6teVwoxO9gv77B1KBQoSi-B32dwVKemXnDx9w@mail.gmail.com>
If I disable git-shell's interactive mode by removing the
~/git-shell-commands directory, then attempts to use 'ssh' with the
git account interactively produce an error message intended for the
administrator:
$ ssh git@myserver
fatal: Interactive git shell is not enabled.
hint: ~/git-shell-commands should exist and have read and execute access.
$
It is better to give the user a friendly hint that she is on the
right track, like GitHub does:
Hi <username>! You've successfully authenticated, but
GitHub does not provide shell access.
An appropriate greeting might even include more complex information,
like a list of repositories the user has access to. A
git-shell-commands directory with only a "help" script can get us most
of the way there, but it unfortunately it produces a "git>" prompt
where the user can do nothing but ask for more help or exit. So allow
the "help" script to abort the shell by exiting with nonzero status.
Downside: this will prevent interactive git-shell logins in existing
setups where the "help" script exits with nonzero status by mistake.
Hopefully those are rare enough to not cause much trouble in practice.
Reported-by: Ethan Reesor <firelizzard@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Sitaram Chamarty wrote:
> Indeed! In gitolite, I borrowed that idea added to it by making it
> print a list of repos you have access to, along with what permissions
> (R or RW) you have :-)
>
> I'm not suggesting git should do that, but instead of a fixed string,
> a default command to be executed would be better.
Good call.
[...]
> This of course now means that the ~/git-shell-commands should not be
> empty, since that is where this default command also will be present.
How about this?
A patch on top could change the default "git-shell-commands is not
present" message if that seems worthwhile.
Documentation/git-shell.txt | 26 ++++++++++++++++++++++++++
shell.c | 10 ++++++++--
2 files changed, 34 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-shell.txt b/Documentation/git-shell.txt
index 9b925060..758083ff 100644
--- a/Documentation/git-shell.txt
+++ b/Documentation/git-shell.txt
@@ -29,6 +29,32 @@ read and execute permissions to the directory in order to execute the
programs in it. The programs are executed with a cwd of $HOME, and
<argument> is parsed as a command-line string.
+When run interactively (with no arguments), 'git-shell' will
+automatically run `~/git-shell-commands/help` on startup, provided it
+exists. If the 'help' command fails then the interactive shell is
+aborted.
+
+EXAMPLE
+-------
+
+To disable interactive logins, displaying a greeting instead:
++
+----------------
+$ chsh -s /usr/bin/git-shell
+$ mkdir $HOME/git-shell-commands
+$ cat >$HOME/git-shell-commands/help <<\EOF
+#!/bin/sh
+printf '%s\n' "Hi $USER! You've successfully authenticated, but I do not"
+printf '%s\n' "provide interactive shell access."
+exit 128
+EOF
+$ chmod +x $HOME/git-shell-commands/help
+----------------
+
+SEE ALSO
+--------
+contrib/git-shell-commands/README
+
GIT
---
Part of the linkgit:git[1] suite
diff --git a/shell.c b/shell.c
index 84b237fe..3abc2b84 100644
--- a/shell.c
+++ b/shell.c
@@ -63,10 +63,16 @@ static void cd_to_homedir(void)
static void run_shell(void)
{
- int done = 0;
+ int done = 0, status;
static const char *help_argv[] = { HELP_COMMAND, NULL };
/* Print help if enabled */
- run_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE);
+ status = run_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE);
+ if (!status)
+ ; /* success */
+ else if (status == -1 && errno == ENOENT)
+ ; /* help disabled */
+ else
+ exit(status);
do {
struct strbuf line = STRBUF_INIT;
--
1.8.1.3
^ permalink raw reply related
* Re: inotify to minimize stat() calls
From: Duy Nguyen @ 2013-02-11 2:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ramkumar Ramachandra, Robert Zeh, Git List, finnag
In-Reply-To: <7vhaljudos.fsf@alter.siamese.dyndns.org>
On Mon, Feb 11, 2013 at 3:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
> The other "lstat()" experiment was a very interesting one, but this
> is not yet an interesting experiment to see where in the "ignore"
> codepath we are spending times.
>
> We know that we can tell wt_status_collect_untracked() not to bother
> with the untracked or ignored files with !s->show_untracked_files
> already, but I think the more interesting question is if we can show
> the untracked files with less overhead.
>
> If we want to show untrackedd files, it is a given that we need to
> read directories to see what paths there are on the filesystem. Is
> the opendir/readdir cost dominating in the process? Are we spending
> a lot of time sifting the result of opendir/readdir via the ignore
> mechanism? Is reading the "ignore" files costing us much to prime
> the ignore mechanism?
>
> If readdir cost is dominant, then that makes "cache gitignore" a
> nonsense proposition, I think. If you really want to "cache"
> something, you need to have somebody (i.e. a daemon) who constantly
> keeps an eye on the filesystem changes and can respond with the up
> to date result directly to fill_directory(). I somehow doubt that
> it is a direction we would want to go in, though.
Yeah, it did not cut out syscall cost, I also cut a lot of user-space
processing (plus .gitignore content access). From the timings I posted
earlier,
> unmodified dir.c
> real 0m0.550s 0m0.287s
> user 0m0.305s 0m0.201s
> sys 0m0.240s 0m0.084s
sys time is reduced from 0.24s to 0.08s, so readdir+opendir definitely
has something to do with it (and perhaps reading .gitignore). But it
also reduces user time from 0.305 to 0.201s. I don't think avoiding
readdir+openddir will bring us this gain. It's probably the cost of
matching .gitignore. I'll try to replace opendir+readdir with a
no-syscall version. At this point "untracked caching" sounds more
feasible (and less complex) than ".gitignore cachine".
--
Duy
^ permalink raw reply
* Re: inotify to minimize stat() calls
From: Duy Nguyen @ 2013-02-11 3:03 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Robert Zeh, Junio C Hamano, Git List, finnag
In-Reply-To: <CALkWK0kLieAfPihX3j=BzD+ndo-g-2210Za2xN=HbHcRVwgMtA@mail.gmail.com>
On Sun, Feb 10, 2013 at 11:45 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> So you're skipping the rest of refresh_cache_ent(), which contains our
> lstat() and returning immediately. Instead of marking paths with the
> "assume unchanged" bit, as core.ignoreStat does, you're directly
> attacking the function that refreshes the index and bypassing the
> lstat() call. How are they different? read-cache.c:1030 checks
> ce->flags & CE_VALID (which is set in read-cache.c:88 if
> assume_unchanged) and bypasses the lstat() call anyway. So why didn't
> you just set core.ignoreStat for your test?
It just did not occur to me that core.ignoreStat does the same.
> Ah, read_directory(), from the .gitignore/ exclude angle. Yes,
> read_directory() seems to be the main culprit there, from my reading
> of Documentation/technical/api-directory-listing.txt.
>
> So, what did you do? You short-circuited the function into never
> executing read_directory_recursive(), so the opendir() and readdir()
> are gone. I'm confused about what this means: will new directories
> fail to appear as "untracked" now?
No, read_directory returns the list of untracked/ignored files.
Returning empty lists means no untracked nor ignored files.
--
Duy
^ permalink raw reply
* Re: inotify to minimize stat() calls
From: Duy Nguyen @ 2013-02-11 3:21 UTC (permalink / raw)
To: Robert Zeh; +Cc: Junio C Hamano, Ramkumar Ramachandra, Git List
In-Reply-To: <CAKXa9=qQwJqxZLxhAS35QeF1+dwH+ukod0NfFggVCuUZHz-USg@mail.gmail.com>
On Mon, Feb 11, 2013 at 2:03 AM, Robert Zeh <robert.allan.zeh@gmail.com> wrote:
> On Sat, Feb 9, 2013 at 1:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>>
>>> This is much better than Junio's suggestion to study possible
>>> implementations on all platforms and designing a generic daemon/
>>> communication channel. That's no weekend project.
>>
>> It appears that you misunderstood what I wrote. That was not "here
>> is a design; I want it in my system. Go implemment it".
>>
>> It was "If somebody wants to discuss it but does not know where to
>> begin, doing a small experiment like this and reporting how well it
>> worked here may be one way to do so.", nothing more.
>
> What if instead of communicating over a socket, the daemon
> dumped a file containing all of the lstat information after git
> wrote a file? By definition the daemon should know about file writes.
>
> There would be no network communication, which I think would make
> things more secure. It would simplify the rendezvous by insisting on
> well known locations in $GIT_DIR.
We need some sort of interactive communication to the daemon anyway,
to validate that the information is uptodate. Assume that a user makes
some changes to his worktree before starting the daemon, git needs to
know that what the daemon provides does not represent a complete
file-change picture and it better refreshes the index the old way
once, then trust the daemon.
I think we could solve that by storing a "session id", provided by the
daemon, in .git/index. If the session id is not present (or does not
match what the current daemon gives), refresh the old way. After
refreshing, it may ask the daemon for new session id and store it.
Next time if the session id is still valid, trust the daemon's data.
This session id should be different every time the daemon restarts for
this to work.
--
Duy
^ permalink raw reply
* Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell
From: Junio C Hamano @ 2013-02-11 3:44 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Sitaram Chamarty, Jeff King, Ethan Reesor, git,
Ramkumar Ramachandra, Greg Brockman
In-Reply-To: <20130211012016.GA13243@elie.Belkin>
Jonathan Nieder <jrnieder@gmail.com> writes:
> How about this?
>
> A patch on top could change the default "git-shell-commands is not
> present" message if that seems worthwhile.
Hmph.
I wonder if rewording the message when git-shell-commmands directory
is not there may be a better first step (which actually could be the
last step)?
That is, showing something like this,
> +printf '%s\n' "Hi $USER! You've successfully authenticated, but I do not"
> +printf '%s\n' "provide interactive shell access."
but rephrased with a reference to "git help shell" for people
preparing their own server when ~/git-shell-commands/ in good order?
Something like
diff --git a/shell.c b/shell.c
index 84b237f..71ff04f 100644
--- a/shell.c
+++ b/shell.c
@@ -162,9 +162,11 @@ int main(int argc, char **argv)
/* Allow the user to run an interactive shell */
cd_to_homedir();
if (access(COMMAND_DIR, R_OK | X_OK) == -1) {
- die("Interactive git shell is not enabled.\n"
+ die("The user has been recognized as '%s' but "
+ "interactive git shell is not enabled.\n"
"hint: ~/" COMMAND_DIR " should exist "
- "and have read and execute access.");
+ "and have read and execute access.",
+ get_user_name());
}
run_shell();
exit(0);
^ permalink raw reply related
* Re: inotify to minimize stat() calls
From: Duy Nguyen @ 2013-02-11 3:53 UTC (permalink / raw)
To: kusmabite
Cc: Ramkumar Ramachandra, Robert Zeh, Junio C Hamano, Git List,
finnag, Karsten Blees
In-Reply-To: <CABPQNSZ282Lre=sy-+ZQdJA9JnGqQguq2bQDOwvjb0fP+1-w8Q@mail.gmail.com>
On Sun, Feb 10, 2013 at 11:58 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
> Karsten Blees has done something similar-ish on Windows, and he posted
> the results here:
>
> https://groups.google.com/forum/#!topic/msysgit/fL_jykUmUNE/discussion
>
> I also seem to remember he doing a ReadDirectoryChangesW version, but
> I don't remember what happened with that.
Thanks. I came across that but did not remember. For one thing, we
know the inotify alternative for Windows: ReadDirectoryChangesW.
But the meat of the patch is not about that function. In fact it's
dropped in fscache-v3 [1]. It seems that doing
FindFirstFile/FindNextFile for an entire directory, cache the results
and use it to simulate lstat() is faster on Windows. Sounds similar to
preload-index. And because directory listing is cached anyway,
opendir/readdir is replaced to read from cache instead of opening the
directory again.
So it is orthogonal with using ReadDirectoryChangesW/inotify to
further reduce the system calls.
I copy "git status"'s (impressive) numbers from fscache-v0 for those
who are interested in:
preload | -u | normal | cached | gain
--------+-----+--------+--------+------
false | all | 25.144 | 3.055 | 8.2
false | no | 22.822 | 1.748 | 12.8
true | all | 9.234 | 2.179 | 4.2
true | no | 6.833 | 0.955 | 7.2
[1] https://github.com/kblees/git/commit/35f319609aa046d2350db32d3afa1fa44920e880
--
Duy
^ permalink raw reply
* Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell
From: Jeff King @ 2013-02-11 3:59 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Sitaram Chamarty, Junio C Hamano, Ethan Reesor, git,
Ramkumar Ramachandra, Greg Brockman
In-Reply-To: <20130211012016.GA13243@elie.Belkin>
On Sun, Feb 10, 2013 at 05:20:16PM -0800, Jonathan Nieder wrote:
> > This of course now means that the ~/git-shell-commands should not be
> > empty, since that is where this default command also will be present.
>
> How about this?
I like the general direction this is going, but:
> +When run interactively (with no arguments), 'git-shell' will
> +automatically run `~/git-shell-commands/help` on startup, provided it
> +exists. If the 'help' command fails then the interactive shell is
> +aborted.
Doesn't that mean that people who currently do allow interactive access
and have a ~/git-shell-commands/help (that returns zero) will get
spammed by its as a motd each time they connect?
To be honest, I am not really clear on what interactive git-shell is
used for. AFAIK, it does nothing unless you have set up custom commands,
and I have never actually seen them in the wild. So maybe it is not a
big deal.
If I understand correctly, calling it "check-interactive", "greeting",
or something instead of "help" would be sufficient, and then you
wouldn't have to worry about backwards compatibility.
> diff --git a/shell.c b/shell.c
> index 84b237fe..3abc2b84 100644
> --- a/shell.c
> +++ b/shell.c
> @@ -63,10 +63,16 @@ static void cd_to_homedir(void)
>
> static void run_shell(void)
> {
> - int done = 0;
> + int done = 0, status;
> static const char *help_argv[] = { HELP_COMMAND, NULL };
> /* Print help if enabled */
> - run_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE);
> + status = run_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE);
> + if (!status)
> + ; /* success */
> + else if (status == -1 && errno == ENOENT)
> + ; /* help disabled */
> + else
> + exit(status);
This kicks in only when there is no command given, right? So if I ran
"ssh example.com", it would give me the help message rather than (or in
addition) giving me interactive access.
What about "ssh example.com foo"? Do we want to allow a custom message
there, too (it might be different there; e.g., an allowed list of
commands might make more sense)?
-Peff
^ permalink raw reply
* Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell
From: Jonathan Nieder @ 2013-02-11 4:14 UTC (permalink / raw)
To: Jeff King
Cc: Sitaram Chamarty, Junio C Hamano, Ethan Reesor, git,
Ramkumar Ramachandra, Greg Brockman
In-Reply-To: <20130211035908.GA4543@sigill.intra.peff.net>
Jeff King wrote:
> On Sun, Feb 10, 2013 at 05:20:16PM -0800, Jonathan Nieder wrote:
>> +When run interactively (with no arguments), 'git-shell' will
>> +automatically run `~/git-shell-commands/help` on startup, provided it
>> +exists. If the 'help' command fails then the interactive shell is
>> +aborted.
>
> Doesn't that mean that people who currently do allow interactive access
> and have a ~/git-shell-commands/help (that returns zero) will get
> spammed by its as a motd each time they connect?
Only interactive connections. That's the existing behavior.
[...]
> What about "ssh example.com foo"? Do we want to allow a custom message
> there, too (it might be different there; e.g., an allowed list of
> commands might make more sense)?
I wouldn't mind, but it's definitely not my itch.
Hoping that clarifies,
Jonathan
^ permalink raw reply
* Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell
From: Jonathan Nieder @ 2013-02-11 4:17 UTC (permalink / raw)
To: Junio C Hamano
Cc: Sitaram Chamarty, Jeff King, Ethan Reesor, git,
Ramkumar Ramachandra, Greg Brockman
In-Reply-To: <7v7gmfqzt1.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>> How about this?
>>
>> A patch on top could change the default "git-shell-commands is not
>> present" message if that seems worthwhile.
>
> Hmph.
>
> I wonder if rewording the message when git-shell-commmands directory
> is not there may be a better first step (which actually could be the
> last step)?
Maybe, but it's not a step that I'm interested in. I don't think it
changes the desirability of the patch I sent. They are independent.
[...]
> --- a/shell.c
> +++ b/shell.c
> @@ -162,9 +162,11 @@ int main(int argc, char **argv)
> /* Allow the user to run an interactive shell */
> cd_to_homedir();
> if (access(COMMAND_DIR, R_OK | X_OK) == -1) {
> - die("Interactive git shell is not enabled.\n"
> + die("The user has been recognized as '%s' but "
> + "interactive git shell is not enabled.\n"
> "hint: ~/" COMMAND_DIR " should exist "
> - "and have read and execute access.");
> + "and have read and execute access.",
> + get_user_name());
Personally I don't think the hint should be here at all (it should be
obvious that git-shell(1) is the place to read about the login
behavior of an account with shell set to git-shell), but I don't mind
as long as it's possible to override the message.
Thanks,
Jonathan
^ permalink raw reply
* Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell
From: Jeff King @ 2013-02-11 4:17 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Sitaram Chamarty, Junio C Hamano, Ethan Reesor, git,
Ramkumar Ramachandra, Greg Brockman
In-Reply-To: <20130211041404.GA15329@elie.Belkin>
On Sun, Feb 10, 2013 at 08:14:04PM -0800, Jonathan Nieder wrote:
> Jeff King wrote:
> > On Sun, Feb 10, 2013 at 05:20:16PM -0800, Jonathan Nieder wrote:
>
> >> +When run interactively (with no arguments), 'git-shell' will
> >> +automatically run `~/git-shell-commands/help` on startup, provided it
> >> +exists. If the 'help' command fails then the interactive shell is
> >> +aborted.
> >
> > Doesn't that mean that people who currently do allow interactive access
> > and have a ~/git-shell-commands/help (that returns zero) will get
> > spammed by its as a motd each time they connect?
>
> Only interactive connections. That's the existing behavior.
Ah, sorry. I misread the patch. I see now that we already run help, and
this is just making the exit value significant. In that case, yeah, I
think it's fine.
-Peff
^ permalink raw reply
* Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell
From: Jonathan Nieder @ 2013-02-11 4:26 UTC (permalink / raw)
To: Jeff King
Cc: Sitaram Chamarty, Junio C Hamano, Ethan Reesor, git,
Ramkumar Ramachandra, Greg Brockman
In-Reply-To: <20130211041714.GA12281@sigill.intra.peff.net>
Jeff King wrote:
> On Sun, Feb 10, 2013 at 08:14:04PM -0800, Jonathan Nieder wrote:
>> Only interactive connections. That's the existing behavior.
>
> Ah, sorry. I misread the patch. I see now that we already run help, and
> this is just making the exit value significant. In that case, yeah, I
> think it's fine.
No problem --- the description was unclear. Would retitling the patch
to "shell: pay attention to exit status from 'help' command" work?
^ permalink raw reply
* Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell
From: Junio C Hamano @ 2013-02-11 4:30 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Sitaram Chamarty, Jeff King, Ethan Reesor, git,
Ramkumar Ramachandra, Greg Brockman
In-Reply-To: <20130211041706.GB15329@elie.Belkin>
Jonathan Nieder <jrnieder@gmail.com> writes:
> Junio C Hamano wrote:
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>>> How about this?
>>>
>>> A patch on top could change the default "git-shell-commands is not
>>> present" message if that seems worthwhile.
>>
>> Hmph.
>>
>> I wonder if rewording the message when git-shell-commmands directory
>> is not there may be a better first step (which actually could be the
>> last step)?
>
> Maybe, but it's not a step that I'm interested in. I don't think it
> changes the desirability of the patch I sent. They are independent.
What I thought I read in the log message was that you wanted to give
a better message telling the users that the site does _not_ allow an
interactive shell access. I do not see how that is independent from
a message given from this codepath, where the side has forbidden
shell access by not having ~/git-shell-commands directory in the
first place. Are you shooting for customizability?
^ permalink raw reply
* Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell
From: Jonathan Nieder @ 2013-02-11 4:32 UTC (permalink / raw)
To: Junio C Hamano
Cc: Sitaram Chamarty, Jeff King, Ethan Reesor, git,
Ramkumar Ramachandra, Greg Brockman
In-Reply-To: <7vwqufpj50.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Are you shooting for customizability?
Yes, and the ability to generate the message dynamically.
^ 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