* Re: [PATCH] git-send-email: add ~/.authinfo parsing
From: Ted Zlatanov @ 2013-02-04 20:28 UTC (permalink / raw)
To: Jeff King; +Cc: Michal Nazarewicz, Junio C Hamano, git, Krzysztof Mazur
In-Reply-To: <20130204201040.GA13272@sigill.intra.peff.net>
On Mon, 4 Feb 2013 15:10:40 -0500 Jeff King <peff@peff.net> wrote:
JK> Technically you can speak a particular protocol on an alternate port:
JK> https://example.com:31337/repo.git
JK> In this case, git will send you the host as:
JK> example.com:31337
JK> You might want to map this to "port" in .autoinfo separately if it's
JK> available.
That would create the following possibilities:
* host example.com:31337, protocol https
* host example.com:31337, protocol unspecified
* host example.com, protocol https
* host example.com, protocol unspecified
How would you like each one to be handled? My preference would be to
make the user say "host example.com:31337" in the netrc file (the
current situation); that's what we do in Emacs and it lets applications
request credentials for a logical service no matter what the port is.
It means that example.com credentials won't be used for
example.com:31337. In practice, that has not been a problem for us.
Ted
^ permalink raw reply
* Re: [PATCH v3] submodule: add 'deinit' command
From: Junio C Hamano @ 2013-02-04 20:55 UTC (permalink / raw)
To: Jens Lehmann
Cc: Git Mailing List, Heiko Voigt, Michael J Gruber, Phil Hord,
Marc Branchaud, W. Trevor King
In-Reply-To: <5110157A.2040402@web.de>
Jens Lehmann <Jens.Lehmann@web.de> writes:
> With "git submodule init" the user is able to tell git he cares about one
> or more submodules and wants to have it populated on the next call to "git
> submodule update". But currently there is no easy way he could tell git he
> does not care about a submodule anymore and wants to get rid of his local
> work tree (except he knows a lot about submodule internals and removes the
> "submodule.$name.url" setting from .git/config together with the work tree
> himself).
>
> Help those users by providing a 'deinit' command. This removes the whole
> submodule.<name> section from .git/config either for the given
> submodule(s) (or for all those which have been initialized if '.' is
> given). Fail if the current work tree contains modifications unless
> forced. Complain when for a submodule given on the command line the url
> setting can't be found in .git/config, but nonetheless don't fail.
>
> Add tests and link the man pages of "git submodule deinit" and "git rm"
> to assist the user in deciding whether removing or unregistering the
> submodule is the right thing to do for him.
>
> Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
> ---
>
> Ok, here is the reroll following the discussion in $gmane/212884.
>
> Changes since v2 are:
> - deinit always needs an argument; if the user wants to deinit all initialized
> submodules he can use "." (and we tell him that when failing without any
> arguments).
> - We also remove the work tree of the submodule. When it contains local changes
> (tested with "git rm -n") this fails unless forced.
>
>
> Documentation/git-rm.txt | 4 +++
> Documentation/git-submodule.txt | 18 ++++++++++-
> git-submodule.sh | 70 ++++++++++++++++++++++++++++++++++++++++-
> t/t7400-submodule-basic.sh | 24 ++++++++++++++
> 4 files changed, 114 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-rm.txt b/Documentation/git-rm.txt
> index 262436b..8ae72f7 100644
> --- a/Documentation/git-rm.txt
> +++ b/Documentation/git-rm.txt
> @@ -149,6 +149,10 @@ files that aren't ignored are present in the submodules work tree.
> Ignored files are deemed expendable and won't stop a submodule's work
> tree from being removed.
>
> +If you only want to remove the local checkout of a submodule from your
> +work tree without committing that use linkgit:git-submodule[1] `deinit`
> +instead.
> +
> EXAMPLES
> --------
> `git rm Documentation/\*.txt`::
> diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
> index b1996f1..9a20e1d 100644
> --- a/Documentation/git-submodule.txt
> +++ b/Documentation/git-submodule.txt
> @@ -13,6 +13,7 @@ SYNOPSIS
> [--reference <repository>] [--] <repository> [<path>]
> 'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...]
> 'git submodule' [--quiet] init [--] [<path>...]
> +'git submodule' [--quiet] deinit [-f|--force] [--] <path>...
> 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch] [--rebase]
> [--reference <repository>] [--merge] [--recursive] [--] [<path>...]
> 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) <n>]
> @@ -134,6 +135,19 @@ init::
> the explicit 'init' step if you do not intend to customize
> any submodule locations.
>
> +deinit::
> + Unregister the given submodules, i.e. remove the whole
> + `submodule.$name` section from .git/config together with their work
> + tree. Further calls to `git submodule update`, `git submodule foreach`
> + and `git submodule sync` will skip any unregistered submodules until
> + they are initialized again, so use this command if you don't want to
> + have a local checkout of the submodule in your work tree anymore. If
> + you really want to remove a submodule from the repository and commit
> + that use linkgit:git-rm[1] instead.
> ++
> +If `--force` is specified, the submodule's work tree will be removed even if
> +it contains local modifications.
> +
> update::
> Update the registered submodules, i.e. clone missing submodules and
> checkout the commit specified in the index of the containing repository.
> @@ -213,8 +227,10 @@ OPTIONS
>
> -f::
> --force::
> - This option is only valid for add and update commands.
> + This option is only valid for add, deinit and update commands.
> When running add, allow adding an otherwise ignored submodule path.
> + When running deinit the submodule work trees will be removed even if
> + they contain local changes.
> When running update, throw away local changes in submodules when
> switching to a different commit; and always run a checkout operation
> in the submodule, even if the commit listed in the index of the
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 22ec5b6..e01f6b5 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -8,6 +8,7 @@ dashless=$(basename "$0" | sed -e 's/-/ /')
> USAGE="[--quiet] add [-b <branch>] [-f|--force] [--name <name>] [--reference <repository>] [--] <repository> [<path>]
> or: $dashless [--quiet] status [--cached] [--recursive] [--] [<path>...]
> or: $dashless [--quiet] init [--] [<path>...]
> + or: $dashless [--quiet] deinit [-f|--force] [--] <path>...
> or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--rebase] [--reference <repository>] [--merge] [--recursive] [--] [<path>...]
> or: $dashless [--quiet] summary [--cached|--files] [--summary-limit <n>] [commit] [--] [<path>...]
> or: $dashless [--quiet] foreach [--recursive] <command>
> @@ -547,6 +548,73 @@ cmd_init()
> }
>
> #
> +# Unregister submodules from .git/config and remove their work tree
> +#
> +# $@ = requested paths (use '.' to deinit all submodules)
> +#
> +cmd_deinit()
> +{
> + # parse $args after "submodule ... init".
> + while test $# -ne 0
> + do
> + case "$1" in
> + -f|--force)
> + force=$1
> + ;;
> + -q|--quiet)
> + GIT_QUIET=1
> + ;;
> + --)
> + shift
> + break
> + ;;
> + -*)
> + usage
> + ;;
> + *)
> + break
> + ;;
> + esac
> + shift
> + done
> +
> + if test "$#" == "0"
No need to quote either of these (imitate what you did above with
"while"); also use single "=" for string comparison.
> + then
> + die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")"
> + fi
> +
> + module_list "$@" |
> + while read mode sha1 stage sm_path
> + do
> + die_if_unmatched "$mode"
> + name=$(module_name "$sm_path") || exit
> + url=$(git config submodule."$name".url)
> + if test -z "$url"
> + then
> + # Only mention uninitialized submodules when its
> + # path have been specified
> + test "$@" != "." &&
If your $# is not 1, "test" will fail with "too many arguments"
here, I think.
I would probably set a variable before running module_list,
needs_uninitialized_warning=
if $# = 1 || $1 != .
then
needs_uninitialized_warning=yes
fi
and check that variable here, perhaps.
But stepping back a bit, why does it warn only for non '.' case?
Shouldn't
git submodule deinit 'foo*'
behave the same way between foo1 and foo2 when it sees foo1 has been
initialized but foo2 hasn't?
> + say "$(eval_gettext "No url found for submodule path '\$sm_path' in .git/config")"
> + continue
> + fi
> +
> + # Remove the submodule work tree
> + if test -z $force
As $force is initialized to an empty string, this without dq around
it will become a syntax error, feeding "test" with a single argument
"-z".
> + then
> + git rm -n "$sm_path" ||
> + die "$(eval_gettext "Submodule work tree $sm_path contains local modifications, use '-f' to discard them")"
> + fi
> + rm -rf "$sm_path"
Do we want to make sure that $sm_path/.git is a gitfile here? I do
not think it matters that much, but the user may have local commits
that he has not pushed out, in which case it would be nicer to give
a way to preserve them.
> + mkdir "$sm_path"
Also can these rm -rf or mkdir fail, and what would we want to do
when they do?
I think it is preferrable to run "config --remove-section"
regardless, but we may want to tell the user what happened (i.e. the
submodule has been unregistered as far as Git is concerned, but the
worktree may have some cruft we couldn't remove).
> +
> + # Remove the whole section so we have a clean state when the
> + # user later decides to init this submodule again
> + git config --remove-section submodule."$name" &&
> + say "$(eval_gettext "Submodule '\$name' (\$url) unregistered for path '\$sm_path'")"
> + done
> +}
> +
> +#
> # Update each submodule path to correct revision, using clone and checkout as needed
> #
> # $@ = requested paths (default to all)
> @@ -1157,7 +1225,7 @@ cmd_sync()
> while test $# != 0 && test -z "$command"
> do
> case "$1" in
> - add | foreach | init | update | status | summary | sync)
> + add | foreach | init | deinit | update | status | summary | sync)
> command=$1
> ;;
> -q|--quiet)
^ permalink raw reply
* Re: [PATCH] git-send-email: add ~/.authinfo parsing
From: Jeff King @ 2013-02-04 20:59 UTC (permalink / raw)
To: Ted Zlatanov; +Cc: Michal Nazarewicz, Junio C Hamano, git, Krzysztof Mazur
In-Reply-To: <87a9rju8l7.fsf@lifelogs.com>
On Mon, Feb 04, 2013 at 03:28:52PM -0500, Ted Zlatanov wrote:
> JK> You might want to map this to "port" in .autoinfo separately if it's
> JK> available.
>
> That would create the following possibilities:
>
> * host example.com:31337, protocol https
> * host example.com:31337, protocol unspecified
> * host example.com, protocol https
> * host example.com, protocol unspecified
Possibilities for .netrc, or for git? Git will always specify the
protocol.
> How would you like each one to be handled? My preference would be to
> make the user say "host example.com:31337" in the netrc file (the
> current situation); that's what we do in Emacs and it lets applications
> request credentials for a logical service no matter what the port is.
>
> It means that example.com credentials won't be used for
> example.com:31337. In practice, that has not been a problem for us.
Yeah, I think that is a good thing. The credentials used for
example.com:31337 are not necessarily the same as for the main site.
It's less convenient, but a more secure default.
What I was more wondering (and I know very little about .netrc, so this
might not be a possibility at all) is a line like:
host example.com port 5001 protocol https username foo password bar
To match git's representation on a token-by-token basis, you would have
to either split out git's "host:port" pair, or combine the .netrc's
representation to "example.com:5001".
-Peff
^ permalink raw reply
* Re: [PATCH] git-send-email: add ~/.authinfo parsing
From: Ted Zlatanov @ 2013-02-04 21:08 UTC (permalink / raw)
To: Jeff King; +Cc: Michal Nazarewicz, Junio C Hamano, git, Krzysztof Mazur
In-Reply-To: <20130204205911.GA13186@sigill.intra.peff.net>
On Mon, 4 Feb 2013 15:59:11 -0500 Jeff King <peff@peff.net> wrote:
JK> On Mon, Feb 04, 2013 at 03:28:52PM -0500, Ted Zlatanov wrote:
JK> You might want to map this to "port" in .autoinfo separately if it's
JK> available.
>>
>> That would create the following possibilities:
>>
>> * host example.com:31337, protocol https
>> * host example.com:31337, protocol unspecified
>> * host example.com, protocol https
>> * host example.com, protocol unspecified
JK> Possibilities for .netrc, or for git? Git will always specify the
JK> protocol.
Possibilities for the netrc data. How clever do we want to be with
taking 31337 and mapping it to the "protocol"? My preference is to be
very simple here.
JK> What I was more wondering (and I know very little about .netrc, so this
JK> might not be a possibility at all) is a line like:
JK> host example.com port 5001 protocol https username foo password bar
JK> To match git's representation on a token-by-token basis, you would have
JK> to either split out git's "host:port" pair, or combine the .netrc's
JK> representation to "example.com:5001".
Currently, we map both the "port" and "protocol" netrc tokens to the
credential helper protocol's "protocol". So this will have undefined
results. To do what you specify could be pretty simple: we could do a
preliminary scan of the tokens, looking for "host X port Y" where Y is
an integer, and rewriting the host to be "X:Y". That would be clean and
simple, unless the user breaks it with "host x:23 port 22". Let me know
if you agree and I'll do.
Ted
^ permalink raw reply
* Re: [PATCH] Add contrib/credentials/netrc with GPG support
From: Jeff King @ 2013-02-04 21:17 UTC (permalink / raw)
To: Ted Zlatanov; +Cc: Junio C Hamano, git
In-Reply-To: <87ehgvua6h.fsf@lifelogs.com>
On Mon, Feb 04, 2013 at 02:54:30PM -0500, Ted Zlatanov wrote:
> + print <<EOHIPPUS;
> +
> +$0 [-f AUTHFILE] [-d] get
> +
> +Version $VERSION by tzz\@lifelogs.com. License: BSD.
This here-doc is interpolated so you can use $0 and $VERSION, and
therefore have to quote the @-sign. But later in the here-doc...
> +Thus, when we get "protocol=https\nusername=tzz", this credential
> +helper will look for lines in AUTHFILE that match
Do you need to quote "\n" here?
> +die "Sorry, you need to specify an existing netrc file (with or without a .gpg extension) with -f AUTHFILE"
> + unless defined $file;
> +
> +unless (-f $file) {
> + print STDERR "Sorry, the specified netrc $file is not accessible\n" if $debug;
> + exit 0;
> +}
Hmm, so it's not an error (just a warning) to say:
git credential-netrc -f /does/not/exist
but it is an error to say:
git credential-netrc
and have it fail to find any netrc files. Shouldn't the latter be a
lesser error than the former?
> +while (<STDIN>) {
> + next unless m/([^=]+)=(.+)/;
> +
> + my ($token, $value) = ($1, $2);
> + die "Unknown search token $1" unless exists $q{$token};
> + $q{$token} = $value;
> +}
Should this regex be anchored at the start of the string? I think the
left-to-right matching means we will correctly match:
key=value with=in it
so it may be OK.
> +if ($debug) {
> + printf STDERR "searching for %s = %s\n", $_, $q{$_} || '(any value)'
> + foreach sort keys %q;
> +}
Leftover one-char indent.
> [...]
The rest looks OK to me.
^ permalink raw reply
* Re: [PATCH] git-send-email: add ~/.authinfo parsing
From: Jeff King @ 2013-02-04 21:22 UTC (permalink / raw)
To: Ted Zlatanov; +Cc: Michal Nazarewicz, Junio C Hamano, git, Krzysztof Mazur
In-Reply-To: <8738xbu6qj.fsf@lifelogs.com>
On Mon, Feb 04, 2013 at 04:08:52PM -0500, Ted Zlatanov wrote:
> >> That would create the following possibilities:
> >>
> >> * host example.com:31337, protocol https
> >> * host example.com:31337, protocol unspecified
> >> * host example.com, protocol https
> >> * host example.com, protocol unspecified
>
> JK> Possibilities for .netrc, or for git? Git will always specify the
> JK> protocol.
>
> Possibilities for the netrc data. How clever do we want to be with
> taking 31337 and mapping it to the "protocol"? My preference is to be
> very simple here.
I think simple is OK, as we can iterate on it as specific use-cases come
up. The important thing is to make sure we err on the side of "does not
match" and not "oops, we accidentally sent your plaintext credentials to
the wrong server".
> Currently, we map both the "port" and "protocol" netrc tokens to the
> credential helper protocol's "protocol". So this will have undefined
> results. To do what you specify could be pretty simple: we could do a
> preliminary scan of the tokens, looking for "host X port Y" where Y is
> an integer, and rewriting the host to be "X:Y". That would be clean and
> simple, unless the user breaks it with "host x:23 port 22". Let me know
> if you agree and I'll do.
Yeah, I think that is simple and obvious. If the user is saying "host
x:23 port 22", that is nonsensical.
-Peff
^ permalink raw reply
* Re: [PATCH] Add contrib/credentials/netrc with GPG support
From: Ted Zlatanov @ 2013-02-04 21:32 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20130204211726.GB13186@sigill.intra.peff.net>
On Mon, 4 Feb 2013 16:17:26 -0500 Jeff King <peff@peff.net> wrote:
JK> Do you need to quote "\n" here?
Fixed.
JK> Hmm, so it's not an error (just a warning) to say:
JK> git credential-netrc -f /does/not/exist
JK> but it is an error to say:
JK> git credential-netrc
JK> and have it fail to find any netrc files. Shouldn't the latter be a
JK> lesser error than the former?
Fixed, they should both exit(0).
>> + next unless m/([^=]+)=(.+)/;
JK> Should this regex be anchored at the start of the string?
Fixed.
>> + printf STDERR "searching for %s = %s\n", $_, $q{$_} || '(any value)'
>> + foreach sort keys %q;
JK> Leftover one-char indent.
Fixed.
Ted
^ permalink raw reply
* Re: [PATCH] git-send-email: add ~/.authinfo parsing
From: Ted Zlatanov @ 2013-02-04 21:41 UTC (permalink / raw)
To: Jeff King; +Cc: Michal Nazarewicz, Junio C Hamano, git, Krzysztof Mazur
In-Reply-To: <20130204212203.GC13186@sigill.intra.peff.net>
On Mon, 4 Feb 2013 16:22:03 -0500 Jeff King <peff@peff.net> wrote:
>> Currently, we map both the "port" and "protocol" netrc tokens to the
>> credential helper protocol's "protocol". So this will have undefined
>> results. To do what you specify could be pretty simple: we could do a
>> preliminary scan of the tokens, looking for "host X port Y" where Y is
>> an integer, and rewriting the host to be "X:Y". That would be clean and
>> simple, unless the user breaks it with "host x:23 port 22". Let me know
>> if you agree and I'll do.
JK> Yeah, I think that is simple and obvious. If the user is saying "host
JK> x:23 port 22", that is nonsensical.
OK; added.
Ted
^ permalink raw reply
* [PATCH] Add contrib/credentials/netrc with GPG support, try #2
From: Ted Zlatanov @ 2013-02-04 21:44 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20130204211726.GB13186@sigill.intra.peff.net>
Signed-off-by: Ted Zlatanov <tzz@lifelogs.com>
---
contrib/credential/netrc/git-credential-netrc | 236 +++++++++++++++++++++++++
1 files changed, 236 insertions(+), 0 deletions(-)
create mode 100755 contrib/credential/netrc/git-credential-netrc
diff --git a/contrib/credential/netrc/git-credential-netrc b/contrib/credential/netrc/git-credential-netrc
new file mode 100755
index 0000000..d265fde
--- /dev/null
+++ b/contrib/credential/netrc/git-credential-netrc
@@ -0,0 +1,236 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use Getopt::Long;
+use File::Basename;
+
+my $VERSION = "0.1";
+
+my %options = (
+ help => 0,
+ debug => 0,
+
+ # identical token maps, e.g. host -> host, will be inserted later
+ tmap => {
+ port => 'protocol',
+ machine => 'host',
+ path => 'path',
+ login => 'username',
+ user => 'username',
+ password => 'password',
+ }
+ );
+
+# map each credential protocol token to itself on the netrc side
+$options{tmap}->{$_} = $_ foreach values %{$options{tmap}};
+
+foreach my $suffix ('.gpg', '') {
+ foreach my $base (qw/authinfo netrc/) {
+ my $file = glob("~/.$base$suffix");
+ next unless (defined $file && -f $file);
+ $options{file} = $file ;
+ }
+}
+
+Getopt::Long::Configure("bundling");
+
+# TODO: maybe allow the token map $options{tmap} to be configurable.
+GetOptions(\%options,
+ "help|h",
+ "debug|d",
+ "file|f=s",
+ );
+
+if ($options{help}) {
+ my $shortname = basename($0);
+ $shortname =~ s/git-credential-//;
+
+ print <<EOHIPPUS;
+
+$0 [-f AUTHFILE] [-d] get
+
+Version $VERSION by tzz\@lifelogs.com. License: BSD.
+
+Options:
+ -f AUTHFILE: specify a netrc-style file
+ -d: turn on debugging
+
+To enable (note that Git will prepend "git-credential-" to the helper
+name and look for it in the path):
+
+ git config credential.helper '$shortname -f AUTHFILE'
+
+And if you want lots of debugging info:
+
+ git config credential.helper '$shortname -f AUTHFILE -d'
+
+Only "get" mode is supported by this credential helper. It opens
+AUTHFILE and looks for entries that match the requested search
+criteria:
+
+ 'port|protocol':
+ The protocol that will be used (e.g., https). (protocol=X)
+
+ 'machine|host':
+ The remote hostname for a network credential. (host=X)
+
+ 'path':
+ The path with which the credential will be used. (path=X)
+
+ 'login|user|username':
+ The credential’s username, if we already have one. (username=X)
+
+Thus, when we get this query on STDIN:
+
+protocol=https
+username=tzz
+
+this credential helper will look for lines in AUTHFILE that match
+
+port https login tzz
+
+OR
+
+protocol https login tzz
+
+OR... etc. acceptable tokens as listed above. Any unknown tokens are
+simply ignored.
+
+Then, the helper will print out whatever tokens it got from the line,
+including "password" tokens, mapping e.g. "port" back to "protocol".
+
+The first matching line is used. Tokens can be quoted as 'STRING' or
+"STRING".
+
+No caching is performed by this credential helper.
+
+EOHIPPUS
+
+ exit;
+}
+
+my $mode = shift @ARGV;
+
+# credentials may get 'get', 'store', or 'erase' as parameters but
+# only acknowledge 'get'
+die "Syntax: $0 [-f AUTHFILE] [-d] get" unless defined $mode;
+
+# only support 'get' mode
+exit unless $mode eq 'get';
+
+my $debug = $options{debug};
+my $file = $options{file};
+
+unless (defined $file) {
+ print STDERR "Please specify an existing netrc file (with or without a .gpg extension) with -f AUTHFILE\n" if $debug;
+ exit 0;
+}
+
+unless (-f $file) {
+ print STDERR "Sorry, the specified netrc $file is not accessible\n" if $debug;
+ exit 0;
+}
+
+my @data;
+if ($file =~ m/\.gpg$/) {
+ @data = load('-|', qw(gpg --decrypt), $file)
+}
+else {
+ @data = load('<', $file);
+}
+
+chomp @data;
+
+unless (scalar @data) {
+ print STDERR "Sorry, we could not load data from [$file]\n" if $debug;
+ exit;
+}
+
+# the query: start with every token with no value
+my %q = map { $_ => undef } values(%{$options{tmap}});
+
+while (<STDIN>) {
+ next unless m/^([^=]+)=(.+)/;
+
+ my ($token, $value) = ($1, $2);
+ die "Unknown search token $1" unless exists $q{$token};
+ $q{$token} = $value;
+}
+
+# build reverse token map
+my %rmap;
+foreach my $k (keys %{$options{tmap}}) {
+ push @{$rmap{$options{tmap}->{$k}}}, $k;
+}
+
+# there are CPAN modules to do this better, but we want to avoid
+# dependencies and generally, complex netrc-style files are rare
+
+if ($debug) {
+ printf STDERR "searching for %s = %s\n", $_, $q{$_} || '(any value)'
+ foreach sort keys %q;
+}
+
+LINE: foreach my $line (@data) {
+
+ print STDERR "line [$line]\n" if $debug;
+ my @tok;
+ # gratefully stolen from Net::Netrc
+ while (length $line &&
+ $line =~ s/^("((?:[^"]+|\\.)*)"|((?:[^\\\s]+|\\.)*))\s*//) {
+ (my $tok = $+) =~ s/\\(.)/$1/g;
+ push(@tok, $tok);
+ }
+
+ # skip blank lines, comments, etc.
+ next LINE unless scalar @tok;
+
+ my %tokens;
+ my $num_port;
+ while (@tok) {
+ my ($k, $v) = (shift @tok, shift @tok);
+ next unless defined $v;
+ next unless exists $options{tmap}->{$k};
+ $tokens{$options{tmap}->{$k}} = $v;
+ $num_port = $v if $k eq 'port' && $v =~ m/^\d+$/;
+ }
+
+ # for "host X port Y" where Y is an integer (captured by
+ # $num_port above), set the host to "X:Y"
+ $tokens{host} = join(':', $tokens{host}, $num_port)
+ if defined $tokens{host} && defined $num_port;
+
+ foreach my $check (sort keys %q) {
+ if (exists $tokens{$check} && defined $q{$check}) {
+ print STDERR "comparing [$tokens{$check}] to [$q{$check}] in line [$line]\n" if $debug;
+ next LINE unless $tokens{$check} eq $q{$check};
+ }
+ else {
+ print STDERR "we could not find [$check] but it's OK\n" if $debug;
+ }
+ }
+
+ print STDERR "line has passed all the search checks\n" if $debug;
+ TOKEN:
+ foreach my $token (sort keys %rmap) {
+ print STDERR "looking for useful token $token\n" if $debug;
+ next unless exists $tokens{$token}; # did we match?
+
+ foreach my $rctoken (@{$rmap{$token}}) {
+ next TOKEN if defined $q{$rctoken}; # don't re-print given tokens
+ }
+
+ print STDERR "FOUND: $token=$tokens{$token}\n" if $debug;
+ printf "%s=%s\n", $token, $tokens{$token};
+ }
+
+ last;
+}
+
+sub load {
+ # this supports pipes too
+ my $io = new IO::File(@_) or die "Could not open [@_]: $!\n";
+ return <$io>; # whole file
+}
--
1.7.9.rc2
^ permalink raw reply related
* Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2
From: Junio C Hamano @ 2013-02-04 22:56 UTC (permalink / raw)
To: Ted Zlatanov; +Cc: Jeff King, git
In-Reply-To: <87mwvjsqjc.fsf_-_@lifelogs.com>
Ted Zlatanov <tzz@lifelogs.com> writes:
> Signed-off-by: Ted Zlatanov <tzz@lifelogs.com>
The space above your S-o-b: could be utilized a bit better ;-)
> @@ -0,0 +1,236 @@
> +#!/usr/bin/perl
> +
> +use strict;
> +use warnings;
> +
> +use Getopt::Long;
> +use File::Basename;
> +
> +my $VERSION = "0.1";
> +
> +my %options = (
> + help => 0,
> + debug => 0,
> +
> + # identical token maps, e.g. host -> host, will be inserted later
> + tmap => {
> + port => 'protocol',
> + machine => 'host',
> + path => 'path',
> + login => 'username',
> + user => 'username',
> + password => 'password',
> + }
> + );
> +
> +# map each credential protocol token to itself on the netrc side
> +$options{tmap}->{$_} = $_ foreach values %{$options{tmap}};
> +
> +foreach my $suffix ('.gpg', '') {
> + foreach my $base (qw/authinfo netrc/) {
> + my $file = glob("~/.$base$suffix");
> + next unless (defined $file && -f $file);
> + $options{file} = $file ;
> + }
> +}
This checks .gpg first and then unencrypted, and checks authinfo
first and netrc second, both of which makes sense. It is good to
encourage use of encrypted files, and it is good to use newer
authinfo files over netrc files.
However, it is strange that you let the ones that are discovered
later in the loop to override the ones that are discovered earlier.
Perhaps you meant
next unless (... exists there ...);
$options{"file"} = $file;
last;
instead?
> +Getopt::Long::Configure("bundling");
Hmm, OK.
> +# TODO: maybe allow the token map $options{tmap} to be configurable.
> +GetOptions(\%options,
> + "help|h",
> + "debug|d",
> + "file|f=s",
> + );
> +
> +if ($options{help}) {
> + my $shortname = basename($0);
> + $shortname =~ s/git-credential-//;
> +
> + print <<EOHIPPUS;
> +
> +$0 [-f AUTHFILE] [-d] get
> +
> +Version $VERSION by tzz\@lifelogs.com. License: BSD.
> +
> +Options:
> + -f AUTHFILE: specify a netrc-style file
> + -d: turn on debugging
> +
> +To enable (note that Git will prepend "git-credential-" to the helper
> +name and look for it in the path):
> +
> + git config credential.helper '$shortname -f AUTHFILE'
> +
> +And if you want lots of debugging info:
> +
> + git config credential.helper '$shortname -f AUTHFILE -d'
> +
> +Only "get" mode is supported by this credential helper. It opens
> +AUTHFILE and looks for entries that match the requested search
> +criteria:
> +
> + 'port|protocol':
> + The protocol that will be used (e.g., https). (protocol=X)
> +
> + 'machine|host':
> + The remote hostname for a network credential. (host=X)
> +
> + 'path':
> + The path with which the credential will be used. (path=X)
> +
> + 'login|user|username':
> + The credential’s username, if we already have one. (username=X)
> +
> +Thus, when we get this query on STDIN:
> +
> +protocol=https
> +username=tzz
> +
> +this credential helper will look for lines in AUTHFILE that match
> +
> +port https login tzz
> +
> +OR
> +
> +protocol https login tzz
> +
> +OR... etc. acceptable tokens as listed above. Any unknown tokens are
> +simply ignored.
I recall that netrc/authinfo files are _not_ line oriented. Earlier
you said "looks for entries that match" which is a lot more correct,
but then we see "look for lines in authfile".
> +Then, the helper will print out whatever tokens it got from the line,
> +including "password" tokens, mapping e.g. "port" back to "protocol".
Again "line" is mentioned twice, above and below.
> +The first matching line is used. Tokens can be quoted as 'STRING' or
> +"STRING".
> +
> +No caching is performed by this credential helper.
> +
> +EOHIPPUS
> +
> + exit;
> +}
> +my $mode = shift @ARGV;
> +
> +# credentials may get 'get', 'store', or 'erase' as parameters but
> +# only acknowledge 'get'
> +die "Syntax: $0 [-f AUTHFILE] [-d] get" unless defined $mode;
> +
> +# only support 'get' mode
> +exit unless $mode eq 'get';
The above looks strange. Why does the invoker get the error message
only when it runs this without arguments? Did you mean to say more
like this?
unless (defined $mode && $mode eq 'get') {
die "...";
}
By the way, I think statement modifiers tend to get overused and
make the resulting program harder to read. die "..." at the
beginning of line makes the reader go "Whoa, it already is done and
existing on error", and then forces the eyes to scan the error
message to find "unless" and the condition.
It may be a cute syntax and some may find it even cool, but cuteness
or coolness is less valuable compared with the readability.
> +my $debug = $options{debug};
> +my $file = $options{file};
> +
> +unless (defined $file) {
> + print STDERR "Please specify an existing netrc file (with or without a .gpg extension) with -f AUTHFILE\n" if $debug;
> + exit 0;
> +}
> +
> +unless (-f $file) {
> + print STDERR "Sorry, the specified netrc $file is not accessible\n" if $debug;
> + exit 0;
> +}
Perhaps "-r $file", if you say "is not accessible"?
Is it sensible to squelch the error message by default and force
user to specify --debug? You could argue that the option is to
debug the user's configuration, but the name of the option sounds
more like it is for debugging this script itself.
I saw Peff already pointed out error conditions, but I am not sure
why all of these exit with 0. If the user has configured
git config credential.helper 'netrc -f $HOME/.netcr'
shouldn't it be diagnosed as an error? It is understandable to let
this go silently
git config credential.helper 'netrc'
and let other credential helpers take over when no $HOME/.{netrc,authinfo}{,.gpg}
file exist, but in that case the user may still want to remove the
config item that is not doing anything useful and erroring out with
a message may be a way to help the user know about the situation.
> +my @data;
> +if ($file =~ m/\.gpg$/) {
> + @data = load('-|', qw(gpg --decrypt), $file)
> +}
> +else {
> + @data = load('<', $file);
> +}
> +
> +chomp @data;
> +
> +unless (scalar @data) {
Shouldn't this error check come logically before chomping?
> + print STDERR "Sorry, we could not load data from [$file]\n" if $debug;
> + exit;
> +}
Is this really an error? The file perhaps was empty. Shouldn't
that case treated the same way as the case where no entry that
matches the criteria invoker gave you was found?
^ permalink raw reply
* Re: [PATCH 1/3] Add contrib/credentials/netrc with GPG support
From: Junio C Hamano @ 2013-02-04 23:10 UTC (permalink / raw)
To: Ted Zlatanov; +Cc: git, Jeff King
In-Reply-To: <87lib3uats.fsf@lifelogs.com>
Ted Zlatanov <tzz@lifelogs.com> writes:
> JCH> I thought that we tend to avoid Emacs/Vim formatting cruft left in
> JCH> the file. Do we have any in existing file outside contrib/?
>
> No, but it's a nice way to express the settings so no one is guessing
> what the project prefers. At least for me it's not an issue anymore,
> since I understand your criteria better now, so let me know if you want
> me to express it in the CodingGuidelines, in a dir-locals.el file, or
> somewhere else.
Historically we treated this from CodingGuidelines a sufficient
clue:
As for more concrete guidelines, just imitate the existing code
(this is a good guideline, no matter which project you are
contributing to). It is always preferable to match the _local_
convention. New code added to git suite is expected to match
the overall style of existing code. Modifications to existing
code is expected to match the style the surrounding code already
uses (even if it doesn't match the overall style of existing code).
but over time people wanted more specific guidelines and added
language specific style guides there. We have sections that cover
C, shell and Python, and I do not think adding Perl would not hurt.
^ permalink raw reply
* Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2
From: Jeff King @ 2013-02-04 23:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ted Zlatanov, git
In-Reply-To: <7vd2wf1yex.fsf@alter.siamese.dyndns.org>
On Mon, Feb 04, 2013 at 02:56:06PM -0800, Junio C Hamano wrote:
> > +my $mode = shift @ARGV;
> > +
> > +# credentials may get 'get', 'store', or 'erase' as parameters but
> > +# only acknowledge 'get'
> > +die "Syntax: $0 [-f AUTHFILE] [-d] get" unless defined $mode;
> > +
> > +# only support 'get' mode
> > +exit unless $mode eq 'get';
>
> The above looks strange. Why does the invoker get the error message
> only when it runs this without arguments? Did you mean to say more
> like this?
>
> unless (defined $mode && $mode eq 'get') {
> die "...";
> }
Not having a mode is an invocation error; the credential-helper
documentation indicates that the helper will always be invoked with an
action. The likely culprit for not having one is the user invoking it
manually, and showing the usage there is a sensible action.
Whereas invoking it with a mode other than "get" is not an error at all.
Git will run it with the "store" and "erase" actions, too. Those happen
to be no-ops for this helper, so it exits silently. The credential docs
specify that any other actions should be ignored, too, to allow for
future expansion.
> > +my $debug = $options{debug};
> > +my $file = $options{file};
> > +
> > +unless (defined $file) {
> > + print STDERR "Please specify an existing netrc file (with or without a .gpg extension) with -f AUTHFILE\n" if $debug;
> > + exit 0;
> > +}
> > +
> > +unless (-f $file) {
> > + print STDERR "Sorry, the specified netrc $file is not accessible\n" if $debug;
> > + exit 0;
> > +}
>
> Perhaps "-r $file", if you say "is not accessible"?
Even better: look at whether opening the file was successful. Though I
guess that is complicated by the use of gpg, who will probably not
distinguish ENOENT from other failures for us.
> Is it sensible to squelch the error message by default and force
> user to specify --debug? You could argue that the option is to
> debug the user's configuration, but the name of the option sounds
> more like it is for debugging this script itself.
>
> I saw Peff already pointed out error conditions, but I am not sure
> why all of these exit with 0. If the user has configured
It was from my suggestion to ignore missing files, which is that the
user might have the helper configured (e.g., via /etc/gitconfig, or by a
shared ~/.gitconfig) but not actually have a netrc.
It gets confusing because the contents of $file may have been
auto-detected, or it may have come from the command-line, and we do not
remember which at this point.
I was trying not to be too nit-picky with my review, but here is how I
would have written the outer logic of the script:
my $tokens = read_credential_data_from_stdin();
if ($options{file}) {
my @entries = load_netrc($options{file})
or die "unable to open $options{file}: $!";
check_netrc($tokens, @entries);
}
else {
foreach my $ext ('.gpg', '') {
foreach my $base (qw(authinfo netrc)) {
my @entries = load_netrc("$base$ext")
or next;
if (check_netrc($tokens, @entries)) {
last;
}
}
}
}
I.e., to fail on "-f", but otherwise treat unreadable auto-selected
files as a no-op, for whatever reason. I'd also consider checking all
files if they are available, in case the user has multiple (e.g., they
keep low-quality junk unencrypted but some high-security passwords in a
.gpg file). Not that likely, but not any harder to implement.
-Peff
^ permalink raw reply
* [PATCHv3] Add contrib/credentials/netrc with GPG support
From: Ted Zlatanov @ 2013-02-04 23:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7vd2wf1yex.fsf@alter.siamese.dyndns.org>
Changes since PATCHv2:
- don't keep looking at netrc candidates if one good one is found
- fixed wording of "line" to "entry" everywhere suitable
- many (but not all) statement modifiers changed to block format
- use -r everywhere instead of -f
- move chomp to when we know @data has contents
Signed-off-by: Ted Zlatanov <tzz@lifelogs.com>
---
contrib/credential/netrc/git-credential-netrc | 243 +++++++++++++++++++++++++
1 files changed, 243 insertions(+), 0 deletions(-)
create mode 100755 contrib/credential/netrc/git-credential-netrc
diff --git a/contrib/credential/netrc/git-credential-netrc b/contrib/credential/netrc/git-credential-netrc
new file mode 100755
index 0000000..99ab204
--- /dev/null
+++ b/contrib/credential/netrc/git-credential-netrc
@@ -0,0 +1,243 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use Getopt::Long;
+use File::Basename;
+
+my $VERSION = "0.1";
+
+my %options = (
+ help => 0,
+ debug => 0,
+
+ # identical token maps, e.g. host -> host, will be inserted later
+ tmap => {
+ port => 'protocol',
+ machine => 'host',
+ path => 'path',
+ login => 'username',
+ user => 'username',
+ password => 'password',
+ }
+ );
+
+# map each credential protocol token to itself on the netrc side
+foreach (values %{$options{tmap}}) {
+ $options{tmap}->{$_} = $_;
+}
+
+FILE:
+foreach my $suffix ('.gpg', '') {
+ foreach my $base (qw/authinfo netrc/) {
+ my $file = glob("~/.$base$suffix");
+ next unless (defined $file && -r $file);
+ $options{file} = $file;
+ last FILE;
+ }
+}
+
+Getopt::Long::Configure("bundling");
+
+# TODO: maybe allow the token map $options{tmap} to be configurable.
+GetOptions(\%options,
+ "help|h",
+ "debug|d",
+ "file|f=s",
+ );
+
+if ($options{help}) {
+ my $shortname = basename($0);
+ $shortname =~ s/git-credential-//;
+
+ print <<EOHIPPUS;
+
+$0 [-f AUTHFILE] [-d] get
+
+Version $VERSION by tzz\@lifelogs.com. License: BSD.
+
+Options:
+ -f AUTHFILE: specify a netrc-style file
+ -d: turn on debugging
+
+To enable (note that Git will prepend "git-credential-" to the helper
+name and look for it in the path):
+
+ git config credential.helper '$shortname -f AUTHFILE'
+
+And if you want lots of debugging info:
+
+ git config credential.helper '$shortname -f AUTHFILE -d'
+
+Only "get" mode is supported by this credential helper. It opens
+AUTHFILE and looks for entries that match the requested search
+criteria:
+
+ 'port|protocol':
+ The protocol that will be used (e.g., https). (protocol=X)
+
+ 'machine|host':
+ The remote hostname for a network credential. (host=X)
+
+ 'path':
+ The path with which the credential will be used. (path=X)
+
+ 'login|user|username':
+ The credential’s username, if we already have one. (username=X)
+
+Thus, when we get this query on STDIN:
+
+protocol=https
+username=tzz
+
+this credential helper will look for entries in AUTHFILE that match
+
+port https login tzz
+
+OR
+
+protocol https login tzz
+
+OR... etc. acceptable tokens as listed above. Any unknown tokens are
+simply ignored.
+
+Then, the helper will print out whatever tokens it got from the entry,
+including "password" tokens, mapping e.g. "port" back to "protocol".
+
+The first matching entry is used. Tokens can be quoted as 'STRING' or
+"STRING".
+
+No caching is performed by this credential helper.
+
+EOHIPPUS
+
+ exit;
+}
+
+my $mode = shift @ARGV;
+
+# credentials may get 'get', 'store', or 'erase' as parameters but
+# only acknowledge 'get'
+die "Syntax: $0 [-f AUTHFILE] [-d] get" unless defined $mode;
+
+# only support 'get' mode
+exit unless $mode eq 'get';
+
+my $debug = $options{debug};
+my $file = $options{file};
+
+unless (defined $file) {
+ print STDERR "Please specify an existing netrc file (with or without a .gpg extension) with -f AUTHFILE\n" if $debug;
+ exit 0;
+}
+
+unless (-r $file) {
+ print STDERR "Sorry, the specified netrc $file is not accessible\n" if $debug;
+ exit 0;
+}
+
+my @data;
+if ($file =~ m/\.gpg$/) {
+ @data = load('-|', qw(gpg --decrypt), $file)
+}
+else {
+ @data = load('<', $file);
+}
+
+unless (scalar @data) {
+ print STDERR "Sorry, we could not load data from [$file]\n" if $debug;
+ exit;
+}
+
+chomp @data;
+
+# the query: start with every token with no value
+my %q = map { $_ => undef } values(%{$options{tmap}});
+
+while (<STDIN>) {
+ next unless m/^([^=]+)=(.+)/;
+
+ my ($token, $value) = ($1, $2);
+ die "Unknown search token $1" unless exists $q{$token};
+ $q{$token} = $value;
+}
+
+# build reverse token map
+my %rmap;
+foreach my $k (keys %{$options{tmap}}) {
+ push @{$rmap{$options{tmap}->{$k}}}, $k;
+}
+
+# there are CPAN modules to do this better, but we want to avoid
+# dependencies and generally, complex netrc-style files are rare
+
+if ($debug) {
+ foreach (sort keys %q) {
+ printf STDERR "searching for %s = %s\n", $_, $q{$_} || '(any value)';
+ }
+}
+
+LINE: foreach my $line (@data) {
+
+ print STDERR "line [$line]\n" if $debug;
+ my @tok;
+ # gratefully stolen from Net::Netrc
+ while (length $line &&
+ $line =~ s/^("((?:[^"]+|\\.)*)"|((?:[^\\\s]+|\\.)*))\s*//) {
+ (my $tok = $+) =~ s/\\(.)/$1/g;
+ push(@tok, $tok);
+ }
+
+ # skip blank lines, comments, etc.
+ next LINE unless scalar @tok;
+
+ my %tokens;
+ my $num_port;
+ while (@tok) {
+ my ($k, $v) = (shift @tok, shift @tok);
+ next unless defined $v;
+ next unless exists $options{tmap}->{$k};
+ $tokens{$options{tmap}->{$k}} = $v;
+ $num_port = ($k eq 'port' && $v =~ m/^\d+$/) ? $v : undef;
+ }
+
+ # for "host X port Y" where Y is an integer (captured by
+ # $num_port above), set the host to "X:Y"
+ if (defined $tokens{host} && defined $num_port) {
+ $tokens{host} = join(':', $tokens{host}, $num_port);
+ }
+
+ foreach my $check (sort keys %q) {
+ if (exists $tokens{$check} && defined $q{$check}) {
+ print STDERR "comparing [$tokens{$check}] to [$q{$check}] in entry [$line]\n" if $debug;
+ next LINE unless $tokens{$check} eq $q{$check};
+ }
+ else {
+ print STDERR "we could not find [$check] but it's OK\n" if $debug;
+ }
+ }
+
+ print STDERR "entry has passed all the search checks\n" if $debug;
+ TOKEN:
+ foreach my $token (sort keys %rmap) {
+ print STDERR "looking for useful token $token\n" if $debug;
+ next unless exists $tokens{$token}; # did we match?
+
+ foreach my $rctoken (@{$rmap{$token}}) {
+ # don't re-print given tokens
+ next TOKEN if defined $q{$rctoken};
+ }
+
+ print STDERR "FOUND: $token=$tokens{$token}\n" if $debug;
+ printf "%s=%s\n", $token, $tokens{$token};
+ }
+
+ last;
+}
+
+sub load {
+ # this supports pipes too
+ my $io = new IO::File(@_) or die "Could not open [@_]: $!\n";
+ return <$io>; # whole file
+}
--
1.7.9.rc2
^ permalink raw reply related
* Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2
From: Ted Zlatanov @ 2013-02-04 23:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7vd2wf1yex.fsf@alter.siamese.dyndns.org>
On Mon, 04 Feb 2013 14:56:06 -0800 Junio C Hamano <gitster@pobox.com> wrote:
JCH> I recall that netrc/authinfo files are _not_ line oriented. Earlier
JCH> you said "looks for entries that match" which is a lot more correct,
JCH> but then we see "look for lines in authfile".
Hmm, do you mean backslashed newlines? I think the Net::Netrc parser
doesn't support them, and I haven't seen them in the wild, but I could
support them if you think that's useful.
>> +my $mode = shift @ARGV;
>> +
>> +# credentials may get 'get', 'store', or 'erase' as parameters but
>> +# only acknowledge 'get'
>> +die "Syntax: $0 [-f AUTHFILE] [-d] get" unless defined $mode;
>> +
>> +# only support 'get' mode
>> +exit unless $mode eq 'get';
JCH> The above looks strange. Why does the invoker get the error message
JCH> only when it runs this without arguments? Did you mean to say more
JCH> like this?
JCH> unless (defined $mode && $mode eq 'get') {
JCH> die "...";
JCH> }
I mean:
- if the mode is not given, exit badly (since it's required)
- if the mode is given but we don't support it, exit pleasantly
I thought that was the right thing, according to my reading of the
credentials API. If not, I'll be glad to change it.
JCH> By the way, I think statement modifiers tend to get overused and
JCH> make the resulting program harder to read. die "..." at the
JCH> beginning of line makes the reader go "Whoa, it already is done and
JCH> existing on error", and then forces the eyes to scan the error
JCH> message to find "unless" and the condition.
JCH> It may be a cute syntax and some may find it even cool, but cuteness
JCH> or coolness is less valuable compared with the readability.
Your coding guidelines said you prefer one-line if statements, and I
thought it would be OK to lean on modifiers. I changed many of the
modifiers but not all; please let me know if you'd like me to change
them all. It's no problem.
JCH> Is it sensible to squelch the error message by default and force
JCH> user to specify --debug? You could argue that the option is to
JCH> debug the user's configuration, but the name of the option sounds
JCH> more like it is for debugging this script itself.
It's both... without a clear separation because it's such a small
script. Let me know how you'd like to change it, if at all.
JCH> I saw Peff already pointed out error conditions, but I am not sure
JCH> why all of these exit with 0. If the user has configured
JCH> git config credential.helper 'netrc -f $HOME/.netcr'
JCH> shouldn't it be diagnosed as an error? It is understandable to let
JCH> this go silently
JCH> git config credential.helper 'netrc'
JCH> and let other credential helpers take over when no $HOME/.{netrc,authinfo}{,.gpg}
JCH> file exist, but in that case the user may still want to remove the
JCH> config item that is not doing anything useful and erroring out with
JCH> a message may be a way to help the user know about the situation.
You and Peff should tell me how it should behave, or perhaps make the
changes after it's in. I'm happy to change it any way you like, but at
this point I'm just following instructions, not really contributing,
about the exit statuses. I thought I knew what you wanted 2 iterations
ago :)
>> + print STDERR "Sorry, we could not load data from [$file]\n" if $debug;
>> + exit;
JCH> Is this really an error? The file perhaps was empty. Shouldn't
JCH> that case treated the same way as the case where no entry that
JCH> matches the criteria invoker gave you was found?
exit(0) is not an error, so the behavior is exactly the same, we just
don't print anything to STDOUT because there was no data, with a nicer
error message. I think that's what we want?
PATCHv3 is out with the rest of your suggestions. Thank you for the
thorough review. I am happy to improve the script to meet your standards.
Thanks
Ted
^ permalink raw reply
* Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2
From: Junio C Hamano @ 2013-02-04 23:36 UTC (permalink / raw)
To: Jeff King; +Cc: Ted Zlatanov, git
In-Reply-To: <20130204232317.GA17705@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Feb 04, 2013 at 02:56:06PM -0800, Junio C Hamano wrote:
>
>> > +my $mode = shift @ARGV;
>> > +
>> > +# credentials may get 'get', 'store', or 'erase' as parameters but
>> > +# only acknowledge 'get'
>> > +die "Syntax: $0 [-f AUTHFILE] [-d] get" unless defined $mode;
>> > +
>> > +# only support 'get' mode
>> > +exit unless $mode eq 'get';
>>
>> The above looks strange. Why does the invoker get the error message
>> only when it runs this without arguments? Did you mean to say more
>> like this?
>>
>> unless (defined $mode && $mode eq 'get') {
>> die "...";
>> }
>
> Not having a mode is an invocation error; the credential-helper
> documentation indicates that the helper will always be invoked with an
> action. The likely culprit for not having one is the user invoking it
> manually, and showing the usage there is a sensible action.
>
> Whereas invoking it with a mode other than "get" is not an error at all.
> Git will run it with the "store" and "erase" actions, too. Those happen
> to be no-ops for this helper, so it exits silently. The credential docs
> specify that any other actions should be ignored, too, to allow for
> future expansion.
OK. The code didn't express the above reasoning clearly enough.
> I was trying not to be too nit-picky with my review,...
I wasn't either. Mine was still at design level review to get the
semantics right (e.g. what to consider as errors, the input is _not_
one entry per line, etc.), before reviewing the details of the
implementation.
> but here is how I
> would have written the outer logic of the script:
>
> my $tokens = read_credential_data_from_stdin();
> if ($options{file}) {
> my @entries = load_netrc($options{file})
> or die "unable to open $options{file}: $!";
> check_netrc($tokens, @entries);
> }
> else {
> foreach my $ext ('.gpg', '') {
> foreach my $base (qw(authinfo netrc)) {
> my @entries = load_netrc("$base$ext")
> or next;
> if (check_netrc($tokens, @entries)) {
> last;
> }
> }
> }
> }
>
> I.e., to fail on "-f", but otherwise treat unreadable auto-selected
> files as a no-op, for whatever reason. I'd also consider checking all
> files if they are available, in case the user has multiple (e.g., they
> keep low-quality junk unencrypted but some high-security passwords in a
> .gpg file). Not that likely, but not any harder to implement.
Yeah, I think that looks like the right top-level codeflow.
^ permalink raw reply
* Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2
From: Junio C Hamano @ 2013-02-04 23:40 UTC (permalink / raw)
To: Ted Zlatanov; +Cc: Jeff King, git
In-Reply-To: <87bobzslke.fsf@lifelogs.com>
Ted Zlatanov <tzz@lifelogs.com> writes:
>>> +my $mode = shift @ARGV;
>>> +
>>> +# credentials may get 'get', 'store', or 'erase' as parameters but
>>> +# only acknowledge 'get'
>>> +die "Syntax: $0 [-f AUTHFILE] [-d] get" unless defined $mode;
>>> +
>>> +# only support 'get' mode
>>> +exit unless $mode eq 'get';
>
> JCH> The above looks strange. Why does the invoker get the error message
> JCH> only when it runs this without arguments? Did you mean to say more
> JCH> like this?
>
> JCH> unless (defined $mode && $mode eq 'get') {
> JCH> die "...";
> JCH> }
>
> I mean:
>
> - if the mode is not given, exit badly (since it's required)
>
> - if the mode is given but we don't support it, exit pleasantly
>
> I thought that was the right thing, according to my reading of the
> credentials API. If not, I'll be glad to change it.
As Peff noted, I mistead what the code was doing, especially with
somewhat cryptic "only support x mode" comment, as if it is
rejecting other modes.
>>> + print STDERR "Sorry, we could not load data from [$file]\n" if $debug;
>>> + exit;
>
> JCH> Is this really an error? The file perhaps was empty. Shouldn't
> JCH> that case treated the same way as the case where no entry that
> JCH> matches the criteria invoker gave you was found?
>
> exit(0) is not an error, so the behavior is exactly the same, we just
> don't print anything to STDOUT because there was no data, with a nicer
> error message. I think that's what we want?
"Sorry we couldn't" sounded like an error messag to me. If this is
a normal exit, then please make sure it is a normal exit.
The review cycle is not like reviewers give you instructions and
designs and you blindly implement them. It is a creative process
where you show the design and a clear implementation of that design.
Thanks.
^ permalink raw reply
* Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2
From: Ted Zlatanov @ 2013-02-04 23:42 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20130204232317.GA17705@sigill.intra.peff.net>
On Mon, 4 Feb 2013 18:23:17 -0500 Jeff King <peff@peff.net> wrote:
>> Perhaps "-r $file", if you say "is not accessible"?
JK> Even better: look at whether opening the file was successful. Though I
JK> guess that is complicated by the use of gpg, who will probably not
JK> distinguish ENOENT from other failures for us.
Yup. I think the outcome for the user will be the same, so this is
mostly for debugging, right? And we do look at the outcome of opening
the file, and die if that failed (which would change if your suggestion
below is implemented).
JK> I was trying not to be too nit-picky with my review, but here is how I
JK> would have written the outer logic of the script:
JK> my $tokens = read_credential_data_from_stdin();
JK> if ($options{file}) {
JK> my @entries = load_netrc($options{file})
JK> or die "unable to open $options{file}: $!";
JK> check_netrc($tokens, @entries);
JK> }
JK> else {
JK> foreach my $ext ('.gpg', '') {
JK> foreach my $base (qw(authinfo netrc)) {
JK> my @entries = load_netrc("$base$ext")
JK> or next;
JK> if (check_netrc($tokens, @entries)) {
JK> last;
JK> }
JK> }
JK> }
JK> }
JK> I.e., to fail on "-f", but otherwise treat unreadable auto-selected
JK> files as a no-op, for whatever reason.
JK> I'd also consider checking all files if they are available, in case
JK> the user has multiple (e.g., they keep low-quality junk unencrypted
JK> but some high-security passwords in a .gpg file). Not that likely,
JK> but not any harder to implement.
I think that makes everything more complicated, and the user can name a
specific netrc file in the helper spec if he wants it. It's too
automagic for me. But if you and Junio feel this is the right approach,
I'll rewrite to basically allow --file to take a list of filenames and
default that list to the base list of ~/.{authinfo,netrc}{,.gpg}
Ted
^ permalink raw reply
* Re: [PATCH] Verify Content-Type from smart HTTP servers
From: Shawn Pearce @ 2013-02-04 23:49 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20130204083824.GB30835@sigill.intra.peff.net>
On Mon, Feb 4, 2013 at 12:38 AM, Jeff King <peff@peff.net> wrote:
> On Sun, Feb 03, 2013 at 11:17:33PM -0800, Junio C Hamano wrote:
>
> > Does this look good to both of you (relative to Shawn's patch)?
> >
> > remote-curl.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/remote-curl.c b/remote-curl.c
> > index e6f3b63..933c69a 100644
> > --- a/remote-curl.c
> > +++ b/remote-curl.c
> > @@ -134,14 +134,14 @@ static struct discovery* discover_refs(const char *service)
> > last->buf_alloc = strbuf_detach(&buffer, &last->len);
> > last->buf = last->buf_alloc;
> >
> > - if (maybe_smart && 5 <= last->len && last->buf[4] == '#') {
> > + strbuf_addf(&exp, "application/x-%s-advertisement", service);
> > + if (maybe_smart &&
> > + (5 <= last->len && last->buf[4] == '#') &&
> > + !strbuf_cmp(&exp, &type)) {
> > /*
> > * smart HTTP response; validate that the service
> > * pkt-line matches our request.
> > */
> > - strbuf_addf(&exp, "application/x-%s-advertisement", service);
> > - if (strbuf_cmp(&exp, &type))
> > - die("invalid content-type %s", type.buf);
> > if (packet_get_line(&buffer, &last->buf, &last->len) <= 0)
> > die("%s has invalid packet header", refs_url);
> > if (buffer.len && buffer.buf[buffer.len - 1] == '\n')
>
> Yeah, I think that's fine. Thanks.
Looks fine to me too, but I think the test won't work now. :-)
^ permalink raw reply
* Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2
From: Ted Zlatanov @ 2013-02-04 23:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7vy5f3zlzj.fsf@alter.siamese.dyndns.org>
On Mon, 04 Feb 2013 15:40:32 -0800 Junio C Hamano <gitster@pobox.com> wrote:
JCH> "Sorry we couldn't" sounded like an error messag to me. If this is
JCH> a normal exit, then please make sure it is a normal exit.
OK; done in PATCHv4: removed all "Sorry" because they are not abnormal
exits. I'll hold PATCHv4 until the below are known.
JCH> The review cycle is not like reviewers give you instructions and
JCH> designs and you blindly implement them. It is a creative process
JCH> where you show the design and a clear implementation of that design.
OK. I would like you to make the decisions I asked for, though:
- do you want to support backslashed newlines?
- do you want me to remove the statement modifiers?
- should all die() calls just print to STDERR and exit(0)?
- do you want to support multiple netrc files, as you and Peff suggested?
On all of those, I can go either way, it's just a little more work for me.
Ted
^ permalink raw reply
* A fast alternative to git-filter-branch - The BFG Repo-Cleaner
From: Roberto Tyley @ 2013-02-05 0:04 UTC (permalink / raw)
To: git
I recently released The BFG Repo-Cleaner, a new tool for cleansing bad
data out of Git repository histories. The BFG is typically at least
10-50x faster than git-filter-branch at these tasks:
* Removing Crazy Big Files from repo history
* Removing Passwords, Credentials & other Private data
http://rtyley.github.com/bfg-repo-cleaner/
As an example, these are timings for deleting an arbitrary file from
the large GCC repository (148495 commits):
The BFG : 3m29s
$ bfg -D README-fixinc
git filter-branch : 472m31s
$ git filter-branch --index-filter 'git rm --cached --ignore-unmatch
gcc/README-fixinc' --prune-empty --tag-name-filter cat -- --all
(roughly a 135x speed increase, reducing the task of processing a
large codebase from an overnight job to the work of a few minutes....
all timings done in a 4GB tmpfs ramdisk)
The BFG has some simple but very powerful command-line options, which
perform at similar speed:
remove all blobs bigger than 1 megabyte :
$ bfg --strip-blobs-bigger-than 1M my-repo.git
replace all passwords (listed in a file 'passwords.txt') with ***REMOVED*** :
$ bfg --replace-banned-strings passwords.txt my-repo.git
The main source of the BFG's performance advantage comes from
preventing repeated examination of the same tree objects. The approach
of git-filter-branch performs filtering for each commit, against the
complete file-hierarchy of each commit, one after the other, even
though commit trees are largely very similar. For the use-cases of The
BFG that's unnecessary- we don't care where, and in which commit, a
'bad' file exists - we just want it dealt with. Consequently the BFG
processes the Git object db on a memoised tree-by-tree basis,
processing each and every file & folder exactly once - the final
processing of the commit hierarchy is very quick. This _does_ mean
that it's not possible to delete files based on their absolute path
within the repo, but they can deleted based on their filename,
blob-id, or contents. This, and multi-core processing by default,
gives the dramatic speed-up while still providing the same results.
There's more performance data here:
https://docs.google.com/spreadsheet/ccc?key=0AsR1d5Zpes8HdER3VGU1a3dOcmVHMmtzT2dsS2xNenc
I'd welcome feedback, and if anyone has cause to filter a repository's
history in future, I'd appreciate you giving the BFG a try and letting
me know how you found it.
thanks,
Roberto Tyley
software dev @ The Guardian
http://rtyley.github.com/bfg-repo-cleaner/
^ permalink raw reply
* Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2
From: Junio C Hamano @ 2013-02-05 0:15 UTC (permalink / raw)
To: Ted Zlatanov; +Cc: Jeff King, git
In-Reply-To: <87zjzjr5y4.fsf@lifelogs.com>
Ted Zlatanov <tzz@lifelogs.com> writes:
> On Mon, 04 Feb 2013 15:40:32 -0800 Junio C Hamano <gitster@pobox.com> wrote:
>
> JCH> "Sorry we couldn't" sounded like an error messag to me. If this is
> JCH> a normal exit, then please make sure it is a normal exit.
>
> OK; done in PATCHv4: removed all "Sorry" because they are not abnormal
> exits. I'll hold PATCHv4 until the below are known.
>
> JCH> The review cycle is not like reviewers give you instructions and
> JCH> designs and you blindly implement them. It is a creative process
> JCH> where you show the design and a clear implementation of that design.
>
> OK. I would like you to make the decisions I asked for, though:
>
> - do you want to support backslashed newlines?
What for? netrc/authinfo is not a line oriented file format at all,
and
machine k.org
login me
password mysecret
is a single entry; you do not need backslash at the end of any line.
Perhaps you are asking something different?
> - do you want me to remove the statement modifiers?
I do not think we are at that "implementation nitpick" level yet.
> - should all die() calls just print to STDERR and exit(0)?
Where "when unhandled, the helper should silently exit with 0" is
expected by the invoker, we shouldn't say anything to error stream,
and exit with zero. Please leave a comment to make it easy to
understand to the readers that is what is going on there.
If on the other hand it diagnosed an error (not a bug in the
implementation but a misconfiguration on the user's side), I _think_
it should loudly die() so that the user can notice and take
corrective action.
> - do you want to support multiple netrc files, as you and Peff suggested?
I didn't even suggest such thing IIRC---I expected it to iterate
from the most desirable (.authinfo.gpg) to the least (.netrc) and
stop at the first found one. There may be use cases people use more
than one and expect an entry to be found in any file, but I suspect
that might be more confusing than it is worth. But I do not care
very deeply myself either way.
^ permalink raw reply
* Re: [PATCH] Verify Content-Type from smart HTTP servers
From: Junio C Hamano @ 2013-02-05 0:21 UTC (permalink / raw)
To: Shawn Pearce; +Cc: Jeff King, git
In-Reply-To: <CAJo=hJtZ64ER4X+axtFZJ5ArnEg3h_nCVEBdd8KmE0nUpskzBA@mail.gmail.com>
Shawn Pearce <spearce@spearce.org> writes:
> Looks fine to me too, but I think the test won't work now. :-)
Heh, that's amusing ;-)
t/t5551-http-fetch.sh | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh
index cb95b95..47eb769 100755
--- a/t/t5551-http-fetch.sh
+++ b/t/t5551-http-fetch.sh
@@ -158,9 +158,8 @@ test_expect_success 'GIT_SMART_HTTP can disable smart http' '
'
test_expect_success 'invalid Content-Type rejected' '
- echo "fatal: invalid content-type text/html" >expect
test_must_fail git clone $HTTPD_URL/broken_smart/repo.git 2>actual
- test_cmp expect actual
+ grep "not valid:" actual
'
test -n "$GIT_TEST_LONG" && test_set_prereq EXPENSIVE
^ permalink raw reply related
* Assorted contrib/subtree Patches
From: David A. Greene @ 2013-02-05 4:06 UTC (permalink / raw)
To: James Nylen, git, Techlive Zheng, Wayne Walter,
Avery Pennarun ", Jakub Suder, John Yani
All of the patches I have received from others as well as a few of my
own follow. Probably the most controversial is a patch to remove
--annotate. After some discussion on the list it became clear that we
really want a more general commit rewrite feature. Removing
--annotate means we don't have to also support --unannotate and carry
both forward as backward-compatibility baggage.
Before --annotate was added, git-subtree would force an annotation of
"*" on every split commit message. It now does no such thing so
there's no need to unannotate anything.
Please review and integrate. Thanks!
-David
^ permalink raw reply
* [PATCH 01/13] contrib/subtree: Remove Test Number Comments
From: David A. Greene @ 2013-02-05 4:06 UTC (permalink / raw)
To: James Nylen, git, Techlive Zheng, Wayne Walter,
Avery Pennarun ", Jakub Suder, John Yani
Cc: David A. Greene
In-Reply-To: <1360037173-23291-1-git-send-email-greened@obbligato.org>
From: "David A. Greene" <greened@obbligato.org>
Delete the comments indicating test numbers as it causes maintenance
headaches. t*.sh -i will help us find any broken tests.
Signed-off-by: David A. Greene <greened@obbligato.org>
---
contrib/subtree/t/t7900-subtree.sh | 55 ------------------------------------
1 file changed, 55 deletions(-)
diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh
index bc2eeb0..6cf9fb9 100755
--- a/contrib/subtree/t/t7900-subtree.sh
+++ b/contrib/subtree/t/t7900-subtree.sh
@@ -60,7 +60,6 @@ last_commit_message()
git log --pretty=format:%s -1
}
-# 1
test_expect_success 'init subproj' '
test_create_repo subproj
'
@@ -68,7 +67,6 @@ test_expect_success 'init subproj' '
# To the subproject!
cd subproj
-# 2
test_expect_success 'add sub1' '
create sub1 &&
git commit -m "sub1" &&
@@ -76,14 +74,12 @@ test_expect_success 'add sub1' '
git branch -m master subproj
'
-# 3
test_expect_success 'add sub2' '
create sub2 &&
git commit -m "sub2" &&
git branch sub2
'
-# 4
test_expect_success 'add sub3' '
create sub3 &&
git commit -m "sub3" &&
@@ -93,7 +89,6 @@ test_expect_success 'add sub3' '
# Back to mainline
cd ..
-# 5
test_expect_success 'add main4' '
create main4 &&
git commit -m "main4" &&
@@ -101,101 +96,85 @@ test_expect_success 'add main4' '
git branch subdir
'
-# 6
test_expect_success 'fetch subproj history' '
git fetch ./subproj sub1 &&
git branch sub1 FETCH_HEAD
'
-# 7
test_expect_success 'no subtree exists in main tree' '
test_must_fail git subtree merge --prefix=subdir sub1
'
-# 8
test_expect_success 'no pull from non-existant subtree' '
test_must_fail git subtree pull --prefix=subdir ./subproj sub1
'
-# 9
test_expect_success 'check if --message works for add' '
git subtree add --prefix=subdir --message="Added subproject" sub1 &&
check_equal ''"$(last_commit_message)"'' "Added subproject" &&
undo
'
-# 10
test_expect_success 'check if --message works as -m and --prefix as -P' '
git subtree add -P subdir -m "Added subproject using git subtree" sub1 &&
check_equal ''"$(last_commit_message)"'' "Added subproject using git subtree" &&
undo
'
-# 11
test_expect_success 'check if --message works with squash too' '
git subtree add -P subdir -m "Added subproject with squash" --squash sub1 &&
check_equal ''"$(last_commit_message)"'' "Added subproject with squash" &&
undo
'
-# 12
test_expect_success 'add subproj to mainline' '
git subtree add --prefix=subdir/ FETCH_HEAD &&
check_equal ''"$(last_commit_message)"'' "Add '"'subdir/'"' from commit '"'"'''"$(git rev-parse sub1)"'''"'"'"
'
-# 13
# this shouldn't actually do anything, since FETCH_HEAD is already a parent
test_expect_success 'merge fetched subproj' '
git merge -m "merge -s -ours" -s ours FETCH_HEAD
'
-# 14
test_expect_success 'add main-sub5' '
create subdir/main-sub5 &&
git commit -m "main-sub5"
'
-# 15
test_expect_success 'add main6' '
create main6 &&
git commit -m "main6 boring"
'
-# 16
test_expect_success 'add main-sub7' '
create subdir/main-sub7 &&
git commit -m "main-sub7"
'
-# 17
test_expect_success 'fetch new subproj history' '
git fetch ./subproj sub2 &&
git branch sub2 FETCH_HEAD
'
-# 18
test_expect_success 'check if --message works for merge' '
git subtree merge --prefix=subdir -m "Merged changes from subproject" sub2 &&
check_equal ''"$(last_commit_message)"'' "Merged changes from subproject" &&
undo
'
-# 19
test_expect_success 'check if --message for merge works with squash too' '
git subtree merge --prefix subdir -m "Merged changes from subproject using squash" --squash sub2 &&
check_equal ''"$(last_commit_message)"'' "Merged changes from subproject using squash" &&
undo
'
-# 20
test_expect_success 'merge new subproj history into subdir' '
git subtree merge --prefix=subdir FETCH_HEAD &&
git branch pre-split &&
check_equal ''"$(last_commit_message)"'' "Merge commit '"'"'"$(git rev-parse sub2)"'"'"' into mainline"
'
-# 21
test_expect_success 'Check that prefix argument is required for split' '
echo "You must provide the --prefix option." > expected &&
test_must_fail git subtree split > actual 2>&1 &&
@@ -207,7 +186,6 @@ test_expect_success 'Check that prefix argument is required for split' '
rm -f expected actual
'
-# 22
test_expect_success 'Check that the <prefix> exists for a split' '
echo "'"'"'non-existent-directory'"'"'" does not exist\; use "'"'"'git subtree add'"'"'" > expected &&
test_must_fail git subtree split --prefix=non-existent-directory > actual 2>&1 &&
@@ -219,7 +197,6 @@ test_expect_success 'Check that the <prefix> exists for a split' '
# rm -f expected actual
'
-# 23
test_expect_success 'check if --message works for split+rejoin' '
spl1=''"$(git subtree split --annotate='"'*'"' --prefix subdir --onto FETCH_HEAD --message "Split & rejoin" --rejoin)"'' &&
git branch spl1 "$spl1" &&
@@ -227,7 +204,6 @@ test_expect_success 'check if --message works for split+rejoin' '
undo
'
-# 24
test_expect_success 'check split with --branch' '
spl1=$(git subtree split --annotate='"'*'"' --prefix subdir --onto FETCH_HEAD --message "Split & rejoin" --rejoin) &&
undo &&
@@ -235,7 +211,6 @@ test_expect_success 'check split with --branch' '
check_equal ''"$(git rev-parse splitbr1)"'' "$spl1"
'
-# 25
test_expect_success 'check split with --branch for an existing branch' '
spl1=''"$(git subtree split --annotate='"'*'"' --prefix subdir --onto FETCH_HEAD --message "Split & rejoin" --rejoin)"'' &&
undo &&
@@ -244,13 +219,10 @@ test_expect_success 'check split with --branch for an existing branch' '
check_equal ''"$(git rev-parse splitbr2)"'' "$spl1"
'
-# 26
test_expect_success 'check split with --branch for an incompatible branch' '
test_must_fail git subtree split --prefix subdir --onto FETCH_HEAD --branch subdir
'
-
-# 27
test_expect_success 'check split+rejoin' '
spl1=''"$(git subtree split --annotate='"'*'"' --prefix subdir --onto FETCH_HEAD --message "Split & rejoin" --rejoin)"'' &&
undo &&
@@ -258,7 +230,6 @@ test_expect_success 'check split+rejoin' '
check_equal ''"$(last_commit_message)"'' "Split '"'"'subdir/'"'"' into commit '"'"'"$spl1"'"'"'"
'
-# 28
test_expect_success 'add main-sub8' '
create subdir/main-sub8 &&
git commit -m "main-sub8"
@@ -267,14 +238,12 @@ test_expect_success 'add main-sub8' '
# To the subproject!
cd ./subproj
-# 29
test_expect_success 'merge split into subproj' '
git fetch .. spl1 &&
git branch spl1 FETCH_HEAD &&
git merge FETCH_HEAD
'
-# 30
test_expect_success 'add sub9' '
create sub9 &&
git commit -m "sub9"
@@ -283,19 +252,16 @@ test_expect_success 'add sub9' '
# Back to mainline
cd ..
-# 31
test_expect_success 'split for sub8' '
split2=''"$(git subtree split --annotate='"'*'"' --prefix subdir/ --rejoin)"''
git branch split2 "$split2"
'
-# 32
test_expect_success 'add main-sub10' '
create subdir/main-sub10 &&
git commit -m "main-sub10"
'
-# 33
test_expect_success 'split for sub10' '
spl3=''"$(git subtree split --annotate='"'*'"' --prefix subdir --rejoin)"'' &&
git branch spl3 "$spl3"
@@ -304,7 +270,6 @@ test_expect_success 'split for sub10' '
# To the subproject!
cd ./subproj
-# 34
test_expect_success 'merge split into subproj' '
git fetch .. spl3 &&
git branch spl3 FETCH_HEAD &&
@@ -318,13 +283,11 @@ chkms_sub=$(echo $chkms | multiline | sed 's,^,subdir/,' | fixnl)
chks="sub1 sub2 sub3 sub9"
chks_sub=$(echo $chks | multiline | sed 's,^,subdir/,' | fixnl)
-# 35
test_expect_success 'make sure exactly the right set of files ends up in the subproj' '
subfiles=''"$(git ls-files | fixnl)"'' &&
check_equal "$subfiles" "$chkms $chks"
'
-# 36
test_expect_success 'make sure the subproj history *only* contains commits that affect the subdir' '
allchanges=''"$(git log --name-only --pretty=format:'"''"' | sort | fixnl)"'' &&
check_equal "$allchanges" "$chkms $chks"
@@ -333,20 +296,17 @@ test_expect_success 'make sure the subproj history *only* contains commits that
# Back to mainline
cd ..
-# 37
test_expect_success 'pull from subproj' '
git fetch ./subproj subproj-merge-spl3 &&
git branch subproj-merge-spl3 FETCH_HEAD &&
git subtree pull --prefix=subdir ./subproj subproj-merge-spl3
'
-# 38
test_expect_success 'make sure exactly the right set of files ends up in the mainline' '
mainfiles=''"$(git ls-files | fixnl)"'' &&
check_equal "$mainfiles" "$chkm $chkms_sub $chks_sub"
'
-# 39
test_expect_success 'make sure each filename changed exactly once in the entire history' '
# main-sub?? and /subdir/main-sub?? both change, because those are the
# changes that were split into their own history. And subdir/sub?? never
@@ -355,12 +315,10 @@ test_expect_success 'make sure each filename changed exactly once in the entire
check_equal "$allchanges" ''"$(echo $chkms $chkm $chks $chkms_sub | multiline | sort | fixnl)"''
'
-# 40
test_expect_success 'make sure the --rejoin commits never make it into subproj' '
check_equal ''"$(git log --pretty=format:'"'%s'"' HEAD^2 | grep -i split)"'' ""
'
-# 41
test_expect_success 'make sure no "git subtree" tagged commits make it into subproj' '
# They are meaningless to subproj since one side of the merge refers to the mainline
check_equal ''"$(git log --pretty=format:'"'%s%n%b'"' HEAD^2 | grep "git-subtree.*:")"'' ""
@@ -370,14 +328,12 @@ test_expect_success 'make sure no "git subtree" tagged commits make it into subp
mkdir test2
cd test2
-# 42
test_expect_success 'init main' '
test_create_repo main
'
cd main
-# 43
test_expect_success 'add main1' '
create main1 &&
git commit -m "main1"
@@ -385,14 +341,12 @@ test_expect_success 'add main1' '
cd ..
-# 44
test_expect_success 'init sub' '
test_create_repo sub
'
cd sub
-# 45
test_expect_success 'add sub2' '
create sub2 &&
git commit -m "sub2"
@@ -402,7 +356,6 @@ cd ../main
# check if split can find proper base without --onto
-# 46
test_expect_success 'add sub as subdir in main' '
git fetch ../sub master &&
git branch sub2 FETCH_HEAD &&
@@ -411,7 +364,6 @@ test_expect_success 'add sub as subdir in main' '
cd ../sub
-# 47
test_expect_success 'add sub3' '
create sub3 &&
git commit -m "sub3"
@@ -419,20 +371,17 @@ test_expect_success 'add sub3' '
cd ../main
-# 48
test_expect_success 'merge from sub' '
git fetch ../sub master &&
git branch sub3 FETCH_HEAD &&
git subtree merge --prefix subdir sub3
'
-# 49
test_expect_success 'add main-sub4' '
create subdir/main-sub4 &&
git commit -m "main-sub4"
'
-# 50
test_expect_success 'split for main-sub4 without --onto' '
git subtree split --prefix subdir --branch mainsub4
'
@@ -442,19 +391,16 @@ test_expect_success 'split for main-sub4 without --onto' '
# have been sub3, but it was not, because its cache was not set to
# itself)
-# 51
test_expect_success 'check that the commit parent is sub3' '
check_equal ''"$(git log --pretty=format:%P -1 mainsub4)"'' ''"$(git rev-parse sub3)"''
'
-# 52
test_expect_success 'add main-sub5' '
mkdir subdir2 &&
create subdir2/main-sub5 &&
git commit -m "main-sub5"
'
-# 53
test_expect_success 'split for main-sub5 without --onto' '
# also test that we still can split out an entirely new subtree
# if the parent of the first commit in the tree is not empty,
@@ -487,7 +433,6 @@ joincommits()
echo "$commit $all"
}
-# 54
test_expect_success 'verify one file change per commit' '
x= &&
list=''"$(git log --pretty=format:'"'commit: %H'"' | joincommits)"'' &&
--
1.7.10.4
^ permalink raw reply related
* [PATCH 02/13] contrib/subtree: Use %B for Split Subject/Body
From: David A. Greene @ 2013-02-05 4:06 UTC (permalink / raw)
To: James Nylen, git, Techlive Zheng, Wayne Walter,
Avery Pennarun ", Jakub Suder, John Yani
Cc: David A. Greene
In-Reply-To: <1360037173-23291-1-git-send-email-greened@obbligato.org>
From: Techlive Zheng <techlivezheng@gmail.com>
Use %B to format the commit message and body to avoid an extra newline
if a commit only has a subject line.
Signed-off-by: Techlive Zheng <techlivezheng@gmail.com>
Signed-off-by: David A. Greene <greened@obbligato.org>
---
contrib/subtree/git-subtree.sh | 2 +-
contrib/subtree/t/t7900-subtree.sh | 15 +++++++++++++++
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 920c664..5598210 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -296,7 +296,7 @@ copy_commit()
# We're going to set some environment vars here, so
# do it in a subshell to get rid of them safely later
debug copy_commit "{$1}" "{$2}" "{$3}"
- git log -1 --pretty=format:'%an%n%ae%n%ad%n%cn%n%ce%n%cd%n%s%n%n%b' "$1" |
+ git log -1 --pretty=format:'%an%n%ae%n%ad%n%cn%n%ce%n%cd%n%B' "$1" |
(
read GIT_AUTHOR_NAME
read GIT_AUTHOR_EMAIL
diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh
index 6cf9fb9..3f17f55 100755
--- a/contrib/subtree/t/t7900-subtree.sh
+++ b/contrib/subtree/t/t7900-subtree.sh
@@ -74,6 +74,10 @@ test_expect_success 'add sub1' '
git branch -m master subproj
'
+# Save this hash for testing later.
+
+subdir_hash=`git rev-parse HEAD`
+
test_expect_success 'add sub2' '
create sub2 &&
git commit -m "sub2" &&
@@ -211,6 +215,17 @@ test_expect_success 'check split with --branch' '
check_equal ''"$(git rev-parse splitbr1)"'' "$spl1"
'
+test_expect_success 'check hash of split' '
+ spl1=$(git subtree split --prefix subdir) &&
+ undo &&
+ git subtree split --prefix subdir --branch splitbr1test &&
+ check_equal ''"$(git rev-parse splitbr1test)"'' "$spl1"
+ git checkout splitbr1test &&
+ new_hash=$(git rev-parse HEAD~2) &&
+ git checkout mainline &&
+ check_equal ''"$new_hash"'' "$subdir_hash"
+'
+
test_expect_success 'check split with --branch for an existing branch' '
spl1=''"$(git subtree split --annotate='"'*'"' --prefix subdir --onto FETCH_HEAD --message "Split & rejoin" --rejoin)"'' &&
undo &&
--
1.7.10.4
^ permalink raw reply related
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