* [PATCH 1/2] git-svn: reduce scope of input record separator change
From: Eric Wong @ 2016-10-14 1:46 UTC (permalink / raw)
To: Mathieu Arnold
Cc: Jakub Narębski, git, Nguyễn Thái Ngọc Duy,
Stefan Beller, Eric Wong
In-Reply-To: <20161014014623.15223-1-e@80x24.org>
Reducing the scope of where we change the record separator ($/)
avoids bugs in calls which rely on the input record separator
further down, such as the 'chomp' usage in command_oneline.
This is necessary for a future change to git-svn, but exists in
Git.pm since it seems useful for gitweb and our other Perl
scripts, too.
Signed-off-by: Eric Wong <e@80x24.org>
---
git-svn.perl | 4 ++--
perl/Git.pm | 16 +++++++++++++++-
perl/Git/SVN/Editor.pm | 12 +++++-------
perl/Git/SVN/Fetcher.pm | 15 +++++----------
4 files changed, 27 insertions(+), 20 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index 4d41d22..6d1a142 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -44,6 +44,7 @@ use Git qw(
command_close_pipe
command_bidi_pipe
command_close_bidi_pipe
+ get_record
);
BEGIN {
@@ -1880,10 +1881,9 @@ sub get_commit_entry {
{
require Encode;
# SVN requires messages to be UTF-8 when entering the repo
- local $/;
open $log_fh, '<', $commit_msg or croak $!;
binmode $log_fh;
- chomp($log_entry{log} = <$log_fh>);
+ chomp($log_entry{log} = get_record($log_fh, undef));
my $enc = Git::config('i18n.commitencoding') || 'UTF-8';
my $msg = $log_entry{log};
diff --git a/perl/Git.pm b/perl/Git.pm
index ce7e4e8..d2c5a8d 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -59,7 +59,7 @@ require Exporter;
command_bidi_pipe command_close_bidi_pipe
version exec_path html_path hash_object git_cmd_try
remote_refs prompt
- get_tz_offset
+ get_tz_offset get_record
credential credential_read credential_write
temp_acquire temp_is_locked temp_release temp_reset temp_path);
@@ -538,6 +538,20 @@ sub get_tz_offset {
return sprintf("%s%02d%02d", $sign, (gmtime(abs($t - $gm)))[2,1]);
}
+=item get_record ( FILEHANDLE, INPUT_RECORD_SEPARATOR )
+
+Read one record from FILEHANDLE delimited by INPUT_RECORD_SEPARATOR,
+removing any trailing INPUT_RECORD_SEPARATOR.
+
+=cut
+
+sub get_record {
+ my ($fh, $rs) = @_;
+ local $/ = $rs;
+ my $rec = <$fh>;
+ chomp $rec if defined $rs;
+ $rec;
+}
=item prompt ( PROMPT , ISPASSWORD )
diff --git a/perl/Git/SVN/Editor.pm b/perl/Git/SVN/Editor.pm
index 4c4199a..0df16ed 100644
--- a/perl/Git/SVN/Editor.pm
+++ b/perl/Git/SVN/Editor.pm
@@ -7,7 +7,9 @@ use SVN::Delta;
use Carp qw/croak/;
use Git qw/command command_oneline command_noisy command_output_pipe
command_input_pipe command_close_pipe
- command_bidi_pipe command_close_bidi_pipe/;
+ command_bidi_pipe command_close_bidi_pipe
+ get_record/;
+
BEGIN {
@ISA = qw(SVN::Delta::Editor);
}
@@ -57,11 +59,9 @@ sub generate_diff {
push @diff_tree, "-l$_rename_limit" if defined $_rename_limit;
push @diff_tree, $tree_a, $tree_b;
my ($diff_fh, $ctx) = command_output_pipe(@diff_tree);
- local $/ = "\0";
my $state = 'meta';
my @mods;
- while (<$diff_fh>) {
- chomp $_; # this gets rid of the trailing "\0"
+ while (defined($_ = get_record($diff_fh, "\0"))) {
if ($state eq 'meta' && /^:(\d{6})\s(\d{6})\s
($::sha1)\s($::sha1)\s
([MTCRAD])\d*$/xo) {
@@ -173,9 +173,7 @@ sub rmdirs {
my ($fh, $ctx) = command_output_pipe(qw/ls-tree --name-only -r -z/,
$self->{tree_b});
- local $/ = "\0";
- while (<$fh>) {
- chomp;
+ while (defined($_ = get_record($fh, "\0"))) {
my @dn = split m#/#, $_;
while (pop @dn) {
delete $rm->{join '/', @dn};
diff --git a/perl/Git/SVN/Fetcher.pm b/perl/Git/SVN/Fetcher.pm
index d8c21ad..64e900a 100644
--- a/perl/Git/SVN/Fetcher.pm
+++ b/perl/Git/SVN/Fetcher.pm
@@ -9,7 +9,8 @@ use Carp qw/croak/;
use File::Basename qw/dirname/;
use Git qw/command command_oneline command_noisy command_output_pipe
command_input_pipe command_close_pipe
- command_bidi_pipe command_close_bidi_pipe/;
+ command_bidi_pipe command_close_bidi_pipe
+ get_record/;
BEGIN {
@ISA = qw(SVN::Delta::Editor);
}
@@ -86,11 +87,9 @@ sub _mark_empty_symlinks {
my $printed_warning;
chomp(my $empty_blob = `git hash-object -t blob --stdin < /dev/null`);
my ($ls, $ctx) = command_output_pipe(qw/ls-tree -r -z/, $cmt);
- local $/ = "\0";
my $pfx = defined($switch_path) ? $switch_path : $git_svn->path;
$pfx .= '/' if length($pfx);
- while (<$ls>) {
- chomp;
+ while (defined($_ = get_record($ls, "\0"))) {
s/\A100644 blob $empty_blob\t//o or next;
unless ($printed_warning) {
print STDERR "Scanning for empty symlinks, ",
@@ -179,9 +178,7 @@ sub delete_entry {
my ($ls, $ctx) = command_output_pipe(qw/ls-tree
-r --name-only -z/,
$tree);
- local $/ = "\0";
- while (<$ls>) {
- chomp;
+ while (defined($_ = get_record($ls, "\0"))) {
my $rmpath = "$gpath/$_";
$self->{gii}->remove($rmpath);
print "\tD\t$rmpath\n" unless $::_q;
@@ -247,9 +244,7 @@ sub add_directory {
my ($ls, $ctx) = command_output_pipe(qw/ls-tree
-r --name-only -z/,
$self->{c});
- local $/ = "\0";
- while (<$ls>) {
- chomp;
+ while (defined($_ = get_record($ls, "\0"))) {
$self->{gii}->remove($_);
print "\tD\t$_\n" unless $::_q;
push @deleted_gpath, $gpath;
--
EW
^ permalink raw reply related
* [PATCH 2/2] git-svn: "git worktree" awareness
From: Eric Wong @ 2016-10-14 1:46 UTC (permalink / raw)
To: Mathieu Arnold
Cc: Jakub Narębski, git, Nguyễn Thái Ngọc Duy,
Stefan Beller, Eric Wong
In-Reply-To: <20161014014623.15223-1-e@80x24.org>
git-svn internals were previously not aware of repository
layout differences for users of the "git worktree" command.
Introduce this awareness by using "git rev-parse --git-path"
instead of relying on outdated uses of GIT_DIR and friends.
Thanks-to: Duy Nguyen <pclouds@gmail.com>
Reported-by: Mathieu Arnold <mat@freebsd.org>
Signed-off-by: Eric Wong <e@80x24.org>
---
git-svn.perl | 9 +++++----
perl/Git/SVN.pm | 24 +++++++++++++++---------
perl/Git/SVN/Migration.pm | 37 ++++++++++++++++++++++---------------
3 files changed, 42 insertions(+), 28 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index 6d1a142..fa42364 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1700,7 +1700,7 @@ sub cmd_gc {
"files will not be compressed.\n";
}
File::Find::find({ wanted => \&gc_directory, no_chdir => 1},
- "$ENV{GIT_DIR}/svn");
+ Git::SVN::svn_dir());
}
########################### utility functions #########################
@@ -1734,7 +1734,7 @@ sub post_fetch_checkout {
return unless verify_ref('HEAD^0');
return if $ENV{GIT_DIR} !~ m#^(?:.*/)?\.git$#;
- my $index = $ENV{GIT_INDEX_FILE} || "$ENV{GIT_DIR}/index";
+ my $index = command_oneline(qw(rev-parse --git-path index));
return if -f $index;
return if command_oneline(qw/rev-parse --is-inside-work-tree/) eq 'false';
@@ -1836,8 +1836,9 @@ sub get_tree_from_treeish {
sub get_commit_entry {
my ($treeish) = shift;
my %log_entry = ( log => '', tree => get_tree_from_treeish($treeish) );
- my $commit_editmsg = "$ENV{GIT_DIR}/COMMIT_EDITMSG";
- my $commit_msg = "$ENV{GIT_DIR}/COMMIT_MSG";
+ my @git_path = qw(rev-parse --git-path);
+ my $commit_editmsg = command_oneline(@git_path, 'COMMIT_EDITMSG');
+ my $commit_msg = command_oneline(@git_path, 'COMMIT_MSG');
open my $log_fh, '>', $commit_editmsg or croak $!;
my $type = command_oneline(qw/cat-file -t/, $treeish);
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 018beb8..499e84b 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -807,10 +807,15 @@ sub get_fetch_range {
(++$min, $max);
}
+sub svn_dir {
+ command_oneline(qw(rev-parse --git-path svn));
+}
+
sub tmp_config {
my (@args) = @_;
- my $old_def_config = "$ENV{GIT_DIR}/svn/config";
- my $config = "$ENV{GIT_DIR}/svn/.metadata";
+ my $svn_dir = svn_dir();
+ my $old_def_config = "$svn_dir/config";
+ my $config = "$svn_dir/.metadata";
if (! -f $config && -f $old_def_config) {
rename $old_def_config, $config or
die "Failed rename $old_def_config => $config: $!\n";
@@ -1671,7 +1676,7 @@ sub tie_for_persistent_memoization {
return if $memoized;
$memoized = 1;
- my $cache_path = "$ENV{GIT_DIR}/svn/.caches/";
+ my $cache_path = svn_dir() . '/.caches/';
mkpath([$cache_path]) unless -d $cache_path;
my %lookup_svn_merge_cache;
@@ -1712,7 +1717,7 @@ sub tie_for_persistent_memoization {
sub clear_memoized_mergeinfo_caches {
die "Only call this method in non-memoized context" if ($memoized);
- my $cache_path = "$ENV{GIT_DIR}/svn/.caches/";
+ my $cache_path = svn_dir() . '/.caches/';
return unless -d $cache_path;
for my $cache_file (("$cache_path/lookup_svn_merge",
@@ -2446,12 +2451,13 @@ sub _new {
"refs/remotes/$prefix$default_ref_id";
}
$_[1] = $repo_id;
- my $dir = "$ENV{GIT_DIR}/svn/$ref_id";
+ my $svn_dir = svn_dir();
+ my $dir = "$svn_dir/$ref_id";
- # Older repos imported by us used $GIT_DIR/svn/foo instead of
- # $GIT_DIR/svn/refs/remotes/foo when tracking refs/remotes/foo
+ # Older repos imported by us used $svn_dir/foo instead of
+ # $svn_dir/refs/remotes/foo when tracking refs/remotes/foo
if ($ref_id =~ m{^refs/remotes/(.+)}) {
- my $old_dir = "$ENV{GIT_DIR}/svn/$1";
+ my $old_dir = "$svn_dir/$1";
if (-d $old_dir && ! -d $dir) {
$dir = $old_dir;
}
@@ -2461,7 +2467,7 @@ sub _new {
mkpath([$dir]);
my $obj = bless {
ref_id => $ref_id, dir => $dir, index => "$dir/index",
- config => "$ENV{GIT_DIR}/svn/config",
+ config => "$svn_dir/config",
map_root => "$dir/.rev_map", repo_id => $repo_id }, $class;
# Ensure it gets canonicalized
diff --git a/perl/Git/SVN/Migration.pm b/perl/Git/SVN/Migration.pm
index cf6ffa7..dc90f6a 100644
--- a/perl/Git/SVN/Migration.pm
+++ b/perl/Git/SVN/Migration.pm
@@ -44,7 +44,9 @@ use Git qw(
command_noisy
command_output_pipe
command_close_pipe
+ command_oneline
);
+use Git::SVN;
sub migrate_from_v0 {
my $git_dir = $ENV{GIT_DIR};
@@ -55,7 +57,9 @@ sub migrate_from_v0 {
chomp;
my ($id, $orig_ref) = ($_, $_);
next unless $id =~ s#^refs/heads/(.+)-HEAD$#$1#;
- next unless -f "$git_dir/$id/info/url";
+ my $info_url = command_oneline(qw(rev-parse --git-path),
+ "$id/info/url");
+ next unless -f $info_url;
my $new_ref = "refs/remotes/$id";
if (::verify_ref("$new_ref^0")) {
print STDERR "W: $orig_ref is probably an old ",
@@ -82,7 +86,7 @@ sub migrate_from_v1 {
my $git_dir = $ENV{GIT_DIR};
my $migrated = 0;
return $migrated unless -d $git_dir;
- my $svn_dir = "$git_dir/svn";
+ my $svn_dir = Git::SVN::svn_dir();
# just in case somebody used 'svn' as their $id at some point...
return $migrated if -d $svn_dir && ! -f "$svn_dir/info/url";
@@ -97,27 +101,28 @@ sub migrate_from_v1 {
my $x = $_;
next unless $x =~ s#^refs/remotes/##;
chomp $x;
- next unless -f "$git_dir/$x/info/url";
- my $u = eval { ::file_to_s("$git_dir/$x/info/url") };
+ my $info_url = command_oneline(qw(rev-parse --git-path),
+ "$x/info/url");
+ next unless -f $info_url;
+ my $u = eval { ::file_to_s($info_url) };
next unless $u;
- my $dn = dirname("$git_dir/svn/$x");
+ my $dn = dirname("$svn_dir/$x");
mkpath([$dn]) unless -d $dn;
if ($x eq 'svn') { # they used 'svn' as GIT_SVN_ID:
- mkpath(["$git_dir/svn/svn"]);
+ mkpath(["$svn_dir/svn"]);
print STDERR " - $git_dir/$x/info => ",
- "$git_dir/svn/$x/info\n";
- rename "$git_dir/$x/info", "$git_dir/svn/$x/info" or
+ "$svn_dir/$x/info\n";
+ rename "$git_dir/$x/info", "$svn_dir/$x/info" or
croak "$!: $x";
# don't worry too much about these, they probably
# don't exist with repos this old (save for index,
# and we can easily regenerate that)
foreach my $f (qw/unhandled.log index .rev_db/) {
- rename "$git_dir/$x/$f", "$git_dir/svn/$x/$f";
+ rename "$git_dir/$x/$f", "$svn_dir/$x/$f";
}
} else {
- print STDERR " - $git_dir/$x => $git_dir/svn/$x\n";
- rename "$git_dir/$x", "$git_dir/svn/$x" or
- croak "$!: $x";
+ print STDERR " - $git_dir/$x => $svn_dir/$x\n";
+ rename "$git_dir/$x", "$svn_dir/$x" or croak "$!: $x";
}
$migrated++;
}
@@ -139,9 +144,10 @@ sub read_old_urls {
push @dir, $_;
}
}
+ my $svn_dir = Git::SVN::svn_dir();
foreach (@dir) {
my $x = $_;
- $x =~ s!^\Q$ENV{GIT_DIR}\E/svn/!!o;
+ $x =~ s!^\Q$svn_dir\E/!!o;
read_old_urls($l_map, $x, $_);
}
}
@@ -150,7 +156,7 @@ sub migrate_from_v2 {
my @cfg = command(qw/config -l/);
return if grep /^svn-remote\..+\.url=/, @cfg;
my %l_map;
- read_old_urls(\%l_map, '', "$ENV{GIT_DIR}/svn");
+ read_old_urls(\%l_map, '', Git::SVN::svn_dir());
my $migrated = 0;
require Git::SVN;
@@ -239,7 +245,8 @@ sub minimize_connections {
}
}
if (@emptied) {
- my $file = $ENV{GIT_CONFIG} || "$ENV{GIT_DIR}/config";
+ my $file = $ENV{GIT_CONFIG} ||
+ command_oneline(qw(rev-parse --git-path config));
print STDERR <<EOF;
The following [svn-remote] sections in your config file ($file) are empty
and can be safely removed:
--
EW
^ permalink raw reply related
* [PATCH 0/2] git-svn: implement "git worktree" awareness
From: Eric Wong @ 2016-10-14 1:46 UTC (permalink / raw)
To: Mathieu Arnold
Cc: Jakub Narębski, git, Nguyễn Thái Ngọc Duy,
Stefan Beller, Eric Wong
In-Reply-To: <20161013205555.GA599@dcvr>
+Cc Jakub since gitweb could probably take advantage of get_record
from the first patch, too. I'm not completely sure about the API
for this, though.
The following changes since commit 3cdd5d19178a54d2e51b5098d43b57571241d0ab:
Sync with maint (2016-10-11 14:55:48 -0700)
are available in the git repository at:
git://bogomips.org/git-svn.git svn-wt
for you to fetch changes up to 112423eb905cf28c9445781a7647ba590d597ab3:
git-svn: "git worktree" awareness (2016-10-14 01:36:12 +0000)
----------------------------------------------------------------
Eric Wong (2):
git-svn: reduce scope of input record separator change
git-svn: "git worktree" awareness
git-svn.perl | 13 +++++++------
perl/Git.pm | 16 +++++++++++++++-
perl/Git/SVN.pm | 24 +++++++++++++++---------
perl/Git/SVN/Editor.pm | 12 +++++-------
perl/Git/SVN/Fetcher.pm | 15 +++++----------
perl/Git/SVN/Migration.pm | 37 ++++++++++++++++++++++---------------
6 files changed, 69 insertions(+), 48 deletions(-)
--
EW
^ permalink raw reply
* Change Default merge strategy options
From: Daniel Lopez @ 2016-10-13 15:57 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: Francisco Carreira
In-Reply-To: <HE1PR0101MB2187D29367CB67F42373647CBDDC0@HE1PR0101MB2187.eurprd01.prod.exchangelabs.com>
Hi,
How to use 'git config --global' to set default strategy like recursive.
Example:
Currently , when we want to enforce a specific strategic we need to include its reference on the command line :
git.exe merge --strategy=recursive --strategy-option=ignore-all-space dev-local
we would like to define it as the default strategic to follow and be able to simplify the command line to:
git.exe merge dev-fix1.3-local
Using alias is not an option as the git is being called from TortoiseGit (our current gui tool).
Daniel Lopez
Concept Developer
Tel: +351 289 100683 | Email: dlopez@csscorporate.com | www.csscorporate.com |
^ permalink raw reply
* Re: [GIT GUI l18n 1/2] git-gui: Mark 'All' in remote.tcl for translation
From: Pat Thoyts @ 2016-10-14 0:46 UTC (permalink / raw)
To: Alexander Shopov; +Cc: git
In-Reply-To: <20161013184349.42950-1-ash@kambanaria.org>
Alexander Shopov <ash@kambanaria.org> writes:
>Signed-off-by: Alexander Shopov <ash@kambanaria.org>
>---
> lib/remote.tcl | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/lib/remote.tcl b/lib/remote.tcl
>index 4e5c784..26af8ae 100644
>--- a/lib/remote.tcl
>+++ b/lib/remote.tcl
>@@ -250,12 +250,12 @@ proc update_all_remotes_menu_entry {} {
>
> $fetch_m insert end separator
> $fetch_m insert end command \
>- -label "All" \
>+ -label [mc "All"] \
> -command fetch_from_all
>
> $prune_m insert end separator
> $prune_m insert end command \
>- -label "All" \
>+ -label [mc "All" ]\
> -command prune_from_all
> }
> } else {
OK - this looks fine except the line just above compares the text of
this entry so also needs [mc] adding. I've applied it as:
-- a/lib/remote.tcl
+++ b/lib/remote.tcl
@@ -246,22 +246,22 @@ proc update_all_remotes_menu_entry {} {
if {$have_remote > 1} {
make_sure_remote_submenues_exist $remote_m
if {[$fetch_m type end] eq "command" \
- && [$fetch_m entrycget end -label] ne "All"} {
+ && [$fetch_m entrycget end -label] ne [mc "All"]} {
$fetch_m insert end separator
$fetch_m insert end command \
- -label "All" \
+ -label [mc "All"] \
-command fetch_from_all
$prune_m insert end separator
$prune_m insert end command \
- -label "All" \
+ -label [mc "All"] \
-command prune_from_all
}
} else {
if {[winfo exists $fetch_m]} {
if {[$fetch_m type end] eq "command" \
- && [$fetch_m entrycget end -label] eq "All"} {
+ && [$fetch_m entrycget end -label] eq [mc "All"]} {
delete_from_menu $fetch_m end
delete_from_menu $fetch_m end
--
Pat Thoyts http://www.patthoyts.tk/
PGP fingerprint 2C 6E 98 07 2C 59 C8 97 10 CE 11 E6 04 E0 B9 DD
^ permalink raw reply
* Re: [PATCHv3] attr: convert to new threadsafe API
From: Stefan Beller @ 2016-10-13 22:08 UTC (permalink / raw)
To: Junio C Hamano
Cc: Brandon Williams, git@vger.kernel.org, Johannes Sixt,
Jacob Keller
In-Reply-To: <CAGZ79kYEefzKJT5TLXaGV0ZYoW=OUzrRBPTOovDG0ofO8-i5Jg@mail.gmail.com>
> On Wed, Oct 12, 2016 at 4:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
> so I am hoping that we won't have to do this uglier variant
---8<--- attr.h:
...
struct git_attr_result {
int check_nr;
/* Whether is was statically allocated and cannot be resized. */
int static_alloc;
const char *value[FLEX_ARRAY];
};
/* create a pointer pointing to a git_attr_result with a given size: */
#define GIT_ATTR_RESULT_INIT_FOR(name, size) \
struct { \
int check_nr; \
int static_alloc; \
const char *value[size]; \
} local_##name; \
struct git_attr_result *name = \
(struct git_attr_result *)&(local_##name); \
local_##name.static_alloc = 1;
...
---8<--- e.g. ws.c:
static struct git_attr_check *attr_whitespace_rule;
GIT_ATTR_RESULT_INIT_FOR(result, 1);
git_attr_check_initl(&attr_whitespace_rule, "whitespace", NULL);
if (!git_check_attr(pathname, attr_whitespace_rule, result)) {
if (ATTR_TRUE(result->value[0])) {
...
} else if (ATTR_FALSE(result->value[0])) {
...
^ permalink raw reply
* Re: problem with git worktree and git svn
From: Eric Wong @ 2016-10-13 20:55 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Mathieu Arnold, Stefan Beller, Git Mailing List
In-Reply-To: <CACsJy8ADmHLW5ba4bVTGN9ccqpMDNFgkqE_5JvnqZ0BxWcaCvw@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> wrote:
> On Thu, Oct 13, 2016 at 8:52 AM, Eric Wong <e@80x24.org> wrote:
> > +sub svn_dir {
> > + my $git_dir = scalar @_ ? $_[0] : $ENV{GIT_DIR};
> > + my $common = $ENV{GIT_COMMON_DIR} || "$git_dir/commondir";
> > + $git_dir .= '/'.::file_to_s($common) if -e $common;
> > + my $svn_dir = $git_dir . '/svn';
> > + $svn_dir =~ tr!/!/!s;
> > + $svn_dir;
> > +}
>
>
> If this is shell script, this function could be just
>
> svn_dir() {
> git rev-parse --git-path svn
> }
Ah, thanks; I missed --git-path. I will do this in Git/SVN.pm:
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -808,12 +808,7 @@ sub get_fetch_range {
}
sub svn_dir {
- my $git_dir = scalar @_ ? $_[0] : $ENV{GIT_DIR};
- my $common = $ENV{GIT_COMMON_DIR} || "$git_dir/commondir";
- $git_dir .= '/'.::file_to_s($common) if -e $common;
- my $svn_dir = $git_dir . '/svn';
- $svn_dir =~ tr!/!/!s;
- $svn_dir;
+ command_oneline(qw(rev-parse --git-path svn));
}
sub tmp_config {
> which should give you correct path in either single or multi-worktree
> context and you don't need to bother with details like
> $GIT_COMMON_DIR. But I don't know how Perl bindings are implemented, I
> don't know if we have something similar (or easy to add it, like
> Git::git_path()).
I'm not sure it's necessary given the convenience of command_oneline,
and I'd rather avoid the overhead of documenting+supporting a new API
for Git.pm
> I don't know much about git-svn, but from the look of it I agree
> replacing $ENV{GIT_DIR}/svn with svn_dir() should fix it, assuming
> that you don't hard code $ENV{GIT_DIR}/blahblah somewhere else. I
> don't see any other potential problems (from multi-worktree
> perspective).
I think there was a place where $GIT_DIR/config was used, but
only for documentation purposes.
^ permalink raw reply
* Re: Huge performance bottleneck reading packs
From: Jeff King @ 2016-10-13 20:43 UTC (permalink / raw)
To: Vegard Nossum
Cc: git, Quentin Casasnovas, Shawn Pearce,
Nguyễn Thái Ngọc Duy, Junio C Hamano
In-Reply-To: <d727ee5d-5f0f-e6df-3f83-35fe74641ace@oracle.com>
On Thu, Oct 13, 2016 at 08:18:11PM +0200, Vegard Nossum wrote:
> > My guess is that the number is relatively high. And that would explain
> > why nobody else has really complained much; such a pattern is probably
> > uncommon.
>
> I get ~3,700 objects "they are advertising that we don't have".
>
> They are all indeed tags which I don't have (nor want) locally.
Thanks. That's about what I expected.
> So without your patch I get these numbers:
>
> % cumulative self self total
> time seconds seconds calls s/call s/call name
> 37.34 29.83 29.83 1948265116 0.00 0.00 strip_suffix_mem
> 27.56 51.85 22.02 11358 0.00 0.01 prepare_packed_git_one
> 14.24 63.23 11.38 1924457702 0.00 0.00 strip_suffix
> 4.88 67.13 3.90 26045319 0.00 0.00 find_pack_entry_one
> 4.17 70.46 3.33 84828149 0.00 0.00 hashcmp
> 3.28 73.08 2.62 79760199 0.00 0.00 hashcmp
> 2.44 75.03 1.95 const_error
>
> the call graph data shows all the reprepare_packed_git() calls to come
> from a chain like this:
>
> do_for_each_ref
> do_for_each_ref_iterator
> ref_iterator_advance
> files_ref_iterator_advance
> ref_resolves_to_object
> has_sha1_file
> has_sha1_file_with_flags
> reprepare_packed_git
Hrm. It seems weird that we'd hit reprepare_packed_git() via
do_for_each_ref(), because that's looking at _your_ refs. So any time
that code path hits reprepare_packed_git(), it should be complaining
about repository corruption to stderr.
And that also wouldn't make sense that my patch would improve it. Are
you sure you've read the perf output correctly (I'll admit that I am
often confused by the way it reports call graphs)?
> With your (first) patch I get this instead:
>
> % cumulative self self total
> time seconds seconds calls ms/call ms/call name
> 29.41 0.25 0.25 4490399 0.00 0.00 hashcmp
> 16.47 0.39 0.14 843447 0.00 0.00 find_pack_entry_one
These functions appearing at the top are probably a sign that you have
too many packs (these are just object lookup, which has to linearly try
each pack).
So I'd expect things to improve after a git-gc (and especially if you
turn off the packSizeLimit).
> Do you have all the profile data you were interested in before I try a
> 'git gc'?
Yes, I think so. At least I'm satisfied that there's not another
HAS_SHA1_QUICK case that I'm missing.
> I really appreciate the quick response and the work you put into fixing
> this, even when it's my fault for disabling gc, thanks!
No problem. I do think you'll benefit a lot from packing everything into
a single pack, but it's also clear that Git was doing more work than it
needed to be. This was a known issue when we added the racy-check to
has_sha1_file(), and knew that we might have to field reports like this
one.
-Peff
^ permalink raw reply
* Re: [PATCH v3 25/25] sequencer: mark all error messages for translation
From: Johannes Sixt @ 2016-10-13 20:35 UTC (permalink / raw)
To: Johannes Schindelin, Junio C Hamano; +Cc: git, Jakub Narębski
In-Reply-To: <alpine.DEB.2.20.1610131518220.197091@virtualbox>
Am 13.10.2016 um 16:56 schrieb Johannes Schindelin:
> On Wed, 12 Oct 2016, Junio C Hamano wrote:
>> You have at least two independent changes relative to Dscho's
>> version.
>>
>> (1) Show line breaks more prominently by avoiding "\n\n" and
>> breaking the string at "\n"; this matches the way how the
>> result would be displayed more closely to how the source looks
>> like.
>>
>> (2) Ignore the usual indentation rule and have messages start at
>> the left end of the source.
>>
>> Which one are you saying "makes sense" to? Both?
>>
>> I guess both can be grouped together into one theme: match the way
>> the final output and the source code look like.
Yes, that summarizes it pretty well.
> I agree with that motivation, but I decided to go about it in a way that
> is more in line with the existing source code:
>
> -- snipsnap --
> diff --git a/sequencer.c b/sequencer.c
> index 8e10bb5..1cf70f7 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -483,6 +483,20 @@ static char **read_author_script(void)
> return env;
> }
>
> +static const char staged_changes_advice[] =
> +N_("you have staged changes in your working tree\n"
> +"If these changes are meant to be squashed into the previous commit, run:\n"
> +"\n"
> +" git commit --amend %s\n"
> +"\n"
> +"If they are meant to go into a new commit, run:\n"
> +"\n"
> +" git commit %s\n"
> +"\n"
> +"In both cases, once you're done, continue with:\n"
> +"\n"
> +" git rebase --continue\n");
Much better! Thank you.
-- Hannes
^ permalink raw reply
* Re: [PATCH] fetch: use "quick" has_sha1_file for tag following
From: Jeff King @ 2016-10-13 20:06 UTC (permalink / raw)
To: Vegard Nossum
Cc: git, Quentin Casasnovas, Shawn Pearce,
Nguyễn Thái Ngọc Duy, Junio C Hamano
In-Reply-To: <20161013170443.43slna3zvcvrse5r@sigill.intra.peff.net>
On Thu, Oct 13, 2016 at 01:04:43PM -0400, Jeff King wrote:
> > This patch teaches fetch to use HAS_SHA1_QUICK to sacrifice
> > accuracy for speed, in cases where we might be racy with a
> > simultaneous repack. This is similar to the fix in 0eeb077
> > (index-pack: avoid excessive re-reading of pack directory,
> > 2015-06-09). As with that case, it's OK for has_sha1_file()
> > occasionally say "no I don't have it" when we do, because
> > the worst case is not a corruption, but simply that we may
> > fail to auto-follow a tag that points to it.
>
> Failing in this direction doesn't make me feel great. I had hoped it
> would fail the _other_ way, and request an object that we might already
> have.
>
> There are two alternatives that might be worth pursuing:
>
> 1. The thing that makes this really painful is the quadratic
> duplicate-search in prepare_packed_git_one(). We could speed that
> up pretty easily by keeping a hash of the packs indexed by their
> names, and looking up in that.
>
> That drops the quadratic factor, but it's still going to be
> O(nr_tags * nr_packs), as we have to at the very least readdir()
> all of objects/pack to see that nothing new showed up.
That hash patch would look something like what is below.
Here are numbers for the perf script ("quick" is the one that skips
reprepare entirely, "hash" is this hash table patch, and "quick+hash" is
the combination of the two):
origin quick hash quick+hash
--------------------------------------------------------------------------------------------
11.09(10.38+0.70) 0.08(0.06+0.01) -99.3% 1.41(0.65+0.75) -87.3% 0.07(0.05+0.01) -99.4%
So yes, it does improve this case, but not nearly as much as the "quick"
approach. Putting it on top of the "quick" approach is barely noticeable
(it is speeding up the initial prepare_packed_git() call, but it's just
not expensive enough in the first place to be worth it).
The "hash" patch does fix a potentially quadratic behavior, though, so I
guess in theory it is worth it. But I had to go up to quite a large
number of packs to make it worthwhile. Here it is at 7500 packs, running
"git cat-file -e $some_object_that_exists":
[origin]
real 0m0.417s
user 0m0.376s
sys 0m0.040s
[hash]
real 0m0.054s
user 0m0.008s
sys 0m0.044s
So it's certainly better. But 7500 packs is just silly, and squeezing
out ~400ms there is hardly worth it. If you repack this same case into a
single pack, the command drops to 5ms. So yes, there's close to an order
of magnitude speedup here, but you get that _and_ another order of
magnitude just by repacking.
So I think it's only worth pursuing if we wanted to scrap my original
patch, and continue to aggressively reprepare_packed_git(). I'd worry
that it's still expensive on some systems, even with the hash, because
the opendir/readdir might cost a lot (OTOH, we'll by definition have
just done a stat() on the loose version of the object, so there's a
limit to how fast we can make it).
I dunno.
---
cache.h | 2 ++
sha1_file.c | 44 +++++++++++++++++++++++++++++++++++---------
2 files changed, 37 insertions(+), 9 deletions(-)
diff --git a/cache.h b/cache.h
index ec791a6..0d8b4e8 100644
--- a/cache.h
+++ b/cache.h
@@ -1411,6 +1411,7 @@ struct pack_window {
};
extern struct packed_git {
+ struct hashmap_entry name_entry;
struct packed_git *next;
struct pack_window *windows;
off_t pack_size;
@@ -1428,6 +1429,7 @@ extern struct packed_git {
do_not_close:1;
unsigned char sha1[20];
struct revindex_entry *revindex;
+ size_t basename_len;
/* something like ".git/objects/pack/xxxxx.pack" */
char pack_name[FLEX_ARRAY]; /* more */
} *packed_git;
diff --git a/sha1_file.c b/sha1_file.c
index c652cb6..eb6a5f3 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -602,6 +602,8 @@ struct packed_git *packed_git;
static struct mru packed_git_mru_storage;
struct mru *packed_git_mru = &packed_git_mru_storage;
+static struct hashmap pack_name_index;
+
void pack_report(void)
{
fprintf(stderr,
@@ -1260,6 +1262,30 @@ struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path)
return p;
}
+static int pack_name_hashcmp(const void *va, const void *vb, const void *name)
+{
+ const struct packed_git *a = va, *b = vb;
+
+ if (a->basename_len != b->basename_len)
+ return -1;
+ else if (name)
+ return memcmp(a->pack_name, name, a->basename_len);
+ else
+ return memcmp(a->pack_name, b->pack_name, a->basename_len);
+}
+
+static int pack_loaded(const char *name, size_t len)
+{
+ struct packed_git key;
+
+ if (!pack_name_index.tablesize)
+ return 0;
+
+ hashmap_entry_init(&key, memhash(name, len));
+ key.basename_len = len;
+ return !!hashmap_get(&pack_name_index, &key, name);
+}
+
void install_packed_git(struct packed_git *pack)
{
if (pack->pack_fd != -1)
@@ -1267,6 +1293,12 @@ void install_packed_git(struct packed_git *pack)
pack->next = packed_git;
packed_git = pack;
+
+ if (!pack_name_index.tablesize)
+ hashmap_init(&pack_name_index, pack_name_hashcmp, 0);
+ strip_suffix(pack->pack_name, ".pack", &pack->basename_len);
+ hashmap_entry_init(pack, memhash(pack->pack_name, pack->basename_len));
+ hashmap_add(&pack_name_index, pack);
}
void (*report_garbage)(unsigned seen_bits, const char *path);
@@ -1347,15 +1379,9 @@ static void prepare_packed_git_one(char *objdir, int local)
base_len = path.len;
if (strip_suffix_mem(path.buf, &base_len, ".idx")) {
- /* Don't reopen a pack we already have. */
- for (p = packed_git; p; p = p->next) {
- size_t len;
- if (strip_suffix(p->pack_name, ".pack", &len) &&
- len == base_len &&
- !memcmp(p->pack_name, path.buf, len))
- break;
- }
- if (p == NULL &&
+ if (
+ /* Don't reopen a pack we already have. */
+ !pack_loaded(path.buf, base_len) &&
/*
* See if it really is a valid .idx file with
* corresponding .pack file that we can map.
--
2.10.1.589.gca16299
^ permalink raw reply related
* Re: Uninitialized submodules as symlinks
From: Kevin Daudt @ 2016-10-13 19:35 UTC (permalink / raw)
To: Heiko Voigt; +Cc: David Turner, git@vger.kernel.org
In-Reply-To: <20161013161017.GC29710@book.hvoigt.net>
On Thu, Oct 13, 2016 at 06:10:17PM +0200, Heiko Voigt wrote:
> On Fri, Oct 07, 2016 at 06:17:05PM +0000, David Turner wrote:
> > Presently, uninitialized submodules are materialized in the working
> > tree as empty directories. We would like to consider having them be
> > symlinks. Specifically, we'd like them to be symlinks into a FUSE
> > filesystem which retrieves files on demand.
>
> How about portability? This feature would only work on Unix like
> operating systems. You have to be careful to not break Windows since
> they do not have symlinks.
>
NTFS does have symlinks, but you need admin right to create them though
(unless you change the policy).
^ permalink raw reply
* Re: [PATCHv3] attr: convert to new threadsafe API
From: Stefan Beller @ 2016-10-13 18:42 UTC (permalink / raw)
To: Junio C Hamano
Cc: Brandon Williams, git@vger.kernel.org, Johannes Sixt,
Jacob Keller
In-Reply-To: <xmqqfuo116t0.fsf@gitster.mtv.corp.google.com>
On Wed, Oct 12, 2016 at 4:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> @@ -89,15 +114,20 @@ static void setup_check(void)
>>
>> ------------
>> const char *path;
>> + struct git_attr_result *result;
>>
>> setup_check();
>> - git_check_attr(path, check);
>> + result = git_check_attr(path, check);
>
> This looks stale by a few revisions of the other parts of the patch?
I'll update the documentation.
>
>> diff --git a/archive.c b/archive.c
>> index 11e3951..15849a8 100644
>> --- a/archive.c
>> +++ b/archive.c
>> @@ -107,10 +107,12 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
>> void *context)
>> {
>> static struct strbuf path = STRBUF_INIT;
>> + static struct git_attr_check *check;
>> +
>> struct archiver_context *c = context;
>> struct archiver_args *args = c->args;
>> write_archive_entry_fn_t write_entry = c->write_entry;
>> - static struct git_attr_check *check;
>> + struct git_attr_result result = GIT_ATTR_RESULT_INIT;
>> const char *path_without_prefix;
>> int err;
>>
>> @@ -124,12 +126,16 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
>> strbuf_addch(&path, '/');
>> path_without_prefix = path.buf + args->baselen;
>>
>> - if (!check)
>> - check = git_attr_check_initl("export-ignore", "export-subst", NULL);
>> - if (!git_check_attr(path_without_prefix, check)) {
>> - if (ATTR_TRUE(check->check[0].value))
>> + git_attr_check_initl(&check, "export-ignore", "export-subst", NULL);
>> + git_attr_result_init(&result, check);
>> +
>> + if (!git_check_attr(path_without_prefix, check, &result)) {
>> + if (ATTR_TRUE(result.value[0])) {
>> + git_attr_result_clear(&result);
>> return 0;
>> - args->convert = ATTR_TRUE(check->check[1].value);
>> + }
>> + args->convert = ATTR_TRUE(result.value[1]);
>> + git_attr_result_clear(&result);
>> }
>
> This is exactly what I meant by "can we avoid alloc/free of result
> in leaf function when we _know_ how many attributes we are
> interested in already, which is the majority of the case?".
We can avoid that. For now I settled with an implementation that
has no "answer" type, but uses a bare "const char *result[FLEX_ARRAY];",
or rather a const char **.
>
> Starting with a simple but unoptimized internal implementation of
> the attr subsystem is one thing (which is good). Exposing an API that
> cannot be optimally implemented later without changing the callers
> is another (which is bad).
>
> By encapsulating each element into "struct git_attr_result", we can
> extend the API without changing the API user [*1*].
Oh I see.
So instead of a raw string we want to have
struct git_attr_result {
const char *value;
};
just to have it extensible. Makes sense. I'll redo that.
>
> But I do not think of a way to allow an efficient implementation
> later unless the source of the API user somehow says "this user is
> only interested in this many attributes", like having this
>
> struct git_attr_result result[2];
const char *result[2] = {NULL, NULL};
as of now would be
struct git_attr_result result[2];
but we'd lose the ability to set them to NULL easily. Probably not needed.
>
> (because this caller only wants "ignore" and "subst") on the API
> user's side [*2*]. Without such a clue (like the patch above, that
> only says "there is a structure called 'result'"), I do not think of
> a way to avoid dynamic allocation on the API implementation side.
>
> All the other callers in the patch (pack-objects, convert, ll-merge,
> etc.) seem to share the exact same pattern. Each of the leaf
> functions knows a fixed set of attributes it is interested in, the
> caller iterates over many paths and makes calls to these leaf
> functions, and it is a waste to pay alloc/free overhead for each and
> every iteration when we know how many elements result needs to
> store.
>
Right.
>
> [Footnote]
>
> *1* Would we need a wrapping struct around the array of results? If
> that is the case, we may need something ugly like this on the
> API user side:
>
> GIT_ATTR_RESULT_TYPE(2) result = {2,};
>
> with something like the following on the API implementation
> side:
>
> #define GIT_ATTR_RESULT_TYPE(n) \
> struct { \
> int num_slots; \
> const char *value[n]; \
> }
>
> struct git_attr_result {
> int num_slots;
> const char *value[FLEX_ARRAY];
> };
> git_attr_result_init(void *result_, struct git_attr_check *check)
> {
> struct git_attr_result *result = result_;
>
> assert(result->num_slots, check->num_attrs);
> ...
> }
> git_check_attr(const char *path,
> struct git_attr_check *check,
> void *result_)
> {
> struct git_attr_result *result = result_;
>
> assert(result->num_slots, check->num_attrs);
> for (i = 0; i < check_num_attrs; i++)
> result->value[i] = ... found value ...;
> }
>
>
> *2* Or the uglier
>
> GIT_ATTR_RESULT_TYPE(2) result = {2,};
>
> I can see why the "check" side would benefit from a structure
> that contains an array, but I do not see why "result" side would
> want to, so I am hoping that we won't have to do this uglier
> variant and just go with the simple "array of resulting values".
So I currently have the "array of resulting values", but not wrapped.
Do we expect to get more than the values out of the attr system?
^ permalink raw reply
* [GIT GUI l18n 1/2] git-gui: Mark 'All' in remote.tcl for translation
From: Alexander Shopov @ 2016-10-13 18:43 UTC (permalink / raw)
To: git, patthoyts; +Cc: Alexander Shopov
Signed-off-by: Alexander Shopov <ash@kambanaria.org>
---
lib/remote.tcl | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/remote.tcl b/lib/remote.tcl
index 4e5c784..26af8ae 100644
--- a/lib/remote.tcl
+++ b/lib/remote.tcl
@@ -250,12 +250,12 @@ proc update_all_remotes_menu_entry {} {
$fetch_m insert end separator
$fetch_m insert end command \
- -label "All" \
+ -label [mc "All"] \
-command fetch_from_all
$prune_m insert end separator
$prune_m insert end command \
- -label "All" \
+ -label [mc "All" ]\
-command prune_from_all
}
} else {
--
2.10.1
^ permalink raw reply related
* Re: Huge performance bottleneck reading packs
From: Vegard Nossum @ 2016-10-13 18:18 UTC (permalink / raw)
To: Jeff King
Cc: git, Quentin Casasnovas, Shawn Pearce,
Nguyễn Thái Ngọc Duy, Junio C Hamano
In-Reply-To: <20161013152632.ynsabbv3yrthgidy@sigill.intra.peff.net>
On 10/13/2016 05:26 PM, Jeff King wrote:
> On Thu, Oct 13, 2016 at 09:20:07AM +0200, Vegard Nossum wrote:
>
>>> Does the patch below help?
>>
>> Yes, ~2m10s -> ~1m25s when I test a git fetch this morning (the other
>> variation in time may be due to different CPU usage by other programs,
>> but I ran with/without the patch multiple times and the difference is
>> consistent).
>> [...]
>> There are some 20k refs on the remote, closer to 25k locally.
>
> OK, that makes some sense. For whatever reason, your remote has a bunch
> of tags that point to objects you do not already have. That could
> happen, I think, if the remote added a lot of tags since you cloned
> (because clone will grab all of the tags), but those tags do not point
> to history that you are otherwise fetching (since fetch by default will
> "auto-follow" such tags).
>
> It might be interesting to see the results of:
>
> # all the objects we have
> git cat-file --batch-all-objects --batch-check='%(objectname)' >us
>
> # all the objects the remote is advertising
> git ls-remote origin | cut -f1 | sort -u >them
>
> # what they are advertising that we don't have
> comm -13 us them | wc -l
>
> My guess is that the number is relatively high. And that would explain
> why nobody else has really complained much; such a pattern is probably
> uncommon.
I get ~3,700 objects "they are advertising that we don't have".
They are all indeed tags which I don't have (nor want) locally.
>> The fetch doesn't actually get anything from the remote as everything is
>> already up to date (that makes the 2m40s times even more frustrating in
>> a way :-)). Here's count-objects:
>
> If the fetch is largely a noop, then that makes me wonder why we are
> spending even a minute in the "good" case. I wonder if there is some
It was user time I reported and this is over network, so some of it
might be IO wait on the network? With the (first) patch:
$ time git fetch
real 1m9.248s
user 0m40.808s
sys 0m2.996s
Also see the profile data below.
> other spot that is wasting CPU on some inefficient data structure
> related to the number of refs you have. If you can do any profiling that
> points to a hot spot, that would be interesting to see (and also whether
> a gc improves things).
>
> I see in find_non_local_tags() that we build up a sorted string_list via
> repeated calls to string_list_insert(), which will keep the thing sorted
> at each stage. That's not as efficient as just sorting at the end, but I
> think it's probably OK in practice because we actually feed it via
> for_each_ref(), whose output is sorted, and so we'd always just be
> appending to the end.
So without your patch I get these numbers:
% cumulative self self total
time seconds seconds calls s/call s/call name
37.34 29.83 29.83 1948265116 0.00 0.00 strip_suffix_mem
27.56 51.85 22.02 11358 0.00 0.01
prepare_packed_git_one
14.24 63.23 11.38 1924457702 0.00 0.00 strip_suffix
4.88 67.13 3.90 26045319 0.00 0.00 find_pack_entry_one
4.17 70.46 3.33 84828149 0.00 0.00 hashcmp
3.28 73.08 2.62 79760199 0.00 0.00 hashcmp
2.44 75.03 1.95 const_error
the call graph data shows all the reprepare_packed_git() calls to come
from a chain like this:
do_for_each_ref
do_for_each_ref_iterator
ref_iterator_advance
files_ref_iterator_advance
ref_resolves_to_object
has_sha1_file
has_sha1_file_with_flags
reprepare_packed_git
the do_for_each_ref() calls come from prepare_replace_object(),
do_fetch(), find_common(), everything_local(),
check_for_new_submodule_commits(), and find_non_local_tags().
With your (first) patch I get this instead:
% cumulative self self total
time seconds seconds calls ms/call ms/call name
29.41 0.25 0.25 4490399 0.00 0.00 hashcmp
16.47 0.39 0.14 843447 0.00 0.00 find_pack_entry_one
10.59 0.48 0.09 60490 0.00 0.00
unpack_object_header_buffer
4.71 0.52 0.04 167441 0.00 0.00 lookup_object
3.53 0.55 0.03 843447 0.00 0.00 fill_pack_entry
3.53 0.58 0.03 213169 0.00 0.00
check_refname_component
2.35 0.60 0.02 550723 0.00 0.00 strip_suffix_mem
2.35 0.62 0.02 214607 0.00 0.00 insert_obj_hash
2.35 0.64 0.02 158866 0.00 0.00 hashcmp
2.35 0.66 0.02 153936 0.00 0.00
nth_packed_object_offset
Do you have all the profile data you were interested in before I try a
'git gc'?
I really appreciate the quick response and the work you put into fixing
this, even when it's my fault for disabling gc, thanks!
Vegard
^ permalink raw reply
* Re: [PATCH] fetch: use "quick" has_sha1_file for tag following
From: Jeff King @ 2016-10-13 17:04 UTC (permalink / raw)
To: Vegard Nossum
Cc: git, Quentin Casasnovas, Shawn Pearce,
Nguyễn Thái Ngọc Duy, Junio C Hamano
In-Reply-To: <20161013165344.jv7hyj74q33yb4ip@sigill.intra.peff.net>
On Thu, Oct 13, 2016 at 12:53:44PM -0400, Jeff King wrote:
> -- >8 --
> Subject: [PATCH] fetch: use "quick" has_sha1_file for tag following
A few comments on my own patch...
> This patch teaches fetch to use HAS_SHA1_QUICK to sacrifice
> accuracy for speed, in cases where we might be racy with a
> simultaneous repack. This is similar to the fix in 0eeb077
> (index-pack: avoid excessive re-reading of pack directory,
> 2015-06-09). As with that case, it's OK for has_sha1_file()
> occasionally say "no I don't have it" when we do, because
> the worst case is not a corruption, but simply that we may
> fail to auto-follow a tag that points to it.
Failing in this direction doesn't make me feel great. I had hoped it
would fail the _other_ way, and request an object that we might already
have.
There are two alternatives that might be worth pursuing:
1. The thing that makes this really painful is the quadratic
duplicate-search in prepare_packed_git_one(). We could speed that
up pretty easily by keeping a hash of the packs indexed by their
names, and looking up in that.
That drops the quadratic factor, but it's still going to be
O(nr_tags * nr_packs), as we have to at the very least readdir()
all of objects/pack to see that nothing new showed up.
I wonder if we could trust the timestamp on the objects/pack
directory to avoid re-reading it at all. That turns it into a
single stat() call.
2. We could stop bothering to reprepare_packed_git() only after the
nth time it is called. This basically turns on the "sacrifice
accuracy for speed" behavior automatically, but it means that most
cases would never do so, because it wouldn't be creating a
performance issue in the first place.
It feels weird and flaky that git might behave differently based on
the number of unfetched tags the remote happens to have, though.
> Here are results from the included perf script, which sets
> up a situation similar to the one described above:
>
> Test HEAD^ HEAD
> ----------------------------------------------------------
> 5550.4: fetch 11.21(10.42+0.78) 0.08(0.04+0.02) -99.3%
The situation in this perf script is obviously designed to show off this
one specific optimization. It feels a bit overly specific, and I doubt
anybody will be all that interested in running it again once the fix is
in. OTOH, I did want to document my reproduction steps, and this seemed
like the only reasonable place to do so. And as the perf suite is
already pretty expensive, perhaps nobody minds adding to it too much.
-Peff
^ permalink raw reply
* [PATCH] fetch: use "quick" has_sha1_file for tag following
From: Jeff King @ 2016-10-13 16:53 UTC (permalink / raw)
To: Vegard Nossum
Cc: git, Quentin Casasnovas, Shawn Pearce,
Nguyễn Thái Ngọc Duy, Junio C Hamano
In-Reply-To: <20161013152632.ynsabbv3yrthgidy@sigill.intra.peff.net>
On Thu, Oct 13, 2016 at 11:26:32AM -0400, Jeff King wrote:
> On Thu, Oct 13, 2016 at 09:20:07AM +0200, Vegard Nossum wrote:
>
> > > Does the patch below help?
> >
> > Yes, ~2m10s -> ~1m25s when I test a git fetch this morning (the other
> > variation in time may be due to different CPU usage by other programs,
> > but I ran with/without the patch multiple times and the difference is
> > consistent).
> > [...]
> > There are some 20k refs on the remote, closer to 25k locally.
>
> OK, that makes some sense. For whatever reason, your remote has a bunch
> of tags that point to objects you do not already have. That could
> happen, I think, if the remote added a lot of tags since you cloned
> (because clone will grab all of the tags), but those tags do not point
> to history that you are otherwise fetching (since fetch by default will
> "auto-follow" such tags).
Armed with this information, I was able to reproduce the issue locally.
However, once my patch is applied, it's now quite fast. So I still don't
know where your other 1m25s is going.
So here's that same patch wrapped up with a commit message. Note that I
converted one more call site to the "quick" form; it would trigger when
the candidate tags are real tag objects, not just pointers to commits.
That might improve your runtime more, depending on what is actually in
your repository.
-- >8 --
Subject: [PATCH] fetch: use "quick" has_sha1_file for tag following
When we auto-follow tags in a fetch, we look at all of the
tags advertised by the remote and fetch ones where we don't
already have the tag, but we do have the object it peels to.
This involves a lot of calls to has_sha1_file(), some of
which we can reasonably expect to fail. Since 45e8a74
(has_sha1_file: re-check pack directory before giving up,
2013-08-30), this may cause many calls to
reprepare_packed_git(), which is potentially expensive.
This has gone unnoticed for several years because it
requires a fairly unique setup to matter:
1. You need to have a lot of packs on the client side to
make reprepare_packed_git() expensive (the most
expensive part is finding duplicates in an unsorted
list, which is currently quadratic).
2. You need a large number of tag refs on the server side
that are candidates for auto-following (i.e., that the
client doesn't have). Each one triggers a re-read of
the pack directory.
3. Under normal circumstances, the client would
auto-follow those tags and after one large fetch, (2)
would no longer be true. But if those tags point to
history which is disconnected from what the client
otherwise fetches, then it will never auto-follow, and
those candidates will impact it on every fetch.
So when all three are true, each fetch pays an extra
O(nr_tags * nr_packs^2) cost, mostly in string comparisons
on the pack names. This was exacerbated by 47bf4b0
(prepare_packed_git_one: refactor duplicate-pack check,
2014-06-30) which uses a slightly more expensive string
check, under the assumption that the duplicate check doesn't
happen very often (and it shouldn't; the real problem here
is how often we are calling reprepare_packed_git()).
This patch teaches fetch to use HAS_SHA1_QUICK to sacrifice
accuracy for speed, in cases where we might be racy with a
simultaneous repack. This is similar to the fix in 0eeb077
(index-pack: avoid excessive re-reading of pack directory,
2015-06-09). As with that case, it's OK for has_sha1_file()
occasionally say "no I don't have it" when we do, because
the worst case is not a corruption, but simply that we may
fail to auto-follow a tag that points to it.
Here are results from the included perf script, which sets
up a situation similar to the one described above:
Test HEAD^ HEAD
----------------------------------------------------------
5550.4: fetch 11.21(10.42+0.78) 0.08(0.04+0.02) -99.3%
Reported-by: Vegard Nossum <vegard.nossum@oracle.com>
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/fetch.c | 11 ++++--
cache.h | 1 +
sha1_file.c | 5 +++
t/perf/p5550-fetch-tags.sh | 99 ++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 112 insertions(+), 4 deletions(-)
create mode 100755 t/perf/p5550-fetch-tags.sh
diff --git a/builtin/fetch.c b/builtin/fetch.c
index d5329f9..74c0546 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -241,9 +241,10 @@ static void find_non_local_tags(struct transport *transport,
* as one to ignore by setting util to NULL.
*/
if (ends_with(ref->name, "^{}")) {
- if (item && !has_object_file(&ref->old_oid) &&
+ if (item &&
+ !has_object_file_with_flags(&ref->old_oid, HAS_SHA1_QUICK) &&
!will_fetch(head, ref->old_oid.hash) &&
- !has_sha1_file(item->util) &&
+ !has_sha1_file_with_flags(item->util, HAS_SHA1_QUICK) &&
!will_fetch(head, item->util))
item->util = NULL;
item = NULL;
@@ -256,7 +257,8 @@ static void find_non_local_tags(struct transport *transport,
* to check if it is a lightweight tag that we want to
* fetch.
*/
- if (item && !has_sha1_file(item->util) &&
+ if (item &&
+ !has_sha1_file_with_flags(item->util, HAS_SHA1_QUICK) &&
!will_fetch(head, item->util))
item->util = NULL;
@@ -276,7 +278,8 @@ static void find_non_local_tags(struct transport *transport,
* We may have a final lightweight tag that needs to be
* checked to see if it needs fetching.
*/
- if (item && !has_sha1_file(item->util) &&
+ if (item &&
+ !has_sha1_file_with_flags(item->util, HAS_SHA1_QUICK) &&
!will_fetch(head, item->util))
item->util = NULL;
diff --git a/cache.h b/cache.h
index 2cfb1ca..ec791a6 100644
--- a/cache.h
+++ b/cache.h
@@ -1154,6 +1154,7 @@ static inline int has_sha1_file(const unsigned char *sha1)
/* Same as the above, except for struct object_id. */
extern int has_object_file(const struct object_id *oid);
+extern int has_object_file_with_flags(const struct object_id *oid, int flags);
/*
* Return true iff an alternate object database has a loose object
diff --git a/sha1_file.c b/sha1_file.c
index 309e87d..c652cb6 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -3309,6 +3309,11 @@ int has_object_file(const struct object_id *oid)
return has_sha1_file(oid->hash);
}
+int has_object_file_with_flags(const struct object_id *oid, int flags)
+{
+ return has_sha1_file_with_flags(oid->hash, flags);
+}
+
static void check_tree(const void *buf, size_t size)
{
struct tree_desc desc;
diff --git a/t/perf/p5550-fetch-tags.sh b/t/perf/p5550-fetch-tags.sh
new file mode 100755
index 0000000..a5dc39f
--- /dev/null
+++ b/t/perf/p5550-fetch-tags.sh
@@ -0,0 +1,99 @@
+#!/bin/sh
+
+test_description='performance of tag-following with many tags
+
+This tests a fairly pathological case, so rather than rely on a real-world
+case, we will construct our own repository. The situation is roughly as
+follows.
+
+The parent repository has a large number of tags which are disconnected from
+the rest of history. That makes them candidates for tag-following, but we never
+actually grab them (and thus they will impact each subsequent fetch).
+
+The child repository is a clone of parent, without the tags, and is at least
+one commit behind the parent (meaning that we will fetch one object and then
+examine the tags to see if they need followed). Furthermore, it has a large
+number of packs.
+
+The exact values of "large" here are somewhat arbitrary; I picked values that
+start to show a noticeable performance problem on my machine, but without
+taking too long to set up and run the tests.
+'
+. ./perf-lib.sh
+
+# make a long nonsense history on branch $1, consisting of $2 commits, each
+# with a unique file pointing to the blob at $2.
+create_history () {
+ perl -le '
+ my ($branch, $n, $blob) = @ARGV;
+ for (1..$n) {
+ print "commit refs/heads/$branch";
+ print "committer nobody <nobody@example.com> now";
+ print "data 4";
+ print "foo";
+ print "M 100644 $blob $_";
+ }
+ ' "$@" |
+ git fast-import --date-format=now
+}
+
+# make a series of tags, one per commit in the revision range given by $@
+create_tags () {
+ git rev-list "$@" |
+ perl -lne 'print "create refs/tags/$. $_"' |
+ git update-ref --stdin
+}
+
+# create $1 nonsense packs, each with a single blob
+create_packs () {
+ perl -le '
+ my ($n) = @ARGV;
+ for (1..$n) {
+ print "blob";
+ print "data <<EOF";
+ print "$_";
+ print "EOF";
+ }
+ ' "$@" |
+ git fast-import &&
+
+ git cat-file --batch-all-objects --batch-check='%(objectname)' |
+ while read sha1
+ do
+ echo $sha1 | git pack-objects .git/objects/pack/pack
+ done
+}
+
+test_expect_success 'create parent and child' '
+ git init parent &&
+ git -C parent commit --allow-empty -m base &&
+ git clone parent child &&
+ git -C parent commit --allow-empty -m trigger-fetch
+'
+
+test_expect_success 'populate parent tags' '
+ (
+ cd parent &&
+ blob=$(echo content | git hash-object -w --stdin) &&
+ create_history cruft 3000 $blob &&
+ create_tags cruft &&
+ git branch -D cruft
+ )
+'
+
+test_expect_success 'create child packs' '
+ (
+ cd child &&
+ git config gc.auto 0 &&
+ git config gc.autopacklimit 0 &&
+ create_packs 500
+ )
+'
+
+test_perf 'fetch' '
+ # make sure there is something to fetch on each iteration
+ git -C child update-ref -d refs/remotes/origin/master &&
+ git -C child fetch
+'
+
+test_done
--
2.10.1.589.gca16299
^ permalink raw reply related
* Re: Huge performance bottleneck reading packs
From: Jeff King @ 2016-10-13 15:26 UTC (permalink / raw)
To: Vegard Nossum
Cc: git, Quentin Casasnovas, Shawn Pearce,
Nguyễn Thái Ngọc Duy, Junio C Hamano
In-Reply-To: <af801f22-0e24-525d-a862-f2114941719a@oracle.com>
On Thu, Oct 13, 2016 at 09:20:07AM +0200, Vegard Nossum wrote:
> > Does the patch below help?
>
> Yes, ~2m10s -> ~1m25s when I test a git fetch this morning (the other
> variation in time may be due to different CPU usage by other programs,
> but I ran with/without the patch multiple times and the difference is
> consistent).
> [...]
> There are some 20k refs on the remote, closer to 25k locally.
OK, that makes some sense. For whatever reason, your remote has a bunch
of tags that point to objects you do not already have. That could
happen, I think, if the remote added a lot of tags since you cloned
(because clone will grab all of the tags), but those tags do not point
to history that you are otherwise fetching (since fetch by default will
"auto-follow" such tags).
It might be interesting to see the results of:
# all the objects we have
git cat-file --batch-all-objects --batch-check='%(objectname)' >us
# all the objects the remote is advertising
git ls-remote origin | cut -f1 | sort -u >them
# what they are advertising that we don't have
comm -13 us them | wc -l
My guess is that the number is relatively high. And that would explain
why nobody else has really complained much; such a pattern is probably
uncommon.
> The fetch doesn't actually get anything from the remote as everything is
> already up to date (that makes the 2m40s times even more frustrating in
> a way :-)). Here's count-objects:
If the fetch is largely a noop, then that makes me wonder why we are
spending even a minute in the "good" case. I wonder if there is some
other spot that is wasting CPU on some inefficient data structure
related to the number of refs you have. If you can do any profiling that
points to a hot spot, that would be interesting to see (and also whether
a gc improves things).
I see in find_non_local_tags() that we build up a sorted string_list via
repeated calls to string_list_insert(), which will keep the thing sorted
at each stage. That's not as efficient as just sorting at the end, but I
think it's probably OK in practice because we actually feed it via
for_each_ref(), whose output is sorted, and so we'd always just be
appending to the end.
-Peff
^ permalink raw reply
* Re: [PATCH v2 3/3] batch check whether submodule needs pushing into one call
From: Heiko Voigt @ 2016-10-13 15:59 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Stefan Beller, git, Jens.Lehmann, Fredrik Gustafsson,
Leandro Lucarella
In-Reply-To: <xmqqwphd4gf6.fsf@gitster.mtv.corp.google.com>
On Wed, Oct 12, 2016 at 10:37:33AM -0700, Junio C Hamano wrote:
> Heiko Voigt <hvoigt@hvoigt.net> writes:
>
> >> If we do not even have these commits locally, then there is no point
> >> attempting to push, so returning 0 (i.e. it is not "needs pushing"
> >> situation) is correct but it is a but subtle. It's not "we know
> >> they already have them", but it is "even if we tried to push, it
> >> won't do us or the other side any good." A single-liner in-code
> >> comment may help.
> >
> > First the naming part. How about:
> >
> > submodule_has_commits()
>
> Nice.
Ok will use that. And while I am at it: I will also rename all the
'hashes' variables to commits because that makes the code way clearer I
think.
> > Returning 0 here means: "No push needed" but the correct answer would
> > be: "We do not know".
>
> Is it? Perhaps I am misreading the "submodule-has-commits"; I
> thought it was "the remote may or may not need updating, but we
> ourselves don't have what they may need to have commits in their
> submodule that are referenced by their superproject, so it would not
> help them even if we pushed our submodule to them". It indeed is
> different from "No push needed" (rather, "our pushing would be
> pointless").
Yes you could also rephrase/see it that way. But the question is: If we
do not have what the remote needs would the user expect us to tell him
that fact and stop or does he usually not care?
> > So how about:
> >
> >
> > if (!submodule_has_hashes(path, hashes))
> > /* NEEDSWORK: The correct answer here is "We do not
> > * know" instead of "No". We currently proceed pushing
> > * here as if the submodules commits are available on a
> > * remote, which is not always correct. */
> > return 0;
>
> I am not sure.
>
> What should happen in this scenario?
>
> * We have two remotes, A and B, for our superproject.
>
> * We are not interested in one submodule at path X. Our repository
> is primarily used to work on the superproject and possibly other
> submodules but not the one at path X.
>
> * We pulled from A to update ourselves. They were actively working
> on the submodule we are not interested in, and path X in the
> superproject records a new commit that we do not have.
>
> * We are now trying to push to B.
I am not sure if this is a typical scenario? Well, if you are updating
your main branch from someone else and then push it to your own fork
maybe. You could specify --no-recurse-submodules for this case though.
The proper solution for this case would probably be something along the
lines of 'submodule.<name>.fetchRecurseSubmodules' but for push so we
can mark certain submodules as uninteresting by default.
I like to be more protective to the user here. Its usually more
annoying for possibly many others when you push out things that have
missing things compared to one person not being able to push because his
submodule is not up-to-date/initialized.
> Should different things happen in these two subcases?
>
> - We are not interested in submodule at path X, so we haven't even
> done "submodule init" on it.
>
> - We are not interested in submodule at path X, so even though we
> do have a rather stale clone of it, we do not usually bother
> updating what is checked out at path X and commit our changes
> outside that area.
>
> I tend to think that in these two cases the same thing should
> happen. I am not sure if that same thing should be rejection
> (i.e. "you do not know for sure that the commit at path X of the
> superproject you are pushing exists in the submodule repository at
> the receiving end, so I'd refuse to push the superproject"), as it
> makes the only remedy for the situation is for you to make a full
> clone of the submodule you are not interested in and you have never
> touched yourself in either of these two subcases.
I also think in both situations the same thing should happen. A decision
that something different should happen should be made explicitly instead
of implicitly just because some submodule is not initialized. That might
be by accident or because a certain submodule is new so here the choice
should be made deliberately by the user, IMO.
Cheers Heiko
^ permalink raw reply
* Re: Uninitialized submodules as symlinks
From: Heiko Voigt @ 2016-10-13 16:10 UTC (permalink / raw)
To: David Turner; +Cc: git@vger.kernel.org
In-Reply-To: <fd5bcf57f92944c0b7f6f2f8342c342c@exmbdft7.ad.twosigma.com>
On Fri, Oct 07, 2016 at 06:17:05PM +0000, David Turner wrote:
> Presently, uninitialized submodules are materialized in the working
> tree as empty directories. We would like to consider having them be
> symlinks. Specifically, we'd like them to be symlinks into a FUSE
> filesystem which retrieves files on demand.
How about portability? This feature would only work on Unix like
operating systems. You have to be careful to not break Windows since
they do not have symlinks.
Cheers Heiko
^ permalink raw reply
* Re: Formatting problem send_mail in version 2.10.0
From: Matthieu Moy @ 2016-10-13 16:05 UTC (permalink / raw)
To: Kevin Daudt
Cc: Jeff King, Junio C Hamano, Larry Finger, Mathieu Lienard--Mayor,
Remi Lespinet, git
In-Reply-To: <20161013153317.GA1698@ikke.info>
Kevin Daudt <me@ikke.info> writes:
> On Wed, Oct 12, 2016 at 07:13:22PM -0400, Jeff King wrote:
>
>> I think the answer is pretty clearly no. It's just that historically we
>> have auto-munged it into something useful. I think the viable options
>> are basically:
>>
>> 1. Tell people not to do that, and to do something RFC compliant like
>> "Stable [4.8+]" <stable@vger.kernel.org>. This is a little funny
>> for git because we otherwise do not require things like
>> rfc-compliant quoting for our name/email pairs. But it Just Works
>> without anybody having to write extra code, or worry about corner
>> cases in parsing.
>>
>> 2. Drop everything after the trailing ">". This gives a valid rfc2822
>> cc, and people can pick the "# 4.8" from the cc line in the body.
>
> Comments, surrounded by parenthesis are allowed after the ">" according
> to the RFC, just plain dropping everything comming after that would
> break that support.
Our in-house parser does consider (...) comments, and my patch does not
change how they are handled: they are still kept after the address part.
However, another piece of code does strip everything behind ">":
sub sanitize_address {
...
# remove garbage after email address
$recipient =~ s/(.*>).*$/$1/;
introduced in 831a488b76e0 (git-send-email: remove garbage after email
address, 2012-11-22).
IMHO, it's OK to continue doing this: removing comments from To: and Cc:
is not really a problem (and I think we've seen nobody complain about it
since 2012). But after my patch, these two lines can probably safely be
removed, as there can no longer be "garbage" after the email, only
comments.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: 2 directories same spelling one directory is camel cased
From: Torsten Bögershausen @ 2016-10-13 16:03 UTC (permalink / raw)
To: git
In-Reply-To: <e0a3eb0590552c98638d486db8ad4a12@davidwbrown.name>
On 12.10.16 18:05, David Brown wrote:
> Howdy git gurus,
>
> I have the dubious distinction of working with a remote repo (master) that has a class loader run-time error when cloned, built and executed.
>
> The reason for the runtime issue is a directory hierarchical path has to directories (folders) with the same name spelling but one of the directories is camel-cased. The package names are the same.
>
> The compiler doesn't care but the run-time class loader has an issue with the 2 'same like named' classes.
>
> How to remove the offending directories and files at the locally cloned repo but not push 'deleted' directories and files back to origin/master the remote repo?
>
> Please advise.
>
> Regards.
This email did not resolve from here: David Brown <david@davidwbrown.name>
It is somewhat unclear, which issue the class loader has.
What does
git ls-files | grep -i "sameNameBitDifferent"
say ?
What do you mean with
"How to remove the offending directories" ?
Just run
rm -rf "offending directories" ?
Or, may be
git mv dir1 NewDir1
If you don't want to push, you don't have to, or what do I miss ?
^ permalink raw reply
* Re: git branches & merge
From: Kevin Daudt @ 2016-10-13 15:49 UTC (permalink / raw)
To: Anatoly Borodin; +Cc: webmaster, git
In-Reply-To: <CACNzp2nG1KfFuiSH1UUGVSz1T39PjzfDgCp6vqOVe7-Lph2yNw@mail.gmail.com>
On Wed, Oct 12, 2016 at 04:43:07PM +0200, Anatoly Borodin wrote:
> Hi,
>
>
> the IP will not be overwritten, you'll still have the new IP in
> master. Nothing to worry about :)
>
>
To expand on that, git does a so called 3-way merge. This means git will
look for a common base commit, and compare changes from both sides to
see which side actually made a change.
In your case, the base and the release branch both should show the old
ip, and the master side would show the new IP. This tells git that
master has changed, and not the release branch, and takes the master
side of the change, resulting the new IP to show up.
Hope this helps, Kevin.
^ permalink raw reply
* Re: Formatting problem send_mail in version 2.10.0
From: Kevin Daudt @ 2016-10-13 15:33 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Matthieu Moy, Larry Finger,
Mathieu Lienard--Mayor, Remi Lespinet, git
In-Reply-To: <20161012231321.ar2csdmvhdya2q7r@sigill.intra.peff.net>
On Wed, Oct 12, 2016 at 07:13:22PM -0400, Jeff King wrote:
> On Wed, Oct 12, 2016 at 01:53:52PM -0700, Junio C Hamano wrote:
>
> > Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> >
> > >>> If it's not in the body of the message, then where is it?
> > >>
> > >> This point is clarified in the thread
> > >> http://marc.info/?l=linux-wireless&m=147625930203434&w=2, which is
> > >> with my upstream maintainer.
> > >
> > > Which explicitly states that the syntax is not [$number], but # $number,
> > > right?
> >
> > But I do not think that works, either. Let's step back.
> >
> > People write things like these
> >
> > Cc: Stable <stable@vger.kernel.org> # 4.8
> > Cc: Stable <stable@vger.kernel.org> [4.8+]
> >
> > in the trailer part in the body of the message. Are these lines
> > meant to be usable if they appear as Cc: headers of an outgoing
> > piece of e-mail as-is?
>
> I think the answer is pretty clearly no. It's just that historically we
> have auto-munged it into something useful. I think the viable options
> are basically:
>
> 1. Tell people not to do that, and to do something RFC compliant like
> "Stable [4.8+]" <stable@vger.kernel.org>. This is a little funny
> for git because we otherwise do not require things like
> rfc-compliant quoting for our name/email pairs. But it Just Works
> without anybody having to write extra code, or worry about corner
> cases in parsing.
>
> 2. Drop everything after the trailing ">". This gives a valid rfc2822
> cc, and people can pick the "# 4.8" from the cc line in the body.
Comments, surrounded by parenthesis are allowed after the ">" according
to the RFC, just plain dropping everything comming after that would
break that support.
^ permalink raw reply
* Re: [PATCH v2 1/3] serialize collection of changed submodules
From: Heiko Voigt @ 2016-10-13 15:27 UTC (permalink / raw)
To: Junio C Hamano
Cc: Stefan Beller, Jeff King, git@vger.kernel.org, Jens Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <xmqq1szl5vvf.fsf@gitster.mtv.corp.google.com>
On Wed, Oct 12, 2016 at 10:18:28AM -0700, Junio C Hamano wrote:
> Heiko Voigt <hvoigt@hvoigt.net> writes:
>
> > Which seems quite extensively long for a static function so how about
> > we shorten it a bit and add a comment:
> >
> > /* lookup or create commit object list for submodule */
> > get_commit_objects_for_submodule_path(...
>
> Or you can even lose "get_" and "path", I guess. You are not even
> "getting" commits but the array that holds them, so the caller can
> use it to "get" one of them or it can even use it to "put" a new
> one, no? "get-commit-objects" is a misnomer in that sense. Either
> one of
>
> get_submodule_commits_array()
> submodule_commits()
>
> perhaps? I dunno.
I like the last one. Will use 'submodule_commits()'.
Cheers Heiko
^ permalink raw reply
* Re: [PATCH v3 25/25] sequencer: mark all error messages for translation
From: Johannes Schindelin @ 2016-10-13 14:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, git, Jakub Narębski
In-Reply-To: <xmqqlgxt2rch.fsf@gitster.mtv.corp.google.com>
Hi,
On Wed, 12 Oct 2016, Junio C Hamano wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
>
> > Can we please have the following change instead? I think it makes sense
> > to deviate from the usual conventions in a case like this.
>
> You have at least two independent changes relative to Dscho's
> version.
>
> (1) Show line breaks more prominently by avoiding "\n\n" and
> breaking the string at "\n"; this matches the way how the
> result would be displayed more closely to how the source looks
> like.
>
> (2) Ignore the usual indentation rule and have messages start at
> the left end of the source.
>
> Which one are you saying "makes sense" to? Both?
>
> I guess both can be grouped together into one theme: match the way
> the final output and the source code look like.
>
> If that is the motivation behind "makes sense", I'd prefer to see
> the change explained explicitly with that rationale in the log
> message.
>
> Thanks. I personally agree with that motivation (if the one I
> guessed above is your motivation, that is).
I agree with that motivation, but I decided to go about it in a way that
is more in line with the existing source code:
-- snipsnap --
diff --git a/sequencer.c b/sequencer.c
index 8e10bb5..1cf70f7 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -483,6 +483,20 @@ static char **read_author_script(void)
return env;
}
+static const char staged_changes_advice[] =
+N_("you have staged changes in your working tree\n"
+"If these changes are meant to be squashed into the previous commit, run:\n"
+"\n"
+" git commit --amend %s\n"
+"\n"
+"If they are meant to go into a new commit, run:\n"
+"\n"
+" git commit %s\n"
+"\n"
+"In both cases, once you're done, continue with:\n"
+"\n"
+" git rebase --continue\n");
+
/*
* If we are cherry-pick, and if the merge did not result in
* hand-editing, we will hit this commit and inherit the original
@@ -509,18 +523,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
if (!env) {
const char *gpg_opt = gpg_sign_opt_quoted(opts);
- return error(_("you have staged changes in your "
- "working tree. If these changes are "
- "meant to be\n"
- "squashed into the previous commit, "
- "run:\n\n"
- " git commit --amend %s\n\n"
- "If they are meant to go into a new "
- "commit, run:\n\n"
- " git commit %s\n\n"
- "In both cases, once you're done, "
- "continue with:\n\n"
- " git rebase --continue\n"),
+ return error(_(staged_changes_advice),
gpg_opt, gpg_opt);
}
}
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox