Git development
 help / color / mirror / Atom feed
* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users
From: Sven Strickroth @ 2011-12-28 21:47 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Jakub Narebski, Jeff King
In-Reply-To: <7vpqf8z8a6.fsf@alter.siamese.dyndns.org>

Am 28.12.2011 22:38 schrieb Junio C Hamano:
> I am however not sure if the second patch in this series is a good thing
> in the current shape. For GUI users who do not have a terminal, earlier
> they couldn't respond to these questions but now they can, so in that
> narrow sense we are not going backwards.
> 
> But for people who use *_ASKPASS and are working from the terminal, it is
> a regression to ask these non-password questions using *_ASKPASS. Most
> likely, these helpers that are designed for password entry will hide what
> is typed, and I also wouldn't be surprised if some of them have fairly low
> input-length restriction that may be shorter than a long-ish pathname that
> users might want to give as an answer, which they could do in the terminal
> based interaction but will become impossible with this patch.
> 
> I suspect that we would need to enhance *_ASKPASS interface first, so that
> we can ask things other than passwords. Until that happens, I do not think
> we should apply the second patch to use *_ASKPASS for non-passwords.

git-core also asks for username using *_ASKPASS, this is the reason why
I implemented it this way. I noticed it when I tried to push to google
code (using https).

-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

^ permalink raw reply

* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users
From: Junio C Hamano @ 2011-12-28 21:38 UTC (permalink / raw)
  To: git; +Cc: Sven Strickroth, Jakub Narebski, Jeff King
In-Reply-To: <7v39c41keo.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

> I actually was hoping that the answer is "it depends on the helper
> specified by *_ASKPASS".
>
> In any case, let's not add that extra "Certificate unknown. " prefix at
> all to avoid regressions and queue this patch series for real.
>
> After somebody comes up with a way to deal with overlong prompt, building
> on top of this series, we can work on making this particular prompt longer
> and more descriptive.

I've queued the two patches with minor tweaks.

I think the first patch is a definite improvement for both GUI users and
terminal users who use the *_ASKPASS environment variable. Other parts of
git already asks the latter their password using *_ASKPASS anyway, so I do
not foresee complaints from them saying that git-svn suddenly stopped
reading the password from the terminal.

I am however not sure if the second patch in this series is a good thing
in the current shape. For GUI users who do not have a terminal, earlier
they couldn't respond to these questions but now they can, so in that
narrow sense we are not going backwards.

But for people who use *_ASKPASS and are working from the terminal, it is
a regression to ask these non-password questions using *_ASKPASS. Most
likely, these helpers that are designed for password entry will hide what
is typed, and I also wouldn't be surprised if some of them have fairly low
input-length restriction that may be shorter than a long-ish pathname that
users might want to give as an answer, which they could do in the terminal
based interaction but will become impossible with this patch.

I suspect that we would need to enhance *_ASKPASS interface first, so that
we can ask things other than passwords. Until that happens, I do not think
we should apply the second patch to use *_ASKPASS for non-passwords.

^ permalink raw reply

* Re: [PATCH] gc --auto: warn garbage collection happens soon
From: Jeff King @ 2011-12-28 21:30 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Nguyễn Thái Ngọc Duy, git
In-Reply-To: <7vfwg41n3p.fsf@alter.siamese.dyndns.org>

On Wed, Dec 28, 2011 at 12:02:18PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > [1] Actually, it's not just having objects. You may have just exploded
> >     unreachable objects from a pack, but they are still younger than the
> >     2 week expiration period. Therefore trying to prune them is
> >     pointless, because even if they are unreachable, you won't delete
> >     them. So you really want to say "how many actual candidate objects
> >     do we have for pruning?"
> 
> An obvious knee-jerk reaction is "Ugh, if we have very recently repacked,
> don't we know what are reachable and what are not already, and use that
> knowledge while pruning to avoid traversing everything again?"

Especially now that prune has learned about progress reporting, it's
easy to see in "git gc" that the "Counting objects" phase of the repack
and the connectivity search in prune are counting the same objects.  It
would obviously be easy to just dump the set of sha1s in packed binary
format, and let git-prune reference that.

But it doesn't work in the general case. Running "git gc" will repack
everything, and so it looks at all reachable objects. But "git gc
--auto" will typically do an incremental pack (unless you have too many
packs), which means its counting objects phase only looks at part of
the graph.  So that result can't be used for object reachability, since
many objects won't be marked[1].

So yes, it's an optimization we can do, but it only works some of the
time. And worse, it works in the time we care less (when we are doing a
full repack anyway, so we are already spending more time counting
objects, and more I/O rewriting existing packed objects), but not when
we want it most (doing a few seconds of incremental repack during "git
gc --auto", which balloons to a minute because of the prune time).

-Peff

[1] It's tempting to say "well, we just repacked incrementally, so if
    something was referenced and not packed, we would have just packed
    it, right?" But look at how incremental packing works. We do a
    traversal with "--unpacked", which means we don't dig down past
    commit objects that are already packed. And that's why its fast.

    But packs don't necessarily respect reachability. It's possible for
    you to have object X in a pack, but X^{tree} is not (or X^, or
    whatever)[2]. I believe using "git repack" would fail to actually
    pack that. But that's OK, because it almost never happens, and the
    worst case is that the object doesn't get packed until you do a full
    repack.

    But I'm not sure you would want the same level of shortcut for
    git-prune, which would actually be _deleting_ the object. We want to
    be very sure in that case.

[2] The obvious way to get into this situation is to give weird rev-list
    parameters to pack-objects. But I think you could also do it
    accidentally by having commit X loose, then fetching history
    containing commit Y that builds on X. If the fetch is big enough,
    we'll keep the pack that we got from the other side. So X remains
    loose, but its ancestors are packed. Running an incremental repack
    will stop the traversal at Y and never consider X for packing.

    I didn't actually test this, but that's my reading of the code (see
    the revs->unpacked check in revision.c:get_commit_action).

^ permalink raw reply

* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users
From: Junio C Hamano @ 2011-12-28 21:00 UTC (permalink / raw)
  To: Sven Strickroth; +Cc: git, Jakub Narebski, Jeff King
In-Reply-To: <4EFAF241.9050806@tu-clausthal.de>

Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:

> Am 28.12.2011 03:41 schrieb Junio C Hamano:
>> I am afraid the extra "Certificate unknown. " prefix may make the prompt
>> way too long to fit on a line on the terminal or in the GUI. Would it be
>> Ok to perhaps add LF to make it a multi-line prompt? Do GUI based helpers
>> make that into a dialog box with multi-line prompt, or do they just barf?
>
> LF is problematic. But we could do $prompt =~ s/\n/ /g; in _prompt()-method.

I actually was hoping that the answer is "it depends on the helper
specified by *_ASKPASS".

In any case, let's not add that extra "Certificate unknown. " prefix at
all to avoid regressions and queue this patch series for real.

After somebody comes up with a way to deal with overlong prompt, building
on top of this series, we can work on making this particular prompt longer
and more descriptive.

^ permalink raw reply

* Re: GIT and SSH
From: Thomas Hochstein @ 2011-12-28 11:34 UTC (permalink / raw)
  To: git
In-Reply-To: <loom.20111228T091942-66@post.gmane.org>

Reza Mostafid schrieb:

> a.) Does the communication that takes place between a GIT `client` and a remote
>     GIT `repository` involve 'ssh' traffic?

Depends on how you do it (as already described by others).

> Our connections here are heavily censored and ssh traffic is suppressed most of
> the time which is why I need to figure out why a simple  
>
>     $ git clone git://<URL> 
>
> command chokes to a halt and freezes most of the time ( the same command when
> executed remotely on our V.P.S. in Europe works flawlessly ).

"git://" uses the git protocol which does not involve SSH.

    $ git clone ssh://<URL>

would use SSH.

-thh

^ permalink raw reply

* Re: [PATCH] gc --auto: warn garbage collection happens soon
From: Junio C Hamano @ 2011-12-28 20:02 UTC (permalink / raw)
  To: Jeff King; +Cc: Nguyễn Thái Ngọc Duy, git
In-Reply-To: <20111228184000.GA18780@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> [1] Actually, it's not just having objects. You may have just exploded
>     unreachable objects from a pack, but they are still younger than the
>     2 week expiration period. Therefore trying to prune them is
>     pointless, because even if they are unreachable, you won't delete
>     them. So you really want to say "how many actual candidate objects
>     do we have for pruning?"

An obvious knee-jerk reaction is "Ugh, if we have very recently repacked,
don't we know what are reachable and what are not already, and use that
knowledge while pruning to avoid traversing everything again?"

My memory around repack, fsck and prune needs refreshing, though, to tell
if that suggestion is feasible.

^ permalink raw reply

* Re: Possible submodule or submodule documentation issue
From: Jens Lehmann @ 2011-12-28 19:47 UTC (permalink / raw)
  To: Bill Zaumen; +Cc: git
In-Reply-To: <1325013859.1987.65.camel@yos>

Am 27.12.2011 20:24, schrieb Bill Zaumen:
> For the 'add' command, the man page for get-submodule states
> 
> "<repository> is the URL of the new submodule’s origin repository. This
> may be either an absolute URL, or (if it begins with ./ or ../), the
> location relative to the superproject’s origin repository."
> 
> and
> 
> "In either case, the given URL is recorded into .gitmodules for use by
> subsequent users cloning the superproject. If the URL is given relative
> to the superproject’s repository, the presumption is the superproject
> and submodule repositories will be kept together in the same relative
> location, and only the superproject’s URL needs to be provided:
> git-submodule will correctly locate the submodule using the relative URL
> in .gitmodules."
> 
> Based on that documentation, I tried the following sequence of commands:
> 
> git init --bare library.git
> git init --bare library-pkg.git
> git clone library.git
> cd library;
> echo hello > hello
> git add hello
> git commit -m "initial"
> git push origin master
> cd ..
> git clone library-pkg.git

I assume you did forget to add a "cd library-pkg" here.

> echo goodbye > goodbye
> git add goodbye
> git submodule add ./library.git src   <FAILS>

With Git 1.7.1 I get the following error message:

fatal: '/tmp/library-pkg.git/library.git' does not appear to be a git repository
fatal: The remote end hung up unexpectedly
Clone of '/tmp/library-pkg.git/library.git' into submodule path 'src' failed

Which is rather what I would have expected: Using "./library.git src"
implies the library living *inside* the "library-pkg.git" repo, not
next to it (and the error message shows that). "../library.git" is the
correct relative path, so the next command works as expected.

> git submodule add ../library.git src  <WORKS>
> git commit -a -m "initial pkg"
> git push origin master
> 
> The documentation as written suggests that the first use of 
> "git submodule add" is the one that should have worked because
> library.git is in the same directory as the origin repository
> library-pgk.git .  I didn't try moving the two .git directories
> to a server to see if the behavior is the same in that case (my
> test was using the local file system).

Hmm, the documentation says "the location relative to the
superproject’s origin repository", not the directory containing
it. This means you have to use ".." first to get out of the
repository itself, no?

^ permalink raw reply

* Re: [PATCH] add post-fetch hook
From: Joey Hess @ 2011-12-28 19:30 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Junio C Hamano, git
In-Reply-To: <4EFA3833.80409@kdbg.org>

[-- Attachment #1: Type: text/plain, Size: 3821 bytes --]

Johannes Sixt wrote:
> If I read the loop below correctly, you should be able to run it using
> only the functions sha1_to_hex(), strlen() and write_in_full(). This
> would avoid any problems with concurrent calls to xmalloc().
> 
> > +	int ret;
> > +
> > +	for (ref = post_fetch_hook_refs; ref; ref = ref->next) {
> > +		strbuf_addstr(&buf, sha1_to_hex(ref->old_sha1));
> 
> sha1_to_hex() works with a static buffer. Are you certain that it is not
> called concurrently in the main thread?

Thanks very much for pointing that thread-unsafty out.

Based on that, my current thinking for a generic hook interface is that,
rather than the caller providing an arbitrary "feeder" function that gets
run async, the caller should provide a function that generates a strbuf
containing the stdout for the hook, and then a very simple async writer
can handle the actual writing.

static int feed_hook(int in, int out, void *data)
{
        struct strbuf *buf = data;
        return write_in_full(out, buf->buf, buf->len) != buf->len;
}

(I assume that write_in_full is safe to be run async?)

I am working on a patch that will involve adding a hook_complex()
and changing hook() to be implemented in terms of it. The header
for that is included below, you should get a very good idea of how
it will work from the data structure.

/*
 * This data structure controls how a hook is run.
 */
struct hook {
	/* The name of the hook being run. */
	const char *name;
	/* Parameters to pass to the hook program, not including the name
	 * of the hook. May be NULL. */
	struct argv_array *argv_array;
	/* Pathname to an index file to use, or NULL if the hook
	 * uses the default index file or no index is needed. */
	const char *index_file;
	/*
	 * An arbitrary data structure, can be populated and modified to
	 * communicate between the feeder, reader, and caller of the hook.
	 */
	void *data;
	/* 
	 * A hook can optionally not consume all of its stdin.
	 * If partial_stdin is 0, it is an error for some stdin not
	 * to be consumed.
	 */
	int partial_stdin;
	/* 
	 * feeder populates a strbuf with the content to send to the
	 * hook on its standard input.
	 *
	 * May be NULL, if the hook does not consume standard input.
	 *
	 * Note that feeder might be run more than once, if multiple
	 * programs are run as part of a single hook. It should avoid
	 * taking any actions except for reading from data and generating
	 * the strbuf. It will *not* be run async, and need not worry
	 * about contending with other threads.
	 */
	struct strbuf *(*feeder)(struct hook *hook);
	/*
	 * reader processes the hook's standard output from the handle,
	 * returning 0 on success, non-zero on failure.
	 *
	 * May be NULL, if the hook's stdin is not processed. (It will
	 * instead be redirected to stderr.)
	 *
	 * Note that reader might be run more than once, if multiple
	 * programs are run as part of a single hook. It should avoid
	 * taking any actions except for reading from the input handle,
	 * changing the content of data, and printing any necessary
	 * warnings. It will *not* be run async, and need not worry
	 * about contending with other threads.
	 */
	int (*reader)(struct hook *hook, int handle);
};

extern int run_hook(const char *index_file, const char *name, ...);

extern int run_hook_complex(struct hook *hook);


This design allows for a future where multiple scripts get run for a
single hook. In that case, the feeder and reader functions would get
called repeatedly in a loop, with a data flow like this, where the
reader modifies hook.data, providing the next call of the feeder with
the new data read from the hook script:
    feeder | hook_script_1 | reader | feeder | hook_script_2 | reader

-- 
see shy jo

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

^ permalink raw reply

* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS
From: Jakub Narebski @ 2011-12-28 18:56 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Sven Strickroth, git, Jeff King
In-Reply-To: <7vboqt2zm4.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:
> Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:

> I only have a few minor nits, and request for extra set of eyeballs from
> Perl-y people.
> 
> >  sub _read_password {
> >  	my ($prompt, $realm) = @_;
> > -	my $password = '';
> > -	if (exists $ENV{GIT_ASKPASS}) {
> > -		open(PH, "-|", $ENV{GIT_ASKPASS}, $prompt);
> > -		$password = <PH>;
> > -		$password =~ s/[\012\015]//; # \n\r
> > - ...
> > -		while (defined(my $key = Term::ReadKey::ReadKey(0))) {
> > -			last if $key =~ /[\012\015]/; # \n\r
> > -			$password .= $key;
> > -		}
> > - ...
> > +	my $password = Git->prompt($prompt);
> >  	$password;
> >  }
> > ...
> > +Check if GIT_ASKPASS or SSH_ASKPASS is set, use first matching for querying
> > +user and return answer. If no *_ASKPASS variable is set, the variable is
> > +empty or an error occoured, the terminal is tried as a fallback.
> 
> Looks like a description that is correct, but I feel a slight hiccup when
> trying to read the first sentence aloud.  Perhaps other reviewers on the
> list can offer an easier to read alternative?

Perhaps

  Query user for password with given PROMPT and return answer.  It respects
  GIT_ASKPASS and SSH_ASKPASS environment variables, with terminal in a
  password mode (no echo) as a fallback.  Returns undef if it cannot ask
  for password. 

> > +sub prompt {
> > +	my ($self, $prompt) = _maybe_self(@_);
> > +	my $ret;
> > +	if (exists $ENV{'GIT_ASKPASS'}) {

Wouldn't it be simpler and more resilent to just check for $ENV{'GIT_ASKPASS'}?
Assuming that nobody uses command named '0' it would cover both GIT_ASKPASS
not being set (!exists) and being set to empty value (eq '').

> > +		$ret = _prompt($ENV{'GIT_ASKPASS'}, $prompt);
> > +	}
> > +	if (!defined $ret && exists $ENV{'SSH_ASKPASS'}) {
> > +		$ret = _prompt($ENV{'SSH_ASKPASS'}, $prompt);
> > +	}
> > +	if (!defined $ret) {
> > +		print STDERR $prompt;
> > +		STDERR->flush;
> > +		require Term::ReadKey;
> > +		Term::ReadKey::ReadMode('noecho');
> > +		while (defined(my $key = Term::ReadKey::ReadKey(0))) {
> > +			last if $key =~ /[\012\015]/; # \n\r
> > +			$ret .= $key;

I wonder if the last part wouldn't be better to be refactored into
a separate subroutine, e.g. _prompt_readkey.

> 
> Unlike the original in _read_password, $ret ($password over there) is left
> "undef" here; I am wondering if "$ret .= $key" might trigger a warning and
> if that is the case, probably we should have an explicit "$ret = '';"
> before going into the while loop.

No that is not a problem.  In Perl undefined variable functions as 0 in
numeric context ($foo++), as '' in string context ($foo .= $key), and []
in arrayref context (push @$foo, $key).

> > +sub _prompt {
> > +	my ($askpass, $prompt) = @_;
> > +	unless ($askpass) {
> > +		return undef;
> > +	}
> 
> Perl gurus on the list might prefer to rewrite this with statement
> modifier as "return undef unless (...);" but I am not one of them.
> 
> > +	my $ret;
> > +	open my $fh, "-|", $askpass, $prompt || return undef;
> 
> I am so used see this spelled with the lower-precedence "or" like this
> 
> 	open my $fh, "-|", $askpass, $prompt
>         	or return undef;
> 
> that I am no longer sure if the use of "||" is Ok here. Help from Perl
> gurus on the list?

It is incorrect, which you can check with B::Deparse.

$ perl -MO=Deparse,-p -e 'open my $fh, "-|", $askpass, $prompt || return undef;'

  open(my $fh, '-|', $askpass, ($prompt || return(undef)));
 
Anyway, wouldn't it be simpler and better to use command_oneline or its
backend here?

> > +	$ret = <$fh>;
> > +	$ret =~ s/[\012\015]//g; # strip \n\r, chomp does not work on all systems (i.e. windows) as expected
> 
> The original reads one line from the helper process, removes the first \n
> or \r (expecting there is only one), and returns the result. The new code
> reads one line, removes all \n and \r everywhere, and returns the result.
> 
> I do not think it makes any difference in practice, but shouldn't this
> logically be more like "s/\r?\n$//", that is "remove the CRLF or LF at the
> end"?
> 
> > +	close ($fh);
> 
> It seems that we aquired a SP after "close" compared to the
> original. What's the prevailing coding style in our Perl code?
> 
> This close() of pipe to the subprocess is where a lot of error checking
> happens, no? Can this return an error?
> 
> I can see the original ignored an error condition, but do we care, or not
> care?
 
If we use command_oneline or its backend we wouldn't have to worry
about this.

-- 
Jakub Narebski
Poland

^ permalink raw reply

* Re: [PATCH] gc --auto: warn garbage collection happens soon
From: Jeff King @ 2011-12-28 18:40 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Nguyễn Thái Ngọc Duy, git
In-Reply-To: <7vpqf94r8c.fsf@alter.siamese.dyndns.org>

On Tue, Dec 27, 2011 at 01:52:35PM -0800, Junio C Hamano wrote:

> And if the answer to that tongue-in-cheek question is no, what is the
> reason why the users will not find the messages disturbing, while loathing
> the auto-gc?
> 
> I suspect that is because auto-gc takes long time, making the user wait,
> compared to the new message that may be noisy but quick.  Perhaps the real
> cure for the disease is not to add the message but to make an auto-gc less
> painful, no?
> 
> What are the things we could do to make auto-gc less painful?
>
> Are we doing something that is not necessary in auto-gc that takes time
> but that we can live without doing?

I don't personally find gc all that painful (though maybe that is
because I tend to gc myself and rarely hit the auto-gc), but I have
noticed that git-prune takes by far the most time to run. If you are
just doing an incremental pack, you might be packing only a few thousand
objects and not touching old history at all (and with many cores, the
delta compression flies by). But prune requires running "git rev-list
--objects --all", which takes something like 45 seconds for linux-2.6 on
my fast-ish laptop (and about 23 seconds for git.git).

We could perhaps cut out pruning in the auto-gc case unless there are a
lot of objects left over after the packing phase. It's not worth doing a
full prune to clean up a dozen objects[1]. It probably is if you have a
thousand objects left after packing.

-Peff

[1] Actually, it's not just having objects. You may have just exploded
    unreachable objects from a pack, but they are still younger than the
    2 week expiration period. Therefore trying to prune them is
    pointless, because even if they are unreachable, you won't delete
    them. So you really want to say "how many actual candidate objects
    do we have for pruning?"

^ permalink raw reply

* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS
From: Sven Strickroth @ 2011-12-28 16:17 UTC (permalink / raw)
  To: git; +Cc: Jakub Narebski, Jeff King
In-Reply-To: <7vboqt2zm4.fsf@alter.siamese.dyndns.org>

Am 28.12.2011 03:34 schrieb Junio C Hamano:
>> +	close ($fh);
> 
> It seems that we aquired a SP after "close" compared to the
> original. What's the prevailing coding style in our Perl code?
> 
> This close() of pipe to the subprocess is where a lot of error checking
> happens, no? Can this return an error?
> 
> I can see the original ignored an error condition, but do we care, or not
> care?

close() can return a number in case of an error, but we already got our
response/line, so why care?

-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

^ permalink raw reply

* Re: GIT and SSH
From: Jakub Narebski @ 2011-12-28 11:02 UTC (permalink / raw)
  To: Carlos Martín Nieto; +Cc: Dov Grobgeld, Reza Mostafid, git
In-Reply-To: <20111228101512.GA2192@beez.lab.cmartin.tk>

Carlos Martín Nieto <cmn@elego.de> writes:
> On Wed, Dec 28, 2011 at 11:55:24AM +0200, Dov Grobgeld wrote:

> > Git supports multiple transport protocols. Among them are git, ssh,
> > and https. (You can also use direct file system access, but it is
> > questionable whether to call that a protocol). Each of the protocols
> > have their advantages and drawbacks. The git protocol is only used for
> > reading, and not for writing, but is supposed to be very fast. The

FYI git:// protocol can theoretically be used for pushing, but it is
not recommended because git:// protocol is not authenthicated - that
is why you need to enable pushing via git:// explicitly.

Just git trivia.

> > common firewall filtering of the git protocol port 9418 is another
> > problem. ssh is the prefered protocol for writing to a remote
> > protocol. But if ssh is filtered, then http/https may be used, which
> > is very slow for large repositories, but it has the advantage that it
> > is the least blocked protocol.
> 
> Slow for large repositories? Are you thinking of the dumb HTTP
> transport? That one shouldn't be used for doing any serious work. The
> Smart HTTP transport is just the git smart protocol (the same one
> git:// and ssh:// URLs speak) wrapped inside HTTP communication. The
> negotiation phase is a more expensive than with either git or ssh, as
> HTTP is stateless and you need to remind the remote what you want and
> what you've already agreed on, but the actual transfer of data is done
> the same way than with the others so overall it shouldn't be that
> noticeable.

Note that for "smart" transports ("smart" HTTP, SSH) you need to have
git installed on server, or at least have git-upload-pack and
git-receive-pack somewhere (you can configure client where it is to be
found on server).
 
Note that git uses curl for both smart and dumb HTTP transports, so
things like 'http_proxy' and 'HTTPS_PROXY' environment variables
should work.

> Now, to the OP's concerns: yes, the ssh transport does generate ssh
> transport, as that's the whole point. You authenticate against the
> remote machine's ssh daemon and run git-upload-pack or
> git-receive-pack as needed (for fetching or pushing). If corporate
> policy doesn't allow ssh you should either fix the policy or use the
> smart HTTP protocol, though this involves messing with passwords and
> their associated problems. I'm not saying ssh keys don't have their
> complications, but I much prefer them.

Note that if the problem is giving shell accounts with SSH access, the
solution is to use one of git repository management tools, like
Gitosis (Python + setuptools, no longer developed) or Gitolite (Perl).
From what I remember both use _single_ *restricted* account and
public-key authenthication.
 
If I am not mistaken Gitolite can help with "smart" HTTP transport
access too.

> We can't help you diagnose why your clone is stalling without more
> information. It could be that the connection between the computers is
> flaky, git trying to take too much memory on the remote or local
> machines or any number of things. See if setting GIT_TRACE=1 in the
> environment helps to see what's going on.

Or even undocumented (!) GIT_TRACE_PACKET.

-- 
Jakub Narebski

^ permalink raw reply

* Re: GIT and SSH
From: Reza Mostafid @ 2011-12-28 11:01 UTC (permalink / raw)
  To: git
In-Reply-To: <loom.20111228T091942-66@post.gmane.org>

I forgot to mention, I am an embedded developer located in Iran.

The filtering I am talking about is by the government. All ISP's get their
bandwith from centrally allocated trunks. This allows control.

I know that ssh packets get dropped for extended periods frequently.
We have an Ubuntu virtual server outside of Iran which we use as a proxy and
connect to it via 'ssh' sox provxy ( -D 8090 ).

Many times some sort of script or intelligence is operation which severely
throttles the connection as soon as the data rate exceeds certain "benign" 
levels. All this has been confirmed by the network `gurus` and admin people I
work with. This is the best we can tell from what we observe.

As mentioned when we execute the simple GIT clone command from our VPS 
( located outside Iran ) the command works flawlessly.

What I would be interested in is to somehow make git avoid using transport
over ssh. The government censors are interested only in blocking people
accessing illicit sites via S.O.X-5 or VPN. 

To them anything over SSH is suspicious. If we could somehow update using a
plain transport method, they couldn't care less about source code being sent to 
us.

Regards

Reza

^ permalink raw reply

* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users
From: Sven Strickroth @ 2011-12-28 10:41 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Jakub Narebski, Jeff King
In-Reply-To: <7vpqf91kqo.fsf@alter.siamese.dyndns.org>

Am 28.12.2011 03:41 schrieb Junio C Hamano:
> I am afraid the extra "Certificate unknown. " prefix may make the prompt
> way too long to fit on a line on the terminal or in the GUI. Would it be
> Ok to perhaps add LF to make it a multi-line prompt? Do GUI based helpers
> make that into a dialog box with multi-line prompt, or do they just barf?

LF is problematic. But we could do $prompt =~ s/\n/ /g; in _prompt()-method.

-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

^ permalink raw reply

* Re: GIT and SSH
From: Carlos Martín Nieto @ 2011-12-28 10:15 UTC (permalink / raw)
  To: Dov Grobgeld; +Cc: Reza Mostafid, git
In-Reply-To: <CA++fsGFOC6bV4gC+ozBKP3EmoAX4CcfTrHjjpMWPkh7vYOfgAw@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 2104 bytes --]

On Wed, Dec 28, 2011 at 11:55:24AM +0200, Dov Grobgeld wrote:
> Git supports multiple transport protocols. Among them are git, ssh,
> and https. (You can also use direct file system access, but it is
> questionable whether to call that a protocol). Each of the protocols
> have their advantages and drawbacks. The git protocol is only used for
> reading, and not for writing, but is supposed to be very fast. The
> common firewall filtering of the git protocol port 9418 is another
> problem. ssh is the prefered protocol for writing to a remote
> protocol. But if ssh is filtered, then http/https may be used, which
> is very slow for large repositories, but it has the advantage that it
> is the least blocked protocol.

Slow for large repositories? Are you thinking of the dumb HTTP
transport? That one shouldn't be used for doing any serious work. The
Smart HTTP transport is just the git smart protocol (the same one
git:// and ssh:// URLs speak) wrapped inside HTTP communication. The
negotiation phase is a more expensive than with either git or ssh, as
HTTP is stateless and you need to remind the remote what you want and
what you've already agreed on, but the actual transfer of data is done
the same way than with the others so overall it shouldn't be that
noticeable.

Now, to the OP's concerns: yes, the ssh transport does generate ssh
transport, as that's the whole point. You authenticate against the
remote machine's ssh daemon and run git-upload-pack or
git-receive-pack as needed (for fetching or pushing). If corporate
policy doesn't allow ssh you should either fix the policy or use the
smart HTTP protocol, though this involves messing with passwords and
their associated problems. I'm not saying ssh keys don't have their
complications, but I much prefer them.

We can't help you diagnose why your clone is stalling without more
information. It could be that the connection between the computers is
flaky, git trying to take too much memory on the remote or local
machines or any number of things. See if setting GIT_TRACE=1 in the
environment helps to see what's going on.

   cmn

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

^ permalink raw reply

* Re: GIT and SSH
From: Dov Grobgeld @ 2011-12-28  9:55 UTC (permalink / raw)
  To: Reza Mostafid; +Cc: git
In-Reply-To: <loom.20111228T091942-66@post.gmane.org>

Git supports multiple transport protocols. Among them are git, ssh,
and https. (You can also use direct file system access, but it is
questionable whether to call that a protocol). Each of the protocols
have their advantages and drawbacks. The git protocol is only used for
reading, and not for writing, but is supposed to be very fast. The
common firewall filtering of the git protocol port 9418 is another
problem. ssh is the prefered protocol for writing to a remote
protocol. But if ssh is filtered, then http/https may be used, which
is very slow for large repositories, but it has the advantage that it
is the least blocked protocol.

For more info see:

    http://progit.org/book/ch4-1.html

Regards,
Dov


On Wed, Dec 28, 2011 at 10:43, Reza Mostafid <m.r.mostafid@gmail.com> wrote:
> I am starting to use GIT and I would be grateful for a simple answer to a
> specific situation to help me find the right ball-park.
>
>
> a.) Does the communication that takes place between a GIT `client` and a remote
>    GIT `repository` involve 'ssh' traffic?
>
>
> Our connections here are heavily censored and ssh traffic is suppressed most of
> the time which is why I need to figure out why a simple
>
>    $ git clone git://<URL>
>
> command chokes to a halt and freezes most of the time ( the same command when
> executed remotely on our V.P.S. in Europe works flawlessly ).
>
>
> b.) Are there means to make the `git` client on my machine circumvent this?
>
>
> Y/N answers or brief hints to my questions suffice, I'll work out the rest.
>
> Basically I would like to know whether there is a point at all trying to make
> git work from where I am, given the limitations mentioned.
>
> Regards
>
> Reza
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* GIT and SSH
From: Reza Mostafid @ 2011-12-28  8:43 UTC (permalink / raw)
  To: git

I am starting to use GIT and I would be grateful for a simple answer to a
specific situation to help me find the right ball-park.


a.) Does the communication that takes place between a GIT `client` and a remote
    GIT `repository` involve 'ssh' traffic?


Our connections here are heavily censored and ssh traffic is suppressed most of
the time which is why I need to figure out why a simple  

    $ git clone git://<URL> 

command chokes to a halt and freezes most of the time ( the same command when
executed remotely on our V.P.S. in Europe works flawlessly ).
 

b.) Are there means to make the `git` client on my machine circumvent this?


Y/N answers or brief hints to my questions suffice, I'll work out the rest. 

Basically I would like to know whether there is a point at all trying to make
git work from where I am, given the limitations mentioned.

Regards

Reza

^ permalink raw reply

* Re: Git beginner - Need help understanding
From: Dov Grobgeld @ 2011-12-28  8:05 UTC (permalink / raw)
  To: chirin; +Cc: git
In-Reply-To: <1325036313909-7131536.post@n2.nabble.com>

I'm glad to hear that your problem was solved.

On Wed, Dec 28, 2011 at 03:38, chirin <takonatto@gmail.com> wrote:
> [stuff deleted]
> What should I know about the 'states of A, B and the repository A and B
> interact with'?

The state of a repository are most easily described by the sequence of
commands done to it from its creation.  The best way of learning and
solving problems is trying to reproduce them with the minimum amount
of steps. I.e. even if you are using a GUI tool for your convenience,
you should be familiar with the command line tools, since that is the
best way of conveying what you have done. My motto is: "Don't describe
in human language what you have done, but show me the exact
commands!". And there is no language better for such than the exact
command line.

Regards,
Dov
>
> --
> View this message in context: http://git.661346.n2.nabble.com/Git-beginner-Need-help-understanding-tp7129186p7131536.html
> Sent from the git mailing list archive at Nabble.com.
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users
From: Junio C Hamano @ 2011-12-28  2:41 UTC (permalink / raw)
  To: Sven Strickroth; +Cc: git, Jakub Narebski, Jeff King
In-Reply-To: <4EFA5F08.2060705@tu-clausthal.de>

Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:

> @@ -4357,11 +4357,10 @@ sub ssl_server_trust {
>  	                               issuer_dname fingerprint);
>  	my $choice;
>  prompt:
> -	print STDERR $may_save ?
> +	my $options = $may_save ?
>  	      "(R)eject, accept (t)emporarily or accept (p)ermanently? " :
>  	      "(R)eject or accept (t)emporarily? ";
> -	STDERR->flush;
> -	$choice = lc(substr(<STDIN> || 'R', 0, 1));
> +	$choice = substr(Git->prompt("Certificate unknown. " . $options) || 'R', 0, 1);

I am afraid the extra "Certificate unknown. " prefix may make the prompt
way too long to fit on a line on the terminal or in the GUI. Would it be
Ok to perhaps add LF to make it a multi-line prompt? Do GUI based helpers
make that into a dialog box with multi-line prompt, or do they just barf?

>  	if ($choice =~ /^t$/i) {
>  		$cred->may_save(undef);

We seem to have lost lc() there, but it probably is deliberate and
harmless, as the value is checked with /^x$/i later.

As we are making sure $choice has a single character anyway, I think that
checking with "=~ /^x$/i" is unnecessarily ugly and wrong, even though it
is obviously not the fault of this patch.

> @@ -4378,10 +4377,7 @@ prompt:
>  sub ssl_client_cert {
>  	my ($cred, $realm, $may_save, $pool) = @_;
>  	$may_save = undef if $_no_auth_cache;
> -	print STDERR "Client certificate filename: ";
> -	STDERR->flush;
> -	chomp(my $filename = <STDIN>);
> -	$cred->cert_file($filename);
> +	$cred->cert_file(Git->prompt("Client certificate filename: "));
>  	$cred->may_save($may_save);
>  	$SVN::_Core::SVN_NO_ERROR;
>  }

We may later add an option to "prompt" method to allow the caller to say
that we are asking for a filename, and let GUI prompt helper to run a file
picker, but I think that is outside the immediate scope of this patch.
Just a thing for the future to keep in mind.

This patch itself looks almost perfect to me (modulo the above minor
nits), and except that it textually depends on 1/2 that may need to be
updated.

Thanks.

^ permalink raw reply

* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS
From: Junio C Hamano @ 2011-12-28  2:34 UTC (permalink / raw)
  To: Sven Strickroth; +Cc: git, Jakub Narebski, Jeff King
In-Reply-To: <4EFA5EB3.4000802@tu-clausthal.de>

Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:

> git-svn reads passwords from an interactive terminal or by using
> GIT_ASKPASS helper tool. But if GIT_ASKPASS environment variable is not
> set, git-svn does not try to use SSH_ASKPASS as git-core does. This
> cause GUIs (w/o STDIN connected) to hang waiting forever for git-svn to
> complete (http://code.google.com/p/tortoisegit/issues/detail?id=967).
>
> Commit 56a853b62c0ae7ebaad0a7a0a704f5ef561eb795 tried to solve this
> issue, but was incomplete as described above.
>
> Instead of using hand-rolled prompt-response code that only works with
> the interactive terminal, a reusable prompt() method is introduced in
> this commit. This change also adds a fallback to SSH_ASKPASS.
>
> Signed-off-by: Sven Strickroth <email@cs-ware.de>
> ---

Thanks. Vastly more readable ;-)

I only have a few minor nits, and request for extra set of eyeballs from
Perl-y people.

>  sub _read_password {
>  	my ($prompt, $realm) = @_;
> -	my $password = '';
> -	if (exists $ENV{GIT_ASKPASS}) {
> -		open(PH, "-|", $ENV{GIT_ASKPASS}, $prompt);
> -		$password = <PH>;
> -		$password =~ s/[\012\015]//; # \n\r
> - ...
> -		while (defined(my $key = Term::ReadKey::ReadKey(0))) {
> -			last if $key =~ /[\012\015]/; # \n\r
> -			$password .= $key;
> -		}
> - ...
> +	my $password = Git->prompt($prompt);
>  	$password;
>  }
> ...
> +Check if GIT_ASKPASS or SSH_ASKPASS is set, use first matching for querying
> +user and return answer. If no *_ASKPASS variable is set, the variable is
> +empty or an error occoured, the terminal is tried as a fallback.

Looks like a description that is correct, but I feel a slight hiccup when
trying to read the first sentence aloud.  Perhaps other reviewers on the
list can offer an easier to read alternative?

> +sub prompt {
> +	my ($self, $prompt) = _maybe_self(@_);
> +	my $ret;
> +	if (exists $ENV{'GIT_ASKPASS'}) {
> +		$ret = _prompt($ENV{'GIT_ASKPASS'}, $prompt);
> +	}
> +	if (!defined $ret && exists $ENV{'SSH_ASKPASS'}) {
> +		$ret = _prompt($ENV{'SSH_ASKPASS'}, $prompt);
> +	}
> +	if (!defined $ret) {
> +		print STDERR $prompt;
> +		STDERR->flush;
> +		require Term::ReadKey;
> +		Term::ReadKey::ReadMode('noecho');
> +		while (defined(my $key = Term::ReadKey::ReadKey(0))) {
> +			last if $key =~ /[\012\015]/; # \n\r
> +			$ret .= $key;

Unlike the original in _read_password, $ret ($password over there) is left
"undef" here; I am wondering if "$ret .= $key" might trigger a warning and
if that is the case, probably we should have an explicit "$ret = '';"
before going into the while loop.

> +sub _prompt {
> +	my ($askpass, $prompt) = @_;
> +	unless ($askpass) {
> +		return undef;
> +	}

Perl gurus on the list might prefer to rewrite this with statement
modifier as "return undef unless (...);" but I am not one of them.

> +	my $ret;
> +	open my $fh, "-|", $askpass, $prompt || return undef;

I am so used see this spelled with the lower-precedence "or" like this

	open my $fh, "-|", $askpass, $prompt
        	or return undef;

that I am no longer sure if the use of "||" is Ok here. Help from Perl
gurus on the list?

> +	$ret = <$fh>;
> +	$ret =~ s/[\012\015]//g; # strip \n\r, chomp does not work on all systems (i.e. windows) as expected

The original reads one line from the helper process, removes the first \n
or \r (expecting there is only one), and returns the result. The new code
reads one line, removes all \n and \r everywhere, and returns the result.

I do not think it makes any difference in practice, but shouldn't this
logically be more like "s/\r?\n$//", that is "remove the CRLF or LF at the
end"?

> +	close ($fh);

It seems that we aquired a SP after "close" compared to the
original. What's the prevailing coding style in our Perl code?

This close() of pipe to the subprocess is where a lot of error checking
happens, no? Can this return an error?

I can see the original ignored an error condition, but do we care, or not
care?

^ permalink raw reply

* Re: Git beginner - Need help understanding
From: chirin @ 2011-12-28  1:38 UTC (permalink / raw)
  To: git
In-Reply-To: <7vr4zp7q15.fsf@alter.siamese.dyndns.org>

The problem was solved by using another Gerrit ID for remote.origin.url in
the config. I remain confused with Git. 


Junio C Hamano wrote
> 
> Compared to that, your version above does not say anything about what the
> state of A, B and the repository A and B interact with were in before the
> problem started, so even if Dob wanted to help you by trying to reproduce
> your situation, there is not enough information to do so.
> 

My apologies on being vague.. I'm here for learning purpose instead of
get-someone-to-help-me-solve purpose. :P I'm really keen on learning what to
look for, where to start looking to understand Git.

I'm having difficulties providing information that I do not yet know how to,
as I'm still at the stage where I'm studying simple terminologies like
origin, master, etc. and I tend to confuse myself more by comparing it to
SVN.

What should I know about the 'states of A, B and the repository A and B
interact with'? 

--
View this message in context: http://git.661346.n2.nabble.com/Git-beginner-Need-help-understanding-tp7129186p7131536.html
Sent from the git mailing list archive at Nabble.com.

^ permalink raw reply

* [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users
From: Sven Strickroth @ 2011-12-28  0:12 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Jakub Narebski, Jeff King
In-Reply-To: <7vty4l4rr8.fsf@alter.siamese.dyndns.org>

git-svn reads usernames and other user queries from an interactive
terminal. This cause GUIs (w/o STDIN connected) to hang waiting forever
for git-svn to complete (http://code.google.com/p/tortoisegit/issues/detail?id=967).

This change extends the Git->prompt method, so that it can also be used
for non password queries, and makes use of it instead of using
hand-rolled prompt-response code that only works with the interactive
terminal.

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
 git-svn.perl |   16 +++++-----------
 perl/Git.pm  |   25 +++++++++++++++----------
 2 files changed, 20 insertions(+), 21 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index bcee8aa..1f30dc2 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -4357,11 +4357,10 @@ sub ssl_server_trust {
 	                               issuer_dname fingerprint);
 	my $choice;
 prompt:
-	print STDERR $may_save ?
+	my $options = $may_save ?
 	      "(R)eject, accept (t)emporarily or accept (p)ermanently? " :
 	      "(R)eject or accept (t)emporarily? ";
-	STDERR->flush;
-	$choice = lc(substr(<STDIN> || 'R', 0, 1));
+	$choice = substr(Git->prompt("Certificate unknown. " . $options) || 'R', 0, 1);
 	if ($choice =~ /^t$/i) {
 		$cred->may_save(undef);
 	} elsif ($choice =~ /^r$/i) {
@@ -4378,10 +4377,7 @@ prompt:
 sub ssl_client_cert {
 	my ($cred, $realm, $may_save, $pool) = @_;
 	$may_save = undef if $_no_auth_cache;
-	print STDERR "Client certificate filename: ";
-	STDERR->flush;
-	chomp(my $filename = <STDIN>);
-	$cred->cert_file($filename);
+	$cred->cert_file(Git->prompt("Client certificate filename: "));
 	$cred->may_save($may_save);
 	$SVN::_Core::SVN_NO_ERROR;
 }
@@ -4404,9 +4400,7 @@ sub username {
 	if (defined $_username) {
 		$username = $_username;
 	} else {
-		print STDERR "Username: ";
-		STDERR->flush;
-		chomp($username = <STDIN>);
+		$username = Git->prompt("Username: ");
 	}
 	$cred->username($username);
 	$cred->may_save($may_save);
@@ -4415,7 +4409,7 @@ sub username {

 sub _read_password {
 	my ($prompt, $realm) = @_;
-	my $password = Git->prompt($prompt);
+	my $password = Git->prompt($prompt, 1);
 	$password;
 }

diff --git a/perl/Git.pm b/perl/Git.pm
index b1c7c50..62b824c 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -512,18 +512,19 @@ C<git --html-path>). Useful mostly only internally.
 sub html_path { command_oneline('--html-path') }


-=item prompt ( PROMPT )
+=item prompt ( PROMPT , ISPASSWORD )

 Query user C<PROMPT> and return answer from user.

 Check if GIT_ASKPASS or SSH_ASKPASS is set, use first matching for querying
 user and return answer. If no *_ASKPASS variable is set, the variable is
 empty or an error occoured, the terminal is tried as a fallback.
+If C<ISPASSWORD> is set and true, the terminal disables echo.

 =cut

 sub prompt {
-	my ($self, $prompt) = _maybe_self(@_);
+	my ($self, $prompt, $isPassword) = _maybe_self(@_);
 	my $ret;
 	if (exists $ENV{'GIT_ASKPASS'}) {
 		$ret = _prompt($ENV{'GIT_ASKPASS'}, $prompt);
@@ -534,15 +535,19 @@ sub prompt {
 	if (!defined $ret) {
 		print STDERR $prompt;
 		STDERR->flush;
-		require Term::ReadKey;
-		Term::ReadKey::ReadMode('noecho');
-		while (defined(my $key = Term::ReadKey::ReadKey(0))) {
-			last if $key =~ /[\012\015]/; # \n\r
-			$ret .= $key;
+		if (defined $isPassword && $isPassword) {
+			require Term::ReadKey;
+			Term::ReadKey::ReadMode('noecho');
+			while (defined(my $key = Term::ReadKey::ReadKey(0))) {
+				last if $key =~ /[\012\015]/; # \n\r
+				$ret .= $key;
+			}
+			Term::ReadKey::ReadMode('restore');
+			print STDERR "\n";
+			STDERR->flush;
+		} else {
+			chomp($ret = <STDIN>);
 		}
-		Term::ReadKey::ReadMode('restore');
-		print STDERR "\n";
-		STDERR->flush;
 	}
 	return $ret;
 }
-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

^ permalink raw reply related

* [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS
From: Sven Strickroth @ 2011-12-28  0:11 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Jakub Narebski, Jeff King
In-Reply-To: <7vty4l4rr8.fsf@alter.siamese.dyndns.org>

git-svn reads passwords from an interactive terminal or by using
GIT_ASKPASS helper tool. But if GIT_ASKPASS environment variable is not
set, git-svn does not try to use SSH_ASKPASS as git-core does. This
cause GUIs (w/o STDIN connected) to hang waiting forever for git-svn to
complete (http://code.google.com/p/tortoisegit/issues/detail?id=967).

Commit 56a853b62c0ae7ebaad0a7a0a704f5ef561eb795 tried to solve this
issue, but was incomplete as described above.

Instead of using hand-rolled prompt-response code that only works with
the interactive terminal, a reusable prompt() method is introduced in
this commit. This change also adds a fallback to SSH_ASKPASS.

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
 git-svn.perl |   20 +-------------------
 perl/Git.pm  |   51 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 51 insertions(+), 20 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index eeb83d3..bcee8aa 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -4415,25 +4415,7 @@ sub username {

 sub _read_password {
 	my ($prompt, $realm) = @_;
-	my $password = '';
-	if (exists $ENV{GIT_ASKPASS}) {
-		open(PH, "-|", $ENV{GIT_ASKPASS}, $prompt);
-		$password = <PH>;
-		$password =~ s/[\012\015]//; # \n\r
-		close(PH);
-	} else {
-		print STDERR $prompt;
-		STDERR->flush;
-		require Term::ReadKey;
-		Term::ReadKey::ReadMode('noecho');
-		while (defined(my $key = Term::ReadKey::ReadKey(0))) {
-			last if $key =~ /[\012\015]/; # \n\r
-			$password .= $key;
-		}
-		Term::ReadKey::ReadMode('restore');
-		print STDERR "\n";
-		STDERR->flush;
-	}
+	my $password = Git->prompt($prompt);
 	$password;
 }

diff --git a/perl/Git.pm b/perl/Git.pm
index f7ce511..b1c7c50 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -58,7 +58,7 @@ require Exporter;
                 command_output_pipe command_input_pipe command_close_pipe
                 command_bidi_pipe command_close_bidi_pipe
                 version exec_path html_path hash_object git_cmd_try
-                remote_refs
+                remote_refs prompt
                 temp_acquire temp_release temp_reset temp_path);


@@ -512,6 +512,55 @@ C<git --html-path>). Useful mostly only internally.
 sub html_path { command_oneline('--html-path') }


+=item prompt ( PROMPT )
+
+Query user C<PROMPT> and return answer from user.
+
+Check if GIT_ASKPASS or SSH_ASKPASS is set, use first matching for querying
+user and return answer. If no *_ASKPASS variable is set, the variable is
+empty or an error occoured, the terminal is tried as a fallback.
+
+=cut
+
+sub prompt {
+	my ($self, $prompt) = _maybe_self(@_);
+	my $ret;
+	if (exists $ENV{'GIT_ASKPASS'}) {
+		$ret = _prompt($ENV{'GIT_ASKPASS'}, $prompt);
+	}
+	if (!defined $ret && exists $ENV{'SSH_ASKPASS'}) {
+		$ret = _prompt($ENV{'SSH_ASKPASS'}, $prompt);
+	}
+	if (!defined $ret) {
+		print STDERR $prompt;
+		STDERR->flush;
+		require Term::ReadKey;
+		Term::ReadKey::ReadMode('noecho');
+		while (defined(my $key = Term::ReadKey::ReadKey(0))) {
+			last if $key =~ /[\012\015]/; # \n\r
+			$ret .= $key;
+		}
+		Term::ReadKey::ReadMode('restore');
+		print STDERR "\n";
+		STDERR->flush;
+	}
+	return $ret;
+}
+
+sub _prompt {
+	my ($askpass, $prompt) = @_;
+	unless ($askpass) {
+		return undef;
+	}
+	my $ret;
+	open my $fh, "-|", $askpass, $prompt || return undef;
+	$ret = <$fh>;
+	$ret =~ s/[\012\015]//g; # strip \n\r, chomp does not work on all systems (i.e. windows) as expected
+	close ($fh);
+	return $ret;
+}
+
+
 =item repo_path ()

 Return path to the git repository. Must be called on a repository instance.
-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

^ permalink raw reply related

* What's cooking in git.git (Dec 2011, #09; Tue, 27)
From: Junio C Hamano @ 2011-12-27 23:37 UTC (permalink / raw)
  To: git

Here are the topics that have been cooking.  Commits prefixed with '-' are
only in 'pu' (proposed updates) while commits prefixed with '+' are in
'next'.

By now I know a bit better than taking the lack of serious regression
reports during the holiday weekend as a sign of perfection of the upcoming
release, but I will tag -rc0 soonish anyway. As far as I can see the tip
of 'master' is feature complete for 1.7.9, modulo possible bugs and
regressions.

Here are the repositories that have my integration branches:

With maint, master, next, pu, todo:

        git://git.kernel.org/pub/scm/git/git.git
        git://repo.or.cz/alt-git.git
        https://code.google.com/p/git-core/
        https://github.com/git/git

With only maint and master:

        git://git.sourceforge.jp/gitroot/git-core/git.git
        git://git-core.git.sourceforge.net/gitroot/git-core/git-core

With all the topics and integration branches:

        https://github.com/gitster/git

The preformatted documentation in HTML and man format are found in:

        git://git.kernel.org/pub/scm/git/git-{htmldocs,manpages}.git/
        git://repo.or.cz/git-{htmldocs,manpages}.git/
        https://code.google.com/p/git-{htmldocs,manpages}.git/
        https://github.com/gitster/git-{htmldocs,manpages}.git/

--------------------------------------------------
[New Topics]

* jh/fetch-head-update (2011-12-27) 1 commit
 - write first for-merge ref to FETCH_HEAD first

Needs sign-off. I have squashed minimal fixes in.

* jv/maint-config-set (2011-12-27) 1 commit
  (merged to 'next' on 2011-12-27 at 551ac8f)
 + Fix an incorrect reference to --set-all.

Will merge to "master" before -rc0.

* nd/index-pack-no-recurse (2011-12-27) 4 commits
 - fixup! 3413d4d
 - index-pack: eliminate unlimited recursion in get_delta_base()
 - index-pack: eliminate recursion in find_unresolved_deltas
 - Eliminate recursion in setting/clearing marks in commit list

Expecting a reroll.

* ss/git-svn-askpass (2011-12-27) 5 commits
 - make askpass_prompt a global prompt method for asking users
 - ignore empty *_ASKPASS variables
 - honour *_ASKPASS for querying username and for querying further actions like unknown certificates
 - switch to central prompt method
 - add central method for prompting a user using GIT_ASKPASS or SSH_ASKPASS

Expecting a reroll.

--------------------------------------------------
[Graduated to "master"]

* ab/sun-studio-portability (2011-12-21) 3 commits
  (merged to 'next' on 2011-12-21 at 0cc5a63)
 + Appease Sun Studio by renaming "tmpfile"
 + Fix a bitwise negation assignment issue spotted by Sun Studio
 + Fix an enum assignment issue spotted by Sun Studio

* jn/maint-gitweb-utf8-fix (2011-12-19) 4 commits
  (merged to 'next' on 2011-12-20 at b816812)
 + gitweb: Fix fallback mode of to_utf8 subroutine
 + gitweb: Output valid utf8 in git_blame_common('data')
 + gitweb: esc_html() site name for title in OPML
 + gitweb: Call to_utf8() on input string in chop_and_escape_str()

* rr/revert-cherry-pick (2011-12-15) 6 commits
  (merged to 'next' on 2011-12-21 at d0428dc)
 + t3502, t3510: clarify cherry-pick -m failure
 + t3510 (cherry-pick-sequencer): use exit status
 + revert: simplify getting commit subject in format_todo()
 + revert: tolerate extra spaces, tabs in insn sheet
 + revert: make commit subjects in insn sheet optional
 + revert: free msg in format_todo()

* tr/bash-read-unescaped (2011-12-21) 1 commit
  (merged to 'next' on 2011-12-21 at de865c1)
 + bash completion: use read -r everywhere

* tr/doc-sh-setup (2011-12-20) 1 commit
  (merged to 'next' on 2011-12-21 at bd73695)
 + git-sh-setup: make require_clean_work_tree part of the interface

* tr/pty-all (2011-12-19) 1 commit
  (merged to 'next' on 2011-12-20 at 9b637d3)
 + test-terminal: set output terminals to raw mode

Kept only the second one from the original.

--------------------------------------------------
[Stalled]

* jc/advise-push-default (2011-12-18) 1 commit
 - push: hint to use push.default=upstream when appropriate

Peff had a good suggestion outlining an updated code structure so that
somebody new can try to dip his or her toes in the development. Any
takers?

Waiting for a reroll.

* mh/ref-api-rest (2011-12-12) 35 commits
 - repack_without_ref(): call clear_packed_ref_cache()
 - read_packed_refs(): keep track of the directory being worked in
 - is_refname_available(): query only possibly-conflicting references
 - refs: read loose references lazily
 - read_loose_refs(): take a (ref_entry *) as argument
 - struct ref_dir: store a reference to the enclosing ref_cache
 - sort_ref_dir(): take (ref_entry *) instead of (ref_dir *)
 - do_for_each_ref_in_dir*(): take (ref_entry *) instead of (ref_dir *)
 - add_entry(): take (ref_entry *) instead of (ref_dir *)
 - search_ref_dir(): take (ref_entry *) instead of (ref_dir *)
 - find_containing_direntry(): use (ref_entry *) instead of (ref_dir *)
 - add_ref(): take (ref_entry *) instead of (ref_dir *)
 - read_packed_refs(): take (ref_entry *) instead of (ref_dir *)
 - find_ref(): take (ref_entry *) instead of (ref_dir *)
 - is_refname_available(): take (ref_entry *) instead of (ref_dir *)
 - get_loose_refs(): return (ref_entry *) instead of (ref_dir *)
 - get_packed_refs(): return (ref_entry *) instead of (ref_dir *)
 - refs: wrap top-level ref_dirs in ref_entries
 - get_ref_dir(): keep track of the current ref_dir
 - do_for_each_ref(): only iterate over the subtree that was requested
 - refs: sort ref_dirs lazily
 - sort_ref_dir(): do not sort if already sorted
 - refs: store references hierarchically
 - refs.c: rename ref_array -> ref_dir
 - struct ref_entry: nest the value part in a union
 - check_refname_component(): return 0 for zero-length components
 - free_ref_entry(): new function
 - refs.c: reorder definitions more logically
 - is_refname_available(): reimplement using do_for_each_ref_in_array()
 - names_conflict(): simplify implementation
 - names_conflict(): new function, extracted from is_refname_available()
 - repack_without_ref(): reimplement using do_for_each_ref_in_array()
 - do_for_each_ref_in_arrays(): new function
 - do_for_each_ref_in_array(): new function
 - do_for_each_ref(): correctly terminate while processesing extra_refs

The API for extra anchoring points may require rethought first; that would
hopefully make the "ref" part a lot simpler.

Waiting for a reroll.

* jc/split-blob (2011-12-01) 6 commits
 . WIP (streaming chunked)
 - chunked-object: fallback checkout codepaths
 - bulk-checkin: support chunked-object encoding
 - bulk-checkin: allow the same data to be multiply hashed
 - new representation types in the packstream
 - varint-in-pack: refactor varint encoding/decoding

Not ready.

At least pack-objects and fsck need to learn the new encoding for the
series to be usable locally, and then index-pack/unpack-objects needs to
learn it to be used remotely.

* jc/advise-i18n (2011-12-22) 1 commit
 - i18n of multi-line advice messages

Allow localization of advice messages that tend to be longer and
multi-line formatted. For now this is deliberately limited to advise()
interface and not vreportf() in general as touching the latter has
interactions with error() that has plumbing callers whose prefix "error: "
should never be translated.

--------------------------------------------------
[Cooking]

* pw/p4-docs-and-tests (2011-12-27) 11 commits
 - git-p4: document and test submit options
 - git-p4: test and document --use-client-spec
 - git-p4: test --keep-path
 - git-p4: test --max-changes
 - git-p4: document and test --import-local
 - git-p4: honor --changesfile option and test
 - git-p4: document and test clone --branch
 - git-p4: test cloning with two dirs, clarify doc
 - git-p4: clone does not use --git-dir
 - git-p4: introduce asciidoc documentation
 - rename git-p4 tests

Rorolled.
Not urgent.

* jc/signed-commit (2011-11-29) 5 commits
  (merged to 'next' on 2011-12-21 at 8fcbf00)
 + gpg-interface: allow use of a custom GPG binary
 + pretty: %G[?GS] placeholders
 + test "commit -S" and "log --show-signature"
 + log: --show-signature
 + commit: teach --gpg-sign option

I am ambivalent on this one. I do not desperately need it myself, I know
the kernel folks do not, I heard some other people might.

Opinions?

^ permalink raw reply

* Re: [PATCH 1/5] add central method for prompting a user using GIT_ASKPASS or SSH_ASKPASS
From: Junio C Hamano @ 2011-12-27 23:35 UTC (permalink / raw)
  To: Thomas Adam; +Cc: Sven Strickroth, git, Jakub Narebski, Jeff King
In-Reply-To: <CA+39Oz5J82GVyLfzWbWz20VS=Gp=8q9WsHQY33GuOKT1PyFCbQ@mail.gmail.com>

Thomas Adam <thomas@xteddy.org> writes:

> On 27 December 2011 20:47, Junio C Hamano <gitster@pobox.com> wrote:
>> Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:
>>> +sub askpass_prompt {
>>> +     my ($self, $prompt) = _maybe_self(@_);
>>> +     if (exists $ENV{'GIT_ASKPASS'}) {
>>> +             return _askpass_prompt($ENV{'GIT_ASKPASS'}, $prompt);
>>> +     } elsif (exists $ENV{'SSH_ASKPASS'}) {
>>> +             return _askpass_prompt($ENV{'SSH_ASKPASS'}, $prompt);
>>> +     } else {
>>> +             return undef;
>>
>> Two problems with this if/elsif/else cascade.
>>
>>  - If _askpass_prompt() fails to open the pipe to ENV{'GIT_ASKPASS'}, it
>>   will return 'undef' to us. Don't we want to fall back to SSH_ASKPASS in
>>   such a case?
>>
>>  - The last "return undef" makes all callers of this method to implement a
>>   fall-back way somehow. I find it very likely that they will want to use
>
> Not only that, "return undef" will have nasty side-effects if this
> subroutine is called in list-context -- it's usually discouraged to
> have explicit returns of "undef", where in scalar context that might
> be OK, but in list context, the caller will see:
>
> (undef)
>
> and not:
>
> ()
>
> i.e., the empty list.

Well, for this particular function whose interface is "I'll give you a
prompt, use it to interact with the user and give me what the user gave us
in response", a scalar caller would do

	my $response = askpass_prompt("What is your password?");

while a list context caller would instead do

	my ($response) = askpass_prompt("What is your password?");

or

	my @answer = askpass_prompt("What is your password?");
        my $response = $answer[0];

and all three callers would get "undef" in $response. I suspect returning
(undef) is a better thing to do, than relying that

        my @answer = ();
        my $response = $answer[0];

happes to give undef to $response because the access goes beyond the end
of the array, no?

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox