* [PATCH v7 07/16] i18n: add--interactive: mark patch prompt for translation
From: Vasco Almeida @ 2016-12-14 12:54 UTC (permalink / raw)
To: git
Cc: Vasco Almeida, Jiang Xin, Ævar Arnfjörð Bjarmason,
Jean-Noël AVILA, Jakub Narębski, David Aguilar,
Junio C Hamano
In-Reply-To: <20161005172110.30801-1-vascomalmeida@sapo.pt>
Mark prompt message assembled in place for translation, unfolding each
use case for each entry in the %patch_modes hash table.
Previously, this script relied on whether $patch_mode was set to run the
command patch_update_cmd() or show status and loop the main loop. Now,
it uses $cmd to indicate we must run patch_update_cmd() and $patch_mode
is used to tell which flavor of the %patch_modes are we on. This is
introduced in order to be able to mark and unfold the message prompt
knowing in which context we are.
The tracking of context was done previously by point %patch_mode_flavour
hash table to the correct entry of %patch_modes, focusing only on value
of %patch_modes. Now, we are also interested in the key ('staged',
'stash', 'checkout_head', ...).
Signed-off-by: Vasco Almeida <vascomalmeida@sapo.pt>
---
Makefile | 2 +-
git-add--interactive.perl | 54 ++++++++++++++++++++++++++++++++++++++++-------
perl/Git/I18N.pm | 11 +++++++++-
t/t0202/test.pl | 5 ++++-
4 files changed, 61 insertions(+), 11 deletions(-)
diff --git a/Makefile b/Makefile
index fdef1dd94..3c889dc63 100644
--- a/Makefile
+++ b/Makefile
@@ -2115,7 +2115,7 @@ XGETTEXT_FLAGS_C = $(XGETTEXT_FLAGS) --language=C \
XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \
--keyword=gettextln --keyword=eval_gettextln
XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --language=Perl \
- --keyword=__ --keyword="__n:1,2"
+ --keyword=__ --keyword=N__ --keyword="__n:1,2"
LOCALIZED_C = $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
LOCALIZED_SH = $(SCRIPT_SH)
LOCALIZED_SH += git-parse-remote.sh
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index cd617837b..b7d382b10 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -93,6 +93,7 @@ sub colored {
}
# command line options
+my $cmd;
my $patch_mode;
my $patch_mode_revision;
@@ -173,7 +174,8 @@ my %patch_modes = (
},
);
-my %patch_mode_flavour = %{$patch_modes{stage}};
+$patch_mode = 'stage';
+my %patch_mode_flavour = %{$patch_modes{$patch_mode}};
sub run_cmd_pipe {
if ($^O eq 'MSWin32') {
@@ -1311,6 +1313,44 @@ sub display_hunks {
return $i;
}
+my %patch_update_prompt_modes = (
+ stage => {
+ mode => N__("Stage mode change [y,n,q,a,d,/%s,?]? "),
+ deletion => N__("Stage deletion [y,n,q,a,d,/%s,?]? "),
+ hunk => N__("Stage this hunk [y,n,q,a,d,/%s,?]? "),
+ },
+ stash => {
+ mode => N__("Stash mode change [y,n,q,a,d,/%s,?]? "),
+ deletion => N__("Stash deletion [y,n,q,a,d,/%s,?]? "),
+ hunk => N__("Stash this hunk [y,n,q,a,d,/%s,?]? "),
+ },
+ reset_head => {
+ mode => N__("Unstage mode change [y,n,q,a,d,/%s,?]? "),
+ deletion => N__("Unstage deletion [y,n,q,a,d,/%s,?]? "),
+ hunk => N__("Unstage this hunk [y,n,q,a,d,/%s,?]? "),
+ },
+ reset_nothead => {
+ mode => N__("Apply mode change to index [y,n,q,a,d,/%s,?]? "),
+ deletion => N__("Apply deletion to index [y,n,q,a,d,/%s,?]? "),
+ hunk => N__("Apply this hunk to index [y,n,q,a,d,/%s,?]? "),
+ },
+ checkout_index => {
+ mode => N__("Discard mode change from worktree [y,n,q,a,d,/%s,?]? "),
+ deletion => N__("Discard deletion from worktree [y,n,q,a,d,/%s,?]? "),
+ hunk => N__("Discard this hunk from worktree [y,n,q,a,d,/%s,?]? "),
+ },
+ checkout_head => {
+ mode => N__("Discard mode change from index and worktree [y,n,q,a,d,/%s,?]? "),
+ deletion => N__("Discard deletion from index and worktree [y,n,q,a,d,/%s,?]? "),
+ hunk => N__("Discard this hunk from index and worktree [y,n,q,a,d,/%s,?]? "),
+ },
+ checkout_nothead => {
+ mode => N__("Apply mode change to index and worktree [y,n,q,a,d,/%s,?]? "),
+ deletion => N__("Apply deletion to index and worktree [y,n,q,a,d,/%s,?]? "),
+ hunk => N__("Apply this hunk to index and worktree [y,n,q,a,d,/%s,?]? "),
+ },
+);
+
sub patch_update_file {
my $quit = 0;
my ($ix, $num);
@@ -1383,12 +1423,9 @@ sub patch_update_file {
for (@{$hunk[$ix]{DISPLAY}}) {
print;
}
- print colored $prompt_color, $patch_mode_flavour{VERB},
- ($hunk[$ix]{TYPE} eq 'mode' ? ' mode change' :
- $hunk[$ix]{TYPE} eq 'deletion' ? ' deletion' :
- ' this hunk'),
- $patch_mode_flavour{TARGET},
- " [y,n,q,a,d,/$other,?]? ";
+ print colored $prompt_color,
+ sprintf(__($patch_update_prompt_modes{$patch_mode}{$hunk[$ix]{TYPE}}), $other);
+
my $line = prompt_single_character;
last unless defined $line;
if ($line) {
@@ -1644,6 +1681,7 @@ sub process_args {
die sprintf(__("invalid argument %s, expecting --"),
$arg) unless $arg eq "--";
%patch_mode_flavour = %{$patch_modes{$patch_mode}};
+ $cmd = 1;
}
elsif ($arg ne "--") {
die sprintf(__("invalid argument %s, expecting --"), $arg);
@@ -1680,7 +1718,7 @@ sub main_loop {
process_args();
refresh();
-if ($patch_mode) {
+if ($cmd) {
patch_update_cmd();
}
else {
diff --git a/perl/Git/I18N.pm b/perl/Git/I18N.pm
index 617d8c2a1..c41425c8d 100644
--- a/perl/Git/I18N.pm
+++ b/perl/Git/I18N.pm
@@ -13,7 +13,7 @@ BEGIN {
}
}
-our @EXPORT = qw(__ __n);
+our @EXPORT = qw(__ __n N__);
our @EXPORT_OK = @EXPORT;
sub __bootstrap_locale_messages {
@@ -54,6 +54,8 @@ BEGIN
*__ = sub ($) { $_[0] };
*__n = sub ($$$) { $_[2] == 1 ? $_[0] : $_[1] };
};
+
+ sub N__($) { return shift; }
}
1;
@@ -74,6 +76,7 @@ Git::I18N - Perl interface to Git's Gettext localizations
printf __n("commited %d file\n", "commited %d files\n", $files), $files;
+
=head1 DESCRIPTION
Git's internal Perl interface to gettext via L<Locale::Messages>. If
@@ -95,6 +98,12 @@ passthrough fallback function.
L<Locale::Messages>'s ngettext function or passthrough fallback function.
+=head2 N__($)
+
+No-operation that only returns its argument. Use this if you want xgettext to
+extract the text to the pot template but do not want to trigger retrival of the
+translation at run time.
+
=head1 AUTHOR
E<AElig>var ArnfjE<ouml>rE<eth> Bjarmason <avarab@gmail.com>
diff --git a/t/t0202/test.pl b/t/t0202/test.pl
index 4101833a8..2cbf7b959 100755
--- a/t/t0202/test.pl
+++ b/t/t0202/test.pl
@@ -4,7 +4,7 @@ use lib (split(/:/, $ENV{GITPERLLIB}));
use strict;
use warnings;
use POSIX qw(:locale_h);
-use Test::More tests => 11;
+use Test::More tests => 13;
use Git::I18N;
my $has_gettext_library = $Git::I18N::__HAS_LIBRARY;
@@ -32,6 +32,7 @@ is_deeply(\@Git::I18N::EXPORT, \@Git::I18N::EXPORT_OK, "sanity: Git::I18N export
my %prototypes = (qw(
__ $
__n $$$
+ N__ $
));
while (my ($sub, $proto) = each %prototypes) {
is(prototype(\&{"Git::I18N::$sub"}), $proto, "sanity: $sub has a $proto prototype");
@@ -55,6 +56,8 @@ is_deeply(\@Git::I18N::EXPORT, \@Git::I18N::EXPORT_OK, "sanity: Git::I18N export
"Get singular string through __n() in C locale");
is(__n($got_singular, $got_plural, 2), $expect_plural,
"Get plural string through __n() in C locale");
+
+ is(N__($got), $expect, "Passing a string through N__() in the C locale works");
}
# Test a basic message on different locales
--
2.11.0.44.g7d42c6c
^ permalink raw reply related
* [PATCH v7 13/16] i18n: send-email: mark warnings and errors for translation
From: Vasco Almeida @ 2016-12-14 12:54 UTC (permalink / raw)
To: git
Cc: Vasco Almeida, Jiang Xin, Ævar Arnfjörð Bjarmason,
Jean-Noël AVILA, Jakub Narębski, David Aguilar,
Junio C Hamano
In-Reply-To: <20161005172110.30801-1-vascomalmeida@sapo.pt>
Mark warnings, errors and other messages for translation.
Signed-off-by: Vasco Almeida <vascomalmeida@sapo.pt>
---
git-send-email.perl | 34 +++++++++++++++++-----------------
1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 06e64699b..00d234e11 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -118,20 +118,20 @@ sub format_2822_time {
my $localmin = $localtm[1] + $localtm[2] * 60;
my $gmtmin = $gmttm[1] + $gmttm[2] * 60;
if ($localtm[0] != $gmttm[0]) {
- die "local zone differs from GMT by a non-minute interval\n";
+ die __("local zone differs from GMT by a non-minute interval\n");
}
if ((($gmttm[6] + 1) % 7) == $localtm[6]) {
$localmin += 1440;
} elsif ((($gmttm[6] - 1) % 7) == $localtm[6]) {
$localmin -= 1440;
} elsif ($gmttm[6] != $localtm[6]) {
- die "local time offset greater than or equal to 24 hours\n";
+ die __("local time offset greater than or equal to 24 hours\n");
}
my $offset = $localmin - $gmtmin;
my $offhour = $offset / 60;
my $offmin = abs($offset % 60);
if (abs($offhour) >= 24) {
- die ("local time offset greater than or equal to 24 hours\n");
+ die __("local time offset greater than or equal to 24 hours\n");
}
return sprintf("%s, %2d %s %d %02d:%02d:%02d %s%02d%02d",
@@ -199,13 +199,13 @@ sub do_edit {
map {
system('sh', '-c', $editor.' "$@"', $editor, $_);
if (($? & 127) || ($? >> 8)) {
- die("the editor exited uncleanly, aborting everything");
+ die(__("the editor exited uncleanly, aborting everything"));
}
} @_;
} else {
system('sh', '-c', $editor.' "$@"', $editor, @_);
if (($? & 127) || ($? >> 8)) {
- die("the editor exited uncleanly, aborting everything");
+ die(__("the editor exited uncleanly, aborting everything"));
}
}
}
@@ -299,7 +299,7 @@ my $help;
my $rc = GetOptions("h" => \$help,
"dump-aliases" => \$dump_aliases);
usage() unless $rc;
-die "--dump-aliases incompatible with other options\n"
+die __("--dump-aliases incompatible with other options\n")
if !$help and $dump_aliases and @ARGV;
$rc = GetOptions(
"sender|from=s" => \$sender,
@@ -362,7 +362,7 @@ unless ($rc) {
usage();
}
-die "Cannot run git format-patch from outside a repository\n"
+die __("Cannot run git format-patch from outside a repository\n")
if $format_patch and not $repo;
# Now, let's fill any that aren't set in with defaults:
@@ -617,7 +617,7 @@ while (defined(my $f = shift @ARGV)) {
}
if (@rev_list_opts) {
- die "Cannot run git format-patch from outside a repository\n"
+ die __("Cannot run git format-patch from outside a repository\n")
unless $repo;
push @files, $repo->command('format-patch', '-o', tempdir(CLEANUP => 1), @rev_list_opts);
}
@@ -638,7 +638,7 @@ if (@files) {
print $_,"\n" for (@files);
}
} else {
- print STDERR "\nNo patch files specified!\n\n";
+ print STDERR __("\nNo patch files specified!\n\n");
usage();
}
@@ -730,7 +730,7 @@ EOT
$sender = $1;
next;
} elsif (/^(?:To|Cc|Bcc):/i) {
- print "To/Cc/Bcc fields are not interpreted yet, they have been ignored\n";
+ print __("To/Cc/Bcc fields are not interpreted yet, they have been ignored\n");
next;
}
print $c2 $_;
@@ -739,7 +739,7 @@ EOT
close $c2;
if ($summary_empty) {
- print "Summary email is empty, skipping it\n";
+ print __("Summary email is empty, skipping it\n");
$compose = -1;
}
} elsif ($annotate) {
@@ -1316,7 +1316,7 @@ EOF
$_ = ask(__("Send this email? ([y]es|[n]o|[q]uit|[a]ll): "),
valid_re => qr/^(?:yes|y|no|n|quit|q|all|a)/i,
default => $ask_default);
- die "Send this email reply required" unless defined $_;
+ die __("Send this email reply required") unless defined $_;
if (/^n/i) {
return 0;
} elsif (/^q/i) {
@@ -1342,7 +1342,7 @@ EOF
} else {
if (!defined $smtp_server) {
- die "The required SMTP server is not properly defined."
+ die __("The required SMTP server is not properly defined.")
}
if ($smtp_encryption eq 'ssl') {
@@ -1427,10 +1427,10 @@ EOF
}
print $header, "\n";
if ($smtp) {
- print "Result: ", $smtp->code, ' ',
+ print __("Result: "), $smtp->code, ' ',
($smtp->message =~ /\n([^\n]+\n)$/s), "\n";
} else {
- print "Result: OK\n";
+ print __("Result: OK\n");
}
}
@@ -1703,7 +1703,7 @@ sub apply_transfer_encoding {
$message = MIME::Base64::decode($message)
if ($from eq 'base64');
- die "cannot send message as 7bit"
+ die __("cannot send message as 7bit")
if ($to eq '7bit' and $message =~ /[^[:ascii:]]/);
return $message
if ($to eq '7bit' or $to eq '8bit');
@@ -1711,7 +1711,7 @@ sub apply_transfer_encoding {
if ($to eq 'quoted-printable');
return MIME::Base64::encode($message, "\n")
if ($to eq 'base64');
- die "invalid transfer encoding";
+ die __("invalid transfer encoding");
}
sub unique_email_list {
--
2.11.0.44.g7d42c6c
^ permalink raw reply related
* [PATCH v7 16/16] i18n: difftool: mark warnings for translation
From: Vasco Almeida @ 2016-12-14 12:54 UTC (permalink / raw)
To: git
Cc: Vasco Almeida, Jiang Xin, Ævar Arnfjörð Bjarmason,
Jean-Noël AVILA, Jakub Narębski, David Aguilar,
Junio C Hamano
In-Reply-To: <20161005172110.30801-1-vascomalmeida@sapo.pt>
Signed-off-by: Vasco Almeida <vascomalmeida@sapo.pt>
---
git-difftool.perl | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/git-difftool.perl b/git-difftool.perl
index a5790d03a..8d3632e55 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -22,6 +22,7 @@ use File::Path qw(mkpath rmtree);
use File::Temp qw(tempdir);
use Getopt::Long qw(:config pass_through);
use Git;
+use Git::I18N;
sub usage
{
@@ -122,7 +123,7 @@ sub setup_dir_diff
my $i = 0;
while ($i < $#rawdiff) {
if ($rawdiff[$i] =~ /^::/) {
- warn << 'EOF';
+ warn __ <<'EOF';
Combined diff formats ('-c' and '--cc') are not supported in
directory diff mode ('-d' and '--dir-diff').
EOF
@@ -338,7 +339,7 @@ sub main
if (length($opts{difftool_cmd}) > 0) {
$ENV{GIT_DIFF_TOOL} = $opts{difftool_cmd};
} else {
- print "No <tool> given for --tool=<tool>\n";
+ print __("No <tool> given for --tool=<tool>\n");
usage(1);
}
}
@@ -346,7 +347,7 @@ sub main
if (length($opts{extcmd}) > 0) {
$ENV{GIT_DIFFTOOL_EXTCMD} = $opts{extcmd};
} else {
- print "No <cmd> given for --extcmd=<cmd>\n";
+ print __("No <cmd> given for --extcmd=<cmd>\n");
usage(1);
}
}
@@ -419,11 +420,11 @@ sub dir_diff
}
if (exists $wt_modified{$file} and exists $tmp_modified{$file}) {
- my $errmsg = "warning: Both files modified: ";
- $errmsg .= "'$workdir/$file' and '$b/$file'.\n";
- $errmsg .= "warning: Working tree file has been left.\n";
- $errmsg .= "warning:\n";
- warn $errmsg;
+ warn sprintf(__(
+ "warning: Both files modified:\n" .
+ "'%s/%s' and '%s/%s'.\n" .
+ "warning: Working tree file has been left.\n" .
+ "warning:\n"), $workdir, $file, $b, $file);
$error = 1;
} elsif (exists $tmp_modified{$file}) {
my $mode = stat("$b/$file")->mode;
@@ -435,8 +436,9 @@ sub dir_diff
}
}
if ($error) {
- warn "warning: Temporary files exist in '$tmpdir'.\n";
- warn "warning: You may want to cleanup or recover these.\n";
+ warn sprintf(__(
+ "warning: Temporary files exist in '%s'.\n" .
+ "warning: You may want to cleanup or recover these.\n"), $tmpdir);
exit(1);
} else {
exit_cleanup($tmpdir, $rc);
--
2.11.0.44.g7d42c6c
^ permalink raw reply related
* [PATCH v7 15/16] i18n: send-email: mark composing message for translation
From: Vasco Almeida @ 2016-12-14 12:54 UTC (permalink / raw)
To: git
Cc: Vasco Almeida, Jiang Xin, Ævar Arnfjörð Bjarmason,
Jean-Noël AVILA, Jakub Narębski, David Aguilar,
Junio C Hamano
In-Reply-To: <20161005172110.30801-1-vascomalmeida@sapo.pt>
When composing an e-mail, there is a message for the user whose lines
begin in "GIT:" that can be marked for translation.
Signed-off-by: Vasco Almeida <vascomalmeida@sapo.pt>
---
git-send-email.perl | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 7f3297cdf..068d60b3e 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -672,18 +672,20 @@ if ($compose) {
my $tpl_subject = $initial_subject || '';
my $tpl_reply_to = $initial_reply_to || '';
- print $c <<EOT;
+ print $c <<EOT1, Git::prefix_lines("GIT: ", __ <<EOT2), <<EOT3;
From $tpl_sender # This line is ignored.
-GIT: Lines beginning in "GIT:" will be removed.
-GIT: Consider including an overall diffstat or table of contents
-GIT: for the patch you are writing.
-GIT:
-GIT: Clear the body content if you don't wish to send a summary.
+EOT1
+Lines beginning in "GIT:" will be removed.
+Consider including an overall diffstat or table of contents
+for the patch you are writing.
+
+Clear the body content if you don't wish to send a summary.
+EOT2
From: $tpl_sender
Subject: $tpl_subject
In-Reply-To: $tpl_reply_to
-EOT
+EOT3
for my $f (@files) {
print $c get_patch_subject($f);
}
--
2.11.0.44.g7d42c6c
^ permalink raw reply related
* [ANNOUNCE] Git Rev News edition 22
From: Christian Couder @ 2016-12-14 13:06 UTC (permalink / raw)
To: git
Cc: lwn, Thomas Ferris Nicolaisen, Jakub Narebski, Markus Jansen,
Johannes Schindelin, Junio C Hamano, Jeff King, Michael Haggerty,
David Aguilar, Dun Peal, Stefan Beller, Robert Dailey,
Jacob Keller
Hi everyone,
The 22nd edition of Git Rev News is now published:
https://git.github.io/rev_news/2016/12/14/edition-22/
Thanks a lot to all the contributors and helpers, especially David!
Enjoy,
Christian, Thomas, Jakub and Markus.
^ permalink raw reply
* Re: [PATCH v2 28/34] run_command_opt(): optionally hide stderr when the command succeeds
From: Jeff King @ 2016-12-14 13:06 UTC (permalink / raw)
To: Johannes Sixt
Cc: Johannes Schindelin, git, Junio C Hamano, Kevin Daudt,
Dennis Kaarsemaker
In-Reply-To: <20161214125322.o3naglvyuzgk2pri@sigill.intra.peff.net>
On Wed, Dec 14, 2016 at 07:53:23AM -0500, Jeff King wrote:
> On Wed, Dec 14, 2016 at 09:34:20AM +0100, Johannes Sixt wrote:
>
> > I wanted to see what it would look like if we make it the caller's
> > responsibility to throw away stderr. The patch is below, as fixup
> > of patch 29/34. The change is gross, but the end result is not that
> > bad, though not really a delightful read, either, mostly due to the
> > strange cleanup semantics of the start_command/finish_command combo,
> > so... I dunno.
>
> I don't have a strong opinion on the patches under discussion, but here
> are a few pointers on the run-command interface:
> [...]
And here is a patch representing my suggestions, on top of yours. Not
tested beyond "make test".
I think read_author_script() could be simplified even more by appending
to the env array in the first loop, but I didn't want to refactor the
quote handling.
---
sequencer.c | 65 +++++++++++------------------------
1 file changed, 20 insertions(+), 45 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index f375880bd..27a9eaf15 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -591,18 +591,17 @@ static int write_author_script(const char *message)
}
/*
- * Read the author-script file into an environment block, ready for use in
- * run_command(), that can be free()d afterwards.
+ * Read the author-script file into an environment block. Returns -1
+ * on error, 0 otherwise.
*/
-static char **read_author_script(void)
+static int read_author_script(struct argv_array *env)
{
struct strbuf script = STRBUF_INIT;
int i, count = 0;
- char *p, *p2, **env;
- size_t env_size;
+ char *p, *p2;
if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)
- return NULL;
+ return -1;
for (p = script.buf; *p; p++)
if (skip_prefix(p, "'\\\\''", (const char **)&p2))
@@ -614,19 +613,12 @@ static char **read_author_script(void)
count++;
}
- env_size = (count + 1) * sizeof(*env);
- strbuf_grow(&script, env_size);
- memmove(script.buf + env_size, script.buf, script.len);
- p = script.buf + env_size;
- env = (char **)strbuf_detach(&script, NULL);
-
for (i = 0; i < count; i++) {
- env[i] = p;
+ argv_array_push(env, p);
p += strlen(p) + 1;
}
- env[count] = NULL;
- return env;
+ return 0;
}
static const char staged_changes_advice[] =
@@ -659,20 +651,18 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
int allow_empty, int edit, int amend,
int cleanup_commit_message)
{
- char **env = NULL;
- int rc;
const char *value;
- struct strbuf errout = STRBUF_INIT;
struct child_process cmd = CHILD_PROCESS_INIT;
cmd.git_cmd = 1;
if (is_rebase_i(opts)) {
- if (!edit)
+ if (!edit) {
cmd.stdout_to_stderr = 1;
+ cmd.err = -1;
+ }
- env = read_author_script();
- if (!env) {
+ if (read_author_script(&cmd.env_array)) {
const char *gpg_opt = gpg_sign_opt_quoted(opts);
return error(_(staged_changes_advice),
@@ -706,35 +696,20 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
if (opts->allow_empty_message)
argv_array_push(&cmd.args, "--allow-empty-message");
- cmd.env = (const char *const *)env;
-
- if (cmd.stdout_to_stderr) {
+ if (cmd.err == -1) {
/* hide stderr on success */
- cmd.err = -1;
- rc = -1;
- if (start_command(&cmd) < 0)
- goto cleanup;
-
- if (strbuf_read(&errout, cmd.err, 0) < 0) {
- close(cmd.err);
- finish_command(&cmd); /* throw away exit code */
- goto cleanup;
- }
-
- close(cmd.err);
- rc = finish_command(&cmd);
+ struct strbuf errout = STRBUF_INIT;
+ int rc = pipe_command(&cmd,
+ NULL, 0, /* stdin */
+ NULL, 0, /* stdout */
+ &errout, 0);
if (rc)
fputs(errout.buf, stderr);
- } else {
- rc = run_command(&cmd);
+ strbuf_release(&errout);
+ return rc;
}
-cleanup:
- child_process_clear(&cmd);
- strbuf_release(&errout);
- free(env);
-
- return rc;
+ return run_command(&cmd);
}
static int is_original_commit_empty(struct commit *commit)
^ permalink raw reply related
* git stash filename - stashing single files.
From: Jonas Hartmann @ 2016-12-14 13:53 UTC (permalink / raw)
To: git
[-- Attachment #1.1.1: Type: text/plain, Size: 449 bytes --]
Hello,
http://stackoverflow.com/questions/3040833/stash-only-one-file-out-of-multiple-files-that-have-changed-with-git#comment32451416_3040833
Could it be possible to have "git stash [filename][filename]...", to
stash only single files?
There seems to be a broad community desire.
Best
--
Dipl.-Soz. Jonas Hartmann
HT Studios · Custom-Tailored Software-Engineering
jh@ht-studios.de
+49 160 9924 4679
http://www.ht-studios.de
[-- Attachment #1.1.2: jh.vcf --]
[-- Type: text/x-vcard, Size: 356 bytes --]
begin:vcard
fn:Dipl.-Soz. Jonas Hartmann
n:Hartmann;Jonas
org;quoted-printable:HT Studios =C2=B7 Custom-Tailored Software-Engineering;Founder
adr:;;Mainstr. 93;Pfungstadt;Hessen;64319;Germany
email;internet:jh@ht-studios.de
title:Dipl.-Soz.
tel;work:+49 160 99 24 46 79
x-mozilla-html:FALSE
url:http://www.ht-studios.de
version:2.1
end:vcard
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* [PATCH 0/4] "make tags" improvements
From: Jeff King @ 2016-12-14 14:25 UTC (permalink / raw)
To: git; +Cc: Chris Packham
A discussion earlier today made me notice that our "make tags" target
includes some cruft that we shouldn't be indexing. This fixes that, and
starts indexing some of the shell scripts, which is something I've been
doing in a hacky way for a while. It's very convenient when working on
the test suite (especially because I can never remember which functions
are declared in test-lib.sh, and which in test-lib-functions.sh).
[1/4]: Makefile: reformat FIND_SOURCE_FILES
[2/4]: Makefile: exclude test cruft from FIND_SOURCE_FILES
[3/4]: Makefile: match shell scripts in FIND_SOURCE_FILES
[4/4]: Makefile: exclude contrib from FIND_SOURCE_FILES
Makefile | 19 ++++++++++++++++---
1 file changed, 16 insertions(+), 3 deletions(-)
-Peff
^ permalink raw reply
* [PATCH 1/4] Makefile: reformat FIND_SOURCE_FILES
From: Jeff King @ 2016-12-14 14:26 UTC (permalink / raw)
To: git; +Cc: Chris Packham
In-Reply-To: <20161214142533.svktxk63eiwaaeor@sigill.intra.peff.net>
As we add to this in future commits, the formatting is going
to make it harder and harder to read. Let's write it more as
we would in a shell script, putting each logical block on
its own line.
Signed-off-by: Jeff King <peff@peff.net>
---
Just to make the other patches easier to read; no behavior change
intended.
I was tempted to actually pull this out into its own
find-source-files.sh script. I don't know if that is
preferable or not; it certainly makes the "make tags"
output less ugly.
Makefile | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/Makefile b/Makefile
index f53fcc90d..f42b1953d 100644
--- a/Makefile
+++ b/Makefile
@@ -2149,9 +2149,12 @@ endif
po/build/locale/%/LC_MESSAGES/git.mo: po/%.po
$(QUIET_MSGFMT)mkdir -p $(dir $@) && $(MSGFMT) -o $@ $<
-FIND_SOURCE_FILES = ( git ls-files '*.[hcS]' 2>/dev/null || \
- $(FIND) . \( -name .git -type d -prune \) \
- -o \( -name '*.[hcS]' -type f -print \) )
+FIND_SOURCE_FILES = ( \
+ git ls-files '*.[hcS]' 2>/dev/null || \
+ $(FIND) . \
+ \( -name .git -type d -prune \) \
+ -o \( -name '*.[hcS]' -type f -print \) \
+ )
$(ETAGS_TARGET): FORCE
$(RM) $(ETAGS_TARGET)
--
2.11.0.341.g202cd3142
^ permalink raw reply related
* [PATCH 2/4] Makefile: exclude test cruft from FIND_SOURCE_FILES
From: Jeff King @ 2016-12-14 14:28 UTC (permalink / raw)
To: git; +Cc: Chris Packham
In-Reply-To: <20161214142533.svktxk63eiwaaeor@sigill.intra.peff.net>
The test directory may contain three types of files that
match our patterns:
1. Helper programs in t/helper.
2. Sample data files (e.g., t/t4051/hello.c).
3. Untracked cruft in trash directories and t/perf/build.
We want to match (1), but not the other two, as they just
clutter up the list.
For the ls-files method, we can drop (2) with a negative
pathspec. We do not have to care about (3), since ls-files
will not list untracked files.
For `find`, we can match both cases with `-prune` patterns.
Signed-off-by: Jeff King <peff@peff.net>
---
I expected that:
':![tp][0-9][0-9][0-9][0-9]'
would work, but it doesn't. I think because we don't match intermediate
directories against pathspecs. So you have to use wildcards to match the
rest of the path.
Makefile | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index f42b1953d..001126931 100644
--- a/Makefile
+++ b/Makefile
@@ -2150,9 +2150,15 @@ po/build/locale/%/LC_MESSAGES/git.mo: po/%.po
$(QUIET_MSGFMT)mkdir -p $(dir $@) && $(MSGFMT) -o $@ $<
FIND_SOURCE_FILES = ( \
- git ls-files '*.[hcS]' 2>/dev/null || \
+ git ls-files \
+ '*.[hcS]' \
+ ':!*[tp][0-9][0-9][0-9][0-9]*' \
+ 2>/dev/null || \
$(FIND) . \
\( -name .git -type d -prune \) \
+ -o \( -name '[tp][0-9][0-9][0-9][0-9]' -type d -prune \) \
+ -o \( -name build -type d -prune \) \
+ -o \( -name 'trash*' -type d -prune \) \
-o \( -name '*.[hcS]' -type f -print \) \
)
--
2.11.0.341.g202cd3142
^ permalink raw reply related
* [PATCH 3/4] Makefile: match shell scripts in FIND_SOURCE_FILES
From: Jeff King @ 2016-12-14 14:29 UTC (permalink / raw)
To: git; +Cc: Chris Packham
In-Reply-To: <20161214142533.svktxk63eiwaaeor@sigill.intra.peff.net>
We feed FIND_SOURCE_FILES to ctags to help developers
navigate to particular functions, but we only feed C source
code. The same feature can be helpful when working with
shell scripts (especially the test suite). Modern versions
of ctags know how to parse shell scripts; we just need to
feed the filenames to it.
This patch specifically avoids including the individual test
scripts themselves. Those are unlikely to be of interest,
and there are a lot of them to process. It does pick up
test-lib.sh and test-lib-functions.sh.
Note that our negative pathspec already excludes the
individual scripts for the ls-files case, but we need to
loosen the `find` rule to match it.
Signed-off-by: Jeff King <peff@peff.net>
---
Maybe people would find this annoying. It does increase the number of
entries in the tags file. I've been running something like it for a few
months and have found it useful.
It's also possible that some people have a version of ctags or etags
that doesn't handle shell scripts. I imagine few enough people actually
run "make tags" in the first place that we can probably try it and see.
Makefile | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index 001126931..ef8de4a75 100644
--- a/Makefile
+++ b/Makefile
@@ -2152,14 +2152,16 @@ po/build/locale/%/LC_MESSAGES/git.mo: po/%.po
FIND_SOURCE_FILES = ( \
git ls-files \
'*.[hcS]' \
+ '*.sh' \
':!*[tp][0-9][0-9][0-9][0-9]*' \
2>/dev/null || \
$(FIND) . \
\( -name .git -type d -prune \) \
- -o \( -name '[tp][0-9][0-9][0-9][0-9]' -type d -prune \) \
+ -o \( -name '[tp][0-9][0-9][0-9][0-9]*' -prune \) \
-o \( -name build -type d -prune \) \
-o \( -name 'trash*' -type d -prune \) \
-o \( -name '*.[hcS]' -type f -print \) \
+ -o \( -name '*.sh' -type f -print \) \
)
$(ETAGS_TARGET): FORCE
--
2.11.0.341.g202cd3142
^ permalink raw reply related
* [PATCH 4/4] Makefile: exclude contrib from FIND_SOURCE_FILES
From: Jeff King @ 2016-12-14 14:32 UTC (permalink / raw)
To: git; +Cc: Chris Packham
In-Reply-To: <20161214142533.svktxk63eiwaaeor@sigill.intra.peff.net>
When you're working on the git project, you're unlikely to
care about random bits in contrib/ (e.g., you would not want
to jump to the copy of xmalloc in the wincred credential
helper). Nobody has really complained because there are
relatively few C files in contrib.
Now that we're matching shell scripts, too, we get quite a
few more hits, especially in the obsolete contrib/examples
directory. Looking for usage() should turn up the one in
git-sh-setup, not in some long-dead version of git-clone.
Let's just exclude all of contrib. Any specific projects
there which are big enough to want tags can generate them
separately.
Signed-off-by: Jeff King <peff@peff.net>
---
Makefile | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Makefile b/Makefile
index ef8de4a75..76267262c 100644
--- a/Makefile
+++ b/Makefile
@@ -2154,10 +2154,12 @@ FIND_SOURCE_FILES = ( \
'*.[hcS]' \
'*.sh' \
':!*[tp][0-9][0-9][0-9][0-9]*' \
+ ':!contrib' \
2>/dev/null || \
$(FIND) . \
\( -name .git -type d -prune \) \
-o \( -name '[tp][0-9][0-9][0-9][0-9]*' -prune \) \
+ -o \( -name contrib -type d -prune \) \
-o \( -name build -type d -prune \) \
-o \( -name 'trash*' -type d -prune \) \
-o \( -name '*.[hcS]' -type f -print \) \
--
2.11.0.341.g202cd3142
^ permalink raw reply related
* Re: git stash filename - stashing single files.
From: Jeff King @ 2016-12-14 14:44 UTC (permalink / raw)
To: Jonas Hartmann; +Cc: git
In-Reply-To: <b528e23b-c763-846e-4040-504a58b690fd@ht-studios.de>
On Wed, Dec 14, 2016 at 02:53:20PM +0100, Jonas Hartmann wrote:
> http://stackoverflow.com/questions/3040833/stash-only-one-file-out-of-multiple-files-that-have-changed-with-git#comment32451416_3040833
>
> Could it be possible to have "git stash [filename][filename]...", to
> stash only single files?
> There seems to be a broad community desire.
I think this would be useful. You can pick and choose with "git stash
-p", but I have still often wanted "git stash -p [filename]".
There is one problem, though: any non-option arguments to "git stash
save" are interpreted as the stash message. So just:
git stash save file
would break backwards compatibility. Annoyingly, so would:
git stash save -- file
which uses the "--" to let you have a message which starts with a dash.
Personally, I think this is a pretty terrible interface. Besides the
fact that I have never written a stash message in all my years of using
git, it's totally inconsistent with the rest of git (which would use
"-m" for the message, and treat arguments as pathspecs).
So it might be worth changing, but we'd probably have to deal with the
backwards compatibility fallout, have a deprecation period, etc.
As for "git stash" without "save", there is magic to rewrite:
git stash [opts]
into
git stash save [opts]
but it explicitly does not allow non-option arguments. So:
git stash foo
is an error (and not unreasonably, since "git stash list" creates an
ambiguity problem). Perhaps:
git stash -- foo
could be allowed to treat "foo" as a filename. There wouldn't be any
backwards compatibility problems, though it would be weird and
inconsistent to be able to specify filenames via the "shortcut"
invocation, but not with "git stash save".
-Peff
^ permalink raw reply
* Re: [RFC/PATCHv2] Makefile: add cppcheck target
From: Jeff King @ 2016-12-14 14:46 UTC (permalink / raw)
To: Chris Packham; +Cc: git, stefan.naewe, gitter.spiros
In-Reply-To: <20161214112401.mq3n5kui5eeebdtk@sigill.intra.peff.net>
On Wed, Dec 14, 2016 at 06:24:01AM -0500, Jeff King wrote:
> On Wed, Dec 14, 2016 at 10:27:31PM +1300, Chris Packham wrote:
>
> > Changes in v2:
> > - only run over actual git source files.
> > - omit any files in t/
>
> I actually wonder if FIND_SOURCE_FILES should be taking care of the "t/"
> thing. I think "make tags" finds tags in t4051/appended1.c, which is
> just silly.
I just posted a series[1] that should improve things there. But be aware
that it _also_ adds in shell scripts to the result. So you'd maybe want
to pipe the result through "grep -v '\.sh'" for cppcheck.
-Peff
[1] http://public-inbox.org/git/20161214142533.svktxk63eiwaaeor@sigill.intra.peff.net/t/#u
^ permalink raw reply
* Re: git stash filename - stashing single files.
From: Paul Smith @ 2016-12-14 15:00 UTC (permalink / raw)
To: Jeff King, Jonas Hartmann; +Cc: git
In-Reply-To: <20161214144444.k4v64accedl6xvho@sigill.intra.peff.net>
On Wed, 2016-12-14 at 09:44 -0500, Jeff King wrote:
> Personally, I think this is a pretty terrible interface. Besides the
> fact that I have never written a stash message in all my years of using
> git, it's totally inconsistent with the rest of git (which would use
> "-m" for the message, and treat arguments as pathspecs).
I agree that this is a terrible (and inconsistent) interface and I would
love to see it changed.
I do use messages on stashes all the time (because I can never remember
their context a week later without a hint--just looking at the diff is
not enough for me) but I would welcome this change. Definitely the
first step would be introducing the "-m" option so people and tools
could begin the switch to using it.
^ permalink raw reply
* [PATCH] parse-options: print "fatal:" before usage_msg_opt()
From: Jeff King @ 2016-12-14 15:10 UTC (permalink / raw)
To: git
Programs may use usage_msg_opt() to print a brief message
followed by the program usage, and then exit. The message
isn't prefixed at all, though, so it doesn't match our usual
error output and is easy to overlook:
$ git clone 1 2 3
Too many arguments.
usage: git clone [<options>] [--] <repo> [<dir>]
-v, --verbose be more verbose
-q, --quiet be more quiet
--progress force progress reporting
-n, --no-checkout don't create a checkout
--bare create a bare repository
[...and so on for another 31 lines...]
It looks especially bad when the message starts with an
option, like:
$ git replace -e
-e needs exactly one argument
usage: git replace [-f] <object> <replacement>
or: git replace [-f] --edit <object>
[...etc...]
Let's put our usual "fatal:" prefix in front of it.
Signed-off-by: Jeff King <peff@peff.net>
---
Some of the message in git-clone could stand to be rewritten to match
our usual style, too (no capitals, no trailing period), but that's
obviously out of scope for this patch. I don't think this change makes
them look any worse.
parse-options.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/parse-options.c b/parse-options.c
index 312a85dbd..4fbe924a5 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -661,7 +661,7 @@ void NORETURN usage_msg_opt(const char *msg,
const char * const *usagestr,
const struct option *options)
{
- fprintf(stderr, "%s\n\n", msg);
+ fprintf(stderr, "fatal: %s\n\n", msg);
usage_with_options(usagestr, options);
}
--
2.11.0.341.g202cd3142
^ permalink raw reply related
* Re: [PATCHv3 1/3] merge: Add '--continue' option as a synonym for 'git commit'
From: Jeff King @ 2016-12-14 15:20 UTC (permalink / raw)
To: Chris Packham; +Cc: git, mah, jacob.keller, gitster
In-Reply-To: <20161214083757.26412-1-judge.packham@gmail.com>
On Wed, Dec 14, 2016 at 09:37:55PM +1300, Chris Packham wrote:
> + if (continue_current_merge) {
> + int nargc = 1;
> + const char *nargv[] = {"commit", NULL};
> +
> + if (orig_argc != 2)
> + usage_msg_opt("--continue expects no arguments",
> + builtin_merge_usage, builtin_merge_options);
This message should probably be inside a _() for translation.
I noticed when running it that the output looks funny:
$ git merge --continue foo
--continue expects no arguments
usage: [...]
I was going to suggest adding something like "fatal:" here, but I
actually think it should be the responsibility of usage_msg_opt().
Looking at its other callers, they would all benefit. I posted a
patch:
http://public-inbox.org/git/20161214151009.4wdzjb44f6aki5ug@sigill.intra.peff.net/
I also wondered what it would look like to support "--quiet" on top of
this. I don't care that much about it in particular, but I just want to
make sure we're not painting ourselves into a corner.
Here's what I came up with;
diff --git a/builtin/merge.c b/builtin/merge.c
index 668aaffb8..b13523ce9 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1160,10 +1160,16 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
show_progress = 0;
if (abort_current_merge) {
- int nargc = 2;
- const char *nargv[] = {"reset", "--merge", NULL};
+ int acceptable_arguments = 2; /* argv[0] plus --abort */
+ struct argv_array nargv = ARGV_ARRAY_INIT;
- if (orig_argc != 2)
+ argv_array_pushl(&nargv, "reset", "--merge", NULL);
+ if (verbosity < 0) {
+ acceptable_arguments++;
+ argv_array_push(&nargv, "--quiet");
+ }
+
+ if (orig_argc != acceptable_arguments)
usage_msg_opt("--abort expects no arguments",
builtin_merge_usage, builtin_merge_options);
@@ -1171,15 +1177,22 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
die(_("There is no merge to abort (MERGE_HEAD missing)."));
/* Invoke 'git reset --merge' */
- ret = cmd_reset(nargc, nargv, prefix);
+ ret = cmd_reset(nargv.argc, nargv.argv, prefix);
+ argv_array_clear(&nargv);
goto done;
}
if (continue_current_merge) {
- int nargc = 1;
- const char *nargv[] = {"commit", NULL};
+ int acceptable_arguments = 2; /* argv[0] plus --abort */
+ struct argv_array nargv = ARGV_ARRAY_INIT;
+
+ argv_array_push(&nargv, "commit");
+ if (verbosity < 0) {
+ acceptable_arguments++;
+ argv_array_push(&nargv, "--quiet");
+ }
- if (orig_argc != 2)
+ if (orig_argc != acceptable_arguments)
usage_msg_opt("--continue expects no arguments",
builtin_merge_usage, builtin_merge_options);
@@ -1187,7 +1200,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
die(_("There is no merge in progress (MERGE_HEAD missing)."));
/* Invoke 'git commit' */
- ret = cmd_commit(nargc, nargv, prefix);
+ ret = cmd_commit(nargv.argc, nargv.argv, prefix);
+ argv_array_clear(&nargv);
goto done;
}
So not too bad (and you could probably refactor it to avoid some of the
duplication). Though it does get some obscure cases wrong, like:
git merge --continue --verbose --quiet
I dunno. Maybe I am leading you down a rabbit hole, and we should just
live with silently ignoring useless options. I looked at what
cherry-pick does for this case, and its verify_opt_compatible is
somewhat scary from a maintenance standpoint. It's a whitelist, not a
blacklist, so it's easy to forget options (and it looks like "git
cherry-pick --abort -Sfoo" is missed, for example).
-Peff
^ permalink raw reply related
* Re: [PATCH v9 3/5] http: always warn if libcurl version is too old
From: Jeff King @ 2016-12-14 16:01 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, bburky, gitster, jrnieder
In-Reply-To: <1481679637-133137-4-git-send-email-bmwill@google.com>
On Tue, Dec 13, 2016 at 05:40:35PM -0800, Brandon Williams wrote:
> diff --git a/transport.c b/transport.c
> index e1ba78b..fbd799d 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -700,11 +700,6 @@ void transport_check_allowed(const char *type)
> die("transport '%s' not allowed", type);
> }
>
> -int transport_restrict_protocols(void)
> -{
> - return !!protocol_whitelist();
> -}
> -
This function was subtly broken as of patch 2 of the series. It's
probably not a big deal in the long run, but should the series be
re-ordered to put this one first?
I think the commit message would need adjusted, but it probably should
mention the reasons this is a good idea even _without_ the new config
system. Namely that even when there's no protocol whitelist, newer
versions of curl have all of the other non-http protocols disabled.
I wonder if anybody is actually using a version of curl old enough to
trigger this. If so, they're going to get the warning every time they
fetch via http. We might need to stick it behind an "advice.*" config
option, though I'm inclined to leave it as-is and see if anybody
actually complains.
-Peff
^ permalink raw reply
* Re: [PATCH v9 4/5] http: create function to get curl allowed protocols
From: Jeff King @ 2016-12-14 16:03 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, bburky, gitster, jrnieder
In-Reply-To: <1481679637-133137-5-git-send-email-bmwill@google.com>
On Tue, Dec 13, 2016 at 05:40:36PM -0800, Brandon Williams wrote:
> Move the creation of an allowed protocols whitelist to a helper
> function.
This is "what" but not "why". You can figure it out if you see the next
patch, but it's often nice to make a brief mention, like:
This will be useful when we need to compute the set of allowed
protocols differently for normal and redirect cases.
-Peff
^ permalink raw reply
* Re: [PATCH v9 5/5] transport: add from_user parameter to is_transport_allowed
From: Jeff King @ 2016-12-14 16:40 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, bburky, gitster, jrnieder
In-Reply-To: <1481679637-133137-6-git-send-email-bmwill@google.com>
On Tue, Dec 13, 2016 at 05:40:37PM -0800, Brandon Williams wrote:
> Add the from_user parameter to the 'is_transport_allowed' function.
> This allows callers to query if a transport protocol is allowed, given
> that the caller knows that the protocol is coming from the user (1) or
> not from the user (0) such as redirects in libcurl. If unknown a -1
> should be provided which falls back to reading `GIT_PROTOCOL_FROM_USER`
> to determine if the protocol came from the user.
I think your commit message is upside-down with respect to the purpose
of the patch. The end goal we want is for http to distinguish between
protocol restrictions for redirects versus initial requests. The rest is
an implementation detail. It's definitely still worth discussing that
implementation detail (though I think your in-code comments may be
sufficient), but I don't see the rationale discussed here at all.
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
> http.c | 14 +++++++-------
> transport.c | 8 +++++---
> transport.h | 13 ++++++++++---
> 3 files changed, 22 insertions(+), 13 deletions(-)
I'm trying to think of a way to test this. I guess the case we are
covering here is when a server redirects, but the protocol is only
allowed from the user. So:
diff --git a/t/t5812-proto-disable-http.sh b/t/t5812-proto-disable-http.sh
index 044cc152f..d911afd24 100755
--- a/t/t5812-proto-disable-http.sh
+++ b/t/t5812-proto-disable-http.sh
@@ -30,5 +30,12 @@ test_expect_success 'curl limits redirects' '
test_must_fail git clone "$HTTPD_URL/loop-redir/smart/repo.git"
'
+test_expect_success 'http can be limited to from-user' '
+ git -c protocol.http.allow=user \
+ clone "$HTTPD_URL/smart/repo.git" plain.git &&
+ test_must_fail git -c protocol.http.allow=user \
+ clone "$HTTPD_URL/smart-redir-perm/repo.git" redir.git
+'
+
stop_httpd
test_done
It's an oddball configuration, and you'd probably just set
http.followRedirects=false in practice, but it does correctly check this
case.
> @@ -588,9 +588,9 @@ static CURL *get_curl_handle(void)
> #endif
> #if LIBCURL_VERSION_NUM >= 0x071304
> curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
> - get_curl_allowed_protocols());
> + get_curl_allowed_protocols(0));
> curl_easy_setopt(result, CURLOPT_PROTOCOLS,
> - get_curl_allowed_protocols());
> + get_curl_allowed_protocols(-1));
This covers internal redirects done by libcurl, but not the dumb-walker
http-alternates nonsense. We have to feed the URL from http-alternates
back to curl ourselves, so it uses CURLOPT_PROTOCOLS even though it
should count as "not from the user".
To fix that, I think we'd need something like:
- get_curl_handle() stops setting these options, as it is done only
once when the curl handle is initialized. Instead, the protocol
restrictions should go into get_active_slot(), which is called for
each request. The values set would remain the same, and be the
baseline.
- the http-walker.c code would need to know when it's requesting from
the base URL, and when it's an alternate. I think this would depend
on the position of the "alt" in in the linked list it keeps.
- when requesting from an alternate, http-walker would set
CURLOPT_PROTOCOLS with get_curl_allowed_protocols(0)
I have to admit that it sounds like a fair bit of work for a pretty
obscure case. You'd have to:
1. Turn http.allowRedirects to "true", to allow redirects even for
non-initial contact.
2. Turn one of protocol.{http,https,ftp,ftps}.allow to "user" to
restrict it from being used in a redirect.
I'm tempted to punt on it and just do:
if (http_follow_config == HTTP_FOLLOW_ALWAYS &&
get_curl_allowed_protocols(0) != get_curl_allowed_protocols(-1))
die("user-only protocol restrictions not implemented for http-alternates");
which errs on the safe side. We could even shove that down into the case
where we actually see some alternates, like:
diff --git a/http-walker.c b/http-walker.c
index c2f81cd6a..5bcc850b1 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -160,6 +160,12 @@ static void prefetch(struct walker *walker, unsigned char *sha1)
#endif
}
+static void check_alternates_protocol_restrictions(void)
+{
+ if (get_curl_allowed_protocols(0) != get_curl_allowed_protocol(-1))
+ die("user-only protocol restrictions not implemented for http alternates");
+}
+
static void process_alternates_response(void *callback_data)
{
struct alternates_request *alt_req =
@@ -272,6 +278,7 @@ static void process_alternates_response(void *callback_data)
/* skip "objects\n" at end */
if (okay) {
struct strbuf target = STRBUF_INIT;
+ check_alternates_protocol_restrictions();
strbuf_add(&target, base, serverlen);
strbuf_add(&target, data + i, posn - i - 7);
warning("adding alternate object store: %s",
I find it unlikely that anybody would ever care, but at least we'd do
the safe thing. I dunno. Maybe I am just being lazy.
-Peff
^ permalink raw reply related
* Re: [PATCHv3 1/3] merge: Add '--continue' option as a synonym for 'git commit'
From: Junio C Hamano @ 2016-12-14 17:01 UTC (permalink / raw)
To: Jeff King; +Cc: Chris Packham, git, mah, jacob.keller
In-Reply-To: <20161214152039.swtll7xrmcdwz7bc@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> So not too bad (and you could probably refactor it to avoid some of the
> duplication). Though it does get some obscure cases wrong, like:
>
> git merge --continue --verbose --quiet
>
> I dunno. Maybe I am leading you down a rabbit hole, and we should just
> live with silently ignoring useless options.
I think you need to handle this in parse-options API if you really
wanted to do this correctly.
<xmqq60mn671x.fsf@gitster.mtv.corp.google.com> may serve as a
reasonable outline for building one.
^ permalink raw reply
* Re: [PATCHv2 0/7] Fix and generalize version sort reordering
From: Jeff King @ 2016-12-14 17:08 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy, Leho Kraav,
git
In-Reply-To: <20161208142401.1329-1-szeder.dev@gmail.com>
On Thu, Dec 08, 2016 at 03:23:54PM +0100, SZEDER Gábor wrote:
> > With my patches it looks like this:
> >
> > $ git -c versionsort.prereleasesuffix=-pre \
> > -c versionsort.prereleasesuffix=-prerelease \
> > tag -l --sort=version:refname
> > v1.0.0-prerelease1
> > v1.0.0-pre1
> > v1.0.0-pre2
> > v1.0.0
>
> And when there happen to be more than one matching suffixes starting
> at the same earliest position, then we should pick the longest of
> them. The new patch 6/7 implements this behavior.
The whole approach taken by the suffix code (before your patches) leaves
me with the nagging feeling that the comparison is not always going to
be transitive (i.e., that "a < b && b < c" does not always imply "a <
c"), which is going to cause nonsensical sorting results.
And that may be part of the issue your 6/7 fixes.
I spent some time playing with this the other day, though, and couldn't
come up with a specific example that fails the condition above.
It just seems like the whole thing would conceptually easier if we
pre-parsed the versions into a sequence of elements, then the comparison
between any two elements would just walk that sequence. The benefit
there is that you can implement whatever rules you like for the parsing
(like "prefer longer suffixes to shorter"), but you know the comparison
will always be consistent.
It would also be more efficient, I think (it seems like the sort is
O(nr_tags * lg(nr_tags) * nr_suffixes) due to parsing suffixes in the
comparator). Though that probably doesn't matter much in practice.
I dunno. I think maybe your 6/7 has converged on an equivalent behavior.
And I am certainly not volunteering to re-write it, so if what you have
works, I'm not opposed to it.
-Peff
^ permalink raw reply
* Re: [PATCH] fix pushing to //server/share/dir paths on Windows
From: Jeff King @ 2016-12-14 17:30 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Johannes Schindelin, Git Mailing List
In-Reply-To: <2ff2613c-47da-a780-5d38-93e16cb16328@kdbg.org>
On Tue, Dec 13, 2016 at 10:32:01PM +0100, Johannes Sixt wrote:
> normalize_path_copy() is not prepared to keep the double-slash of a
> //server/share/dir kind of path, but treats it like a regular POSIX
> style path and transforms it to /server/share/dir.
>
> The bug manifests when 'git push //server/share/dir master' is run,
> because tmp_objdir_add_as_alternate() uses the path in normalized
> form when it registers the quarantine object database via
> link_alt_odb_entries(). Needless to say that the directory cannot be
> accessed using the wrongly normalized path.
Thanks for digging this up! I had a feeling that the problem was going
to be in the underlying path code, but I didn't want to just pass the
buck without evidence. :)
> - if (is_dir_sep(*src)) {
> + /*
> + * Handle initial part of absolute path: "/", "C:/", "\\server\share/".
> + */
> + offset = offset_1st_component(src);
> + if (offset) {
> + /* Convert the trailing separator to '/' on Windows. */
> + memcpy(dst, src, offset - 1);
> + dst += offset - 1;
> *dst++ = '/';
Hmm. So this is the "change-of-behavior" bit. Would it be reasonable to
write:
/* Copy initial part of absolute path, converting separators on Windows */
const char *end = src + offset_1st_component(src);
while (src < end) {
char c = *src++;
if (c == '\\')
c = '/';
*dst++ = c;
}
? I'm not sure if it's wrong to convert backslashes in that first
component or not (but certainly we were before). I don't think we'd need
is_dir_sep() in that "if()", because we can leave slashes as-is. But
maybe it would make the code easier to read.
-Peff
^ permalink raw reply
* Re: [PATCHv2 0/7] Fix and generalize version sort reordering
From: Junio C Hamano @ 2016-12-14 17:36 UTC (permalink / raw)
To: Jeff King
Cc: SZEDER Gábor, Nguyễn Thái Ngọc Duy,
Leho Kraav, git
In-Reply-To: <20161214170852.bzh5pyl4bov6rwbt@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Thu, Dec 08, 2016 at 03:23:54PM +0100, SZEDER Gábor wrote:
>
>> > With my patches it looks like this:
>> >
>> > $ git -c versionsort.prereleasesuffix=-pre \
>> > -c versionsort.prereleasesuffix=-prerelease \
>> > tag -l --sort=version:refname
>> > v1.0.0-prerelease1
>> > v1.0.0-pre1
>> > v1.0.0-pre2
>> > v1.0.0
>>
>> And when there happen to be more than one matching suffixes starting
>> at the same earliest position, then we should pick the longest of
>> them. The new patch 6/7 implements this behavior.
>
> The whole approach taken by the suffix code (before your patches) leaves
> me with the nagging feeling that the comparison is not always going to
> be transitive (i.e., that "a < b && b < c" does not always imply "a <
> c"), which is going to cause nonsensical sorting results.
>
> And that may be part of the issue your 6/7 fixes.
>
> I spent some time playing with this the other day, though, and couldn't
> come up with a specific example that fails the condition above.
>
> It just seems like the whole thing would conceptually easier if we
> pre-parsed the versions into a sequence of elements, then the comparison
> between any two elements would just walk that sequence. The benefit
> there is that you can implement whatever rules you like for the parsing
> (like "prefer longer suffixes to shorter"), but you know the comparison
> will always be consistent.
>
> It would also be more efficient, I think (it seems like the sort is
> O(nr_tags * lg(nr_tags) * nr_suffixes) due to parsing suffixes in the
> comparator). Though that probably doesn't matter much in practice.
>
> I dunno. I think maybe your 6/7 has converged on an equivalent behavior.
> And I am certainly not volunteering to re-write it, so if what you have
> works, I'm not opposed to it.
I also had worries about transitiveness but couldn't break it after
trying for some time. I find your pre-parsing suggestion a great
one, not from the point of view of performance, but because I would
imagine that the resulting logic would become a lot easier to
understand.
^ permalink raw reply
* Re: [PATCH v2 4/6] update-unicode.sh: automatically download newer definition files
From: Beat Bolli @ 2016-12-14 17:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
In-Reply-To: <1481671904-1143-5-git-send-email-dev+git@drbeat.li>
On 14.12.16 00:31, Beat Bolli wrote:
> [PATCH v2 4/6] update-unicode.sh: automatically download newer definition files
Dang! And again I'm not capable of putting an underline instead of the
dash...
Junio, would you please reword the subject to
Re: [PATCH v2 4/6] update_unicode.sh: automatically download newer
definition files
Thanks,
Beat
> we should also download them if a newer version exists on the Unicode
> consortium's servers. Option -N of wget does this nicely for us.
>
> Reviewed-by: Torsten Bögershausen <tboegi@web.de>
> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
> ---
> contrib/update-unicode/update_unicode.sh | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/contrib/update-unicode/update_unicode.sh b/contrib/update-unicode/update_unicode.sh
> index 9f1bf31..56871a1 100755
> --- a/contrib/update-unicode/update_unicode.sh
> +++ b/contrib/update-unicode/update_unicode.sh
> @@ -8,12 +8,8 @@
> cd "$(dirname "$0")"
> UNICODEWIDTH_H=$(git rev-parse --show-toplevel)/unicode_width.h
>
> -if ! test -f UnicodeData.txt; then
> - wget http://www.unicode.org/Public/UCD/latest/ucd/UnicodeData.txt
> -fi &&
> -if ! test -f EastAsianWidth.txt; then
> - wget http://www.unicode.org/Public/UCD/latest/ucd/EastAsianWidth.txt
> -fi &&
> +wget -N http://www.unicode.org/Public/UCD/latest/ucd/UnicodeData.txt \
> + http://www.unicode.org/Public/UCD/latest/ucd/EastAsianWidth.txt &&
> if ! test -d uniset; then
> git clone https://github.com/depp/uniset.git &&
> ( cd uniset && git checkout 4b186196dd )
>
^ 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