* [PATCH 2/4] Git.pm: Allow pipes to be closed prior to calling command_close_bidi_pipe
From: Michal Nazarewicz @ 2013-02-06 20:47 UTC (permalink / raw)
To: Junio C Hamano, Ted Zlatanov, Jeff King, Matthieu Moy; +Cc: git
In-Reply-To: <cover.1360183427.git.mina86@mina86.com>
From: Michal Nazarewicz <mina86@mina86.com>
The command_close_bidi_pipe() function will insist on closing both
input and output pipes returned by command_bidi_pipe(). With this
change it is possible to close one of the pipes in advance and
pass undef as an argument.
This allows for something like:
my ($pid, $in, $out, $ctx) = command_bidi_pipe(...);
print $out "write data";
close $out;
# ... do stuff with $in
command_close_bidi_pipe($pid, $in, undef, $ctx);
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
perl/Git.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/perl/Git.pm b/perl/Git.pm
index bbb753a..6a2d52d 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -432,7 +432,7 @@ sub command_close_bidi_pipe {
local $?;
my ($self, $pid, $in, $out, $ctx) = _maybe_self(@_);
foreach my $fh ($in, $out) {
- unless (close $fh) {
+ if (defined $fh && !close $fh) {
if ($!) {
carp "error closing pipe: $!";
} elsif ($? >> 8) {
--
1.8.1.2.549.g4fa355e
^ permalink raw reply related
* [PATCH 3/4] Git.pm: Add interface for git credential command.
From: Michal Nazarewicz @ 2013-02-06 20:47 UTC (permalink / raw)
To: Junio C Hamano, Ted Zlatanov, Jeff King, Matthieu Moy; +Cc: git
In-Reply-To: <cover.1360183427.git.mina86@mina86.com>
From: Michal Nazarewicz <mina86@mina86.com>
Add a credential() function which is an interface to the
git credential command.
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
perl/Git.pm | 112 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 111 insertions(+), 1 deletion(-)
diff --git a/perl/Git.pm b/perl/Git.pm
index 6a2d52d..5a18921 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -59,7 +59,8 @@ require Exporter;
command_bidi_pipe command_close_bidi_pipe
version exec_path html_path hash_object git_cmd_try
remote_refs prompt
- temp_acquire temp_release temp_reset temp_path);
+ temp_acquire temp_release temp_reset temp_path
+ credential);
=head1 DESCRIPTION
@@ -1000,6 +1001,115 @@ sub _close_cat_blob {
}
+sub _credential_read {
+ my %credential;
+ my ($reader, $op) = (@_);
+ while (<$reader>) {
+ chomp;
+ my ($key, $value) = /([^=]*)=(.*)/;
+ if (not defined $key) {
+ throw Error::Simple("unable to parse git credential $op response:\n$_\n");
+ }
+ $credential{$key} = $value;
+ }
+ return %credential;
+}
+
+sub _credential_write {
+ my ($credential, $writer) = @_;
+
+ for my $key (sort {
+ # url overwrites other fields, so it must come first
+ return -1 if $a eq 'url';
+ return 1 if $b eq 'url';
+ return $a cmp $b;
+ } keys %$credential) {
+ if (defined $credential->{$key} && length $credential->{$key}) {
+ print $writer $key, '=', $credential->{$key}, "\n";
+ }
+ }
+ print $writer "\n";
+}
+
+sub _credential_run {
+ my ($self, $credential, $op) = _maybe_self(@_);
+
+ my ($pid, $reader, $writer, $ctx) = command_bidi_pipe('credential', $op);
+
+ _credential_write $credential, $writer;
+ close $writer;
+
+ if ($op eq "fill") {
+ %$credential = _credential_read $reader, $op;
+ } elsif (<$reader>) {
+ throw Error::Simple("unexpected output from git credential $op response:\n$_\n");
+ }
+
+ command_close_bidi_pipe($pid, $reader, undef, $ctx);
+}
+
+=item credential( CREDENTIAL_HASH [, OPERATION ] )
+
+=item credential( CREDENTIAL_HASH, CODE )
+
+Executes C<git credential> for a given set of credentials and
+specified operation. In both form C<CREDENTIAL_HASH> needs to be
+a reference to a hash which stores credentials. Under certain
+conditions the hash can change.
+
+In the first form, C<OPERATION> can be C<'fill'> (or omitted),
+C<'approve'> or C<'reject'>, and function will execute corresponding
+C<git credential> sub-command. In case of C<'fill'> the values stored
+in C<CREDENTIAL_HASH> will be changed to the ones returned by the
+C<git credential> command. The usual usage would look something like:
+
+ my %cred = (
+ 'protocol' => 'https',
+ 'host' => 'example.com',
+ 'username' => 'bob'
+ );
+ Git::credential \%cred;
+ if (try_to_authenticate($cred{'username'}, $cred{'password'})) {
+ Git::credential \%cred, 'approve';
+ ... do more stuff ...
+ } else {
+ Git::credential \%cred, 'reject';
+ }
+
+In the second form, C<CODE> needs to be a reference to a subroutine.
+The function will execute C<git credential fill> to fill provided
+credential hash, than call C<CODE> with C<CREDENTIAL> as the sole
+argument, and finally depending on C<CODE>'s return value execute
+C<git credential approve> (if return value yields true) or C<git
+credential reject> (otherwise). The return value is the same as what
+C<CODE> returned. With this form, the usage might look as follows:
+
+ if (Git::credential {
+ 'protocol' => 'https',
+ 'host' => 'example.com',
+ 'username' => 'bob'
+ }, sub {
+ my $cred = shift;
+ return try_to_authenticate($cred->{'username'}, $cred->{'password'});
+ }) {
+ ... do more stuff ...
+ }
+
+=cut
+
+sub credential {
+ my ($self, $credential, $op_or_code) = (_maybe_self(@_), 'fill');
+
+ if ('CODE' eq ref $op_or_code) {
+ _credential_run $credential, 'fill';
+ my $ret = $op_or_code->($credential);
+ _credential_run $credential, $ret ? 'approve' : 'reject';
+ return $ret;
+ } else {
+ _credential_run $credential, $op_or_code;
+ }
+}
+
{ # %TEMP_* Lexical Context
my (%TEMP_FILEMAP, %TEMP_FILES);
--
1.8.1.2.549.g4fa355e
^ permalink raw reply related
* Re: Why is ident_is_sufficient different on Windows?
From: Junio C Hamano @ 2013-02-06 20:47 UTC (permalink / raw)
To: Max Horn; +Cc: git
In-Reply-To: <7vmwvhb2fm.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> I suspect somebody from the Windows camp saw a patch I posted
> without the ifdef, noticed that there is a problem to expect
> IDENT_NAME_GIVEN to be set on Windows for some reason, and resulted
> in a reroll of the function in that shape.
>
> I didn't find anything in the list archive, though. So I am
> stumped.
The only thing I can think of is that on Unix we can guess name from
GECOS, which could be considered sufficiently your name, while on
Windows we probably do not get anything useful there.
^ permalink raw reply
* [PATCH 4/4] git-send-email: Use git credential to obtain password.
From: Michal Nazarewicz @ 2013-02-06 20:47 UTC (permalink / raw)
To: Junio C Hamano, Ted Zlatanov, Jeff King, Matthieu Moy; +Cc: git
In-Reply-To: <cover.1360183427.git.mina86@mina86.com>
From: Michal Nazarewicz <mina86@mina86.com>
If smtp_user is provided but smtp_pass is not, instead of prompting
for password, make git-send-email use git credential command
instead.
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
Documentation/git-send-email.txt | 4 +--
git-send-email.perl | 59 +++++++++++++++++++++++-----------------
2 files changed, 36 insertions(+), 27 deletions(-)
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 44a1f7c..0cffef8 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -164,8 +164,8 @@ Sending
Furthermore, passwords need not be specified in configuration files
or on the command line. If a username has been specified (with
'--smtp-user' or a 'sendemail.smtpuser'), but no password has been
-specified (with '--smtp-pass' or 'sendemail.smtppass'), then the
-user is prompted for a password while the input is masked for privacy.
+specified (with '--smtp-pass' or 'sendemail.smtppass'), then
+a password is obtained using 'git-credential'.
--smtp-server=<host>::
If set, specifies the outgoing SMTP server to use (e.g.
diff --git a/git-send-email.perl b/git-send-email.perl
index be809e5..76bbfc3 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1045,6 +1045,39 @@ sub maildomain {
return maildomain_net() || maildomain_mta() || 'localhost.localdomain';
}
+# Returns 1 if authentication succeeded or was not necessary
+# (smtp_user was not specified), and 0 otherwise.
+
+sub smtp_auth_maybe {
+ if (!defined $smtp_authuser || $auth) {
+ return 1;
+ }
+
+ # Workaround AUTH PLAIN/LOGIN interaction defect
+ # with Authen::SASL::Cyrus
+ eval {
+ require Authen::SASL;
+ Authen::SASL->import(qw(Perl));
+ };
+
+ # TODO: Authentication may fail not because credentials were
+ # invalid but due to other reasons, in which we should not
+ # reject credentials.
+ $auth = Git::credential({
+ 'protocol' => 'smtp',
+ 'host' => join(':', $smtp_server, $smtp_server_port),
+ 'username' => $smtp_authuser,
+ # if there's no password, "git credential fill" will
+ # give us one, otherwise it'll just pass this one.
+ 'password' => $smtp_authpass
+ }, sub {
+ my $cred = shift;
+ return !!$smtp->auth($cred->{'username'}, $cred->{'password'});
+ });
+
+ return $auth;
+}
+
# Returns 1 if the message was sent, and 0 otherwise.
# In actuality, the whole program dies when there
# is an error sending a message.
@@ -1185,31 +1218,7 @@ X-Mailer: git-send-email $gitversion
defined $smtp_server_port ? " port=$smtp_server_port" : "";
}
- if (defined $smtp_authuser) {
- # Workaround AUTH PLAIN/LOGIN interaction defect
- # with Authen::SASL::Cyrus
- eval {
- require Authen::SASL;
- Authen::SASL->import(qw(Perl));
- };
-
- if (!defined $smtp_authpass) {
-
- system "stty -echo";
-
- do {
- print "Password: ";
- $_ = <STDIN>;
- print "\n";
- } while (!defined $_);
-
- chomp($smtp_authpass = $_);
-
- system "stty echo";
- }
-
- $auth ||= $smtp->auth( $smtp_authuser, $smtp_authpass ) or die $smtp->message;
- }
+ smtp_auth_maybe or die $smtp->message;
$smtp->mail( $raw_from ) or die $smtp->message;
$smtp->to( @recipients ) or die $smtp->message;
--
1.8.1.2.549.g4fa355e
^ permalink raw reply related
* Re: Why is ident_is_sufficient different on Windows?
From: Junio C Hamano @ 2013-02-06 20:52 UTC (permalink / raw)
To: Max Horn; +Cc: git
In-Reply-To: <7vip65b25c.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> I suspect somebody from the Windows camp saw a patch I posted
>> without the ifdef, noticed that there is a problem to expect
>> IDENT_NAME_GIVEN to be set on Windows for some reason, and resulted
>> in a reroll of the function in that shape.
>>
>> I didn't find anything in the list archive, though. So I am
>> stumped.
>
> The only thing I can think of is that on Unix we can guess name from
> GECOS, which could be considered sufficiently your name, while on
> Windows we probably do not get anything useful there.
http://thread.gmane.org/gmane.comp.version-control.git/137312/focus=137345
These days, we encourage setting user.name explicitly even on a
system on which it is likely that we will see a good GECOS value, so
removing the ifdef and always check with ALL may not hurt anybody.
I dunno.
^ permalink raw reply
* Re: [PATCH 0/4] Make git-send-email git-credential
From: Junio C Hamano @ 2013-02-06 20:54 UTC (permalink / raw)
To: Michal Nazarewicz; +Cc: Ted Zlatanov, Jeff King, Matthieu Moy, git
In-Reply-To: <xa1thalpp47z.fsf@mina86.com>
Michal Nazarewicz <mina86@mina86.com> writes:
> On second thought, give me a moment, ;) I've just discovered a bug
> preventing git-send-email from mailing a patchset.
I somehow found this highly amusing.
I wish all the bugs are like that: if your series is buggy, some
parts of the system prevents you from sending it to the list.
;-)
^ permalink raw reply
* [PATCH v4] submodule: add 'deinit' command
From: Jens Lehmann @ 2013-02-06 21:11 UTC (permalink / raw)
To: Git Mailing List
Cc: Junio C Hamano, Heiko Voigt, Michael J Gruber, Phil Hord,
Marc Branchaud, W. Trevor King
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>
---
Changes since v3:
- Add deinit to the --force documentation of "git submodule"
- Never remove submodules containing a .git dir, even when forced
- Diagnostic output when "rm -rf" or "mkdir" fails
- More test cases
Documentation/git-rm.txt | 4 ++
Documentation/git-submodule.txt | 18 +++++++-
git-submodule.sh | 78 ++++++++++++++++++++++++++++++-
t/t7400-submodule-basic.sh | 100 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 198 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-rm.txt b/Documentation/git-rm.txt
index 92bac27..1d876c2 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 the removal,
+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 a0c9df8..bc06159 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 004c034..f1b552f 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,81 @@ 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
+ 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
+ say "$(eval_gettext "No url found for submodule path '\$sm_path' in .git/config")"
+ continue
+ fi
+
+ # Remove the submodule work tree (unless the user already did it)
+ if test -d "$sm_path"
+ then
+ # Protect submodules containing a .git directory
+ if test -d "$sm_path/.git"
+ then
+ echo >&2 "$(eval_gettext "Submodule work tree $sm_path contains a .git directory")"
+ die "$(eval_gettext "(use 'rm -rf' if you really want to remove it including all of its history)")"
+ fi
+
+ if test -z "$force"
+ 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" || say "$(eval_gettext "Could not remove submodule work tree '\$sm_path'")"
+ fi
+
+ mkdir "$sm_path" || say "$(eval_gettext "Could not create empty submodule directory '\$sm_path'")"
+
+ # 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 +1233,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)
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 2683cba..f54a40d 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -757,4 +757,104 @@ test_expect_success 'submodule add with an existing name fails unless forced' '
)
'
+test_expect_success 'set up a second submodule' '
+ git submodule add ./init2 example2 &&
+ git commit -m "submodle example2 added"
+'
+
+test_expect_success 'submodule deinit should remove the whole submodule section from .git/config' '
+ git config submodule.example.foo bar &&
+ git config submodule.example2.frotz nitfol &&
+ git submodule deinit init &&
+ test -z "$(git config submodule.example.url)" &&
+ test -z "$(git config submodule.example.foo)" &&
+ test -n "$(git config submodule.example2.url)" &&
+ test -n "$(git config submodule.example2.frotz)" &&
+ test -f example2/.git &&
+ rmdir init
+'
+
+test_expect_success 'submodule deinit . deinits all initialized submodules' '
+ git submodule update --init &&
+ git config submodule.example.foo bar &&
+ git config submodule.example2.frotz nitfol &&
+ test_must_fail git submodule deinit &&
+ git submodule deinit . &&
+ test -z "$(git config submodule.example.url)" &&
+ test -z "$(git config submodule.example.foo)" &&
+ test -z "$(git config submodule.example2.url)" &&
+ test -z "$(git config submodule.example2.frotz)" &&
+ rmdir init example2
+'
+
+test_expect_success 'submodule deinit deinits a submodule when its work tree is missing or empty' '
+ git submodule update --init &&
+ rm -rf init example2/* example2/.git &&
+ git submodule deinit init example2 &&
+ test -z "$(git config submodule.example.url)" &&
+ test -z "$(git config submodule.example2.url)" &&
+ rmdir init
+'
+
+test_expect_success 'submodule deinit fails when the submodule contains modifications unless forced' '
+ git submodule update --init &&
+ echo X >>init/s &&
+ test_must_fail git submodule deinit init &&
+ test -n "$(git config submodule.example.url)" &&
+ test -f example2/.git &&
+ git submodule deinit -f init &&
+ test -z "$(git config submodule.example.url)" &&
+ rmdir init
+'
+
+test_expect_success 'submodule deinit fails when the submodule contains untracked files unless forced' '
+ git submodule update --init &&
+ echo X >>init/untracked &&
+ test_must_fail git submodule deinit init &&
+ test -n "$(git config submodule.example.url)" &&
+ test -f example2/.git &&
+ git submodule deinit -f init &&
+ test -z "$(git config submodule.example.url)" &&
+ rmdir init
+'
+
+test_expect_success 'submodule deinit fails when the submodule HEAD does not match unless forced' '
+ git submodule update --init &&
+ (
+ cd init &&
+ git checkout HEAD^
+ ) &&
+ test_must_fail git submodule deinit init &&
+ test -n "$(git config submodule.example.url)" &&
+ test -f example2/.git &&
+ git submodule deinit -f init &&
+ test -z "$(git config submodule.example.url)" &&
+ rmdir init
+'
+
+test_expect_success 'submodule deinit complains but does not fail when used on an uninitialized submodule' '
+ git submodule update --init &&
+ git submodule deinit init >actual &&
+ test_i18ngrep "Submodule .example. (.*) unregistered for path .init" actual
+ git submodule deinit init >actual &&
+ test_i18ngrep "No url found for submodule path .init. in .git/config" actual &&
+ git submodule deinit . >actual &&
+ test_i18ngrep "Submodule .example2. (.*) unregistered for path .example2" actual
+ rmdir init example2
+'
+
+test_expect_success 'submodule deinit fails when submodule has a .git directory even when forced' '
+ git submodule update --init &&
+ (
+ cd init &&
+ rm .git &&
+ cp -R ../.git/modules/example .git &&
+ GIT_WORK_TREE=. git config --unset core.worktree
+ ) &&
+ test_must_fail git submodule deinit init &&
+ test_must_fail git submodule deinit -f init &&
+ test -d init/.git &&
+ test -n "$(git config submodule.example.url)"
+'
+
test_done
--
1.8.1.2.546.gea155c0
^ permalink raw reply related
* Re: will git provide `submodule remove` option ?
From: Jens Lehmann @ 2013-02-06 21:27 UTC (permalink / raw)
To: Lingcha X; +Cc: git@vger.kernel.org
In-Reply-To: <BAY176-W2328CE9FE72C92BCB27421B4000@phx.gbl>
Am 05.02.2013 11:32, schrieb Lingcha X:
> As we all know, git provides `submodule add , init, update, sync, sumary, foreach, status`, but where is `submodule remove`?
>
> will
> I not delete one of them sometime in the future? Although most people
> will not use submodule or one who uses it can remove submodule by hand, providing complete options may be a good idea.
Is assume either "git rm <submodule>" (available since 1.8.1) or the upcoming
"git submodule deinit" (currently in pu) will do what you want?
^ permalink raw reply
* Re: [PATCH v3 0/8] Hiding refs
From: Michael Haggerty @ 2013-02-06 21:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Duy Nguyen, Jonathan Nieder, git, Jeff King, Shawn Pearce
In-Reply-To: <7v4nhpckwd.fsf@alter.siamese.dyndns.org>
On 02/06/2013 08:17 PM, Junio C Hamano wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Tue, Feb 5, 2013 at 5:29 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>> Hiderefs creates a "dark" corner of a remote git repo that can hold
>>> arbitrary content that is impossible for anybody to discover but
>>> nevertheless possible for anybody to download (if they know the name of
>>> a hidden reference). In earlier versions of the patch series I believe
>>> that it was possible to push to a hidden reference hierarchy, which made
>>> it possible to upload dark content. The new version appears (from the
>>> code) to prohibit adding references in a hidden hierarchy, which would
>>> close the main loophole that I was worried about. But the documentation
>>> and the unit tests only explicitly say that updates and deletes are
>>> prohibited; nothing is said about adding references (unless "update" is
>>> understood to include "add"). I think the true behavior should be
>>> clarified and tested.
>>>
>>> I was worried that somehow this "dark" content could be used for
>>> malicious purposes; for example, pushing compromised code then
>>> convincing somebody to download it by SHA1 with the implicit argument
>>> "it's safe since it comes directly from the project's official
>>> repository". If it is indeed impossible to populate the dark namespace
>>> remotely then I can't think of a way to exploit it.
>>
>> Or you can think hiderefs is the first step to addressing the
>> initial ref advertisment problem. The series says hidden refs are
>> to be fetched out of band, but that's not the only way.
>
> Let me help unconfuse this thread.
>
> I think the series as 8-patch series was poorly presented, and
> separating it into two will help understanding what they are about.
>
> The first three:
>
> upload-pack: share more code
> upload-pack: simplify request validation
> upload/receive-pack: allow hiding ref hierarchies
>
> is _the_ topic of the series. As far as I am concerned (I am not
> speaking for Gerrit users, but am speaking as the Git maintainer),
> the topic is solely about uncluttering. There may be refs that the
> server end may need to keep for its operation, but that remote users
> have _no_ business knowing about. Allowing the server to keep these
> refs in the repository, while not showing these refs over the wire,
> is the problem the series solves.
>
> In other words, it is not about "these are *usually* not wanted by
> clients, so do not show them by default". It is about "these are
> not to be shown, ever".
>
> OK?
Yes, the first three patches sound much more reasonable if this is the
goal. Do you know of users who want the feature defined by the first
three patches, or is it only a stepping stone towards an actually useful
feature? (I ask because I have trouble imagining a real-world scenario
where these alone would be useful.)
> Now, there may be some refs that are not *usually* wanted by clients
> but there may be cases where clients want to
>
> (1) learn about them via the same protocol; and/or
> (2) fetch them over the protocol.
>
> If you want to solve both of these two issues generally, the
> solution has to involve a separate protocol from the today's
> protocol. It would go like this:
[... omitted clear explanation of how delayed advertisement could be
implemented via a new protocol ...]
> But in the meantime, if there is a niche use case where a solution
> to only the second problem is sufficient (and Gerrit and GitHub pull
> requests could both be such use cases), the remainder of the series
> can help, without waiting the solution to solve "usually not wanted
> but may need to be learned" problem. That is the latter 4 patches
> (the very last one is a demonstration to illustrate why allowing a
> push to hidden ref hierarchy would not and should not work, and is
> not for application):
Given that some people *do* want to fetch all pull requests, is this a
feature that any hosting service would really turn on? True, the
majority of users would be spared clutter, but at the cost of completely
preventing other users from fetching all pull requests, mirroring the
repository, etc.
In other words, I wonder whether your two incremental steps are useful
at all, in the real world, without yet-to-be-implemented future changes.
If not, then it doesn't make sense to merge them without at least
imagining the final goal and gaining confidence that they are not false
starts.
I think that a more useful interim solution would be to make it easy to
have two URLs accessing a single git repository, with different levels
of reference visibility applied to each. This is something that
providers could turn on without sacrificing any existing functionality.
And it would solve all three problems: clutter, bandwidth, and provenance.
Your first three patches would allow two-tier access to be implemented,
for example by setting GIT_CONFIG or GIT_CONFIG_PARAMETERS or
command-line parameters differently for the processes serving the two
URLs, like:
git upload-pack ...
vs.
GIT_CONFIG=config-with-hidden-refs git upload-pack ...
or
git -c transfer.hiderefs=refs/pull upload-pack ...
But this is a bit awkward because the admin would either have to
maintain two config files, or maintain the hiderefs configuration in the
script starting upload-pack rather than in the configuration file.
Therefore, I suggest a slight change to how hiderefs are configured to
make two-tier URLs easier to configure, such as
# Define one or more views:
[view "uncluttered"]
hiderefs = refs/pull
# This would set the default view for all services:
[transfer]
view = uncluttered
# Peff also wanted the possibility to configure each service
# independently which could be done like this:
[receive]
view = uncluttered
[uploadpack]
view = full
I also tentatively suggest that we add a git-level option "--view" and
an environment variable GIT_VIEW (similar to "--namespace" and
GIT_NAMESPACE) to override the default setting:
GIT_VIEW=uncluttered git upload-pack ...
This way whoever starts the process only needs to choose a particular
view name; the actual definition would reside in the config file.
I think these changes would make it easier to support two-tier URLs and
would also leave the way open to use the "view" concept for other things
in the future.
I've said my piece now and am gratified that there has been more
discussion about your proposal, which was my main goal. Therefore FWIW
I turn my -1 into a -0 and leave it up to the people experiencing more
clutter-induced pain to decide how to proceed.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: [PATCH] git-send-email: add ~/.authinfo parsing
From: Jeff King @ 2013-02-06 21:57 UTC (permalink / raw)
To: Ted Zlatanov
Cc: Matthieu Moy, Junio C Hamano, Michal Nazarewicz, git,
Krzysztof Mazur, Michal Nazarewicz
In-Reply-To: <87sj59mo2y.fsf@lifelogs.com>
On Wed, Feb 06, 2013 at 10:58:13AM -0500, Ted Zlatanov wrote:
> MM> I don't know about the netrc credential helper, but I guess that's
> MM> another layer. The git-remote-mediawiki code is the code to call the
> MM> credential C API, that in turn may (or may not) call a credential
> MM> helper.
>
> Yup. But what you call "read" and "write" are, to the credential
> helper, "write" and "read" but it's the same protocol :) So maybe the
> names should be changed to reflect that, e.g. "query" and "response."
Is that true? As a user of the credential system, git-remote-mediawiki
would want to "write" to git-credential, then "read" the response. As a
helper, git-credential-netrc would want to "read" the query then
"write" the response. The order is different, but the operations should
be the same in both cases.
The big difference is that mediawiki would want an additional function
to open a pipe to "git credential" and operate on that, whereas the
helper will be reading/writing stdio.
-Peff
^ permalink raw reply
* [PATCH] connect.c: Tell *PLink to always use ssh protocol
From: Sven Strickroth @ 2013-02-06 21:58 UTC (permalink / raw)
To: git; +Cc: gitster, Jeff King
Default values for *plink can be set using PuTTY. If a user makes
telnet the default in PuTTY this breaks ssh clones in git.
Since git clones of the type user@host:path use ssh, tell *plink
to use ssh and override PuTTY defaults for the protocol to use.
Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
connect.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/connect.c b/connect.c
index 49e56ba..d337b6f 100644
--- a/connect.c
+++ b/connect.c
@@ -625,6 +625,8 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
if (!ssh) ssh = "ssh";
*arg++ = ssh;
+ if (putty)
+ *arg++ = "-ssh";
if (putty && !strcasestr(ssh, "tortoiseplink"))
*arg++ = "-batch";
if (port) {
--
Best regards,
Sven Strickroth
PGP key id F5A9D4C4 @ any key-server
^ permalink raw reply related
* Re: [RFC/PATCH 1/4] show: obey --textconv for blobs
From: Jeff King @ 2013-02-06 22:06 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Junio C Hamano
In-Reply-To: <883f0163cb732932061a368ea9bc187c13e4ecca.1360162813.git.git@drmicha.warpmail.net>
On Wed, Feb 06, 2013 at 04:08:50PM +0100, Michael J Gruber wrote:
> diff --git a/builtin/log.c b/builtin/log.c
> index 8f0b2e8..f83870d 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -402,10 +402,28 @@ static void show_tagger(char *buf, int len, struct rev_info *rev)
> strbuf_release(&out);
> }
>
> -static int show_blob_object(const unsigned char *sha1, struct rev_info *rev)
> +static int show_blob_object(const unsigned char *sha1, struct rev_info *rev, const char *obj_name)
Should this maybe just take the whole object_array_entry as a cleanup?
> {
> + unsigned char sha1c[20];
> + struct object_context obj_context;
> + char *buf;
> + unsigned long size;
> +
> fflush(stdout);
> - return stream_blob_to_fd(1, sha1, NULL, 0);
> + if (!DIFF_OPT_TST(&rev->diffopt, ALLOW_TEXTCONV))
> + return stream_blob_to_fd(1, sha1, NULL, 0);
> +
> + if (get_sha1_with_context(obj_name, 0, sha1c, &obj_context))
> + die("Not a valid object name %s", obj_name);
It seems a little hacky that we have to look up the sha1 again. What
should happen in the off chance that "hashcmp(sha1, sha1c) != 0" due to
a race with a simultaneous update of a ref?
Would it be better if object_array_entry replaced its "mode" member with
an object_context? The only downside I see is that we might waste a
significant amount of memory (each context has a PATH_MAX buffer in it).
-Peff
^ permalink raw reply
* Re: [PATCH v3 0/8] Hiding refs
From: Michael Haggerty @ 2013-02-06 22:01 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, git, Jeff King, Shawn Pearce
In-Reply-To: <20130206195515.GC21003@google.com>
On 02/06/2013 08:55 PM, Jonathan Nieder wrote:
> Michael Haggerty wrote:
> [...]
>> But now every time I do a "gitk --all" or "git log --decorate", the
>> output is cluttered with all of his references (most of which are just
>> old versions of references from the upstream repository that we both
>> use). I would like to be able to hide his references most of the time
>> but turn them back on when I need them.
>>
>> Scenario 5: Our upstream repository has gazillions of release tags under
>> "refs/tags/releases/...", sometimes including customer-specific
>> releases. In my daily life these are just clutter.
>
> For both of these use cases, putting the refs somewhere other than
> refs/heads, refs/tags, and refs/remotes should be enough to avoid
> clutter.
Thanks, yes, for release tags in particular your suggestion might be
workable. But I also like the idea of being able to turn subsets of
references on and off easily, and have the choice persist until I change it.
> [...]
>> * Some small improvements (e.g. allowing *multiple* views to be
>> defined) would provide much more benefit for about the same effort,
>> and would be a better base for building other features in the future
>> (e.g., local views).
>
> Would advertising GIT_CONFIG_PARAMETERS and giving examples for server
> admins to set it in inetd et al to provide different kinds of access
> to a same repository through different URLs work?
>
>> Thanks for listening.
>> Michael
>>
>> [1] Theoretically one could support multiple views of a single
>> repository by using something like "GIT_CONFIG=view_1_config git
>> upload-pack ..." or "git -c transfer.hiderefs=... git upload-pack ...",
>> but this would be awkward.
>
> Ah, I missed this comment before. What's awkward about that? I
> think it's a clean way to make many aspects of how a repository is
> presented (including hook actions) configurable.
Awkwardness using GIT_CONFIG: the admin would have to maintain two
separate config files with mostly overlapping content.
Awkwardness using GIT_CONFIG_PARAMETERS or "-c transfer.hiderefs=...":
the hiderefs configuration would have to be maintained in some Apache
config or inetd or ... (or multiple places!) rather than in the
repository's config file, where it belongs.
Additional awkwardness using "-c transfer.hiderefs=...": AFAIK there is
no way to turn *off* a configuration variable via a command-line option.
It's all doable, but I find it awkward.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: [RFC/PATCH 1/4] show: obey --textconv for blobs
From: Jeff King @ 2013-02-06 22:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael J Gruber, git
In-Reply-To: <7vmwvhmli7.fsf@alter.siamese.dyndns.org>
On Wed, Feb 06, 2013 at 08:53:52AM -0800, Junio C Hamano wrote:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
>
> > Currently, "diff" and "cat-file" for blobs obey "--textconv" options
> > (with the former defaulting to "--textconv" and the latter to
> > "--no-textconv") whereas "show" does not obey this option, even though
> > it takes diff options.
> >
> > Make "show" on blobs behave like "diff", i.e. obey "--textconv" by
> > default and "--no-textconv" when given.
>
> What does "log -p" do currently, and what should it do? Does/should
> it also use --textconv?
>
> The --textconv is a natural extension of what --ext-diff provides us,
> so I think it should trigger the same way as how --ext-diff triggers.
>
> We apply "--ext-diff" for "diff" by default but not for "log -p" and
> "show"; I suspect this may have been for a good reason but I do not
> recall the discussion that led to the current behaviour offhand.
I think Michael's commit message explains the situation badly.
--textconv is already on for "git show" (and for "git log") by default.
Diffs already use it.
This is more about the fact that when showing a single blob, we do not
bother to remember the context of the sha1 lookup, including its
pathname. Therefore we were not previously able to apply any
.gitattributes to the output. So this patch really does two things:
1. Pass the information along to show_blob_object so that it can
look up .gitattributes.
2. Apply the textconv attribute (if ALLOW_TEXTCONV is on, of course).
And stating it that way makes it clear that there may be other missing
steps (3 and up) to apply other gitattributes. For example, should "git
show $blob" respect crlf attributes? Smudge filters?
-Peff
^ permalink raw reply
* Re: [PATCH v3 0/8] Hiding refs
From: Junio C Hamano @ 2013-02-06 22:12 UTC (permalink / raw)
To: Michael Haggerty
Cc: Duy Nguyen, Jonathan Nieder, git, Jeff King, Shawn Pearce
In-Reply-To: <5112D028.4050005@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> On 02/06/2013 08:17 PM, Junio C Hamano wrote:
> ...
>
> Yes, the first three patches sound much more reasonable if this is the
> goal.
> ...
> I think that a more useful interim solution would be to make it easy to
> have two URLs accessing a single git repository, with different levels
> of reference visibility applied to each.
I think you said "more reasonable" without understanding what you
are saying is "more reasonable", then. The mechanism is about
server side wanting to use refs to protect its own metadata from gc
without having to expose them; there is no "different levels".
> ...
> GIT_CONFIG=config-with-hidden-refs git upload-pack ...
> or
> git -c transfer.hiderefs=refs/pull upload-pack ...
>
> But this is a bit awkward ...
It is awkward to use hammer to drive screws in wood, too. You want
to use a screwdriver. The first three patches are to drive a nail
with hammer, OK? Screws you keep bringing up is to be handled by
delayed ref advertisement.
^ permalink raw reply
* Re: [RFC/PATCH 2/4] cat-file: do not die on --textconv without textconv filters
From: Jeff King @ 2013-02-06 22:19 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Junio C Hamano
In-Reply-To: <b20e91bc71e59b5390005f2e6428e69a467e80b5.1360162813.git.git@drmicha.warpmail.net>
On Wed, Feb 06, 2013 at 04:08:51PM +0100, Michael J Gruber wrote:
> When a command is supposed to use textconv filters (by default or with
> "--textconv") and none are configured then the blob is output without
> conversion; the only exception to this rule is "cat-file --textconv".
>
> Make it behave like the rest of textconv aware commands.
Makes sense.
> - if (!textconv_object(obj_context.path, obj_context.mode, sha1, 1, &buf, &size))
> - die("git cat-file --textconv: unable to run textconv on %s",
> - obj_name);
> - break;
> + if (textconv_object(obj_context.path, obj_context.mode, sha1, 1, &buf, &size))
> + break;
The implication here is that textconv_object should be handling its own
errors and dying, and the return is always "yes, I converted" or "no, I
did not". Which I think is the case.
> +
> + /* otherwise expect a blob */
> + exp_type = "blob";
>
> case 0:
> if (type_from_string(exp_type) == OBJ_BLOB) {
I wondered at first why we needed to set exp_type here; shouldn't we
already be expecting a blob if we are doing textconv? But then I see
this is really about the fall-through in the switch (which we might want
an explicit comment for).
Which made me wonder: what happens with:
git cat-file --textconv HEAD
It looks like we die just before textconv-ing, because we have no
obj_context.path. But that is also unlike all of the other --textconv
switches, which mean "turn on textconv if you are showing a blob that
supports it" and not "the specific operation is --textconv, apply it to
this blob". I don't know if that is worth changing or not.
-Peff
^ permalink raw reply
* Re: [RFC/PATCH 3/4] grep: allow to use textconv filters
From: Jeff King @ 2013-02-06 22:23 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Junio C Hamano
In-Reply-To: <da8c01b918c94c84ab61859b1b1453885bff5b06.1360162813.git.git@drmicha.warpmail.net>
On Wed, Feb 06, 2013 at 04:08:52PM +0100, Michael J Gruber wrote:
> From: Jeff King <peff@peff.net>
>
> Recently and not so recently, we made sure that log/grep type operations
> use textconv filters when a userfacing diff would do the same:
>
> ef90ab6 (pickaxe: use textconv for -S counting, 2012-10-28)
> b1c2f57 (diff_grep: use textconv buffers for add/deleted files, 2012-10-28)
> 0508fe5 (combine-diff: respect textconv attributes, 2011-05-23)
>
> "git grep" currently does not use textconv filters at all, that is
> neither for displaying the match and context nor for the actual grepping.
>
> Introduce an option "--textconv" which makes git grep use any configured
> textconv filters for grepping and output purposes. It is off by default.
>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
Signed-off-by: Jeff King <peff@peff.net>
I'd really love to see the refactoring I talked about in my earlier
message. But as I'm not willing to devote the time to do it right now,
and I do not think this patch has any particular bugs, I think it is OK
as it gets the job done, and does not make the later refactoring any
harder.
The one ugliness that still remains is:
> + if (opt->allow_textconv) {
> + grep_source_load_driver(gs);
> + /*
> + * We might set up the shared textconv cache data here, which
> + * is not thread-safe.
> + */
> + grep_attr_lock();
> + textconv = userdiff_get_textconv(gs->driver);
> + grep_attr_unlock();
> + }
We lock/unlock the grep_attr_lock twice here: once in
grep_source_load_driver, and then immediately again to call
userdiff_get_textconv. I don't know if it is worth doing the two under
the same lock or not (I guess it should not increase lock contention,
since we do the same amount of work, so it is really just the extra lock
instructions).
-Peff
^ permalink raw reply
* Re: [RFC/PATCH 2/4] cat-file: do not die on --textconv without textconv filters
From: Junio C Hamano @ 2013-02-06 22:23 UTC (permalink / raw)
To: Jeff King; +Cc: Michael J Gruber, git
In-Reply-To: <20130206221912.GD27507@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Which made me wonder: what happens with:
>
> git cat-file --textconv HEAD
>
> It looks like we die just before textconv-ing, because we have no
> obj_context.path. But that is also unlike all of the other --textconv
> switches, which mean "turn on textconv if you are showing a blob that
> supports it" and not "the specific operation is --textconv, apply it to
> this blob". I don't know if that is worth changing or not.
OK, so in that sense, "cat-file --textconv HEAD" (or HEAD:) should
die with "Hey, that is not a blob", in other words, Michael's patch
does what we want without further tweaks, right?
By the way are we sure textconv_object() barfs and dies if fed a non
blob? Otherwise the above does not hold, and the semantics become
"turn on textconv if the object you are showing supports it,
otherwise it has to be a blob.", I think.
^ permalink raw reply
* Re: [PATCH v3 0/8] Hiding refs
From: Ævar Arnfjörð Bjarmason @ 2013-02-06 22:26 UTC (permalink / raw)
To: Junio C Hamano
Cc: Duy Nguyen, Michael Haggerty, Jonathan Nieder, git, Jeff King,
Shawn Pearce
In-Reply-To: <7v4nhpckwd.fsf@alter.siamese.dyndns.org>
On Wed, Feb 6, 2013 at 8:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
Maybe this should be split up into a different thread, but:
> The upload-pack-2 service sits on a port different from today's
> [...].
I think there's a simpler way to do this, which is that:
* New clients supporting v2 of the protocol send some piece of data
that would break old servers.
* If that fails the new client goes "oh jeeze, I guess it's an old
server", and try again with the old protocol.
* The client then saves a date (or the version the server gave us)
indicating that it tried the new protocol on that remote, tries
again sometime later.
We already covered in previous discussions how this would be simpler
with the HTTP protocol, since you could just send an extra header
inviting the server to speak the new protocol.
But for the other transports we can just try the new protocol and
retry with the old one as a fallback if it doesn't work. That'll allow
us to gracefully migrate without needing to change the git:// port.
Besides, I think the vast majority of users are using Git via http://
or ssh://, where we can't just change the port, but even so making
people change the port when we could handle this more gracefully would
be a big PITA. Adding new firewall holes is often a big bureaucratic
nightmare in some organizations.
^ permalink raw reply
* Re: [RFC/PATCH 4/4] grep: obey --textconv for the case rev:path
From: Jeff King @ 2013-02-06 22:36 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Junio C Hamano
In-Reply-To: <a2808975f00bac5de0902e6830f254e4b064e8a5.1360162813.git.git@drmicha.warpmail.net>
On Wed, Feb 06, 2013 at 04:08:53PM +0100, Michael J Gruber wrote:
> - add_object_array(object, arg, &list);
> + add_object_array_with_context(object, arg, &list, xmemdupz(&oc, sizeof(struct object_context)));
If we go this route, this new _with_context variant should be used in
patch 1, too.
> @@ -265,9 +260,28 @@ void add_object_array_with_mode(struct object *obj, const char *name, struct obj
> objects[nr].item = obj;
> objects[nr].name = name;
> objects[nr].mode = mode;
> + objects[nr].context = context;
> array->nr = ++nr;
> }
This seems a little gross. Who is responsible for allocating the
context? Who frees it? It looks like we duplicate it in cmd_grep. Which
I think is OK, but it means all of this context infrastructure in
object.[ch] is just bolted-on junk waiting for somebody to use it wrong
or get confused. It does not get set, for example, by the regular
setup_revisions code path.
It would be nice if we could just always have the context available,
then setup_revisions could set it up by default (and replace the "mode"
parameter entirely). But we'd need to do something to avoid the
PATH_MAX-sized buffer for each entry, as some code paths may have a
large number of pending objects.
-Peff
^ permalink raw reply
* Re: [RFC/PATCH 2/4] cat-file: do not die on --textconv without textconv filters
From: Jeff King @ 2013-02-06 22:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael J Gruber, git
In-Reply-To: <7vvca59j4n.fsf@alter.siamese.dyndns.org>
On Wed, Feb 06, 2013 at 02:23:36PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Which made me wonder: what happens with:
> >
> > git cat-file --textconv HEAD
> >
> > It looks like we die just before textconv-ing, because we have no
> > obj_context.path. But that is also unlike all of the other --textconv
> > switches, which mean "turn on textconv if you are showing a blob that
> > supports it" and not "the specific operation is --textconv, apply it to
> > this blob". I don't know if that is worth changing or not.
>
> OK, so in that sense, "cat-file --textconv HEAD" (or HEAD:) should
> die with "Hey, that is not a blob", in other words, Michael's patch
> does what we want without further tweaks, right?
Right, it will die because we do not find a path in the object_context.
For the same reason that "cat-file --textconv $sha1" would die.
A more interesting case is "cat-file --textconv HEAD:Documentation",
which does have a path, but not a blob. And I think that speaks to your
point that we want to fall-through to the pretty-print case, not the
blob case.
> By the way are we sure textconv_object() barfs and dies if fed a non
> blob? Otherwise the above does not hold, and the semantics become
> "turn on textconv if the object you are showing supports it,
> otherwise it has to be a blob.", I think.
I'm not sure. The sha1 would get passed all the way down to
fill_textconv. I think because sha1_valid is set, it will not try to
reuse the working tree file, so we will end up in
diff_populate_filespec, and we could actually textconv the tree object
itself.
So yeah, I think we want a check that makes sure we are working with a
blob before we even call that function, and "--textconv" should just be
"you must feed me a blob of the form $treeish:$path". In practice nobody
wants to do anything else anyway, so let's keep the code paths simple;
we can always loosen it later if there is a good reason to do so.
-Peff
^ permalink raw reply
* Re: [PATCH] Verify Content-Type from smart HTTP servers
From: Jeff King @ 2013-02-06 22:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Schubert, Shawn Pearce, git
In-Reply-To: <7v4nhpo2qv.fsf@alter.siamese.dyndns.org>
On Wed, Feb 06, 2013 at 07:56:08AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Is it worth having a strbuf_set* family of functions to match the
> > strbuf_add*? We seem to have these sorts of errors with strbuf from time
> > to time, and I wonder if that would make it easier (and more readable)
> > to do the right thing.
>
> Possibly.
>
> The callsite below may be a poor example, though; you would need the
> _reset() even if you change the _addstr() we can see in the context
> to _setstr() to make sure later strbuf_*(type) will start from a
> clean slate when !t anyway, no?
Ah, true. Let's not worry about it, then.
-Peff
^ permalink raw reply
* Re: [PATCH v3 0/8] Hiding refs
From: Jeff King @ 2013-02-06 22:56 UTC (permalink / raw)
To: Junio C Hamano
Cc: Duy Nguyen, Michael Haggerty, Jonathan Nieder, git, Shawn Pearce
In-Reply-To: <7v4nhpckwd.fsf@alter.siamese.dyndns.org>
On Wed, Feb 06, 2013 at 11:17:06AM -0800, Junio C Hamano wrote:
> Let me help unconfuse this thread.
>
> I think the series as 8-patch series was poorly presented, and
> separating it into two will help understanding what they are about.
>
> The first three:
>
> upload-pack: share more code
> upload-pack: simplify request validation
> upload/receive-pack: allow hiding ref hierarchies
>
> is _the_ topic of the series. As far as I am concerned (I am not
> speaking for Gerrit users, but am speaking as the Git maintainer),
> the topic is solely about uncluttering. There may be refs that the
> server end may need to keep for its operation, but that remote users
> have _no_ business knowing about. Allowing the server to keep these
> refs in the repository, while not showing these refs over the wire,
> is the problem the series solves.
>
> In other words, it is not about "these are *usually* not wanted by
> clients, so do not show them by default". It is about "these are
> not to be shown, ever".
>
> OK?
Right. I am not opposed to this series, as it does have a use-case. And
if it helps Gerrit folks or other users unclutter, great. The fact that
I could throw away the custom receive.hiderefs patch we use at GitHub is
a bonus. If people want fancier things, they can do them separately.
_But_. As a potential user of the feature (to hide refs/pull/*), I do
not think it is sufficiently flexible for me to use transfer.hiderefs
(or uploadpack.hiderefs). We use "fetch" internally to migrate objects
between forks and our alternates repos. And in that case, we really do
want to see all refs. In other words, all fetches are not the same: we
would want upload-pack to understand the difference between a client
fetch and an internal administrative fetch. But this feature does not
provide that lee-way. Even if you tried:
git fetch -u 'git -c uploadpack.hiderefs= upload-pack'
the list nature of the config variable means you cannot reset it.
This isn't a show-stopper for the series; it may just mean that it is
not a good fit for GitHub's use case, but others (like Gerrit) may
benefit. But since refs/pull is used as an example of where this could
be applied, I wanted to point out that it does not achieve that goal.
-Peff
^ permalink raw reply
* Re: [PATCH 2/4] Git.pm: Allow pipes to be closed prior to calling command_close_bidi_pipe
From: Jeff King @ 2013-02-06 23:04 UTC (permalink / raw)
To: Michal Nazarewicz; +Cc: Junio C Hamano, Ted Zlatanov, Matthieu Moy, git
In-Reply-To: <c0966644278b0addbef6a03289ef9c553addf573.1360183427.git.mina86@mina86.com>
On Wed, Feb 06, 2013 at 09:47:04PM +0100, Michal Nazarewicz wrote:
> From: Michal Nazarewicz <mina86@mina86.com>
>
> The command_close_bidi_pipe() function will insist on closing both
> input and output pipes returned by command_bidi_pipe(). With this
> change it is possible to close one of the pipes in advance and
> pass undef as an argument.
>
> This allows for something like:
>
> my ($pid, $in, $out, $ctx) = command_bidi_pipe(...);
> print $out "write data";
> close $out;
> # ... do stuff with $in
> command_close_bidi_pipe($pid, $in, undef, $ctx);
Should this part go into the documentation for command_close_bidi_pipe
in Git.pm?
-Peff
^ permalink raw reply
* Re: [PATCH] git-send-email: add ~/.authinfo parsing
From: Ted Zlatanov @ 2013-02-06 23:12 UTC (permalink / raw)
To: Jeff King
Cc: Matthieu Moy, Junio C Hamano, Michal Nazarewicz, git,
Krzysztof Mazur, Michal Nazarewicz
In-Reply-To: <20130206215724.GA27507@sigill.intra.peff.net>
On Wed, 6 Feb 2013 16:57:24 -0500 Jeff King <peff@peff.net> wrote:
JK> On Wed, Feb 06, 2013 at 10:58:13AM -0500, Ted Zlatanov wrote:
MM> I don't know about the netrc credential helper, but I guess that's
MM> another layer. The git-remote-mediawiki code is the code to call the
MM> credential C API, that in turn may (or may not) call a credential
MM> helper.
>>
>> Yup. But what you call "read" and "write" are, to the credential
>> helper, "write" and "read" but it's the same protocol :) So maybe the
>> names should be changed to reflect that, e.g. "query" and "response."
JK> Is that true? As a user of the credential system, git-remote-mediawiki
JK> would want to "write" to git-credential, then "read" the response. As a
JK> helper, git-credential-netrc would want to "read" the query then
JK> "write" the response. The order is different, but the operations should
JK> be the same in both cases.
Logically they are different steps (query and response), even though the
data protocol is the same. But it's really not a big deal, I know what
it means either way.
JK> The big difference is that mediawiki would want an additional function
JK> to open a pipe to "git credential" and operate on that, whereas the
JK> helper will be reading/writing stdio.
Yup.
Ted
^ 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