Re: %C(auto) not working as expected

[Duy Nguyen]

On Mon, Oct 10, 2016 at 6:46 AM, Jeff King  wrote:
> On Sun, Oct 09, 2016 at 03:24:17PM +0200, René Scharfe wrote:
>
>> Offering a way to enable terminal-detection for all color codes of a
>> format would be useful, but using the existing "auto," prefix for that
>> would be a behaviour change that could surprise users.

I wonder if we made a mistake associating terminal-detection with
%C(auto,...). The more likely use case is enable or disable all
colors, not "the next tag".

> Yeah. In retrospect, it probably would have been saner to make %C(red) a
> noop when --color is not in effect (either because of --no-color, or
> more likely when --color=auto is in effect and stdout is not a
> terminal). But that ship has long since sailed, I think.
>
> If we do a revamp of the pretty-formats to bring them more in line with
> ref-filter (e.g., something like "%(color:red)") maybe that would be an
> opportunity to make minor adjustments. Though, hmm, it looks like
> for-each-ref already knows "%(color:red)", and it's unconditional.
>  So perhaps we would need to go through some deprecation period or
> other transition.

We could add some new tag to change the behavior of all following %C
tags. Something like %C(tty) maybe (probably a bad name), then
discourage the use if "%C(auto" for terminal detection?
-- 
Duy

Re: Bug? git worktree fails with master on bare repo

[Duy Nguyen]

On Sun, Oct 9, 2016 at 8:42 PM, Michael Tutty  wrote:
> Dennis,
> Thanks for the great response, and for spending time on my issue.
> I'll try that first patch and see what happens.
>
> In the meantime, it got weirder...
>
> I created a brand-new (bare) repo

Elaboration needed here. If I create a bare _clone_, then "HEAD" could
be detached, or point to some branch, depending on where "HEAD" is in
the source repo. If source repo's HEAD is "master", I got the same
behavior (worktree add fails). If it's detached or points to some
other branch, it's ok. If this is "git init --bare" then I got "fatal:
invalid reference: master".

> and was able to git add worktree
> /path master.  I was able to do this repeatedly, even using the
> worktree to merge other branches to master.  I didn't find any
> condition or step that caused some kind of orphan master work tree,
> which was what I thought the underlying problem might be.
-- 
Duy

git merge deletes my changes

[Eduard Egorov]

Hello Git experts,

A week ago, I've reset a state of 'ceph-ansible' folder in %current% branch 
with code from corresponding branch (that tracks an upstream from github):

# git read-tree --prefix=ceph-ansible/ -u ceph_ansible

Then I've committed several changes, including:

1. Renamed file and commited:
# git mv site.yml.sample site.yml

2. Made some changes and committed

3. Pulled updates from original branch by:
# git merge -s subtree --squash ceph_ansible

It said:
Auto-merging ceph-ansible/site.yml.sample
blablabla
Squash commit -- not updating HEAD
Automatic merge went well; stopped before committing as requested

I can see that "my" ceph-ansible/site.yml is deleted (as well as few new files 
added by me), ceph-ansible/site.yml.sample is restored and doesn't contain my 
changes (it's just restored to the state before my changes).
`git log ceph_ansible site.yml.sample` shows the latest change on ceph_ansible 
branch done from 26th of August, 2016, my changes in %current% branch was this 
October, so it shouldn't be any conflict either (even if it was).
I believe there is some obvious explanation for this behavior. Could you help 
me please? What information can  I provide in order to troubleshoot this issue? 

A post on SO: 
http://stackoverflow.com/questions/39954265/git-merge-deletes-my-changes

With best regards
Eduard Egorov

Re: [PATCH v2 1/2] files_read_raw_ref: avoid infinite loop on broken symlinks

[Michael Haggerty]

On 10/06/2016 09:41 PM, Jeff King wrote:
> [...]
> Subject: files_read_raw_ref: avoid infinite loop on broken symlinks
> [...]

This patch is

Reviewed-by: Michael Haggerty 

Michael

Re: [PATCH 2/2] files_read_raw_ref: prevent infinite retry loops in general

[Michael Haggerty]

On 10/06/2016 06:48 PM, Jeff King wrote:
> Limit the number of retries to 3. That should be adequate to
> prevent any races, while preventing the possibility of
> infinite loops if the logic fails to handle any other
> possible error modes correctly.
> 
> After the fix in the previous commit, there's no known way
> to trigger an infinite loop, but I did manually verify that
> this fixes the test in that commit even when the code change
> is not applied.
> [...]

This patch is

Reviewed-by: Michael Haggerty 

Michael

[PATCH v4 00/14] Mark strings in Perl scripts for translation

[Vasco Almeida]

Mark messages in some perl scripts for translation.

Fix minor stuff and follow Jakub Narębski's suggestion to use N__() instead of
__() in the hash tables.

Interdiff included below.

Vasco Almeida (14):
  i18n: add--interactive: mark strings for translation
  i18n: add--interactive: mark simple here-documents for translation
  i18n: add--interactive: mark strings with interpolation for
translation
  i18n: clean.c: match string with git-add--interactive.perl
  i18n: add--interactive: mark plural strings
  i18n: add--interactive: mark patch prompt for translation
  i18n: add--interactive: i18n of help_patch_cmd
  i18n: add--interactive: mark edit_hunk_manually message for
translation
  i18n: add--interactive: remove %patch_modes entries
  i18n: add--interactive: mark status words for translation
  i18n: send-email: mark strings for translation
  i18n: send-email: mark warnings and errors for translation
  i18n: send-email: mark string with interpolation for translation
  i18n: difftool: mark warnings for translation

 Makefile  |   3 +-
 builtin/clean.c   |  10 +-
 git-add--interactive.perl | 322 ++
 git-difftool.perl |  22 ++--
 git-send-email.perl   | 176 +
 perl/Git/I18N.pm  |  17 ++-
 t/t0202/test.pl   |  14 +-
 7 files changed, 353 insertions(+), 211 deletions(-)


diff --git a/Makefile b/Makefile
index 4ef0344..9dc95cb 100644
--- a/Makefile
+++ b/Makefile
@@ -2112,7 +2112,7 @@ XGETTEXT_FLAGS_C = $(XGETTEXT_FLAGS) --language=C \
 XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \
--keyword=gettextln --keyword=eval_gettextln
 XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --language=Perl \
-   --keyword=__ --keyword="__n:1,2"
+   --keyword=__ --keyword=N__ --keyword="__n:1,2"
 LOCALIZED_C = $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
 LOCALIZED_SH = $(SCRIPT_SH)
 LOCALIZED_SH += git-parse-remote.sh
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 0b4a27c..4754104 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -601,7 +601,7 @@ sub list_and_choose {
}
}
if ($opts->{SINGLETON} && $bottom != $top) {
-   error_msg sprintf(__("Huh (%s)?"), $choice);
+   error_msg sprintf(__("Huh (%s)?\n"), $choice);
next TOPLOOP;
}
for ($i = $bottom-1; $i <= $top-1; $i++) {
@@ -704,7 +704,7 @@ sub revert_cmd {
$_->{INDEX_ADDDEL} eq 'create') {
system(qw(git update-index 
--force-remove --),
   $_->{VALUE});
-   print "note: $_->{VALUE} is untracked 
now.\n";
+   printf(__("note: %s is untracked 
now.\n"), $_->{VALUE});
}
}
}
@@ -1038,25 +1038,25 @@ sub color_diff {
 }
 
 my %edit_hunk_manually_modes = (
-   stage => __(
+   stage => N__(
 "# If the patch applies cleanly, the edited hunk will immediately be
 # marked for staging."),
-   stash => __(
+   stash => N__(
 "# If the patch applies cleanly, the edited hunk will immediately be
 # marked for stashing."),
-   reset_head => __(
+   reset_head => N__(
 "# If the patch applies cleanly, the edited hunk will immediately be
 # marked for unstaging."),
-   reset_nothead => __(
+   reset_nothead => N__(
 "# If the patch applies cleanly, the edited hunk will immediately be
 # marked for applying."),
-   checkout_index => __(
+   checkout_index => N__(
 "# If the patch applies cleanly, the edited hunk will immediately be
 # marked for discarding"),
-   checkout_head => __(
+   checkout_head => N__(
 "# If the patch applies cleanly, the edited hunk will immediately be
 # marked for discarding."),
-   checkout_nothead => __(
+   checkout_nothead => N__(
 "# If the patch applies cleanly, the edited hunk will immediately be
 # marked for applying."),
 );
@@ -1078,7 +1078,7 @@ sub edit_hunk_manually {
 # To remove '%s' lines, delete them.
 # Lines starting with # will be removed.
 #\n"), $remove_minus, $remove_plus),
-$edit_hunk_manually_modes{$patch_mode}, __(
+__($edit_hunk_manually_modes{$patch_mode}), __(
 # TRANSLATORS: 'it' refers to the patch mentioned in the previous messages.
 " If it does not apply cleanly, you will be given
 # an opportunity to edit again. If all lines of the hunk are removed,
@@ -1192,43 +1192,43 @@ sub edit_hunk_loop {
 }
 
 my %help_patch_modes = (
-   stage => __(
+   stage => N__(
 "y - stage this hunk
 n - do not stage this hunk
 q - quit; do not stage this hunk or any of the remaining ones
 a - stage this hunk and all later hunks in

[PATCH v4 07/14] i18n: add--interactive: i18n of help_patch_cmd

[Vasco Almeida]

Mark help message of help_patch_cmd for translation.  The message must
be unfolded to be free of variables so we can have high quality
translations.

Signed-off-by: Vasco Almeida 
---
 git-add--interactive.perl | 54 ---
 1 file changed, 46 insertions(+), 8 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index b7d382b..045b847 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1189,15 +1189,53 @@ sub edit_hunk_loop {
}
 }
 
+my %help_patch_modes = (
+   stage => N__(
+"y - stage this hunk
+n - do not stage this hunk
+q - quit; do not stage this hunk or any of the remaining ones
+a - stage this hunk and all later hunks in the file
+d - do not stage this hunk or any of the later hunks in the file"),
+   stash => N__(
+"y - stash this hunk
+n - do not stash this hunk
+q - quit; do not stash this hunk or any of the remaining ones
+a - stash this hunk and all later hunks in the file
+d - do not stash this hunk or any of the later hunks in the file"),
+   reset_head => N__(
+"y - unstage this hunk
+n - do not unstage this hunk
+q - quit; do not unstage this hunk or any of the remaining ones
+a - unstage this hunk and all later hunks in the file
+d - do not unstage this hunk or any of the later hunks in the file"),
+   reset_nothead => N__(
+"y - apply this hunk to index
+n - do not apply this hunk to index
+q - quit; do not apply this hunk or any of the remaining ones
+a - apply this hunk and all later hunks in the file
+d - do not apply this hunk or any of the later hunks in the file"),
+   checkout_index => N__(
+"y - discard this hunk from worktree
+n - do not discard this hunk from worktree
+q - quit; do not discard this hunk or any of the remaining ones
+a - discard this hunk and all later hunks in the file
+d - do not discard this hunk or any of the later hunks in the file"),
+   checkout_head => N__(
+"y - discard this hunk from index and worktree
+n - do not discard this hunk from index and worktree
+q - quit; do not discard this hunk or any of the remaining ones
+a - discard this hunk and all later hunks in the file
+d - do not discard this hunk or any of the later hunks in the file"),
+   checkout_nothead => N__(
+"y - apply this hunk to index and worktree
+n - do not apply this hunk to index and worktree
+q - quit; do not apply this hunk or any of the remaining ones
+a - apply this hunk and all later hunks in the file
+d - do not apply this hunk or any of the later hunks in the file"),
+);
+
 sub help_patch_cmd {
-   my $verb = lc $patch_mode_flavour{VERB};
-   my $target = $patch_mode_flavour{TARGET};
-   print colored $help_color, <

[PATCH v4 02/14] i18n: add--interactive: mark simple here-documents for translation

[Vasco Almeida]

Mark messages in here-document without interpolation for translation.

The here-document delimiter \EOF, which is the same as 'EOF', indicate
that the text is to be treated literally without interpolation of its
content.  Unfortunately xgettext is not able to extract here documents
with delimiter \EOF but it is with delimiter enclosed in single quotes.
Then change \EOF to 'EOF', although in this case does not make
difference what variation of here-document to use since there is nothing
to interpolate.

Signed-off-by: Vasco Almeida 
---
 git-add--interactive.perl | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index cf216ec..5800010 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -639,7 +639,7 @@ sub list_and_choose {
 }
 
 sub singleton_prompt_help_cmd {
-   print colored $help_color, <<\EOF ;
+   print colored $help_color, __ <<'EOF' ;
 Prompt help:
 1  - select a numbered item
 foo- select item based on unique prefix
@@ -648,7 +648,7 @@ EOF
 }
 
 sub prompt_help_cmd {
-   print colored $help_color, <<\EOF ;
+   print colored $help_color, __ <<'EOF' ;
 Prompt help:
 1  - select a single item
 3-5- select a range of items
@@ -1584,7 +1584,9 @@ sub quit_cmd {
 }
 
 sub help_cmd {
-   print colored $help_color, <<\EOF ;
+# TRANSLATORS: please do not translate the command names
+# 'status', 'update', 'revert', etc.
+   print colored $help_color, __ <<'EOF' ;
 status- show paths with changes
 update- add working tree state to the staged set of changes
 revert- revert staged set of changes back to the HEAD version
-- 
2.7.4

[PATCH v4 03/14] i18n: add--interactive: mark strings with interpolation for translation

[Vasco Almeida]

Since at this point Git::I18N.perl lacks support for Perl i18n
placeholder substitution, use of sprintf following die or error_msg is
necessary for placeholder substitution take place.

Signed-off-by: Vasco Almeida 
---
 git-add--interactive.perl | 25 +
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 5800010..d05ac60 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -615,12 +615,12 @@ sub list_and_choose {
else {
$bottom = $top = find_unique($choice, @stuff);
if (!defined $bottom) {
-   error_msg "Huh ($choice)?\n";
+   error_msg sprintf(__("Huh (%s)?\n"), 
$choice);
next TOPLOOP;
}
}
if ($opts->{SINGLETON} && $bottom != $top) {
-   error_msg "Huh ($choice)?\n";
+   error_msg sprintf(__("Huh (%s)?\n"), $choice);
next TOPLOOP;
}
for ($i = $bottom-1; $i <= $top-1; $i++) {
@@ -717,7 +717,7 @@ sub revert_cmd {
$_->{INDEX_ADDDEL} eq 'create') {
system(qw(git update-index 
--force-remove --),
   $_->{VALUE});
-   print "note: $_->{VALUE} is untracked 
now.\n";
+   printf(__("note: %s is untracked 
now.\n"), $_->{VALUE});
}
}
}
@@ -1056,7 +1056,7 @@ sub edit_hunk_manually {
my $hunkfile = $repo->repo_path . "/addp-hunk-edit.diff";
my $fh;
open $fh, '>', $hunkfile
-   or die "failed to open hunk edit file for writing: " . $!;
+   or die sprintf(__("failed to open hunk edit file for writing: 
%s"), $!);
print $fh "# Manual hunk edit mode -- see bottom for a quick guide\n";
print $fh @$oldtext;
my $participle = $patch_mode_flavour{PARTICIPLE};
@@ -1083,7 +1083,7 @@ EOF
}
 
open $fh, '<', $hunkfile
-   or die "failed to open hunk edit file for reading: " . $!;
+   or die sprintf(__("failed to open hunk edit file for reading: 
%s"), $!);
my @newtext = grep { !/^#/ } <$fh>;
close $fh;
unlink $hunkfile;
@@ -1236,7 +1236,7 @@ sub apply_patch_for_checkout_commit {
 
 sub patch_update_cmd {
my @all_mods = list_modified($patch_mode_flavour{FILTER});
-   error_msg "ignoring unmerged: $_->{VALUE}\n"
+   error_msg sprintf(__("ignoring unmerged: %s\n"), $_->{VALUE})
for grep { $_->{UNMERGED} } @all_mods;
@all_mods = grep { !$_->{UNMERGED} } @all_mods;
 
@@ -1418,7 +1418,8 @@ sub patch_update_file {
chomp $response;
}
if ($response !~ /^\s*\d+\s*$/) {
-   error_msg "Invalid number: 
'$response'\n";
+   error_msg sprintf(__("Invalid number: 
'%s'\n"),
+$response);
} elsif (0 < $response && $response <= $num) {
$ix = $response - 1;
} else {
@@ -1460,7 +1461,7 @@ sub patch_update_file {
if ($@) {
my ($err,$exp) = ($@, $1);
$err =~ s/ at .*git-add--interactive 
line \d+,  line \d+.*$//;
-   error_msg "Malformed search regexp 
$exp: $err\n";
+   error_msg sprintf(__("Malformed search 
regexp %s: %s\n"), $exp, $err);
next;
}
my $iy = $ix;
@@ -1625,18 +1626,18 @@ sub process_args {
$patch_mode = $1;
$arg = shift @ARGV or die __("missing --");
} else {
-   die "unknown --patch mode: $1";
+   die sprintf(__("unknown --patch mode: %s"), $1);
}
} else {
$patch_mode = 'stage';
$arg = shift @ARGV or die __("missing --");
}
-   die "invalid argument $arg, expecting --"
-   unless $arg eq "--";
+   die sprintf(__("invalid argument %s, expecting --"),
+  $a

[PATCH v4 01/14] i18n: add--interactive: mark strings for translation

[Vasco Almeida]

Mark simple strings (without interpolation) for translation.

Brackets around first parameter of ternary operator is necessary because
otherwise xgettext fails to extract strings marked for translation from
the rest of the file.

Signed-off-by: Vasco Almeida 
---
 git-add--interactive.perl | 76 ++-
 1 file changed, 42 insertions(+), 34 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index ee3d812..cf216ec 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -4,6 +4,7 @@ use 5.008;
 use strict;
 use warnings;
 use Git;
+use Git::I18N;
 
 binmode(STDOUT, ":raw");
 
@@ -253,8 +254,9 @@ sub list_untracked {
run_cmd_pipe(qw(git ls-files --others --exclude-standard --), @ARGV);
 }
 
-my $status_fmt = '%12s %12s %s';
-my $status_head = sprintf($status_fmt, 'staged', 'unstaged', 'path');
+# TRANSLATORS: you can adjust this to align "git add -i" status menu
+my $status_fmt = __('%12s %12s %s');
+my $status_head = sprintf($status_fmt, __('staged'), __('unstaged'), 
__('path'));
 
 {
my $initial;
@@ -680,7 +682,7 @@ sub update_cmd {
my @mods = list_modified('file-only');
return if (!@mods);
 
-   my @update = list_and_choose({ PROMPT => 'Update',
+   my @update = list_and_choose({ PROMPT => __('Update'),
   HEADER => $status_head, },
 @mods);
if (@update) {
@@ -692,7 +694,7 @@ sub update_cmd {
 }
 
 sub revert_cmd {
-   my @update = list_and_choose({ PROMPT => 'Revert',
+   my @update = list_and_choose({ PROMPT => __('Revert'),
   HEADER => $status_head, },
 list_modified());
if (@update) {
@@ -726,13 +728,13 @@ sub revert_cmd {
 }
 
 sub add_untracked_cmd {
-   my @add = list_and_choose({ PROMPT => 'Add untracked' },
+   my @add = list_and_choose({ PROMPT => __('Add untracked') },
  list_untracked());
if (@add) {
system(qw(git update-index --add --), @add);
say_n_paths('added', @add);
} else {
-   print "No untracked files.\n";
+   print __("No untracked files.\n");
}
print "\n";
 }
@@ -1166,8 +1168,14 @@ sub edit_hunk_loop {
}
else {
prompt_yesno(
-   'Your edited hunk does not apply. Edit again '
-   . '(saying "no" discards!) [y/n]? '
+   # TRANSLATORS: do not translate [y/n]
+   # The program will only accept that input
+   # at this point.
+   # Consider translating (saying "no" discards!) 
as
+   # (saying "n" for "no" discards!) if the 
translation
+   # of the word "no" does not start with n.
+   __('Your edited hunk does not apply. Edit again 
'
+  . '(saying "no" discards!) [y/n]? ')
) or return undef;
}
}
@@ -1213,11 +1221,11 @@ sub apply_patch_for_checkout_commit {
run_git_apply 'apply '.$reverse, @_;
return 1;
} elsif (!$applies_index) {
-   print colored $error_color, "The selected hunks do not apply to 
the index!\n";
-   if (prompt_yesno "Apply them to the worktree anyway? ") {
+   print colored $error_color, __("The selected hunks do not apply 
to the index!\n");
+   if (prompt_yesno __("Apply them to the worktree anyway? ")) {
return run_git_apply 'apply '.$reverse, @_;
} else {
-   print colored $error_color, "Nothing was applied.\n";
+   print colored $error_color, __("Nothing was 
applied.\n");
return 0;
}
} else {
@@ -1237,9 +1245,9 @@ sub patch_update_cmd {
 
if (!@mods) {
if (@all_mods) {
-   print STDERR "Only binary files changed.\n";
+   print STDERR __("Only binary files changed.\n");
} else {
-   print STDERR "No changes.\n";
+   print STDERR __("No changes.\n");
}
return 0;
}
@@ -1247,7 +1255,7 @@ sub patch_update_cmd {
@them = @mods;
}
else {
-   @them = list_and_choose({ PROMPT => 'Patch update',
+   @them = list_and_choose({ PROMPT => __('Patch update'),
  HEADER => $status_head, },
@mods);
}
@@ -1397,12 +1405,12 @@ sub patch_update_file {

[PATCH v4 04/14] i18n: clean.c: match string with git-add--interactive.perl

[Vasco Almeida]

Change strings for help to match the ones in git-add--interactive.perl.
The strings now represent one entry to translate each rather then two
entries each different only by an ending newline character.

Signed-off-by: Vasco Almeida 
---
 builtin/clean.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index 0371010..d6bc3aa 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -287,11 +287,11 @@ static void pretty_print_menus(struct string_list 
*menu_list)
 static void prompt_help_cmd(int singleton)
 {
clean_print_color(CLEAN_COLOR_HELP);
-   printf_ln(singleton ?
+   printf(singleton ?
  _("Prompt help:\n"
"1  - select a numbered item\n"
"foo- select item based on unique prefix\n"
-   "   - (empty) select nothing") :
+   "   - (empty) select nothing\n") :
  _("Prompt help:\n"
"1  - select a single item\n"
"3-5- select a range of items\n"
@@ -299,7 +299,7 @@ static void prompt_help_cmd(int singleton)
"foo- select item based on unique prefix\n"
"-...   - unselect specified items\n"
"*  - choose all items\n"
-   "   - (empty) finish selecting"));
+   "   - (empty) finish selecting\n"));
clean_print_color(CLEAN_COLOR_RESET);
 }
 
@@ -508,7 +508,7 @@ static int parse_choice(struct menu_stuff *menu_stuff,
if (top <= 0 || bottom <= 0 || top > menu_stuff->nr || bottom > 
top ||
(is_single && bottom != top)) {
clean_print_color(CLEAN_COLOR_ERROR);
-   printf_ln(_("Huh (%s)?"), (*ptr)->buf);
+   printf(_("Huh (%s)?\n"), (*ptr)->buf);
clean_print_color(CLEAN_COLOR_RESET);
continue;
}
@@ -774,7 +774,7 @@ static int ask_each_cmd(void)
 static int quit_cmd(void)
 {
string_list_clear(&del_list, 0);
-   printf_ln(_("Bye."));
+   printf(_("Bye.\n"));
return MENU_RETURN_NO_LOOP;
 }
 
-- 
2.7.4

[PATCH v4 05/14] i18n: add--interactive: mark plural strings

[Vasco Almeida]

Mark plural strings for translation.  Unfold each action case in one
entire sentence.

Pass new keyword for xgettext to extract.

Update test to include new subroutine __n() for plural strings handling.

Update documentation to include a description of the new __n() subroutine.

Signed-off-by: Vasco Almeida 
---
 Makefile  |  3 ++-
 git-add--interactive.perl | 27 ++-
 perl/Git/I18N.pm  |  9 -
 t/t0202/test.pl   | 11 ++-
 4 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/Makefile b/Makefile
index 1aad150..4ef0344 100644
--- a/Makefile
+++ b/Makefile
@@ -2111,7 +2111,8 @@ XGETTEXT_FLAGS_C = $(XGETTEXT_FLAGS) --language=C \
--keyword=_ --keyword=N_ --keyword="Q_:1,2"
 XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \
--keyword=gettextln --keyword=eval_gettextln
-XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --keyword=__ --language=Perl
+XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --language=Perl \
+   --keyword=__ --keyword="__n:1,2"
 LOCALIZED_C = $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
 LOCALIZED_SH = $(SCRIPT_SH)
 LOCALIZED_SH += git-parse-remote.sh
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index d05ac60..cd61783 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -669,12 +669,18 @@ sub status_cmd {
 sub say_n_paths {
my $did = shift @_;
my $cnt = scalar @_;
-   print "$did ";
-   if (1 < $cnt) {
-   print "$cnt paths\n";
-   }
-   else {
-   print "one path\n";
+   if ($did eq 'added') {
+   printf(__n("added %d path\n", "added %d paths\n",
+  $cnt), $cnt);
+   } elsif ($did eq 'updated') {
+   printf(__n("updated %d path\n", "updated %d paths\n",
+  $cnt), $cnt);
+   } elsif ($did eq 'reverted') {
+   printf(__n("reverted %d path\n", "reverted %d paths\n",
+  $cnt), $cnt);
+   } else {
+   printf(__n("touched %d path\n", "touched %d paths\n",
+  $cnt), $cnt);
}
 }
 
@@ -1423,7 +1429,8 @@ sub patch_update_file {
} elsif (0 < $response && $response <= $num) {
$ix = $response - 1;
} else {
-   error_msg "Sorry, only $num hunks 
available.\n";
+   error_msg sprintf(__n("Sorry, only %d 
hunk available.\n",
+ "Sorry, only %d 
hunks available.\n", $num), $num);
}
next;
}
@@ -1518,8 +1525,10 @@ sub patch_update_file {
elsif ($other =~ /s/ && $line =~ /^s/) {
my @split = split_hunk($hunk[$ix]{TEXT}, 
$hunk[$ix]{DISPLAY});
if (1 < @split) {
-   print colored $header_color, "Split 
into ",
-   scalar(@split), " hunks.\n";
+   print colored $header_color, sprintf(
+   __n("Split into %d hunk.\n",
+   "Split into %d hunks.\n",
+   scalar(@split)), 
scalar(@split));
}
splice (@hunk, $ix, 1, @split);
$num = scalar @hunk;
diff --git a/perl/Git/I18N.pm b/perl/Git/I18N.pm
index f889fd6..3f7ac25 100644
--- a/perl/Git/I18N.pm
+++ b/perl/Git/I18N.pm
@@ -13,7 +13,7 @@ BEGIN {
}
 }
 
-our @EXPORT = qw(__);
+our @EXPORT = qw(__ __n);
 our @EXPORT_OK = @EXPORT;
 
 sub __bootstrap_locale_messages {
@@ -44,6 +44,7 @@ BEGIN
eval {
__bootstrap_locale_messages();
*__ = \&Locale::Messages::gettext;
+   *__n = \&Locale::Messages::ngettext;
1;
} or do {
# Tell test.pl that we couldn't load the gettext library.
@@ -51,6 +52,7 @@ BEGIN
 
# Just a fall-through no-op
*__ = sub ($) { $_[0] };
+   *__n = sub ($$$) { $_[2] == 1 ? $_[0] : $_[1] };
};
 }
 
@@ -70,6 +72,8 @@ Git::I18N - Perl interface to Git's Gettext localizations
 
printf __("The following error occurred: %s\n"), $error;
 
+   printf __n("commited %d file", "commited %d files", $files), $files;
+
 =head1 DESCRIPTION
 
 Git's internal Perl interface to gettext via L. If
@@ -87,6 +91,9 @@ it.
 L's gettext function if all goes well, otherwise our
 passthrough fallback function.
 
+=head2 __n($$$)
+L's ngettext function or passthrough fallback function.
+
 =head1 AUTHOR
 
 Evar ArnfjErE Bjarmason 
diff --git a/t/t0

[PATCH v4 06/14] i18n: add--interactive: mark patch prompt for translation

[Vasco Almeida]

Mark prompt message assembled in place for translation, unfolding each
use case for each entry in the %patch_modes hash table.

Previously, this script relied on whether $patch_mode was set to run the
command patch_update_cmd() or show status and loop the main loop. Now,
it uses $cmd to indicate we must run patch_update_cmd() and $patch_mode
is used to tell which flavor of the %patch_modes are we on.  This is
introduced in order to be able to mark and unfold the message prompt
knowing in which context we are.

The tracking of context was done previously by point %patch_mode_flavour
hash table to the correct entry of %patch_modes, focusing only on value
of %patch_modes. Now, we are also interested in the key ('staged',
'stash', 'checkout_head', ...).

Signed-off-by: Vasco Almeida 
---
 Makefile  |  2 +-
 git-add--interactive.perl | 54 ---
 perl/Git/I18N.pm  | 10 -
 t/t0202/test.pl   |  5 -
 4 files changed, 60 insertions(+), 11 deletions(-)

diff --git a/Makefile b/Makefile
index 4ef0344..9dc95cb 100644
--- a/Makefile
+++ b/Makefile
@@ -2112,7 +2112,7 @@ XGETTEXT_FLAGS_C = $(XGETTEXT_FLAGS) --language=C \
 XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \
--keyword=gettextln --keyword=eval_gettextln
 XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --language=Perl \
-   --keyword=__ --keyword="__n:1,2"
+   --keyword=__ --keyword=N__ --keyword="__n:1,2"
 LOCALIZED_C = $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
 LOCALIZED_SH = $(SCRIPT_SH)
 LOCALIZED_SH += git-parse-remote.sh
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index cd61783..b7d382b 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -93,6 +93,7 @@ sub colored {
 }
 
 # command line options
+my $cmd;
 my $patch_mode;
 my $patch_mode_revision;
 
@@ -173,7 +174,8 @@ my %patch_modes = (
},
 );
 
-my %patch_mode_flavour = %{$patch_modes{stage}};
+$patch_mode = 'stage';
+my %patch_mode_flavour = %{$patch_modes{$patch_mode}};
 
 sub run_cmd_pipe {
if ($^O eq 'MSWin32') {
@@ -1311,6 +1313,44 @@ sub display_hunks {
return $i;
 }
 
+my %patch_update_prompt_modes = (
+   stage => {
+   mode => N__("Stage mode change [y,n,q,a,d,/%s,?]? "),
+   deletion => N__("Stage deletion [y,n,q,a,d,/%s,?]? "),
+   hunk => N__("Stage this hunk [y,n,q,a,d,/%s,?]? "),
+   },
+   stash => {
+   mode => N__("Stash mode change [y,n,q,a,d,/%s,?]? "),
+   deletion => N__("Stash deletion [y,n,q,a,d,/%s,?]? "),
+   hunk => N__("Stash this hunk [y,n,q,a,d,/%s,?]? "),
+   },
+   reset_head => {
+   mode => N__("Unstage mode change [y,n,q,a,d,/%s,?]? "),
+   deletion => N__("Unstage deletion [y,n,q,a,d,/%s,?]? "),
+   hunk => N__("Unstage this hunk [y,n,q,a,d,/%s,?]? "),
+   },
+   reset_nothead => {
+   mode => N__("Apply mode change to index [y,n,q,a,d,/%s,?]? "),
+   deletion => N__("Apply deletion to index [y,n,q,a,d,/%s,?]? "),
+   hunk => N__("Apply this hunk to index [y,n,q,a,d,/%s,?]? "),
+   },
+   checkout_index => {
+   mode => N__("Discard mode change from worktree 
[y,n,q,a,d,/%s,?]? "),
+   deletion => N__("Discard deletion from worktree 
[y,n,q,a,d,/%s,?]? "),
+   hunk => N__("Discard this hunk from worktree [y,n,q,a,d,/%s,?]? 
"),
+   },
+   checkout_head => {
+   mode => N__("Discard mode change from index and worktree 
[y,n,q,a,d,/%s,?]? "),
+   deletion => N__("Discard deletion from index and worktree 
[y,n,q,a,d,/%s,?]? "),
+   hunk => N__("Discard this hunk from index and worktree 
[y,n,q,a,d,/%s,?]? "),
+   },
+   checkout_nothead => {
+   mode => N__("Apply mode change to index and worktree 
[y,n,q,a,d,/%s,?]? "),
+   deletion => N__("Apply deletion to index and worktree 
[y,n,q,a,d,/%s,?]? "),
+   hunk => N__("Apply this hunk to index and worktree 
[y,n,q,a,d,/%s,?]? "),
+   },
+);
+
 sub patch_update_file {
my $quit = 0;
my ($ix, $num);
@@ -1383,12 +1423,9 @@ sub patch_update_file {
for (@{$hunk[$ix]{DISPLAY}}) {
print;
}
-   print colored $prompt_color, $patch_mode_flavour{VERB},
- ($hunk[$ix]{TYPE} eq 'mode' ? ' mode change' :
-  $hunk[$ix]{TYPE} eq 'deletion' ? ' deletion' :
-  ' this hunk'),
- $patch_mode_flavour{TARGET},
- " [y,n,q,a,d,/$other,?]? ";
+   print colored $prompt_color,
+   
sprintf(__($patch_update_prompt_modes{$patch_mode}{$hunk[$ix]{TYPE}}), $other);
+
my $line = prompt_single_character;
last unless defined $line;
if ($line) {
@@ -1644,6 

[PATCH v4 09/14] i18n: add--interactive: remove %patch_modes entries

[Vasco Almeida]

Remove unnecessary entries from %patch_modes. After the i18n conversion,
these entries are not used anymore.

Signed-off-by: Vasco Almeida 
---
 git-add--interactive.perl | 21 -
 1 file changed, 21 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 861f7b0..d7a8e0d 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -106,9 +106,6 @@ my %patch_modes = (
DIFF => 'diff-files -p',
APPLY => sub { apply_patch 'apply --cached', @_; },
APPLY_CHECK => 'apply --cached',
-   VERB => 'Stage',
-   TARGET => '',
-   PARTICIPLE => 'staging',
FILTER => 'file-only',
IS_REVERSE => 0,
},
@@ -116,9 +113,6 @@ my %patch_modes = (
DIFF => 'diff-index -p HEAD',
APPLY => sub { apply_patch 'apply --cached', @_; },
APPLY_CHECK => 'apply --cached',
-   VERB => 'Stash',
-   TARGET => '',
-   PARTICIPLE => 'stashing',
FILTER => undef,
IS_REVERSE => 0,
},
@@ -126,9 +120,6 @@ my %patch_modes = (
DIFF => 'diff-index -p --cached',
APPLY => sub { apply_patch 'apply -R --cached', @_; },
APPLY_CHECK => 'apply -R --cached',
-   VERB => 'Unstage',
-   TARGET => '',
-   PARTICIPLE => 'unstaging',
FILTER => 'index-only',
IS_REVERSE => 1,
},
@@ -136,9 +127,6 @@ my %patch_modes = (
DIFF => 'diff-index -R -p --cached',
APPLY => sub { apply_patch 'apply --cached', @_; },
APPLY_CHECK => 'apply --cached',
-   VERB => 'Apply',
-   TARGET => ' to index',
-   PARTICIPLE => 'applying',
FILTER => 'index-only',
IS_REVERSE => 0,
},
@@ -146,9 +134,6 @@ my %patch_modes = (
DIFF => 'diff-files -p',
APPLY => sub { apply_patch 'apply -R', @_; },
APPLY_CHECK => 'apply -R',
-   VERB => 'Discard',
-   TARGET => ' from worktree',
-   PARTICIPLE => 'discarding',
FILTER => 'file-only',
IS_REVERSE => 1,
},
@@ -156,9 +141,6 @@ my %patch_modes = (
DIFF => 'diff-index -p',
APPLY => sub { apply_patch_for_checkout_commit '-R', @_ },
APPLY_CHECK => 'apply -R',
-   VERB => 'Discard',
-   TARGET => ' from index and worktree',
-   PARTICIPLE => 'discarding',
FILTER => undef,
IS_REVERSE => 1,
},
@@ -166,9 +148,6 @@ my %patch_modes = (
DIFF => 'diff-index -R -p',
APPLY => sub { apply_patch_for_checkout_commit '', @_ },
APPLY_CHECK => 'apply',
-   VERB => 'Apply',
-   TARGET => ' to index and worktree',
-   PARTICIPLE => 'applying',
FILTER => undef,
IS_REVERSE => 0,
},
-- 
2.7.4

[PATCH v4 08/14] i18n: add--interactive: mark edit_hunk_manually message for translation

[Vasco Almeida]

Mark message of edit_hunk_manually displayed in the editing file when
user chooses 'e' option.  The message had to be unfolded to allow
translation of the $participle verb.

Some messages end up being exactly the same for some uses cases, but
left it for easier change in the future, e.g., wanting to change wording
of one particular use case.

Signed-off-by: Vasco Almeida 
---
 git-add--interactive.perl | 45 ++---
 1 file changed, 34 insertions(+), 11 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 045b847..861f7b0 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1058,6 +1058,30 @@ sub color_diff {
} @_;
 }
 
+my %edit_hunk_manually_modes = (
+   stage => N__(
+"# If the patch applies cleanly, the edited hunk will immediately be
+# marked for staging."),
+   stash => N__(
+"# If the patch applies cleanly, the edited hunk will immediately be
+# marked for stashing."),
+   reset_head => N__(
+"# If the patch applies cleanly, the edited hunk will immediately be
+# marked for unstaging."),
+   reset_nothead => N__(
+"# If the patch applies cleanly, the edited hunk will immediately be
+# marked for applying."),
+   checkout_index => N__(
+"# If the patch applies cleanly, the edited hunk will immediately be
+# marked for discarding"),
+   checkout_head => N__(
+"# If the patch applies cleanly, the edited hunk will immediately be
+# marked for discarding."),
+   checkout_nothead => N__(
+"# If the patch applies cleanly, the edited hunk will immediately be
+# marked for applying."),
+);
+
 sub edit_hunk_manually {
my ($oldtext) = @_;
 
@@ -1065,22 +1089,21 @@ sub edit_hunk_manually {
my $fh;
open $fh, '>', $hunkfile
or die sprintf(__("failed to open hunk edit file for writing: 
%s"), $!);
-   print $fh "# Manual hunk edit mode -- see bottom for a quick guide\n";
+   print $fh __("# Manual hunk edit mode -- see bottom for a quick 
guide\n");
print $fh @$oldtext;
-   my $participle = $patch_mode_flavour{PARTICIPLE};
my $is_reverse = $patch_mode_flavour{IS_REVERSE};
my ($remove_plus, $remove_minus) = $is_reverse ? ('-', '+') : ('+', 
'-');
-   print $fh <

[PATCH v4 12/14] i18n: send-email: mark warnings and errors for translation

[Vasco Almeida]

Mark warnings, errors and other messages for translation.

Signed-off-by: Vasco Almeida 
---
 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_encryptio

[PATCH v4 10/14] i18n: add--interactive: mark status words for translation

[Vasco Almeida]

Mark words 'nothing', 'unchanged' and 'binary' used to display what has
been staged or not, in "git add -i" status command.

Alternatively one could mark N__('nothing') no-op in order to
xgettext(1) extract the string and then trigger the translation at run
time only with __($print->{FILE}), but that has the side effect of triggering
retrieval of translations for the changes indicator too (e.g. +2/-1)
which may or may not be a problem.

To avoid that potential problem, mark only where there is certain to
trigger translation only of those words but in this case we must also
retrieve the translation for the eq tests, since the value assigned was
of the translation, not the English source.

Signed-off-by: Vasco Almeida 
---
 git-add--interactive.perl | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index d7a8e0d..4754104 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -295,7 +295,7 @@ sub list_modified {
my ($change, $bin);
$file = unquote_path($file);
if ($add eq '-' && $del eq '-') {
-   $change = 'binary';
+   $change = __('binary');
$bin = 1;
}
else {
@@ -304,7 +304,7 @@ sub list_modified {
$data{$file} = {
INDEX => $change,
BINARY => $bin,
-   FILE => 'nothing',
+   FILE => __('nothing'),
}
}
elsif (($adddel, $file) =
@@ -320,7 +320,7 @@ sub list_modified {
$file = unquote_path($file);
my ($change, $bin);
if ($add eq '-' && $del eq '-') {
-   $change = 'binary';
+   $change = __('binary');
$bin = 1;
}
else {
@@ -340,7 +340,7 @@ sub list_modified {
$file = unquote_path($2);
if (!exists $data{$file}) {
$data{$file} = +{
-   INDEX => 'unchanged',
+   INDEX => __('unchanged'),
BINARY => 0,
};
}
@@ -355,10 +355,10 @@ sub list_modified {
 
if ($only) {
if ($only eq 'index-only') {
-   next if ($it->{INDEX} eq 'unchanged');
+   next if ($it->{INDEX} eq __('unchanged'));
}
if ($only eq 'file-only') {
-   next if ($it->{FILE} eq 'nothing');
+   next if ($it->{FILE} eq __('nothing'));
}
}
push @return, +{
-- 
2.7.4

[PATCH v4 11/14] i18n: send-email: mark strings for translation

[Vasco Almeida]

Mark strings often displayed to the user for translation.

Signed-off-by: Vasco Almeida 
---
 git-send-email.perl | 52 ++--
 1 file changed, 30 insertions(+), 22 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index da81be4..78eb59b 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -28,6 +28,7 @@ use File::Temp qw/ tempdir tempfile /;
 use File::Spec::Functions qw(catfile);
 use Error qw(:try);
 use Git;
+use Git::I18N;
 
 Getopt::Long::Configure qw/ pass_through /;
 
@@ -797,12 +798,12 @@ foreach my $f (@files) {
 }
 
 if (!defined $auto_8bit_encoding && scalar %broken_encoding) {
-   print "The following files are 8bit, but do not declare " .
-   "a Content-Transfer-Encoding.\n";
+   print __("The following files are 8bit, but do not declare " .
+"a Content-Transfer-Encoding.\n");
foreach my $f (sort keys %broken_encoding) {
print "$f\n";
}
-   $auto_8bit_encoding = ask("Which 8bit encoding should I declare 
[UTF-8]? ",
+   $auto_8bit_encoding = ask(__("Which 8bit encoding should I declare 
[UTF-8]? "),
  valid_re => qr/.{4}/, confirm_only => 1,
  default => "UTF-8");
 }
@@ -829,7 +830,7 @@ if (defined $sender) {
 # But it's a no-op to run sanitize_address on an already sanitized address.
 $sender = sanitize_address($sender);
 
-my $to_whom = "To whom should the emails be sent (if anyone)?";
+my $to_whom = __("To whom should the emails be sent (if anyone)?");
 my $prompting = 0;
 if (!@initial_to && !defined $to_cmd) {
my $to = ask("$to_whom ",
@@ -859,7 +860,7 @@ sub expand_one_alias {
 
 if ($thread && !defined $initial_reply_to && $prompting) {
$initial_reply_to = ask(
-   "Message-ID to be used as In-Reply-To for the first email (if 
any)? ",
+   __("Message-ID to be used as In-Reply-To for the first email 
(if any)? "),
default => "",
valid_re => qr/\@.*\./, confirm_only => 1);
 }
@@ -918,7 +919,10 @@ sub validate_address {
my $address = shift;
while (!extract_valid_address($address)) {
print STDERR "error: unable to extract a valid address from: 
$address\n";
-   $_ = ask("What to do with this address? ([q]uit|[d]rop|[e]dit): 
",
+   # TRANSLATORS: Make sure to include [q] [d] [e] in your
+   # translation. The program will only accept English input
+   # at this point.
+   $_ = ask(__("What to do with this address? 
([q]uit|[d]rop|[e]dit): "),
valid_re => qr/^(?:quit|q|drop|d|edit|e)/i,
default => 'q');
if (/^d/i) {
@@ -1293,17 +1297,21 @@ Message-Id: $message_id
if ($needs_confirm eq "inform") {
$confirm_unconfigured = 0; # squelch this message for 
the rest of this run
$ask_default = "y"; # assume yes on EOF since user 
hasn't explicitly asked for confirmation
-   print "The Cc list above has been expanded by 
additional\n";
-   print "addresses found in the patch commit message. 
By default\n";
-   print "send-email prompts before sending whenever 
this occurs.\n";
-   print "This behavior is controlled by the 
sendemail.confirm\n";
-   print "configuration setting.\n";
-   print "\n";
-   print "For additional information, run 'git 
send-email --help'.\n";
-   print "To retain the current behavior, but squelch 
this message,\n";
-   print "run 'git config --global sendemail.confirm 
auto'.\n\n";
+   print __(
+"The Cc list above has been expanded by additional
+addresses found in the patch commit message. By default
+send-email prompts before sending whenever this occurs.
+This behavior is controlled by the sendemail.confirm
+configuration setting.
+
+For additional information, run 'git send-email --help'.
+To retain the current behavior, but squelch this message,
+run 'git config --global sendemail.confirm auto'."), "\n\n";
}
-   $_ = ask("Send this email? ([y]es|[n]o|[q]uit|[a]ll): ",
+   # TRANSLATORS: Make sure to include [y] [n] [q] [a] in your
+   # translation. The program will only accept English input
+   # at this point.
+   $_ = 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 $_;
@@ -1405,7 +1413,7 @@ Message-Id: $message_id
if ($quiet) {

[PATCH v4 13/14] i18n: send-email: mark string with interpolation for translation

[Vasco Almeida]

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 
---
 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(< $repo->repo_path()) 
:
tempfile(".gitsendemail.msg.XX", 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

[PATCH v4 14/14] i18n: difftool: mark warnings for translation

[Vasco Almeida]

Signed-off-by: Vasco Almeida 
---
 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  given for --tool=\n";
+   print __("No  given for --tool=\n");
usage(1);
}
}
@@ -346,7 +347,7 @@ sub main
if (length($opts{extcmd}) > 0) {
$ENV{GIT_DIFFTOOL_EXTCMD} = $opts{extcmd};
} else {
-   print "No  given for --extcmd=\n";
+   print __("No  given for --extcmd=\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

Re: [PATCH v2] gpg-interface: use more status letters

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 06.10.2016 23:43:
> Junio C Hamano  writes:
> 
>> Michael J Gruber  writes:
>>
>>> Also, I'm open to using another letter for EXPKEYSIG but couldn't decide
>>> between 'Y', 'Z', 'K'. 'K' could be confused with REVKEYSIG, I'm afraid.
>>> 'Y' is next to 'X' and contained in 'KEY', it would be my first choice.
>>
>> Sounds good enough to me.  Thanks.
> 
> I really do not want to leave too many topics listed in the "What's
> cooking" report to be in undecided / waiting state.  How about
> squashing this in, with a fully updated log message to replace?

Sorry, this got "lost in vacation". Before that, I was looking for an
easy way to test expired signatures, but gpg1 and gpg2 behave somewhat
differently in that respect (2 does not allow to create already expired
signatures).

Is there anything I should or could do now?

Michael

> -- >8 --
> From: Michael J Gruber 
> Date: Wed, 28 Sep 2016 16:24:13 +0200
> Subject: [PATCH] SQUASH: gpg-interface: use more status letters
> 
> According to gpg2's doc/DETAILS:
> 
> For each signature only one of the codes GOODSIG, BADSIG,
> EXPSIG, EXPKEYSIG, REVKEYSIG or ERRSIG will be emitted.
> 
> gpg1 ("classic") behaves the same (although doc/DETAILS differs).
> 
> Currently, we parse gpg's status output for GOODSIG, BADSIG and
> trust information and translate that into status codes G, B, U, N
> for the %G?  format specifier.
> 
> git-verify-* returns success in the GOODSIG case only. This is
> somewhat in disagreement with gpg, which considers the first 5 of
> the 6 above as VALIDSIG, but we err on the very safe side.
> 
> Introduce additional status codes E, X, Y, R for ERRSIG, EXPSIG,
> EXPKEYSIG, and REVKEYSIG so that a user of %G? gets more information
> about the absence of a 'G' on first glance.
> 
> Requested-by: Alex 
> Signed-off-by: Michael J Gruber 
> Signed-off-by: Junio C Hamano 
> ---
>  Documentation/pretty-formats.txt | 3 ++-
>  gpg-interface.c  | 2 +-
>  pretty.c | 1 +
>  3 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/pretty-formats.txt 
> b/Documentation/pretty-formats.txt
> index c28ff2b919..179c9389aa 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -146,7 +146,8 @@ endif::git-rev-list[]
>  - '%G?': show "G" for a good (valid) signature,
>"B" for a bad signature,
>"U" for a good signature with unknown validity,
> -  "X" for a good expired signature, or good signature made by an expired key,
> +  "X" for a good signature that has expired,
> +  "Y" for a good signature made by an expired key,
>"R" for a good signature made by a revoked key,
>"E" if the signature cannot be checked (e.g. missing key)
>and "N" for no signature
> diff --git a/gpg-interface.c b/gpg-interface.c
> index 6999e7b469..e44cc27da1 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -35,7 +35,7 @@ static struct {
>   { 'U', "\n[GNUPG:] TRUST_UNDEFINED" },
>   { 'E', "\n[GNUPG:] ERRSIG "},
>   { 'X', "\n[GNUPG:] EXPSIG "},
> - { 'X', "\n[GNUPG:] EXPKEYSIG "},
> + { 'Y', "\n[GNUPG:] EXPKEYSIG "},
>   { 'R', "\n[GNUPG:] REVKEYSIG "},
>  };
>  
> diff --git a/pretty.c b/pretty.c
> index 39a36cd825..f98b271069 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1236,6 +1236,7 @@ static size_t format_commit_one(struct strbuf *sb, /* 
> in UTF-8 */
>   case 'U':
>   case 'N':
>   case 'X':
> + case 'Y':
>   case 'R':
>   strbuf_addch(sb, c->signature_check.result);
>   }
> 

Re: Bug? git worktree fails with master on bare repo

[Michael Tutty]

> If I create a bare _clone_, then "HEAD" could be detached, or point to some 
> branch, depending on where "HEAD" is in the source repo

I didn't mean a clone, I meant a brand-new (bare) repo.  Then I would
clone it somewhere, add commits and branches, and push them to the
bare repo.


> If source repo's HEAD is "master", I got the same behavior (worktree add 
> fails)

So if it's possible for a bare repo to have HEAD pointing at master,
is there a safe way for me to change this (e.g., as a cleanup step
before doing my actual merge process)?

On Mon, Oct 10, 2016 at 4:45 AM, Duy Nguyen  wrote:
> On Sun, Oct 9, 2016 at 8:42 PM, Michael Tutty  wrote:
>> Dennis,
>> Thanks for the great response, and for spending time on my issue.
>> I'll try that first patch and see what happens.
>>
>> In the meantime, it got weirder...
>>
>> I created a brand-new (bare) repo
>
> Elaboration needed here. If I create a bare _clone_, then "HEAD" could
> be detached, or point to some branch, depending on where "HEAD" is in
> the source repo. If source repo's HEAD is "master", I got the same
> behavior (worktree add fails). If it's detached or points to some
> other branch, it's ok. If this is "git init --bare" then I got "fatal:
> invalid reference: master".
>
>> and was able to git add worktree
>> /path master.  I was able to do this repeatedly, even using the
>> worktree to merge other branches to master.  I didn't find any
>> condition or step that caused some kind of orphan master work tree,
>> which was what I thought the underlying problem might be.
> --
> Duy



-- 
Michael Tutty, CTO

e: mtu...@gforgegroup.com
t: @mtutty, @gforgegroup
v: 515-789-0772
w: http://gforgegroup.com, http://gforge.com

Re: [PATCH] t4014-format-patch: Adjust git_version regex to better handle distro changes to DEF_VER

[Jeff King]

On Sun, Oct 09, 2016 at 07:53:23PM -0700, Jeremy Huddleston Sequoia wrote:

> Subject: Re: [PATCH] t4014-format-patch: Adjust git_version regex to better
>  handle distro changes to DEF_VER
>
> Regressed-in: 480871e09ed2e5275b4ba16b278681e5a8c122ae
> Signed-off-by: Jeremy Huddleston Sequoia 

I see there was a discussion elsewhere on the list about exactly what
you are putting into DEF_VAR that causes the problem. Perhaps the commit
message here would be a good place to mention that, why the current
regex breaks it, and why your new version fixes not only it, but other
possible values of DEF_VAR.

-Peff

git filter-branch --subdirectory-filter not working as expected, history of other folders is preserved

[Seaders Oloinsigh]

From

https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History

Now your new project root is what was in the trunk subdirectory each time.
> Git will also automatically remove commits that did not affect the
> subdirectory.
>

We have a git repository that looks like

sdk/
android/
ios/
unity/
windows/

Which we'd like to split into 4 repositories, 1 for each platform.  To
start this process (for splitting android out), I ran,

git filter-branch -f --prune-empty --subdirectory-filter android -- --all

Which rewrote a ton of history and commits, and looked like it worked, but
on closer inspection had left a ton of history behind.

If I run

git log --all -- unity/

It returns a list of commits that happened in the unity/ subfolder of the
original root.

commit c4ea2797...
> Author: tom... 
> Date:   Thu Feb 25 14:20:59 2016 +
>
> kick off build
>

> ...
>


Which only contains an edit to a file with path "unity/tom" relative to the
root *before* the filter-branch, doesn't exist any more, and from my
understanding of the docs, shouldn't have been taken across.

It's also not an isolated instance, if I run the same checks against
"ios/", "windows/", any file that existed in a folder other than "android"
to the old root, that history is also preserved.

I've just about resorted to running multiple other, explicit filters to
remove all references to those other folders, but it seems like this would
be redoing the job that I understood git filter-branch should have been
doing.

Any help in this regard is greatly appreciated.

seaders.

Re: %C(auto) not working as expected

[Jeff King]

On Mon, Oct 10, 2016 at 04:26:18PM +0700, Duy Nguyen wrote:

> > If we do a revamp of the pretty-formats to bring them more in line with
> > ref-filter (e.g., something like "%(color:red)") maybe that would be an
> > opportunity to make minor adjustments. Though, hmm, it looks like
> > for-each-ref already knows "%(color:red)", and it's unconditional.
> >  So perhaps we would need to go through some deprecation period or
> > other transition.
> 
> We could add some new tag to change the behavior of all following %C
> tags. Something like %C(tty) maybe (probably a bad name), then
> discourage the use if "%C(auto" for terminal detection?

Yeah, adding a "%C(enable-auto-color)" or something would be backwards
compatible and less painful than using "%C(auto)" everywhere. I do
wonder if anybody actually _wants_ the "always show color, even if
--no-color" behavior. I'm having trouble thinking of a good use for it.

IOW, I'm wondering if anyone would disagree that the current behavior is
simply buggy. Reading the thread at:

  http://public-inbox.org/git/7v4njkmq07@alter.siamese.dyndns.org/

I don't really see any compelling reason against it (there was some
question of which config to use, but we already answered that with
"%C(auto)", and use the value from the pretty_ctx).

-Peff

Re: How to watch a mailing list & repo for patches which affect a certain area of code?

[Eric Wong]

Ian Kelling  wrote:
> I've got patches in various projects, and I don't have time to keep up
> with the mailing list, but I'd like to help out with maintenance of that
> code, or the functions/files it touches. People don't cc me. I figure I
> could filter the list, test patches submitted, commits made, mentions of
> files/functions, build filters based on the code I have in the repo even
> if it's been moved or changed subsequently. I'm wondering what other
> people have implemented already for automation around this, or general
> thoughts. Web search is not showing me much.

For the mailing list, you can try following an Atom feed for
any specific search query.
https://public-inbox.org/git/?q=FILE_OR_FUNCTION&x=A
(the "x=A" makes it an Atom feed)

It's all still a work-in-progress but there'll be better
filename and diff handling in public-inbox soon.


It's all AGPL and the data is only sourced from this mailing
list, so 100% reproducible as I'm incapable of running a
reliable server :>   Clone instructions at the bottom of
https://public-inbox.org/git/

[PATCH] pretty: respect color settings for %C placeholders

[Jeff King]

On Mon, Oct 10, 2016 at 10:28:18AM -0400, Jeff King wrote:

> > We could add some new tag to change the behavior of all following %C
> > tags. Something like %C(tty) maybe (probably a bad name), then
> > discourage the use if "%C(auto" for terminal detection?
> 
> Yeah, adding a "%C(enable-auto-color)" or something would be backwards
> compatible and less painful than using "%C(auto)" everywhere. I do
> wonder if anybody actually _wants_ the "always show color, even if
> --no-color" behavior. I'm having trouble thinking of a good use for it.
> 
> IOW, I'm wondering if anyone would disagree that the current behavior is
> simply buggy. Reading the thread at:
> 
>   http://public-inbox.org/git/7v4njkmq07@alter.siamese.dyndns.org/
> 
> I don't really see any compelling reason against it (there was some
> question of which config to use, but we already answered that with
> "%C(auto)", and use the value from the pretty_ctx).

So here's what a patch to do that would look like. I admit that "I can't
think of a good use" does not mean there _isn't_ one, but perhaps by
posting this, it might provoke other people to think on it, too. And if
nobody can come up with, maybe it's a good idea.

-- >8 --
Subject: pretty: respect color settings for %C placeholders

The color placeholders have traditionally been
unconditional, showing colors even when git is not otherwise
configured to do so. This was not so bad for their original
use, which was on the command-line (and the user could
decide at that moment whether to add colors or not). But
these days we have configured formats via pretty.*, and
those should operate in multiple contexts.

In 3082517 (log --format: teach %C(auto,black) to respect
color config, 2012-12-17), we gave an extended placeholder
that could be used to accomplish this. But it's rather
clunky to use, because you have to specify it individually
for each color (and their matching resets) in the format.
We shied away from just switching the default to auto,
because it is technically breaking backwards compatibility.

However, there's not really a use case for unconditional
colors. The most plausible reason you would want them
unconditional is to redirect "git log" output to a file. But
there, the right answer is --color=always, as it does the
right thing both with custom user-format colors and
git-generated colors.

So let's switch to the more useful default. In the
off-chance that somebody really does find a use for
unconditional colors without wanting to enable the rest of
git's colors, we can provide %C(always,...) to enable the
old behavior.

Signed-off-by: Jeff King 
---
The tests unsurprisingly needed updating, as we're breaking the old
behavior. The diff is easier to read read with "-w".

 Documentation/pretty-formats.txt | 13 +++---
 pretty.c | 19 +---
 t/t6006-rev-list-format.sh   | 94 
 3 files changed, 70 insertions(+), 56 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index a942d57..7aa1a8b 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -167,11 +167,14 @@ endif::git-rev-list[]
 - '%Cblue': switch color to blue
 - '%Creset': reset color
 - '%C(...)': color specification, as described in color.branch.* config option;
-  adding `auto,` at the beginning will emit color only when colors are
-  enabled for log output (by `color.diff`, `color.ui`, or `--color`, and
-  respecting the `auto` settings of the former if we are going to a
-  terminal). `auto` alone (i.e. `%C(auto)`) will turn on auto coloring
-  on the next placeholders until the color is switched again.
+  By default, colors are shown only when enabled for log output (by
+  `color.diff`, `color.ui`, or `--color`, and respecting the `auto`
+  settings of the former if we are going to a terminal). `%C(auto,...)`
+  is accepted as a historical synonym for the default. Specifying
+  `%C(always,...) will show the colors always, even when colors are not
+  otherwise enabled (to enable this behavior for the whole format, use
+  `--color=always`). `auto` alone (i.e. `%C(auto)`) will turn on auto
+  coloring on the next placeholders until the color is switched again.
 - '%m': left (`<`), right (`>`) or boundary (`-`) mark
 - '%n': newline
 - '%%': a raw '%'
diff --git a/pretty.c b/pretty.c
index 25efbca..73e58b5 100644
--- a/pretty.c
+++ b/pretty.c
@@ -965,22 +965,31 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 
*/
 
if (!end)
return 0;
-   if (skip_prefix(begin, "auto,", &begin)) {
+
+   if (!skip_prefix(begin, "always,", &begin)) {
if (!want_color(c->pretty_ctx->color))
return end - placeholder + 1;
}
+
+   /* this is a historical noop */
+   skip_prefix(begin, "auto,", &begin);
+
if (color_parse_

Re: git merge deletes my changes

[Jeff King]

On Mon, Oct 10, 2016 at 09:39:13AM +, Eduard Egorov wrote:

> A week ago, I've reset a state of 'ceph-ansible' folder in %current%
> branch with code from corresponding branch (that tracks an upstream
> from github):
> 
> # git read-tree --prefix=ceph-ansible/ -u ceph_ansible

This pulls in the subtree files, but there's no actual relationship with
the commit history in ceph_ansible.

So later...

> Then I've committed several changes, including:
> 
> 1. Renamed file and commited:
> # git mv site.yml.sample site.yml
> 
> 2. Made some changes and committed
> 
> 3. Pulled updates from original branch by:
> # git merge -s subtree --squash ceph_ansible
> 
> It said:
> Auto-merging ceph-ansible/site.yml.sample
> blablabla
> Squash commit -- not updating HEAD
> Automatic merge went well; stopped before committing as requested

When you merge from ceph_ansible, there is no shared history, and git
uses the empty tree as a common ancestor. It looks like the other side
added site.yml.sample, for instance, because that is a change from the
empty tree.

> A post on SO: 
> http://stackoverflow.com/questions/39954265/git-merge-deletes-my-changes

As you noted on SO, modern git disallows merges of unrelated history by
default, because it's usually a mistake to do so.

If you are doing repeated merges into the subtree, you need to somehow
tell git how the histories are related. The obvious answer is to do a
"git merge -s ours ceph_ansible" after your initial read-tree, so that
git knows you've pulled in the changes up to that point. But I'd guess
from your use of "--squash" that you don't want to carry the
ceph_ansible history in your project.

So you need to record the original upstream commit somewhere (probably
in the commit message when you commit the read-tree result), and then
ask git to use that as the merge-base during subsequent merges (which
will require using plumbing codes, as git-merge wants to compute the
merge base itself).  I believe the git-subtree command (in
contrib/subtree of git.git) handles this use case, but I haven't used it
myself.

-Peff

Re: git filter-branch --subdirectory-filter not working as expected, history of other folders is preserved

[Jeff King]

On Mon, Oct 10, 2016 at 02:42:36PM +0100, Seaders Oloinsigh wrote:

> We have a git repository that looks like
> 
> sdk/
> android/
> ios/
> unity/
> windows/
> 
> Which we'd like to split into 4 repositories, 1 for each platform.  To
> start this process (for splitting android out), I ran,
> 
> git filter-branch -f --prune-empty --subdirectory-filter android -- --all

OK, so that should rewrite each ref to have only the contents of the
"android" directory at the top-level.

Note that filter-branch saves a copy of the old refs in refs/original.

> Which rewrote a ton of history and commits, and looked like it worked, but
> on closer inspection had left a ton of history behind.
> 
> If I run
> 
> git log --all -- unity/
> 
> It returns a list of commits that happened in the unity/ subfolder of the
> original root.

Here you asked for "--all", which includes refs/original. So you are
seeing the original, unwritten commits (and none of your new ones, of
course, because they do not have a unity/ directory!).

Try:

  git log --all --source -- unity

to see which ref each commit is coming from.

Or try:

  git log --branches --tags -- unity

to confirm that your branches and tags do not include that path.

Or just:

  git for-each-ref --format='delete %(refname)' refs/original |
  git update-ref --stdin

to get rid of the backup refs entirely.

-Peff

Re: git filter-branch --subdirectory-filter not working as expected, history of other folders is preserved

[Seaders Oloinsigh]

Thanks for the reply, Jeff.

Clearing the backups of the branches, those starting with
"refs/original" has gotten me closer, but I also needed to do that
with "refs/tags" as well, or change my filter-branch command to,

  git filter-branch -f --prune-empty --tag-name-filter cat
--subdirectory-filter android -- --all

I still have remnants of that other history, though.

Due to the structure of this repo, it looks like there are some
branches that never had anything to do with the android/ subdirectory,
so they're not getting wiped out.  My branch is in a better state to
how I want it, but still, if I run your suggestion,

  git log --all --source -- unity/

I get output like

> commit 4853c... refs/heads/unity-sdk-3_1_3
> Author: serg... 
> Date:   Thu Sep 11 16:30:01 2014 +0100
>
>Started 3.1.3

Which is basically logs of branches which contain only edits within
the unity/ subdirectory of the original root.  There are other
branches like that for the other platforms / subdirectories of the
original root, which if that is the case, I would consider
filter-branch with the subdirectory-filter isn't acting as expected,
and doesn't get rid of all the history you want it to get rid of.


On Mon, Oct 10, 2016 at 4:30 PM, Jeff King  wrote:
> On Mon, Oct 10, 2016 at 02:42:36PM +0100, Seaders Oloinsigh wrote:
>
>> We have a git repository that looks like
>>
>> sdk/
>> android/
>> ios/
>> unity/
>> windows/
>>
>> Which we'd like to split into 4 repositories, 1 for each platform.  To
>> start this process (for splitting android out), I ran,
>>
>> git filter-branch -f --prune-empty --subdirectory-filter android -- --all
>
> OK, so that should rewrite each ref to have only the contents of the
> "android" directory at the top-level.
>
> Note that filter-branch saves a copy of the old refs in refs/original.
>
>> Which rewrote a ton of history and commits, and looked like it worked, but
>> on closer inspection had left a ton of history behind.
>>
>> If I run
>>
>> git log --all -- unity/
>>
>> It returns a list of commits that happened in the unity/ subfolder of the
>> original root.
>
> Here you asked for "--all", which includes refs/original. So you are
> seeing the original, unwritten commits (and none of your new ones, of
> course, because they do not have a unity/ directory!).
>
> Try:
>
>   git log --all --source -- unity
>
> to see which ref each commit is coming from.
>
> Or try:
>
>   git log --branches --tags -- unity
>
> to confirm that your branches and tags do not include that path.
>
> Or just:
>
>   git for-each-ref --format='delete %(refname)' refs/original |
>   git update-ref --stdin
>
> to get rid of the backup refs entirely.
>
> -Peff

Re: [PATCH] t4014-format-patch: Adjust git_version regex to better handle distro changes to DEF_VER

[Jeremy Huddleston Sequoia]

> On Oct 10, 2016, at 06:10, Jeff King  wrote:
> 
> On Sun, Oct 09, 2016 at 07:53:23PM -0700, Jeremy Huddleston Sequoia wrote:
> 
>> Subject: Re: [PATCH] t4014-format-patch: Adjust git_version regex to better
>> handle distro changes to DEF_VER
>> 
>> Regressed-in: 480871e09ed2e5275b4ba16b278681e5a8c122ae
>> Signed-off-by: Jeremy Huddleston Sequoia 
> 
> I see there was a discussion elsewhere on the list about exactly what
> you are putting into DEF_VAR that causes the problem. Perhaps the commit
> message here would be a good place to mention that, why the current
> regex breaks it, and why your new version fixes not only it, but other
> possible values of DEF_VAR.

Thanks, I've added this blurb:

For example, git distributed with Apple's Xcode reports a version like:
git version 2.9.3 (Apple Git-75)

Apple started doing this to help customers distinguish between different
versions of their packaged git which have the same base version (eg: with
different patches applied).



smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH] t4014-format-patch: Adjust git_version regex to better handle distro changes to DEF_VER

[Junio C Hamano]

Jeremy Huddleston Sequoia  writes:

> Regressed-in: 480871e09ed2e5275b4ba16b278681e5a8c122ae

Please be considerate to future readers of "git log" to help them
avoid mistakes earlier commits made that caused you troubles.

This line by itself without any explanation of what was broken is
quite useless as a commit message.  What can the readers do?  They'd
go back and say "git show 480871e09" and then what?  The test added
or modified by the commit has been working quite well for others
since it was made, so it is very likely the reader wouldn't be able
to tell if anything is wrong in it.

You would help if you said what is different in _your_ environment
from others who have happily been running and passing this test for
others to understand and learn from your fix.  What is it?

The "Adjust ... distro changes" in the title offers some hint but it
could be more explicit.  Please write something along this line
instead.

Subject: t4014: git --version can have SP in it

480871e09e ("format-patch: show base info before email
signature", 2016-09-07) added a helper function to recreate the
signature at the end of the e-mail, i.e. "-- " line followed by
the version string of Git, using output from "git --version" and
stripping everything before the last SP.

Because the default Git version string looks like "git version
2.10.0-1-g480871e09e", this was mostly OK, but people can change
this version string to arbitrary thing while compiling, which
can break the assumption if they had SP in it.  Notably, Apple
ships modified Git with " (Apple Git-xx)" appended to its
version number.

Instead, come up with the version string by stripping the "git
version " from the beginning.

> Signed-off-by: Jeremy Huddleston Sequoia 

Good.

> CC: Josh Triplett 
> CC: Junio C Hamano 

Please don't do this in your log message.  These belong to your
e-mail headers, not here.

> ---
>  t/t4014-format-patch.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index 8d90a6e..33f6940 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -754,7 +754,7 @@ test_expect_success 'format-patch --ignore-if-in-upstream 
> HEAD' '
>   git format-patch --ignore-if-in-upstream HEAD
>  '
>  
> -git_version="$(git --version | sed "s/.* //")"
> +git_version="$(git --version | sed "s/git version //")"

Anchor the fixed prefix to the beginning, so that we can protect
ourselves from another distro that would add "git version" in the
middle of their arbitrary versioning scheme ;-).  I.e.

sed "s/^git version //"

>  
>  signature() {
>   printf "%s\n%s\n\n" "-- " "${1:-$git_version}"

Re: [PATCH] t4014-format-patch: Adjust git_version regex to better handle distro changes to DEF_VER

[Junio C Hamano]

Jeff King  writes:

> On Sun, Oct 09, 2016 at 07:53:23PM -0700, Jeremy Huddleston Sequoia wrote:
>
>> Subject: Re: [PATCH] t4014-format-patch: Adjust git_version regex to better
>>  handle distro changes to DEF_VER
>>
>> Regressed-in: 480871e09ed2e5275b4ba16b278681e5a8c122ae
>> Signed-off-by: Jeremy Huddleston Sequoia 
>
> I see there was a discussion elsewhere on the list about exactly what
> you are putting into DEF_VAR that causes the problem. Perhaps the commit
> message here would be a good place to mention that, why the current
> regex breaks it, and why your new version fixes not only it, but other
> possible values of DEF_VAR.

Ah, should have known that you'd already be helping with this.
Thanks.

Re: [PATCH] t3700-add.sh: Avoid filename collission between tests which could lead to test failure

[Junio C Hamano]

Jeremy Huddleston Sequoia  writes:

> Regressed-in: 610d55af0f082f6b866dc858e144c03d8ed4424c
> Signed-off-by: Jeremy Huddleston Sequoia 
> CC: Thomas Gummerer 
> CC: Junio C Hamano 

This is also under-explained.  Was the test broken for everybody and
you are the first to report, or was your environment somehow different
that the existing problem was revealed there first?

RE: git merge deletes my changes

[Eduard Egorov]

Hello again,

I've noticed that my git is quite old (1.8.3) and built it from source tarball 
(2.10.1). Now the output is:

# ~/gitbuild/git-2.10.1/git merge -s subtree --squash ceph_ansible
fatal: refusing to merge unrelated histories

I've checked my command history again:
  738  git rm -rf ceph-ansible/
  739  ll
  740  ll ceph-ansible/
  741  rm ceph-ansible/purge_ceph_cluster.yml 
  742  git read-tree --prefix=ceph-ansible/ -u ceph_ansible
  743  gs
  744  gc "ceph-ansible: reset repo state"

A quick googling showed 
(http://stackoverflow.com/questions/37937984/git-refusing-to-merge-unrelated-histories
 ) that the default behavior is changed. Adding ' --allow-unrelated-histories' 
clears the error message but the merge itself is still wrong (my changes are 
lost). 

This error message might explain why git can't merge it correctly (since these 
repos doesn't has any relations, right). Can somebody confirm this please? 
Doesn't "merge -s subtree" really merges branches?

With best regards
Eduard Egorov

-Original Message-
From: Eduard Egorov 
Sent: Monday, October 10, 2016 12:39 PM
To: 'git@vger.kernel.org' 
Subject: git merge deletes my changes

Hello Git experts,

A week ago, I've reset a state of 'ceph-ansible' folder in %current% branch 
with code from corresponding branch (that tracks an upstream from github):

# git read-tree --prefix=ceph-ansible/ -u ceph_ansible

Then I've committed several changes, including:

1. Renamed file and commited:
# git mv site.yml.sample site.yml

2. Made some changes and committed

3. Pulled updates from original branch by:
# git merge -s subtree --squash ceph_ansible

It said:
Auto-merging ceph-ansible/site.yml.sample
blablabla
Squash commit -- not updating HEAD
Automatic merge went well; stopped before committing as requested

I can see that "my" ceph-ansible/site.yml is deleted (as well as few new files 
added by me), ceph-ansible/site.yml.sample is restored and doesn't contain my 
changes (it's just restored to the state before my changes).
`git log ceph_ansible site.yml.sample` shows the latest change on ceph_ansible 
branch done from 26th of August, 2016, my changes in %current% branch was this 
October, so it shouldn't be any conflict either (even if it was).
I believe there is some obvious explanation for this behavior. Could you help 
me please? What information can  I provide in order to troubleshoot this issue? 

A post on SO: 
http://stackoverflow.com/questions/39954265/git-merge-deletes-my-changes

With best regards
Eduard Egorov

Re: [PATCH] t4014-format-patch: Adjust git_version regex to better handle distro changes to DEF_VER

[Jeremy Huddleston Sequoia]

> On Oct 10, 2016, at 09:42, Junio C Hamano  wrote:
> 
> Jeremy Huddleston Sequoia  writes:
> 
>> Regressed-in: 480871e09ed2e5275b4ba16b278681e5a8c122ae
> 
> Please be considerate to future readers of "git log" to help them
> avoid mistakes earlier commits made that caused you troubles.
> 
> This line by itself without any explanation of what was broken is
> quite useless as a commit message.  What can the readers do?  They'd
> go back and say "git show 480871e09" and then what?  The test added
> or modified by the commit has been working quite well for others
> since it was made, so it is very likely the reader wouldn't be able
> to tell if anything is wrong in it.
> 
> You would help if you said what is different in _your_ environment
> from others who have happily been running and passing this test for
> others to understand and learn from your fix.  What is it?
> 
> The "Adjust ... distro changes" in the title offers some hint but it
> could be more explicit.  Please write something along this line
> instead.
> 
>Subject: t4014: git --version can have SP in it
> 
>480871e09e ("format-patch: show base info before email
>signature", 2016-09-07) added a helper function to recreate the
>signature at the end of the e-mail, i.e. "-- " line followed by
>the version string of Git, using output from "git --version" and
>stripping everything before the last SP.
> 
>Because the default Git version string looks like "git version
>2.10.0-1-g480871e09e", this was mostly OK, but people can change
>this version string to arbitrary thing while compiling, which
>can break the assumption if they had SP in it.  Notably, Apple
>ships modified Git with " (Apple Git-xx)" appended to its
>version number.
> 
>Instead, come up with the version string by stripping the "git
>version " from the beginning.

Ok, I'll add that explanation to the commit message, thanks.


> 
>> Signed-off-by: Jeremy Huddleston Sequoia 
> 
> Good.
> 
>> CC: Josh Triplett 
>> CC: Junio C Hamano 
> 
> Please don't do this in your log message.  These belong to your
> e-mail headers, not here.

Sorry, we do that consistently on other projects during the review process, so 
git send-email picks them up and adds them automatically for each patch being 
sent out.  It's quite useful, actually.

> 
>> ---
>> t/t4014-format-patch.sh | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
>> index 8d90a6e..33f6940 100755
>> --- a/t/t4014-format-patch.sh
>> +++ b/t/t4014-format-patch.sh
>> @@ -754,7 +754,7 @@ test_expect_success 'format-patch 
>> --ignore-if-in-upstream HEAD' '
>>  git format-patch --ignore-if-in-upstream HEAD
>> '
>> 
>> -git_version="$(git --version | sed "s/.* //")"
>> +git_version="$(git --version | sed "s/git version //")"
> 
> Anchor the fixed prefix to the beginning, so that we can protect
> ourselves from another distro that would add "git version" in the
> middle of their arbitrary versioning scheme ;-).  I.e.
> 
>sed "s/^git version //"

Ok.

> 
>> 
>> signature() {
>>  printf "%s\n%s\n\n" "-- " "${1:-$git_version}"



smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH] remote.c: free previous results when looking for a ref match

[Stefan Beller]

On Sat, Oct 8, 2016 at 6:38 AM, René Scharfe  wrote:

>
> Again, this check is not necessary.  If I read the code correctly the
> pointer could be uninitialized at that point, though, causing free(3) to
> crash.

yep, this patch is bogus.

Re: [PATCH] t3700-add.sh: Avoid filename collission between tests which could lead to test failure

[Jeremy Huddleston Sequoia]

> On Oct 10, 2016, at 09:44, Junio C Hamano  wrote:
> 
> Jeremy Huddleston Sequoia  writes:
> 
>> Regressed-in: 610d55af0f082f6b866dc858e144c03d8ed4424c
>> Signed-off-by: Jeremy Huddleston Sequoia 
>> CC: Thomas Gummerer 
>> CC: Junio C Hamano 
> 
> This is also under-explained.  Was the test broken for everybody and
> you are the first to report, or was your environment somehow different
> that the existing problem was revealed there first?

There is nothing obviously different about my environment.  I'd expect this 
test to fail for everyone that runs the entire test because xfoo3 is not 
cleaned up after this earlier test:

test_expect_success \
'git update-index --add: Test that executable bit is not used...' \
'git config core.filemode 0 &&
 test_ln_s_add xfoo2 xfoo3 &&   # runs git update-index --add
 test_mode_in_index 12 xfoo3'

If anyone ran the test by itself or otherwise skipped the 'git update-index 
--add: Test that executable bit is not used...' test, they would not encounter 
the issue.  However, the normal case is to run all of the tests, so I would 
expect everyone to be tripping over this issue.  If they're not, then that 
itself might be indicative of another bug because clearly that previous test 
just added xfoo3 as a symlink.

--Jeremy

smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH] pretty: respect color settings for %C placeholders

[René Scharfe]

Am 10.10.2016 um 17:15 schrieb Jeff King:

On Mon, Oct 10, 2016 at 10:28:18AM -0400, Jeff King wrote:


We could add some new tag to change the behavior of all following %C
tags. Something like %C(tty) maybe (probably a bad name), then
discourage the use if "%C(auto" for terminal detection?


Yeah, adding a "%C(enable-auto-color)" or something would be backwards
compatible and less painful than using "%C(auto)" everywhere. I do
wonder if anybody actually _wants_ the "always show color, even if
--no-color" behavior. I'm having trouble thinking of a good use for it.

IOW, I'm wondering if anyone would disagree that the current behavior is
simply buggy. Reading the thread at:

  http://public-inbox.org/git/7v4njkmq07@alter.siamese.dyndns.org/

I don't really see any compelling reason against it (there was some
question of which config to use, but we already answered that with
"%C(auto)", and use the value from the pretty_ctx).


So here's what a patch to do that would look like. I admit that "I can't
think of a good use" does not mean there _isn't_ one, but perhaps by
posting this, it might provoke other people to think on it, too. And if
nobody can come up with, maybe it's a good idea.


Color tags that respect the config and the --color option make the most 
sense to me as well.


Nevertheless a possible counterpoint: Coloring is used in commands that 
are intended for human consumption.  Most of the time there is no need 
to to conserve the output in a file.  But even then, and of course with 
pipes, once you look at it using less -R or grep you still get nice 
colored lines and not a monochrome wall of text.



-- >8 --
Subject: pretty: respect color settings for %C placeholders

The color placeholders have traditionally been
unconditional, showing colors even when git is not otherwise
configured to do so. This was not so bad for their original
use, which was on the command-line (and the user could
decide at that moment whether to add colors or not). But
these days we have configured formats via pretty.*, and
those should operate in multiple contexts.

In 3082517 (log --format: teach %C(auto,black) to respect
color config, 2012-12-17), we gave an extended placeholder
that could be used to accomplish this. But it's rather
clunky to use, because you have to specify it individually
for each color (and their matching resets) in the format.
We shied away from just switching the default to auto,
because it is technically breaking backwards compatibility.

However, there's not really a use case for unconditional
colors. The most plausible reason you would want them
unconditional is to redirect "git log" output to a file. But
there, the right answer is --color=always, as it does the
right thing both with custom user-format colors and
git-generated colors.

So let's switch to the more useful default. In the
off-chance that somebody really does find a use for
unconditional colors without wanting to enable the rest of
git's colors, we can provide %C(always,...) to enable the
old behavior.

Signed-off-by: Jeff King 
---
The tests unsurprisingly needed updating, as we're breaking the old
behavior. The diff is easier to read read with "-w".

 Documentation/pretty-formats.txt | 13 +++---
 pretty.c | 19 +---
 t/t6006-rev-list-format.sh   | 94 
 3 files changed, 70 insertions(+), 56 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index a942d57..7aa1a8b 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -167,11 +167,14 @@ endif::git-rev-list[]
 - '%Cblue': switch color to blue
 - '%Creset': reset color
 - '%C(...)': color specification, as described in color.branch.* config option;
-  adding `auto,` at the beginning will emit color only when colors are
-  enabled for log output (by `color.diff`, `color.ui`, or `--color`, and
-  respecting the `auto` settings of the former if we are going to a
-  terminal). `auto` alone (i.e. `%C(auto)`) will turn on auto coloring
-  on the next placeholders until the color is switched again.
+  By default, colors are shown only when enabled for log output (by
+  `color.diff`, `color.ui`, or `--color`, and respecting the `auto`
+  settings of the former if we are going to a terminal). `%C(auto,...)`
+  is accepted as a historical synonym for the default. Specifying
+  `%C(always,...) will show the colors always, even when colors are not
+  otherwise enabled (to enable this behavior for the whole format, use
+  `--color=always`). `auto` alone (i.e. `%C(auto)`) will turn on auto
+  coloring on the next placeholders until the color is switched again.
 - '%m': left (`<`), right (`>`) or boundary (`-`) mark
 - '%n': newline
 - '%%': a raw '%'
diff --git a/pretty.c b/pretty.c
index 25efbca..73e58b5 100644
--- a/pretty.c
+++ b/pretty.c
@@ -965,22 +965,31 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 
*/

if (!

[PATCH v3 01/25] sequencer: use static initializers for replay_opts

[Johannes Schindelin]

This change is not completely faithful: instead of initializing all fields
to 0, we choose to initialize command and subcommand to -1 (instead of
defaulting to REPLAY_REVERT and REPLAY_NONE, respectively). Practically,
it makes no difference at all, but future-proofs the code to require
explicit assignments for both fields.

Signed-off-by: Johannes Schindelin 
---
 builtin/revert.c | 6 ++
 sequencer.h  | 1 +
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 4e69380..7365559 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -178,10 +178,9 @@ static void parse_args(int argc, const char **argv, struct 
replay_opts *opts)
 
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
-   struct replay_opts opts;
+   struct replay_opts opts = REPLAY_OPTS_INIT;
int res;
 
-   memset(&opts, 0, sizeof(opts));
if (isatty(0))
opts.edit = 1;
opts.action = REPLAY_REVERT;
@@ -195,10 +194,9 @@ int cmd_revert(int argc, const char **argv, const char 
*prefix)
 
 int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 {
-   struct replay_opts opts;
+   struct replay_opts opts = REPLAY_OPTS_INIT;
int res;
 
-   memset(&opts, 0, sizeof(opts));
opts.action = REPLAY_PICK;
git_config(git_default_config, NULL);
parse_args(argc, argv, &opts);
diff --git a/sequencer.h b/sequencer.h
index 5ed5cb1..db425ad 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -47,6 +47,7 @@ struct replay_opts {
/* Only used by REPLAY_NONE */
struct rev_info *revs;
 };
+#define REPLAY_OPTS_INIT { -1, -1 }
 
 int sequencer_pick_revisions(struct replay_opts *opts);
 
-- 
2.10.0.windows.1.325.ge6089c1

[PATCH v3 00/25] Prepare the sequencer for the upcoming rebase -i patches

[Johannes Schindelin]

This patch series marks the  '4' in the countdown to speed up rebase -i
by implementing large parts in C. It is based on the `libify-sequencer`
patch series that I submitted last week.

The patches in this series merely prepare the sequencer code for the
next patch series that actually teaches the sequencer to run an
interactive rebase.

The reason to split these two patch series is simple: to keep them at a
sensible size.

The two patch series after that are much smaller: a two-patch "series"
that switches rebase -i to use the sequencer (except with --root or
--preserve-merges), and a couple of patches to move several pretty
expensive script processing steps to C (think: autosquash).

The end game of this patch series is a git-rebase--helper that makes
rebase -i 5x faster on Windows (according to t/perf/p3404). Travis says
that even MacOSX and Linux benefit (4x and 3x, respectively).

I have been working on this since early February, whenever time allowed,
and it is time to put it into the users' hands. To that end, I will most
likely submit the remaining three patch series in the next two days, and
integrate the whole shebang into Git for Windows 2.10.0.

Therefore I would be most grateful for every in-depth review.

Changes vs v2:

- dramatically simplified the logic to retain do_recursive_merge()'s
  return value, even when positive (indicating merge conflicts).

- ensured that write_message() rolls back locked files in case of
  errors.

- added a patch to downcase all first letters of sequencer's error
  messages, as suggested by Junio.

- replaced 25/25 ("sequencer: remove bogus hint for translators") by a
  fix to 8/25 ("completely revamp todo parsing"): the first %s is no
  longer "revert" or "cherry-pick", but a "todo" command.

- backed out the sequencer_entrust() function and uglified the code to
  always duplicate the option values for the sake of being able to
  properly releasing them afterwards.

- CR/LFs are now handled like LFs in todo scripts: they are stripped
  (thanks to Hannes Sixt for pointing that out).

- unexported sequencer_commit() (it can safely remain a static
  function). While at it, renamed it back to run_git_commit() (the
  unspecific name does not hurt if it remains private to sequencer.c).

- clarified in a code comment what read_author_script() does.

- marked for translation the last remaining error message in sequencer.c
  that was not yet marked for translation.


Johannes Schindelin (25):
  sequencer: use static initializers for replay_opts
  sequencer: use memoized sequencer directory path
  sequencer: avoid unnecessary indirection
  sequencer: future-proof remove_sequencer_state()
  sequencer: eventually release memory allocated for the option values
  sequencer: future-proof read_populate_todo()
  sequencer: completely revamp the "todo" script parsing
  sequencer: strip CR from the todo script
  sequencer: avoid completely different messages for different actions
  sequencer: get rid of the subcommand field
  sequencer: refactor the code to obtain a short commit name
  sequencer: remember the onelines when parsing the todo file
  sequencer: prepare for rebase -i's commit functionality
  sequencer: introduce a helper to read files written by scripts
  sequencer: allow editing the commit message on a case-by-case basis
  sequencer: support amending commits
  sequencer: support cleaning up commit messages
  sequencer: do not try to commit when there were merge conflicts
  sequencer: left-trim lines read from the script
  sequencer: refactor write_message()
  sequencer: remove overzealous assumption in rebase -i mode
  sequencer: mark action_name() for translation
  sequencer: quote filenames in error messages
  sequencer: start error messages consistently with lower case
  sequencer: mark all error messages for translation

 builtin/commit.c  |   2 +-
 builtin/revert.c  |  48 +--
 sequencer.c   | 690 --
 sequencer.h   |  23 +-
 t/t3501-revert-cherry-pick.sh |   2 +-
 5 files changed, 503 insertions(+), 262 deletions(-)

Published-As: https://github.com/dscho/git/releases/tag/prepare-sequencer-v3
Fetch-It-Via: git fetch https://github.com/dscho/git prepare-sequencer-v3

Interdiff vs v2:

 diff --git a/builtin/revert.c b/builtin/revert.c
 index c9ae4dc..0a7b5f4 100644
 --- a/builtin/revert.c
 +++ b/builtin/revert.c
 @@ -165,6 +165,12 @@ static int run_sequencer(int argc, const char **argv, 
struct replay_opts *opts)
if (argc > 1)
usage_with_options(usage_str, options);
  
 +  /* These option values will be free()d */
 +  if (opts->gpg_sign)
 +  opts->gpg_sign = xstrdup(opts->gpg_sign);
 +  if (opts->strategy)
 +  opts->strategy = xstrdup(opts->strategy);
 +
if (cmd == 'q')
return sequencer_remove_state(opts);
if (cmd == 'c')
 diff --git a/sequencer.c b/sequencer.c
 index cdff0f1..86

[PATCH v3 02/25] sequencer: use memoized sequencer directory path

[Johannes Schindelin]

Signed-off-by: Johannes Schindelin 
---
 builtin/commit.c |  2 +-
 sequencer.c  | 11 ++-
 sequencer.h  |  5 +
 3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 1cba3b7..9fddb19 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -183,7 +183,7 @@ static void determine_whence(struct wt_status *s)
whence = FROM_MERGE;
else if (file_exists(git_path_cherry_pick_head())) {
whence = FROM_CHERRY_PICK;
-   if (file_exists(git_path(SEQ_DIR)))
+   if (file_exists(git_path_seq_dir()))
sequencer_in_use = 1;
}
else
diff --git a/sequencer.c b/sequencer.c
index eec8a60..cb16cbd 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -21,10 +21,11 @@
 const char sign_off_header[] = "Signed-off-by: ";
 static const char cherry_picked_prefix[] = "(cherry picked from commit ";
 
-static GIT_PATH_FUNC(git_path_todo_file, SEQ_TODO_FILE)
-static GIT_PATH_FUNC(git_path_opts_file, SEQ_OPTS_FILE)
-static GIT_PATH_FUNC(git_path_seq_dir, SEQ_DIR)
-static GIT_PATH_FUNC(git_path_head_file, SEQ_HEAD_FILE)
+GIT_PATH_FUNC(git_path_seq_dir, "sequencer")
+
+static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo")
+static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts")
+static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
 
 static int is_rfc2822_line(const char *buf, int len)
 {
@@ -112,7 +113,7 @@ static void remove_sequencer_state(void)
 {
struct strbuf seq_dir = STRBUF_INIT;
 
-   strbuf_addstr(&seq_dir, git_path(SEQ_DIR));
+   strbuf_addstr(&seq_dir, git_path_seq_dir());
remove_dir_recursively(&seq_dir, 0);
strbuf_release(&seq_dir);
 }
diff --git a/sequencer.h b/sequencer.h
index db425ad..dd4d33a 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -1,10 +1,7 @@
 #ifndef SEQUENCER_H
 #define SEQUENCER_H
 
-#define SEQ_DIR"sequencer"
-#define SEQ_HEAD_FILE  "sequencer/head"
-#define SEQ_TODO_FILE  "sequencer/todo"
-#define SEQ_OPTS_FILE  "sequencer/opts"
+const char *git_path_seq_dir(void);
 
 #define APPEND_SIGNOFF_DEDUP (1u << 0)
 
-- 
2.10.0.windows.1.325.ge6089c1

[PATCH v3 05/25] sequencer: eventually release memory allocated for the option values

[Johannes Schindelin]

The sequencer is our attempt to lib-ify cherry-pick. Yet it behaves
like a one-shot command when it reads its configuration: memory is
allocated and released only when the command exits.

This is kind of okay for git-cherry-pick, which *is* a one-shot
command. All the work to make the sequencer its work horse was
done to allow using the functionality as a library function, though,
including proper clean-up after use.

To remedy that, we now take custody of the option values in question,
requiring those values to be malloc()ed or strdup()ed (an alternative
approach, to add a list of pointers to malloc()ed data and to ask the
sequencer to release all of them in the end, was proposed earlier but
rejected during review).

Sadly, the current approach makes the code uglier, as we now have to
take care to strdup() the values passed via the command-line.

Signed-off-by: Johannes Schindelin 
---
 builtin/revert.c |  6 ++
 sequencer.c  | 32 ++--
 sequencer.h  |  6 +++---
 3 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 7365559..fce9c75 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -174,6 +174,12 @@ static void parse_args(int argc, const char **argv, struct 
replay_opts *opts)
 
if (argc > 1)
usage_with_options(usage_str, options);
+
+   /* These option values will be free()d */
+   if (opts->gpg_sign)
+   opts->gpg_sign = xstrdup(opts->gpg_sign);
+   if (opts->strategy)
+   opts->strategy = xstrdup(opts->strategy);
 }
 
 int cmd_revert(int argc, const char **argv, const char *prefix)
diff --git a/sequencer.c b/sequencer.c
index 8d272fb..22c31c8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -117,6 +117,13 @@ static int has_conforming_footer(struct strbuf *sb, struct 
strbuf *sob,
 static void remove_sequencer_state(const struct replay_opts *opts)
 {
struct strbuf dir = STRBUF_INIT;
+   int i;
+
+   free(opts->gpg_sign);
+   free(opts->strategy);
+   for (i = 0; i < opts->xopts_nr; i++)
+   free(opts->xopts[i]);
+   free(opts->xopts);
 
strbuf_addf(&dir, "%s", get_dir(opts));
remove_dir_recursively(&dir, 0);
@@ -280,7 +287,7 @@ static int do_recursive_merge(struct commit *base, struct 
commit *next,
struct merge_options o;
struct tree *result, *next_tree, *base_tree, *head_tree;
int clean;
-   const char **xopt;
+   char **xopt;
static struct lock_file index_lock;
 
hold_locked_index(&index_lock, 1);
@@ -583,7 +590,8 @@ static int do_pick_commit(struct commit *commit, struct 
replay_opts *opts)
 
commit_list_insert(base, &common);
commit_list_insert(next, &remotes);
-   res |= try_merge_command(opts->strategy, opts->xopts_nr, 
opts->xopts,
+   res |= try_merge_command(opts->strategy,
+opts->xopts_nr, (const char 
**)opts->xopts,
common, sha1_to_hex(head), remotes);
free_commit_list(common);
free_commit_list(remotes);
@@ -802,10 +810,22 @@ static int populate_opts_cb(const char *key, const char 
*value, void *data)
opts->allow_ff = git_config_bool_or_int(key, value, 
&error_flag);
else if (!strcmp(key, "options.mainline"))
opts->mainline = git_config_int(key, value);
-   else if (!strcmp(key, "options.strategy"))
-   git_config_string(&opts->strategy, key, value);
-   else if (!strcmp(key, "options.gpg-sign"))
-   git_config_string(&opts->gpg_sign, key, value);
+   else if (!strcmp(key, "options.strategy")) {
+   if (!value)
+   config_error_nonbool(key);
+   else {
+   free(opts->strategy);
+   opts->strategy = xstrdup(value);
+   }
+   }
+   else if (!strcmp(key, "options.gpg-sign")) {
+   if (!value)
+   config_error_nonbool(key);
+   else {
+   free(opts->gpg_sign);
+   opts->gpg_sign = xstrdup(value);
+   }
+   }
else if (!strcmp(key, "options.strategy-option")) {
ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
opts->xopts[opts->xopts_nr++] = xstrdup(value);
diff --git a/sequencer.h b/sequencer.h
index dd4d33a..8453669 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -34,11 +34,11 @@ struct replay_opts {
 
int mainline;
 
-   const char *gpg_sign;
+   char *gpg_sign;
 
/* Merge strategy */
-   const char *strategy;
-   const char **xopts;
+   char *strategy;
+   char **xopts;
size_t xopts_nr, xopts_alloc;
 
/* Only used by REPLAY_NONE */
-- 
2.10.0.windows.1.325.ge6089c1

[PATCH v3 04/25] sequencer: future-proof remove_sequencer_state()

[Johannes Schindelin]

In a couple of commits, we will teach the sequencer to handle the
nitty gritty of the interactive rebase, which keeps its state in a
different directory.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 21 +
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index c2fbf6f..8d272fb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -27,6 +27,11 @@ static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo")
 static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts")
 static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
 
+static const char *get_dir(const struct replay_opts *opts)
+{
+   return git_path_seq_dir();
+}
+
 static int is_rfc2822_line(const char *buf, int len)
 {
int i;
@@ -109,13 +114,13 @@ static int has_conforming_footer(struct strbuf *sb, 
struct strbuf *sob,
return 1;
 }
 
-static void remove_sequencer_state(void)
+static void remove_sequencer_state(const struct replay_opts *opts)
 {
-   struct strbuf seq_dir = STRBUF_INIT;
+   struct strbuf dir = STRBUF_INIT;
 
-   strbuf_addstr(&seq_dir, git_path_seq_dir());
-   remove_dir_recursively(&seq_dir, 0);
-   strbuf_release(&seq_dir);
+   strbuf_addf(&dir, "%s", get_dir(opts));
+   remove_dir_recursively(&dir, 0);
+   strbuf_release(&dir);
 }
 
 static const char *action_name(const struct replay_opts *opts)
@@ -940,7 +945,7 @@ static int sequencer_rollback(struct replay_opts *opts)
}
if (reset_for_rollback(sha1))
goto fail;
-   remove_sequencer_state();
+   remove_sequencer_state(opts);
strbuf_release(&buf);
return 0;
 fail:
@@ -1034,7 +1039,7 @@ static int pick_commits(struct commit_list *todo_list, 
struct replay_opts *opts)
 * Sequence of picks finished successfully; cleanup by
 * removing the .git/sequencer directory
 */
-   remove_sequencer_state();
+   remove_sequencer_state(opts);
return 0;
 }
 
@@ -1095,7 +1100,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 * one that is being continued
 */
if (opts->subcommand == REPLAY_REMOVE_STATE) {
-   remove_sequencer_state();
+   remove_sequencer_state(opts);
return 0;
}
if (opts->subcommand == REPLAY_ROLLBACK)
-- 
2.10.0.windows.1.325.ge6089c1

[PATCH v3 08/25] sequencer: strip CR from the todo script

[Johannes Schindelin]

It is not unheard of that editors on Windows write CR/LF even if the
file originally had only LF. This is particularly awkward for exec lines
of a rebase -i todo sheet. Take for example the insn "exec echo": The
shell script parser splits at the LF and leaves the CR attached to
"echo", which leads to the unknown command "echo\r".

Work around that by stripping CR when reading the todo commands, as we
already do for LF.

This happens to fix t9903.14 and .15 in MSYS1 environments (with the
rebase--helper patches based on this patch series): the todo script
constructed in such a setup contains CR/LF thanks to MSYS1 runtime's
cleverness.

Based on a report and a patch by Johannes Sixt.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index 678fdf3..cee7e50 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -774,6 +774,9 @@ static int parse_insn_buffer(char *buf, struct todo_list 
*todo_list)
 
next_p = *eol ? eol + 1 /* skip LF */ : eol;
 
+   if (p != eol && eol[-1] == '\r')
+   eol--; /* skip Carriage Return */
+
item = append_new_todo(todo_list);
item->offset_in_buf = p - todo_list->buf.buf;
if (parse_insn_line(item, p, eol)) {
-- 
2.10.0.windows.1.325.ge6089c1

[PATCH v3 06/25] sequencer: future-proof read_populate_todo()

[Johannes Schindelin]

Over the next commits, we will work on improving the sequencer to the
point where it can process the todo script of an interactive rebase. To
that end, we will need to teach the sequencer to read interactive
rebase's todo file. In preparation, we consolidate all places where
that todo file is needed to call a function that we will later extend.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 18 +++---
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 22c31c8..4e00c5e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -32,6 +32,11 @@ static const char *get_dir(const struct replay_opts *opts)
return git_path_seq_dir();
 }
 
+static const char *get_todo_path(const struct replay_opts *opts)
+{
+   return git_path_todo_file();
+}
+
 static int is_rfc2822_line(const char *buf, int len)
 {
int i;
@@ -769,25 +774,24 @@ static int parse_insn_buffer(char *buf, struct 
commit_list **todo_list,
 static int read_populate_todo(struct commit_list **todo_list,
struct replay_opts *opts)
 {
+   const char *todo_file = get_todo_path(opts);
struct strbuf buf = STRBUF_INIT;
int fd, res;
 
-   fd = open(git_path_todo_file(), O_RDONLY);
+   fd = open(todo_file, O_RDONLY);
if (fd < 0)
-   return error_errno(_("Could not open %s"),
-  git_path_todo_file());
+   return error_errno(_("Could not open %s"), todo_file);
if (strbuf_read(&buf, fd, 0) < 0) {
close(fd);
strbuf_release(&buf);
-   return error(_("Could not read %s."), git_path_todo_file());
+   return error(_("Could not read %s."), todo_file);
}
close(fd);
 
res = parse_insn_buffer(buf.buf, todo_list, opts);
strbuf_release(&buf);
if (res)
-   return error(_("Unusable instruction sheet: %s"),
-   git_path_todo_file());
+   return error(_("Unusable instruction sheet: %s"), todo_file);
return 0;
 }
 
@@ -1077,7 +1081,7 @@ static int sequencer_continue(struct replay_opts *opts)
 {
struct commit_list *todo_list = NULL;
 
-   if (!file_exists(git_path_todo_file()))
+   if (!file_exists(get_todo_path(opts)))
return continue_single_pick();
if (read_populate_opts(opts) ||
read_populate_todo(&todo_list, opts))
-- 
2.10.0.windows.1.325.ge6089c1

[PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing

[Johannes Schindelin]

When we came up with the "sequencer" idea, we really wanted to have
kind of a plumbing equivalent of the interactive rebase. Hence the
choice of words: the "todo" script, a "pick", etc.

However, when it came time to implement the entire shebang, somehow this
idea got lost and the sequencer was used as working horse for
cherry-pick and revert instead. So as not to interfere with the
interactive rebase, it even uses a separate directory to store its
state.

Furthermore, it also is stupidly strict about the "todo" script it
accepts: while it parses commands in a way that was *designed* to be
similar to the interactive rebase, it then goes on to *error out* if the
commands disagree with the overall action (cherry-pick or revert).

Finally, the sequencer code chose to deviate from the interactive rebase
code insofar that when it comes to writing the file with the remaining
commands, it *reformats* the "todo" script instead of just writing the
part of the parsed script that were not yet processed. This is not only
unnecessary churn, but might well lose information that is valuable to
the user (i.e. comments after the commands).

Let's just bite the bullet and rewrite the entire parser; the code now
becomes not only more elegant: it allows us to go on and teach the
sequencer how to parse *true* "todo" scripts as used by the interactive
rebase itself. In a way, the sequencer is about to grow up to do its
older brother's job. Better.

In particular, we choose to maintain the list of commands in an array
instead of a linked list: this is flexible enough to allow us later on to
even implement rebase -i's reordering of fixup!/squash! commits very
easily (and with a very nice speed bonus, at least on Windows).

While at it, do not stop at the first problem, but list *all* of the
problems. This will help the user when the sequencer will do `rebase
-i`'s work by allowing to address all issues in one go rather than going
back and forth until the todo list is valid.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 288 +++-
 1 file changed, 167 insertions(+), 121 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 4e00c5e..678fdf3 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -465,7 +465,26 @@ static int allow_empty(struct replay_opts *opts, struct 
commit *commit)
return 1;
 }
 
-static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
+enum todo_command {
+   TODO_PICK = 0,
+   TODO_REVERT
+};
+
+static const char *todo_command_strings[] = {
+   "pick",
+   "revert"
+};
+
+static const char *command_to_string(const enum todo_command command)
+{
+   if (command < ARRAY_SIZE(todo_command_strings))
+   return todo_command_strings[command];
+   die("Unknown command: %d", command);
+}
+
+
+static int do_pick_commit(enum todo_command command, struct commit *commit,
+   struct replay_opts *opts)
 {
unsigned char head[20];
struct commit *base, *next, *parent;
@@ -524,10 +543,13 @@ static int do_pick_commit(struct commit *commit, struct 
replay_opts *opts)
return fast_forward_to(commit->object.oid.hash, head, unborn, 
opts);
 
if (parent && parse_commit(parent) < 0)
-   /* TRANSLATORS: The first %s will be "revert" or
-  "cherry-pick", the second %s a SHA1 */
+   /*
+* TRANSLATORS: The first %s will be a "todo" command like
+* "revert" or "pick", the second %s a SHA1.
+*/
return error(_("%s: cannot parse parent commit %s"),
-   action_name(opts), oid_to_hex(&parent->object.oid));
+   command_to_string(command),
+   oid_to_hex(&parent->object.oid));
 
if (get_message(commit, &msg) != 0)
return error(_("Cannot get commit message for %s"),
@@ -540,7 +562,7 @@ static int do_pick_commit(struct commit *commit, struct 
replay_opts *opts)
 * reverse of it if we are revert.
 */
 
-   if (opts->action == REPLAY_REVERT) {
+   if (command == TODO_REVERT) {
base = commit;
base_label = msg.label;
next = parent;
@@ -581,7 +603,7 @@ static int do_pick_commit(struct commit *commit, struct 
replay_opts *opts)
}
}
 
-   if (!opts->strategy || !strcmp(opts->strategy, "recursive") || 
opts->action == REPLAY_REVERT) {
+   if (!opts->strategy || !strcmp(opts->strategy, "recursive") || command 
== TODO_REVERT) {
res = do_recursive_merge(base, next, base_label, next_label,
 head, &msgbuf, opts);
if (res < 0)
@@ -608,17 +630,17 @@ static int do_pick_commit(struct commit *commit, struct 
replay_opts *opts)
 * However, if the merge did not even start, then we don't want to
 * write it at all.
   

[PATCH v3 03/25] sequencer: avoid unnecessary indirection

[Johannes Schindelin]

We really do not need the *pointer to a* pointer to the options in
the read_populate_opts() function.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index cb16cbd..c2fbf6f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -813,7 +813,7 @@ static int populate_opts_cb(const char *key, const char 
*value, void *data)
return 0;
 }
 
-static int read_populate_opts(struct replay_opts **opts)
+static int read_populate_opts(struct replay_opts *opts)
 {
if (!file_exists(git_path_opts_file()))
return 0;
@@ -823,7 +823,7 @@ static int read_populate_opts(struct replay_opts **opts)
 * about this case, though, because we wrote that file ourselves, so we
 * are pretty certain that it is syntactically correct.
 */
-   if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) 
< 0)
+   if (git_config_from_file(populate_opts_cb, git_path_opts_file(), opts) 
< 0)
return error(_("Malformed options sheet: %s"),
git_path_opts_file());
return 0;
@@ -1054,7 +1054,7 @@ static int sequencer_continue(struct replay_opts *opts)
 
if (!file_exists(git_path_todo_file()))
return continue_single_pick();
-   if (read_populate_opts(&opts) ||
+   if (read_populate_opts(opts) ||
read_populate_todo(&todo_list, opts))
return -1;
 
-- 
2.10.0.windows.1.325.ge6089c1

[PATCH v3 11/25] sequencer: refactor the code to obtain a short commit name

[Johannes Schindelin]

Not only does this DRY up the code (providing a better documentation what
the code is about, as well as allowing to change the behavior in a single
place), it also makes it substantially shorter to use the same
functionality in functions to be introduced when we teach the sequencer to
process interactive-rebase's git-rebase-todo file.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 14b1746..afc494e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -149,13 +149,18 @@ struct commit_message {
const char *message;
 };
 
+static const char *short_commit_name(struct commit *commit)
+{
+   return find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV);
+}
+
 static int get_message(struct commit *commit, struct commit_message *out)
 {
const char *abbrev, *subject;
int subject_len;
 
out->message = logmsg_reencode(commit, NULL, 
get_commit_output_encoding());
-   abbrev = find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV);
+   abbrev = short_commit_name(commit);
 
subject_len = find_commit_subject(out->message, &subject);
 
@@ -642,8 +647,7 @@ static int do_pick_commit(enum todo_command command, struct 
commit *commit,
error(command == TODO_REVERT
  ? _("could not revert %s... %s")
  : _("could not apply %s... %s"),
- find_unique_abbrev(commit->object.oid.hash, 
DEFAULT_ABBREV),
- msg.subject);
+ short_commit_name(commit), msg.subject);
print_advice(res == 1, opts);
rerere(opts->allow_rerere_auto);
goto leave;
@@ -910,9 +914,7 @@ static int walk_revs_populate_todo(struct todo_list 
*todo_list,
item->offset_in_buf = todo_list->buf.len;
subject_len = find_commit_subject(commit_buffer, &subject);
strbuf_addf(&todo_list->buf, "%s %s %.*s\n", command_string,
-   find_unique_abbrev(commit->object.oid.hash,
-   DEFAULT_ABBREV),
-   subject_len, subject);
+   short_commit_name(commit), subject_len, subject);
unuse_commit_buffer(commit, commit_buffer);
}
return 0;
-- 
2.10.0.windows.1.325.ge6089c1

[PATCH v3 09/25] sequencer: avoid completely different messages for different actions

[Johannes Schindelin]

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index cee7e50..443a238 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -224,11 +224,8 @@ static int error_dirty_index(struct replay_opts *opts)
if (read_cache_unmerged())
return error_resolve_conflict(action_name(opts));
 
-   /* Different translation strings for cherry-pick and revert */
-   if (opts->action == REPLAY_PICK)
-   error(_("Your local changes would be overwritten by 
cherry-pick."));
-   else
-   error(_("Your local changes would be overwritten by revert."));
+   error(_("Your local changes would be overwritten by %s."),
+   action_name(opts));
 
if (advice_commit_before_merge)
advise(_("Commit your changes or stash them to proceed."));
-- 
2.10.0.windows.1.325.ge6089c1

[PATCH v3 19/25] sequencer: left-trim lines read from the script

[Johannes Schindelin]

Interactive rebase's scripts may be indented; we need to handle this
case, too, now that we prepare the sequencer to process interactive
rebases.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index 23fe7db..45a3651 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -873,6 +873,9 @@ static int parse_insn_line(struct todo_item *item, const 
char *bol, char *eol)
char *end_of_object_name;
int i, saved, status, padding;
 
+   /* left-trim */
+   bol += strspn(bol, " \t");
+
for (i = 0; i < ARRAY_SIZE(todo_command_strings); i++)
if (skip_prefix(bol, todo_command_strings[i], &bol)) {
item->command = i;
-- 
2.10.0.windows.1.325.ge6089c1

[PATCH v3 13/25] sequencer: prepare for rebase -i's commit functionality

[Johannes Schindelin]

In interactive rebases, we commit a little bit differently than the
sequencer did so far: we heed the "author-script", the "message" and the
"amend" files in the .git/rebase-merge/ subdirectory.

Likewise, we may want to edit the commit message *even* when providing a
file containing the suggested commit message. Therefore we change the
code to not even provide a default message when we do not want any, and
to call the editor explicitly.

Also, in "interactive rebase" mode we want to skip reading the options
in the state directory of the cherry-pick/revert commands.

Finally, as interactive rebase's GPG settings are configured differently
from how cherry-pick (and therefore sequencer) handles them, we will
leave support for that to the next commit.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 99 ++---
 1 file changed, 89 insertions(+), 10 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 7ba5e07..b694ace 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -27,6 +27,19 @@ static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo")
 static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts")
 static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
 
+/*
+ * A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
+ * GIT_AUTHOR_DATE that will be used for the commit that is currently
+ * being rebased.
+ */
+static GIT_PATH_FUNC(rebase_path_author_script, "rebase-merge/author-script")
+
+/* We will introduce the 'interactive rebase' mode later */
+static inline int is_rebase_i(const struct replay_opts *opts)
+{
+   return 0;
+}
+
 static const char *get_dir(const struct replay_opts *opts)
 {
return git_path_seq_dir();
@@ -370,19 +383,79 @@ static int is_index_unchanged(void)
 }
 
 /*
+ * Read the author-script file into an environment block, ready for use in
+ * run_command(), that can be free()d afterwards.
+ */
+static char **read_author_script(void)
+{
+   struct strbuf script = STRBUF_INIT;
+   int i, count = 0;
+   char *p, *p2, **env;
+   size_t env_size;
+
+   if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)
+   return NULL;
+
+   for (p = script.buf; *p; p++)
+   if (skip_prefix(p, "'''", (const char **)&p2))
+   strbuf_splice(&script, p - script.buf, p2 - p, "'", 1);
+   else if (*p == '\'')
+   strbuf_splice(&script, p-- - script.buf, 1, "", 0);
+   else if (*p == '\n') {
+   *p = '\0';
+   count++;
+   }
+
+   env_size = (count + 1) * sizeof(*env);
+   strbuf_grow(&script, env_size);
+   memmove(script.buf + env_size, script.buf, script.len);
+   p = script.buf + env_size;
+   env = (char **)strbuf_detach(&script, NULL);
+
+   for (i = 0; i < count; i++) {
+   env[i] = p;
+   p += strlen(p) + 1;
+   }
+   env[count] = NULL;
+
+   return env;
+}
+
+/*
  * If we are cherry-pick, and if the merge did not result in
  * hand-editing, we will hit this commit and inherit the original
  * author date and name.
+ *
  * If we are revert, or if our cherry-pick results in a hand merge,
  * we had better say that the current user is responsible for that.
+ *
+ * An exception is when run_git_commit() is called during an
+ * interactive rebase: in that case, we will want to retain the
+ * author metadata.
  */
 static int run_git_commit(const char *defmsg, struct replay_opts *opts,
  int allow_empty)
 {
+   char **env = NULL;
struct argv_array array;
int rc;
const char *value;
 
+   if (is_rebase_i(opts)) {
+   env = read_author_script();
+   if (!env)
+   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 $gpg_sign_opt_quoted\n\n"
+   "If they are meant to go into a new commit, "
+   "run:\n\n"
+   "  git commit $gpg_sign_opt_quoted\n\n"
+   "In both cases, once you're done, continue "
+   "with:\n\n"
+   "  git rebase --continue\n");
+   }
+
argv_array_init(&array);
argv_array_push(&array, "commit");
argv_array_push(&array, "-n");
@@ -391,14 +464,13 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
argv_array_pushf(&array, "-S%s", opts->gpg_sign);
if (opts->signoff)
argv_array_push(&array, "-s");
-   if (!opts->edit) {
-   argv_array_push(&array, "-F");
-   argv_array_push(&array, defmsg);
-  

[PATCH v3 20/25] sequencer: refactor write_message()

[Johannes Schindelin]

The write_message() function safely writes an strbuf to a file.
Sometimes it is inconvenient to require an strbuf, though: the text to
be written may not be stored in a strbuf, or the strbuf should not be
released after writing.

Let's refactor "safely writing string to a file" into
write_with_lock_file(), and make write_message() use it. The new
function makes it easy to create new convenience function
write_file_gently(); as some of the upcoming callers of this new
function would want to append a newline character, add a flag for it in
write_file_gently(), and thus in write_with_lock_file().

While at it, roll back the locked files in case of failure, as pointed
out by Hannes Sixt.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 31 ++-
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 45a3651..5cca8d8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -234,22 +234,43 @@ static void print_advice(int show_hint, struct 
replay_opts *opts)
}
 }
 
-static int write_message(struct strbuf *msgbuf, const char *filename)
+static int write_with_lock_file(const char *filename,
+   const void *buf, size_t len, int append_eol)
 {
static struct lock_file msg_file;
 
int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);
if (msg_fd < 0)
return error_errno(_("Could not lock '%s'"), filename);
-   if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
-   return error_errno(_("Could not write to %s"), filename);
-   strbuf_release(msgbuf);
-   if (commit_lock_file(&msg_file) < 0)
+   if (write_in_full(msg_fd, buf, len) < 0) {
+   rollback_lock_file(&msg_file);
+   return error_errno(_("Could not write to '%s'"), filename);
+   }
+   if (append_eol && write(msg_fd, "\n", 1) < 0) {
+   rollback_lock_file(&msg_file);
+   return error_errno(_("Could not write eol to '%s"), filename);
+   }
+   if (commit_lock_file(&msg_file) < 0) {
+   rollback_lock_file(&msg_file);
return error(_("Error wrapping up %s."), filename);
+   }
 
return 0;
 }
 
+static int write_message(struct strbuf *msgbuf, const char *filename)
+{
+   int res = write_with_lock_file(filename, msgbuf->buf, msgbuf->len, 0);
+   strbuf_release(msgbuf);
+   return res;
+}
+
+static int write_file_gently(const char *filename,
+const char *text, int append_eol)
+{
+   return write_with_lock_file(filename, text, strlen(text), append_eol);
+}
+
 /*
  * Reads a file that was presumably written by a shell script, i.e.
  * with an end-of-line marker that needs to be stripped.
-- 
2.10.0.windows.1.325.ge6089c1

[PATCH v3 16/25] sequencer: support amending commits

[Johannes Schindelin]

This teaches the run_git_commit() function to take an argument that will
allow us to implement "todo" commands that need to amend the commit
messages ("fixup", "squash" and "reword").

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index b621f4b..403a4f0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -481,7 +481,7 @@ static char **read_author_script(void)
  * author metadata.
  */
 static int run_git_commit(const char *defmsg, struct replay_opts *opts,
- int allow_empty, int edit)
+ int allow_empty, int edit, int amend)
 {
char **env = NULL;
struct argv_array array;
@@ -510,6 +510,8 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
argv_array_push(&array, "commit");
argv_array_push(&array, "-n");
 
+   if (amend)
+   argv_array_push(&array, "--amend");
if (opts->gpg_sign)
argv_array_pushf(&array, "-S%s", opts->gpg_sign);
if (opts->signoff)
@@ -785,7 +787,7 @@ static int do_pick_commit(enum todo_command command, struct 
commit *commit,
}
if (!opts->no_commit)
res = run_git_commit(opts->edit ? NULL : git_path_merge_msg(),
-opts, allow, opts->edit);
+opts, allow, opts->edit, 0);
 
 leave:
free_message(commit, &msg);
-- 
2.10.0.windows.1.325.ge6089c1

[PATCH v3 17/25] sequencer: support cleaning up commit messages

[Johannes Schindelin]

The run_git_commit() function already knows how to amend commits, and
with this new option, it can also clean up commit messages (i.e. strip
out commented lines). This is needed to implement rebase -i's 'fixup'
and 'squash' commands as sequencer commands.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 403a4f0..108bca8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -481,7 +481,8 @@ static char **read_author_script(void)
  * author metadata.
  */
 static int run_git_commit(const char *defmsg, struct replay_opts *opts,
- int allow_empty, int edit, int amend)
+ int allow_empty, int edit, int amend,
+ int cleanup_commit_message)
 {
char **env = NULL;
struct argv_array array;
@@ -518,9 +519,12 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
argv_array_push(&array, "-s");
if (defmsg)
argv_array_pushl(&array, "-F", defmsg, NULL);
+   if (cleanup_commit_message)
+   argv_array_push(&array, "--cleanup=strip");
if (edit)
argv_array_push(&array, "-e");
-   else if (!opts->signoff && !opts->record_origin &&
+   else if (!cleanup_commit_message &&
+!opts->signoff && !opts->record_origin &&
 git_config_get_value("commit.cleanup", &value))
argv_array_push(&array, "--cleanup=verbatim");
 
@@ -787,7 +791,7 @@ static int do_pick_commit(enum todo_command command, struct 
commit *commit,
}
if (!opts->no_commit)
res = run_git_commit(opts->edit ? NULL : git_path_merge_msg(),
-opts, allow, opts->edit, 0);
+opts, allow, opts->edit, 0, 0);
 
 leave:
free_message(commit, &msg);
-- 
2.10.0.windows.1.325.ge6089c1

[PATCH v3 15/25] sequencer: allow editing the commit message on a case-by-case basis

[Johannes Schindelin]

In the upcoming commits, we will implement more and more of rebase -i's
functionality inside the sequencer. One particular feature of the
commands to come is that some of them allow editing the commit message
while others don't, i.e. we cannot define in the replay_opts whether the
commit message should be edited or not.

Let's add a new parameter to the run_git_commit() function. Previously,
it was the duty of the caller to ensure that the opts->edit setting
indicates whether to let the user edit the commit message or not,
indicating that it is an "all or nothing" setting, i.e. that the
sequencer wants to let the user edit *all* commit message, or none at
all. In the upcoming rebase -i mode, it will depend on the particular
command that is currently executed, though.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 48 
 1 file changed, 40 insertions(+), 8 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 681552a..b621f4b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -15,6 +15,7 @@
 #include "merge-recursive.h"
 #include "refs.h"
 #include "argv-array.h"
+#include "quote.h"
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
@@ -33,6 +34,11 @@ static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
  * being rebased.
  */
 static GIT_PATH_FUNC(rebase_path_author_script, "rebase-merge/author-script")
+/*
+ * The following files are written by git-rebase just after parsing the
+ * command-line (and are only consumed, not modified, by the sequencer).
+ */
+static GIT_PATH_FUNC(rebase_path_gpg_sign_opt, "rebase-merge/gpg_sign_opt")
 
 /* We will introduce the 'interactive rebase' mode later */
 static inline int is_rebase_i(const struct replay_opts *opts)
@@ -132,6 +138,16 @@ static int has_conforming_footer(struct strbuf *sb, struct 
strbuf *sob,
return 1;
 }
 
+static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
+{
+   static struct strbuf buf = STRBUF_INIT;
+
+   strbuf_reset(&buf);
+   if (opts->gpg_sign)
+   sq_quotef(&buf, "-S%s", opts->gpg_sign);
+   return buf.buf;
+}
+
 int sequencer_remove_state(struct replay_opts *opts)
 {
struct strbuf dir = STRBUF_INIT;
@@ -465,7 +481,7 @@ static char **read_author_script(void)
  * author metadata.
  */
 static int run_git_commit(const char *defmsg, struct replay_opts *opts,
- int allow_empty)
+ int allow_empty, int edit)
 {
char **env = NULL;
struct argv_array array;
@@ -474,17 +490,20 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
 
if (is_rebase_i(opts)) {
env = read_author_script();
-   if (!env)
+   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 $gpg_sign_opt_quoted\n\n"
+   "  git commit --amend %s\n\n"
"If they are meant to go into a new commit, "
"run:\n\n"
-   "  git commit $gpg_sign_opt_quoted\n\n"
+   "  git commit %s\n\n"
"In both cases, once you're done, continue "
"with:\n\n"
-   "  git rebase --continue\n");
+   "  git rebase --continue\n", gpg_opt, gpg_opt);
+   }
}
 
argv_array_init(&array);
@@ -497,7 +516,7 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
argv_array_push(&array, "-s");
if (defmsg)
argv_array_pushl(&array, "-F", defmsg, NULL);
-   if (opts->edit)
+   if (edit)
argv_array_push(&array, "-e");
else if (!opts->signoff && !opts->record_origin &&
 git_config_get_value("commit.cleanup", &value))
@@ -766,7 +785,7 @@ static int do_pick_commit(enum todo_command command, struct 
commit *commit,
}
if (!opts->no_commit)
res = run_git_commit(opts->edit ? NULL : git_path_merge_msg(),
-opts, allow);
+opts, allow, opts->edit);
 
 leave:
free_message(commit, &msg);
@@ -990,8 +1009,21 @@ static int populate_opts_cb(const char *key, const char 
*value, void *data)
 
 static int read_populate_opts(struct replay_opts *opts)
 {
-   if (is_rebase_i(opts))
+   if (is_rebase_i(opts)) {
+   struct strbuf buf = STRBUF_INIT;
+
+   if (read_oneliner(&buf, rebase_path_gpg_sign_opt(), 1)) {
+   if (!starts_with(buf.buf,

[PATCH v3 14/25] sequencer: introduce a helper to read files written by scripts

[Johannes Schindelin]

As we are slowly teaching the sequencer to perform the hard work for
the interactive rebase, we need to read files that were written by
shell scripts.

These files typically contain a single line and are invariably ended
by a line feed (and possibly a carriage return before that). Let's use
a helper to read such files and to remove the line ending.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 31 +++
 1 file changed, 31 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index b694ace..681552a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -234,6 +234,37 @@ static int write_message(struct strbuf *msgbuf, const char 
*filename)
return 0;
 }
 
+/*
+ * Reads a file that was presumably written by a shell script, i.e.
+ * with an end-of-line marker that needs to be stripped.
+ *
+ * Returns 1 if the file was read, 0 if it could not be read or does not exist.
+ */
+static int read_oneliner(struct strbuf *buf,
+   const char *path, int skip_if_empty)
+{
+   int orig_len = buf->len;
+
+   if (!file_exists(path))
+   return 0;
+
+   if (strbuf_read_file(buf, path, 0) < 0) {
+   warning_errno(_("could not read '%s'"), path);
+   return 0;
+   }
+
+   if (buf->len > orig_len && buf->buf[buf->len - 1] == '\n') {
+   if (--buf->len > orig_len && buf->buf[buf->len - 1] == '\r')
+   --buf->len;
+   buf->buf[buf->len] = '\0';
+   }
+
+   if (skip_if_empty && buf->len == orig_len)
+   return 0;
+
+   return 1;
+}
+
 static struct tree *empty_tree(void)
 {
return lookup_tree(EMPTY_TREE_SHA1_BIN);
-- 
2.10.0.windows.1.325.ge6089c1

[PATCH v3 18/25] sequencer: do not try to commit when there were merge conflicts

[Johannes Schindelin]

The return value of do_recursive_merge() may be positive (indicating merge
conflicts), or 0 (indicating success). It also may be negative, indicating
a fatal error that requires us to abort.

Now, if the return value indicates that there are merge conflicts, we
should not try to commit those changes, of course.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 108bca8..23fe7db 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -789,7 +789,7 @@ static int do_pick_commit(enum todo_command command, struct 
commit *commit,
res = allow;
goto leave;
}
-   if (!opts->no_commit)
+   if (!res && !opts->no_commit)
res = run_git_commit(opts->edit ? NULL : git_path_merge_msg(),
 opts, allow, opts->edit, 0, 0);
 
-- 
2.10.0.windows.1.325.ge6089c1

[PATCH v3 10/25] sequencer: get rid of the subcommand field

[Johannes Schindelin]

The subcommands are used exactly once, at the very beginning of
sequencer_pick_revisions(), to determine what to do. This is an
unnecessary level of indirection: we can simply call the correct
function to begin with. So let's do that.

While at it, ensure that the subcommands return an error code so that
they do not have to die() all over the place (bad practice for library
functions...).

Signed-off-by: Johannes Schindelin 
---
 builtin/revert.c | 36 
 sequencer.c  | 35 +++
 sequencer.h  | 13 -
 3 files changed, 31 insertions(+), 53 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index fce9c75..0a7b5f4 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -71,7 +71,7 @@ static void verify_opt_compatible(const char *me, const char 
*base_opt, ...)
die(_("%s: %s cannot be used with %s"), me, this_opt, base_opt);
 }
 
-static void parse_args(int argc, const char **argv, struct replay_opts *opts)
+static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 {
const char * const * usage_str = revert_or_cherry_pick_usage(opts);
const char *me = action_name(opts);
@@ -115,25 +115,15 @@ static void parse_args(int argc, const char **argv, 
struct replay_opts *opts)
if (opts->keep_redundant_commits)
opts->allow_empty = 1;
 
-   /* Set the subcommand */
-   if (cmd == 'q')
-   opts->subcommand = REPLAY_REMOVE_STATE;
-   else if (cmd == 'c')
-   opts->subcommand = REPLAY_CONTINUE;
-   else if (cmd == 'a')
-   opts->subcommand = REPLAY_ROLLBACK;
-   else
-   opts->subcommand = REPLAY_NONE;
-
/* Check for incompatible command line arguments */
-   if (opts->subcommand != REPLAY_NONE) {
+   if (cmd) {
char *this_operation;
-   if (opts->subcommand == REPLAY_REMOVE_STATE)
+   if (cmd == 'q')
this_operation = "--quit";
-   else if (opts->subcommand == REPLAY_CONTINUE)
+   else if (cmd == 'c')
this_operation = "--continue";
else {
-   assert(opts->subcommand == REPLAY_ROLLBACK);
+   assert(cmd == 'a');
this_operation = "--abort";
}
 
@@ -156,7 +146,7 @@ static void parse_args(int argc, const char **argv, struct 
replay_opts *opts)
"--edit", opts->edit,
NULL);
 
-   if (opts->subcommand != REPLAY_NONE) {
+   if (cmd) {
opts->revs = NULL;
} else {
struct setup_revision_opt s_r_opt;
@@ -180,6 +170,14 @@ static void parse_args(int argc, const char **argv, struct 
replay_opts *opts)
opts->gpg_sign = xstrdup(opts->gpg_sign);
if (opts->strategy)
opts->strategy = xstrdup(opts->strategy);
+
+   if (cmd == 'q')
+   return sequencer_remove_state(opts);
+   if (cmd == 'c')
+   return sequencer_continue(opts);
+   if (cmd == 'a')
+   return sequencer_rollback(opts);
+   return sequencer_pick_revisions(opts);
 }
 
 int cmd_revert(int argc, const char **argv, const char *prefix)
@@ -191,8 +189,7 @@ int cmd_revert(int argc, const char **argv, const char 
*prefix)
opts.edit = 1;
opts.action = REPLAY_REVERT;
git_config(git_default_config, NULL);
-   parse_args(argc, argv, &opts);
-   res = sequencer_pick_revisions(&opts);
+   res = run_sequencer(argc, argv, &opts);
if (res < 0)
die(_("revert failed"));
return res;
@@ -205,8 +202,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char 
*prefix)
 
opts.action = REPLAY_PICK;
git_config(git_default_config, NULL);
-   parse_args(argc, argv, &opts);
-   res = sequencer_pick_revisions(&opts);
+   res = run_sequencer(argc, argv, &opts);
if (res < 0)
die(_("cherry-pick failed"));
return res;
diff --git a/sequencer.c b/sequencer.c
index 443a238..14b1746 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -119,7 +119,7 @@ static int has_conforming_footer(struct strbuf *sb, struct 
strbuf *sob,
return 1;
 }
 
-static void remove_sequencer_state(const struct replay_opts *opts)
+int sequencer_remove_state(struct replay_opts *opts)
 {
struct strbuf dir = STRBUF_INIT;
int i;
@@ -133,6 +133,8 @@ static void remove_sequencer_state(const struct replay_opts 
*opts)
strbuf_addf(&dir, "%s", get_dir(opts));
remove_dir_recursively(&dir, 0);
strbuf_release(&dir);
+
+   return 0;
 }
 
 static const char *action_name(const struct replay_opts *opts)
@@ -977,7 +979,7 @@ static int rollback_single_pick(void)
return reset_for_rollback(head_sha1);
 }
 
-static in

[PATCH v3 12/25] sequencer: remember the onelines when parsing the todo file

[Johannes Schindelin]

The `git-rebase-todo` file contains a list of commands. Most of those
commands have the form

  

The  is displayed primarily for the user's convenience, as
rebase -i really interprets only the   part. However, there
are *some* places in interactive rebase where the  is used to
display messages, e.g. for reporting at which commit we stopped.

So let's just remember it when parsing the todo file; we keep a copy of
the entire todo file anyway (to write out the new `done` and
`git-rebase-todo` file just before processing each command), so all we
need to do is remember the begin offsets and lengths.

As we will have to parse and remember the command-line for `exec` commands
later, we do not call the field "oneline" but rather "arg" (and will reuse
that for exec's command-line).

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index afc494e..7ba5e07 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -708,6 +708,8 @@ static int read_and_refresh_cache(struct replay_opts *opts)
 struct todo_item {
enum todo_command command;
struct commit *commit;
+   const char *arg;
+   int arg_len;
size_t offset_in_buf;
 };
 
@@ -759,6 +761,9 @@ static int parse_insn_line(struct todo_item *item, const 
char *bol, char *eol)
status = get_sha1(bol, commit_sha1);
*end_of_object_name = saved;
 
+   item->arg = end_of_object_name + strspn(end_of_object_name, " \t");
+   item->arg_len = (int)(eol - item->arg);
+
if (status < 0)
return -1;
 
@@ -911,6 +916,8 @@ static int walk_revs_populate_todo(struct todo_list 
*todo_list,
 
item->command = command;
item->commit = commit;
+   item->arg = NULL;
+   item->arg_len = 0;
item->offset_in_buf = todo_list->buf.len;
subject_len = find_commit_subject(commit_buffer, &subject);
strbuf_addf(&todo_list->buf, "%s %s %.*s\n", command_string,
-- 
2.10.0.windows.1.325.ge6089c1

[PATCH v3 24/25] sequencer: start error messages consistently with lower case

[Johannes Schindelin]

Quite a few error messages touched by this developer during the work to
speed up rebase -i started with an upper case letter, violating our
current conventions. Instead of sneaking in this fix (and forgetting
quite a few error messages), let's just have one wholesale patch fixing
all of the error messages in the sequencer.

While at it, the funny "error: Error wrapping up..." was changed to a
less funny, but more helpful, "error: failed to finalize...".

Pointed out by Junio Hamano.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c   | 68 +--
 t/t3501-revert-cherry-pick.sh |  2 +-
 2 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 4596540..676f16c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -241,18 +241,18 @@ static int write_with_lock_file(const char *filename,
 
int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);
if (msg_fd < 0)
-   return error_errno(_("Could not lock '%s'"), filename);
+   return error_errno(_("could not lock '%s'"), filename);
if (write_in_full(msg_fd, buf, len) < 0) {
rollback_lock_file(&msg_file);
-   return error_errno(_("Could not write to '%s'"), filename);
+   return error_errno(_("could not write to '%s'"), filename);
}
if (append_eol && write(msg_fd, "\n", 1) < 0) {
rollback_lock_file(&msg_file);
-   return error_errno(_("Could not write eol to '%s"), filename);
+   return error_errno(_("could not write eol to '%s"), filename);
}
if (commit_lock_file(&msg_file) < 0) {
rollback_lock_file(&msg_file);
-   return error(_("Error wrapping up '%s'."), filename);
+   return error(_("failed to finalize '%s'."), filename);
}
 
return 0;
@@ -312,11 +312,11 @@ static int error_dirty_index(struct replay_opts *opts)
if (read_cache_unmerged())
return error_resolve_conflict(_(action_name(opts)));
 
-   error(_("Your local changes would be overwritten by %s."),
+   error(_("your local changes would be overwritten by %s."),
_(action_name(opts)));
 
if (advice_commit_before_merge)
-   advise(_("Commit your changes or stash them to proceed."));
+   advise(_("commit your changes or stash them to proceed."));
return -1;
 }
 
@@ -425,7 +425,7 @@ static int is_index_unchanged(void)
struct commit *head_commit;
 
if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, head_sha1, NULL))
-   return error(_("Could not resolve HEAD commit\n"));
+   return error(_("could not resolve HEAD commit\n"));
 
head_commit = lookup_commit(head_sha1);
 
@@ -445,7 +445,7 @@ static int is_index_unchanged(void)
 
if (!cache_tree_fully_valid(active_cache_tree))
if (cache_tree_update(&the_index, 0))
-   return error(_("Unable to update cache tree\n"));
+   return error(_("unable to update cache tree\n"));
 
return !hashcmp(active_cache_tree->sha1, 
head_commit->tree->object.oid.hash);
 }
@@ -515,7 +515,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 "
+   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"
@@ -568,12 +568,12 @@ static int is_original_commit_empty(struct commit *commit)
const unsigned char *ptree_sha1;
 
if (parse_commit(commit))
-   return error(_("Could not parse commit %s\n"),
+   return error(_("could not parse commit %s\n"),
 oid_to_hex(&commit->object.oid));
if (commit->parents) {
struct commit *parent = commit->parents->item;
if (parse_commit(parent))
-   return error(_("Could not parse parent commit %s\n"),
+   return error(_("could not parse parent commit %s\n"),
oid_to_hex(&parent->object.oid));
ptree_sha1 = parent->tree->object.oid.hash;
} else {
@@ -657,7 +657,7 @@ static int do_pick_commit(enum todo_command command, struct 
commit *commit,
 * to work on.
 */
if (write_cache_as_tree(head, 0, NULL))
-   return error(_("Your index file is unmerged."));
+   return error(_("your index file is unmerged."));
} else {

[PATCH v3 23/25] sequencer: quote filenames in error messages

[Johannes Schindelin]

This makes the code consistent by fixing quite a couple of error messages.

Suggested by Jakub Narębski.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 22 +++---
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 40ef33c..4596540 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -252,7 +252,7 @@ static int write_with_lock_file(const char *filename,
}
if (commit_lock_file(&msg_file) < 0) {
rollback_lock_file(&msg_file);
-   return error(_("Error wrapping up %s."), filename);
+   return error(_("Error wrapping up '%s'."), filename);
}
 
return 0;
@@ -963,16 +963,16 @@ static int read_populate_todo(struct todo_list *todo_list,
strbuf_reset(&todo_list->buf);
fd = open(todo_file, O_RDONLY);
if (fd < 0)
-   return error_errno(_("Could not open %s"), todo_file);
+   return error_errno(_("Could not open '%s'"), todo_file);
if (strbuf_read(&todo_list->buf, fd, 0) < 0) {
close(fd);
-   return error(_("Could not read %s."), todo_file);
+   return error(_("Could not read '%s'."), todo_file);
}
close(fd);
 
res = parse_insn_buffer(todo_list->buf.buf, todo_list);
if (res)
-   return error(_("Unusable instruction sheet: %s"), todo_file);
+   return error(_("Unusable instruction sheet: '%s'"), todo_file);
 
if (!is_rebase_i(opts)) {
enum todo_command valid =
@@ -1065,7 +1065,7 @@ static int read_populate_opts(struct replay_opts *opts)
 * are pretty certain that it is syntactically correct.
 */
if (git_config_from_file(populate_opts_cb, git_path_opts_file(), opts) 
< 0)
-   return error(_("Malformed options sheet: %s"),
+   return error(_("Malformed options sheet: '%s'"),
git_path_opts_file());
return 0;
 }
@@ -1108,7 +1108,7 @@ static int create_seq_dir(void)
return -1;
}
else if (mkdir(git_path_seq_dir(), 0777) < 0)
-   return error_errno(_("Could not create sequencer directory %s"),
+   return error_errno(_("Could not create sequencer directory 
'%s'"),
   git_path_seq_dir());
return 0;
 }
@@ -1127,12 +1127,12 @@ static int save_head(const char *head)
strbuf_addf(&buf, "%s\n", head);
if (write_in_full(fd, buf.buf, buf.len) < 0) {
rollback_lock_file(&head_lock);
-   return error_errno(_("Could not write to %s"),
+   return error_errno(_("Could not write to '%s'"),
   git_path_head_file());
}
if (commit_lock_file(&head_lock) < 0) {
rollback_lock_file(&head_lock);
-   return error(_("Error wrapping up %s."), git_path_head_file());
+   return error(_("Error wrapping up '%s'."), 
git_path_head_file());
}
return 0;
 }
@@ -1177,9 +1177,9 @@ int sequencer_rollback(struct replay_opts *opts)
return rollback_single_pick();
}
if (!f)
-   return error_errno(_("cannot open %s"), git_path_head_file());
+   return error_errno(_("cannot open '%s'"), git_path_head_file());
if (strbuf_getline_lf(&buf, f)) {
-   error(_("cannot read %s: %s"), git_path_head_file(),
+   error(_("cannot read '%s': %s"), git_path_head_file(),
  ferror(f) ?  strerror(errno) : _("unexpected end of 
file"));
fclose(f);
goto fail;
@@ -1218,7 +1218,7 @@ static int save_todo(struct todo_list *todo_list, struct 
replay_opts *opts)
todo_list->buf.len - offset) < 0)
return error_errno(_("Could not write to '%s'"), todo_path);
if (commit_lock_file(&todo_lock) < 0)
-   return error(_("Error wrapping up %s."), todo_path);
+   return error(_("Error wrapping up '%s'."), todo_path);
return 0;
 }
 
-- 
2.10.0.windows.1.325.ge6089c1

[PATCH v3 25/25] sequencer: mark all error messages for translation

[Johannes Schindelin]

There was actually only one error message that was not yet marked for
translation.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 23 +--
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 676f16c..86d86ce 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -515,16 +515,19 @@ 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", gpg_opt, gpg_opt);
+   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"),
+gpg_opt, gpg_opt);
}
}
 
-- 
2.10.0.windows.1.325.ge6089c1

[PATCH v3 22/25] sequencer: mark action_name() for translation

[Johannes Schindelin]

The definition of this function goes back all the way to 043a449
(sequencer: factor code out of revert builtin, 2012-01-11), long before a
serious effort was made to translate all the error messages.

It is slightly out of the context of the current patch series (whose
purpose it is to re-implement the performance critical parts of the
interactive rebase in C) to make the error messages in the sequencer
translatable, but what the heck. We'll just do it while we're looking at
this part of the code.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index ffce095..40ef33c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -168,7 +168,7 @@ int sequencer_remove_state(struct replay_opts *opts)
 
 static const char *action_name(const struct replay_opts *opts)
 {
-   return opts->action == REPLAY_REVERT ? "revert" : "cherry-pick";
+   return opts->action == REPLAY_REVERT ? N_("revert") : N_("cherry-pick");
 }
 
 struct commit_message {
@@ -310,10 +310,10 @@ static struct tree *empty_tree(void)
 static int error_dirty_index(struct replay_opts *opts)
 {
if (read_cache_unmerged())
-   return error_resolve_conflict(action_name(opts));
+   return error_resolve_conflict(_(action_name(opts)));
 
error(_("Your local changes would be overwritten by %s."),
-   action_name(opts));
+   _(action_name(opts)));
 
if (advice_commit_before_merge)
advise(_("Commit your changes or stash them to proceed."));
@@ -331,7 +331,7 @@ static int fast_forward_to(const unsigned char *to, const 
unsigned char *from,
if (checkout_fast_forward(from, to, 1))
return -1; /* the callee should have complained already */
 
-   strbuf_addf(&sb, _("%s: fast-forward"), action_name(opts));
+   strbuf_addf(&sb, _("%s: fast-forward"), _(action_name(opts)));
 
transaction = ref_transaction_begin(&err);
if (!transaction ||
@@ -407,7 +407,7 @@ static int do_recursive_merge(struct commit *base, struct 
commit *next,
write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
/* TRANSLATORS: %s will be "revert" or "cherry-pick" */
return error(_("%s: Unable to write new index file"),
-   action_name(opts));
+   _(action_name(opts)));
rollback_lock_file(&index_lock);
 
if (opts->signoff)
@@ -844,14 +844,14 @@ static int read_and_refresh_cache(struct replay_opts 
*opts)
if (read_index_preload(&the_index, NULL) < 0) {
rollback_lock_file(&index_lock);
return error(_("git %s: failed to read the index"),
-   action_name(opts));
+   _(action_name(opts)));
}
refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, 
NULL);
if (the_index.cache_changed && index_fd >= 0) {
if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) {
rollback_lock_file(&index_lock);
return error(_("git %s: failed to refresh the index"),
-   action_name(opts));
+   _(action_name(opts)));
}
}
rollback_lock_file(&index_lock);
-- 
2.10.0.windows.1.325.ge6089c1

[PATCH v3 21/25] sequencer: remove overzealous assumption in rebase -i mode

[Johannes Schindelin]

The sequencer was introduced to make the cherry-pick and revert
functionality available as library function, with the original idea
being to extend the sequencer to also implement the rebase -i
functionality.

The test to ensure that all of the commands in the script are identical
to the overall operation does not mesh well with that.

Therefore let's disable the test in rebase -i mode.

While at it, error out early if the "instruction sheet" (i.e. the todo
script) could not be parsed.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 5cca8d8..ffce095 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -971,7 +971,10 @@ static int read_populate_todo(struct todo_list *todo_list,
close(fd);
 
res = parse_insn_buffer(todo_list->buf.buf, todo_list);
-   if (!res) {
+   if (res)
+   return error(_("Unusable instruction sheet: %s"), todo_file);
+
+   if (!is_rebase_i(opts)) {
enum todo_command valid =
opts->action == REPLAY_PICK ? TODO_PICK : TODO_REVERT;
int i;
@@ -985,8 +988,6 @@ static int read_populate_todo(struct todo_list *todo_list,
return error(_("Cannot revert during a 
cherry-pick."));
}
 
-   if (res)
-   return error(_("Unusable instruction sheet: %s"), todo_file);
return 0;
 }
 
-- 
2.10.0.windows.1.325.ge6089c1

Re: git 2.10.1 test regression in t3700-add.sh

[Junio C Hamano]

Jeremy Huddleston Sequoia  writes:

> Actually, looks like that as just a rabbit hole.  The real issue
> looks to be because an earlier test drops down xfoo3 as a symlink,
> which causes this test to fail due to the collision.  I'll get out
> a patch in a bit.

[administrivia: please don't top-post, it is extremely hard to
follow what is going on]

There indeed is a test that creates xfoo3 as a symbolic link and
leaves it in the filesystem pointing at xfoo2 much earlier in the
sequence.  It seems that later one of the "add ignore-errors" tests
(there are two back-to-back) runs "git reset --hard" to make it (and
other symbolic links that are similarly named) go away, namely this
part of the code:

test_expect_success POSIXPERM,SANITY 'git add --ignore-errors' '
git reset --hard &&
date >foo1 &&
date >foo2 &&
chmod 0 foo2 &&
test_must_fail git add --verbose --ignore-errors . &&
git ls-files foo1 | grep foo1
'

rm -f foo2

test_expect_success POSIXPERM,SANITY 'git add (add.ignore-errors)' '
git config add.ignore-errors 1 &&
git reset --hard &&
date >foo1 &&
date >foo2 &&
chmod 0 foo2 &&
test_must_fail git add --verbose . &&
git ls-files foo1 | grep foo1
'
rm -f foo2

"git reset --hard" in the first one, because these symbolic links
are not in the index at that point in the sequence, would leave them
untracked and in the working tree.  Then "add" is told to be
non-atomic with "--ignore-errors", adding them to the index while
reporting a failure.  When the test after that runs "git reset --hard"
again, because these symlinks are in the index (and not in HEAD),
they are removed from the working tree.

And that is why normal people won't see xfoo3 in later tests, like
the one you had trouble with.

Are you running without SANITY by any chance (I am not saying that
you are doing a wrong thing--just trying to make sure I am guessing
along the correct route)?

If that is the issue, then I think the right correction would be to
make sure that the files that an individual test expects to be
missing from the file system is indeed missing by explicitly
removing it, perhaps like this.

I also notice that the problematic test uses "chmod 755"; don't we
need POSIXPERM prerequisite on this test, too, I wonder?

Thanks.

-- >8 --
t3700: fix broken test under !SANITY

An "add --chmod=+x" test recently added by 610d55af0f ("add: modify
already added files when --chmod is given", 2016-09-14) used "xfoo3"
as a test file.  The paths xfoo[1-3] were used by earlier tests for
symbolic links but they were expected to have been removed by the
test script reached this new test.

The removal with "git reset --hard" however happened in tests that
are protected by POSIXPERM,SANITY prerequisites.  Platforms and test
environments that lacked these would have seen xfoo3 as a leftover
symbolic link, pointing somewhere else, and chmod test would have
given a wrong result.



 t/t3700-add.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 924a266126..53c0cb6dea 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -350,6 +350,7 @@ test_expect_success POSIXPERM,SYMLINKS 'git add --chmod=+x 
with symlinks' '
 '
 
 test_expect_success 'git add --chmod=[+-]x changes index with already added 
file' '
+   rm -f foo3 xfoo3 &&
echo foo >foo3 &&
git add foo3 &&
git add --chmod=+x foo3 &&

Re: [PATCH] pretty: respect color settings for %C placeholders

[Jeff King]

On Mon, Oct 10, 2016 at 07:09:12PM +0200, René Scharfe wrote:

> > diff --git a/pretty.c b/pretty.c
> > index 25efbca..73e58b5 100644
> > --- a/pretty.c
> > +++ b/pretty.c
> > @@ -965,22 +965,31 @@ static size_t parse_color(struct strbuf *sb, /* in 
> > UTF-8 */
> > 
> > if (!end)
> > return 0;
> > -   if (skip_prefix(begin, "auto,", &begin)) {
> > +
> > +   if (!skip_prefix(begin, "always,", &begin)) {
> > if (!want_color(c->pretty_ctx->color))
> > return end - placeholder + 1;
> > }
> 
> Shouldn't we have an "else" here?

I'm not sure what you mean; can you write it out?

> > -   if (skip_prefix(placeholder + 1, "red", &rest))
> > +   if (skip_prefix(placeholder + 1, "red", &rest) &&
> > +   want_color(c->pretty_ctx->color))
> > strbuf_addstr(sb, GIT_COLOR_RED);
> > -   else if (skip_prefix(placeholder + 1, "green", &rest))
> > +   else if (skip_prefix(placeholder + 1, "green", &rest) &&
> > +want_color(c->pretty_ctx->color))
> > strbuf_addstr(sb, GIT_COLOR_GREEN);
> > -   else if (skip_prefix(placeholder + 1, "blue", &rest))
> > +   else if (skip_prefix(placeholder + 1, "blue", &rest) &&
> > +want_color(c->pretty_ctx->color))
> > strbuf_addstr(sb, GIT_COLOR_BLUE);
> > -   else if (skip_prefix(placeholder + 1, "reset", &rest))
> > +   else if (skip_prefix(placeholder + 1, "reset", &rest) &&
> > +want_color(c->pretty_ctx->color))
> > strbuf_addstr(sb, GIT_COLOR_RESET);
> > return rest - placeholder;
> >  }
> 
> Perhaps it's a funtion like add_color(sb, ctx, color) or similar would be
> nice?

I actually wrote it that way first (I called it "maybe_add_color()"),
but it felt a little funny to have a separate function that people might
be tempted to reuse (the right solution is generally to check
want_color() early as above, but we can't do that here because we have
to find the end of each placeholder).

What I have here is a little funny, too, though, as it keeps trying
other color names if it finds "red" but want_color() returns 0.

-Peff

Re: git 2.10.1 test regression in t3700-add.sh

[Junio C Hamano]

Junio C Hamano  writes:

> ...
> I also notice that the problematic test uses "chmod 755"; don't we
> need POSIXPERM prerequisite on this test, too, I wonder?
>
> Thanks.
>
> -- >8 --
> t3700: fix broken test under !SANITY
>
> An "add --chmod=+x" test recently added by 610d55af0f ("add: modify
> already added files when --chmod is given", 2016-09-14) used "xfoo3"
> as a test file.  The paths xfoo[1-3] were used by earlier tests for
> symbolic links but they were expected to have been removed by the
> test script reached this new test.

Eh, "removed by the time test script reached..."

Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd

[Junio C Hamano]

Jeff King  writes:

> Having separate exec/shell boolean options just punts the overlap from
> the command key to those keys. If you have two mutually exclusive
> options, I think the best thing is a single option, like:
>
>   type = 
>
> and then it is obvious that a second appearance of "type" overrides an
> earlier one, by our usual "last one wins" convention. As opposed to:
>
>   shell = true
>   exec = true
>
> where you have to understand the meaning of each option to know that
> "exec" overrides "shell".

Good.  

Duy's "do we want to chdir or stay?" would be an orthogonal axis to
"what does the command line look like?" and "how is the command line
run?" so it adds one member to the "alias..*" family of
variables, I guess.

Re: git merge deletes my changes

[Paul Smith]

On Mon, 2016-10-10 at 10:19 +, Eduard Egorov wrote:
> # ~/gitbuild/git-2.10.1/git merge -s subtree --squash ceph_ansible
> 
> Can somebody confirm this please? Doesn't "merge -s subtree" really
> merges branches?

I think possibly you're not fully understanding what the --squash flag
does... that's what's causing your issue here, not the "-s" option.

A squash merge takes the commits that would be merged from the origin
branch and squashes them into a single patch and applies them to the
current branch as a new commit... but this new commit is not a merge
commit (that is, when you look at it with "git show" etc. the commit
will have only one parent, not two--or more--parents like a normal merge
commit).

Basically, it's syntactic sugar for a diff plus patch operation plus
some Git goodness wrapped around it to make it easier to use.

But ultimately once you're done, Git has no idea that this new commit
has any relationship whatsoever to the origin branch.  So the next time
you merge, Git doesn't know that there was a previous merge and it will
try to merge everything from scratch rather than starting at the
previous common merge point.

So either you'll have to use a normal, non-squash merge, or else you'll
have to tell Git by hand what the previous common merge point was (as
Jeff King's excellent email suggests).  Or else, you'll have to live
with this behavior.

[PATCH 1/2] submodule: ignore trailing slash on superproject URL

[Stefan Beller]

Before 63e95beb0 (2016-04-15, submodule: port resolve_relative_url from
shell to C), it did not matter if the superprojects URL had a trailing
slash or not. It was just chopped off as one of the first steps
(The "remoteurl=${remoteurl%/}" near the beginning of
resolve_relative_url(), which was removed in said commit).

When porting this to the C version, an off-by-one error was introduced
and we did not check the actual last character to be a slash, but the
NULL delimiter.

Reintroduce the behavior from before 63e95beb0, to ignore the trailing
slash.

Reported-by: 
Helped-by: Dennis Kaarsemaker 
Signed-off-by: Stefan Beller 
---
 builtin/submodule--helper.c | 6 --
 t/t0060-path-utils.sh   | 1 +
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 444ec06..a7841a5 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -95,6 +95,8 @@ static int chop_last_dir(char **remoteurl, int is_relative)
  * NEEDSWORK: This works incorrectly on the domain and protocol part.
  * remote_url  url  outcome  expectation
  * http://a.com/b  ../c http://a.com/c   as is
+ * http://a.com/b/ ../c http://a.com/c   same as previous line, but
+ *   ignore trailing slash in 
url
  * http://a.com/b  ../../c  http://c error out
  * http://a.com/b  ../../../c   http:/c  error out
  * http://a.com/b  ../../../../chttp:c   error out
@@ -113,8 +115,8 @@ static char *relative_url(const char *remote_url,
struct strbuf sb = STRBUF_INIT;
size_t len = strlen(remoteurl);
 
-   if (is_dir_sep(remoteurl[len]))
-   remoteurl[len] = '\0';
+   if (is_dir_sep(remoteurl[len-1]))
+   remoteurl[len-1] = '\0';
 
if (!url_is_local_not_ssh(remoteurl) || is_absolute_path(remoteurl))
is_relative = 0;
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index bf2deee..82b98f8 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -319,6 +319,7 @@ test_submodule_relative_url "../" "foo/bar" "../submodule" 
"../foo/submodule"
 test_submodule_relative_url "../" "foo" "../submodule" "../submodule"
 
 test_submodule_relative_url "(null)" "../foo/bar" "../sub/a/b/c" 
"../foo/sub/a/b/c"
+test_submodule_relative_url "(null)" "../foo/bar/" "../sub/a/b/c" 
"../foo/sub/a/b/c"
 test_submodule_relative_url "(null)" "../foo/bar" "../submodule" 
"../foo/submodule"
 test_submodule_relative_url "(null)" "../foo/submodule" "../submodule" 
"../foo/submodule"
 test_submodule_relative_url "(null)" "../foo" "../submodule" "../submodule"
-- 
2.10.1.382.ga23ca1b.dirty

Re: [PATCH v2] gpg-interface: use more status letters

[Junio C Hamano]

Michael J Gruber  writes:

> Sorry, this got "lost in vacation". Before that, I was looking for an
> easy way to test expired signatures, but gpg1 and gpg2 behave somewhat
> differently in that respect (2 does not allow to create already expired
> signatures).
>
> Is there anything I should or could do now?

I dunno.  It's your itch.

You can say "I'll need more time to figure out the way to test what
I am not testing here, so do not merge it to 'next' yet".

You can also say "This is good enough for now, so go ahead and merge
it to 'next'; more detailed tests can be done as follow-up patches
if needed".

You can also say "Thinking about it again, there is no strong reason
why we need to differentiate EXPSIG and EXPKEYSIG, so don't do this
SQUASH and use my original one as-is".

I'd be happy with any of the above and there may be other ones I'd
be happy with that I haven't thought of ;-)

[PATCH 2/2] submodule: ignore trailing slash in relative url

[Stefan Beller]

This is similar to the previous patch, though no user reported a bug and
I could not find a regressive behavior.

However it is a good thing to be strict on the output and for that we
always omit a trailing slash.

Signed-off-by: Stefan Beller 
---
 builtin/submodule--helper.c | 2 ++
 t/t0060-path-utils.sh   | 1 +
 2 files changed, 3 insertions(+)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a7841a5..260f46f 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -149,6 +149,8 @@ static char *relative_url(const char *remote_url,
}
strbuf_reset(&sb);
strbuf_addf(&sb, "%s%s%s", remoteurl, colonsep ? ":" : "/", url);
+   if (ends_with(url, "/"))
+   strbuf_setlen(&sb, sb.len - 1);
free(remoteurl);
 
if (starts_with_dot_slash(sb.buf))
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 82b98f8..25b48e5 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -319,6 +319,7 @@ test_submodule_relative_url "../" "foo/bar" "../submodule" 
"../foo/submodule"
 test_submodule_relative_url "../" "foo" "../submodule" "../submodule"
 
 test_submodule_relative_url "(null)" "../foo/bar" "../sub/a/b/c" 
"../foo/sub/a/b/c"
+test_submodule_relative_url "(null)" "../foo/bar" "../sub/a/b/c/" 
"../foo/sub/a/b/c"
 test_submodule_relative_url "(null)" "../foo/bar/" "../sub/a/b/c" 
"../foo/sub/a/b/c"
 test_submodule_relative_url "(null)" "../foo/bar" "../submodule" 
"../foo/submodule"
 test_submodule_relative_url "(null)" "../foo/submodule" "../submodule" 
"../foo/submodule"
-- 
2.10.1.382.ga23ca1b.dirty

Re: [PATCH] documentation: clarify submodule..[url, path] variables

[Junio C Hamano]

Stefan Beller  writes:
>  submodule..path::
> + The path within this project for a submodule. This variable is
> + kept in the .gitmodules file. See linkgit:git-submodule[1] and
> + linkgit:gitmodules[5] for details.

What does it exactly mean to be "kept"?

Does it mean "never appears in .git/config, and when it appears it
will not be used at all"?  If so we shouldn't even list it here.

I doubt there is any reason for .path to exist in .git/config; where
each submodule appears in the working tree is what is recorded in
the tree object, and the "identity" (i.e. that which links a
submodule in a tree to one of the repositories kept in
.git/modules/*) by reverse look-up of submodule..path from
in-tree .gitmodules, not from configuration, and it is not something
a per-repository configuration should be able to change at the
conceptual level.

>  submodule..url::
> - The path within this project and URL for a submodule. These
> - variables are initially populated by 'git submodule init'. See
> - linkgit:git-submodule[1] and linkgit:gitmodules[5] for
> - details.
> + The URL for a submodule. This variable is copied from the .gitmodules
> + file to the git config via 'git submodule init'. The user can change
> + the configured URL before obtaining the submodule via 'git submodule
> + update'. After obtaining the submodule, this variable is kept in the
> + config as a boolean flag to indicate whether the submodule is of
> + interest to git commands.  See linkgit:git-submodule[1] and
> + linkgit:gitmodules[5] for details.

I think it is great that you are describing that this serves two
purposes, but "as a boolean flag" is very misleading.  It sounds as
if at some point "git submodule $something" command stores
true/false there.

 - It overrides the URL suggested by the project in .gitmodules and
   replace it with another URL viewed from the local environment
   (e.g. the project may suggest you to use http://github.com/ while
   you may have a local mirror elsewhere).

 - Its presence is also used as a sign that the user is interested in
   the submodule.

Re: How to watch a mailing list & repo for patches which affect a certain area of code?

[Jakub Narębski]

W dniu 09.10.2016 o 21:03, Ian Kelling pisze:

> I've got patches in various projects, and I don't have time to keep up
> with the mailing list, but I'd like to help out with maintenance of that
> code, or the functions/files it touches. People don't cc me. I figure I
> could filter the list, test patches submitted, commits made, mentions of
> files/functions, build filters based on the code I have in the repo even
> if it's been moved or changed subsequently. I'm wondering what other
> people have implemented already for automation around this, or general
> thoughts. Web search is not showing me much.

First, the practice on this mailing list is to Cc all, and from what
I have seen people tend to do that (well, at least the regular participants).

Second, you can read this mailing list (and send emails / respond) with
a news reader via the NNTP interface (gmane or public-inbox).  News readers
usually have good search capability.

Hope that helps
-- 
Jakub Narębski

Re: [PATCH] clean up confusing suggestion for commit references

[Junio C Hamano]

Heiko Voigt  writes:

>  If you want to reference a previous commit in the history of a stable
> -branch, use the format "abbreviated sha1 (subject, date)",
> +branch, use the format 'abbreviated sha1 ("subject", date)',
>  with the subject enclosed in a pair of double-quotes, like this:
>  
>  Commit f86a374 ("pack-bitmap.c: fix a memleak", 2015-03-30)

As long as the "like this" example is there close to the sentence
"use the format ...", I do not think it matters either way in
practice, but I think this change is adding unnecessary confusion.

Both the 'subject' and 'date' on that line are meant to be
placeholders, so where you see subject, you replace it "with the
subject enclosed in a pair of double-quotes" as the next line says.
But then you would end up with:

 Commit f86a374 (""pack-bitmap.c: fix a memleak"", 2015-03-30)

which is not what we want to see.

Re: git filter-branch --subdirectory-filter not working as expected, history of other folders is preserved

[Jeff King]

On Mon, Oct 10, 2016 at 05:12:25PM +0100, Seaders Oloinsigh wrote:

> Due to the structure of this repo, it looks like there are some
> branches that never had anything to do with the android/ subdirectory,
> so they're not getting wiped out.  My branch is in a better state to
> how I want it, but still, if I run your suggestion,
> [...]

Hmm. Yeah, I think this is an artifact of the way that filter-branch
works with pathspec limiting. It keeps a mapping of commits that it has
rewritten (including ones that were rewritten only because their
ancestors were), and realizes that a branch ref needs updated when the
commit it points to was rewritten.

But if we don't touch _any_ commits in the history reachable from a
branch (because they didn't even show up in our pathspec-limited
rev-list), then it doesn't realize we touched the branch's history at
all.

I agree that the right outcome is for it to delete those branches
entirely. I suspect the fix would be pretty tricky, though.

In the meantime, I think you can work around it by either:

  1. Make a pass beforehand for refs that do not touch your desired
 paths at all, like:

   path=android ;# or whatever
   git for-each-ref --format='%(refname)' |
   while read ref; do
 if test "$(git rev-list --count "$ref" -- "$path")" = 0; then
   echo "delete $ref"
 fi
   done |
   git update-ref --stdin

 and then filter what's left:

   git filter-branch --subdirectory-filter $path -- --all

or

  2. Do the filter-branch, and because you know you specified --all and
 that your filters would touch all histories, any ref which _wasn't_
 touched can be deleted. That list is anything which didn't get a
 backup entry in refs/original. So something like:

   git for-each-ref --format='%(refname)' |
   perl -lne 'print $1 if m{^refs/original/(.*)}' >backups

   git for-each-ref --format='%(refname)' |
   grep -v ^refs/original >refs

   comm -23 refs backups |
   sed "s/^/delete /" |
   git update-ref --stdin

-Peff

Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd

[Jeff King]

On Mon, Oct 10, 2016 at 10:52:26AM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > Having separate exec/shell boolean options just punts the overlap from
> > the command key to those keys. If you have two mutually exclusive
> > options, I think the best thing is a single option, like:
> >
> >   type = 
> >
> > and then it is obvious that a second appearance of "type" overrides an
> > earlier one, by our usual "last one wins" convention. As opposed to:
> >
> >   shell = true
> >   exec = true
> >
> > where you have to understand the meaning of each option to know that
> > "exec" overrides "shell".
> 
> Good.  
> 
> Duy's "do we want to chdir or stay?" would be an orthogonal axis to
> "what does the command line look like?" and "how is the command line
> run?" so it adds one member to the "alias..*" family of
> variables, I guess.

Yeah, exactly. There may be other orthogonal axes to add, too, but I do
not have any in mind myself. My main motive in switching to the
"alias.$cmd.key" syntax is that it fixes the ancient mistake of putting
arbitrary content into the key (just like pager.*, as we've discussed
elsewhere).

-Peff

Re: [PATCH] clean up confusing suggestion for commit references

[Junio C Hamano]

Jeff King  writes:

> On Fri, Oct 07, 2016 at 11:56:38AM +0200, Heiko Voigt wrote:
>
>> The description for referencing commits looks as if it is contradicting
>> the example, since it is itself enclosed in double quotes. Lets use
>> single quotes around the description and include the double quotes in
>> the description so it matches the example.
>> ---
>> Sorry for opening this up again but I just looked up the format and was
>> like: "Umm, which one is now the correct one..."
>> 
>> For this makes more sense. What do others think?
>
> Looking over the threads, I wasn't sure there was consensus[1,2]. So it would
> be equally correct to drop the quotes from the example.
>
> I dunno. I am in favor of no-quotes, myself, so maybe I am just
> manufacturing dissent in my mind. :)

I no longer have preference either way myself, even though I was in
favor of no-quotes simply because I had an alias to produce that
format and was used to it.

Re: [PATCH] clean up confusing suggestion for commit references

[Jeff King]

On Mon, Oct 10, 2016 at 11:24:01AM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > On Fri, Oct 07, 2016 at 11:56:38AM +0200, Heiko Voigt wrote:
> >
> >> The description for referencing commits looks as if it is contradicting
> >> the example, since it is itself enclosed in double quotes. Lets use
> >> single quotes around the description and include the double quotes in
> >> the description so it matches the example.
> >> ---
> >> Sorry for opening this up again but I just looked up the format and was
> >> like: "Umm, which one is now the correct one..."
> >> 
> >> For this makes more sense. What do others think?
> >
> > Looking over the threads, I wasn't sure there was consensus[1,2]. So it 
> > would
> > be equally correct to drop the quotes from the example.
> >
> > I dunno. I am in favor of no-quotes, myself, so maybe I am just
> > manufacturing dissent in my mind. :)
> 
> I no longer have preference either way myself, even though I was in
> favor of no-quotes simply because I had an alias to produce that
> format and was used to it.

I'll admit that I don't care _that_ much and am happy to leave it up to
individual authors, as long as nobody quotes SubmittingPatches at me as
some kind of gospel when I use the no-quotes form.

-Peff

Re: [PATCH v4 4/4] mergetool: honor -O

[Junio C Hamano]

David Aguilar  writes:

> Teach mergetool to pass "-O" down to `git diff` when
> specified on the command-line.
>
> Helped-by: Johannes Sixt 
> Signed-off-by: David Aguilar 
> ---
> Since v3:
>
> I missed one last piped invocation of "git mergetool" in the tests,
> which has been fixed.

I only see 4/4 in v4; am I correct to assume that 1-3/4 of v4 are
the same as their counterparts in v3?

Re: [PATCH] contrib: add credential helper for libsecret

[Jeff King]

On Sun, Oct 09, 2016 at 03:34:17PM +0300, Mantas Mikulėnas wrote:

> This is based on the existing gnome-keyring helper, but instead of
> libgnome-keyring (which was specific to GNOME and is deprecated), it
> uses libsecret which can support other implementations of XDG Secret
> Service API.

Thanks for working on this; I'm happy to see credential helpers for as
many storage APIs as possible.

> Passes t0303-credential-external.sh.

Thank you for running that, as my first question would have been about
its results. :)

I don't know much about the Secret Service API, nor do I run a desktop
environment which provides the server side of it. But the code looks
straightforward and reasonable, it passes t0303, and presumably it is
working for you. So this seems worth merging to me.

-Peff

Re: [PATCH] documentation: clarify submodule..[url, path] variables

[Stefan Beller]

On Mon, Oct 10, 2016 at 11:09 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>>  submodule..path::
>> + The path within this project for a submodule. This variable is
>> + kept in the .gitmodules file. See linkgit:git-submodule[1] and
>> + linkgit:gitmodules[5] for details.
>
> What does it exactly mean to be "kept"?
>
> Does it mean "never appears in .git/config, and when it appears it
> will not be used at all"?  If so we shouldn't even list it here.

I meant:
Git wont put it into .git/config on its own. If you really wanted to have
it there, you need to do it yourself.

I assumed you can overwrite the path via the config. It turns out,
the value from the config is ignored, so it doesn't even make sense
to put it into the config. Only the value from the .gitmodules file counts.

So with that knowledge, I'll just remove it here, but I'll cross check if it
is referenced else where enough.

>
> I doubt there is any reason for .path to exist in .git/config; where
> each submodule appears in the working tree is what is recorded in
> the tree object, and the "identity" (i.e. that which links a
> submodule in a tree to one of the repositories kept in
> .git/modules/*) by reverse look-up of submodule..path from
> in-tree .gitmodules, not from configuration, and it is not something
> a per-repository configuration should be able to change at the
> conceptual level.

As said above, currently we'd only rely on the .gitmodules file.

However I think this may conceptually the wrong thing to do.
The name is set in stone and ought to never be changed, unlike
the path.  But as we only obtain this information from within the
repository at all times and not make a copy of it, upstream may
go crazy and change the names within the .gitmodules file.

Then the downstream user is left with a mess and they have no
way of fixing it except by modifying the data in the repository. So it
may be a sensible thing to allow the user to create their own
path -> name mapping in the config file.

We as the Git developers may even think about whether we'd
want to have the path -> name lookup all the time from upstream,
or rather on initialization want to make a copy of the path->name
mapping. Then we can never change the name locally, but we'd
need to track upstream for rename detection of submodule.

>
>>  submodule..url::
>> - The path within this project and URL for a submodule. These
>> - variables are initially populated by 'git submodule init'. See
>> - linkgit:git-submodule[1] and linkgit:gitmodules[5] for
>> - details.
>> + The URL for a submodule. This variable is copied from the .gitmodules
>> + file to the git config via 'git submodule init'. The user can change
>> + the configured URL before obtaining the submodule via 'git submodule
>> + update'. After obtaining the submodule, this variable is kept in the
>> + config as a boolean flag to indicate whether the submodule is of
>> + interest to git commands.  See linkgit:git-submodule[1] and
>> + linkgit:gitmodules[5] for details.
>
> I think it is great that you are describing that this serves two
> purposes, but "as a boolean flag" is very misleading.  It sounds as
> if at some point "git submodule $something" command stores
> true/false there.
>
>  - It overrides the URL suggested by the project in .gitmodules and
>replace it with another URL viewed from the local environment
>(e.g. the project may suggest you to use http://github.com/ while
>you may have a local mirror elsewhere).
>
>  - Its presence is also used as a sign that the user is interested in
>the submodule.

I see, that makes it clearer, I'll reword this.

Re: Feature request: use relative path in worktree config files

[Junio C Hamano]

Duy Nguyen  writes:

>> I think there are some pros and some cons for relative path and absolute 
>> path.
>> Maybe append a "--relative" option with `git worktree add` ?
>>
>> I've converted all path to relative and all work with success.
>>
>> What do you think to append this --relative option.
>
> Patches are welcome.

Hmm, are they really welcome? 

Is an invocation of "git worktree add" really the right point in the
workflow to decide if these references to other repositories should
be relative or absolute?  When you are moving referrer, it is more
convenient if the reference uses absolute path to name the
referrent.  When you are moving both referrer and referrent, it is
more convenient to use relative.  

I somehow doubt that users know which future move they would be
making when doing "git worktree add".

To me, this almost looks like a need for a new subcommand to "git
worktree" that lets you move existing worktree to elsewhere, or turn
absolute reference to relative and vice versa.

Re: How to watch a mailing list & repo for patches which affect a certain area of code?

[Stefan Beller]

+cc Xiaolong Ye 

On Sun, Oct 9, 2016 at 2:26 PM, Jason Pyeron  wrote:
>> -Original Message-
>> From: Ian Kelling
>> Sent: Sunday, October 09, 2016 15:03
>>
>> I've got patches in various projects, and I don't have time to keep up
>> with the mailing list, but I'd like to help out with
>> maintenance of that
>> code, or the functions/files it touches. People don't cc me.
>> I figure I
>> could filter the list, test patches submitted, commits made,
>> mentions of
>> files/functions, build filters based on the code I have in
>> the repo even
>> if it's been moved or changed subsequently. I'm wondering what other
>> people have implemented already for automation around this, or general
>> thoughts. Web search is not showing me much.
>>
>
> One thought would be to apply every patch automatically (to the branches of 
> interest?). Then trigger on the [successful] changed
> code. This would simplify the logic to working on the source only and not 
> parsing the emails.
>
> -Jason
>

I think this is currently attempted by some kernel people.
However it is very hard to tell where to apply a patch, as it is not formalized.
See the series that was merged at 72ce3ff7b51c ('xy/format-patch-base'),
which adds a footer to the patch, that tells you where exactly a patch ought
to be applied.

The intention behind that series was to have some CI system hooked up
and report failures to the mailing list as well IIUC. Maybe that helps with
your use case, too?

Re: [PATCH] pretty: respect color settings for %C placeholders

[Junio C Hamano]

Jeff King  writes:

> So here's what a patch to do that would look like. I admit that "I can't
> think of a good use" does not mean there _isn't_ one, but perhaps by
> posting this, it might provoke other people to think on it, too. And if
> nobody can come up with, maybe it's a good idea.

I do not have a fundamental opposition to this approach myself,
modulo a minor nit.

> +  By default, colors are shown only when enabled for log output (by
> +  `color.diff`, `color.ui`, or `--color`, and respecting the `auto`
> +  settings of the former if we are going to a terminal). `%C(auto,...)`
> +  is accepted as a historical synonym for the default. Specifying
> +  `%C(always,...) will show the colors always, even when colors are not
> +  otherwise enabled (to enable this behavior for the whole format, use
> +  `--color=always`).

It is not just "for the whole format", but also affects other parts
of the output, no?  I am thinking about "git log -p --format=...".

> diff --git a/pretty.c b/pretty.c
> index 25efbca..73e58b5 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -965,22 +965,31 @@ static size_t parse_color(struct strbuf *sb, /* in 
> UTF-8 */
>  
>   if (!end)
>   return 0;
> - if (skip_prefix(begin, "auto,", &begin)) {
> +
> + if (!skip_prefix(begin, "always,", &begin)) {
>   if (!want_color(c->pretty_ctx->color))
>   return end - placeholder + 1;
>   }

As a way to say "when color is not enabled, ignore everything unless
it begins with 'always,'", this was a bit hard to read.  Perhaps an
in-code comment is in order?

> +
> + /* this is a historical noop */
> + skip_prefix(begin, "auto,", &begin);
> +
>   if (color_parse_mem(begin, end - begin, color) < 0)
>   die(_("unable to parse --pretty format"));
>   strbuf_addstr(sb, color);
>   return end - placeholder + 1;
>   }
> - if (skip_prefix(placeholder + 1, "red", &rest))
> + if (skip_prefix(placeholder + 1, "red", &rest) &&
> + want_color(c->pretty_ctx->color))
>   strbuf_addstr(sb, GIT_COLOR_RED);

Hmm.  If we are in "no I do not want color" mode and "always,red"
was given, we countermanded !want_color() check up above and come
here.  Then we check want_color() again and refuse to paint it red?

I must be reading the patch incorrectly, but I cannot quite tell
where I want astray...

RE: How to watch a mailing list & repo for patches which affect a certain area of code?

[Jason Pyeron]

> -Original Message-
> From: Stefan Beller
> Sent: Monday, October 10, 2016 14:43
> 
> +cc Xiaolong Ye 
> 
> On Sun, Oct 9, 2016 at 2:26 PM, Jason Pyeron  wrote:
> >> -Original Message-
> >> From: Ian Kelling
> >> Sent: Sunday, October 09, 2016 15:03
> >>
> >> I've got patches in various projects, and I don't have 
> time to keep up
> >> with the mailing list, but I'd like to help out with
> >> maintenance of that
> >> code, or the functions/files it touches. People don't cc me.
> >> I figure I
> >> could filter the list, test patches submitted, commits made,
> >> mentions of
> >> files/functions, build filters based on the code I have in
> >> the repo even
> >> if it's been moved or changed subsequently. I'm wondering 
> what other
> >> people have implemented already for automation around 
> this, or general
> >> thoughts. Web search is not showing me much.
> >>
> >
> > One thought would be to apply every patch automatically (to 
> the branches of interest?). Then trigger on the [successful] changed
> > code. This would simplify the logic to working on the 
> source only and not parsing the emails.
> >
> > -Jason
> >
> 
> I think this is currently attempted by some kernel people.
> However it is very hard to tell where to apply a patch, as it 

This is one of the reasons why I use bundles instead of format patch.

> is not formalized.
> See the series that was merged at 72ce3ff7b51c 
> ('xy/format-patch-base'),
> which adds a footer to the patch, that tells you where 
> exactly a patch ought
> to be applied.

Cant wait for that.

> 
> The intention behind that series was to have some CI system hooked up
> and report failures to the mailing list as well IIUC. Maybe 
> that helps with
> your use case, too?

I envisioned that it would try for each head he was interested in.

Re: [PATCH] pretty: respect color settings for %C placeholders

[Jeff King]

On Mon, Oct 10, 2016 at 11:52:14AM -0700, Junio C Hamano wrote:

> > +  By default, colors are shown only when enabled for log output (by
> > +  `color.diff`, `color.ui`, or `--color`, and respecting the `auto`
> > +  settings of the former if we are going to a terminal). `%C(auto,...)`
> > +  is accepted as a historical synonym for the default. Specifying
> > +  `%C(always,...) will show the colors always, even when colors are not
> > +  otherwise enabled (to enable this behavior for the whole format, use
> > +  `--color=always`).
> 
> It is not just "for the whole format", but also affects other parts
> of the output, no?  I am thinking about "git log -p --format=...".

True. I consider that a feature and more likely to be pointing the user
in the right direction, but it should probably be s/format/output/ in
the last sentence.

> > diff --git a/pretty.c b/pretty.c
> > index 25efbca..73e58b5 100644
> > --- a/pretty.c
> > +++ b/pretty.c
> > @@ -965,22 +965,31 @@ static size_t parse_color(struct strbuf *sb, /* in 
> > UTF-8 */
> >  
> > if (!end)
> > return 0;
> > -   if (skip_prefix(begin, "auto,", &begin)) {
> > +
> > +   if (!skip_prefix(begin, "always,", &begin)) {
> > if (!want_color(c->pretty_ctx->color))
> > return end - placeholder + 1;
> > }
> 
> As a way to say "when color is not enabled, ignore everything unless
> it begins with 'always,'", this was a bit hard to read.  Perhaps an
> in-code comment is in order?

I'll see what I can do. The most readable one is probably:

  if (skip_prefix(begin, "auto,", &begin)) {
if (!want_color(c->pretty_ctx->color))
return end - placeholder + 1;
  } else if (skip_prefix(begin, "always,", &begin)) {
/* nothing to do; we do not respect want_color at all */
  } else {
/* the default is now the same as "auto" */
if (!want_color(c->pretty_ctx->color))
return end - placeholder + 1;
  }

I avoided that because of the repetition, but it probably is not too
bad.

> > -   if (skip_prefix(placeholder + 1, "red", &rest))
> > +   if (skip_prefix(placeholder + 1, "red", &rest) &&
> > +   want_color(c->pretty_ctx->color))
> > strbuf_addstr(sb, GIT_COLOR_RED);
> 
> Hmm.  If we are in "no I do not want color" mode and "always,red"
> was given, we countermanded !want_color() check up above and come
> here.  Then we check want_color() again and refuse to paint it red?
> 
> I must be reading the patch incorrectly, but I cannot quite tell
> where I want astray...

No, we don't come here from %C() at all. This is for bare "%Cred", which
cannot have "always" (or "auto"), as there is no syntactic spot for it.
It is mostly historical (it _only_ supports red, green, and blue). We
could actually leave this as-is to show the colors unconditionally. I
changed it to keep the new behavior consistent, but I doubt anybody
cares much either way.

-Peff

Re: How to watch a mailing list & repo for patches which affect a certain area of code?

[Stefan Beller]

On Mon, Oct 10, 2016 at 11:56 AM, Jason Pyeron  wrote:
>> -Original Message-
>> From: Stefan Beller
>> Sent: Monday, October 10, 2016 14:43
>>
>> +cc Xiaolong Ye 
>>
>> On Sun, Oct 9, 2016 at 2:26 PM, Jason Pyeron  wrote:
>> >> -Original Message-
>> >> From: Ian Kelling
>> >> Sent: Sunday, October 09, 2016 15:03
>> >>
>> >> I've got patches in various projects, and I don't have
>> time to keep up
>> >> with the mailing list, but I'd like to help out with
>> >> maintenance of that
>> >> code, or the functions/files it touches. People don't cc me.
>> >> I figure I
>> >> could filter the list, test patches submitted, commits made,
>> >> mentions of
>> >> files/functions, build filters based on the code I have in
>> >> the repo even
>> >> if it's been moved or changed subsequently. I'm wondering
>> what other
>> >> people have implemented already for automation around
>> this, or general
>> >> thoughts. Web search is not showing me much.
>> >>
>> >
>> > One thought would be to apply every patch automatically (to
>> the branches of interest?). Then trigger on the [successful] changed
>> > code. This would simplify the logic to working on the
>> source only and not parsing the emails.
>> >
>> > -Jason
>> >
>>
>> I think this is currently attempted by some kernel people.
>> However it is very hard to tell where to apply a patch, as it
>
> This is one of the reasons why I use bundles instead of format patch.

Oh! That sounds interesting for solving the problem where to apply
a change, but the big disadvantage of bundles to patches is the inability
to just comment on it with an inline response.  So I assume you follow
a different workflow than git or the kernel do. Which project do you use it
for and do you have some documentation/blog that explains that workflow?


>
>> is not formalized.
>> See the series that was merged at 72ce3ff7b51c
>> ('xy/format-patch-base'),
>> which adds a footer to the patch, that tells you where
>> exactly a patch ought
>> to be applied.
>
> Cant wait for that.

Well it is found in 2.9 and later. Currently the base footer is
opt-in, e.g. you'd
need to convince people to run `git config format.useAutoBase true` or to
manually add the base to the patch via `format-patch --base=`.

>
>>
>> The intention behind that series was to have some CI system hooked up
>> and report failures to the mailing list as well IIUC. Maybe
>> that helps with
>> your use case, too?
>
> I envisioned that it would try for each head he was interested in.
>

Well the test system can be smart enough to differentiate between:
* the patch you sent did not even compile on your base, so why
   are you sending bogus patches?
* the patch you sent was fine as you sent it, but in the mean time
  the target head progressed, and it doesn't compile/test any more.
  collaboration is hard.
* or an extension to the prior point: this patch is fine but is broken
  by the series xyz that is also in flight, please coordinate with
  name@email.

Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd

[Junio C Hamano]

Jeff King  writes:

> ... My main motive in switching to the
> "alias.$cmd.key" syntax is that it fixes the ancient mistake of putting
> arbitrary content into the key (just like pager.*, as we've discussed
> elsewhere).

Yup, we are on the same page.  It's not too grave a mistake (we said
"these are command names and do not have to be able to contain
random letters" and deliberately did without the usual three-level
hierarchy; I think the reasoning is even more valid for pager.*),
but it does imply case insensitivity not to use three-level names,
and I think it is a good move.

Re: [PATCH] documentation: clarify submodule..[url, path] variables

[Junio C Hamano]

Stefan Beller  writes:

>> Does it mean "never appears in .git/config, and when it appears it
>> will not be used at all"?  If so we shouldn't even list it here.
>
> I meant:
> Git wont put it into .git/config on its own. If you really wanted to have
> it there, you need to do it yourself.
>
> I assumed you can overwrite the path via the config. It turns out,
> the value from the config is ignored, so it doesn't even make sense
> to put it into the config. Only the value from the .gitmodules file counts.
>
> So with that knowledge, I'll just remove it here, but I'll cross check if it
> is referenced else where enough.

Good.  I do recall we designed this part of the system in such a way
that .path is conceptually uncostimizable (it _is_ part of what the
containing tree already records), and was wondering why it is there
as if it can be overridden.

Re: [PATCH] clean up confusing suggestion for commit references

[Junio C Hamano]

Jeff King  writes:

> On Mon, Oct 10, 2016 at 11:24:01AM -0700, Junio C Hamano wrote:
>
>> I no longer have preference either way myself, even though I was in
>> favor of no-quotes simply because I had an alias to produce that
>> format and was used to it.
>
> I'll admit that I don't care _that_ much and am happy to leave it up to
> individual authors, as long as nobody quotes SubmittingPatches at me as
> some kind of gospel when I use the no-quotes form.

;-).  

I just do not want to hear "gitk (or was it git-gui) produces quoted
form, why are you recommending no-quoted form in SubmittingPatches?"

I'd say "use common sense; sometimes it is less confusing to read
without quotes and it is perfectly OK to do so if that is the case".

Re: git 2.10.1 test regression in t3700-add.sh

[Johannes Sixt]

Am 10.10.2016 um 19:41 schrieb Junio C Hamano:

I also notice that the problematic test uses "chmod 755"; don't we
need POSIXPERM prerequisite on this test, too, I wonder?


Good point. Without POSIXPERM the test demonstrate that, since chmod 755 
is basically a noop, the following add --chmod=-x does not leave an x 
bit in the index when there was none there in the first place. I think 
it does not hurt to keep the test even though it does not quite test the 
same thing as on POSIXPERM enabled systems.


-- Hannes

RE: How to watch a mailing list & repo for patches which affect a certain area of code? [OT]

[Jason Pyeron]

> -Original Message-
> From: Stefan Beller
> Sent: Monday, October 10, 2016 15:08
> 
> On Mon, Oct 10, 2016 at 11:56 AM, Jason Pyeron wrote:
> >> -Original Message-
> >> From: Stefan Beller
> >> Sent: Monday, October 10, 2016 14:43
> >>
> >> +cc Xiaolong Ye 
> >>
> >> On Sun, Oct 9, 2016 at 2:26 PM, Jason Pyeron wrote:
> >> >> -Original Message-
> >> >> From: Ian Kelling
> >> >> Sent: Sunday, October 09, 2016 15:03
> >> >>
> >> >> I've got patches in various projects, and I don't have
> >> time to keep up
> >> >> with the mailing list, but I'd like to help out with
> >> >> maintenance of that
> >> >> code, or the functions/files it touches. People don't cc me.
> >> >> I figure I
> >> >> could filter the list, test patches submitted, commits made,
> >> >> mentions of
> >> >> files/functions, build filters based on the code I have in
> >> >> the repo even
> >> >> if it's been moved or changed subsequently. I'm wondering
> >> what other
> >> >> people have implemented already for automation around
> >> this, or general
> >> >> thoughts. Web search is not showing me much.
> >> >>
> >> >
> >> > One thought would be to apply every patch automatically (to
> >> the branches of interest?). Then trigger on the 
> [successful] changed
> >> > code. This would simplify the logic to working on the
> >> source only and not parsing the emails.
> >> >
> >> > -Jason
> >> >
> >>
> >> I think this is currently attempted by some kernel people.
> >> However it is very hard to tell where to apply a patch, as it
> >
> > This is one of the reasons why I use bundles instead of 
> format patch.
> 
> Oh! That sounds interesting for solving the problem where to apply
> a change, but the big disadvantage of bundles to patches is 
> the inability
> to just comment on it with an inline response. 

Yep. It is a big one. I have a personal project to add a footer to a format 
patch with the missing "binary" data. The thoughts were for the main cases 
using a RLE bitmap for the whitespace in the above patch and the remainder of 
the commit blob data. This would allow minimal duplicate information in the 
email but pure text changes would be binary perfect so the commit id will still 
be correct.

Sigh, never have enough free time.

> So I assume you follow
> a different workflow than git or the kernel do. Which project 
> do you use it
> for and do you have some documentation/blog that explains 
> that workflow?

This is used when collaborating cross enterprise. In these situations 
enterprise A will not give access to enterprise B on their CI system, or git 
repo, etc...

We all have a mailing list in common (encrypted/signed emails when it contains 
sensitive info). The rules prevent us from using cloud solutions for almost all 
of our work.

I have also worked on git for cross domain (tin foil hat time) source code 
transfer.

As to a blog, never thought about it. Ask questions on the list and I will help.

> 
> 
> >
> >> is not formalized.
> >> See the series that was merged at 72ce3ff7b51c
> >> ('xy/format-patch-base'),
> >> which adds a footer to the patch, that tells you where
> >> exactly a patch ought
> >> to be applied.
> >
> > Cant wait for that.
> 
> Well it is found in 2.9 and later. Currently the base footer is
> opt-in, e.g. you'd
> need to convince people to run `git config format.useAutoBase 
> true` or to
> manually add the base to the patch via `format-patch --base=`.

Please make default in 2.10.2 .

> 
> >
> >>
> >> The intention behind that series was to have some CI 
> system hooked up
> >> and report failures to the mailing list as well IIUC. Maybe
> >> that helps with
> >> your use case, too?
> >
> > I envisioned that it would try for each head he was interested in.
> >
> 
> Well the test system can be smart enough to differentiate between:

For the OP's case 

Test 1: does it cleanly apply to any head, if no for all raise flag.

> * the patch you sent did not even compile on your base, so why
>are you sending bogus patches?

Test 2: is it in an area I care about, if not stop.

Test 3: does it compile for clean application, if no for all raise flag.

> * the patch you sent was fine as you sent it, but in the mean time
>   the target head progressed, and it doesn't compile/test any more.
>   collaboration is hard.

Yes, especially when you have no time, or management is in the way.

> * or an extension to the prior point: this patch is fine but is broken
>   by the series xyz that is also in flight, please coordinate with
>   name@email.

[PATCHv2] documentation: improve submodule..{url, path} description

[Stefan Beller]

Unlike the url variable a user cannot override the the path variable,
as it is part of the content together with the gitlink at the given
path. To avoid confusion do not mention the .path variable in the config
section and rely on the documentation provided in gitmodules[5].

Enhance the description of submodule..url and mention its two use
cases separately.

Signed-off-by: Stefan Beller 
---

I think the gitmodules[5] is enough for .path, so let's just
do this one instead.

Thanks,
Stefan


 Documentation/config.txt | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index e78293b..fd775b4 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2811,12 +2811,13 @@ stash.showStat::
option will show diffstat of the stash.  Defaults to true.
See description of 'show' command in linkgit:git-stash[1].
 
-submodule..path::
 submodule..url::
-   The path within this project and URL for a submodule. These
-   variables are initially populated by 'git submodule init'. See
-   linkgit:git-submodule[1] and linkgit:gitmodules[5] for
-   details.
+   The URL for a submodule. This variable is copied from the .gitmodules
+   file to the git config via 'git submodule init'. The user can change
+   the configured URL before obtaining the submodule via 'git submodule
+   update'. After obtaining the submodule, the presence of this variable
+   is used as a sign whether the submodule is of interest to git commands.
+   See linkgit:git-submodule[1] and linkgit:gitmodules[5] for details.
 
 submodule..update::
The default update procedure for a submodule. This variable
-- 
2.10.1.382.ga23ca1b.dirty

Re: [PATCH v10 13/14] convert: add filter..process option

[Junio C Hamano]

larsxschnei...@gmail.com writes:

> +# Count unique lines in two files and compare them.
> +test_cmp_count () {
> + for FILE in $@
> + do
> + sort $FILE | uniq -c | sed "s/^[ ]*//" >$FILE.tmp
> + cat $FILE.tmp >$FILE

Unquoted references to $FILE bothers me.  Are you relying on them
getting split at IFS boundaries?  Otherwise write this (and other
similar ones) like so:

for FILE in "$@"
do
do-this-to "$FILE" | ... >"$FILE.tmp" &&
cat "$FILE.tmp" >"$FILE" &&
rm -f "$FILE.tmp"

> + done &&
> + test_cmp $@

The use of "$@" here is quite pointless, as you _know_ all of them
are filenames, and you _know_ that test_cmp takes only two
filenames.  Be explicit and say

test_cmp "$1" "$2"

or even

test_cmp_count () {
expect=$1 actual=$2
for FILE in "$expect" "$actual"
do
...
done &&
test_cmp "$expect" "$actual"

> +# Count unique lines except clean invocations in two files and compare
> +# them. Clean invocations are not counted because their number can vary.
> +# c.f. 
> http://public-inbox.org/git/xmqqshv18i8i@gitster.mtv.corp.google.com/
> +test_cmp_count_except_clean () {
> + for FILE in $@
> + do
> + sort $FILE | uniq -c | sed "s/^[ ]*//" |
> + sed "s/^\([0-9]\) IN: clean/x IN: clean/" >$FILE.tmp
> + cat $FILE.tmp >$FILE
> + done &&
> + test_cmp $@
> +}

Why do you even _care_ about the number of invocations?  While I
told you why "clean" could be called multiple times under racy Git
as an example, that was not meant to be an exhaustive example.  I
wouldn't be surprised if we needed to run smudge twice, for example,
in some weirdly racy cases in the future.

Can we just have the correctness (i.e. "we expect that the working
tree file gets this as the result of checking it out, and we made
sure that is the case") test without getting into such an
implementation detail?

Thanks.

Re: [PATCH 1/2] submodule: ignore trailing slash on superproject URL

[Dennis Kaarsemaker]

[And now with CC to the list, sorry Stefan]

On Mon, 2016-10-10 at 10:56 -0700, Stefan Beller wrote:
> Before 63e95beb0 (2016-04-15, submodule: port resolve_relative_url from
> shell to C), it did not matter if the superprojects URL had a trailing
> slash or not. It was just chopped off as one of the first steps
> (The "remoteurl=${remoteurl%/}" near the beginning of
> resolve_relative_url(), which was removed in said commit).
> 
> When porting this to the C version, an off-by-one error was introduced
> and we did not check the actual last character to be a slash, but the
> NULL delimiter.
> 
> Reintroduce the behavior from before 63e95beb0, to ignore the trailing
> slash.

Looks good to me, and fixes my simple testcase and cloning epiphany
with trailing slash. Thanks!

D.

Re: [PATCH] pretty: respect color settings for %C placeholders

[René Scharfe]

Am 10.10.2016 um 19:42 schrieb Jeff King:

On Mon, Oct 10, 2016 at 07:09:12PM +0200, René Scharfe wrote:


diff --git a/pretty.c b/pretty.c
index 25efbca..73e58b5 100644
--- a/pretty.c
+++ b/pretty.c
@@ -965,22 +965,31 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 
*/

if (!end)
return 0;
-   if (skip_prefix(begin, "auto,", &begin)) {
+
+   if (!skip_prefix(begin, "always,", &begin)) {
if (!want_color(c->pretty_ctx->color))
return end - placeholder + 1;
}


Shouldn't we have an "else" here?


I'm not sure what you mean; can you write it out?


> -  if (skip_prefix(begin, "auto,", &begin)) {
> +
> +  if (!skip_prefix(begin, "always,", &begin)) {
>if (!want_color(c->pretty_ctx->color))
>return end - placeholder + 1;
>}

else {  /* here */

> +  /* this is a historical noop */
> +  skip_prefix(begin, "auto,", &begin);

}

Otherwise "always,auto," would be allowed and mean the same as 
"always,", which just seems wrong.  Not a biggie.



-   if (skip_prefix(placeholder + 1, "red", &rest))
+   if (skip_prefix(placeholder + 1, "red", &rest) &&
+   want_color(c->pretty_ctx->color))
strbuf_addstr(sb, GIT_COLOR_RED);
-   else if (skip_prefix(placeholder + 1, "green", &rest))
+   else if (skip_prefix(placeholder + 1, "green", &rest) &&
+want_color(c->pretty_ctx->color))
strbuf_addstr(sb, GIT_COLOR_GREEN);
-   else if (skip_prefix(placeholder + 1, "blue", &rest))
+   else if (skip_prefix(placeholder + 1, "blue", &rest) &&
+want_color(c->pretty_ctx->color))
strbuf_addstr(sb, GIT_COLOR_BLUE);
-   else if (skip_prefix(placeholder + 1, "reset", &rest))
+   else if (skip_prefix(placeholder + 1, "reset", &rest) &&
+want_color(c->pretty_ctx->color))
strbuf_addstr(sb, GIT_COLOR_RESET);
return rest - placeholder;
 }


Perhaps it's a funtion like add_color(sb, ctx, color) or similar would be
nice?


I actually wrote it that way first (I called it "maybe_add_color()"),
but it felt a little funny to have a separate function that people might
be tempted to reuse (the right solution is generally to check
want_color() early as above, but we can't do that here because we have
to find the end of each placeholder).


OK.  A variable then?  Lazy pseudo-code:

if (RED)
color = red;
else if (GREEN)
...

if (want_color())
strbuf_addstr(sb, color);


What I have here is a little funny, too, though, as it keeps trying
other color names if it finds "red" but want_color() returns 0.


Oh, missed that somehow.. O_o

Re: [PATCH] pretty: respect color settings for %C placeholders

[Jeff King]

On Mon, Oct 10, 2016 at 09:59:17PM +0200, René Scharfe wrote:

> > > Shouldn't we have an "else" here?
> > 
> > I'm not sure what you mean; can you write it out?
> 
> > -   if (skip_prefix(begin, "auto,", &begin)) {
> > +
> > +   if (!skip_prefix(begin, "always,", &begin)) {
> > if (!want_color(c->pretty_ctx->color))
> > return end - placeholder + 1;
> > }
> 
>   else {  /* here */
> 
> > +   /* this is a historical noop */
> > +   skip_prefix(begin, "auto,", &begin);
> 
>   }
> 
> Otherwise "always,auto," would be allowed and mean the same as "always,",
> which just seems wrong.  Not a biggie.

I don't think that will parse "%C(auto,foo)", as we hit the
!skip_prefix() of the conditional, and do not look for "auto," at all.

I think you'd have to move the check for "auto," inside the if block.

I'm leaning towards just writing it out the long way, though, as I did
in my reply to Junio.

> > > Perhaps it's a funtion like add_color(sb, ctx, color) or similar would be
> > > nice?
> > 
> > I actually wrote it that way first (I called it "maybe_add_color()"),
> > but it felt a little funny to have a separate function that people might
> > be tempted to reuse (the right solution is generally to check
> > want_color() early as above, but we can't do that here because we have
> > to find the end of each placeholder).
> 
> OK.  A variable then?  Lazy pseudo-code:
> 
>   if (RED)
>   color = red;
>   else if (GREEN)
>   ...
> 
>   if (want_color())
>   strbuf_addstr(sb, color);

Yeah, that is a bit more clear (the final conditional just needs to
check that we actually found a color).

-Peff

Re: [PATCH v1 2/2] convert.c: stream and early out

[Junio C Hamano]

tbo...@web.de writes:

> -static void gather_stats(const char *buf, unsigned long size, struct 
> text_stat *stats)
> +static void gather_stats_partly(const char *buf, unsigned long len,
> + struct text_stat *stats, unsigned earlyout)
>  {

I think it is OK not to rename the function (you'd be passing earlyout=0
for callers that want exact stat, right?).

>   unsigned long i;
>  
> - memset(stats, 0, sizeof(*stats));
> -
> - for (i = 0; i < size; i++) {
> + if (!buf || !len)
> + return;
> + for (i = 0; i < len; i++) {
>   unsigned char c = buf[i];
>   if (c == '\r') {
> - if (i+1 < size && buf[i+1] == '\n') {
> + stats->stat_bits |= CONVERT_STAT_BITS_ANY_CR;
> + if (i+1 < len && buf[i+1] == '\n') {
>   stats->crlf++;
>   i++;
> - } else
> + stats->stat_bits |= CONVERT_STAT_BITS_TXT_CRLF;
> + } else {
>   stats->lonecr++;
> + stats->stat_bits |= CONVERT_STAT_BITS_BIN;
> + }
>   continue;
>   }
>   if (c == '\n') {
>   stats->lonelf++;
> + stats->stat_bits |= CONVERT_STAT_BITS_TXT_LF;
>   continue;
>   }
>   if (c == 127)
> @@ -67,7 +74,7 @@ static void gather_stats(const char *buf, unsigned long 
> size, struct text_stat *
>   stats->printable++;
>   break;
>   case 0:
> - stats->nul++;
> + stats->stat_bits |= CONVERT_STAT_BITS_BIN;
>   /* fall through */
>   default:
>   stats->nonprintable++;


So depending on the distribution of the bytes in the file, the
bitfields in stats->stat_bits will be filled one bit at a time in
random order.

> @@ -75,10 +82,12 @@ static void gather_stats(const char *buf, unsigned long 
> size, struct text_stat *
>   }
>   else
>   stats->printable++;
> + if (stats->stat_bits & earlyout)
> + break; /* We found what we have been searching for */

But an "earlyout" says that if "any" of the earlyout bit is seen, we
can return.

It somehow felt a bit too limited to me in my initial reading, but I
guess I shouldn't be surprised to see that such a limited interface
is sufficient for a file-local helper function ;-).  

The only caller that the semantics of this exit condition matters is
the one that wants to know "do we have NUL or CR anywhere?", so I
guess this should be sufficient.

>   }
>  
>   /* If file ends with EOF then don't count this EOF as non-printable. */
> - if (size >= 1 && buf[size-1] == '\032')
> + if (len >= 1 && buf[len-1] == '\032')
>   stats->nonprintable--;

This noise is somewhat irritating.  Was there a reason why size was
a bad name for the variable?

> +static const char *convert_stats_ascii(unsigned convert_stats)
>  {
> - unsigned int convert_stats = gather_convert_stats(data, size);
> -
> + const unsigned mask = CONVERT_STAT_BITS_TXT_LF |
> + CONVERT_STAT_BITS_TXT_CRLF;
>   if (convert_stats & CONVERT_STAT_BITS_BIN)
>   return "-text";
> - switch (convert_stats) {
> + switch (convert_stats & mask) {
>   case CONVERT_STAT_BITS_TXT_LF:
>   return "lf";
>   case CONVERT_STAT_BITS_TXT_CRLF:

Subtle.  The caller runs the stat colllection with early-out set to
BITS_BIN, so that this can set "-text" early.  It knows that without
BITS_BIN, the stat was taken for the whole contents and the check lf
or crlf can be reliable.

I wonder if we can/need to do something to remove this subtleness
out of this callchain, which could be a source of confusion.

> @@ -132,24 +162,45 @@ static const char *gather_convert_stats_ascii(const 
> char *data, unsigned long si
>   }
>  }
>  
> +static unsigned get_convert_stats_wt(const char *path)
> +{
> + struct text_stat stats;
> + unsigned earlyout = CONVERT_STAT_BITS_BIN;
> + int fd;
> + memset(&stats, 0, sizeof(stats));
> + fd = open(path, O_RDONLY);
> + if (fd < 0)
> + return 0;
> + for (;;) {
> + char buf[2*1024];

Where is this 2kB come from?  Out of thin air?