* [PATCH v3 12/14] i18n: send-email: mark warnings and errors for translation
From: Vasco Almeida @ 2016-10-05 17:21 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 78eb59b..982c6c0 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) {
@@ -1314,7 +1314,7 @@ Message-Id: $message_id
$_ = 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) {
@@ -1340,7 +1340,7 @@ Message-Id: $message_id
} 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') {
@@ -1425,10 +1425,10 @@ Message-Id: $message_id
}
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");
}
}
@@ -1701,7 +1701,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');
@@ -1709,7 +1709,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.7.4
^ permalink raw reply related
* [PATCH v3 13/14] i18n: send-email: mark string with interpolation for translation
From: Vasco Almeida @ 2016-10-05 17:21 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 that are interpolated for
translation.
We call sprintf() before calling die() and in few other circumstances in
order to replace the values on the placeholders.
Signed-off-by: Vasco Almeida <vascomalmeida@sapo.pt>
---
I changed (y|N) to [y|N] around line 1750 to match with the others
questions style.
git-send-email.perl | 90 ++++++++++++++++++++++++++++-------------------------
1 file changed, 48 insertions(+), 42 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 982c6c0..5c01425 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -279,10 +279,13 @@ sub signal_handler {
# tmp files from --compose
if (defined $compose_filename) {
if (-e $compose_filename) {
- print "'$compose_filename' contains an intermediate version of the email you were composing.\n";
+ printf __("'%s' contains an intermediate version ".
+ "of the email you were composing.\n"),
+ $compose_filename;
}
if (-e ($compose_filename . ".final")) {
- print "'$compose_filename.final' contains the composed email.\n"
+ printf __("'%s.final' contains the composed email.\n"),
+ $compose_filename;
}
}
@@ -431,7 +434,7 @@ $smtp_encryption = '' unless (defined $smtp_encryption);
my(%suppress_cc);
if (@suppress_cc) {
foreach my $entry (@suppress_cc) {
- die "Unknown --suppress-cc field: '$entry'\n"
+ die sprintf(__("Unknown --suppress-cc field: '%s'\n"), $entry)
unless $entry =~ /^(?:all|cccmd|cc|author|self|sob|body|bodycc)$/;
$suppress_cc{$entry} = 1;
}
@@ -460,7 +463,7 @@ my $confirm_unconfigured = !defined $confirm;
if ($confirm_unconfigured) {
$confirm = scalar %suppress_cc ? 'compose' : 'auto';
};
-die "Unknown --confirm setting: '$confirm'\n"
+die sprintf(__("Unknown --confirm setting: '%s'\n"), $confirm)
unless $confirm =~ /^(?:auto|cc|compose|always|never)/;
# Debugging, print out the suppressions.
@@ -492,16 +495,16 @@ my %aliases;
sub parse_sendmail_alias {
local $_ = shift;
if (/"/) {
- print STDERR "warning: sendmail alias with quotes is not supported: $_\n";
+ printf STDERR __("warning: sendmail alias with quotes is not supported: %s\n"), $_;
} elsif (/:include:/) {
- print STDERR "warning: `:include:` not supported: $_\n";
+ printf STDERR __("warning: `:include:` not supported: %s\n"), $_;
} elsif (/[\/|]/) {
- print STDERR "warning: `/file` or `|pipe` redirection not supported: $_\n";
+ printf STDERR __("warning: `/file` or `|pipe` redirection not supported: %s\n"), $_;
} elsif (/^(\S+?)\s*:\s*(.+)$/) {
my ($alias, $addr) = ($1, $2);
$aliases{$alias} = [ split_addrs($addr) ];
} else {
- print STDERR "warning: sendmail line is not recognized: $_\n";
+ printf STDERR __("warning: sendmail line is not recognized: %s\n"), $_;
}
}
@@ -582,13 +585,12 @@ sub is_format_patch_arg {
if (defined($format_patch)) {
return $format_patch;
}
- die(<<EOF);
-File '$f' exists but it could also be the range of commits
+ die sprintf(__(
+"File '%s' exists but it could also be the range of commits
to produce patches for. Please disambiguate by...
- * Saying "./$f" if you mean a file; or
- * Giving --format-patch option if you mean a range.
-EOF
+ * Saying \"./%s\" if you mean a file; or
+ * Giving --format-patch option if you mean a range."), $f, $f);
} catch Git::Error::Command with {
# Not a valid revision. Treat it as a filename.
return 0;
@@ -604,7 +606,7 @@ while (defined(my $f = shift @ARGV)) {
@ARGV = ();
} elsif (-d $f and !is_format_patch_arg($f)) {
opendir my $dh, $f
- or die "Failed to opendir $f: $!";
+ or die sprintf(__("Failed to opendir %s: %s"), $f, $!);
push @files, grep { -f $_ } map { catfile($f, $_) }
sort readdir $dh;
@@ -628,7 +630,8 @@ if ($validate) {
foreach my $f (@files) {
unless (-p $f) {
my $error = validate_patch($f);
- $error and die "fatal: $f: $error\nwarning: no patches were sent\n";
+ $error and die sprintf(__("fatal: %s: %s\nwarning: no patches were sent\n"),
+ $f, $error);
}
}
}
@@ -651,7 +654,7 @@ sub get_patch_subject {
return "GIT: $1\n";
}
close $fh;
- die "No subject line in $fn ?";
+ die sprintf(__("No subject line in %s ?"), $fn);
}
if ($compose) {
@@ -661,7 +664,7 @@ if ($compose) {
tempfile(".gitsendemail.msg.XXXXXX", DIR => $repo->repo_path()) :
tempfile(".gitsendemail.msg.XXXXXX", DIR => "."))[1];
open my $c, ">", $compose_filename
- or die "Failed to open for writing $compose_filename: $!";
+ or die sprintf(__("Failed to open for writing %s: %s"), $compose_filename, $!);
my $tpl_sender = $sender || $repoauthor || $repocommitter || '';
@@ -692,10 +695,10 @@ EOT
}
open my $c2, ">", $compose_filename . ".final"
- or die "Failed to open $compose_filename.final : " . $!;
+ or die sprintf(__("Failed to open %s.final : %s"), $compose_filename, $!);
open $c, "<", $compose_filename
- or die "Failed to open $compose_filename : " . $!;
+ or die sprintf(__("Failed to open %s : %s"), $compose_filename, $!);
my $need_8bit_cte = file_has_nonascii($compose_filename);
my $in_body = 0;
@@ -769,7 +772,9 @@ sub ask {
return $resp;
}
if ($confirm_only) {
- my $yesno = $term->readline("Are you sure you want to use <$resp> [y/N]? ");
+ my $yesno = $term->readline(
+ # TRANSLATORS: please keep [y/N] as is.
+ sprintf(__("Are you sure you want to use <%s> [y/N]? "), $resp));
if (defined $yesno && $yesno =~ /y/i) {
return $resp;
}
@@ -811,9 +816,9 @@ if (!defined $auto_8bit_encoding && scalar %broken_encoding) {
if (!$force) {
for my $f (@files) {
if (get_patch_subject($f) =~ /\Q*** SUBJECT HERE ***\E/) {
- die "Refusing to send because the patch\n\t$f\n"
+ die sprintf(__("Refusing to send because the patch\n\t%s\n"
. "has the template subject '*** SUBJECT HERE ***'. "
- . "Pass --force if you really want to send.\n";
+ . "Pass --force if you really want to send.\n"), $f);
}
}
}
@@ -848,7 +853,7 @@ my %EXPANDED_ALIASES;
sub expand_one_alias {
my $alias = shift;
if ($EXPANDED_ALIASES{$alias}) {
- die "fatal: alias '$alias' expands to itself\n";
+ die sprintf(__("fatal: alias '%s' expands to itself\n"), $alias);
}
local $EXPANDED_ALIASES{$alias} = 1;
return $aliases{$alias} ? expand_aliases(@{$aliases{$alias}}) : $alias;
@@ -910,7 +915,7 @@ sub extract_valid_address {
sub extract_valid_address_or_die {
my $address = shift;
$address = extract_valid_address($address);
- die "error: unable to extract a valid address from: $address\n"
+ die sprintf(__("error: unable to extract a valid address from: %s\n"), $address)
if !$address;
return $address;
}
@@ -918,7 +923,7 @@ sub extract_valid_address_or_die {
sub validate_address {
my $address = shift;
while (!extract_valid_address($address)) {
- print STDERR "error: unable to extract a valid address from: $address\n";
+ printf STDERR __("error: unable to extract a valid address from: %s\n"), $address;
# TRANSLATORS: Make sure to include [q] [d] [e] in your
# translation. The program will only accept English input
# at this point.
@@ -1223,7 +1228,7 @@ sub ssl_verify_params {
return (SSL_verify_mode => SSL_VERIFY_PEER(),
SSL_ca_file => $smtp_ssl_cert_path);
} else {
- die "CA path \"$smtp_ssl_cert_path\" does not exist";
+ die sprintf(__("CA path \"%s\" does not exist"), $smtp_ssl_cert_path);
}
}
@@ -1384,14 +1389,14 @@ Message-Id: $message_id
# supported commands
$smtp->hello($smtp_domain);
} else {
- die "Server does not support STARTTLS! ".$smtp->message;
+ die sprintf(__("Server does not support STARTTLS! %s"), $smtp->message);
}
}
}
if (!$smtp) {
- die "Unable to initialize SMTP properly. Check config and use --smtp-debug. ",
- "VALUES: server=$smtp_server ",
+ die __("Unable to initialize SMTP properly. Check config and use --smtp-debug."),
+ " VALUES: server=$smtp_server ",
"encryption=$smtp_encryption ",
"hello=$smtp_domain",
defined $smtp_server_port ? " port=$smtp_server_port" : "";
@@ -1408,10 +1413,10 @@ Message-Id: $message_id
$smtp->datasend("$line") or die $smtp->message;
}
$smtp->dataend() or die $smtp->message;
- $smtp->code =~ /250|200/ or die "Failed to send $subject\n".$smtp->message;
+ $smtp->code =~ /250|200/ or die sprintf(__("Failed to send %s\n"), $subject).$smtp->message;
}
if ($quiet) {
- printf (($dry_run ? "Dry-" : "")."Sent %s\n", $subject);
+ printf($dry_run ? __("Dry-Sent %s\n") : __("Sent %s\n"), $subject);
} else {
print($dry_run ? __("Dry-OK. Log says:\n") : __("OK. Log says:\n"));
if (!file_name_is_absolute($smtp_server)) {
@@ -1441,7 +1446,7 @@ $subject = $initial_subject;
$message_num = 0;
foreach my $t (@files) {
- open my $fh, "<", $t or die "can't open file $t";
+ open my $fh, "<", $t or die sprintf(__("can't open file %s"), $t);
my $author = undef;
my $sauthor = undef;
@@ -1663,18 +1668,18 @@ sub recipients_cmd {
my @addresses = ();
open my $fh, "-|", "$cmd \Q$file\E"
- or die "($prefix) Could not execute '$cmd'";
+ or die sprintf(__("(%s) Could not execute '%s'"), $prefix, $cmd);
while (my $address = <$fh>) {
$address =~ s/^\s*//g;
$address =~ s/\s*$//g;
$address = sanitize_address($address);
next if ($address eq $sender and $suppress_cc{'self'});
push @addresses, $address;
- printf("($prefix) Adding %s: %s from: '%s'\n",
- $what, $address, $cmd) unless $quiet;
+ printf(__("(%s) Adding %s: %s from: '%s'\n"),
+ $prefix, $what, $address, $cmd) unless $quiet;
}
close $fh
- or die "($prefix) failed to close pipe to '$cmd'";
+ or die sprintf(__("(%s) failed to close pipe to '%s'"), $prefix, $cmd);
return @addresses;
}
@@ -1728,10 +1733,10 @@ sub unique_email_list {
sub validate_patch {
my $fn = shift;
open(my $fh, '<', $fn)
- or die "unable to open $fn: $!\n";
+ or die sprintf(__("unable to open %s: %s\n"), $fn, $!);
while (my $line = <$fh>) {
if (length($line) > 998) {
- return "$.: patch contains a line longer than 998 characters";
+ return sprintf(__("%s: patch contains a line longer than 998 characters"), $.);
}
}
return;
@@ -1747,10 +1752,11 @@ sub handle_backup {
(substr($file, 0, $lastlen) eq $last) &&
($suffix = substr($file, $lastlen)) !~ /^[a-z0-9]/i) {
if (defined $known_suffix && $suffix eq $known_suffix) {
- print "Skipping $file with backup suffix '$known_suffix'.\n";
+ printf(__("Skipping %s with backup suffix '%s'.\n"), $file, $known_suffix);
$skip = 1;
} else {
- my $answer = ask("Do you really want to send $file? (y|N): ",
+ # TRANSLATORS: please keep "[y|N]" as is.
+ my $answer = ask(sprintf(__("Do you really want to send %s? [y|N]: "), $file),
valid_re => qr/^(?:y|n)/i,
default => 'n');
$skip = ($answer ne 'y');
@@ -1778,7 +1784,7 @@ sub handle_backup_files {
sub file_has_nonascii {
my $fn = shift;
open(my $fh, '<', $fn)
- or die "unable to open $fn: $!\n";
+ or die sprintf(__("unable to open %s: %s\n"), $fn, $!);
while (my $line = <$fh>) {
return 1 if $line =~ /[^[:ascii:]]/;
}
@@ -1788,7 +1794,7 @@ sub file_has_nonascii {
sub body_or_subject_has_nonascii {
my $fn = shift;
open(my $fh, '<', $fn)
- or die "unable to open $fn: $!\n";
+ or die sprintf(__("unable to open %s: %s\n"), $fn, $!);
while (my $line = <$fh>) {
last if $line =~ /^$/;
return 1 if $line =~ /^Subject.*[^[:ascii:]]/;
--
2.7.4
^ permalink raw reply related
* [PATCH v3 14/14] i18n: difftool: mark warnings for translation
From: Vasco Almeida @ 2016-10-05 17:21 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 a5790d0..8d3632e 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.7.4
^ permalink raw reply related
* Re: [PATCH v9 10/14] pkt-line: add functions to read/write flush terminated packet streams
From: Lars Schneider @ 2016-10-05 17:35 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Ramsay Jones, Jakub Narębski, Johannes Sixt,
Torsten Bögershausen, Jeff King, mlbright
In-Reply-To: <xmqq8tu3ubzl.fsf@gitster.mtv.corp.google.com>
> On 04 Oct 2016, at 21:53, Junio C Hamano <gitster@pobox.com> wrote:
>
> larsxschneider@gmail.com writes:
>
>> From: Lars Schneider <larsxschneider@gmail.com>
>>
>
>> +int write_packetized_from_buf(const char *src_in, size_t len, int fd_out)
>> +{
>> + static char buf[LARGE_PACKET_DATA_MAX];
>> + int err = 0;
>> + size_t bytes_written = 0;
>> + size_t bytes_to_write;
>> +
>> + while (!err) {
>> + if ((len - bytes_written) > sizeof(buf))
>> + bytes_to_write = sizeof(buf);
>> + else
>> + bytes_to_write = len - bytes_written;
>> + if (bytes_to_write == 0)
>> + break;
>> + err = packet_write_gently(fd_out, src_in + bytes_written, bytes_to_write);
>> + bytes_written += bytes_to_write;
>> + }
>> + if (!err)
>> + err = packet_flush_gently(fd_out);
>> + return err;
>> +}
>
> Hmph, what is buf[] used for, other than its sizeof() taken to yield
> a constant LARGE_PACKET_DATA_MAX?
Agreed. This is stupid. I will fix it.
>>
>> + for (;;) {
>> + strbuf_grow(sb_out, LARGE_PACKET_DATA_MAX);
>> + packet_len = packet_read(fd_in, NULL, NULL,
>> + // TODO: explain + 1
>
> No // C99 comment please.
>
> And I agree that the +1 needs to be explained.
Oh. I did not send the very latest version :-(
Is this explanation OK?
+ strbuf_grow(sb_out, LARGE_PACKET_DATA_MAX);
+ packet_len = packet_read(fd_in, NULL, NULL,
+ /* strbuf_grow() above always allocates one extra byte to
+ * store a '\0' at the end of the string. packet_read()
+ * writes a '\0' extra byte at the end, too. Let it know
+ * that there is already room for the extra byte.
+ */
+ sb_out->buf + sb_out->len, LARGE_PACKET_DATA_MAX+1,
+ PACKET_READ_GENTLE_ON_EOF);
Thanks,
Lars
^ permalink raw reply
* Re: [PATCH v2 04/25] sequencer: future-proof remove_sequencer_state()
From: Junio C Hamano @ 2016-10-05 17:41 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <alpine.DEB.2.20.1610051342490.35196@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> I briefly considered consolidating them and using .git/rebase-merge/ as
> state directory also for cherry-pick/revert, but that would cause
> problems: I am surely not the only user who cherry-picks commits manually
> while running interactive rebases.
Good thinking.
^ permalink raw reply
* Re: [PATCH v2 19/25] sequencer: remember do_recursive_merge()'s return value
From: Junio C Hamano @ 2016-10-05 17:43 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <alpine.DEB.2.20.1610051348020.35196@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Actually, come to think of it, I will change the patch, as it is too
> confusing. What I want is to preserve a positive return value in case of
> merge conflicts, and that is exactly what I should do instead of playing
> games with the Boolean OR operator.
That would be good; that was exactly the confusion I felt that led
to my comments.
^ permalink raw reply
* Re: [PATCH 1/6] git-merge: clarify "usage" by adding "-m <msg>"
From: Junio C Hamano @ 2016-10-05 17:46 UTC (permalink / raw)
To: sorganov; +Cc: git
In-Reply-To: <773a11751c91c31a05c967ade902b0c8279aab56.1475678515.git.sorganov@gmail.com>
sorganov@gmail.com writes:
> From: Sergey Organov <sorganov@gmail.com>
>
> "-m <msg>" is one of essential distinctions between obsolete
> invocation form and the recent one. Add it to the "usage" returned by
> 'git merge -h' for more clarity.
>
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
> builtin/merge.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index a8b57c7..0e367ba 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -43,7 +43,7 @@ struct strategy {
> };
>
> static const char * const builtin_merge_usage[] = {
> - N_("git merge [<options>] [<commit>...]"),
> + N_("git merge [<options>] [-m <msg>] [<commit>...]"),
> N_("git merge [<options>] <msg> HEAD <commit>"),
> N_("git merge --abort"),
> NULL
While this is not wrong per-se, as the deprecated form will go away
soon, I hope you do not mind if I had to drop this one from the
series to avoid merge conflicts to 'pu' (I do not know how bad the
conflict would be yet; I am just reviewing in my MUA).
^ permalink raw reply
* Re: [PATCH 2/6] Documentation/git-merge.txt: remove list of options from SYNOPSIS
From: Junio C Hamano @ 2016-10-05 17:47 UTC (permalink / raw)
To: sorganov; +Cc: git
In-Reply-To: <fa4e150ab54f9a01b4b7ca496dfe514d5e106ff6.1475678515.git.sorganov@gmail.com>
sorganov@gmail.com writes:
> From: Sergey Organov <sorganov@gmail.com>
>
> This partial list of option is confusing as it lacks a lot of
> available options. It also clutters the SYNOPSIS making differences
> between forms of invocation less clear.
>
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
> Documentation/git-merge.txt | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> index b758d55..90342eb 100644
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -9,10 +9,7 @@ git-merge - Join two or more development histories together
> SYNOPSIS
> --------
> [verse]
> -'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]
> - [-s <strategy>] [-X <strategy-option>] [-S[<keyid>]]
> - [--[no-]allow-unrelated-histories]
> - [--[no-]rerere-autoupdate] [-m <msg>] [<commit>...]
> +'git merge' [options] [-m <msg>] [<commit>...]
> 'git merge' <msg> HEAD <commit>...
> 'git merge' --abort
Same comment as 1/6; as we'd hopefully be removing the deprecated
form soonish, it would probably make sense to leave only two, i.e.
git merge [options] [<commit>...]
git merge --abort
in synposis.
^ permalink raw reply
* Re: [PATCH 4/6] Documentation/git-merge.txt: improve short description in NAME
From: Junio C Hamano @ 2016-10-05 17:52 UTC (permalink / raw)
To: sorganov; +Cc: git
In-Reply-To: <a33dd3ec3da0dc2dad72ed85edd29ff01f898831.1475678515.git.sorganov@gmail.com>
sorganov@gmail.com writes:
> From: Sergey Organov <sorganov@gmail.com>
>
> Old description not only raised the question of why the tool is called
> git-merge rather than git-join, but "join histories" also sounds like
> very simple operation, something like what "git-merge -s ours" does.
>
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
> Documentation/git-merge.txt | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> index 216d2f4..cc0329d 100644
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -3,7 +3,8 @@ git-merge(1)
>
> NAME
> ----
> -git-merge - Join two or more development histories together
> +
> +git-merge - Merge one or more branches to the current branch
This patch, evaluated by itself, looks like a regression in that it
tries to explain "merge" by using verb "merge", making it fuzzier to
those who do not yet know what a "merge" is. That was why it tried
to explain "merge" as an operation to join histories.
However, the next one, 5/6, resurrects the "join history" in the
description part to help them, so the damage is not so severe when
we take them together.
I haven't formed firm opinion on this patch yet.
^ permalink raw reply
* Re: [PATCH 4/6] Documentation/git-merge.txt: improve short description in NAME
From: Jeff King @ 2016-10-05 17:55 UTC (permalink / raw)
To: sorganov; +Cc: git, gitster
In-Reply-To: <a33dd3ec3da0dc2dad72ed85edd29ff01f898831.1475678515.git.sorganov@gmail.com>
On Wed, Oct 05, 2016 at 05:46:22PM +0300, sorganov@gmail.com wrote:
> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> index 216d2f4..cc0329d 100644
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -3,7 +3,8 @@ git-merge(1)
>
> NAME
> ----
> -git-merge - Join two or more development histories together
> +
> +git-merge - Merge one or more branches to the current branch
I wonder if we should be more clear that you don't have to merge a
branch; you can merge any commit. I do agree that the original was
unnecessarily general. And I think "the current branch" is accurate
(technically it can be to a detached HEAD, but that is pedantry that
doesn't need to make it into the synopsis).
So maybe "Merge one or more commits into the current branch". I guess
that is a bit vague, too. It is really "commit tips" or "lines of
development" that we are merging. Bringing them in of course brings in
many commits, but the "or more" there is meant to hint at multi-parent
merges.
So perhaps "one or more branches", while not completely accurate, is the
best we can do. I dunno.
-Peff
^ permalink raw reply
* Re: [PATCH 5/6] Documentation/git-merge.txt: improve short description in DESCRIPTION
From: Junio C Hamano @ 2016-10-05 18:07 UTC (permalink / raw)
To: sorganov; +Cc: git
In-Reply-To: <e74ae8afc1bfc4cd9161ccaa56d926a89439551e.1475678515.git.sorganov@gmail.com>
sorganov@gmail.com writes:
> From: Sergey Organov <sorganov@gmail.com>
>
> Old description had a few problems:
>
> - sounded as if commits have changes
>
> - stated that changes are taken since some "divergence point"
> that was not defined.
>
> New description rather uses "common ancestor" and "merge base",
> definitions of which are easily discoverable in the rest of GIT
> documentation.
>
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
> Documentation/git-merge.txt | 25 +++++++++++++++----------
> 1 file changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> index cc0329d..351b8fc 100644
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -16,11 +16,16 @@ SYNOPSIS
>
> DESCRIPTION
> -----------
> -Incorporates changes from the named commits (since the time their
> -histories diverged from the current branch) into the current
> -branch. This command is used by 'git pull' to incorporate changes
> -from another repository and can be used by hand to merge changes
> -from one branch into another.
> +
> +Incorporates changes that lead to the named commits into the current
> +branch, and joins corresponding histories. The best common ancestor of
> +named commits and the current branch, called "merge base", is
> +calculated, and then net changes taken from the merge base to
> +the named commits are applied.
> +
> +This command is used by 'git pull' to incorporate changes from another
> +repository, and can be used by hand to merge changes from one branch
> +into another.
Content change together with re-flowing the text makes it more
costly than necessary to review a change like this. Please avoid
doing so in your future patches.
I like what the updated description says very much. I however
wonder if "and can be used by hand..." is still appropriate, or
needs a bit of modernizing. It feels a bit awkward by making it
sound as if 'git merge' is primarily an implementation detail of
'git pull' but it can also be used as the first-class command, which
used to be the case in the old days back when "git pull . other" was
also perfectly good way to merge the 'other' branch from your own
repository, but I think your update is meant to clarify that we no
longer live in that old world ;-)
> @@ -31,11 +36,11 @@ Assume the following history exists and the current branch is
> D---E---F---G master
> ------------
>
> -Then "`git merge topic`" will replay the changes made on the
> -`topic` branch since it diverged from `master` (i.e., `E`) until
> -its current commit (`C`) on top of `master`, and record the result
> -in a new commit along with the names of the two parent commits and
> -a log message from the user describing the changes.
> -Then "`git merge topic`" will replay the changes made on the `topic`
> -branch since it diverged from `master` (i.e., `E`) until its current
> -commit (`C`) on top of `master`, and record the result in a new commit
> -along with the names of the two parent commits and a log message from
> -the user describing the changes.
> +Then "`git merge topic`" will replay the changes made on the `topic`
> +branch since it diverged from `master` (i.e., `E`) until its current
> +commit (`C`) on top of `master`, and record the result in a new commit
> +along with references to the two parent commits and a log message from
> +the user describing the changes.
Content change together with re-flowing the text makes it more
costly than necessary to review a change like this. Please avoid
doing so in your future patches.
I had to re-flow the original you removed to match how you flowed in
the updated one and stare at it for a while to spot that the only
change was to rephrase "the names of the parents" to "references to
the parents". I do not know if the updated phrasing is better. The
"name" in the original was meant to be a short-hand for "object name",
and I would support a change to spell it out to clarify; "reference"
can be a vague word that can mean different things in Git, and when
the word is given without context, most Git people would think that
the word refers to "refs", but that is definitely not what the new
commit records, so...
^ permalink raw reply
* Re: Reference a submodule branch instead of a commit
From: Heiko Voigt @ 2016-10-05 18:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stefan Beller, Jeremy Morton, git@vger.kernel.org
In-Reply-To: <xmqqlgy2rcxq.fsf@gitster.mtv.corp.google.com>
On Wed, Oct 05, 2016 at 09:13:53AM -0700, Junio C Hamano wrote:
> Heiko Voigt <hvoigt@hvoigt.net> writes:
>
> >> It IS a hack, but having this information in .git<something> would
> >> mean that it can be forced to be in machine readable form, unlike a
> >> mention in README. I do not know if the .gitmodules/.gitignore
> >> combination is a sensible thing to use, but it does smell like a
> >> potentially useful hack.
> >
> > IIRC the tree entries are the reference for submodules in the code. We
> > are iterating over the tree entries in many places so that change does
> > not seem so easy to me.
> >
> > But you are right maybe we should stop arguing against this workflow and
> > just let people use it until they find out whats wrong with it ;)
>
> I didn't say that, though. I am fairly firm on _not_ changing what
> the superproject records in its tree for the submodule, i.e. it must
> record the exact commit, not "a branch name", for reproducibility.
I was not talking about changing what the superproject records in its
tree. I was just talking about changing where we look for submodules
(e.g. for updating and such). I.e. in .git* instead of just the tree as
it is at the moment. Thats what I understood from the discussion above.
Sorry that might have been ambiguous.
I agree that there should always be a commit as a reference for a
submodule. But as far as I understand for some projects its to much
overhead to record every change of a submodule but still they want to
use the latest code during development. Those projects might only want
to record the actual commit when they release something. At least thats
what I imagine.
> I am OK if people ignored the unmatch between the recorded commit
> from a submodule and what they had in the submodule directory while
> they developed and tested the superproject commit. After all, it is
> not an error to make a commit while having a local uncommitted
> changes to tracked files, and it is equally valid to have a commit
> checked out in a submodule directory that is different from what
> goes in the superproject commit. But we do show "modified but not
> committed" in the status output. In that light, submodule.*.ignore
> may have been a mistake.
The original intend for submodule.*.ignore was to help people not
showing submodules as dirty when they had untracked files in them. That
was after status learned to look into submodules. 'untracked' to avoid the
performance overhead and 'dirty' for the people that accidentally worked
with dirty submodules. I agree 'all' might have been to much.
For the above workflow what user might actually want is something that
ignores all changes as long as they are part of the remote branch. But I
am just guessing here. My gut feeling is still that most people that
request this feature come from svn. Thats why I asked whether the
options I described provide the behavior that Jeremy wants.
Cheers Heiko
^ permalink raw reply
* Re: [PATCH 0/18] alternate object database cleanups
From: René Scharfe @ 2016-10-05 18:47 UTC (permalink / raw)
To: Jeff King, git
In-Reply-To: <20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net>
Am 03.10.2016 um 22:33 schrieb Jeff King:
> This series is the result of René nerd-sniping me with the claim that we
> could "easily" teach count-objects to print out the list of alternates
> in:
>
> http://public-inbox.org/git/c27dc1a4-3c7a-2866-d9d8-f5d3eb161650@web.de/
1. Send crappy patch
2. ????
3. PROFIT!!!
Sometimes it works. :)
Thank you!
René
^ permalink raw reply
* Re: [PATCH 16/18] count-objects: report alternates via verbose mode
From: René Scharfe @ 2016-10-05 18:47 UTC (permalink / raw)
To: Jeff King, git
In-Reply-To: <20161003203618.m6kxd3b6h74jbmqz@sigill.intra.peff.net>
Am 03.10.2016 um 22:36 schrieb Jeff King:
> There's no way to get the list of alternates that git
> computes internally; our tests only infer it based on which
> objects are available. In addition to testing, knowing this
> list may be helpful for somebody debugging their alternates
> setup.
>
> Let's add it to the "count-objects -v" output. We could give
> it a separate flag, but there's not really any need.
> "count-objects -v" is already a debugging catch-all for the
> object database, its output is easily extensible to new data
> items, and printing the alternates is not expensive (we
> already had to find them to count the objects).
Good idea.
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Documentation/git-count-objects.txt | 5 +++++
> builtin/count-objects.c | 10 ++++++++++
> t/t5613-info-alternate.sh | 10 ++++++++++
> 3 files changed, 25 insertions(+)
>
> diff --git a/Documentation/git-count-objects.txt b/Documentation/git-count-objects.txt
> index 2ff3568..cb9b4d2 100644
> --- a/Documentation/git-count-objects.txt
> +++ b/Documentation/git-count-objects.txt
> @@ -38,6 +38,11 @@ objects nor valid packs
> +
> size-garbage: disk space consumed by garbage files, in KiB (unless -H is
> specified)
> ++
> +alternate: absolute path of alternate object databases; may appear
> +multiple times, one line per path. Note that if the path contains
> +non-printable characters, it may be surrounded by double-quotes and
> +contain C-style backslashed escape sequences.
>
> -H::
> --human-readable::
> diff --git a/builtin/count-objects.c b/builtin/count-objects.c
> index ba92919..a700409 100644
> --- a/builtin/count-objects.c
> +++ b/builtin/count-objects.c
> @@ -8,6 +8,7 @@
> #include "dir.h"
> #include "builtin.h"
> #include "parse-options.h"
> +#include "quote.h"
>
> static unsigned long garbage;
> static off_t size_garbage;
> @@ -73,6 +74,14 @@ static int count_cruft(const char *basename, const char *path, void *data)
> return 0;
> }
>
> +static int print_alternate(struct alternate_object_database *alt, void *data)
> +{
> + printf("alternate: ");
> + quote_c_style(alt->path, NULL, stdout, 0);
> + putchar('\n');
> + return 0;
> +}
Yeah, quoting paths makes sense.
> +
> static char const * const count_objects_usage[] = {
> N_("git count-objects [-v] [-H | --human-readable]"),
> NULL
> @@ -140,6 +149,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
> printf("prune-packable: %lu\n", packed_loose);
> printf("garbage: %lu\n", garbage);
> printf("size-garbage: %s\n", garbage_buf.buf);
> + foreach_alt_odb(print_alternate, NULL);
> strbuf_release(&loose_buf);
> strbuf_release(&pack_buf);
> strbuf_release(&garbage_buf);
> diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
> index b393613..74f6770 100755
> --- a/t/t5613-info-alternate.sh
> +++ b/t/t5613-info-alternate.sh
> @@ -39,6 +39,16 @@ test_expect_success 'preparing third repository' '
> )
> '
>
> +test_expect_success 'count-objects shows the alternates' '
> + cat >expect <<-EOF &&
> + alternate: $(pwd)/B/.git/objects
> + alternate: $(pwd)/A/.git/objects
> + EOF
> + git -C C count-objects -v >actual &&
> + grep ^alternate: actual >actual.alternates &&
> + test_cmp expect actual.alternates
> +'
> +
> # Note: These tests depend on the hard-coded value of 5 as "too deep". We start
> # the depth at 0 and count links, not repositories, so in a chain like:
> #
>
^ permalink raw reply
* Re: [PATCH 07/18] link_alt_odb_entry: handle normalize_path errors
From: René Scharfe @ 2016-10-05 18:47 UTC (permalink / raw)
To: Jeff King, git
In-Reply-To: <20161003203417.izcgwt4yz3yspdnm@sigill.intra.peff.net>
Am 03.10.2016 um 22:34 schrieb Jeff King:
> When we add a new alternate to the list, we try to normalize
> out any redundant "..", etc. However, we do not look at the
> return value of normalize_path_copy(), and will happily
> continue with a path that could not be normalized. Worse,
> the normalizing process is done in-place, so we are left
> with whatever half-finished working state the normalizing
> function was in.
>
> Fortunately, this cannot cause us to read past the end of
> our buffer, as that working state will always leave the
> NUL from the original path in place. And we do tend to
> notice problems when we check is_directory() on the path.
> But you can see the nonsense that we feed to is_directory
> with an entry like:
>
> this/../../is/../../way/../../too/../../deep/../../to/../../resolve
>
> in your objects/info/alternates, which yields:
>
> error: object directory
> /to/e/deep/too/way//ects/this/../../is/../../way/../../too/../../deep/../../to/../../resolve
> does not exist; check .git/objects/info/alternates.
>
> We can easily fix this just by checking the return value.
> But that makes it hard to generate a good error message,
> since we're normalizing in-place and our input value has
> been overwritten by cruft.
>
> Instead, let's provide a strbuf helper that does an in-place
> normalize, but restores the original contents on error. This
> uses a second buffer under the hood, which is slightly less
> efficient, but this is not a performance-critical code path.
Hmm, in-place functions are quite rare in the strbuf collection. It
looks like a good fit for the two callers and makes sense in general,
though.
^ permalink raw reply
* Re: [PATCH 6/6] Documentation/git-merge.txt: get rid of irrelevant references to git-pull
From: Junio C Hamano @ 2016-10-05 18:57 UTC (permalink / raw)
To: sorganov; +Cc: git
In-Reply-To: <b91ef5e97c60a85cce1a13f88a19218fd0f05655.1475678515.git.sorganov@gmail.com>
sorganov@gmail.com writes:
> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> index 351b8fc..ba5fb0a 100644
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -23,10 +23,6 @@ named commits and the current branch, called "merge base", is
> calculated, and then net changes taken from the merge base to
> the named commits are applied.
>
> -This command is used by 'git pull' to incorporate changes from another
> -repository, and can be used by hand to merge changes from one branch
> -into another.
> -
Good.
> @@ -119,18 +115,17 @@ of `git fetch` for merging are merged to the current branch.
> PRE-MERGE CHECKS
> ----------------
>
> -Before applying outside changes, you should get your own work in
> -good shape and committed locally, so it will not be clobbered if
> -there are conflicts. See also linkgit:git-stash[1].
> -'git pull' and 'git merge' will stop without doing anything when
> -local uncommitted changes overlap with files that 'git pull'/'git
> -merge' may need to update.
> +Before applying outside changes, you should get your own work in good
> +shape and committed locally, so it will not be clobbered if there are
> +conflicts. See also linkgit:git-stash[1]. 'git merge' will stop
> +without doing anything when local uncommitted changes overlap with
> +files that 'git merge' may need to update.
>
> -To avoid recording unrelated changes in the merge commit,
> -'git pull' and 'git merge' will also abort if there are any changes
> -registered in the index relative to the `HEAD` commit. (One
> -exception is when the changed index entries are in the state that
> -would result from the merge already.)
> +To avoid recording unrelated changes in the merge commit, 'git merge'
> +will also abort if there are any changes registered in the index
> +relative to the `HEAD` commit. (One exception is when the changed
> +index entries are in the state that would result from the merge
> +already.)
OK, so "git pull and git merge" have been updated to say "git
merge" and there is no other change. Looks good.
Please do not re-flow and change in the same commit, by the way.
> @@ -138,14 +133,15 @@ will exit early with the message "Already up-to-date."
> FAST-FORWARD MERGE
> ------------------
>
> -Often the current branch head is an ancestor of the named commit.
> +Often the current branch head is an ancestor of the named commit. In
> +this case, a new commit is not needed to store the combined history;
> +instead, the `HEAD` (along with the index) is updated to point at the
> +named commit, without creating an extra merge commit.
> +
> This is the most common case especially when invoked from 'git
> pull': you are tracking an upstream repository, you have committed
> no local changes, and now you want to update to a newer upstream
> -revision. In this case, a new commit is not needed to store the
> -combined history; instead, the `HEAD` (along with the index) is
> -updated to point at the named commit, without creating an extra
> -merge commit.
> +revision.
I am not sure if the post-image of this hunk is better than the
original.
^ permalink raw reply
* Re: [PATCH v2 1/5] check_connected: accept an env argument
From: Jakub Narębski @ 2016-10-05 19:01 UTC (permalink / raw)
To: Jeff King, git; +Cc: David Turner
In-Reply-To: <20161003204908.3nlg6xq2whg2senj@sigill.intra.peff.net>
W dniu 03.10.2016 o 22:49, Jeff King pisze:
> diff --git a/connected.h b/connected.h
> index afa48cc..4ca325f 100644
> --- a/connected.h
> +++ b/connected.h
> @@ -33,6 +33,11 @@ struct check_connected_options {
>
> /* If non-zero, show progress as we traverse the objects. */
> int progress;
> +
> + /*
> + * Insert these variables into the environment of the child process.
> + */
> + const char **env;
> };
Just a nitpick, but I wonder why one comment is in single-line form,
and the other uses block-form with a single line.
--
Jakub Narębski, bikeshedding
^ permalink raw reply
* Re: [PATCH 07/18] link_alt_odb_entry: handle normalize_path errors
From: Jeff King @ 2016-10-05 19:04 UTC (permalink / raw)
To: René Scharfe; +Cc: git
In-Reply-To: <40d3920f-2267-f76d-a5e0-6868fb9f9be2@web.de>
On Wed, Oct 05, 2016 at 08:47:29PM +0200, René Scharfe wrote:
> > Instead, let's provide a strbuf helper that does an in-place
> > normalize, but restores the original contents on error. This
> > uses a second buffer under the hood, which is slightly less
> > efficient, but this is not a performance-critical code path.
>
> Hmm, in-place functions are quite rare in the strbuf collection. It looks
> like a good fit for the two callers and makes sense in general, though.
Yeah, I almost wrote "strbuf_add_normalized_path()" instead. But then
the callers end up having to do the allocate-and-swap thing themselves.
And I think we're still set in the future to add that if somebody wants
it (and we can then implement the in-place version in terms of it).
Another alternative is to observe that the strbuf is generally used in
the first place to make the path absolute. So another interface is
perhaps something like:
strbuf_add_path(struct strbuf *sb, const char *path,
const char *relative_base)
{
struct strbuf scratch = STRBUF_INIT;
int ret;
if (is_absolute_path(path))
strbuf_grow(sb, strlen(path));
else {
if (relative_path)
strbuf_addstr(&scratch, path);
else {
if (strbuf_getcwd(&scratch))
return -1;
}
strbuf_addch(&scratch, '/');
strbuf_addstr(&scratch, path);
strbuf_grow(sb, scratch.len);
path = scratch.buf;
}
ret = normalize_path_copy(sb.buf + sb.len, path);
strbuf_release(&scratch);
return ret;
}
I don't think its worth the complexity of interface for the spots in
this series, but maybe there are other places that could use it. I'll
leave that to somebody else to explore if the ywant to.
-Peff
^ permalink raw reply
* Re: [PATCH v9 09/14] pkt-line: add packet_write_gently()
From: Lars Schneider @ 2016-10-05 19:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, ramsay, jnareb, j6t, tboegi, peff, mlbright
In-Reply-To: <xmqqd1jfucwo.fsf@gitster.mtv.corp.google.com>
> On 04 Oct 2016, at 21:33, Junio C Hamano <gitster@pobox.com> wrote:
>
> larsxschneider@gmail.com writes:
>
>> From: Lars Schneider <larsxschneider@gmail.com>
>>
>>
>> +static int packet_write_gently(const int fd_out, const char *buf, size_t size)
>> +{
>> + static char packet_write_buffer[LARGE_PACKET_MAX];
>> + const size_t packet_size = size + 4;
>> +
>> + if (packet_size > sizeof(packet_write_buffer))
>> + return error("packet write failed - data exceeds max packet size");
>
> Hmph, in the previous round, this used to be "is the size larger
> than sizeof(..) - 4?", which avoided integer overflow issue rather
> nicely and more idiomatic. If size is near the size_t's max,
> packet_size may wrap around to become very small, and we won't hit
> this error, will we?
You are right. Would the solution below be acceptable?
I would like to keep the `packet_size` variable as it eases the rest
of the function.
const size_t packet_size = size + 4;
- if (packet_size > sizeof(packet_write_buffer))
+ if (size > sizeof(packet_write_buffer) - 4)
return error("packet write failed - data exceeds max packet size");
Thanks,
Lars
^ permalink raw reply
* Re: [PATCH v2 1/5] check_connected: accept an env argument
From: Jeff King @ 2016-10-05 19:06 UTC (permalink / raw)
To: Jakub Narębski; +Cc: git, David Turner
In-Reply-To: <6ed4670d-6835-a45e-2842-cfe65b0e2981@gmail.com>
On Wed, Oct 05, 2016 at 09:01:57PM +0200, Jakub Narębski wrote:
> > diff --git a/connected.h b/connected.h
> > index afa48cc..4ca325f 100644
> > --- a/connected.h
> > +++ b/connected.h
> > @@ -33,6 +33,11 @@ struct check_connected_options {
> >
> > /* If non-zero, show progress as we traverse the objects. */
> > int progress;
> > +
> > + /*
> > + * Insert these variables into the environment of the child process.
> > + */
> > + const char **env;
> > };
>
> Just a nitpick, but I wonder why one comment is in single-line form,
> and the other uses block-form with a single line.
I think I wrote something longer originally, and then shortened it
before sending.
I don't generally think it matters much for a case like this (if it were
in the middle of code, I think the shorter form is worth doing, but here
it's basically a header for this variable).
-Peff
^ permalink raw reply
* Re: [PATCH v3 4/6] Export also the has_un{staged,committed}_changed() functions
From: Jakub Narębski @ 2016-10-05 19:20 UTC (permalink / raw)
To: Johannes Schindelin, git; +Cc: Junio C Hamano
In-Reply-To: <017586232230ad87dd7cde5801e011cce9255bc0.1475586229.git.johannes.schindelin@gmx.de>
W dniu 04.10.2016 o 15:05, Johannes Schindelin pisze:
> Subject: Export also the has_un{staged,committed}_changed() functions
s/changed/changes/ that is d -> s
Those are has_unstaged_changes() and has_uncommitted_changes().
Though I wonder if "other has_un*_changes() functions" would be
more readable (while shorter), if less specific...
>
> They will be used in the upcoming rebase helper.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
[...]
> -/* The following function expect that the caller took care of reading the index. */
> +/* The following functions expect that the caller took care of reading the index. */
> +int has_unstaged_changes(void);
> +int has_uncommitted_changes(void);
> int require_clean_work_tree(const char *action, const char *hint, int gently);
Nice to see the fix in comment too. Good work!
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH v3 6/6] wt-status: begin error messages with lower-case
From: Jakub Narębski @ 2016-10-05 19:23 UTC (permalink / raw)
To: Johannes Schindelin, git; +Cc: Junio C Hamano
In-Reply-To: <1d2639277473010731ace0af8358bafd3c622a8d.1475586229.git.johannes.schindelin@gmx.de>
W dniu 04.10.2016 o 15:06, Johannes Schindelin pisze:
> The previous code still followed the old git-pull.sh code which did not
> adhere to our new convention.
Good to know why it used its own convention.
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> builtin/pull.c | 2 +-
> wt-status.c | 6 +++---
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index c639167..0bf9802 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -810,7 +810,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>
> if (!autostash)
> require_clean_work_tree(N_("pull with rebase"),
> - "Please commit or stash them.", 1, 0);
> + "please commit or stash them.", 1, 0);
>
Shouldn't those also be marked for translation with N_() or _()?
Best,
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH 13/18] fill_sha1_file: write "boring" characters
From: Junio C Hamano @ 2016-10-05 19:35 UTC (permalink / raw)
To: Jacob Keller; +Cc: Jeff King, Git mailing list, René Scharfe
In-Reply-To: <CA+P7+xrBX684an5EzUUk+_Dtu6Ep_F+nB1JyWDWsZjUANWcFoA@mail.gmail.com>
Jacob Keller <jacob.keller@gmail.com> writes:
>> The cost of fill function having to do the same thing repeatedly is
>> negligible, so I am OK with the result, but for fairness, this was
>> not "make the callers do this extra thing", but was "the caller can
>> prepare these unchanging parts just once, and the fill function that
>> is repeatedly run does not have to."
>
> Sure, but it's a pretty minor optimization and I think the result is
> easier to understand.
Yes; in case it wasn't clear, my comment was merely for fairness to
the original code. I do agree that the end result of this series
makes a very pleasant read.
^ permalink raw reply
* Re: [RFC/PATCH 0/2] place cherry pick line below commit title
From: Jonathan Tan @ 2016-10-05 19:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Christian Couder
In-Reply-To: <xmqqwphouivf.fsf@gitster.mtv.corp.google.com>
On 10/04/2016 10:25 AM, Junio C Hamano wrote:
> So I would say it is perfectly OK if your update works only for
> cases we can clearly define the semantics for. For example, we can
> even start with something simple like:
>
> * A RFC822-header like line, together with any number of whitespace
> indented lines that immediately follow it, will be taken as a
> single logical trailer element (with embedded LF in it if it uses
> the "line folding"). For the purpose of "replace", the entire
> single logical trailer element is replaced.
>
> * A line that begins with "(cherry picked from" and "[" becomes a
> single logical trailer element. No continuation of anything
> fancy.
>
> * A line with any other shape is a garbage line in a trailer
> block. It is kept in its place, but because it does not even
> have <token> part, it will not participate in locating with
> "trailer.where", "trailer.ifexists", etc.
Sounds reasonable to me. Would the "[" be a bit of overspecification,
though, since Git doesn't produce it? Also, identifying it as a garbage
line probably wouldn't change any behavior - in the Linux kernel
examples, it is used to show what happened in between sign-offs, so
there will always be one "Signed-off-by:" at the top. (But I do not
feel strongly about this.)
> A block of lines that appear as the last paragraph in a commit
> message is a trailer block if and only if certain number or
> percentage of lines are non-garbage lines according to the above
> definition.
I think the number should be 1 - that seems like the easiest to explain.
But I'm OK with other suggestions.
> I wonder if we can share a new helper function to do the detection
> (and classification) of a trailer block and parsing the logical
> lines out of a commit log message. The function signature could be
> as simple as taking a single <const char *> (or a strbuf) that holds
> a commit log message, and splitting it out into something like:
>
> struct {
> const char *whole;
> const char *end_of_message_proper;
> struct {
> const char *token;
> const char *contents;
> } *trailer;
> int alloc_trailers, nr_trailers;
> };
>
> where
>
> - whole points at the first byte of the input, i.e. the beginning
> of the commit message buffer.
>
> - end-of-message-proper points at the first byte of the trailer
> block into the buffer at "whole".
>
> - token is a canonical header name for easy comparison for
> interpret-trailers (you can use NULL for garbage lines, and made
> up token like "[bracket]" and "(cherrypick)" that would not clash
> with real tokens like "Signed-off-by").
>
> - contents is the bytes on the logical line, including the header
> part
>
> E.g. an element in trailer[] array may say
>
> {
> .token = "Signed-off-by",
> .contents = "Signed-Off-By: Some Body <some@body.xz>\n",
> }
I get the impression from the rest of your e-mail that no strings are
meant to be copied - is that true? (That sounds like a good idea to me.)
In which case this might be better:
struct {
const char *first_trailer; /* = end_of_message_proper */
struct {
const char *start;
const char *value;
const char *end;
} *trailers;
int trailers_nr, trailers_alloc;
};
start = value for "[", "(cherry picked from" and garbage lines. We also
need end because there is no \0 there (we didn't copy any strings).
The existing code (in trailer.c) uses a linked list to store trailers,
but an array (as written in your e-mail) is probably better for us since
clients would want to access the last element (as also written in your
e-mail).
> With something like that, you can manipulate the "insert at ...",
> "replace", etc. in the trailer[] array and then produce an updated
> commit message fairly easily (i.e. copy out the bytes beginning at
> "whole" up to "end_of_message_proper", then iterate over trailer[]
> array and show their contents field). The codepaths in the core
> part only need to know
>
> - how to check the last item in trailer[] array to see if it ends
> with the same sign-off as they are trying to add.
>
> - how to append one new element to the trailer[] array.
>
> - reproduce an updated commit log message after the above.
I don't think we need trailer block struct -> commit message conversion
- when adding a new trailer or replacing an existing trailer, the client
code can just remember the index and then modify its behavior
accordingly when iterating through all trailers. But this conversion can
be easily added if/when we need it.
> Hmm?
Overall, this seems like a good idea - I'll go ahead and do this if
there are no other objections.
It just occurred to me that there could be some corner cases when the
trailer separator is configured to not include ":" - I'll make sure to
include tests that check those corner cases.
^ permalink raw reply
* Re: [RFC/PATCH 0/2] place cherry pick line below commit title
From: Jonathan Tan @ 2016-10-05 19:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Christian Couder
In-Reply-To: <xmqq37kcufya.fsf@gitster.mtv.corp.google.com>
On 10/04/2016 11:28 AM, Junio C Hamano wrote:
> An addendum. We may also want to be prepared to accept an input
> that has some garbage lines _after_ the trailer block, if we can
> clearly identify them as such. For example, we could change the
> definition of "the last paragraph" as "the block of lines that
> do not have any empty (or blank) line, that appear either at the end
> of the input, or immediately before three-dash lines", to allow
>
> commit title
>
> explanation of the change
>
> Signed-off-by: Some Body <some@body.xz>
> [some other things]
> Acked-by: Some Other Person <some@other.xz>
>
> ---
> additional comment
>
> which (unfortunately) is a rather common pattern for people who plan
> to send the commit over e-mail.
>
> If we add a new field "const char *beginning_of_tail_garbage" next
> to "end_of_message_proper" that points at the blank line before the
> three-dash line in the above example, the parser should be able to
> break such an input into a parsed form, allow the trailer[] array to
> be manipulated and reproduce a commit log message.
How important is this feature? It doesn't seem too difficult to add,
although it does break compatibility (in particular, "--signoff" must
now be documented as "after the last trailer" instead of "at the end of
the commit message").
^ 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