* 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
* Re: [PATCH] add post-fetch hook
From: Junio C Hamano @ 2011-12-27 23:23 UTC (permalink / raw)
To: Joey Hess; +Cc: git
In-Reply-To: <20111226155152.GA29582@gnu.kitenet.net>
Joey Hess <joey@kitenet.net> writes:
> From 073b0921bb5988628e7af423924c410f522f403a Mon Sep 17 00:00:00 2001
> From: Joey Hess <joey@kitenet.net>
> Date: Mon, 26 Dec 2011 10:53:27 -0400
> Subject: [PATCH 2/2] add tweak-fetch hook
>
> The tweak-fetch hook is fed lines on stdin for all refs that were fetched,
> and outputs on stdout possibly modified lines. Its output is parsed and
> used when git fetch updates the remote tracking refs, records the entries
> in FETCH_HEAD, and produces its report.
> ---
Just a few style things, as this is not a signed-off patch yet.
> @@ -162,6 +162,35 @@ This hook can be used to perform repository validity checks, auto-display
> differences from the previous HEAD if different, or set working dir metadata
> properties.
>
> +tweak-fetch
> +~~~~~~~~~~
The underline does not match what is being underlined. Does this format well?
> +This hook is invoked by 'git fetch' (commonly called by 'git pull'), after
> +refs have been fetched from the remote repository. It is not executed, if
> +nothing was fetched.
> +
> +The output of the hook is used to update the remote-tracking branches, and
> +`.git/FETCH_HEAD`, in preparation for for a later merge operation done by
> +'git merge'.
> +
> +It takes no arguments, but is fed a line of the following format on
> +its standard input for each ref that was fetched.
> +
> + <sha1> SP not-for-merge|merge SP <remote-refname> SP <local-refname> LF
> +
> +Where the "not-for-merge" flag indicates the ref is not to be merged into the
> +current branch, and the "merge" flag indicates that 'git merge' should
> +later merge it. The `<remote-refname>` is the remote's name for the ref
> +that was pulled, and `<local-refname>` is a name of a remote-tracking branch,
s/pulled/fetched/; I think. The remainder of the new text seems to use the
right terminology.
> +int feed_tweak_fetch_hook (int in, int out, void *data)
No SP between function name and the opening parenthesis of its parameter
list. We have SP after control-flow keywords e.g. "for (;;)" though.
Does this name need to be external (same question to many other new
functions in this patch)?
The "in" parameter seems unused. Does it have to be there for the "feed"
callback of the generic hook driver? As long as it is the "feed" callback,
I think that it just needs to take "out" and no "in", no?
> + for (ref = data; ref; ref = ref->next) {
> + strbuf_addstr(&buf, sha1_to_hex(ref->old_sha1));
> + strbuf_addch(&buf, ' ');
> + strbuf_addstr(&buf, ref->merge ? "merge" : "not-for-merge");
> + strbuf_addch(&buf, ' ');
strbuf_addf()?
But this might be a moot point, as J6t seems to have valid worries on
running functions that allocate memory in general...
> + ret = write_in_full(out, buf.buf, buf.len) != buf.len;
> + if (ret)
> + warning("%s hook failed to consume all its input",
> + tweak_fetch_hook);
> + close(out);
I was hoping that this part would be part of more generic hook driver
infrastructure. Even if we were to take this series before we refactor
existing other hook drivers, in order to avoid duplicated work later, we
could at least start from a right implementation of a generic hook driver
with a single user (which is the "tweak-fetch" hook driver), no?
> +struct ref *parse_tweak_fetch_hook_line (char *l,
> + struct string_list *existing_refs)
> +{
> + struct ref *ref = NULL, *peer_ref = NULL;
> + struct string_list_item *peer_item = NULL;
> + char *words[4];
> + int i, word=0;
SP around assingment and initialization "var = val" (throughout this
patch).
> + char *problem;
> +
> + for (i=0; l[i]; i++) {
Likewise.
> + if (isspace(l[i])) {
> + l[i]='\0';
> + words[word]=l;
> + l+=i+1;
> + i=0;
> + word++;
> + if (word > 3) {
> + problem="too many words";
> + goto unparsable;
> + }
> + }
> + }
> + if (word < 3) {
> + problem="not enough words";
> + goto unparsable;
> + }
Perhaps loop for up-to ARRAY_SIZE(words) times and use strchr()?
> + if (strcmp(words[1], "merge") == 0) {
We tend to say "if (!strcmp(...))" instead.
> + ref->merge=1;
> + }
> + else if (strcmp(words[1], "not-for-merge") != 0) {
Likewise.
> +struct refs_result read_tweak_fetch_hook (int in) {
Opening brace at column 1 of the next line.
> + if (prevref) {
> + prevref->next=ref;
> + prevref=ref;
> + }
> + else {
if (...) {
...
} else {
...
}
> +/* The hook is fed lines of the form:
> + * <sha1> SP <not-for-merge|merge> SP <remote-refname> SP <local-refname> LF
> + * And should output rewritten lines of the same form.
> + */
/*
* We write our multi-line comments
* like this (applies to a few other comments
* in this patch).
*/
^ permalink raw reply
* Re: [PATCH 1/5] add central method for prompting a user using GIT_ASKPASS or SSH_ASKPASS
From: Thomas Adam @ 2011-12-27 23:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Sven Strickroth, git, Jakub Narebski, Jeff King
In-Reply-To: <7vwr9h68t9.fsf@alter.siamese.dyndns.org>
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.
-- Thomas Adam
^ permalink raw reply
* [Wish] use postimage when showing common context in "diff -w"?
From: Junio C Hamano @ 2011-12-27 22:16 UTC (permalink / raw)
To: git; +Cc: Joey Hess
Joey's "write first for-merge ref to FETCH_HEAD first" ($gmane/187699)
wraps an existing large for(;;) loop inside another new loop, making the
existing code indented deeper. After queuing the patch, "git show -w"
displays a hunk like this [*1*]:
+ /*
+ * The first pass writes objects to be merged and then the
+ * second pass writes the rest, in order to allow using
+ * FETCH_HEAD as a refname to refer to the ref to be merged.
+ */
+ for (want_merge = 1; 0 <= want_merge; want_merge--) {
for (rm = ref_map; rm; rm = rm->next) {
struct ref *ref = NULL;
+ commit = lookup_commit_reference_gently(rm->old_sha1, 1);
+ if (!commit)
+ rm->merge = 0;
+
+ if (rm->merge != want_merge)
+ continue;
+
if (rm->peer_ref) {
ref = xcalloc(1, sizeof(*ref) + strlen(rm->peer_ref->name) + 1);
strcpy(ref->name, rm->peer_ref->name);
The context lines we can see in the above hunk are shown with incorrect
indentation level; I think we are showing the lines from the preimage.
It would be a really nice holiday gift to us, if somebody can fix this to
show lines from the postimage. It would make reviewing the change much
more pleasant. I obviously cannot throw this into my Amazon wishlist, so
instead I am posting it here ;-)
[Footnote]
*1* The text has my style fix-ups in it and does not match what was
posted. The patch lacked a sign-off and needs to be amended anyway. Also
it needs to adjust some existing tests (at least 5515 seems to break for
obvious reasons).
^ permalink raw reply
* Re: [PATCH] gc --auto: warn garbage collection happens soon
From: Junio C Hamano @ 2011-12-27 21:52 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1324993534-16307-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> This gives users a chance to run gc explicitly elsewhere if they do not
> want gc to run suddenly in current terminal.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
As I am still in a cheerly holiday mood, let's be a bit philosophical,
step back a bit and think.
After this patch gets applied, will the users start feeling bothered by
repeated "you will soon see auto-gc" messages and will want "you will soon
start seeing the you will soon see auto-gc messages" warnings?
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?
It may be a better cure for the disease to force a full gc after
operations that we know the users already know to take long time (e.g. a
clone, a large fetch), so that the next auto-gc do not have to do much
work.
^ permalink raw reply
* Re: [PATCH 5/5] make askpass_prompt a global prompt method for asking users
From: Junio C Hamano @ 2011-12-27 21:41 UTC (permalink / raw)
To: Sven Strickroth; +Cc: git, Jakub Narebski, Jeff King
In-Reply-To: <7vd3b967ql.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> I *suspect* the difference is that you discarded that "return false at the
> end to let the caller do whatever they want" found in patch 1/5 and have
> the fallback inside the prompt() funtion now. And if that is the primary
> difference between the old "askpass_prompt()" and the new "prompt()", I
> tend to think that the series should be restructured to use the "prompt()"
> semantics from the beginning. No reason to start with a known-to-be-wrong
> way to do a thing and then fix it in a series that is new to the codebase.
After reading the series again, I think the right structure of this patch
series should be more like this:
(1/3) Add Git->prompt($prompt) and make _read_password in git-svn.perl to
use it. The prompt method should implement the Term::ReadKey based
fallback, so that _read_password do not have to roll its own. IOW,
a squash of your 1/5, 2/5, and a part of 5/5, plus possibly 4/5.
(2/3) Enhance Git->prompt($prompt, $is_password), and convert the various
existing terminal interacters to use it. The fallback in the prompt
method, when it is not reading in a noecho mode, should read a
single line from the standard input in cooked mode like your 5/5
does. IOW, a squash of your 3/5 and a part of 5/5.
(3/3) Possibly tests and docs.
Thanks.
^ permalink raw reply
* Re: [PATCH] add post-fetch hook
From: Johannes Sixt @ 2011-12-27 21:27 UTC (permalink / raw)
To: Joey Hess; +Cc: Junio C Hamano, git
In-Reply-To: <20111226023154.GA3243@gnu.kitenet.net>
Am 26.12.2011 03:31, schrieb Joey Hess:
> +int feed_post_fetch_hook (int in, int out, void *data)
> +{
> + struct ref *ref;
> + struct strbuf buf = STRBUF_INIT;
Is there a particular reason that you accumulate everything in a buffer?
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?
> + strbuf_addch(&buf, ' ');
> + strbuf_addstr(&buf, ref->merge ? "merge" : "not-for-merge");
> + strbuf_addch(&buf, ' ');
> + if (ref->name)
> + strbuf_addstr(&buf, ref->name);
> + strbuf_addch(&buf, ' ');
> + if (ref->peer_ref && ref->peer_ref->name)
> + strbuf_addstr(&buf, ref->peer_ref->name);
> + strbuf_addch(&buf, '\n');
> + }
-- Hannes
^ permalink raw reply
* Re: [PATCH 5/5] make askpass_prompt a global prompt method for asking users
From: Junio C Hamano @ 2011-12-27 21:10 UTC (permalink / raw)
To: Sven Strickroth; +Cc: git, Jakub Narebski, Jeff King
In-Reply-To: <4EF9ED58.8080205@tu-clausthal.de>
Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:
> Signed-off-by: Sven Strickroth <email@cs-ware.de>
> ---
I think the end result of having a prompt function that can be used with
or without echoing like Peff's git_prompt() series does is a very good
thing. But then we just should introduce "prompt()" from day one of the
series, instead of introducing a half-featured one "askpass_prompt()" and
then later renaming the callers like this patch does.
It may a good idea to take the stepwise approach like this series does,
but in that case, the proposed log message must explain what the new
"prompt()" function is and does. It is derived from askpass_prompt() and
apparently it does more than its ancestor, but what are differences
between the two?
For example, it is totally unclear why these two are equivalent without
any explanation in the commit log message.
> - $choice = Git->askpass_prompt("Certificate unknown. " . $options);
> - if (!defined $choice) {
> - print STDERR $options;
> - STDERR->flush;
> - $choice = lc(substr(<STDIN> || 'R', 0, 1));
> - }
> + $choice = substr(Git->prompt("Certificate unknown. " . $options) || 'R', 0, 1);
or this:
> - my $filename = Git->askpass_prompt("Client certificate filename:");
> - if (!defined $filename) {
> - print STDERR "Client certificate filename: ";
> - STDERR->flush;
> - chomp($filename = <STDIN>);
> - }
> - $cred->cert_file($filename);
> + $cred->cert_file(Git->prompt("Client certificate filename: "));
I *suspect* the difference is that you discarded that "return false at the
end to let the caller do whatever they want" found in patch 1/5 and have
the fallback inside the prompt() funtion now. And if that is the primary
difference between the old "askpass_prompt()" and the new "prompt()", I
tend to think that the series should be restructured to use the "prompt()"
semantics from the beginning. No reason to start with a known-to-be-wrong
way to do a thing and then fix it in a series that is new to the codebase.
Thanks.
^ permalink raw reply
* Re: [PATCH 4/5] ignore empty *_ASKPASS variables
From: Junio C Hamano @ 2011-12-27 21:00 UTC (permalink / raw)
To: Sven Strickroth; +Cc: git, Jakub Narebski, Jeff King, gitster
In-Reply-To: <4EF9ED38.9010502@tu-clausthal.de>
Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:
> Signed-off-by: Sven Strickroth <email@cs-ware.de>
> ---
I *suspect* that this is a fix-up to a bug in patch 1/5 that lets callers
call _askpass_prompt helper without checking the value of the "askpass",
and if that is the case, this patch should be squashed there.
But there is no justification in the proposed log message above, so I
cannot tell.
> perl/Git.pm | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/perl/Git.pm b/perl/Git.pm
> index 7fdf805..c6b3e11 100644
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -537,6 +537,9 @@ sub askpass_prompt {
>
> sub _askpass_prompt {
> my ($self, $askpass, $prompt) = _maybe_self(@_);
> + unless ($askpass) {
> + return undef;
> + }
> my $ret;
> open my $fh, "-|", $askpass, $prompt || return undef;
> $ret = <$fh>;
^ permalink raw reply
* Re: [PATCH 3/5] honour *_ASKPASS for querying username and for querying further actions like unknown certificates
From: Junio C Hamano @ 2011-12-27 20:56 UTC (permalink / raw)
To: Sven Strickroth; +Cc: git, Jakub Narebski, Jeff King
In-Reply-To: <4EF9ED24.2040902@tu-clausthal.de>
Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:
> git-svn reads usernames (and other stuff) from an interactive terminal.
> This behavior cause GUIs to hang waiting for git-svn to complete (http://code.google.com/p/tortoisegit/issues/detail?id=967).
(style) Wrap the line perhaps after "to complete".
Does the above mean "GUIs hang, until the user goes back to the terminal
and authenticates"? Where is the "interactive terminal" connected when
running the GUI?
With that bit information, I think the above is a decent problem
description (i.e. "what problem is this change trying to solve? is it
worth solving?").
The second paragraph (missing) should then discuss what approach is taken
by the proposed patch to solve that problem. Something like
Instead of using hand-rolled prompt-response code that only works with
the interactive terminal, use the git_prompt() method introduced in
the earlier commit.
would suffice (I didn't check what method name you used, though).
> Also see commit 56a853b62c0ae7ebaad0a7a0a704f5ef561eb795.
I checked that commit, and what you wanted to say is unclear. Are you
saying this patch attempts to fix the breakage by that commit? That commit
tried to go in a right direction but did not go far enough and you are
trying to enhance it? Somerthing else?
Which means that you shouldn't have said "Also see..." at all and instead
directly said what you wanted to say here.
^ permalink raw reply
* Re: [PATCH 2/5] switch to central prompt method
From: Junio C Hamano @ 2011-12-27 20:47 UTC (permalink / raw)
To: Sven Strickroth; +Cc: git, Jakub Narebski, Jeff King
In-Reply-To: <4EF9ECEF.6020403@tu-clausthal.de>
Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:
> Signed-off-by: Sven Strickroth <email@cs-ware.de>
> ---
It would be better to have this and the previous patch squashed into one
commit, for three reasons:
- It is far easier to review the patch that way, because it will show the
old way to get the password from the user as the removed code and new
way to do so as the added helper function, enabling reviewers to
compare the two in a single review session, to see if the change keeps
the feature bug-to-bug compatible, introduces any regression, and/or
offers improvements over existing code.
- If there were a bug in the implementation of askpass_prompt method
introduced in patch 1 without being used, and the calling codepath
introduced in patch 2 is bug-free (i.e. it correctly follows the
calling convention of the new method, only the implementation of the
method is buggy), bisection will still point at patch 2 that dumped the
old proven working way and started using the buggy new implementation.
Of course, it is possible that patch 1 perfectly implements the new
method and a bug exists in the way the caller introduced in patch 2
calls the method and in such a case bisection will correctly point out
that the caller is at fault, but the point of this refactoring is to
make it harder for callers to make such mistakes.
- As we can see above the three-dash line, even the author of the series
could not come up with any justification why the proposed change is a
good thing in the proposed commit log message for this patch alone (or
the previous patch alone for that matter). Combining these patches
together would make it clearer why it may be a good thing, which would
make it easier to come up with a better log message.
> git-svn.perl | 9 ++-------
> 1 files changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/git-svn.perl b/git-svn.perl
> index eeb83d3..25d5da7 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -4415,13 +4415,8 @@ 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 {
> + my $password = Git->askpass_prompt($prompt);;
> + if (!defined $password) {
> print STDERR $prompt;
> STDERR->flush;
> require Term::ReadKey;
^ 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 20:47 UTC (permalink / raw)
To: Sven Strickroth; +Cc: git, Jakub Narebski, Jeff King
In-Reply-To: <4EF9ECC0.606@tu-clausthal.de>
Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:
> Signed-off-by: Sven Strickroth <email@cs-ware.de>
> ---
Thanks.
Please indicate what area you are touching on the Subject, e.g.
Subject: [PATCH 1/5] perl/Git.pm: add askpass_prompt() method
and explain what problem it tries to solve, how it tries to solve it, and
why this particular way of solving that problem is a good thing to have in
the proposed commit log message (but see the comments to 2/5 before doing
so).
> perl/Git.pm | 36 +++++++++++++++++++++++++++++++++++-
> 1 files changed, 35 insertions(+), 1 deletions(-)
>
> diff --git a/perl/Git.pm b/perl/Git.pm
> index f7ce511..7fdf805 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 askpass_prompt
> temp_acquire temp_release temp_reset temp_path);
>
>
> @@ -512,6 +512,40 @@ C<git --html-path>). Useful mostly only internally.
> sub html_path { command_oneline('--html-path') }
>
>
> +=item askpass_prompt ( PROMPT)
Is the unbalanced spacing around parentheses your finger slippage?
> +
> +Asks user using *_ASKPASS programs and return answer from user.
Other =item entries in this file (I only checked a couple of earlier ones)
begin with "Construct a new ...", "Execute the given git command ...",
i.e. in imperative mood.
Also I agree with Jakub that it would be more appropriate to start the
description with the purpose and the effect, i.e. what the function is
for, than the internal implementation, i.e. what the function does.
> +Checks if GIT_ASKPASS or SSH_ASKPASS is set, and use first matching for querying
> +user and returns answer.
> +
> +If no *_ASKPASS variable is set, the variable is empty or an error occours,
> +it returns undef and the caller has to ask the user (e.g. on terminal).
> +
> +=cut
> +
> +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
the fall-back code that uses Term::ReadKey found in _read_password, and
they will hate the above askpass_prompt implementation for focing them
to duplicate the code more than they will appreciate the flexibility
that they could implement a different fall-back.
> +}
> +
> +sub _askpass_prompt {
> + my ($self, $askpass, $prompt) = _maybe_self(@_);
I am not sure why you would want _maybe_self() here for this internal
helper function _askpass_prompt that is not even called as a method of
anything.
> + 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.
^ permalink raw reply
* Re: Git beginner - Need help understanding
From: Junio C Hamano @ 2011-12-27 19:50 UTC (permalink / raw)
To: chirin; +Cc: git
In-Reply-To: <1324969372444-7129429.post@n2.nabble.com>
chirin <takonatto@gmail.com> writes:
> From what I understand, your scenario is exactly what I expect. Which is why
> when I asked around my colleagues, no one was able to explain why I'm having
> this issue.
>
> As per your scenario:
>
> # A changes hello.txt
>
> # Going into B (who has not done anything to hello.txt)
> git pull --> merge conflict on hello.txt
>
> git commit
> git pull --> OK
This is quite different from what Dov showed as "the right way to
report". Dob's recipe begins from absolutely empty state and creates three
repositories involved with exact sequence and anybody can reproduce how it
is supposed to work (or break). It was written in such a way that you can
try to follow the sequence to see if you see a behaviour that is different
from Dob expects (in which case your machine, filesystem or the Git binary
you installed might be the culprit). Have you tried it?
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.
>> In this sequence, which fulfills the scenario that you described,
>> there are no conflicts. So I suggest that you try to change the
>> command sequence to illustrate what you don't understand and ask
>> again.
Exactly.
^ permalink raw reply
* Possible submodule or submodule documentation issue
From: Bill Zaumen @ 2011-12-27 19:24 UTC (permalink / raw)
To: git
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
echo goodbye > goodbye
git add goodbye
git submodule add ./library.git src <FAILS>
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).
git --version reports 1.7.1 (current version for my linux
distribution).
^ permalink raw reply
* Re: [PATCH] write first for-merge ref to FETCH_HEAD first
From: Junio C Hamano @ 2011-12-27 18:44 UTC (permalink / raw)
To: Joey Hess; +Cc: git
In-Reply-To: <20111226161656.GB29582@gnu.kitenet.net>
Joey Hess <joey@kitenet.net> writes:
> The FETCH_HEAD refname is supposed to refer to the ref that was fetched
> and should be merged. However all fetched refs are written to
> .git/FETCH_HEAD in an arbitrary order, and resolve_ref_unsafe simply
> takes the first ref as the FETCH_HEAD, which is often the wrong one,
> when other branches were also fetched.
>
> The solution is to write the for-merge ref(s) to FETCH_HEAD first.
> Then, unless --append is used, the FETCH_HEAD refname behaves as intended.
> If the user uses --append, they presumably are doing so in order to
> preserve the old FETCH_HEAD.
>
> Also included a fix to documentation that assumes FETCH_HEAD contains
> only a single ref.
That "single ref" assumption is perfectly fine for the part of the
documentation you patched, actually. The "fetch" command-line the example
shows explicitly fetches a single ref.
It is a good idea to use rev-parse anyway, so the patch itself is good. A
potential problem of that example (I haven't re-tried these examples for
eons since they were written) comes from the fact that FETCH_HEAD contains
not just the object name of what we fetched, but also other information to
describe what happened to that fetched object.
> ---
Sign-off?
> Documentation/git-read-tree.txt | 2 +-
> builtin/fetch.c | 158 +++++++++++++++++++++------------------
> 2 files changed, 85 insertions(+), 75 deletions(-)
^ permalink raw reply
* Re: [PATCH] Use Python's "print" as a function, not as a keyword
From: Pete Wyckoff @ 2011-12-27 16:41 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Sebastian Morr, git, srabbelier
In-Reply-To: <CACBZZX7PVyCFfHTJN_QZfyt5wAcr4UAiJSmo54PSi=8pgv3sYA@mail.gmail.com>
avarab@gmail.com wrote on Wed, 21 Dec 2011 03:48 +0100:
> On Wed, Dec 21, 2011 at 03:19, Sebastian Morr <sebastian@morr.cc> wrote:
>
> > But, as nobody seems to have cared before: Is Git designed to be
> > compatible only with versions prior 3.0?
>
> I'm running Debian unstable and it has Python 2.7. Most people are
> still using Python 2.x as their default system Python since 3.x breaks
> backwards compatibility for common constructs like print.
>
> Does this only break Python 2.6, or all 2.x versions of Python?
>
> What's our currently supported Python version for the Python code in
> Git? It's 5.8.0 for Perl, do we have any particular aim for a
> supported Python version?
I test contrib/fast-import/git-p4 on python 2.5 and 2.7. I'm
hesitant to convert print() now, without committing to testing on
post-3.0 too. The work to support 3.x doesn't buy us much.
-- Pete
^ permalink raw reply
* [PATCH 5/5] make askpass_prompt a global prompt method for asking users
From: Sven Strickroth @ 2011-12-27 16:07 UTC (permalink / raw)
To: git; +Cc: Jakub Narebski, Jeff King, gitster
In-Reply-To: <4EF9EBF4.7070200@tu-clausthal.de>
Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
git-svn.perl | 37 ++++---------------------------------
perl/Git.pm | 43 +++++++++++++++++++++++++++++++------------
2 files changed, 35 insertions(+), 45 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index 2c99aaa..1f30dc2 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -4360,12 +4360,7 @@ prompt:
my $options = $may_save ?
"(R)eject, accept (t)emporarily or accept (p)ermanently? " :
"(R)eject or accept (t)emporarily? ";
- $choice = Git->askpass_prompt("Certificate unknown. " . $options);
- if (!defined $choice) {
- print STDERR $options;
- 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) {
@@ -4382,13 +4377,7 @@ prompt:
sub ssl_client_cert {
my ($cred, $realm, $may_save, $pool) = @_;
$may_save = undef if $_no_auth_cache;
- my $filename = Git->askpass_prompt("Client certificate filename:");
- if (!defined $filename) {
- print STDERR "Client certificate filename: ";
- STDERR->flush;
- chomp($filename = <STDIN>);
- }
- $cred->cert_file($filename);
+ $cred->cert_file(Git->prompt("Client certificate filename: "));
$cred->may_save($may_save);
$SVN::_Core::SVN_NO_ERROR;
}
@@ -4411,12 +4400,7 @@ sub username {
if (defined $_username) {
$username = $_username;
} else {
- $username = Git->askpass_prompt("Username");
- }
- if (!defined $username) {
- print STDERR "Username: ";
- STDERR->flush;
- chomp($username = <STDIN>);
+ $username = Git->prompt("Username: ");
}
$cred->username($username);
$cred->may_save($may_save);
@@ -4425,20 +4409,7 @@ sub username {
sub _read_password {
my ($prompt, $realm) = @_;
- my $password = Git->askpass_prompt($prompt);;
- if (!defined $password) {
- 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, 1);
$password;
}
diff --git a/perl/Git.pm b/perl/Git.pm
index c6b3e11..acc00b4 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 askpass_prompt
+ remote_refs prompt
temp_acquire temp_release temp_reset temp_path);
@@ -512,30 +512,49 @@ C<git --html-path>). Useful mostly only internally.
sub html_path { command_oneline('--html-path') }
-=item askpass_prompt ( PROMPT)
+=item prompt ( PROMPT , ISPASSWORD )
-Asks user using *_ASKPASS programs and return answer from user.
+Asks user using *_ASKPASS programs or terminal the prompt C<PROMPT> and return answer from user.
Checks if GIT_ASKPASS or SSH_ASKPASS is set, and use first matching for querying
user and returns answer.
If no *_ASKPASS variable is set, the variable is empty or an error occours,
-it returns undef and the caller has to ask the user (e.g. on terminal).
+the querying the user using terminal is tried. If C<ISPASSWORD> is set and true,
+the terminal disables echo.
=cut
-sub askpass_prompt {
- my ($self, $prompt) = _maybe_self(@_);
+sub prompt {
+ my ($self, $prompt, $isPassword) = _maybe_self(@_);
+ my $ret;
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;
+ $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;
+ 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>);
+ }
+ }
+ return $ret;
}
-sub _askpass_prompt {
+sub _prompt {
my ($self, $askpass, $prompt) = _maybe_self(@_);
unless ($askpass) {
return undef;
--
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 4/5] ignore empty *_ASKPASS variables
From: Sven Strickroth @ 2011-12-27 16:07 UTC (permalink / raw)
To: git; +Cc: Jakub Narebski, Jeff King, gitster
In-Reply-To: <4EF9EBF4.7070200@tu-clausthal.de>
Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
perl/Git.pm | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/perl/Git.pm b/perl/Git.pm
index 7fdf805..c6b3e11 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -537,6 +537,9 @@ sub askpass_prompt {
sub _askpass_prompt {
my ($self, $askpass, $prompt) = _maybe_self(@_);
+ unless ($askpass) {
+ return undef;
+ }
my $ret;
open my $fh, "-|", $askpass, $prompt || return undef;
$ret = <$fh>;
--
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 3/5] honour *_ASKPASS for querying username and for querying further actions like unknown certificates
From: Sven Strickroth @ 2011-12-27 16:07 UTC (permalink / raw)
To: git; +Cc: Jakub Narebski, Jeff King, gitster
In-Reply-To: <4EF9EBF4.7070200@tu-clausthal.de>
git-svn reads usernames (and other stuff) from an interactive terminal.
This behavior cause GUIs to hang waiting for git-svn to complete (http://code.google.com/p/tortoisegit/issues/detail?id=967).
Also see commit 56a853b62c0ae7ebaad0a7a0a704f5ef561eb795.
Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
git-svn.perl | 22 ++++++++++++++++------
1 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index 25d5da7..2c99aaa 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -4357,11 +4357,15 @@ 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 = Git->askpass_prompt("Certificate unknown. " . $options);
+ if (!defined $choice) {
+ print STDERR $options;
+ STDERR->flush;
+ $choice = lc(substr(<STDIN> || 'R', 0, 1));
+ }
if ($choice =~ /^t$/i) {
$cred->may_save(undef);
} elsif ($choice =~ /^r$/i) {
@@ -4378,9 +4382,12 @@ 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>);
+ my $filename = Git->askpass_prompt("Client certificate filename:");
+ if (!defined $filename) {
+ print STDERR "Client certificate filename: ";
+ STDERR->flush;
+ chomp($filename = <STDIN>);
+ }
$cred->cert_file($filename);
$cred->may_save($may_save);
$SVN::_Core::SVN_NO_ERROR;
@@ -4404,6 +4411,9 @@ sub username {
if (defined $_username) {
$username = $_username;
} else {
+ $username = Git->askpass_prompt("Username");
+ }
+ if (!defined $username) {
print STDERR "Username: ";
STDERR->flush;
chomp($username = <STDIN>);
--
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 2/5] switch to central prompt method
From: Sven Strickroth @ 2011-12-27 16:06 UTC (permalink / raw)
To: git; +Cc: Jakub Narebski, Jeff King, gitster
In-Reply-To: <4EF9EBF4.7070200@tu-clausthal.de>
Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
git-svn.perl | 9 ++-------
1 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index eeb83d3..25d5da7 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -4415,13 +4415,8 @@ 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 {
+ my $password = Git->askpass_prompt($prompt);;
+ if (!defined $password) {
print STDERR $prompt;
STDERR->flush;
require Term::ReadKey;
--
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/5] add central method for prompting a user using GIT_ASKPASS or SSH_ASKPASS
From: Sven Strickroth @ 2011-12-27 16:05 UTC (permalink / raw)
To: git; +Cc: Jakub Narebski, Jeff King, gitster
In-Reply-To: <4EF9EBF4.7070200@tu-clausthal.de>
Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
perl/Git.pm | 36 +++++++++++++++++++++++++++++++++++-
1 files changed, 35 insertions(+), 1 deletions(-)
diff --git a/perl/Git.pm b/perl/Git.pm
index f7ce511..7fdf805 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 askpass_prompt
temp_acquire temp_release temp_reset temp_path);
@@ -512,6 +512,40 @@ C<git --html-path>). Useful mostly only internally.
sub html_path { command_oneline('--html-path') }
+=item askpass_prompt ( PROMPT)
+
+Asks user using *_ASKPASS programs and return answer from user.
+
+Checks if GIT_ASKPASS or SSH_ASKPASS is set, and use first matching for querying
+user and returns answer.
+
+If no *_ASKPASS variable is set, the variable is empty or an error occours,
+it returns undef and the caller has to ask the user (e.g. on terminal).
+
+=cut
+
+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;
+ }
+}
+
+sub _askpass_prompt {
+ my ($self, $askpass, $prompt) = _maybe_self(@_);
+ 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
* [PATCH 0/5] honour *_ASKPASS for querying user in git-svn
From: Sven Strickroth @ 2011-12-27 16:01 UTC (permalink / raw)
To: git; +Cc: Jakub Narebski, Jeff King, gitster
In-Reply-To: <4EF9D8B9.9060106@tu-clausthal.de>
Hi,
git-svn reads usernames (and other stuff) from an interactive terminal.
This behavior cause GUIs to hang waiting for git-svn to complete
(http://code.google.com/p/tortoisegit/issues/detail?id=967).
Again I honoured all your ideas. Hopefully the patches can be applied
now. The new patches follow in the next mails (you can also pull from
git://github.com/csware/git.git askpass-prompt).
The first four patches implement the raw functionality. The last and
fifth patch extends the prompt-method so that it can be used for all
purposes in order to query users.
--
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] honour GIT_ASKPASS for querying username in git-svn
From: Jakub Narebski @ 2011-12-27 16:00 UTC (permalink / raw)
To: Sven Strickroth; +Cc: git, Jeff King
In-Reply-To: <4EF9D8B9.9060106@tu-clausthal.de>
On Tue, 27 Dec 2011, Sven Strickroth wrote:
> Am 27.12.2011 15:33 schrieb Jakub Narebski:
>>> +sub prompt {
>>> + my ($self, $prompt) = _maybe_self(@_);
>>> + if (exists $ENV{'GIT_ASKPASS'}) {
>>> + return _prompt($ENV{'GIT_ASKPASS'}, $prompt);
>>> + } elsif (exists $ENV{'SSH_ASKPASS'}) {
>>> + return _prompt($ENV{'SSH_ASKPASS'}, $prompt);
>>> + } else {
>>> + return undef;
>>> + }
>>> +}
>>
>> ...and provide some kind of fallback even if neither of GIT_ASKPASS
>> nor SSH_ASKPASS are set (perhaps assuming that some Perl packages from
>> CPAN are installed).
>
> If neither of GIT_ASKPASS nor SSH_ASKPASS are set the caller has to
> handle the request. This has to be done this way, because of lots of
> different needs (username, password (no echo) and so on).
I think that Git.pm and therefore git commands written in Perl should
behave the same as git command written in C; and I think builtins do
use common gitprompt fallback.
>>> +sub _prompt {
>>> + my ($self, $askpass, $prompt) = _maybe_self(@_);
>>> + my $ret;
>>> + open(PH, "-|", $askpass, $prompt);
>>> + $ret = <PH>;
>>> + $ret =~ s/[\012\015]//g; # strip \n\r
>>> + close(PH);
>>> + return $ret;
>>> +}
>>
>> Please, use modern Perl, in particula use lexical filehandles instead
>> of typeglobs (which are global variables), i.e.
>
> I used the same style as I found in Git.pm (see lines I removed in patch 2).
Yes, that should be fixed (together with host of other issues), but
one should use modern and _better_ way (no possibility of action at
distance).
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCH] add post-fetch hook
From: Joey Hess @ 2011-12-27 15:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v8vly8qqx.fsf@alter.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 925 bytes --]
Junio C Hamano wrote:
> Joey Hess <joey@kitenet.net> writes:
>
> > .... And other code in git uses an async feeder similarly,
> > see for example convert.c's apply_filter(). So I think this is ok..?
>
> Yeah, I didn't look at your patch (sorry) but if it uses async like the
> filtering codepath does, it should be perfectly fine (please forget about
> the select(2) based kludge I alluded to; the async interface is the right
> thing to use here).
No problem, I was surprised to be getting responses at all over the
holidays. :)
Then async also seems the right thing to use for the hook refactoring. A
caller can provide two function pointers; a feeder function that is
called async, and a reader that is *not* called async (which would allow
it to modify program state), and the refactored hook function handles
running the hook(s) and connecting them to the feeder and/or reader.
--
see shy jo
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply
* Re: [PATCH] honour GIT_ASKPASS for querying username in git-svn
From: Sven Strickroth @ 2011-12-27 14:39 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git, Jeff King
In-Reply-To: <m3d3baf5kd.fsf@localhost.localdomain>
Am 27.12.2011 15:33 schrieb Jakub Narebski:
>> +sub prompt {
>> + my ($self, $prompt) = _maybe_self(@_);
>> + if (exists $ENV{'GIT_ASKPASS'}) {
>> + return _prompt($ENV{'GIT_ASKPASS'}, $prompt);
>> + } elsif (exists $ENV{'SSH_ASKPASS'}) {
>> + return _prompt($ENV{'SSH_ASKPASS'}, $prompt);
>> + } else {
>> + return undef;
>> + }
>> +}
>
> ...and provide some kind of fallback even if neither of GIT_ASKPASS
> nor SSH_ASKPASS are set (perhaps assuming that some Perl packages from
> CPAN are installed).
If neither of GIT_ASKPASS nor SSH_ASKPASS are set the caller has to
handle the request. This has to be done this way, because of lots of
different needs (username, password (no echo) and so on).
>> +sub _prompt {
>> + my ($self, $askpass, $prompt) = _maybe_self(@_);
>> + my $ret;
>> + open(PH, "-|", $askpass, $prompt);
>> + $ret = <PH>;
>> + $ret =~ s/[\012\015]//g; # strip \n\r
>> + close(PH);
>> + return $ret;
>> +}
>
> Please, use modern Perl, in particula use lexical filehandles instead
> of typeglobs (which are global variables), i.e.
I used the same style as I found in Git.pm (see lines I removed in patch 2).
--
Best regards,
Sven Strickroth
ClamAV, a GPL anti-virus toolkit http://www.clamav.net
PGP key id F5A9D4C4 @ any key-server
^ 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