Re: [RFC] test-lib: detect common misuse of test_expect_failure

[Jeff King]

On Fri, Oct 14, 2016 at 03:38:41PM -0700, Junio C Hamano wrote:

> It is a very easy mistake to make to say test_expect_failure when
> making sure a step in the test fails, which must be spelled
> "test_must_fail".  By introducing a toggle $test_in_progress that is
> turned on at the beginning of test_start_() and off at the end of
> test_finish_() helper, we can detect this fairly easily.
> 
> Strictly speaking, writing "test_expect_success" inside another
> test_expect_success (or inside test_expect_failure for that matter)
> can be detected with the same mechanism if we really wanted to, but
> that is a lot less likely confusion, so let's not bother.

I like the general idea, but I'm not sure how this would interact with
the tests in t that test the test suite. It looks like that always
happens in a full sub-shell invocation (via run_sub_test_lib_test), so
we're OK as long as test_in_progress is not exported (and obviously the
subshell cannot accidentally overwrite our variable with a 0 when it is
finished).

> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index fdaeb3a96b..fc8c10a061 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -381,6 +381,10 @@ test_verify_prereq () {
>  }
>  
>  test_expect_failure () {
> + if test "$test_in_progress" = 1
> + then
> + error "bug in the test script: did you mean test_must_fail 
> instead of test_expect_failure?"
> + fi

This follows existing practice for things like the &&-lint-checker, and
bails out on the whole test script. That sometimes makes it hard to find
the problematic test, especially if you're running via something like
"prove", because it doesn't make valid TAP output.

It might be nicer if we just said "this test is malformed, and therefore
fails", and then you get all the usual niceties for recording and
finding the failed test.

I don't think it would be robust enough to try to propagate the error up
to the outer test_expect_success block (and anyway, you'd also want to
know about it in a test_expect_failure block; it's a bug in the test,
not a known breakage). But perhaps error() could dump some TAP-like
output with a "virtual" failed test.

Something like:

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 11562bd..dc6b1f5 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -299,9 +299,8 @@ TERM=dumb
 export TERM
 
 error () {
-   say_color error "error: $*"
-   GIT_EXIT_OK=t
-   exit 1
+   test_failure_ "$@"
+   test_done
 }
 
 say () {
@@ -600,7 +599,7 @@ test_run_ () {
# code of other programs
test_eval_ "(exit 117) && $1"
if test "$?" != 117; then
-   error "bug in the test script: broken &&-chain: $1"
+   error "bug in the test script: broken &&-chain" "$1"
fi
trace=$trace_tmp
fi

which lets "make prove" collect the broken test number.

It would perhaps need to cover the case when $test_count is "0"
separately. I dunno. It would be nicer still if we could continue
running other tests in the script, but I think it's impossible to
robustly jump back to the outer script.

These kinds of "bug in the test suite" are presumably rare enough that
the niceties don't matter that much, but I trigger the &&-checker
reasonably frequently (that and test_line_count, because I can never
remember the correct invocation).

Anyway. That's all orthogonal to your patch. I just wondered if we could
do better, but AFAICT the right way to do better is to hook into
error(), which means your patch would not have to care exactly how it
fails.

-Peff

[RFC] test-lib: detect common misuse of test_expect_failure

[Junio C Hamano]

It is a very easy mistake to make to say test_expect_failure when
making sure a step in the test fails, which must be spelled
"test_must_fail".  By introducing a toggle $test_in_progress that is
turned on at the beginning of test_start_() and off at the end of
test_finish_() helper, we can detect this fairly easily.

Strictly speaking, writing "test_expect_success" inside another
test_expect_success (or inside test_expect_failure for that matter)
can be detected with the same mechanism if we really wanted to, but
that is a lot less likely confusion, so let's not bother.

Signed-off-by: Junio C Hamano 
---

 * It is somewhat embarrassing to admit that I had to stare at the
   offending code for more than 5 minutes to notice what went wrong
   to come up with ;
   if we had something like this, it would have helped.

 t/test-lib-functions.sh | 4 
 t/test-lib.sh   | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index fdaeb3a96b..fc8c10a061 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -381,6 +381,10 @@ test_verify_prereq () {
 }
 
 test_expect_failure () {
+   if test "$test_in_progress" = 1
+   then
+   error "bug in the test script: did you mean test_must_fail 
instead of test_expect_failure?"
+   fi
test_start_
test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
test "$#" = 2 ||
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 11562bde10..4c360216e5 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -344,6 +344,7 @@ test_count=0
 test_fixed=0
 test_broken=0
 test_success=0
+test_in_progress=0
 
 test_external_has_tap=0
 
@@ -625,6 +626,7 @@ test_run_ () {
 }
 
 test_start_ () {
+   test_in_progress=1
test_count=$(($test_count+1))
maybe_setup_verbose
maybe_setup_valgrind
@@ -634,6 +636,7 @@ test_finish_ () {
echo >&3 ""
maybe_teardown_valgrind
maybe_teardown_verbose
+   test_in_progress=0
 }
 
 test_skip () {

Re: [PATCH v15 14/27] t6030: no cleanup with bad merge base

[Junio C Hamano]

Pranit Bauva  writes:

> +test_expect_success 'check whether bisection cleanup is not done with bad 
> merges' '
> + git bisect start $HASH7 $SIDE_HASH7 &&
> + test_expect_failure git bisect bad >out 2>out &&

I think you meant "test_must_fail" here.

> + test_i18ngrep "The merge base" out &&
> + test -e .git/BISECT_START
> +'
> +
>  test_done
>
> --
> https://github.com/git/git/pull/287

Re: Automagic `git checkout branchname` mysteriously fails

[Martin Langhoff]

On Fri, Oct 14, 2016 at 4:58 PM, Kevin Daudt  wrote:
> Correct, this only works when it's unambiguous what branch you actually
> mean.

That's not surprising, but there isn't a warning. IMHO, finding
several branch matches is a strong indication that it'll be worth
reporting to the user that the DWIM machinery got hits, but couldn't
work it out.

I get that process is not geared towards making a friendly msg easy,
but we could print to stderr something like:

 "branch" matches more than one candidate ref, cannot choose automatically.
 If you mean to check out a branch, try git branch command.
 If you mean to check out a file, use -- before the pathname to
 disambiguate.

and then continue with execution. With a bit of wordsmithing, the msg
can be made to be helpful in the various failure modes.

cheers,


m
-- 
 martin.langh...@gmail.com
 - ask interesting questions  ~  http://linkedin.com/in/martinlanghoff
 - don't be distracted~  http://github.com/martin-langhoff
   by shiny stuff

Re: Automagic `git checkout branchname` mysteriously fails

[Kevin Daudt]

On Fri, Oct 14, 2016 at 04:25:49PM -0400, Martin Langhoff wrote:
> In a (private) repo project I have, I recently tried (and failed) to do:
> 
>   git checkout v4.1-support
> 
> getting a "pathspec did not match any files known to git" error.
> 
> There's an origin/v4.1-support, there is no v4.1-support "local"
> branch. Creating the tracking branch explicitly worked.
> 
> Other similar branches in existence upstream did work. Autocomplete
> matched git's own behaviour for this; where git checkout foo woudn't
> work, autocomplete would not offer a completion.
> 
> Why is this?
> 
> One theory I have not explored is that I have other remotes, and some
> have a v4.1-support branch. If that's the case, the error message is
> not very helpful, and could be improved.
> 
> git --version
> 2.7.4
> 
> DWIM in git is remarkably good, even addictive... when it works :-)
> 
> cheers,
> 

Correct, this only works when it's unambiguous what branch you actually
mean.

if remote_a/branch and remote_b/branch exists, git cannot guess which
one you actually mean.

The message you get is because git checkout can be followed by several
things. Either a branch/commit or a file. Git complaining it cannot find
a file with that name is because it has exhausted all other options.

I do agree that message could be a bit more clear.

Automagic `git checkout branchname` mysteriously fails

[Martin Langhoff]

In a (private) repo project I have, I recently tried (and failed) to do:

  git checkout v4.1-support

getting a "pathspec did not match any files known to git" error.

There's an origin/v4.1-support, there is no v4.1-support "local"
branch. Creating the tracking branch explicitly worked.

Other similar branches in existence upstream did work. Autocomplete
matched git's own behaviour for this; where git checkout foo woudn't
work, autocomplete would not offer a completion.

Why is this?

One theory I have not explored is that I have other remotes, and some
have a v4.1-support branch. If that's the case, the error message is
not very helpful, and could be improved.

git --version
2.7.4

DWIM in git is remarkably good, even addictive... when it works :-)

cheers,



m
-- 
 martin.langh...@gmail.com
 - ask interesting questions  ~  http://linkedin.com/in/martinlanghoff
 - don't be distracted~  http://github.com/martin-langhoff
   by shiny stuff

Re: [PATCH v2 3/3] gitweb: Link to "git describe"'d commits in log messages

[Jakub Narębski]

W dniu 06.10.2016 o 11:11, Ævar Arnfjörð Bjarmason pisze:
> Change the log formatting function to know about "git describe" output
> such as "v2.8.0-4-g867ad08", in addition to just plain "867ad08".
> 
> There are still many valid refnames that we don't link to
> e.g. v2.10.0-rc1~2^2~1 is also a valid way to refer to
> v2.8.0-4-g867ad08, but I'm not supporting that with this commit,
> similarly it's trivially possible to create some refnames like
> "æ/var-gf6727b0" or which won't be picked up by this regex.

It's important to cover most common cases occurring naturally in
people's repositories, while trying to avoid false positives (the latter
is important more now, where gitweb doesn't check for rev name validity).

I guess that most common is to use non-hierarchical tags with ASCII-only
names for describe output, so while "æ/var-gf6727b0" won't be picked,
it is IMVHO also much less likely to occur.

> 
> There's surely room for improvement here, but I just wanted to address
> the very common case of sticking "git describe" output into commit
> messages without trying to link to all possible refnames, that's going
> to be a rather futile exercise given that this is free text, and it
> would be prohibitively expensive to look up whether the references in
> question exist in our repository.
> 
> There was on-list discussion about how we could do better than this
> patch. Junio suggested to update parse_commits() to call a new
> "gitweb--helper" command which would pass each of the revision
> candidates through "rev-parse --verify --quiet". That would cut down
> on our false positives (e.g. we'll link to "deadbeef"), and also allow
> us to be more aggressive in selecting candidate revisions.
> 
> That may be too expensive to work in practice, or it may
> not. Investigating that would be a good follow-up to this patch.

All right.  That's good and enough for one patch.

> 
> Signed-off-by: Ævar Arnfjörð Bjarmason 

Acked-by: Jakub Narębski 

BTW. to add to whole "git describe" output or not discussion: having link
span whole revision marker makes for easier to use UI: larger are to hit.

> ---
>  gitweb/gitweb.perl | 18 --
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 92b5e91..7cf68f0 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2036,10 +2036,24 @@ sub format_log_line_html {
>   my $line = shift;
>  
>   $line = esc_html($line, -nbsp=>1);
> - $line =~ s{\b([0-9a-fA-F]{7,40})\b}{
> + $line =~ s{
> +\b
> +(
> +# The output of "git describe", e.g. v2.10.0-297-gf6727b0
> +# or hadoop-20160921-113441-20-g094fb7d
> +(? +[A-Za-z0-9.-]+
> +(?!\.) # refs can't end with ".", see check_refname_format()
> +-g[0-9a-fA-F]{7,40}
> +|
> +# Just a normal looking Git SHA1
> +[0-9a-fA-F]{7,40}
> +)
> +\b

Nice using of eXplained regexp.  It is much easier to understand with
comments.

> +}{
>   $cgi->a({-href => href(action=>"object", hash=>$1),
>   -class => "text"}, $1);
> - }eg;
> + }egx;
>  
>   return $line;
>  }
> 

Re: Change Default merge strategy options

[Jeff King]

On Thu, Oct 13, 2016 at 03:57:55PM +, Daniel Lopez wrote:

> How to use 'git config --global' to set default strategy like
> recursive.

I don't think there is a way to do so, though note that the default
strategy is already "recursive". You can set individual file merge
drivers with gitattributes, but I don't know of a way to set the overall
strategy.

> Example:
> Currently , when we want to enforce a specific strategic we need to
> include  its reference on the command line : 
> git.exe merge --strategy=recursive --strategy-option=ignore-all-space 
> dev-local

Some strategy options have their own config already (like
merge.renormalize). I guess in theory we could grow config options for
the other ones, though I think config for "strategy and its options"
might be more elegant.

-Peff

Re: Can we make interactive add easier to use?

[Jeff King]

On Fri, Oct 14, 2016 at 08:20:40AM -0500, Robert Dailey wrote:

> Normally when I use interactive add, I just want to add files to the
> index via simple numbers, instead of typing paths. So I'll do this as
> quick as I can:

I'd generally second Matthieu's suggestion to use a combination of "git
add" and "git add -p". But if you really like the interactive updater,
then the optimizations I can think of are:

> 1. Type `git add -i`
> 2. Press `u` after prompt appears
> 3. Press numbers for the files I want to add, ENTER key
> 4. ENTER key again to go back to main add -i menu
> 5. Press `q` to exit interactive add
> 6. Type `git commit`

We have "git add -p" to avoid having to do a similar workflow just to
get to the "p"atch menu. So in theory we could have a similar shortcut
to get to "u"pdate. I think it's just not in common enough use that
anybody really bothered to implement it.

We also have "commit -p" (and "commit -i") already, though I do not use
them myself.

That would probably take you down to:

  1. git commit --iu ;# obviously a terrible option name
  2. Press numbers, then ENTER
  3. 'q' or ENTER or ^D to exit, and jump into commit message
 automatically.

You can also set the interactive.singleKey config option to turn (2)
into just "press numbers" (which works right now).

> This feels very tedious. Is there a simplified workflow for this? I
> remember using a "git index" unofficial extension to git that let you
> do a `git status` that showed numbers next to each item (including
> untracked files!) and you could do `git add 1, 2, 3-5`, etc.

TBH, that extension sounds a lot more generically useful, as it works at
the shell level. I _think_ I've seen similar features implemented that
are not even git specific (i.e., they work off of existing
shell-completion libraries and just let you pick the options by number
rather than tab-completing). Sorry I don't have any links, though. It's
not something I ever used myself.

-Peff

Re: Huge performance bottleneck reading packs

[Jeff King]

On Fri, Oct 14, 2016 at 08:55:29AM +0200, Vegard Nossum wrote:

> On 10/13/2016 10:43 PM, Jeff King wrote:
> > No problem. I do think you'll benefit a lot from packing everything into
> > a single pack, but it's also clear that Git was doing more work than it
> > needed to be. This was a known issue when we added the racy-check to
> > has_sha1_file(), and knew that we might have to field reports like this
> > one.
> 
> After 'git gc' (with the .5G limit) I have 23 packs and with your patch
> I now get:
> 
> $ time git fetch
> 
> real0m26.520s
> user0m3.580s
> sys 0m0.636s

Cool. That's about what I'd expect based on the size numbers you gave
earlier. The other 23 seconds are likely being spent on passing the ref
advertisement over the network.

-Peff

Re: [PATCH v2 3/3] gitweb: Link to "git describe"'d commits in log messages

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> I just ran into an example of a better reason for doing it like my
> patch is doing, which is that if you have some tag like:
>
> deployment-20160928-171914-16-g42e13d8
>
> With my patch the whole thing will be a link to the 42e13d8 commit,
> but with this suggestion both 20160928 and 42e13d8 would become commit
> links, the former one would be broken.
>
> Of course we could have some code that would detect that the whole \S+
> is part of one thing that ends in g,...

I think that this example shows a flaw not in the "suffix that looks
like an object name" approach, but more in the boundary regexp,
namely, the \b part; it is probably too loose.

And \S+ is not the right cue, either, for that matter.  IOW, "we
only should take hexstring, optionally prefixed with 'g', that
appears before the whitespace" is too strict, as a sentence

We broke the system with deployment-g42e13d8.

does want to link to 42e13d8, even though full-stop at the end is
not whitespace, and the existing regexp uses \b there as a rough
equivalent to saying "Here must be a whitespace or punctuation".

An attempt to tighten "what a punctuation is" by excluding '-' may
make that "timestamp is in the tagname" example work, but is not a
good solution, either, because two sentences can be concatenated
with an em-dash that is often typed as two hyphen in plain text,
resulting in something like

We broke the system with deployment-g42e13d8--sigh.

and we do want to treat the '-' after 42e13d8 as a punctuation after
a described object name.

So I agree 3/3 is good thing to do as-is.

Thanks.

Re: [PATCH] fetch: use "quick" has_sha1_file for tag following

[Jeff King]

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

> Jeff King  writes:
> 
> > So it's certainly better. But 7500 packs is just silly, and squeezing
> > out ~400ms there is hardly worth it. If you repack this same case into a
> > single pack, the command drops to 5ms. So yes, there's close to an order
> > of magnitude speedup here, but you get that _and_ another order of
> > magnitude just by repacking.
> 
> "7500 is silly" equally applies to the "quick" (and sloppy, if I am
> reading your "Failing in this direction doesn't make me feel great."
> correctly) approach, I think, which argues for not taking either
> change X-<.

I wouldn't quite agree that they're the same. 7500 packs is silly
because a bunch of _other_ things are going to get equally or more slow
before the prepare_packed_git() slowdown is noticeable. And it's in your
power to clean up, and we should encourage users to do so.

Whereas having a bunch of unfetched tags is a state that may linger
indefinitely, and be outside the user's control.

I _am_ open to the argument that calling reprepare_packed_git() over and
over doesn't really matter much if you have a sane number of packs. If
you tweak my perf test like so:

diff --git a/t/perf/p5550-fetch-tags.sh b/t/perf/p5550-fetch-tags.sh
index a5dc39f..7e7ae24 100755
--- a/t/perf/p5550-fetch-tags.sh
+++ b/t/perf/p5550-fetch-tags.sh
@@ -86,7 +86,7 @@ test_expect_success 'create child packs' '
cd child &&
git config gc.auto 0 &&
git config gc.autopacklimit 0 &&
-   create_packs 500
+   create_packs 10
)
 '
 

you get:

Testoriginquick 

5550.4: fetch   0.06(0.02+0.02)   0.02(0.01+0.00) -66.7%

Still an impressive speedup as a percentage, but negligible in absolute
terms. But that's on a local filesystem on a Linux machine. I'd worry
much more about a system with a slow readdir(), e.g., due to NFS.
Somebody's real-world NFS case[1] was what prompted us to do 0eeb077
(index-pack: avoid excessive re-reading of pack directory, 2015-06-09).

It looks like I _did_ look into optimizing this into a single stat()
call in the thread at [1]. I completely forgot about that. I did find
there that naively using stat_validity() on a directory is racy, though
I wonder if we could do something clever with gettimeofday() instead.
IOW, the patches there only bothered to re-read when they saw the mtime
on the directory jump, which suffers from 1-second precision problems.
But if we instead compared the mtime to the current time, we could err
in favor of re-reading the packs, and get false positives for at most 1
second.

[1] 
http://public-inbox.org/git/7fae15f0a93c0144ad8b5fbd584e1c5519758...@c111kxtembx51.erf.thomson.com/

> I agree that the fallout from the inaccuracy of "quick" approach is
> probably acceptable and the next "fetch" will correct it anyway, so
> let's do the "quick but inaccurate" for now and perhaps cook it in
> 'next' for a bit longer than other topics?

I doubt that cooking in 'next' for longer will turn up anything useful.
The case we care about is the race between a repack and a fetch. We
lived with the "quick" version of has_sha1_file() everywhere for 8
years. I only noticed the race on a hosting cluster which puts through
millions of operations per day (I could in theory test the patch on that
same cluster, but we actually don't do very many fetches).

-Peff

Re: [PATCH v2 2/3] gitweb: Link to 7-char+ SHA1s, not only 8-char+

[Junio C Hamano]

Jakub Narębski  writes:

> s/SHA1/SHA-1/g in above paragraph (for correctness and consistency).
>> 
>> I think it's fairly dubious to link to things matching [0-9a-fA-F]
>> here as opposed to just [0-9a-f], that dates back to the initial
>> version of gitweb from 161332a ("first working version",
>> 2005-08-07). Git will accept all-caps SHA1s, but didn't ever produce
>> them as far as I can tell.
>
> All right.  If we decide to be more strict in what we accept, we can
> do it in a separate commit.
>
>> 
>> Signed-off-by: Ævar Arnfjörð Bjarmason 
>
> Acked-by: Jakub Narębski 

Thanks for a review.  As the topic is not yet in 'next', I'll squish
in your Acked-by: to them.  I saw them only for 1 & 2/3; would
another for 3/3 be coming soon?

>
>> ---
>>  gitweb/gitweb.perl | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index cba7405..92b5e91 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -2036,7 +2036,7 @@ sub format_log_line_html {
>>  my $line = shift;
>>  
>>  $line = esc_html($line, -nbsp=>1);
>> -$line =~ s{\b([0-9a-fA-F]{8,40})\b}{
>> +$line =~ s{\b([0-9a-fA-F]{7,40})\b}{
>
> By the way, it is quite long commit message for one character change.
> Not that it is a bad thing...
>
>>  $cgi->a({-href => href(action=>"object", hash=>$1),
>>  -class => "text"}, $1);
>>  }eg;
>> 

Re: [PATCH v2 2/6] trailer: use list.h for doubly-linked list

[Junio C Hamano]

Jonathan Tan  writes:

> Replace the existing handwritten implementation of a doubly-linked list
> in trailer.c with the functions and macros from list.h. This
> significantly simplifies the code.
> ---

The handcrafted one in trailer.c somehow did not use the common
practice of using a doubly-linked cycle as a doubly-linked list with
a designated fake element as the pointers to the first and to the
last elements of the list (instead it used NULL as the "this is the
end in this direction" convention just like a common singly-linked
list), and this update removes the need for special cases handling
the elements at the beginning and at the end that comes from that
choice by switching to list.h macros.  update_last/update_first can
go, two parameters that were passed to point at the variables for
the beginning and the end can go, the special cases for the initial
condition in add_trailer_item() can go, all thanks to this change.

Very nice.

>  trailer.c | 258 
> ++
>  1 file changed, 91 insertions(+), 167 deletions(-)

Re: [PATCH v2 3/3] gitweb: Link to "git describe"'d commits in log messages

[Ævar Arnfjörð Bjarmason]

On Sun, Oct 9, 2016 at 1:20 PM, Ævar Arnfjörð Bjarmason
 wrote:
> On Thu, Oct 6, 2016 at 9:51 PM, Junio C Hamano  wrote:
>> Ævar Arnfjörð Bjarmason   writes:
>>
>>> Change the log formatting function to know about "git describe" output
>>> such as "v2.8.0-4-g867ad08", in addition to just plain "867ad08".
>>>
>>> There are still many valid refnames that we don't link to
>>> e.g. v2.10.0-rc1~2^2~1 is also a valid way to refer to
>>> v2.8.0-4-g867ad08, but I'm not supporting that with this commit,
>>> similarly it's trivially possible to create some refnames like
>>> "æ/var-gf6727b0" or which won't be picked up by this regex.
>>
>> Not a serious counter-proposal or suggestion, and certainly not an
>> objection to the patch I am responding to, but I wonder if it is so
>> bad if we made the 867ad08 into link when showing v2.8.0-4-g867ad08.
>>
>> IOW, in addition to \b followed by [0-9a-f]+ followed by \b, if we
>> allowed an optional 'g' in front of the hex, e.g.
>>
>> -   $line =~ s{\b([0-9a-fA-F]{7,40})\b}{
>> +   $line =~ s{\bg?([0-9a-fA-F]{7,40})\b}{
>>
>> wouldn't that be much simpler, covers more cases and sufficient?
>
> It would be more simpler, maybe that's the right approach. I have a
> preference for making the entire reference a link. I think it makes
> more sense for the UI.

I just ran into an example of a better reason for doing it like my
patch is doing, which is that if you have some tag like:

deployment-20160928-171914-16-g42e13d8

 With my patch the whole thing will be a link to the 42e13d8 commit,
but with this suggestion both 20160928 and 42e13d8 would become commit
links, the former one would be broken.

Of course we could have some code that would detect that the whole \S+
is part of one thing that ends in g, but the complexity in
doing that would be equivalent or more to doing what my patch is
doing.

>>> There's surely room for improvement here, but I just wanted to address
>>> the very common case of sticking "git describe" output into commit
>>> messages without trying to link to all possible refnames, that's going
>>> to be a rather futile exercise given that this is free text, and it
>>> would be prohibitively expensive to look up whether the references in
>>> question exist in our repository.
>>>
>>> There was on-list discussion about how we could do better than this
>>> patch. Junio suggested to update parse_commits() to call a new
>>> "gitweb--helper" command which would pass each of the revision
>>> candidates through "rev-parse --verify --quiet". That would cut down
>>> on our false positives (e.g. we'll link to "deadbeef"), and also allow
>>> us to be more aggressive in selecting candidate revisions.
>>>
>>> That may be too expensive to work in practice, or it may
>>> not. Investigating that would be a good follow-up to this patch.
>>>
>>> Signed-off-by: Ævar Arnfjörð Bjarmason 
>>> ---
>>>  gitweb/gitweb.perl | 18 --
>>>  1 file changed, 16 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>>> index 92b5e91..7cf68f0 100755
>>> --- a/gitweb/gitweb.perl
>>> +++ b/gitweb/gitweb.perl
>>> @@ -2036,10 +2036,24 @@ sub format_log_line_html {
>>>   my $line = shift;
>>>
>>>   $line = esc_html($line, -nbsp=>1);
>>> - $line =~ s{\b([0-9a-fA-F]{7,40})\b}{
>>> + $line =~ s{
>>> +\b
>>> +(
>>> +# The output of "git describe", e.g. v2.10.0-297-gf6727b0
>>> +# or hadoop-20160921-113441-20-g094fb7d
>>> +(?>> +[A-Za-z0-9.-]+
>>> +(?!\.) # refs can't end with ".", see check_refname_format()
>>> +-g[0-9a-fA-F]{7,40}
>>> +|
>>> +# Just a normal looking Git SHA1
>>> +[0-9a-fA-F]{7,40}
>>> +)
>>> +\b
>>> +}{
>>>   $cgi->a({-href => href(action=>"object", hash=>$1),
>>>   -class => "text"}, $1);
>>> - }eg;
>>> + }egx;
>>>
>>>   return $line;
>>>  }

Re: [PATCH 1/3] i18n: apply: mark plural string for translation

[Junio C Hamano]

Vasco Almeida  writes:

> Mark plural string for translation using Q_().
>
> Signed-off-by: Vasco Almeida 
> ---

Thanks for waiting (patiently) for 'master' to become ready to take
these three patches.

Re: [PATCH v2 2/6] trailer: use list.h for doubly-linked list

[Jakub Narębski]

W dniu 13.10.2016 o 01:40, Jonathan Tan pisze:
> Replace the existing handwritten implementation of a doubly-linked list
> in trailer.c with the functions and macros from list.h. This
> significantly simplifies the code.
> ---
>  trailer.c | 258 
> ++
>  1 file changed, 91 insertions(+), 167 deletions(-)

Very nice gains!

BTW. could you sign (Signed-off-by) your patches? TIA.

Best,
-- 
Jakub Narębski

Re: [PATCH v2 2/3] gitweb: Link to 7-char+ SHA1s, not only 8-char+

[Jakub Narębski]

W dniu 06.10.2016 o 11:11, Ævar Arnfjörð Bjarmason pisze:

> Change the minimum length of an abbreviated object identifier in the
> commit message gitweb tries to turn into link from 8 hexchars to 7.
> 
> This arbitrary minimum length of 8 was introduced in bfe2191 ("gitweb:
> SHA-1 in commit log message links to "object" view", 2006-12-10), but
> the default abbreviation length is 7, and has been for a long time.
> 
> It's still possible to reference SHA1s down to 4 characters in length,
> see v1.7.4-1-gdce9648's MINIMUM_ABBREV, but I can't see how to make
> git actually produce that, so I doubt anyone is putting that into log
> messages in practice, but people definitely do put 7 character SHA1s
> into log messages.

That's nice explanation why we want to support 7 character SHA-1
(it is the default, and was seen in the wild), but don't need to
support shorter lengths.

Another reason (just for completeness; you don't need to put it in
the commit message) to not go below 7 characters is that with 
decreasing length there is more and more chance for false positives
(like 'beef' or 'caffee' for 4-char or 5-char hexstring), as I wrote
previously.

s/SHA1/SHA-1/g in above paragraph (for correctness and consistency).

> 
> I think it's fairly dubious to link to things matching [0-9a-fA-F]
> here as opposed to just [0-9a-f], that dates back to the initial
> version of gitweb from 161332a ("first working version",
> 2005-08-07). Git will accept all-caps SHA1s, but didn't ever produce
> them as far as I can tell.

All right.  If we decide to be more strict in what we accept, we can
do it in a separate commit.

> 
> Signed-off-by: Ævar Arnfjörð Bjarmason 

Acked-by: Jakub Narębski 

> ---
>  gitweb/gitweb.perl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index cba7405..92b5e91 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2036,7 +2036,7 @@ sub format_log_line_html {
>   my $line = shift;
>  
>   $line = esc_html($line, -nbsp=>1);
> - $line =~ s{\b([0-9a-fA-F]{8,40})\b}{
> + $line =~ s{\b([0-9a-fA-F]{7,40})\b}{

By the way, it is quite long commit message for one character change.
Not that it is a bad thing...

>   $cgi->a({-href => href(action=>"object", hash=>$1),
>   -class => "text"}, $1);
>   }eg;
> 

Re: [PATCH] fetch: use "quick" has_sha1_file for tag following

[Junio C Hamano]

Jeff King  writes:

> So it's certainly better. But 7500 packs is just silly, and squeezing
> out ~400ms there is hardly worth it. If you repack this same case into a
> single pack, the command drops to 5ms. So yes, there's close to an order
> of magnitude speedup here, but you get that _and_ another order of
> magnitude just by repacking.

"7500 is silly" equally applies to the "quick" (and sloppy, if I am
reading your "Failing in this direction doesn't make me feel great."
correctly) approach, I think, which argues for not taking either
change X-<.

I agree that the fallout from the inaccuracy of "quick" approach is
probably acceptable and the next "fetch" will correct it anyway, so
let's do the "quick but inaccurate" for now and perhaps cook it in
'next' for a bit longer than other topics?

[PATCH v3 6/6] trailer: support values folded to multiple lines

[Jonathan Tan]

Currently, interpret-trailers requires that a trailer be only on 1 line.
For example:

  a: first line
 second line

would be interpreted as one trailer line followed by one non-trailer line.

Make interpret-trailers support RFC 822-style folding, treating those
lines as one logical trailer.

Signed-off-by: Jonathan Tan 
---
 Documentation/git-interpret-trailers.txt |   7 +-
 t/t7513-interpret-trailers.sh| 139 +++
 trailer.c|  22 +++--
 3 files changed, 160 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt 
b/Documentation/git-interpret-trailers.txt
index c480da6..cfec636 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -57,11 +57,12 @@ minus signs start the patch part of the message.
 
 When reading trailers, there can be whitespaces before and after the
 token, the separator and the value. There can also be whitespaces
-inside the token and the value.
+inside the token and the value. The value may be split over multiple lines with
+each subsequent line starting with whitespace, like the "folding" in RFC 822.
 
 Note that 'trailers' do not follow and are not intended to follow many
-rules for RFC 822 headers. For example they do not follow the line
-folding rules, the encoding rules and probably many other rules.
+rules for RFC 822 headers. For example they do not follow
+the encoding rules and probably many other rules.
 
 OPTIONS
 ---
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 7f5cd2a..31db699 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -157,6 +157,145 @@ test_expect_success 'with non-trailer lines only' '
test_cmp expected actual
 '
 
+test_expect_success 'multiline field treated as atomic for placement' '
+   q_to_tab >patch <<-\EOF &&
+
+   another: trailer
+   name: value on
+   Qmultiple lines
+   another: trailer
+   EOF
+   q_to_tab >expected <<-\EOF &&
+
+   another: trailer
+   name: value on
+   Qmultiple lines
+   name: value
+   another: trailer
+   EOF
+   test_config trailer.name.where after &&
+   git interpret-trailers --trailer "name: value" patch >actual &&
+   test_cmp expected actual
+'
+
+test_expect_success 'multiline field treated as atomic for replacement' '
+   q_to_tab >patch <<-\EOF &&
+
+   another: trailer
+   name: value on
+   Qmultiple lines
+   another: trailer
+   EOF
+   q_to_tab >expected <<-\EOF &&
+
+   another: trailer
+   another: trailer
+   name: value
+   EOF
+   test_config trailer.name.ifexists replace &&
+   git interpret-trailers --trailer "name: value" patch >actual &&
+   test_cmp expected actual
+'
+
+test_expect_success 'multiline field treated as atomic for difference check' '
+   q_to_tab >patch <<-\EOF &&
+
+   another: trailer
+   name: first line
+   Qsecond line
+   another: trailer
+   EOF
+   test_config trailer.name.ifexists addIfDifferent &&
+
+   q_to_tab >trailer <<-\EOF &&
+   name: first line
+   Qsecond line
+   EOF
+   q_to_tab >expected <<-\EOF &&
+
+   another: trailer
+   name: first line
+   Qsecond line
+   another: trailer
+   EOF
+   git interpret-trailers --trailer "$(cat trailer)" patch >actual &&
+   test_cmp expected actual &&
+
+   q_to_tab >trailer <<-\EOF &&
+   name: first line
+   Qsecond line
+   EOF
+   q_to_tab >expected <<-\EOF &&
+
+   another: trailer
+   name: first line
+   Qsecond line
+   another: trailer
+   name: first line
+   Qsecond line
+   EOF
+   git interpret-trailers --trailer "$(cat trailer)" patch >actual &&
+   test_cmp expected actual &&
+
+   q_to_tab >trailer <<-\EOF &&
+   name: first line *DIFFERENT*
+   Qsecond line
+   EOF
+   q_to_tab >expected <<-\EOF &&
+
+   another: trailer
+   name: first line
+   Qsecond line
+   another: trailer
+   name: first line *DIFFERENT*
+   Qsecond line
+   EOF
+   git interpret-trailers --trailer "$(cat trailer)" patch >actual &&
+   test_cmp expected actual
+'
+
+test_expect_success 'multiline field treated as atomic for neighbor check' '
+   q_to_tab >patch <<-\EOF &&
+
+   another: trailer
+   name: first line
+   Qsecond line
+   another: trailer
+   EOF
+   test_config 

[PATCH v3 5/6] trailer: allow non-trailers in trailer block

[Jonathan Tan]

Currently, interpret-trailers requires all lines of a trailer block to
be trailers (or comments) - if not it would not identify that block as a
trailer block, and thus create its own trailer block, inserting a blank
line.  For example:

  echo -e "\na: b\nnot trailer" |
  git interpret-trailers --trailer "c: d"

would result in:

  a: b
  not trailer

  c: d

Relax the definition of a trailer block to only require 1 trailer, so
that trailers can be directly added to such blocks, resulting in:

  a: b
  not trailer
  c: d

This allows arbitrary lines to be included in trailer blocks, like those
in [1], and still allow interpret-trailers to be used.

This change also makes comments in the trailer block be treated as any
other non-trailer line, preserving them in the output of
interpret-trailers.

[1]
https://kernel.googlesource.com/pub/scm/linux/kernel/git/stable/linux-stable/+/e7d316a02f683864a12389f8808570e37fb90aa3

Signed-off-by: Jonathan Tan 
---
 Documentation/git-interpret-trailers.txt |  3 +-
 t/t7513-interpret-trailers.sh| 35 +++
 trailer.c| 77 ++--
 3 files changed, 90 insertions(+), 25 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt 
b/Documentation/git-interpret-trailers.txt
index 93d1db6..c480da6 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -48,7 +48,8 @@ with only spaces at the end of the commit message part, one 
blank line
 will be added before the new trailer.
 
 Existing trailers are extracted from the input message by looking for
-a group of one or more lines that contain a colon (by default), where
+a group of one or more lines in which at least one line contains a 
+colon (by default), where
 the group is preceded by one or more empty (or whitespace-only) lines.
 The group must either be at the end of the message or be the last
 non-whitespace lines before a line that starts with '---'. Such three
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index aee785c..7f5cd2a 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -126,6 +126,37 @@ test_expect_success 'with multiline title in the message' '
test_cmp expected actual
 '
 
+test_expect_success 'with non-trailer lines mixed with trailer lines' '
+   cat >patch <<-\EOF &&
+
+   this: is a trailer
+   this is not a trailer
+   EOF
+   cat >expected <<-\EOF &&
+
+   this: is a trailer
+   this is not a trailer
+   token: value
+   EOF
+   git interpret-trailers --trailer "token: value" patch >actual &&
+   test_cmp expected actual
+'
+
+test_expect_success 'with non-trailer lines only' '
+   cat >patch <<-\EOF &&
+
+   this is not a trailer
+   EOF
+   cat >expected <<-\EOF &&
+
+   this is not a trailer
+
+   token: value
+   EOF
+   git interpret-trailers --trailer "token: value" patch >actual &&
+   test_cmp expected actual
+'
+
 test_expect_success 'with config setup' '
git config trailer.ack.key "Acked-by: " &&
cat >expected <<-\EOF &&
@@ -257,6 +288,8 @@ test_expect_success 'with message that has comments' '
cat >>expected <<-\EOF &&
# comment
 
+   # other comment
+   # yet another comment
Reviewed-by: Johan
Cc: Peff
# last comment
@@ -286,6 +319,8 @@ test_expect_success 'with message that has an old style 
conflict block' '
cat >>expected <<-\EOF &&
# comment
 
+   # other comment
+   # yet another comment
Reviewed-by: Johan
Cc: Peff
# last comment
diff --git a/trailer.c b/trailer.c
index a9ed3f8..d6dfc7a 100644
--- a/trailer.c
+++ b/trailer.c
@@ -27,6 +27,10 @@ static struct conf_info default_conf_info;
 
 struct trailer_item {
struct list_head list;
+   /*
+* If this is not a trailer line, the line is stored in value
+* (excluding the terminating newline) and token is NULL.
+*/
char *token;
char *value;
 };
@@ -70,9 +74,14 @@ static size_t token_len_without_separator(const char *token, 
size_t len)
 
 static int same_token(struct trailer_item *a, struct arg_item *b)
 {
-   size_t a_len = token_len_without_separator(a->token, strlen(a->token));
-   size_t b_len = token_len_without_separator(b->token, strlen(b->token));
-   size_t min_len = (a_len > b_len) ? b_len : a_len;
+   size_t a_len, b_len, min_len;
+
+   if (!a->token)
+   return 0;
+
+   a_len = token_len_without_separator(a->token, strlen(a->token));
+   b_len = token_len_without_separator(b->token, strlen(b->token));
+   min_len = (a_len > b_len) ? b_len : a_len;
 
return 

[PATCH v3 4/6] trailer: make args have their own struct

[Jonathan Tan]

Improve type safety by making arguments (whether from configuration or
from the command line) have their own "struct arg_item" type, separate
from the "struct trailer_item" type used to represent the trailers in
the buffer being manipulated.

This change also prepares "struct trailer_item" to be further
differentiated from "struct arg_item" in future patches.

Signed-off-by: Jonathan Tan 
---
 trailer.c | 135 +++---
 1 file changed, 85 insertions(+), 50 deletions(-)

diff --git a/trailer.c b/trailer.c
index 54cc930..a9ed3f8 100644
--- a/trailer.c
+++ b/trailer.c
@@ -29,6 +29,12 @@ struct trailer_item {
struct list_head list;
char *token;
char *value;
+};
+
+struct arg_item {
+   struct list_head list;
+   char *token;
+   char *value;
struct conf_info conf;
 };
 
@@ -62,7 +68,7 @@ static size_t token_len_without_separator(const char *token, 
size_t len)
return len;
 }
 
-static int same_token(struct trailer_item *a, struct trailer_item *b)
+static int same_token(struct trailer_item *a, struct arg_item *b)
 {
size_t a_len = token_len_without_separator(a->token, strlen(a->token));
size_t b_len = token_len_without_separator(b->token, strlen(b->token));
@@ -71,12 +77,12 @@ static int same_token(struct trailer_item *a, struct 
trailer_item *b)
return !strncasecmp(a->token, b->token, min_len);
 }
 
-static int same_value(struct trailer_item *a, struct trailer_item *b)
+static int same_value(struct trailer_item *a, struct arg_item *b)
 {
return !strcasecmp(a->value, b->value);
 }
 
-static int same_trailer(struct trailer_item *a, struct trailer_item *b)
+static int same_trailer(struct trailer_item *a, struct arg_item *b)
 {
return same_token(a, b) && same_value(a, b);
 }
@@ -98,6 +104,13 @@ static inline void strbuf_replace(struct strbuf *sb, const 
char *a, const char *
 
 static void free_trailer_item(struct trailer_item *item)
 {
+   free(item->token);
+   free(item->value);
+   free(item);
+}
+
+static void free_arg_item(struct arg_item *item)
+{
free(item->conf.name);
free(item->conf.key);
free(item->conf.command);
@@ -137,17 +150,29 @@ static void print_all(FILE *outfile, struct list_head 
*head, int trim_empty)
}
 }
 
+static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)
+{
+   struct trailer_item *new = xcalloc(sizeof(*new), 1);
+   new->token = arg_tok->token;
+   new->value = arg_tok->value;
+   arg_tok->token = arg_tok->value = NULL;
+   free_arg_item(arg_tok);
+   return new;
+}
+
 static void add_arg_to_input_list(struct trailer_item *on_tok,
- struct trailer_item *arg_tok)
+ struct arg_item *arg_tok)
 {
-   if (after_or_end(arg_tok->conf.where))
-   list_add(_tok->list, _tok->list);
+   int aoe = after_or_end(arg_tok->conf.where);
+   struct trailer_item *to_add = trailer_from_arg(arg_tok);
+   if (aoe)
+   list_add(_add->list, _tok->list);
else
-   list_add_tail(_tok->list, _tok->list);
+   list_add_tail(_add->list, _tok->list);
 }
 
 static int check_if_different(struct trailer_item *in_tok,
- struct trailer_item *arg_tok,
+ struct arg_item *arg_tok,
  int check_all,
  struct list_head *head)
 {
@@ -200,7 +225,7 @@ static char *apply_command(const char *command, const char 
*arg)
return result;
 }
 
-static void apply_item_command(struct trailer_item *in_tok, struct 
trailer_item *arg_tok)
+static void apply_item_command(struct trailer_item *in_tok, struct arg_item 
*arg_tok)
 {
if (arg_tok->conf.command) {
const char *arg;
@@ -218,13 +243,13 @@ static void apply_item_command(struct trailer_item 
*in_tok, struct trailer_item
 }
 
 static void apply_arg_if_exists(struct trailer_item *in_tok,
-   struct trailer_item *arg_tok,
+   struct arg_item *arg_tok,
struct trailer_item *on_tok,
struct list_head *head)
 {
switch (arg_tok->conf.if_exists) {
case EXISTS_DO_NOTHING:
-   free_trailer_item(arg_tok);
+   free_arg_item(arg_tok);
break;
case EXISTS_REPLACE:
apply_item_command(in_tok, arg_tok);
@@ -241,39 +266,41 @@ static void apply_arg_if_exists(struct trailer_item 
*in_tok,
if (check_if_different(in_tok, arg_tok, 1, head))
add_arg_to_input_list(on_tok, arg_tok);
else
-   free_trailer_item(arg_tok);
+   free_arg_item(arg_tok);
break;
case 

[PATCH v3 2/6] trailer: use list.h for doubly-linked list

[Jonathan Tan]

Replace the existing handwritten implementation of a doubly-linked list
in trailer.c with the functions and macros from list.h. This
significantly simplifies the code.

Signed-off-by: Jonathan Tan 
---
 trailer.c | 258 ++
 1 file changed, 91 insertions(+), 167 deletions(-)

diff --git a/trailer.c b/trailer.c
index 1f191b2..0afa240 100644
--- a/trailer.c
+++ b/trailer.c
@@ -4,6 +4,7 @@
 #include "commit.h"
 #include "tempfile.h"
 #include "trailer.h"
+#include "list.h"
 /*
  * Copyright (c) 2013, 2014 Christian Couder 
  */
@@ -25,19 +26,24 @@ struct conf_info {
 static struct conf_info default_conf_info;
 
 struct trailer_item {
-   struct trailer_item *previous;
-   struct trailer_item *next;
+   struct list_head list;
char *token;
char *value;
struct conf_info conf;
 };
 
-static struct trailer_item *first_conf_item;
+LIST_HEAD(conf_head);
 
 static char *separators = ":";
 
 #define TRAILER_ARG_STRING "$ARG"
 
+/* Iterate over the elements of the list. */
+#define list_for_each_dir(pos, head, is_reverse) \
+   for (pos = is_reverse ? (head)->prev : (head)->next; \
+   pos != (head); \
+   pos = is_reverse ? pos->prev : pos->next)
+
 static int after_or_end(enum action_where where)
 {
return (where == WHERE_AFTER) || (where == WHERE_END);
@@ -120,101 +126,49 @@ static void print_tok_val(FILE *outfile, const char 
*tok, const char *val)
fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
 }
 
-static void print_all(FILE *outfile, struct trailer_item *first, int 
trim_empty)
+static void print_all(FILE *outfile, struct list_head *head, int trim_empty)
 {
+   struct list_head *pos;
struct trailer_item *item;
-   for (item = first; item; item = item->next) {
+   list_for_each(pos, head) {
+   item = list_entry(pos, struct trailer_item, list);
if (!trim_empty || strlen(item->value) > 0)
print_tok_val(outfile, item->token, item->value);
}
 }
 
-static void update_last(struct trailer_item **last)
-{
-   if (*last)
-   while ((*last)->next != NULL)
-   *last = (*last)->next;
-}
-
-static void update_first(struct trailer_item **first)
-{
-   if (*first)
-   while ((*first)->previous != NULL)
-   *first = (*first)->previous;
-}
-
 static void add_arg_to_input_list(struct trailer_item *on_tok,
- struct trailer_item *arg_tok,
- struct trailer_item **first,
- struct trailer_item **last)
-{
-   if (after_or_end(arg_tok->conf.where)) {
-   arg_tok->next = on_tok->next;
-   on_tok->next = arg_tok;
-   arg_tok->previous = on_tok;
-   if (arg_tok->next)
-   arg_tok->next->previous = arg_tok;
-   update_last(last);
-   } else {
-   arg_tok->previous = on_tok->previous;
-   on_tok->previous = arg_tok;
-   arg_tok->next = on_tok;
-   if (arg_tok->previous)
-   arg_tok->previous->next = arg_tok;
-   update_first(first);
-   }
+ struct trailer_item *arg_tok)
+{
+   if (after_or_end(arg_tok->conf.where))
+   list_add(_tok->list, _tok->list);
+   else
+   list_add_tail(_tok->list, _tok->list);
 }
 
 static int check_if_different(struct trailer_item *in_tok,
  struct trailer_item *arg_tok,
- int check_all)
+ int check_all,
+ struct list_head *head)
 {
enum action_where where = arg_tok->conf.where;
+   struct list_head *next_head;
do {
-   if (!in_tok)
-   return 1;
if (same_trailer(in_tok, arg_tok))
return 0;
/*
 * if we want to add a trailer after another one,
 * we have to check those before this one
 */
-   in_tok = after_or_end(where) ? in_tok->previous : in_tok->next;
+   next_head = after_or_end(where) ? in_tok->list.prev
+   : in_tok->list.next;
+   if (next_head == head)
+   break;
+   in_tok = list_entry(next_head, struct trailer_item, list);
} while (check_all);
return 1;
 }
 
-static void remove_from_list(struct trailer_item *item,
-struct trailer_item **first,
-struct trailer_item **last)
-{
-   struct trailer_item *next = item->next;
-   struct trailer_item *previous = item->previous;
-
-

[PATCH v3 3/6] trailer: streamline trailer item create and add

[Jonathan Tan]

Currently, creation and addition (to a list) of trailer items are spread
across multiple functions. Streamline this by only having 2 functions:
one to parse the user-supplied string, and one to add the parsed
information to a list.

Signed-off-by: Jonathan Tan 
---
 trailer.c | 130 +-
 1 file changed, 60 insertions(+), 70 deletions(-)

diff --git a/trailer.c b/trailer.c
index 0afa240..54cc930 100644
--- a/trailer.c
+++ b/trailer.c
@@ -500,10 +500,31 @@ static int git_trailer_config(const char *conf_key, const 
char *value, void *cb)
return 0;
 }
 
-static int parse_trailer(struct strbuf *tok, struct strbuf *val, const char 
*trailer)
+static const char *token_from_item(struct trailer_item *item, char *tok)
+{
+   if (item->conf.key)
+   return item->conf.key;
+   if (tok)
+   return tok;
+   return item->conf.name;
+}
+
+static int token_matches_item(const char *tok, struct trailer_item *item, int 
tok_len)
+{
+   if (!strncasecmp(tok, item->conf.name, tok_len))
+   return 1;
+   return item->conf.key ? !strncasecmp(tok, item->conf.key, tok_len) : 0;
+}
+
+static int parse_trailer(struct strbuf *tok, struct strbuf *val,
+const struct conf_info **conf, const char *trailer)
 {
size_t len;
struct strbuf seps = STRBUF_INIT;
+   struct trailer_item *item;
+   int tok_len;
+   struct list_head *pos;
+
strbuf_addstr(, separators);
strbuf_addch(, '=');
len = strcspn(trailer, seps.buf);
@@ -523,74 +544,31 @@ static int parse_trailer(struct strbuf *tok, struct 
strbuf *val, const char *tra
strbuf_addstr(tok, trailer);
strbuf_trim(tok);
}
-   return 0;
-}
-
-static const char *token_from_item(struct trailer_item *item, char *tok)
-{
-   if (item->conf.key)
-   return item->conf.key;
-   if (tok)
-   return tok;
-   return item->conf.name;
-}
-
-static struct trailer_item *new_trailer_item(struct trailer_item *conf_item,
-char *tok, char *val)
-{
-   struct trailer_item *new = xcalloc(sizeof(*new), 1);
-   new->value = val ? val : xstrdup("");
-
-   if (conf_item) {
-   duplicate_conf(>conf, _item->conf);
-   new->token = xstrdup(token_from_item(conf_item, tok));
-   free(tok);
-   } else {
-   duplicate_conf(>conf, _conf_info);
-   new->token = tok;
-   }
-
-   return new;
-}
-
-static int token_matches_item(const char *tok, struct trailer_item *item, int 
tok_len)
-{
-   if (!strncasecmp(tok, item->conf.name, tok_len))
-   return 1;
-   return item->conf.key ? !strncasecmp(tok, item->conf.key, tok_len) : 0;
-}
-
-static struct trailer_item *create_trailer_item(const char *string)
-{
-   struct strbuf tok = STRBUF_INIT;
-   struct strbuf val = STRBUF_INIT;
-   struct trailer_item *item;
-   int tok_len;
-   struct list_head *pos;
-
-   if (parse_trailer(, , string))
-   return NULL;
-
-   tok_len = token_len_without_separator(tok.buf, tok.len);
 
/* Lookup if the token matches something in the config */
+   tok_len = token_len_without_separator(tok->buf, tok->len);
+   *conf = _conf_info;
list_for_each(pos, _head) {
item = list_entry(pos, struct trailer_item, list);
-   if (token_matches_item(tok.buf, item, tok_len))
-   return new_trailer_item(item,
-   strbuf_detach(, NULL),
-   strbuf_detach(, NULL));
+   if (token_matches_item(tok->buf, item, tok_len)) {
+   char *tok_buf = strbuf_detach(tok, NULL);
+   *conf = >conf;
+   strbuf_addstr(tok, token_from_item(item, tok_buf));
+   free(tok_buf);
+   break;
+   }
}
 
-   return new_trailer_item(NULL,
-   strbuf_detach(, NULL),
-   strbuf_detach(, NULL));
+   return 0;
 }
 
-static void add_trailer_item(struct list_head *head, struct trailer_item *new)
+static void add_trailer_item(struct list_head *head, char *tok, char *val,
+const struct conf_info *conf)
 {
-   if (!new)
-   return;
+   struct trailer_item *new = xcalloc(sizeof(*new), 1);
+   new->token = tok;
+   new->value = val;
+   duplicate_conf(>conf, conf);
list_add_tail(>list, head);
 }
 
@@ -599,21 +577,28 @@ static void process_command_line_args(struct list_head 
*arg_head,
 {
struct string_list_item *tr;
struct trailer_item *item;
+   struct strbuf tok = STRBUF_INIT;
+   struct 

[PATCH v3 1/6] trailer: improve const correctness

[Jonathan Tan]

Change "const char *" to "char *" in struct trailer_item and in the
return value of apply_command (since those strings are owned strings).

Change "struct conf_info *" to "const struct conf_info *" (since that
struct is not modified).

Signed-off-by: Jonathan Tan 
---
 trailer.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/trailer.c b/trailer.c
index c6ea9ac..1f191b2 100644
--- a/trailer.c
+++ b/trailer.c
@@ -27,8 +27,8 @@ static struct conf_info default_conf_info;
 struct trailer_item {
struct trailer_item *previous;
struct trailer_item *next;
-   const char *token;
-   const char *value;
+   char *token;
+   char *value;
struct conf_info conf;
 };
 
@@ -95,8 +95,8 @@ static void free_trailer_item(struct trailer_item *item)
free(item->conf.name);
free(item->conf.key);
free(item->conf.command);
-   free((char *)item->token);
-   free((char *)item->value);
+   free(item->token);
+   free(item->value);
free(item);
 }
 
@@ -215,13 +215,13 @@ static struct trailer_item *remove_first(struct 
trailer_item **first)
return item;
 }
 
-static const char *apply_command(const char *command, const char *arg)
+static char *apply_command(const char *command, const char *arg)
 {
struct strbuf cmd = STRBUF_INIT;
struct strbuf buf = STRBUF_INIT;
struct child_process cp = CHILD_PROCESS_INIT;
const char *argv[] = {NULL, NULL};
-   const char *result;
+   char *result;
 
strbuf_addstr(, command);
if (arg)
@@ -425,7 +425,7 @@ static int set_if_missing(struct conf_info *item, const 
char *value)
return 0;
 }
 
-static void duplicate_conf(struct conf_info *dst, struct conf_info *src)
+static void duplicate_conf(struct conf_info *dst, const struct conf_info *src)
 {
*dst = *src;
if (src->name)
-- 
2.8.0.rc3.226.g39d4020

[PATCH v3 0/6] allow non-trailers and multiple-line trailers

[Jonathan Tan]

Ah, I knew I forgot something. These are exactly the same as v2, except
that these are signed off.

Jonathan Tan (6):
  trailer: improve const correctness
  trailer: use list.h for doubly-linked list
  trailer: streamline trailer item create and add
  trailer: make args have their own struct
  trailer: allow non-trailers in trailer block
  trailer: support values folded to multiple lines

 Documentation/git-interpret-trailers.txt |  10 +-
 t/t7513-interpret-trailers.sh| 174 ++
 trailer.c| 538 +++
 3 files changed, 444 insertions(+), 278 deletions(-)

-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH v2 1/3] gitweb: Fix a typo in a comment

[Jakub Narębski]

W dniu 06.10.2016 o 11:11, Ævar Arnfjörð Bjarmason napisał:

> Change a typo'd MIME type in a comment. The Content-Type is
> application/xhtml+xml, not application/xhtm+xml.
> 
> Fixes up code originally added in 53c4031 ("gitweb: Strip
> non-printable characters from syntax highlighter output", 2011-09-16).
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason 

Good.  Thanks for taking care of this.
For what is worth for such a trivial patch:

Acked-by: Jakub Narębski 

> ---
>  gitweb/gitweb.perl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 44094f4..cba7405 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1616,7 +1616,7 @@ sub esc_path {
>   return $str;
>  }
>  
> -# Sanitize for use in XHTML + application/xml+xhtm (valid XML 1.0)
> +# Sanitize for use in XHTML + application/xml+xhtml (valid XML 1.0)
>  sub sanitize {
>   my $str = shift;
>  
> 

Re: Formatting problem send_mail in version 2.10.0

[Junio C Hamano]

Matthieu Moy  writes:

> We clearly can't guess, but we can be consistent with Mail::Address, so
> that git's behavior depends less on its availability.
>
> Patch follows doing that.

Thanks.  I love that somebody counters with a much better way to
solve it whenever I suggest an outrageous non-solution to a problem
around here ;-)

Re: Uninitialized submodules as symlinks

[Junio C Hamano]

Kevin Daudt  writes:

> On Thu, Oct 13, 2016 at 06:10:17PM +0200, Heiko Voigt wrote:
>> On Fri, Oct 07, 2016 at 06:17:05PM +, David Turner wrote:
>> > Presently, uninitialized submodules are materialized in the working
>> > tree as empty directories.  We would like to consider having them be
>> > symlinks.  Specifically, we'd like them to be symlinks into a FUSE
>> > filesystem which retrieves files on demand.
>> 
>> How about portability? This feature would only work on Unix like
>> operating systems. You have to be careful to not break Windows since
>> they do not have symlinks.
>
> NTFS does have symlinks, but you need admin right to create them though
> (unless you change the policy).

That sounds like saying "It has, but it practically is not usable by
Git as a mechanism to achieve this goal" to me.

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

[Junio C Hamano]

Johannes Schindelin  writes:

>> I think my puzzlement comes from here.  What makes it OK for "am" to
>> expect the contents of author-script file to be quoted but it is not
>> OK to expect the same here?  What makes it not quoted for _this_
>> reader, in other words?
>
> The `git am` command is inherently *not* interactive, while the
> interactive rebase, well, is.
>
> As such, we must assume that enterprisey users did come up with scripts
> that edit, or create, author-script files, and exploited the fact that the
> interactive rebase previously sourced them.

It's like saying "While the program is waiting for user interaction,
the user can attach a debugger to tweak the contents of this string
variable.  Hence we must not assume that the string is always NUL
terminated to protect ourselves."

A correct response to such a user is to tell them not to do that,
and do so at two levels.  

The first level may be "it may be physically possible to tweak the
string via debugger but don't do that in such a way that breaks the
invariants our program relies on, or you are on your own", but more
important is the response at the second level.  Why is the user
futzing with that string in the first place with the debugger?  In
the context of author-script, the question is "Why is the user
futzing with the author-script file"?  To attribute the resulting
commit to a different author, of course.  But we need to step back
and think why the user needs to resort to editing that file to
achieve that.

If that is a common thing users would want to do, then we should
offer an official way to do so, and it should not be "you can futz
with author-script file with your editor".  Something like "have
'exec git commit --amend' in the todo file" may be more appropriate
and if it is important enough, the sequencer command language may
want to learn an extra verb to update the author.

Besides, the opportunity easiest for the user to futz with the
contents of author-script file (or "attach the debugger while we
wait for user interaction") arises when "rebase -i" or "am" stops
waiting for conflict resolution.  Even when you run "am" without its
"-i" option, it is equally susceptible to the "user futzing with the
file", which means your "am is not interactive but 'rebase -i' is"
is irrelevant to the issue.  Oh, also, did I say "am" has "-i"
option, which is a short-hand for "--interactive"?

What disturbs me the most is that I know you know the system well
enough to realize how bogus your argument to claim that "rebase -i"
and "am" are different was and to come up with what I wrote above
yourself.  Which means that I need to conclude that you have some
other reasons why you want to keep this parser different, but I
still do not know what they are.

I guess it entirely is possible that one of the reasons is because
some later patches in the larger "rebase-i to sequencer" series
writes author-script file in a syntax that cannot be read by the
recently refactored code "am" uses to read the author-script file,
and reusing the existing code may end up breaking the endgame
"rebase -i" you have.

As I do not feel like arguing with you on this any longer, and as I
certainly do not want to be blamed for breaking your "rebase -i", I
do not insist the code to be refactored to share with the existing
codepath.  But still I do not see the need to keep them separate (as
I already said in the previous message, I am OK if the one used in
"am" is updated to match).

[PATCH v15 27/27] bisect--helper: remove the dequote in bisect_start()

[Pranit Bauva]

Dequoting the arguments was introduced in 25b48b5c to port the function
`bisect_next()` but after the `bisect_replay()` porting, the dequoting
is carried out itself when it passes the arguments to `bisect_start()`
in a simpler way thus dequoting again isn't required. So remove the
extra "deqouting" code introduced by the commit 25b48b5c.

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 26 ++
 1 file changed, 6 insertions(+), 20 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index d6c2efc..8cd6527 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -561,24 +561,16 @@ static int bisect_start(struct bisect_terms *terms, int 
no_checkout,
no_checkout = 1;
 
for (i = 0; i < argc; i++) {
-   const char *arg;
-   if (starts_with(argv[i], "'"))
-   arg = sq_dequote(xstrdup(argv[i]));
-   else
-   arg = argv[i];
-   if (!strcmp(arg, "--")) {
+   if (!strcmp(argv[i], "--")) {
has_double_dash = 1;
break;
}
}
 
for (i = 0; i < argc; i++) {
-   const char *arg, *commit_id;
-   if (starts_with(argv[i], "'"))
-   arg = sq_dequote(xstrdup(argv[i]));
-   else
-   arg = argv[i];
-   commit_id = xstrfmt("%s^{commit}", arg);
+   const char  *commit_id;
+   const char *arg = argv[i];
+   commit_id = xstrfmt("%s^{commit}", argv[i]);
if (!strcmp(argv[i], "--")) {
has_double_dash = 1;
break;
@@ -586,11 +578,8 @@ static int bisect_start(struct bisect_terms *terms, int 
no_checkout,
no_checkout = 1;
} else if (!strcmp(arg, "--term-good") ||
 !strcmp(arg, "--term-old")) {
-   if (starts_with(argv[++i], "'"))
-   terms->term_good = sq_dequote(xstrdup(argv[i]));
-   else
-   terms->term_good = xstrdup(argv[i]);
must_write_terms = 1;
+   terms->term_good = xstrdup(argv[++i]);
} else if (skip_prefix(arg, "--term-good=", )) {
must_write_terms = 1;
terms->term_good = arg;
@@ -599,10 +588,7 @@ static int bisect_start(struct bisect_terms *terms, int 
no_checkout,
terms->term_good = arg;
} else if (!strcmp(arg, "--term-bad") ||
 !strcmp(arg, "--term-new")) {
-   if (starts_with(argv[++i], "'"))
-   terms->term_bad = sq_dequote(xstrdup(argv[i]));
-   else
-   terms->term_bad = xstrdup(argv[i]);
+   terms->term_bad = xstrdup(argv[++i]);
must_write_terms = 1;
} else if (skip_prefix(arg, "--term-bad=", )) {
must_write_terms = 1;

--
https://github.com/git/git/pull/287

Re: What's cooking in git.git (Oct 2016, #03; Tue, 11)

[Junio C Hamano]

Johannes Schindelin  writes:

>> >> * sb/submodule-ignore-trailing-slash (2016-10-10) 2 commits
>> >>   (merged to 'next' on 2016-10-11 at e37425ed17)
>> >>  + submodule: ignore trailing slash in relative url
>> >>  + submodule: ignore trailing slash on superproject URL
>> >>
>> >>  A minor regression fix for "git submodule".
>> >>
>> >>  Will merge to 'master'.
>> >
>> > Going by the bug report, this *may* be more than
>> > minor and worth merging down to maint as well, eventually.
>> 
>> The topic was forked at a reasonably old commit so that it can be
>> merged as far down to maint-2.9 if we wanted to.  Which means the
>> regression was fairly old and fix is not all that urgent as well.
>
> And if you merge it to `master` and `maint`,...

I'll mark it as "wait for follow-up fix" in whats-cooking.txt (on
'todo' branch) to remind myself not to merge it yet.

Thanks for reminding.

As I was mostly offline yesterday, it will take a while until I
clear my backlog on the list.

[PATCH v15 11/27] bisect--helper: `bisect_next_check` & bisect_voc shell function in C

[Pranit Bauva]

Reimplement `bisect_next_check` shell function in C and add
`bisect-next-check` subcommand to `git bisect--helper` to call it from
git-bisect.sh .

Also reimplement `bisect_voc` shell function in C and call it from
`bisect_next_check` implementation in C.

Using `--bisect-next-check` is a temporary measure to port shell
function to C so as to use the existing test suite. As more functions
are ported, this subcommand will be retired but its implementation will
be called by some other methods.

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 110 ++-
 git-bisect.sh|  60 ++
 2 files changed, 113 insertions(+), 57 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index c6c11e3..317d671 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -6,6 +6,7 @@
 #include "dir.h"
 #include "argv-array.h"
 #include "run-command.h"
+#include "prompt.h"
 
 static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
 static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
@@ -21,6 +22,7 @@ static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --bisect-reset []"),
N_("git bisect--helper --bisect-write
 []"),
N_("git bisect--helper --bisect-check-and-set-terms  
 "),
+   N_("git bisect--helper --bisect-next-check []  
term_bad);
+   char *good_glob = xstrfmt("%s-*", terms->term_good);
+   char *bad_syn, *good_syn;
+
+   if (ref_exists(bad_ref))
+   missing_bad = 0;
+
+   for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/",
+(void *) _good);
+
+   if (!missing_good && !missing_bad)
+   goto finish;
+
+   if (!current_term) {
+   retval = -1;
+   goto finish;
+   }
+
+   if (missing_good && !missing_bad && current_term &&
+   !strcmp(current_term, terms->term_good)) {
+   char *yesno;
+   /*
+* have bad (or new) but not good (or old). We could bisect
+* although this is less optimum.
+*/
+   fprintf(stderr, _("Warning: bisecting only with a %s commit\n"),
+   terms->term_bad);
+   if (!isatty(0))
+   goto finish;
+   /*
+* TRANSLATORS: Make sure to include [Y] and [n] in your
+* translation. The program will only accept English input
+* at this point.
+*/
+   yesno = git_prompt(_("Are you sure [Y/n]? "), PROMPT_ECHO);
+   if (starts_with(yesno, "N") || starts_with(yesno, "n")) {
+   retval = -1;
+   goto finish;
+   }
+
+   goto finish;
+   }
+   bad_syn = xstrdup(bisect_voc("bad"));
+   good_syn = xstrdup(bisect_voc("good"));
+   if (!is_empty_or_missing_file(git_path_bisect_start())) {
+   error(_("You need to give me at least one %s and "
+   "%s revision. You can use \"git bisect %s\" "
+   "and \"git bisect %s\" for that. \n"),
+   bad_syn, good_syn, bad_syn, good_syn);
+   retval = -1;
+   goto finish;
+   }
+   else {
+   error(_("You need to start by \"git bisect start\". You "
+   "then need to give me at least one %s and %s "
+   "revision. You can use \"git bisect %s\" and "
+   "\"git bisect %s\" for that.\n"),
+   good_syn, bad_syn, bad_syn, good_syn);
+   retval = -1;
+   goto finish;
+   }
+   goto finish;
+finish:
+   if (!bad_ref)
+   free(bad_ref);
+   if (!good_glob)
+   free(good_glob);
+   if (!bad_syn)
+   free(bad_syn);
+   if (!good_syn)
+

Re: Why does git checkout -b touch the index?

[Junio C Hamano]

David Turner  writes:

> If I do "git checkout -b fleem", with no additional flags, why does it
> need to rewrite the index?  Or even read the index? 

The "reading" part can be explained easily.

It needs to show the list of dirty paths, which involves reading the
index, refreshing them in-core to cull the timestamp-only changes
out of the report.  

Once in-core index is refreshed, it is a waste not to write them out
if it can, so it is not surprising if it writes, too.

The real reason is a lot more involved.  "git checkout -b new" is
different from "git update-ref refs/heads/new HEAD" (i.e. create a
new branch at the same commit) followed by "git symbolic-ref HEAD
refs/heads/new" (i.e. point the HEAD at it).  If it were, I fully
agree with you that it shouldn't need to touch either index or
working tree at all.

It is "git branch new" followed by "git checkout new", and the
latter step does the full two-tree "read-tree" between the same two
commits.  I personally very much prefer "Switching between the same
two commits?  That should be a no-op by definition.", but we cannot
blindly that naive optimization, as it would break some use cases
(which I personally do not care too much about ;-).

For example, extra working tree files outside the "sparse" area are
removed when sparse-checkout is in use while switching from the
current branch to the new and identical branch.  I do not personally
agree with the behaviour, but that has been what the users accept;
there may be other funnies like that.

Re: [PATCHv3] attr: convert to new threadsafe API

[Junio C Hamano]

Junio C Hamano  writes:

> *1* Would we need a wrapping struct around the array of results?

By the way, I do see a merit on the "check" side (tl;dr: but I do
not think "result" needs it, hence I do not see the need for the
"ugly" variants).

Take "archive" for example.  For each path, it wants to see the
attribute "export-ignore" to decide if it is to be omitted.  In
addition, the usual set of attributes used to smudge blobs into the
working tree representation are inspected by the convert.c API as
part of its implementation of convert_to_working_tree().  This
program has at least two sets of <"check", "result"> that are used
by two git_check_attr() callsites that are unaware of each other.

One of the optimizations we discussed is to trim down the attr-stack
(which caches the attributes read from .gitattributes files that are
in effect for the "last" directory that has the path for which
attrbiutes are queried for) by reading/keeping only the entries that
affect the attributes the caller is interested in.  But when there
are multiple callsites that are interested in different sets of
attributes, we obviously cannot do such an optimization without
taking too much cache-invalidation hit.  Because these callsites are
not unaware of each other, I do not think we can say "keep the
entries that affects the union of all active callsites" very easily,
even if it were possible.

But we could tie this cache to "check", which keeps a constant
subset of attributes that the caller is interested in (i.e. each
callsite would keep its own cache that is useful for its query).
While we are single-threaded, "struct git_attr_check" being a
wrapping struct around the array of "what attributes are of
interest?" is a good place to add that per-check attr-stack cache.
When we go multi-threaded, the attr-stack cache must become
per-thread, and needs to be moved to per-thread storage, and such a
per-thread storage would have multiple attr-stack, one per "check"
instance (i.e. looking up the attr-stack may have to say "who/what
thread am I?" to first go to the thread-local storage for the
current thread, where a table of pointers to attr-stacks is kept and
from there, index into that table to find the attr-stack that
corresponds to the particular "check").  We could use the address of
"check" as the key into this table, but "struct git_attr_check" that
wraps the array gives us another option to allocate a small
consecutive integer every time initl() creates a new "check" and use
it as the index into that attr-stack table, as that integer index
can be in the struct that wraps the array of wanted attributes.

Note. none of the above is a suggestion to do the attr
caching the way exactly described.  The above is primarily
to illustrate how a wrapping struct may give us future
flexibility without affecting a single line of code in the
user of API.

It may turn out that we do not need to have anything other than the
array of wanted attributes in the "check" struct, but unlike
"result", "check" is shared across threads, and do not have to live
directly on the stack, so we can prepare for flexibility.

I do not foresee a similar need for wrapping struct for "result",
and given that we do want to keep the option of having them directly
on the stack, I am inclined to say we shouldn't introduce one.

If we were still to do the wrapping for result, I would say that
basing it around the FLEX_ARRAY idiom, i.e.

> struct git_attr_result {
> int num_slots;
> const char *value[FLEX_ARRAY];
> };

is a horrible idea.  It would be less horrible if it were

struct git_attr_result {
int num_slots;
const char **value;
};

then make the API user write via a convenience macro something like
this

const char *result_values[NUM_ATTRS_OF_INTEREST];
struct git_attr_result result = {
ARRAY_SIZE(result_values), _values
};

instead.  That way, at least the side that implements git_check_attr()
would not have to be type-unsafe like the example of ugliness in the
message I am following-up on.

Re: [PATCH v15 01/27] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Pranit Bauva]

Hey everyone,

It took me some time to get to the next version as I was a bit
preoccupied with my assignments and exams. The diff between the v14[1]
and v15[2] can be found here[3] because gmail web client will wrap the
lines. Thanks Junio for the reviews in v14. I have tried to solves
every issue raised in the previous round.

The major changes in this series are:
 * Use the char * instead of struct strbuf in struct bisect_terms.
 * Use goto mechanism for error handling.
 * Use get_sha1_committish() instead of just get_oid() because there
is also ^{commit} at the end of the call from shell.
 * Use skip_prefix() where ever required.
 * Use return error() instead of die() where ever possible.
 * Restructure a part in bisect_start() to make it more readable.
 * Change the comments in bisect.c to use return instead of exit
because the code does it.
 * Introduce a test to check for cleanups in after bad merge base to
make the patch 15/28 more understood.
 * Also keeping comments in bisect.c to show at which places there
would be cleanups and where not.
 * Restructure bisect_replay() and get_next_word().


[1]: 
http://public-inbox.org/git/01020156b73fe5b4-5dc768ab-b73b-4a21-ab92-018e2a7aa6f7-000...@eu-west-1.amazonses.com/T/#m7c26060fcf95abbd19f93742d7317eef87b915a1
[2]: 
http://public-inbox.org/git/01020157c38b19e0-81123fa5-5d9d-4f64-8f1b-ff336e83ebe4-000...@eu-west-1.amazonses.com/T/#u
[3]: http://paste.ubuntu.com/23323581/

Regards,
Pranit Bauva

[PATCH v15 25/27] bisect--helper: retire `--bisect-autostart` subcommand

[Pranit Bauva]

The `--bisect-autostart` subcommand is no longer used in the shell
script and the function `bisect_autostart()` is called from the C
implementation.

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 11 ---
 1 file changed, 11 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 7f676e2..8653fde 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -28,7 +28,6 @@ static const char * const git_bisect_helper_usage[] = {
  "[--no-checkout] [ 
[...]] [--] [...]"),
N_("git bisect--helper --bisect-next"),
N_("git bisect--helper --bisect-auto-next"),
-   N_("git bisect--helper --bisect-autostart"),
N_("git bisect--helper --bisect-state (bad|new) []"),
N_("git bisect--helper --bisect-state (good|old) [...]"),
N_("git bisect--helper --bisect-replay "),
@@ -995,7 +994,6 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
BISECT_START,
BISECT_NEXT,
BISECT_AUTO_NEXT,
-   BISECT_AUTOSTART,
BISECT_STATE,
BISECT_LOG,
BISECT_REPLAY
@@ -1016,8 +1014,6 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 N_("find the next bisection commit"), BISECT_NEXT),
OPT_CMDMODE(0, "bisect-auto-next", ,
 N_("verify the next bisection state then find the next 
bisection state"), BISECT_AUTO_NEXT),
-   OPT_CMDMODE(0, "bisect-autostart", ,
-N_("start the bisection if BISECT_START empty or 
missing"), BISECT_AUTOSTART),
OPT_CMDMODE(0, "bisect-state", ,
 N_("mark the state of ref (or refs)"), BISECT_STATE),
OPT_CMDMODE(0, "bisect-log", ,
@@ -1079,13 +1075,6 @@ int cmd_bisect__helper(int argc, const char **argv, 
const char *prefix)
get_terms();
res = bisect_auto_next(, prefix);
break;
-   case BISECT_AUTOSTART:
-   if (argc)
-   die(_("--bisect-autostart requires 0 arguments"));
-   terms.term_good = "good";
-   terms.term_bad = "bad";
-   res = bisect_autostart();
-   break;
case BISECT_STATE:
if (argc == 0)
die(_("--bisect-state requires at least 1 argument"));

--
https://github.com/git/git/pull/287

[PATCH v15 08/27] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C

[Pranit Bauva]

Reimplement `is_expected_rev` & `check_expected_revs` shell function in
C and add a `--check-expected-revs` subcommand to `git bisect--helper` to
call it from git-bisect.sh .

Using `--check-expected-revs` subcommand is a temporary measure to port
shell functions to C so as to use the existing test suite. As more
functions are ported, this subcommand would be retired but its
implementation will be called by some other method.

Helped-by: Eric Sunshine 
Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 33 -
 git-bisect.sh| 20 ++--
 2 files changed, 34 insertions(+), 19 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index d84ba86..c542e8b 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -123,13 +123,40 @@ static int bisect_reset(const char *commit)
return bisect_clean_state();
 }
 
+static int is_expected_rev(const char *expected_hex)
+{
+   struct strbuf actual_hex = STRBUF_INIT;
+   int res = 0;
+   if (strbuf_read_file(_hex, git_path_bisect_expected_rev(), 0) >= 
40) {
+   strbuf_trim(_hex);
+   res = !strcmp(actual_hex.buf, expected_hex);
+   }
+   strbuf_release(_hex);
+   return res;
+}
+
+static int check_expected_revs(const char **revs, int rev_nr)
+{
+   int i;
+
+   for (i = 0; i < rev_nr; i++) {
+   if (!is_expected_rev(revs[i])) {
+   unlink_or_warn(git_path_bisect_ancestors_ok());
+   unlink_or_warn(git_path_bisect_expected_rev());
+   return 0;
+   }
+   }
+   return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
enum {
NEXT_ALL = 1,
WRITE_TERMS,
BISECT_CLEAN_STATE,
-   BISECT_RESET
+   BISECT_RESET,
+   CHECK_EXPECTED_REVS
} cmdmode = 0;
int no_checkout = 0;
struct option options[] = {
@@ -141,6 +168,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 N_("cleanup the bisection state"), BISECT_CLEAN_STATE),
OPT_CMDMODE(0, "bisect-reset", ,
 N_("reset the bisection state"), BISECT_RESET),
+   OPT_CMDMODE(0, "check-expected-revs", ,
+N_("check for expected revs"), CHECK_EXPECTED_REVS),
OPT_BOOL(0, "no-checkout", _checkout,
 N_("update BISECT_HEAD instead of checking out the 
current commit")),
OPT_END()
@@ -167,6 +196,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
if (argc > 1)
die(_("--bisect-reset requires either zero or one 
arguments"));
return bisect_reset(argc ? argv[0] : NULL);
+   case CHECK_EXPECTED_REVS:
+   return check_expected_revs(argv, argc);
default:
die("BUG: unknown subcommand '%d'", cmdmode);
}
diff --git a/git-bisect.sh b/git-bisect.sh
index 442397b..c3e43248 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -237,22 +237,6 @@ bisect_write() {
test -n "$nolog" || echo "git bisect $state $rev" 
>>"$GIT_DIR/BISECT_LOG"
 }
 
-is_expected_rev() {
-   test -f "$GIT_DIR/BISECT_EXPECTED_REV" &&
-   test "$1" = $(cat "$GIT_DIR/BISECT_EXPECTED_REV")
-}
-
-check_expected_revs() {
-   for _rev in "$@"; do
-   if ! is_expected_rev "$_rev"
-   then
-   rm -f "$GIT_DIR/BISECT_ANCESTORS_OK"
-   rm -f "$GIT_DIR/BISECT_EXPECTED_REV"
-   return
-   fi
-   done
-}
-
 bisect_skip() {
all=''
for arg in "$@"
@@ -280,7 +264,7 @@ bisect_state() {
rev=$(git rev-parse --verify "$bisected_head") ||
die "$(eval_gettext "Bad rev input: \$bisected_head")"
bisect_write "$state" "$rev"
-   check_expected_revs "$rev" ;;
+   git bisect--helper --check-expected-revs "$rev" ;;
2,"$TERM_BAD"|*,"$TERM_GOOD"|*,skip)
shift
hash_list=''
@@ -294,7 +278,7 @@ bisect_state() {
do
bisect_write "$state" "$rev"
done
-   check_expected_revs $hash_list ;;
+   git bisect--helper --check-expected-revs $hash_list ;;
*,"$TERM_BAD")
die "$(eval_gettext "'git bisect \$TERM_BAD' can take only one 
argument.")" ;;
*)

--
https://github.com/git/git/pull/287

Re: [PATCH 0/2] infinite loop in "git ls-tree" for broken symlink

[Petr Stodulka]

Thank you for info, I totally missed that. Yes, this fixes the issue
perfectly.

Petr

On 14.10.2016 15:42, Jeff King wrote:
> On Fri, Oct 14, 2016 at 03:16:50PM +0200, Petr Stodulka wrote:
> 
>> I have realized that this wasn't fixed after all when refs.c
>> was "rewritten". Issue is caused by broken symlink under refs/heads,
>> which causes infinite loop for "git ls-tree" command. It was replied
>> earlier [0] and Michael previously fixed that in better way probably,
>> then my proposed patch 2/2, but it contains more changes and I have not
>> enough time to study changes in refs* code, so I propose now just my
>> little patch, which was previously modified by Michael.
>>
>> If you prefer different solution, I am ok with this. It is really
>> just quick proposal. Patch 1/2 contains test for this issue. If you
>> will prefer different solution with different exit code, test should
>> be corrected. Basic idea is, that timeout should'n expire with
>> exit code 124.
>>
>> [0] http://marc.info/?l=git=142712669103790=2
>> [1] https://github.com/mhagger/git, branch "ref-broken-symlinks"
> 
> I think I fixed this semi-independently last week; see the thread at:
> 
>   
> http://public-inbox.org/git/20161006164723.ocg2nbgtulpjc...@sigill.intra.peff.net/
> 
> I say semi-independently because I didn't actually know about your
> previous report, but saw it in the wild myself. But I did talk with
> Michael off-list, and he suggested the belt-and-suspenders retry counter
> in my second patch.
> 
> The fix is at e8c42cb in Junio's tree, and it's currently merged to
> 'next'.
> 
> -Peff
> 



signature.asc
Description: OpenPGP digital signature

Why does git checkout -b touch the index?

[David Turner]

If I do "git checkout -b fleem", with no additional flags, why does it
need to rewrite the index?  Or even read the index? 

(this is kind of a bug report, I guess, but it's a pretty minor bug that
I only noticed because I was out of hard drive space)

[PATCH v15 02/27] bisect: rewrite `check_term_format` shell function in C

[Pranit Bauva]

Reimplement the `check_term_format` shell function in C and add
a `--check-term-format` subcommand to `git bisect--helper` to call it
from git-bisect.sh

Using `--check-term-format` subcommand is a temporary measure to port
shell function to C so as to use the existing test suite. As more
functions are ported, this subcommand will be retired and its
implementation will be called by some other method/subcommand. For
eg. In conversion of write_terms() of git-bisect.sh, the subcommand will
be removed and instead check_term_format() will be called in its C
implementation while a new subcommand will be introduced for write_terms().

Helped-by: Johannes Schindelein 
Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 60 +++-
 git-bisect.sh| 31 ++---
 2 files changed, 61 insertions(+), 30 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 8111c91..a47f1f2 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -2,19 +2,73 @@
 #include "cache.h"
 #include "parse-options.h"
 #include "bisect.h"
+#include "refs.h"
 
 static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --next-all [--no-checkout]"),
+   N_("git bisect--helper --check-term-format  "),
NULL
 };
 
+/*
+ * Check whether the string `term` belongs to the set of strings
+ * included in the variable arguments.
+ */
+LAST_ARG_MUST_BE_NULL
+static int one_of(const char *term, ...)
+{
+   int res = 0;
+   va_list matches;
+   const char *match;
+
+   va_start(matches, term);
+   while (!res && (match = va_arg(matches, const char *)))
+   res = !strcmp(term, match);
+   va_end(matches);
+
+   return res;
+}
+
+static int check_term_format(const char *term, const char *orig_term)
+{
+   int res;
+   char *new_term = xstrfmt("refs/bisect/%s", term);
+
+   res = check_refname_format(new_term, 0);
+   free(new_term);
+
+   if (res)
+   return error(_("'%s' is not a valid term"), term);
+
+   if (one_of(term, "help", "start", "skip", "next", "reset",
+   "visualize", "replay", "log", "run", NULL))
+   return error(_("can't use the builtin command '%s' as a term"), 
term);
+
+   /*
+* In theory, nothing prevents swapping completely good and bad,
+* but this situation could be confusing and hasn't been tested
+* enough. Forbid it for now.
+*/
+
+   if ((strcmp(orig_term, "bad") && one_of(term, "bad", "new", NULL)) ||
+(strcmp(orig_term, "good") && one_of(term, "good", "old", 
NULL)))
+   return error(_("can't change the meaning of the term '%s'"), 
term);
+
+   return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
-   enum { NEXT_ALL = 1 } cmdmode = 0;
+   enum {
+   NEXT_ALL = 1,
+   CHECK_TERM_FMT
+   } cmdmode = 0;
int no_checkout = 0;
struct option options[] = {
OPT_CMDMODE(0, "next-all", ,
 N_("perform 'git bisect next'"), NEXT_ALL),
+   OPT_CMDMODE(0, "check-term-format", ,
+N_("check format of the term"), CHECK_TERM_FMT),
OPT_BOOL(0, "no-checkout", _checkout,
 N_("update BISECT_HEAD instead of checking out the 
current commit")),
OPT_END()
@@ -29,6 +83,10 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
switch (cmdmode) {
case NEXT_ALL:
return bisect_next_all(prefix, no_checkout);
+   case CHECK_TERM_FMT:
+   if (argc != 2)
+   die(_("--check-term-format requires two arguments"));
+   return check_term_format(argv[0], argv[1]);
default:
die("BUG: unknown subcommand '%d'", cmdmode);
}
diff --git a/git-bisect.sh b/git-bisect.sh
index ae3cb01..a727c59 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -564,38 +564,11 @@ write_terms () {
then
die "$(gettext "please use two different terms")"
fi
-   check_term_format "$TERM_BAD" bad
-   check_term_format "$TERM_GOOD" good
+   git bisect--helper --check-term-format "$TERM_BAD" bad || exit
+   git bisect--helper --check-term-format "$TERM_GOOD" good || exit
printf '%s\n%s\n' "$TERM_BAD" "$TERM_GOOD" >"$GIT_DIR/BISECT_TERMS"
 }
 
-check_term_format () {
-   term=$1
-   git check-ref-format refs/bisect/"$term" ||
-   die "$(eval_gettext "'\$term' is not a valid term")"
-   case "$term" in
-   help|start|terms|skip|next|reset|visualize|replay|log|run)
-   die 

[PATCH v15 16/27] bisect--helper: retire `--bisect-clean-state` subcommand

[Pranit Bauva]

The `bisect-clean-state` subcommand is no longer used in the shell
script while the C code uses `bisect_clean_state()` thus remove the
subcommand.

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 9 -
 1 file changed, 9 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index fcd7574..45d9336 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -22,7 +22,6 @@ static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
 static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --next-all [--no-checkout]"),
N_("git bisect--helper --write-terms  "),
-   N_("git bisect--helper --bisect-clean-state"),
N_("git bisect--helper --bisect-reset []"),
N_("git bisect--helper --bisect-write
 []"),
N_("git bisect--helper --bisect-check-and-set-terms  
 "),
@@ -761,7 +760,6 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
enum {
NEXT_ALL = 1,
WRITE_TERMS,
-   BISECT_CLEAN_STATE,
BISECT_RESET,
CHECK_EXPECTED_REVS,
BISECT_WRITE,
@@ -778,8 +776,6 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 N_("perform 'git bisect next'"), NEXT_ALL),
OPT_CMDMODE(0, "write-terms", ,
 N_("write the terms to .git/BISECT_TERMS"), 
WRITE_TERMS),
-   OPT_CMDMODE(0, "bisect-clean-state", ,
-N_("cleanup the bisection state"), BISECT_CLEAN_STATE),
OPT_CMDMODE(0, "bisect-reset", ,
 N_("reset the bisection state"), BISECT_RESET),
OPT_CMDMODE(0, "check-expected-revs", ,
@@ -820,11 +816,6 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
die(_("--write-terms requires two arguments"));
res = write_terms(argv[0], argv[1]);
break;
-   case BISECT_CLEAN_STATE:
-   if (argc != 0)
-   die(_("--bisect-clean-state requires no arguments"));
-   res = bisect_clean_state();
-   break;
case BISECT_RESET:
if (argc > 1)
die(_("--bisect-reset requires either zero or one 
arguments"));

--
https://github.com/git/git/pull/287

[PATCH v15 26/27] bisect--helper: retire `--bisect-auto-next` subcommand

[Pranit Bauva]

The `--bisect-auto-next` subcommand is no longer used in the shell
script and the function `bisect_auto_next` is called from the C
implementation.

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 10 --
 1 file changed, 10 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 8653fde..d6c2efc 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -27,7 +27,6 @@ static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --bisect start [--term-{old,good}= 
--term-{new,bad}=]"
  "[--no-checkout] [ 
[...]] [--] [...]"),
N_("git bisect--helper --bisect-next"),
-   N_("git bisect--helper --bisect-auto-next"),
N_("git bisect--helper --bisect-state (bad|new) []"),
N_("git bisect--helper --bisect-state (good|old) [...]"),
N_("git bisect--helper --bisect-replay "),
@@ -993,7 +992,6 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
BISECT_TERMS,
BISECT_START,
BISECT_NEXT,
-   BISECT_AUTO_NEXT,
BISECT_STATE,
BISECT_LOG,
BISECT_REPLAY
@@ -1012,8 +1010,6 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 N_("start the bisect session"), BISECT_START),
OPT_CMDMODE(0, "bisect-next", ,
 N_("find the next bisection commit"), BISECT_NEXT),
-   OPT_CMDMODE(0, "bisect-auto-next", ,
-N_("verify the next bisection state then find the next 
bisection state"), BISECT_AUTO_NEXT),
OPT_CMDMODE(0, "bisect-state", ,
 N_("mark the state of ref (or refs)"), BISECT_STATE),
OPT_CMDMODE(0, "bisect-log", ,
@@ -1069,12 +1065,6 @@ int cmd_bisect__helper(int argc, const char **argv, 
const char *prefix)
get_terms();
res = bisect_next(, prefix);
break;
-   case BISECT_AUTO_NEXT:
-   if (argc)
-   die(_("--bisect-auto-next requires 0 arguments"));
-   get_terms();
-   res = bisect_auto_next(, prefix);
-   break;
case BISECT_STATE:
if (argc == 0)
die(_("--bisect-state requires at least 1 argument"));

--
https://github.com/git/git/pull/287

[PATCH v15 03/27] bisect--helper: `write_terms` shell function in C

[Pranit Bauva]

Reimplement the `write_terms` shell function in C and add a `write-terms`
subcommand to `git bisect--helper` to call it from git-bisect.sh . Also
remove the subcommand `--check-term-format` as it can now be called from
inside the function write_terms() C implementation.

Also `|| exit` is added when calling write-terms subcommand from
git-bisect.sh so as to exit whenever there is an error.

Using `--write-terms` subcommand is a temporary measure to port shell
function to C so as to use the existing test suite. As more functions
are ported, this subcommand will be retired and its implementation will
be called by some other method.

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 36 +---
 git-bisect.sh| 22 +++---
 2 files changed, 36 insertions(+), 22 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index a47f1f2..65cf519 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -4,9 +4,11 @@
 #include "bisect.h"
 #include "refs.h"
 
+static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
+
 static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --next-all [--no-checkout]"),
-   N_("git bisect--helper --check-term-format  "),
+   N_("git bisect--helper --write-terms  "),
NULL
 };
 
@@ -57,18 +59,38 @@ static int check_term_format(const char *term, const char 
*orig_term)
return 0;
 }
 
+static int write_terms(const char *bad, const char *good)
+{
+   FILE *fp = NULL;
+   int res;
+
+   if (!strcmp(bad, good))
+   return error(_("please use two different terms"));
+
+   if (check_term_format(bad, "bad") || check_term_format(good, "good"))
+   return -1;
+
+   fp = fopen(git_path_bisect_terms(), "w");
+   if (!fp)
+   return error_errno(_("could not open the file BISECT_TERMS"));
+
+   res = fprintf(fp, "%s\n%s\n", bad, good);
+   res |= fclose(fp);
+   return (res < 0) ? -1 : 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
enum {
NEXT_ALL = 1,
-   CHECK_TERM_FMT
+   WRITE_TERMS
} cmdmode = 0;
int no_checkout = 0;
struct option options[] = {
OPT_CMDMODE(0, "next-all", ,
 N_("perform 'git bisect next'"), NEXT_ALL),
-   OPT_CMDMODE(0, "check-term-format", ,
-N_("check format of the term"), CHECK_TERM_FMT),
+   OPT_CMDMODE(0, "write-terms", ,
+N_("write the terms to .git/BISECT_TERMS"), 
WRITE_TERMS),
OPT_BOOL(0, "no-checkout", _checkout,
 N_("update BISECT_HEAD instead of checking out the 
current commit")),
OPT_END()
@@ -83,10 +105,10 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
switch (cmdmode) {
case NEXT_ALL:
return bisect_next_all(prefix, no_checkout);
-   case CHECK_TERM_FMT:
+   case WRITE_TERMS:
if (argc != 2)
-   die(_("--check-term-format requires two arguments"));
-   return check_term_format(argv[0], argv[1]);
+   die(_("--write-terms requires two arguments"));
+   return write_terms(argv[0], argv[1]);
default:
die("BUG: unknown subcommand '%d'", cmdmode);
}
diff --git a/git-bisect.sh b/git-bisect.sh
index a727c59..9ef6cb8 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -209,7 +209,7 @@ bisect_start() {
eval "$eval true" &&
if test $must_write_terms -eq 1
then
-   write_terms "$TERM_BAD" "$TERM_GOOD"
+   git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD"
fi &&
echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit
#
@@ -557,18 +557,6 @@ get_terms () {
fi
 }
 
-write_terms () {
-   TERM_BAD=$1
-   TERM_GOOD=$2
-   if test "$TERM_BAD" = "$TERM_GOOD"
-   then
-   die "$(gettext "please use two different terms")"
-   fi
-   git bisect--helper --check-term-format "$TERM_BAD" bad || exit
-   git bisect--helper --check-term-format "$TERM_GOOD" good || exit
-   printf '%s\n%s\n' "$TERM_BAD" "$TERM_GOOD" >"$GIT_DIR/BISECT_TERMS"
-}
-
 check_and_set_terms () {
cmd="$1"
case "$cmd" in
@@ -582,13 +570,17 @@ check_and_set_terms () {
bad|good)
if ! test -s "$GIT_DIR/BISECT_TERMS"
then
-   write_terms bad good
+   TERM_BAD=bad
+   TERM_GOOD=good
+   git 

[PATCH v15 24/27] bisect--helper: retire `--bisect-write` subcommand

[Pranit Bauva]

The `--bisect-write` subcommand is no longer used in the shell script
and the function `bisect_write()` is called from the C implementation.

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 13 -
 1 file changed, 13 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index b367d8d..7f676e2 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -21,7 +21,6 @@ static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
 
 static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --bisect-reset []"),
-   N_("git bisect--helper --bisect-write
 []"),
N_("git bisect--helper --bisect-check-and-set-terms  
 "),
N_("git bisect--helper --bisect-next-check []  
 1)
die(_("--bisect-reset requires either zero or one 
arguments"));
res = bisect_reset(argc ? argv[0] : NULL);
break;
-   case BISECT_WRITE:
-   if (argc != 4 && argc != 5)
-   die(_("--bisect-write requires either 4 or 5 
arguments"));
-   nolog = (argc == 5) && !strcmp(argv[4], "nolog");
-   terms.term_good = xstrdup(argv[2]);
-   terms.term_bad = xstrdup(argv[3]);
-   res = bisect_write(argv[0], argv[1], , nolog);
-   break;
case CHECK_AND_SET_TERMS:
if (argc != 3)
die(_("--check-and-set-terms requires 3 arguments"));

--
https://github.com/git/git/pull/287

[PATCH v15 17/27] bisect--helper: retire `--next-all` subcommand

[Pranit Bauva]

The `--next-all` subcommand is no longer used in the shell script and
the function `bisect_next_all()` is called from the C implementation of
`bisect_next()`.

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 8 +---
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 45d9336..502bf18 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -20,7 +20,6 @@ static GIT_PATH_FUNC(git_path_head_name, "head-name")
 static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
 
 static const char * const git_bisect_helper_usage[] = {
-   N_("git bisect--helper --next-all [--no-checkout]"),
N_("git bisect--helper --write-terms  "),
N_("git bisect--helper --bisect-reset []"),
N_("git bisect--helper --bisect-write
 []"),
@@ -758,8 +757,7 @@ static int bisect_start(struct bisect_terms *terms, int 
no_checkout,
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
enum {
-   NEXT_ALL = 1,
-   WRITE_TERMS,
+   WRITE_TERMS = 1,
BISECT_RESET,
CHECK_EXPECTED_REVS,
BISECT_WRITE,
@@ -772,8 +770,6 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
} cmdmode = 0;
int no_checkout = 0, res = 0;
struct option options[] = {
-   OPT_CMDMODE(0, "next-all", ,
-N_("perform 'git bisect next'"), NEXT_ALL),
OPT_CMDMODE(0, "write-terms", ,
 N_("write the terms to .git/BISECT_TERMS"), 
WRITE_TERMS),
OPT_CMDMODE(0, "bisect-reset", ,
@@ -809,8 +805,6 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 
switch (cmdmode) {
int nolog;
-   case NEXT_ALL:
-   return bisect_next_all(prefix, no_checkout);
case WRITE_TERMS:
if (argc != 2)
die(_("--write-terms requires two arguments"));

--
https://github.com/git/git/pull/287

[PATCH v15 19/27] bisect--helper: `bisect_state` & `bisect_head` shell function in C

[Pranit Bauva]

Reimplement the `bisect_state` shell function in C and also add a
subcommand `--bisect-state` to `git-bisect--helper` to call it from
git-bisect.sh .

Using `--bisect-state` subcommand is a temporary measure to port shell
function to C so as to use the existing test suite. As more functions
are ported, this subcommand will be retired and will be called by some
other methods.

`bisect_head` is called from `bisect_state` thus its not required to
introduce another subcommand.

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 86 
 git-bisect.sh| 57 +++-
 2 files changed, 91 insertions(+), 52 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 1767916..1481c6d 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -31,6 +31,8 @@ static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --bisect-next"),
N_("git bisect--helper --bisect-auto-next"),
N_("git bisect--helper --bisect-autostart"),
+   N_("git bisect--helper --bisect-state (bad|new) []"),
+   N_("git bisect--helper --bisect-state (good|old) [...]"),
NULL
 };
 
@@ -784,6 +786,79 @@ static int bisect_autostart(struct bisect_terms *terms)
return 0;
 }
 
+static char *bisect_head(void)
+{
+   if (is_empty_or_missing_file(git_path_bisect_head()))
+   return "HEAD";
+   else
+   return "BISECT_HEAD";
+}
+
+static int bisect_state(struct bisect_terms *terms, const char **argv,
+   int argc)
+{
+   const char *state = argv[0];
+
+   get_terms(terms);
+   if (check_and_set_terms(terms, state))
+   return -1;
+
+   if (!argc)
+   die(_("Please call `--bisect-state` with at least one 
argument"));
+
+   if (argc == 1 && one_of(state, terms->term_good,
+   terms->term_bad, "skip", NULL)) {
+   const char *bisected_head = xstrdup(bisect_head());
+   const char *hex[1];
+   unsigned char sha1[20];
+
+   if (get_sha1(bisected_head, sha1))
+   die(_("Bad rev input: %s"), bisected_head);
+   if (bisect_write(state, sha1_to_hex(sha1), terms, 0))
+   return -1;
+
+   *hex = xstrdup(sha1_to_hex(sha1));
+   if (check_expected_revs(hex, 1))
+   return -1;
+   return bisect_auto_next(terms, NULL);
+   }
+
+   if ((argc == 2 && !strcmp(state, terms->term_bad)) ||
+   one_of(state, terms->term_good, "skip", NULL)) {
+   int i;
+   struct string_list hex = STRING_LIST_INIT_DUP;
+
+   for (i = 1; i < argc; i++) {
+   unsigned char sha1[20];
+
+   if (get_sha1(argv[i], sha1)) {
+   string_list_clear(, 0);
+   die(_("Bad rev input: %s"), argv[i]);
+   }
+   string_list_append(, sha1_to_hex(sha1));
+   }
+   for (i = 0; i < hex.nr; i++) {
+   const char **hex_string = (const char **) 
[i].string;
+   if(bisect_write(state, *hex_string, terms, 0)) {
+   string_list_clear(, 0);
+   return -1;
+   }
+   if (check_expected_revs(hex_string, 1)) {
+   string_list_clear(, 0);
+   return -1;
+   }
+   }
+   string_list_clear(, 0);
+   return bisect_auto_next(terms, NULL);
+   }
+
+   if (!strcmp(state, terms->term_bad))
+   die(_("'git bisect %s' can take only one argument."),
+ terms->term_bad);
+
+   return -1;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
enum {
@@ -798,6 +873,7 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
BISECT_NEXT,
BISECT_AUTO_NEXT,
BISECT_AUTOSTART,
+   BISECT_STATE
} cmdmode = 0;
int no_checkout = 0, res = 0;
struct option options[] = {
@@ -823,6 +899,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 N_("verify the next bisection state then find the next 
bisection state"), BISECT_AUTO_NEXT),
OPT_CMDMODE(0, "bisect-autostart", ,
 N_("start the bisection if BISECT_START empty or 
missing"), BISECT_AUTOSTART),
+   OPT_CMDMODE(0, "bisect-state", ,
+N_("mark the state of ref 

[PATCH v15 18/27] bisect--helper: `bisect_autostart` shell function in C

[Pranit Bauva]

Reimplement the `bisect_autostart` shell function in C and add the
C implementation from `bisect_next()` which was previously left
uncovered. Also add a subcommand `--bisect-autostart` to
`git bisect--helper` be called from `bisect_state()` from
git-bisect.sh .

Using `--bisect-autostart` subcommand is a temporary measure to port
shell function to C so as to use the existing test suite. As more
functions are ported, this subcommand will be retired and will be called
by `bisect_state()`.

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 40 
 git-bisect.sh| 23 +--
 2 files changed, 41 insertions(+), 22 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 502bf18..1767916 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -30,6 +30,7 @@ static const char * const git_bisect_helper_usage[] = {
  "[--no-checkout] [ 
[...]] [--] [...]"),
N_("git bisect--helper --bisect-next"),
N_("git bisect--helper --bisect-auto-next"),
+   N_("git bisect--helper --bisect-autostart"),
NULL
 };
 
@@ -38,6 +39,8 @@ struct bisect_terms {
const char *term_bad;
 };
 
+static int bisect_autostart(struct bisect_terms *terms);
+
 /*
  * Check whether the string `term` belongs to the set of strings
  * included in the variable arguments.
@@ -422,6 +425,7 @@ static int bisect_next(struct bisect_terms *terms, const 
char *prefix)
 {
int res, no_checkout;
 
+   bisect_autostart(terms);
/*
 * In case of mistaken revs or checkout error, or signals received,
 * "bisect_auto_next" below may exit or misbehave.
@@ -754,6 +758,32 @@ static int bisect_start(struct bisect_terms *terms, int 
no_checkout,
return retval || bisect_auto_next(terms, NULL);
 }
 
+static int bisect_autostart(struct bisect_terms *terms)
+{
+   if (is_empty_or_missing_file(git_path_bisect_start())) {
+   const char *yesno;
+   const char *argv[] = {NULL};
+   fprintf(stderr, _("You need to start by \"git bisect "
+ "start\"\n"));
+
+   if (!isatty(0))
+   return 1;
+
+   /*
+* TRANSLATORS: Make sure to include [Y] and [n] in your
+* translation. THe program will only accept English input
+* at this point.
+*/
+   yesno = git_prompt(_("Do you want me to do it for you "
+"[Y/n]? "), PROMPT_ECHO);
+   if (starts_with(yesno, "n") || starts_with(yesno, "N"))
+   exit(0);
+
+   return bisect_start(terms, 0, argv, 0);
+   }
+   return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
enum {
@@ -767,6 +797,7 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
BISECT_START,
BISECT_NEXT,
BISECT_AUTO_NEXT,
+   BISECT_AUTOSTART,
} cmdmode = 0;
int no_checkout = 0, res = 0;
struct option options[] = {
@@ -790,6 +821,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 N_("find the next bisection commit"), BISECT_NEXT),
OPT_CMDMODE(0, "bisect-auto-next", ,
 N_("verify the next bisection state then find the next 
bisection state"), BISECT_AUTO_NEXT),
+   OPT_CMDMODE(0, "bisect-autostart", ,
+N_("start the bisection if BISECT_START empty or 
missing"), BISECT_AUTOSTART),
OPT_BOOL(0, "no-checkout", _checkout,
 N_("update BISECT_HEAD instead of checking out the 
current commit")),
OPT_END()
@@ -862,6 +895,13 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
get_terms();
res = bisect_auto_next(, prefix);
break;
+   case BISECT_AUTOSTART:
+   if (argc)
+   die(_("--bisect-autostart requires 0 arguments"));
+   terms.term_good = "good";
+   terms.term_bad = "bad";
+   res = bisect_autostart();
+   break;
default:
die("BUG: unknown subcommand '%d'", cmdmode);
}
diff --git a/git-bisect.sh b/git-bisect.sh
index d574c44..cd56551 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -49,27 +49,6 @@ bisect_head()
fi
 }
 
-bisect_autostart() {
-   test -s "$GIT_DIR/BISECT_START" || {
-   gettextln "You need to start by \"git bisect start\"" >&2
-   if test -t 0
-   then
-  

[PATCH v15 12/27] bisect--helper: `get_terms` & `bisect_terms` shell function in C

[Pranit Bauva]

Reimplement the `get_terms` and `bisect_terms` shell function in C and
add `bisect-terms` subcommand to `git bisect--helper` to call it from
git-bisect.sh .

Using `--bisect-terms` subcommand is a temporary measure to port shell
function in C so as to use the existing test suite. As more functions
are ported, this subcommand will be retired but its implementation will
be called by some other methods.

Also use error() to report "no terms defined" and accordingly change the
test in t6030.

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c| 72 +++--
 git-bisect.sh   | 35 ++
 t/t6030-bisect-porcelain.sh |  2 +-
 3 files changed, 73 insertions(+), 36 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 317d671..6a5878c 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -23,6 +23,7 @@ static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --bisect-write
 []"),
N_("git bisect--helper --bisect-check-and-set-terms  
 "),
N_("git bisect--helper --bisect-next-check []  
term_bad = strbuf_detach(, NULL);
+   strbuf_getline_lf(, fp);
+   terms->term_good = strbuf_detach(, NULL);
+   goto finish;
+finish:
+   if (fp)
+   fclose(fp);
+   strbuf_release();
+   return res;
+}
+
+static int bisect_terms(struct bisect_terms *terms, const char **argv, int 
argc)
+{
+   int i;
+   const char bisect_term_usage[] =
+"git bisect--helper --bisect-terms [--term-good | --term-bad | ]"
+"--term-old | --term-new";
+
+   if (get_terms(terms))
+   return error(_("no terms defined"));
+
+   if (argc > 1) {
+   usage(bisect_term_usage);
+   return -1;
+   }
+
+   if (argc == 0) {
+   printf(_("Your current terms are %s for the old state\nand "
+  "%s for the new state.\n"), terms->term_good,
+  terms->term_bad);
+   return 0;
+   }
+
+   for (i = 0; i < argc; i++) {
+   if (!strcmp(argv[i], "--term-good"))
+   printf("%s\n", terms->term_good);
+   else if (!strcmp(argv[i], "--term-bad"))
+   printf("%s\n", terms->term_bad);
+   else
+   die(_("invalid argument %s for 'git bisect "
+ "terms'.\nSupported options are: "
+ "--term-good|--term-old and "
+ "--term-bad|--term-new."), argv[i]);
+   }
+
+   return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
enum {
@@ -353,7 +413,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
CHECK_EXPECTED_REVS,
BISECT_WRITE,
CHECK_AND_SET_TERMS,
-   BISECT_NEXT_CHECK
+   BISECT_NEXT_CHECK,
+   BISECT_TERMS
} cmdmode = 0;
int no_checkout = 0, res = 0;
struct option options[] = {
@@ -373,6 +434,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 N_("check and set terms in a bisection state"), 
CHECK_AND_SET_TERMS),
OPT_CMDMODE(0, "bisect-next-check", ,
 N_("check whether bad or good terms exist"), 
BISECT_NEXT_CHECK),
+   OPT_CMDMODE(0, "bisect-terms", ,
+N_("print out the bisect terms"), BISECT_TERMS),
OPT_BOOL(0, "no-checkout", _checkout,
 N_("update BISECT_HEAD instead of checking out the 
current commit")),
OPT_END()
@@ -380,7 +443,7 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
struct bisect_terms terms;
 
argc = parse_options(argc, argv, prefix, options,
-git_bisect_helper_usage, 0);
+git_bisect_helper_usage, PARSE_OPT_KEEP_UNKNOWN);
 
if (!cmdmode)
usage_with_options(git_bisect_helper_usage, options);
@@ -429,6 +492,11 @@ int cmd_bisect__helper(int argc, const char **argv, const 

[PATCH v15 20/27] bisect--helper: retire `--check-expected-revs` subcommand

[Pranit Bauva]

The `--check-expected-revs` subcommand is no longer used in the shell
script and the function `check_expected_revs()` is called from the C
implementation of `bisect_next()`.

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 1481c6d..d5fe35b 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -864,7 +864,6 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
enum {
WRITE_TERMS = 1,
BISECT_RESET,
-   CHECK_EXPECTED_REVS,
BISECT_WRITE,
CHECK_AND_SET_TERMS,
BISECT_NEXT_CHECK,
@@ -881,8 +880,6 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 N_("write the terms to .git/BISECT_TERMS"), 
WRITE_TERMS),
OPT_CMDMODE(0, "bisect-reset", ,
 N_("reset the bisection state"), BISECT_RESET),
-   OPT_CMDMODE(0, "check-expected-revs", ,
-N_("check for expected revs"), CHECK_EXPECTED_REVS),
OPT_CMDMODE(0, "bisect-write", ,
 N_("write out the bisection state in BISECT_LOG"), 
BISECT_WRITE),
OPT_CMDMODE(0, "check-and-set-terms", ,
@@ -926,9 +923,6 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
die(_("--bisect-reset requires either zero or one 
arguments"));
res = bisect_reset(argc ? argv[0] : NULL);
break;
-   case CHECK_EXPECTED_REVS:
-   res = check_expected_revs(argv, argc);
-   break;
case BISECT_WRITE:
if (argc != 4 && argc != 5)
die(_("--bisect-write requires either 4 or 5 
arguments"));

--
https://github.com/git/git/pull/287

[PATCH v15 14/27] t6030: no cleanup with bad merge base

[Pranit Bauva]

The bisection cleanup should be performed with bad merge base so that
the user can return to its original position with `git bisect reset`.

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 t/t6030-bisect-porcelain.sh | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index e62e2a8..8ac77ee 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -911,4 +911,11 @@ test_expect_success 'git bisect reset cleans bisection 
state properly' '
test_path_is_missing "$GIT_DIR/BISECT_START"
 '
 
+test_expect_success 'check whether bisection cleanup is not done with bad 
merges' '
+   git bisect start $HASH7 $SIDE_HASH7 &&
+   test_expect_failure git bisect bad >out 2>out &&
+   test_i18ngrep "The merge base" out &&
+   test -e .git/BISECT_START
+'
+
 test_done

--
https://github.com/git/git/pull/287

[PATCH v15 01/27] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Pranit Bauva]

`--next-all` is meant to be used as a subcommand to support multiple
"operation mode" though the current implementation does not contain any
other subcommand along side with `--next-all` but further commits will
include some more subcommands.

Helped-by: Johannes Schindelin 
Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 17 +++--
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 3324229..8111c91 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -10,11 +10,11 @@ static const char * const git_bisect_helper_usage[] = {
 
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
-   int next_all = 0;
+   enum { NEXT_ALL = 1 } cmdmode = 0;
int no_checkout = 0;
struct option options[] = {
-   OPT_BOOL(0, "next-all", _all,
-N_("perform 'git bisect next'")),
+   OPT_CMDMODE(0, "next-all", ,
+N_("perform 'git bisect next'"), NEXT_ALL),
OPT_BOOL(0, "no-checkout", _checkout,
 N_("update BISECT_HEAD instead of checking out the 
current commit")),
OPT_END()
@@ -23,9 +23,14 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
argc = parse_options(argc, argv, prefix, options,
 git_bisect_helper_usage, 0);
 
-   if (!next_all)
+   if (!cmdmode)
usage_with_options(git_bisect_helper_usage, options);
 
-   /* next-all */
-   return bisect_next_all(prefix, no_checkout);
+   switch (cmdmode) {
+   case NEXT_ALL:
+   return bisect_next_all(prefix, no_checkout);
+   default:
+   die("BUG: unknown subcommand '%d'", cmdmode);
+   }
+   return 0;
 }

--
https://github.com/git/git/pull/287

[PATCH v15 10/27] bisect--helper: `check_and_set_terms` shell function in C

[Pranit Bauva]

Reimplement the `check_and_set_terms` shell function in C and add
`check-and-set-terms` subcommand to `git bisect--helper` to call it from
git-bisect.sh

Using `--check-and-set-terms` subcommand is a temporary measure to port
shell function in C so as to use the existing test suite. As more
functions are ported, this subcommand will be retired but its
implementation will be called by some other methods.

check_and_set_terms() sets and receives two global variables namely
TERM_GOOD and TERM_BAD in the shell script. Luckily the file BISECT_TERMS
also contains the value of those variables so its appropriate to evoke the
method get_terms() after calling the subcommand so that it retrieves the
value of TERM_GOOD and TERM_BAD from the file BISECT_TERMS. The two
global variables are passed as arguments to the subcommand.

Also introduce set_terms() to copy the `term_good` and `term_bad` into
`struct bisect_terms` and write it out to the file BISECT_TERMS.

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 45 -
 git-bisect.sh| 36 
 2 files changed, 48 insertions(+), 33 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 3f19b68..c6c11e3 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -20,6 +20,7 @@ static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --bisect-clean-state"),
N_("git bisect--helper --bisect-reset []"),
N_("git bisect--helper --bisect-write
 []"),
+   N_("git bisect--helper --bisect-check-and-set-terms  
 "),
NULL
 };
 
@@ -212,6 +213,38 @@ static int bisect_write(const char *state, const char *rev,
return retval;
 }
 
+static int set_terms(struct bisect_terms *terms, const char *bad,
+const char *good)
+{
+   terms->term_good = xstrdup(good);
+   terms->term_bad = xstrdup(bad);
+   return write_terms(terms->term_bad, terms->term_good);
+}
+
+static int check_and_set_terms(struct bisect_terms *terms, const char *cmd)
+{
+   int has_term_file = !is_empty_or_missing_file(git_path_bisect_terms());
+
+   if (one_of(cmd, "skip", "start", "terms", NULL))
+   return 0;
+
+   if (has_term_file &&
+   strcmp(cmd, terms->term_bad) &&
+   strcmp(cmd, terms->term_good))
+   return error(_("Invalid command: you're currently in a "
+   "%s/%s bisect"), terms->term_bad,
+   terms->term_good);
+
+   if (!has_term_file) {
+   if (one_of(cmd, "bad", "good", NULL))
+   return set_terms(terms, "bad", "good");
+   if (one_of(cmd, "new", "old", NULL))
+   return set_terms(terms, "new", "old");
+   }
+
+   return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
enum {
@@ -220,7 +253,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
BISECT_CLEAN_STATE,
BISECT_RESET,
CHECK_EXPECTED_REVS,
-   BISECT_WRITE
+   BISECT_WRITE,
+   CHECK_AND_SET_TERMS
} cmdmode = 0;
int no_checkout = 0, res = 0;
struct option options[] = {
@@ -236,6 +270,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 N_("check for expected revs"), CHECK_EXPECTED_REVS),
OPT_CMDMODE(0, "bisect-write", ,
 N_("write out the bisection state in BISECT_LOG"), 
BISECT_WRITE),
+   OPT_CMDMODE(0, "check-and-set-terms", ,
+N_("check and set terms in a bisection state"), 
CHECK_AND_SET_TERMS),
OPT_BOOL(0, "no-checkout", _checkout,
 N_("update BISECT_HEAD instead of checking out the 
current commit")),
OPT_END()
@@ -278,6 +314,13 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
terms.term_bad = xstrdup(argv[3]);
res = bisect_write(argv[0], argv[1], , nolog);
break;
+   case CHECK_AND_SET_TERMS:
+   if (argc != 3)
+   die(_("--check-and-set-terms requires 3 arguments"));
+   terms.term_good = xstrdup(argv[1]);
+   terms.term_bad = xstrdup(argv[2]);
+   res = check_and_set_terms(, argv[0]);
+   break;
default:
die("BUG: unknown subcommand '%d'", cmdmode);
}
diff --git a/git-bisect.sh b/git-bisect.sh
index dfdec33..bdf2227 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -238,7 +238,8 @@ bisect_skip() {
 bisect_state() {
bisect_autostart

[PATCH v15 09/27] bisect--helper: `bisect_write` shell function in C

[Pranit Bauva]

Reimplement the `bisect_write` shell function in C and add a
`bisect-write` subcommand to `git bisect--helper` to call it from
git-bisect.sh

Using `--bisect-write` subcommand is a temporary measure to port shell
function in C so as to use the existing test suite. As more functions
are ported, this subcommand will be retired but its implementation will
be called by some other methods.

Note: bisect_write() uses two variables namely TERM_GOOD and TERM_BAD
from the global shell script thus we need to pass it to the subcommand
using the arguments. We then store them in a struct bisect_terms and
pass the memory address around functions.

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 94 
 git-bisect.sh| 25 +++--
 2 files changed, 91 insertions(+), 28 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index c542e8b..3f19b68 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -19,9 +19,15 @@ static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --write-terms  "),
N_("git bisect--helper --bisect-clean-state"),
N_("git bisect--helper --bisect-reset []"),
+   N_("git bisect--helper --bisect-write
 []"),
NULL
 };
 
+struct bisect_terms {
+   const char *term_good;
+   const char *term_bad;
+};
+
 /*
  * Check whether the string `term` belongs to the set of strings
  * included in the variable arguments.
@@ -149,6 +155,63 @@ static int check_expected_revs(const char **revs, int 
rev_nr)
return 0;
 }
 
+static int bisect_write(const char *state, const char *rev,
+   const struct bisect_terms *terms, int nolog)
+{
+   struct strbuf tag = STRBUF_INIT;
+   struct strbuf commit_name = STRBUF_INIT;
+   struct object_id oid;
+   struct commit *commit;
+   struct pretty_print_context pp = {0};
+   FILE *fp = NULL;
+   int retval = 0;
+
+   if (!strcmp(state, terms->term_bad))
+   strbuf_addf(, "refs/bisect/%s", state);
+   else if (one_of(state, terms->term_good, "skip", NULL))
+   strbuf_addf(, "refs/bisect/%s-%s", state, rev);
+   else {
+   error(_("Bad bisect_write argument: %s"), state);
+   retval = -1;
+   goto finish;
+   }
+
+   if (get_oid(rev, )) {
+   error(_("couldn't get the oid of the rev '%s'"), rev);
+   retval = -1;
+   goto finish;
+   }
+
+   if (update_ref(NULL, tag.buf, oid.hash, NULL, 0,
+  UPDATE_REFS_MSG_ON_ERR)) {
+   retval = -1;
+   goto finish;
+   }
+
+   fp = fopen(git_path_bisect_log(), "a");
+   if (!fp) {
+   error_errno(_("couldn't open the file '%s'"), 
git_path_bisect_log());
+   retval = -1;
+   goto finish;
+   }
+
+   commit = lookup_commit_reference(oid.hash);
+   format_commit_message(commit, "%s", _name, );
+   fprintf(fp, "# %s: [%s] %s\n", state, sha1_to_hex(oid.hash),
+   commit_name.buf);
+
+   if (!nolog)
+   fprintf(fp, "git bisect %s %s\n", state, rev);
+
+   goto finish;
+finish:
+   if (fp)
+   fclose(fp);
+   strbuf_release();
+   strbuf_release(_name);
+   return retval;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
enum {
@@ -156,9 +219,10 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
WRITE_TERMS,
BISECT_CLEAN_STATE,
BISECT_RESET,
-   CHECK_EXPECTED_REVS
+   CHECK_EXPECTED_REVS,
+   BISECT_WRITE
} cmdmode = 0;
-   int no_checkout = 0;
+   int no_checkout = 0, res = 0;
struct option options[] = {
OPT_CMDMODE(0, "next-all", ,
 N_("perform 'git bisect next'"), NEXT_ALL),
@@ -170,10 +234,13 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 N_("reset the bisection state"), BISECT_RESET),
OPT_CMDMODE(0, "check-expected-revs", ,
 N_("check for expected revs"), CHECK_EXPECTED_REVS),
+   OPT_CMDMODE(0, "bisect-write", ,
+N_("write out the bisection state in BISECT_LOG"), 
BISECT_WRITE),
OPT_BOOL(0, "no-checkout", _checkout,
 N_("update BISECT_HEAD instead of checking out the 
current commit")),
OPT_END()
};
+   struct bisect_terms terms;
 
argc = parse_options(argc, argv, prefix, options,
 git_bisect_helper_usage, 0);
@@ -182,24 +249,37 @@ int 

[PATCH v15 13/27] bisect--helper: `bisect_start` shell function partially in C

[Pranit Bauva]

Reimplement the `bisect_start` shell function partially in C and add
`bisect-start` subcommand to `git bisect--helper` to call it from
git-bisect.sh .

The last part is not converted because it calls another shell function
`bisect_start` shell function will be completed after the `bisect_next`
shell function is ported in C.

Using `--bisect-start` subcommand is a temporary measure to port shell
function in C so as to use the existing test suite. As more functions
are ported, this subcommand will be retired and will be called by some
other methods.

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 217 ++-
 git-bisect.sh| 133 +
 2 files changed, 217 insertions(+), 133 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 6a5878c..1d3e17f 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -7,6 +7,7 @@
 #include "argv-array.h"
 #include "run-command.h"
 #include "prompt.h"
+#include "quote.h"
 
 static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
 static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
@@ -14,6 +15,8 @@ static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, 
"BISECT_ANCESTORS_OK")
 static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
 static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
 static GIT_PATH_FUNC(git_path_bisect_head, "BISECT_HEAD")
+static GIT_PATH_FUNC(git_path_head_name, "head-name")
+static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
 
 static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --next-all [--no-checkout]"),
@@ -24,6 +27,8 @@ static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --bisect-check-and-set-terms  
 "),
N_("git bisect--helper --bisect-next-check []  
term_good = xstrdup(argv[++i]);
+   } else if (skip_prefix(arg, "--term-good=", )) {
+   must_write_terms = 1;
+   terms->term_good = xstrdup(arg);
+   } else if (skip_prefix(arg, "--term-old=", )) {
+   must_write_terms = 1;
+   terms->term_good = xstrdup(arg);
+   } else if (!strcmp(arg, "--term-bad") ||
+!strcmp(arg, "--term-new")) {
+   must_write_terms = 1;
+   terms->term_bad = xstrdup(argv[++i]);
+   } else if (skip_prefix(arg, "--term-bad=", )) {
+   must_write_terms = 1;
+   terms->term_bad = xstrdup(arg);
+   } else if (skip_prefix(arg, "--term-new=", )) {
+   must_write_terms = 1;
+   terms->term_good = xstrdup(arg);
+   } else if (starts_with(arg, "--") &&
+!one_of(arg, "--term-good", "--term-bad", NULL)) {
+   die(_("unrecognised 

[PATCH v15 05/27] t6030: explicitly test for bisection cleanup

[Pranit Bauva]

Add test to explicitly check that 'git bisect reset' is working as
expected. This is already covered implicitly by the test suite.

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 

---
I faced this problem while converting `bisect_clean_state` and the tests
where showing breakages but it wasn't clear as to where exactly are they
breaking. This will patch  will help in that. Also I tested the test
coverage of the test suite before this patch and it covers this (I did
this by purposely changing names of files in git-bisect.sh and running
the test suite).
---
 t/t6030-bisect-porcelain.sh | 17 +
 1 file changed, 17 insertions(+)

diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 5e5370f..18e7998 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -894,4 +894,21 @@ test_expect_success 'bisect start takes options and revs 
in any order' '
test_cmp expected actual
 '
 
+test_expect_success 'git bisect reset cleans bisection state properly' '
+   git bisect reset &&
+   git bisect start &&
+   git bisect good $HASH1 &&
+   git bisect bad $HASH4 &&
+   git bisect reset &&
+   test -z "$(git for-each-ref "refs/bisect/*")" &&
+   test_path_is_missing "$GIT_DIR/BISECT_EXPECTED_REV" &&
+   test_path_is_missing "$GIT_DIR/BISECT_ANCESTORS_OK" &&
+   test_path_is_missing "$GIT_DIR/BISECT_LOG" &&
+   test_path_is_missing "$GIT_DIR/BISECT_RUN" &&
+   test_path_is_missing "$GIT_DIR/BISECT_TERMS" &&
+   test_path_is_missing "$GIT_DIR/head-name" &&
+   test_path_is_missing "$GIT_DIR/BISECT_HEAD" &&
+   test_path_is_missing "$GIT_DIR/BISECT_START"
+'
+
 test_done

--
https://github.com/git/git/pull/287

[PATCH v15 06/27] wrapper: move is_empty_file() and rename it as is_empty_or_missing_file()

[Pranit Bauva]

is_empty_file() can help to refactor a lot of code. This will be very
helpful in porting "git bisect" to C.

Suggested-by: Torsten Bögershausen 
Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/am.c | 20 ++--
 cache.h  |  3 +++
 wrapper.c| 13 +
 3 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 739b34d..9e1e9d6 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -30,22 +30,6 @@
 #include "mailinfo.h"
 
 /**
- * Returns 1 if the file is empty or does not exist, 0 otherwise.
- */
-static int is_empty_file(const char *filename)
-{
-   struct stat st;
-
-   if (stat(filename, ) < 0) {
-   if (errno == ENOENT)
-   return 1;
-   die_errno(_("could not stat %s"), filename);
-   }
-
-   return !st.st_size;
-}
-
-/**
  * Returns the length of the first line of msg.
  */
 static int linelen(const char *msg)
@@ -1324,7 +1308,7 @@ static int parse_mail(struct am_state *state, const char 
*mail)
goto finish;
}
 
-   if (is_empty_file(am_path(state, "patch"))) {
+   if (is_empty_or_missing_file(am_path(state, "patch"))) {
printf_ln(_("Patch is empty. Was it split wrong?"));
die_user_resolve(state);
}
@@ -1896,7 +1880,7 @@ static void am_run(struct am_state *state, int resume)
resume = 0;
}
 
-   if (!is_empty_file(am_path(state, "rewritten"))) {
+   if (!is_empty_or_missing_file(am_path(state, "rewritten"))) {
assert(state->rebasing);
copy_notes_for_rebase(state);
run_post_rewrite_hook(state);
diff --git a/cache.h b/cache.h
index b780a91..49f214b 100644
--- a/cache.h
+++ b/cache.h
@@ -1916,4 +1916,7 @@ void sleep_millisec(int millisec);
  */
 void safe_create_dir(const char *dir, int share);
 
+/* Return 1 if the file is empty or does not exists, 0 otherwise. */
+extern int is_empty_or_missing_file(const char *filename);
+
 #endif /* CACHE_H */
diff --git a/wrapper.c b/wrapper.c
index e7f1979..78f6431 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -679,3 +679,16 @@ void sleep_millisec(int millisec)
 {
poll(NULL, 0, millisec);
 }
+
+int is_empty_or_missing_file(const char *filename)
+{
+   struct stat st;
+
+   if (stat(filename, ) < 0) {
+   if (errno == ENOENT)
+   return 1;
+   die_errno(_("could not stat %s"), filename);
+   }
+
+   return !st.st_size;
+}

--
https://github.com/git/git/pull/287

[PATCH v15 04/27] bisect--helper: `bisect_clean_state` shell function in C

[Pranit Bauva]

Reimplement `bisect_clean_state` shell function in C and add a
`bisect-clean-state` subcommand to `git bisect--helper` to call it from
git-bisect.sh .

Using `--bisect-clean-state` subcommand is a measure to port shell
function to C so as to use the existing test suite. As more functions
are ported, this subcommand will be retired but its implementation  will
be called by bisect_reset() and bisect_start().

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 bisect.c | 43 +++
 bisect.h |  2 ++
 builtin/bisect--helper.c | 14 +-
 git-bisect.sh| 26 +++---
 4 files changed, 61 insertions(+), 24 deletions(-)

diff --git a/bisect.c b/bisect.c
index 6f512c2..45d598d 100644
--- a/bisect.c
+++ b/bisect.c
@@ -430,6 +430,12 @@ static int read_bisect_refs(void)
 
 static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
 static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
+static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
+static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
+static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
+static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
+static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
+static GIT_PATH_FUNC(git_path_head_name, "head-name")
 
 static void read_bisect_paths(struct argv_array *array)
 {
@@ -1040,3 +1046,40 @@ int estimate_bisect_steps(int all)
 
return (e < 3 * x) ? n : n - 1;
 }
+
+static int mark_for_removal(const char *refname, const struct object_id *oid,
+   int flag, void *cb_data)
+{
+   struct string_list *refs = cb_data;
+   char *ref = xstrfmt("refs/bisect%s", refname);
+   string_list_append(refs, ref);
+   return 0;
+}
+
+int bisect_clean_state(void)
+{
+   int result = 0;
+
+   /* There may be some refs packed during bisection */
+   struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
+   for_each_ref_in("refs/bisect", mark_for_removal, (void *) 
_for_removal);
+   string_list_append(_for_removal, xstrdup("BISECT_HEAD"));
+   result = delete_refs(_for_removal, REF_NODEREF);
+   refs_for_removal.strdup_strings = 1;
+   string_list_clear(_for_removal, 0);
+   unlink_or_warn(git_path_bisect_expected_rev());
+   unlink_or_warn(git_path_bisect_ancestors_ok());
+   unlink_or_warn(git_path_bisect_log());
+   unlink_or_warn(git_path_bisect_names());
+   unlink_or_warn(git_path_bisect_run());
+   unlink_or_warn(git_path_bisect_terms());
+   /* Cleanup head-name if it got left by an old version of git-bisect */
+   unlink_or_warn(git_path_head_name());
+   /*
+* Cleanup BISECT_START last to support the --no-checkout option
+* introduced in the commit 4796e823a.
+*/
+   unlink_or_warn(git_path_bisect_start());
+
+   return result;
+}
diff --git a/bisect.h b/bisect.h
index acd12ef..0ae63d4 100644
--- a/bisect.h
+++ b/bisect.h
@@ -28,4 +28,6 @@ extern int estimate_bisect_steps(int all);
 
 extern void read_bisect_terms(const char **bad, const char **good);
 
+extern int bisect_clean_state(void);
+
 #endif
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 65cf519..4254d61 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -5,10 +5,15 @@
 #include "refs.h"
 
 static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
+static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
+static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
+static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
+static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
 
 static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --next-all [--no-checkout]"),
N_("git bisect--helper --write-terms  "),
+   N_("git bisect--helper --bisect-clean-state"),
NULL
 };
 
@@ -83,7 +88,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 {
enum {
NEXT_ALL = 1,
-   WRITE_TERMS
+   WRITE_TERMS,
+   BISECT_CLEAN_STATE
} cmdmode = 0;
int no_checkout = 0;
struct option options[] = {
@@ -91,6 +97,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 N_("perform 'git bisect next'"), NEXT_ALL),
OPT_CMDMODE(0, "write-terms", ,
 N_("write the terms to .git/BISECT_TERMS"), 
WRITE_TERMS),
+   OPT_CMDMODE(0, "bisect-clean-state", ,
+N_("cleanup the bisection state"), BISECT_CLEAN_STATE),
OPT_BOOL(0, "no-checkout", _checkout,
 N_("update BISECT_HEAD instead of 

[PATCH v15 07/27] bisect--helper: `bisect_reset` shell function in C

[Pranit Bauva]

Reimplement `bisect_reset` shell function in C and add a `--bisect-reset`
subcommand to `git bisect--helper` to call it from git-bisect.sh .

Using `bisect_reset` subcommand is a temporary measure to port shell
functions to C so as to use the existing test suite. As more functions
are ported, this subcommand would be retired but its implementation will
be called by some other method.

Note: --bisect-clean-state subcommand has not been retired as there are
still a function namely `bisect_start()` which still uses this
subcommand.

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 48 +++-
 git-bisect.sh| 28 ++--
 2 files changed, 49 insertions(+), 27 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 4254d61..d84ba86 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -3,17 +3,22 @@
 #include "parse-options.h"
 #include "bisect.h"
 #include "refs.h"
+#include "dir.h"
+#include "argv-array.h"
+#include "run-command.h"
 
 static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
 static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
 static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
 static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
 static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
+static GIT_PATH_FUNC(git_path_bisect_head, "BISECT_HEAD")
 
 static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --next-all [--no-checkout]"),
N_("git bisect--helper --write-terms  "),
N_("git bisect--helper --bisect-clean-state"),
+   N_("git bisect--helper --bisect-reset []"),
NULL
 };
 
@@ -84,12 +89,47 @@ static int write_terms(const char *bad, const char *good)
return (res < 0) ? -1 : 0;
 }
 
+static int bisect_reset(const char *commit)
+{
+   struct strbuf branch = STRBUF_INIT;
+
+   if (!commit) {
+   if (strbuf_read_file(, git_path_bisect_start(), 0) < 1) {
+   printf("We are not bisecting.\n");
+   return 0;
+   }
+   strbuf_rtrim();
+   } else {
+   unsigned char sha1[20];
+   if (get_sha1_committish(commit, sha1))
+   return error(_("'%s' is not a valid commit"), commit);
+   strbuf_addstr(, commit);
+   }
+
+   if (!file_exists(git_path_bisect_head())) {
+   struct argv_array argv = ARGV_ARRAY_INIT;
+   argv_array_pushl(, "checkout", branch.buf, "--", NULL);
+   if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
+   error(_("Could not check out original HEAD '%s'. Try "
+   "'git bisect reset '."), branch.buf);
+   strbuf_release();
+   argv_array_clear();
+   return -1;
+   }
+   argv_array_clear();
+   }
+
+   strbuf_release();
+   return bisect_clean_state();
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
enum {
NEXT_ALL = 1,
WRITE_TERMS,
-   BISECT_CLEAN_STATE
+   BISECT_CLEAN_STATE,
+   BISECT_RESET
} cmdmode = 0;
int no_checkout = 0;
struct option options[] = {
@@ -99,6 +139,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 N_("write the terms to .git/BISECT_TERMS"), 
WRITE_TERMS),
OPT_CMDMODE(0, "bisect-clean-state", ,
 N_("cleanup the bisection state"), BISECT_CLEAN_STATE),
+   OPT_CMDMODE(0, "bisect-reset", ,
+N_("reset the bisection state"), BISECT_RESET),
OPT_BOOL(0, "no-checkout", _checkout,
 N_("update BISECT_HEAD instead of checking out the 
current commit")),
OPT_END()
@@ -121,6 +163,10 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
if (argc != 0)
die(_("--bisect-clean-state requires no arguments"));
return bisect_clean_state();
+   case BISECT_RESET:
+   if (argc > 1)
+   die(_("--bisect-reset requires either zero or one 
arguments"));
+   return bisect_reset(argc ? argv[0] : NULL);
default:
die("BUG: unknown subcommand '%d'", cmdmode);
}
diff --git a/git-bisect.sh b/git-bisect.sh
index f1202df..442397b 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -409,35 +409,11 @@ bisect_visualize() {
eval '"$@"' --bisect -- $(cat "$GIT_DIR/BISECT_NAMES")
 }
 
-bisect_reset() {
-   test -s 

[PATCH v15 22/27] bisect--helper: `bisect_log` shell function in C

[Pranit Bauva]

Reimplement the `bisect_log` shell function in C and also add
`--bisect-log` subcommand to `git bisect--helper` to call it from
git-bisect.sh .

Using `--bisect-log` subcommand is a temporary measure to port shell
function to C so as to use the existing test suite. As more functions
are ported, this subcommand will be retired and will be called by some
other method.

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 27 ++-
 git-bisect.sh|  7 +--
 2 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 493034c..c18ca07 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -858,6 +858,23 @@ static int bisect_state(struct bisect_terms *terms, const 
char **argv,
return -1;
 }
 
+static int bisect_log(void)
+{
+   int fd, status;
+   fd = open(git_path_bisect_log(), O_RDONLY);
+   if (fd < 0)
+   return -1;
+
+   status = copy_fd(fd, 1);
+   if (status) {
+   close(fd);
+   return -1;
+   }
+
+   close(fd);
+   return status;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
enum {
@@ -870,7 +887,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
BISECT_NEXT,
BISECT_AUTO_NEXT,
BISECT_AUTOSTART,
-   BISECT_STATE
+   BISECT_STATE,
+   BISECT_LOG
} cmdmode = 0;
int no_checkout = 0, res = 0;
struct option options[] = {
@@ -894,6 +912,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 N_("start the bisection if BISECT_START empty or 
missing"), BISECT_AUTOSTART),
OPT_CMDMODE(0, "bisect-state", ,
 N_("mark the state of ref (or refs)"), BISECT_STATE),
+   OPT_CMDMODE(0, "bisect-log", ,
+N_("output the contents of BISECT_LOG"), BISECT_LOG),
OPT_BOOL(0, "no-checkout", _checkout,
 N_("update BISECT_HEAD instead of checking out the 
current commit")),
OPT_END()
@@ -973,6 +993,11 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
get_terms();
res = bisect_state(, argv, argc);
break;
+   case BISECT_LOG:
+   if (argc > 1)
+   die(_("--bisect-log requires 0 arguments"));
+   res = bisect_log();
+   break;
default:
die("BUG: unknown subcommand '%d'", cmdmode);
}
diff --git a/git-bisect.sh b/git-bisect.sh
index a9eebbb..a47e3b5 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -166,11 +166,6 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
done
 }
 
-bisect_log () {
-   test -s "$GIT_DIR/BISECT_LOG" || die "$(gettext "We are not 
bisecting.")"
-   cat "$GIT_DIR/BISECT_LOG"
-}
-
 get_terms () {
if test -s "$GIT_DIR/BISECT_TERMS"
then
@@ -208,7 +203,7 @@ case "$#" in
replay)
bisect_replay "$@" ;;
log)
-   bisect_log ;;
+   git bisect--helper --bisect-log ;;
run)
bisect_run "$@" ;;
terms)

--
https://github.com/git/git/pull/287

[PATCH v15 21/27] bisect--helper: retire `--write-terms` subcommand

[Pranit Bauva]

The `--write-terms` subcommand is no longer used in the shell script and
the function `write_terms()` is called from the C implementation of
`set_terms()` and `bisect_start()`.

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 11 +--
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index d5fe35b..493034c 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -20,7 +20,6 @@ static GIT_PATH_FUNC(git_path_head_name, "head-name")
 static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
 
 static const char * const git_bisect_helper_usage[] = {
-   N_("git bisect--helper --write-terms  "),
N_("git bisect--helper --bisect-reset []"),
N_("git bisect--helper --bisect-write
 []"),
N_("git bisect--helper --bisect-check-and-set-terms  
 "),
@@ -862,8 +861,7 @@ static int bisect_state(struct bisect_terms *terms, const 
char **argv,
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
enum {
-   WRITE_TERMS = 1,
-   BISECT_RESET,
+   BISECT_RESET = 1,
BISECT_WRITE,
CHECK_AND_SET_TERMS,
BISECT_NEXT_CHECK,
@@ -876,8 +874,6 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
} cmdmode = 0;
int no_checkout = 0, res = 0;
struct option options[] = {
-   OPT_CMDMODE(0, "write-terms", ,
-N_("write the terms to .git/BISECT_TERMS"), 
WRITE_TERMS),
OPT_CMDMODE(0, "bisect-reset", ,
 N_("reset the bisection state"), BISECT_RESET),
OPT_CMDMODE(0, "bisect-write", ,
@@ -913,11 +909,6 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 
switch (cmdmode) {
int nolog;
-   case WRITE_TERMS:
-   if (argc != 2)
-   die(_("--write-terms requires two arguments"));
-   res = write_terms(argv[0], argv[1]);
-   break;
case BISECT_RESET:
if (argc > 1)
die(_("--bisect-reset requires either zero or one 
arguments"));

--
https://github.com/git/git/pull/287

[PATCH v15 23/27] bisect--helper: `bisect_replay` shell function in C

[Pranit Bauva]

Reimplement the `bisect_replay` shell function in C and also add
`--bisect-replay` subcommand to `git bisect--helper` to call it from
git-bisect.sh

Using `--bisect-replay` subcommand is a temporary measure to port shell
function to C so as to use the existing test suite. As more functions
are ported, this subcommand will be retired and will be called by some
other method.

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 125 ++-
 git-bisect.sh|  32 +---
 2 files changed, 124 insertions(+), 33 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index c18ca07..b367d8d 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -32,6 +32,7 @@ static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --bisect-autostart"),
N_("git bisect--helper --bisect-state (bad|new) []"),
N_("git bisect--helper --bisect-state (good|old) [...]"),
+   N_("git bisect--helper --bisect-replay "),
NULL
 };
 
@@ -601,7 +602,6 @@ static int bisect_start(struct bisect_terms *terms, int 
no_checkout,
terms->term_good = arg;
} else if (!strcmp(arg, "--term-bad") ||
 !strcmp(arg, "--term-new")) {
-   const char *next_arg;
if (starts_with(argv[++i], "'"))
terms->term_bad = sq_dequote(xstrdup(argv[i]));
else
@@ -875,6 +875,117 @@ static int bisect_log(void)
return status;
 }
 
+static int get_next_word(const char *line, int pos, struct strbuf *word)
+{
+   int i, len = strlen(line), begin = 0;
+   strbuf_reset(word);
+   for (i = pos; i < len; i++) {
+   if (line[i] == ' ' && begin)
+   return i + 1;
+
+   if (!begin)
+   begin = 1;
+   strbuf_addch(word, line[i]);
+   }
+
+   return i;
+}
+
+static int bisect_replay(struct bisect_terms *terms, const char *filename)
+{
+   struct strbuf line = STRBUF_INIT;
+   struct strbuf word = STRBUF_INIT;
+   FILE *fp = NULL;
+   int res = 0;
+
+   if (is_empty_or_missing_file(filename)) {
+   error(_("no such file with name '%s' exists"), filename);
+   res = -1;
+   goto finish;
+   }
+
+   if (bisect_reset(NULL)) {
+   res = -1;
+   goto finish;
+   }
+
+   fp = fopen(filename, "r");
+   if (!fp) {
+   res = -1;
+   goto finish;
+   }
+
+   while (strbuf_getline(, fp) != EOF) {
+   int pos = 0;
+   while (pos < line.len) {
+   pos = get_next_word(line.buf, pos, );
+
+   if (!strcmp(word.buf, "git")) {
+   continue;
+   } else if (!strcmp(word.buf, "git-bisect")) {
+   continue;
+   } else if (!strcmp(word.buf, "bisect")) {
+   continue;
+   } else if (!strcmp(word.buf, "#")) {
+   break;
+   }
+
+   get_terms(terms);
+   if (check_and_set_terms(terms, word.buf)) {
+   res = -1;
+   goto finish;
+   }
+
+   if (!strcmp(word.buf, "start")) {
+   struct argv_array argv = ARGV_ARRAY_INIT;
+   sq_dequote_to_argv_array(line.buf+pos, );
+   if (bisect_start(terms, 0, argv.argv, 
argv.argc)) {
+   argv_array_clear();
+   res = -1;
+   goto finish;
+   }
+   argv_array_clear();
+   break;
+   }
+
+   if (one_of(word.buf, terms->term_good,
+   terms->term_bad, "skip", NULL)) {
+   if (bisect_write(word.buf, line.buf+pos, terms, 
0)) {
+   res = -1;
+   goto finish;
+   }
+   break;
+   }
+
+   if (!strcmp(word.buf, "terms")) {
+   struct argv_array argv = ARGV_ARRAY_INIT;
+   sq_dequote_to_argv_array(line.buf+pos, );
+   if (bisect_terms(terms, argv.argv, argv.argc)) {
+   

[PATCH v15 15/27] bisect--helper: `bisect_next` and `bisect_auto_next` shell function in C

[Pranit Bauva]

Reimplement the `bisect_next` and the `bisect_auto_next` shell function
in C and add the subcommands to `git bisect--helper` to call it from
git-bisect.sh .

Along with this conversion of `bisect_start` has also finished and thus
it has been fully ported to C.

A lot of parts of bisect.c uses exit() and these signals are then
trapped in the `bisect_start` function. Since the shell script ceases
its existence it would be necessary to convert those exit() calls to
return statements so that errors can be reported.

As more and more calls are happening to the subcommands in `git
bisect--helper`, more specifically when `bisect_start $rev` is converted to
`git bisect--helper --bisect-start $rev` it is necessary to dequote the
arguments because of shell to C conversion.

Using `--bisect-next` and `--bisect-auto-start` subcommands is a
temporary measure to port shell function to C so as to use the existing
test suite. As more functions are ported, this subcommand will be
retired and will be called by some other methods.

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 bisect.c | 128 
 builtin/bisect--helper.c | 189 ---
 git-bisect.sh|  74 ++-
 3 files changed, 285 insertions(+), 106 deletions(-)

diff --git a/bisect.c b/bisect.c
index 45d598d..7c97e85 100644
--- a/bisect.c
+++ b/bisect.c
@@ -618,6 +618,12 @@ static void bisect_rev_setup(struct rev_info *revs, const 
char *prefix,
struct argv_array rev_argv = ARGV_ARRAY_INIT;
int i;
 
+   /*
+* Since the code is slowly being converted to C, there might be
+* instances where the revisions were initialized before. Thus
+* we first need to reset it.
+*/
+   reset_revision_walk();
init_revisions(revs, prefix);
revs->abbrev = 0;
revs->commit_format = CMIT_FMT_UNSPECIFIED;
@@ -638,17 +644,22 @@ static void bisect_rev_setup(struct rev_info *revs, const 
char *prefix,
 
 static void bisect_common(struct rev_info *revs)
 {
+   /*
+* We don't want to clean the bisection state
+* as we need to get back to where we started
+* by using `git bisect reset`.
+*/
if (prepare_revision_walk(revs))
die("revision walk setup failed");
if (revs->tree_objects)
mark_edges_uninteresting(revs, NULL);
 }
 
-static void exit_if_skipped_commits(struct commit_list *tried,
+static int exit_if_skipped_commits(struct commit_list *tried,
const struct object_id *bad)
 {
if (!tried)
-   return;
+   return 0;
 
printf("There are only 'skip'ped commits left to test.\n"
   "The first %s commit could be any of:\n", term_bad);
@@ -659,7 +670,13 @@ static void exit_if_skipped_commits(struct commit_list 
*tried,
if (bad)
printf("%s\n", oid_to_hex(bad));
printf(_("We cannot bisect more!\n"));
-   exit(2);
+
+   /*
+* We don't want to clean the bisection state
+* as we need to get back to where we started
+* by using `git bisect reset`.
+*/
+   return 2;
 }
 
 static int is_expected_rev(const struct object_id *oid)
@@ -700,7 +717,7 @@ static int bisect_checkout(const unsigned char *bisect_rev, 
int no_checkout)
int res;
res = run_command_v_opt(argv_checkout, RUN_GIT_CMD);
if (res)
-   exit(res);
+   return res;
}
 
argv_show_branch[1] = bisect_rev_hex;
@@ -729,7 +746,7 @@ static struct commit **get_bad_and_good_commits(int *rev_nr)
return rev;
 }
 
-static void handle_bad_merge_base(void)
+static int handle_bad_merge_base(void)
 {
if (is_expected_rev(current_bad_oid)) {
char *bad_hex = oid_to_hex(current_bad_oid);
@@ -750,17 +767,23 @@ static void handle_bad_merge_base(void)
"between %s and [%s].\n"),
bad_hex, term_bad, term_good, bad_hex, 
good_hex);
}
-   exit(3);
+   /*
+* We don't want to clean the bisection state
+* as we need to get back to where we started
+* by using `git bisect reset`.
+*/
+   return 3;
}
 
fprintf(stderr, _("Some %s revs are not ancestor of the %s rev.\n"
"git bisect cannot work properly in this case.\n"
"Maybe you mistook %s and %s revs?\n"),
term_good, term_bad, term_good, term_bad);
-   exit(1);
+   bisect_clean_state();
+   return 1;
 }
 
-static void handle_skipped_merge_base(const unsigned char *mb)
+static int 

Re: Can we make interactive add easier to use?

[Matthieu Moy]

Robert Dailey  writes:

> Normally when I use interactive add, I just want to add files to the
> index via simple numbers, instead of typing paths. So I'll do this as
> quick as I can:
>
> 1. Type `git add -i`
> 2. Press `u` after prompt appears
> 3. Press numbers for the files I want to add, ENTER key
> 4. ENTER key again to go back to main add -i menu
> 5. Press `q` to exit interactive add
> 6. Type `git commit`
>
> This feels very tedious. Is there a simplified workflow for this?

My workflow is to ... not use "git add -i" ;-).

To add patch hunks individually, "git add -p" jumps directly to the
"patch" inner loop of "git add -i".

To add whole individual files, a plain "git add" using zsh's smart
completion (autocompletes only files for which "git add" is not a
no-op), or globs.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 0/2] infinite loop in "git ls-tree" for broken symlink

[Jeff King]

On Fri, Oct 14, 2016 at 03:16:50PM +0200, Petr Stodulka wrote:

> I have realized that this wasn't fixed after all when refs.c
> was "rewritten". Issue is caused by broken symlink under refs/heads,
> which causes infinite loop for "git ls-tree" command. It was replied
> earlier [0] and Michael previously fixed that in better way probably,
> then my proposed patch 2/2, but it contains more changes and I have not
> enough time to study changes in refs* code, so I propose now just my
> little patch, which was previously modified by Michael.
> 
> If you prefer different solution, I am ok with this. It is really
> just quick proposal. Patch 1/2 contains test for this issue. If you
> will prefer different solution with different exit code, test should
> be corrected. Basic idea is, that timeout should'n expire with
> exit code 124.
> 
> [0] http://marc.info/?l=git=142712669103790=2
> [1] https://github.com/mhagger/git, branch "ref-broken-symlinks"

I think I fixed this semi-independently last week; see the thread at:

  
http://public-inbox.org/git/20161006164723.ocg2nbgtulpjc...@sigill.intra.peff.net/

I say semi-independently because I didn't actually know about your
previous report, but saw it in the wild myself. But I did talk with
Michael off-list, and he suggested the belt-and-suspenders retry counter
in my second patch.

The fix is at e8c42cb in Junio's tree, and it's currently merged to
'next'.

-Peff

[PATCH v4 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(, 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(, 0, sizeof(opts));
opts.action = REPLAY_PICK;
git_config(git_default_config, NULL);
parse_args(argc, argv, );
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.1.513.g00ef6dd

[PATCH v4 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 (read: there will be three more patch
series after that before the full benefit hits git.git: sequencer-i,
rebase--helper and rebase-i-extra). It is based on the `libify-sequencer`
patch series I submitted earlier.

The patches in this series merely prepare the sequencer code for the
next patch series that actually teaches the sequencer to run rebase -i's
commands.

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 already
integrated the whole shebang into Git for Windows 2.10.0 and 2.10.1
where it has been running without complaints (and some quite positive
feedback).

Changes vs v3:

- fixed TRANSLATORS: comment to help the tool extracting those comments.

- reordered the patch introducing the short_commit_name() function so it
  can be used in the patch revamping the todo parsing right away, as
  opposed to fixing up the find_unique_abbrev() ugliness in a later
  patch.

- backed out the write_message_gently() function of this patch series:
  it is not used by the end of this patch series and would therefore let
  the build fail with DEVELOPER=1. That function is now introduced as
  part of the patch in the sequencer-i series that adds support for the
  'edit' command.

- edited "skip CR/skip LF" to say "strip" instead.

- used xstrdup_or_null() where appropriate.

- abstracted out git_config_string_dup() instead of adding
  near-duplicate code.

- marked the append_new_todo() function as file-local, pointed out by
  Ramsay.

- made the "you have staged changes" advice prettier by moving it out of
  the run_git_commit() function, based on a suggestion by Hannes Sixt.


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: refactor the code to obtain a short commit name
  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: 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  |  46 ++-
 sequencer.c   | 679 --
 sequencer.h   |  23 +-
 t/t3501-revert-cherry-pick.sh |   2 +-
 5 files changed, 492 insertions(+), 260 deletions(-)


base-commit: 3cdd5d19178a54d2e51b5098d43b57571241d0ab
Published-As: https://github.com/dscho/git/releases/tag/prepare-sequencer-v4
Fetch-It-Via: git fetch https://github.com/dscho/git prepare-sequencer-v4

Interdiff vs v3:

 diff --git a/builtin/revert.c b/builtin/revert.c
 index 0a7b5f4..4ca5b51 100644
 --- a/builtin/revert.c
 +++ b/builtin/revert.c
 @@ -166,10 +166,8 @@ static int run_sequencer(int argc, const char **argv, 
struct replay_opts *opts)
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);
 +  opts->gpg_sign = xstrdup_or_null(opts->gpg_sign);
 +  opts->strategy = xstrdup_or_null(opts->strategy);
  
if (cmd == 'q')
return sequencer_remove_state(opts);
 diff --git a/sequencer.c 

[PATCH 2/2] resolve_ref_unsafe(): limit the number of "stat_ref" retries

[Petr Stodulka]

From: Michael Haggerty 

If there is a broken symlink where a loose reference file is expected,
then the attempt to open() it fails with ENOENT. This error is
misinterpreted to mean that the loose reference file itself has
disappeared due to a race, causing the lookup to be retried. But in
this scenario, the retries all suffer from the same problem, causing
an infinite loop.

So put a limit (of 5) on the number of times that the stat_ref step
can be retried.

Based-on-patch-by: Petr Stodulka 
Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c | 6 --
 refs/refs-internal.h | 6 ++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index d16feb1..245a0b5 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1353,6 +1353,7 @@ static int files_read_raw_ref(struct ref_store *ref_store,
int fd;
int ret = -1;
int save_errno;
+   int retries = 0;
 
*type = 0;
strbuf_reset(_path);
@@ -1390,7 +1391,8 @@ static int files_read_raw_ref(struct ref_store *ref_store,
if (S_ISLNK(st.st_mode)) {
strbuf_reset(_contents);
if (strbuf_readlink(_contents, path, 0) < 0) {
-   if (errno == ENOENT || errno == EINVAL)
+   if ((errno == ENOENT || errno == EINVAL) &&
+   retries++ < MAXRETRIES) 
/* inconsistent with lstat; retry */
goto stat_ref;
else
@@ -1426,7 +1428,7 @@ static int files_read_raw_ref(struct ref_store *ref_store,
 */
fd = open(path, O_RDONLY);
if (fd < 0) {
-   if (errno == ENOENT)
+   if (errno == ENOENT && retries++ < MAXRETRIES)
/* inconsistent with lstat; retry */
goto stat_ref;
else
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 708b260..37e6b99 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -255,6 +255,12 @@ int rename_ref_available(const char *old_refname, const 
char *new_refname);
 /* We allow "recursive" symbolic refs. Only within reason, though */
 #define SYMREF_MAXDEPTH 5
 
+/* 
+ * We allow only MAXRETRIES tries to jump on stat_ref, because of possible
+ * infinite loop
+ */
+#define MAXRETRIES 5
+
 /* Include broken references in a do_for_each_ref*() iteration: */
 #define DO_FOR_EACH_INCLUDE_BROKEN 0x01
 
-- 
2.5.5

[PATCH v4 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 36c24b6..af88753 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 {
@@ -304,10 +304,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."));
@@ -325,7 +325,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(, _("%s: fast-forward"), action_name(opts));
+   strbuf_addf(, _("%s: fast-forward"), _(action_name(opts)));
 
transaction = ref_transaction_begin();
if (!transaction ||
@@ -401,7 +401,7 @@ static int do_recursive_merge(struct commit *base, struct 
commit *next,
write_locked_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(_lock);
 
if (opts->signoff)
@@ -836,14 +836,14 @@ static int read_and_refresh_cache(struct replay_opts 
*opts)
if (read_index_preload(_index, NULL) < 0) {
rollback_lock_file(_lock);
return error(_("git %s: failed to read the index"),
-   action_name(opts));
+   _(action_name(opts)));
}
refresh_index(_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, 
NULL);
if (the_index.cache_changed && index_fd >= 0) {
if (write_locked_index(_index, _lock, COMMIT_LOCK)) {
rollback_lock_file(_lock);
return error(_("git %s: failed to refresh the index"),
-   action_name(opts));
+   _(action_name(opts)));
}
}
rollback_lock_file(_lock);
-- 
2.10.1.513.g00ef6dd

[PATCH v4 11/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 ba5a88c..4ca5b51 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;
@@ -178,6 +168,14 @@ static void parse_args(int argc, const char **argv, struct 
replay_opts *opts)
/* These option values will be free()d */
opts->gpg_sign = xstrdup_or_null(opts->gpg_sign);
opts->strategy = xstrdup_or_null(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)
@@ -189,8 +187,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, );
-   res = sequencer_pick_revisions();
+   res = run_sequencer(argc, argv, );
if (res < 0)
die(_("revert failed"));
return res;
@@ -203,8 +200,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, );
-   res = sequencer_pick_revisions();
+   res = run_sequencer(argc, argv, );
if (res < 0)
die(_("cherry-pick failed"));
return res;
diff --git a/sequencer.c b/sequencer.c
index 9bca056..5e4bf23 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(, "%s", get_dir(opts));
remove_dir_recursively(, 0);
strbuf_release();
+
+   return 0;
 }
 
 static const char *action_name(const struct replay_opts *opts)
@@ -975,7 +977,7 @@ static int rollback_single_pick(void)
return reset_for_rollback(head_sha1);
 }
 

[PATCH v4 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 af88753..3e26631 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -252,7 +252,7 @@ static int write_with_lock_file(const char *filename,
}
if (commit_lock_file(_file) < 0) {
rollback_lock_file(_file);
-   return error(_("Error wrapping up %s."), filename);
+   return error(_("Error wrapping up '%s'."), filename);
}
 
return 0;
@@ -955,16 +955,16 @@ static int read_populate_todo(struct todo_list *todo_list,
strbuf_reset(_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(_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 =
@@ -1055,7 +1055,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;
 }
@@ -1098,7 +1098,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;
 }
@@ -1117,12 +1117,12 @@ static int save_head(const char *head)
strbuf_addf(, "%s\n", head);
if (write_in_full(fd, buf.buf, buf.len) < 0) {
rollback_lock_file(_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(_lock) < 0) {
rollback_lock_file(_lock);
-   return error(_("Error wrapping up %s."), git_path_head_file());
+   return error(_("Error wrapping up '%s'."), 
git_path_head_file());
}
return 0;
 }
@@ -1167,9 +1167,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(, 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;
@@ -1208,7 +1208,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(_lock) < 0)
-   return error(_("Error wrapping up %s."), todo_path);
+   return error(_("Error wrapping up '%s'."), todo_path);
return 0;
 }
 
-- 
2.10.1.513.g00ef6dd

[PATCH v4 07/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 | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index fb0b94b..499f5ee 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -147,13 +147,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, );
 
@@ -621,8 +626,7 @@ static int do_pick_commit(struct commit *commit, struct 
replay_opts *opts)
error(opts->action == REPLAY_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;
-- 
2.10.1.513.g00ef6dd

[PATCH 1/2] Add test for ls-tree with broken symlink under refs/heads

[Petr Stodulka]

git ls-tree goes into an infinite loop while serving pretty vanilla requests,
if the refs/heads/ directory contains a symlink that's broken. Added test
which check if git ends with expected exit code or timeout expires.
---
 t/t3103-ls-tree-misc.sh | 9 +
 1 file changed, 9 insertions(+)

diff --git a/t/t3103-ls-tree-misc.sh b/t/t3103-ls-tree-misc.sh
index 09dcf04..faf79c4 100755
--- a/t/t3103-ls-tree-misc.sh
+++ b/t/t3103-ls-tree-misc.sh
@@ -21,4 +21,13 @@ test_expect_success 'ls-tree fails with non-zero exit code 
on broken tree' '
test_must_fail git ls-tree -r HEAD
 '
 
+test_expect_success 'ls-tree fails due to broken symlink instead of infinite 
loop' '
+   mkdir foo_infinit &&
+   cd foo_infinit &&
+   git init testrepo &&
+   cd testrepo &&
+   mkdir -p .git/refs/remotes &&
+   ln -s ../remotes/foo .git/refs/heads/bar &&
+   test_expect_code 128 timeout 2 git ls-tree bar
+'
 test_done
-- 
2.5.5

[PATCH v4 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(_dir, git_path_seq_dir());
-   remove_dir_recursively(_dir, 0);
-   strbuf_release(_dir);
+   strbuf_addf(, "%s", get_dir(opts));
+   remove_dir_recursively(, 0);
+   strbuf_release();
 }
 
 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();
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.1.513.g00ef6dd

[PATCH v4 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 will make it easy to create a new convenience function
write_file_gently() that does not die(); as some of the upcoming callers
of this new function will want to append a newline character, we already
add that flag as parameter to 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 | 25 -
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 914a576..baccee9 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -234,22 +234,37 @@ 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(_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(_file) < 0)
+   if (write_in_full(msg_fd, buf, len) < 0) {
+   rollback_lock_file(_file);
+   return error_errno(_("Could not write to '%s'"), filename);
+   }
+   if (append_eol && write(msg_fd, "\n", 1) < 0) {
+   rollback_lock_file(_file);
+   return error_errno(_("Could not write eol to '%s"), filename);
+   }
+   if (commit_lock_file(_file) < 0) {
+   rollback_lock_file(_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;
+}
+
 /*
  * 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.1.513.g00ef6dd

[PATCH v4 10/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 eac531b..9bca056 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -229,11 +229,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.1.513.g00ef6dd

[PATCH v4 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 caef51e..99c39fb 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(, rebase_path_author_script(), 256) <= 0)
+   return NULL;
+
+   for (p = script.buf; *p; p++)
+   if (skip_prefix(p, "'''", (const char **)))
+   strbuf_splice(, p - script.buf, p2 - p, "'", 1);
+   else if (*p == '\'')
+   strbuf_splice(, p-- - script.buf, 1, "", 0);
+   else if (*p == '\n') {
+   *p = '\0';
+   count++;
+   }
+
+   env_size = (count + 1) * sizeof(*env);
+   strbuf_grow(, env_size);
+   memmove(script.buf + env_size, script.buf, script.len);
+   p = script.buf + env_size;
+   env = (char **)strbuf_detach(, 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();
argv_array_push(, "commit");
argv_array_push(, "-n");
@@ -391,14 +464,13 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
argv_array_pushf(, "-S%s", opts->gpg_sign);
if (opts->signoff)
argv_array_push(, "-s");
-   if (!opts->edit) {
-   argv_array_push(, "-F");
-   argv_array_push(, defmsg);
-   if (!opts->signoff &&
-

[PATCH 0/2] infinite loop in "git ls-tree" for broken symlink

[Petr Stodulka]

Hi,
I have realized that this wasn't fixed after all when refs.c
was "rewritten". Issue is caused by broken symlink under refs/heads,
which causes infinite loop for "git ls-tree" command. It was replied
earlier [0] and Michael previously fixed that in better way probably,
then my proposed patch 2/2, but it contains more changes and I have not
enough time to study changes in refs* code, so I propose now just my
little patch, which was previously modified by Michael.

If you prefer different solution, I am ok with this. It is really
just quick proposal. Patch 1/2 contains test for this issue. If you
will prefer different solution with different exit code, test should
be corrected. Basic idea is, that timeout should'n expire with
exit code 124.

[0] http://marc.info/?l=git=142712669103790=2
[1] https://github.com/mhagger/git, branch "ref-broken-symlinks"

Michael Haggerty (1):
  resolve_ref_unsafe(): limit the number of "stat_ref" retries

Petr Stodulka (1):
  Add test for ls-tree with broken symlink under refs/heads

 refs/files-backend.c| 6 --
 refs/refs-internal.h| 6 ++
 t/t3103-ls-tree-misc.sh | 9 +
 3 files changed, 19 insertions(+), 2 deletions(-)

-- 
2.5.5

Can we make interactive add easier to use?

[Robert Dailey]

Normally when I use interactive add, I just want to add files to the
index via simple numbers, instead of typing paths. So I'll do this as
quick as I can:

1. Type `git add -i`
2. Press `u` after prompt appears
3. Press numbers for the files I want to add, ENTER key
4. ENTER key again to go back to main add -i menu
5. Press `q` to exit interactive add
6. Type `git commit`

This feels very tedious. Is there a simplified workflow for this? I
remember using a "git index" unofficial extension to git that let you
do a `git status` that showed numbers next to each item (including
untracked files!) and you could do `git add 1, 2, 3-5`, etc.

Thoughts? Even though this feels like I'm complaining, honestly I am
just consulting the experts here to see if I'm missing out on some
usability features. Thanks in advance!!

[PATCH v4 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(_dir, git_path(SEQ_DIR));
+   strbuf_addstr(_dir, git_path_seq_dir());
remove_dir_recursively(_dir, 0);
strbuf_release(_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.1.513.g00ef6dd

[PATCH v4 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 3e26631..57c5c0c 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(_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(_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(_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(_file) < 0) {
rollback_lock_file(_file);
-   return error(_("Error wrapping up '%s'."), filename);
+   return error(_("failed to finalize '%s'."), filename);
}
 
return 0;
@@ -306,11 +306,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;
 }
 
@@ -419,7 +419,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);
 
@@ -439,7 +439,7 @@ static int is_index_unchanged(void)
 
if (!cache_tree_fully_valid(active_cache_tree))
if (cache_tree_update(_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);
 }
@@ -509,7 +509,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"
@@ -562,12 +562,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(>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(>object.oid));
ptree_sha1 = parent->tree->object.oid.hash;
} else {
@@ -651,7 +651,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 {
unborn = 

[PATCH v4 09/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 f797e8a..eac531b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -776,6 +776,9 @@ static int parse_insn_buffer(char *buf, struct todo_list 
*todo_list)
 
next_p = *eol ? eol + 1 /* strip LF */ : eol;
 
+   if (p != eol && eol[-1] == '\r')
+   eol--; /* strip 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.1.513.g00ef6dd

[PATCH v4 08/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 | 284 ++--
 1 file changed, 163 insertions(+), 121 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 499f5ee..f797e8a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -470,7 +470,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;
@@ -529,10 +548,11 @@ 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(>object.oid));
+   command_to_string(command),
+   oid_to_hex(>object.oid));
 
if (get_message(commit, ) != 0)
return error(_("Cannot get commit message for %s"),
@@ -545,7 +565,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;
@@ -586,7 +606,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, , opts);
if (res < 0)
@@ -613,17 +633,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.
 */
-   if (opts->action 

[PATCH v4 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 5e4bf23..caef51e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -706,6 +706,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;
 };
 
@@ -757,6 +759,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;
 
@@ -907,6 +912,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, );
strbuf_addf(_list->buf, "%s %s %.*s\n", command_string,
-- 
2.10.1.513.g00ef6dd

Re: [PATCH 2/2] resolve_ref_unsafe(): limit the number of "stat_ref" retries

[Petr Stodulka]

FYI,
I modified the patch slightly.

On 14.10.2016 15:16, Petr Stodulka wrote:
> From: Michael Haggerty 
> 
> If there is a broken symlink where a loose reference file is expected,
> then the attempt to open() it fails with ENOENT. This error is
> misinterpreted to mean that the loose reference file itself has
> disappeared due to a race, causing the lookup to be retried. But in
> this scenario, the retries all suffer from the same problem, causing
> an infinite loop.
> 
> So put a limit (of 5) on the number of times that the stat_ref step
> can be retried.
> 
> Based-on-patch-by: Petr Stodulka 
> Signed-off-by: Michael Haggerty 
> ---
>  refs/files-backend.c | 6 --
>  refs/refs-internal.h | 6 ++
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index d16feb1..245a0b5 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -1353,6 +1353,7 @@ static int files_read_raw_ref(struct ref_store 
> *ref_store,
>   int fd;
>   int ret = -1;
>   int save_errno;
> + int retries = 0;
>  
>   *type = 0;
>   strbuf_reset(_path);
> @@ -1390,7 +1391,8 @@ static int files_read_raw_ref(struct ref_store 
> *ref_store,
>   if (S_ISLNK(st.st_mode)) {
>   strbuf_reset(_contents);
>   if (strbuf_readlink(_contents, path, 0) < 0) {
> - if (errno == ENOENT || errno == EINVAL)
> + if ((errno == ENOENT || errno == EINVAL) &&
> + retries++ < MAXRETRIES) 
>   /* inconsistent with lstat; retry */
>   goto stat_ref;
>   else
> @@ -1426,7 +1428,7 @@ static int files_read_raw_ref(struct ref_store 
> *ref_store,
>*/
>   fd = open(path, O_RDONLY);
>   if (fd < 0) {
> - if (errno == ENOENT)
> + if (errno == ENOENT && retries++ < MAXRETRIES)
>   /* inconsistent with lstat; retry */
>   goto stat_ref;
>   else
> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index 708b260..37e6b99 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -255,6 +255,12 @@ int rename_ref_available(const char *old_refname, const 
> char *new_refname);
>  /* We allow "recursive" symbolic refs. Only within reason, though */
>  #define SYMREF_MAXDEPTH 5
>  
> +/* 
> + * We allow only MAXRETRIES tries to jump on stat_ref, because of possible
> + * infinite loop
> + */
> +#define MAXRETRIES 5
> +
>  /* Include broken references in a do_for_each_ref*() iteration: */
>  #define DO_FOR_EACH_INCLUDE_BROKEN 0x01
>  
> 



signature.asc
Description: OpenPGP digital signature

[PATCH v4 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 | 26 --
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 57c5c0c..1cf70f7 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -483,6 +483,20 @@ static char **read_author_script(void)
return env;
 }
 
+static const char staged_changes_advice[] =
+N_("you have staged changes in your working tree\n"
+"If these changes are meant to be squashed into the previous commit, run:\n"
+"\n"
+"  git commit --amend %s\n"
+"\n"
+"If they are meant to go into a new commit, run:\n"
+"\n"
+"  git commit %s\n"
+"\n"
+"In both cases, once you're done, continue with:\n"
+"\n"
+"  git rebase --continue\n");
+
 /*
  * If we are cherry-pick, and if the merge did not result in
  * hand-editing, we will hit this commit and inherit the original
@@ -509,16 +523,8 @@ 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(_(staged_changes_advice),
+gpg_opt, gpg_opt);
}
}
 
-- 
2.10.1.513.g00ef6dd

[PATCH v4 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 baccee9..36c24b6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -963,7 +963,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;
@@ -977,8 +980,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.1.513.g00ef6dd

[PATCH v4 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() ||
+   if (read_populate_opts(opts) ||
read_populate_todo(_list, opts))
return -1;
 
-- 
2.10.1.513.g00ef6dd

[PATCH v4 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 9ffc090..914a576 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -871,6 +871,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], )) {
item->command = i;
-- 
2.10.1.513.g00ef6dd

[PATCH v4 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 04c55f2..fb0b94b 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(, fd, 0) < 0) {
close(fd);
strbuf_release();
-   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();
if (res)
-   return error(_("Unusable instruction sheet: %s"),
-   git_path_todo_file());
+   return error(_("Unusable instruction sheet: %s"), todo_file);
return 0;
 }
 
@@ -1075,7 +1079,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(_list, opts))
-- 
2.10.1.513.g00ef6dd

[PATCH v4 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 |  4 
 sequencer.c  | 26 ++
 sequencer.h  |  6 +++---
 3 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 7365559..ba5a88c 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -174,6 +174,10 @@ 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 */
+   opts->gpg_sign = xstrdup_or_null(opts->gpg_sign);
+   opts->strategy = xstrdup_or_null(opts->strategy);
 }
 
 int cmd_revert(int argc, const char **argv, const char *prefix)
diff --git a/sequencer.c b/sequencer.c
index 8d272fb..04c55f2 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(, "%s", get_dir(opts));
remove_dir_recursively(, 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(_lock, 1);
@@ -583,7 +590,8 @@ static int do_pick_commit(struct commit *commit, struct 
replay_opts *opts)
 
commit_list_insert(base, );
commit_list_insert(next, );
-   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);
@@ -783,6 +791,16 @@ static int read_populate_todo(struct commit_list 
**todo_list,
return 0;
 }
 
+static int git_config_string_dup(char **dest,
+const char *var, const char *value)
+{
+   if (!value)
+   return config_error_nonbool(var);
+   free(*dest);
+   *dest = xstrdup(value);
+   return 0;
+}
+
 static int populate_opts_cb(const char *key, const char *value, void *data)
 {
struct replay_opts *opts = data;
@@ -803,9 +821,9 @@ static int populate_opts_cb(const char *key, const char 
*value, void *data)
else if (!strcmp(key, "options.mainline"))
opts->mainline = git_config_int(key, value);
else if (!strcmp(key, "options.strategy"))
-   git_config_string(>strategy, key, value);
+   git_config_string_dup(>strategy, key, value);
else if (!strcmp(key, "options.gpg-sign"))
-   git_config_string(>gpg_sign, key, value);
+   git_config_string_dup(>gpg_sign, key, 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.1.513.g00ef6dd

[PATCH v4 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 de6b95f..2ef80e7 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();
+   if (opts->gpg_sign)
+   sq_quotef(, "-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();
@@ -497,7 +516,7 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
argv_array_push(, "-s");
if (defmsg)
argv_array_pushl(, "-F", defmsg, NULL);
-   if (opts->edit)
+   if (edit)
argv_array_push(, "-e");
else if (!opts->signoff && !opts->record_origin &&
 git_config_get_value("commit.cleanup", ))
@@ -764,7 +783,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, );
@@ -986,8 +1005,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(, rebase_path_gpg_sign_opt(), 1)) {
+   if (!starts_with(buf.buf, "-S"))
+ 

[PATCH v4 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 2ef80e7..fa77c82 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(, "commit");
argv_array_push(, "-n");
 
+   if (amend)
+   argv_array_push(, "--amend");
if (opts->gpg_sign)
argv_array_pushf(, "-S%s", opts->gpg_sign);
if (opts->signoff)
@@ -783,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->edit);
+opts, allow, opts->edit, 0);
 
 leave:
free_message(commit, );
-- 
2.10.1.513.g00ef6dd

[PATCH v4 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 cbc3742..9ffc090 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -787,7 +787,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.1.513.g00ef6dd

[PATCH v4 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 99c39fb..de6b95f 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.1.513.g00ef6dd

[PATCH v4 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 fa77c82..cbc3742 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(, "-s");
if (defmsg)
argv_array_pushl(, "-F", defmsg, NULL);
+   if (cleanup_commit_message)
+   argv_array_push(, "--cleanup=strip");
if (edit)
argv_array_push(, "-e");
-   else if (!opts->signoff && !opts->record_origin &&
+   else if (!cleanup_commit_message &&
+!opts->signoff && !opts->record_origin &&
 git_config_get_value("commit.cleanup", ))
argv_array_push(, "--cleanup=verbatim");
 
@@ -785,7 +789,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, );
-- 
2.10.1.513.g00ef6dd

[PATCH 2/3] i18n: apply: mark info messages for translation

[Vasco Almeida]

Mark messages for translation printed to stderr.

Signed-off-by: Vasco Almeida 
---
 apply.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/apply.c b/apply.c
index 201d3a7..13b2064 100644
--- a/apply.c
+++ b/apply.c
@@ -3554,7 +3554,7 @@ static int try_threeway(struct apply_state *state,
return error("repository lacks the necessary blob to fall back 
on 3-way merge.");
 
if (state->apply_verbosity > verbosity_silent)
-   fprintf(stderr, "Falling back to three-way merge...\n");
+   fprintf(stderr, _("Falling back to three-way merge...\n"));
 
img = strbuf_detach(, );
prepare_image(_image, img, len, 1);
@@ -3586,7 +3586,7 @@ static int try_threeway(struct apply_state *state,
if (status < 0) {
if (state->apply_verbosity > verbosity_silent)
fprintf(stderr,
-   "Failed to fall back on three-way merge...\n");
+   _("Failed to fall back on three-way 
merge...\n"));
return status;
}
 
@@ -3600,12 +3600,12 @@ static int try_threeway(struct apply_state *state,
oidcpy(>threeway_stage[2], _oid);
if (state->apply_verbosity > verbosity_silent)
fprintf(stderr,
-   "Applied patch to '%s' with conflicts.\n",
+   _("Applied patch to '%s' with conflicts.\n"),
patch->new_name);
} else {
if (state->apply_verbosity > verbosity_silent)
fprintf(stderr,
-   "Applied patch to '%s' cleanly.\n",
+   _("Applied patch to '%s' cleanly.\n"),
patch->new_name);
}
return 0;
-- 
2.10.1.459.g5fd885d

[PATCH 1/3] i18n: apply: mark plural string for translation

[Vasco Almeida]

Mark plural string for translation using Q_().

Signed-off-by: Vasco Almeida 
---
 apply.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/apply.c b/apply.c
index b03d274..201d3a7 100644
--- a/apply.c
+++ b/apply.c
@@ -4869,10 +4869,12 @@ int apply_all_patches(struct apply_state *state,
goto end;
}
if (state->applied_after_fixing_ws && state->apply)
-   warning("%d line%s applied after"
-   " fixing whitespace errors.",
-   state->applied_after_fixing_ws,
-   state->applied_after_fixing_ws == 1 ? "" : "s");
+   warning(Q_("%d line applied after"
+  " fixing whitespace errors.",
+  "%d lines applied after"
+  " fixing whitespace errors.",
+  state->applied_after_fixing_ws),
+   state->applied_after_fixing_ws);
else if (state->whitespace_error)
warning(Q_("%d line adds whitespace errors.",
   "%d lines add whitespace errors.",
-- 
2.10.1.459.g5fd885d

[PATCH 3/3] i18n: apply: mark error messages for translation

[Vasco Almeida]

Mark error messages for translation passed to error() and die()
functions.

Signed-off-by: Vasco Almeida 
---
 apply.c | 48 
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/apply.c b/apply.c
index 13b2064..8215874 100644
--- a/apply.c
+++ b/apply.c
@@ -122,9 +122,9 @@ int check_apply_state(struct apply_state *state, int 
force_apply)
int is_not_gitdir = !startup_info->have_repository;
 
if (state->apply_with_reject && state->threeway)
-   return error("--reject and --3way cannot be used together.");
+   return error(_("--reject and --3way cannot be used together."));
if (state->cached && state->threeway)
-   return error("--cached and --3way cannot be used together.");
+   return error(_("--cached and --3way cannot be used together."));
if (state->threeway) {
if (is_not_gitdir)
return error(_("--3way outside a repository"));
@@ -3095,8 +3095,8 @@ static int apply_binary_fragment(struct apply_state 
*state,
/* Binary patch is irreversible without the optional second hunk */
if (state->apply_in_reverse) {
if (!fragment->next)
-   return error("cannot reverse-apply a binary patch "
-"without the reverse hunk to '%s'",
+   return error(_("cannot reverse-apply a binary patch "
+  "without the reverse hunk to '%s'"),
 patch->new_name
 ? patch->new_name : patch->old_name);
fragment = fragment->next;
@@ -3141,8 +3141,8 @@ static int apply_binary(struct apply_state *state,
strlen(patch->new_sha1_prefix) != 40 ||
get_oid_hex(patch->old_sha1_prefix, ) ||
get_oid_hex(patch->new_sha1_prefix, ))
-   return error("cannot apply binary patch to '%s' "
-"without full index line", name);
+   return error(_("cannot apply binary patch to '%s' "
+  "without full index line"), name);
 
if (patch->old_name) {
/*
@@ -3151,16 +3151,16 @@ static int apply_binary(struct apply_state *state,
 */
hash_sha1_file(img->buf, img->len, blob_type, oid.hash);
if (strcmp(oid_to_hex(), patch->old_sha1_prefix))
-   return error("the patch applies to '%s' (%s), "
-"which does not match the "
-"current contents.",
+   return error(_("the patch applies to '%s' (%s), "
+  "which does not match the "
+  "current contents."),
 name, oid_to_hex());
}
else {
/* Otherwise, the old one must be empty. */
if (img->len)
-   return error("the patch applies to an empty "
-"'%s' but it is not empty", name);
+   return error(_("the patch applies to an empty "
+  "'%s' but it is not empty"), name);
}
 
get_oid_hex(patch->new_sha1_prefix, );
@@ -3177,8 +3177,8 @@ static int apply_binary(struct apply_state *state,
 
result = read_sha1_file(oid.hash, , );
if (!result)
-   return error("the necessary postimage %s for "
-"'%s' cannot be read",
+   return error(_("the necessary postimage %s for "
+  "'%s' cannot be read"),
 patch->new_sha1_prefix, name);
clear_image(img);
img->buf = result;
@@ -3551,7 +3551,7 @@ static int try_threeway(struct apply_state *state,
write_sha1_file("", 0, blob_type, pre_oid.hash);
else if (get_sha1(patch->old_sha1_prefix, pre_oid.hash) ||
 read_blob_object(, _oid, patch->old_mode))
-   return error("repository lacks the necessary blob to fall back 
on 3-way merge.");
+   return error(_("repository lacks the necessary blob to fall 
back on 3-way merge."));
 
if (state->apply_verbosity > verbosity_silent)
fprintf(stderr, _("Falling back to three-way merge...\n"));
@@ -3570,11 +3570,11 @@ static int try_threeway(struct apply_state *state,
/* our_oid is ours */
if (patch->is_new) {
if (load_current(state, _image, patch))
-   return error("cannot read the current contents of '%s'",
+   return error(_("cannot read the current contents of 
'%s'"),
   

Re: Huge performance bottleneck reading packs

[Jakub Narębski]

W dniu 13.10.2016 o 11:04, Vegard Nossum pisze:
> On 10/13/2016 01:47 AM, Jeff King wrote:
>> On Wed, Oct 12, 2016 at 07:18:07PM -0400, Jeff King wrote:
>>
>>> Also, is it possible to make the repository in question available? I
>>> might be able to reproduce based on your description, but it would save
>>> time if I could directly run gdb on your example.
> 
> I won't be able to make the repository available, sorry.

We have 'git fast-export --anonymize ...', but it would obviously
not preserve the pack structure (though it can help in other cases
when one cannot make repository available).

-- 
Jakub Narębski