Re: [PATCH] diff: add interhunk context config option

[Pranit Bauva]

Hey Vegard,

On Tue, Jan 3, 2017 at 5:01 AM, Vegard Nossum  wrote:
> The --inter-hunk-context= option was added in commit 6d0e674a5754
> ("diff: add option to show context between close hunks"). This patch
> allows configuring a default for this option.
>
> Cc: René Scharfe 
> Signed-off-by: Vegard Nossum 
> ---
>  Documentation/diff-options.txt | 2 ++
>  diff.c | 8 
>  2 files changed, 10 insertions(+)
>
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index e6215c372..163b65444 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -511,6 +511,8 @@ endif::git-format-patch[]
>  --inter-hunk-context=::
> Show the context between diff hunks, up to the specified number
> of lines, thereby fusing hunks that are close to each other.
> +   Defaults to `diff.interhunkcontext` or 0 if the config option
> +   is unset.

Seems good. But I think you have missed adding the documentation of
this to Documentation/diff-config.txt . Generally whatever config
variable one adds, is added to Documentation/config.txt but in this
case, there exists a separate Documentation/diff-config.txt for all
"diff" related things which is included in Documentation/config.txt .
Also, you need to link the link both the parts of this documentation.
Please refer to commit id aaab84203 so as to know what I am talking
about.

>  -W::
>  --function-context::
> diff --git a/diff.c b/diff.c
> index 84dba60c4..40b4e6afe 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -33,6 +33,7 @@ static int diff_rename_limit_default = 400;
>  static int diff_suppress_blank_empty;
>  static int diff_use_color_default = -1;
>  static int diff_context_default = 3;
> +static int diff_interhunk_context_default = 0;
>  static const char *diff_word_regex_cfg;
>  static const char *external_diff_cmd_cfg;
>  static const char *diff_order_file_cfg;
> @@ -248,6 +249,12 @@ int git_diff_ui_config(const char *var, const char 
> *value, void *cb)
> return -1;
> return 0;
> }
> +   if (!strcmp(var, "diff.interhunkcontext")) {
> +   diff_interhunk_context_default = git_config_int(var, value);
> +   if (diff_interhunk_context_default < 0)
> +   return -1;
> +   return 0;
> +   }
> if (!strcmp(var, "diff.renames")) {
> diff_detect_rename_default = git_config_rename(var, value);
> return 0;
> @@ -3371,6 +3378,7 @@ void diff_setup(struct diff_options *options)
> options->rename_limit = -1;
> options->dirstat_permille = diff_dirstat_permille_default;
> options->context = diff_context_default;
> +   options->interhunkcontext = diff_interhunk_context_default;
> options->ws_error_highlight = ws_error_highlight_default;
> DIFF_OPT_SET(options, RENAME_EMPTY);

On a first look, it seems that we can overwrite the default config
values by using a different command line argument which is good.

Also, tests are missing. It seems that t/t4032 might be a good place
to add those tests.

Rest all is quite good! :)

Regards,
Pranit Bauva

Re: [PATCH 13/17] refs: convert each_reflog_ent_fn to struct object_id

[Jeff King]

On Tue, Jan 03, 2017 at 12:30:40AM +0100, Michael Haggerty wrote:

> > I think
> > my next series is going to include a small sscanf-style parser to parse
> > these.  Right now, using constants here is going to leave it extremely
> > difficult to read.  Something like the following for the OIDs:
> > 
> >   strbuf_git_scanf(sb, "%h %h ", , );
> > 
> > and then following up parsing the remainder.
> 
> Maybe something with an interface like skip_prefix wouldn't be too
> obnoxious:
> 
> const char *p = sb.buf;
> if (oid_prefix(p, , ) &&
> *p++ == ' ' &&
> oid_prefix(p, , ) && ...

Yeah, I've used C code before that had a very similar interface for
parsing, and when used consistently it's pretty pleasant. Something
like:

  if (parse_oid(p, , ) &&
  skip_whitespace(p, ) &&
  parse_refname(p, , ))

etc is nicer than some of the magic numbers we end up using in the
various parsers (I don't think anybody needs to mass-convert, I just
mean that something like parse_oid() seems like a step in a good
direction).

-Peff

[PATCH] diff: add interhunk context config option

[Vegard Nossum]

The --inter-hunk-context= option was added in commit 6d0e674a5754
("diff: add option to show context between close hunks"). This patch
allows configuring a default for this option.

Cc: René Scharfe 
Signed-off-by: Vegard Nossum 
---
 Documentation/diff-options.txt | 2 ++
 diff.c | 8 
 2 files changed, 10 insertions(+)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index e6215c372..163b65444 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -511,6 +511,8 @@ endif::git-format-patch[]
 --inter-hunk-context=::
Show the context between diff hunks, up to the specified number
of lines, thereby fusing hunks that are close to each other.
+   Defaults to `diff.interhunkcontext` or 0 if the config option
+   is unset.
 
 -W::
 --function-context::
diff --git a/diff.c b/diff.c
index 84dba60c4..40b4e6afe 100644
--- a/diff.c
+++ b/diff.c
@@ -33,6 +33,7 @@ static int diff_rename_limit_default = 400;
 static int diff_suppress_blank_empty;
 static int diff_use_color_default = -1;
 static int diff_context_default = 3;
+static int diff_interhunk_context_default = 0;
 static const char *diff_word_regex_cfg;
 static const char *external_diff_cmd_cfg;
 static const char *diff_order_file_cfg;
@@ -248,6 +249,12 @@ int git_diff_ui_config(const char *var, const char *value, 
void *cb)
return -1;
return 0;
}
+   if (!strcmp(var, "diff.interhunkcontext")) {
+   diff_interhunk_context_default = git_config_int(var, value);
+   if (diff_interhunk_context_default < 0)
+   return -1;
+   return 0;
+   }
if (!strcmp(var, "diff.renames")) {
diff_detect_rename_default = git_config_rename(var, value);
return 0;
@@ -3371,6 +3378,7 @@ void diff_setup(struct diff_options *options)
options->rename_limit = -1;
options->dirstat_permille = diff_dirstat_permille_default;
options->context = diff_context_default;
+   options->interhunkcontext = diff_interhunk_context_default;
options->ws_error_highlight = ws_error_highlight_default;
DIFF_OPT_SET(options, RENAME_EMPTY);
 
-- 
2.11.0.1.gaa10c3f

Re: [PATCH 13/17] refs: convert each_reflog_ent_fn to struct object_id

[Michael Haggerty]

On 01/02/2017 08:12 PM, brian m. carlson wrote:
> On Mon, Jan 02, 2017 at 04:07:16PM +0100, Michael Haggerty wrote:
>> On 01/01/2017 08:18 PM, brian m. carlson wrote:
>>> /* old SP new SP name  SP time TAB msg LF */
>>> if (sb->len < 83 || sb->buf[sb->len - 1] != '\n' ||
>>> -   get_sha1_hex(sb->buf, osha1) || sb->buf[40] != ' ' ||
>>> -   get_sha1_hex(sb->buf + 41, nsha1) || sb->buf[81] != ' ' ||
>>> +   get_oid_hex(sb->buf, ) || sb->buf[40] != ' ' ||
>>> +   get_oid_hex(sb->buf + 41, ) || sb->buf[81] != ' ' ||
>>
>> Some magic numbers above could be converted to use constants.
> 
> Yes, I saw that.  I opted to leave it as it is for the moment.

Totally understandable.

> I think
> my next series is going to include a small sscanf-style parser to parse
> these.  Right now, using constants here is going to leave it extremely
> difficult to read.  Something like the following for the OIDs:
> 
>   strbuf_git_scanf(sb, "%h %h ", , );
> 
> and then following up parsing the remainder.

Maybe something with an interface like skip_prefix wouldn't be too
obnoxious:

const char *p = sb.buf;
if (oid_prefix(p, , ) &&
*p++ == ' ' &&
oid_prefix(p, , ) && ...

> [...]

Michael

[PATCH] archive-zip: load userdiff config

[Jeff King]

Since 4aff646d17 (archive-zip: mark text files in archives,
2015-03-05), the zip archiver will look at the userdiff
driver to decide whether a file is text or binary. This
usually doesn't need to look any further than the attributes
themselves (e.g., "-diff", etc). But if the user defines a
custom driver like "diff=foo", we need to look at
"diff.foo.binary" in the config. Prior to this patch, we
didn't actually load it.

Signed-off-by: Jeff King 
---
I'd be surprised if anybody actually triggered this in practice. I don't
think any of the custom-driver fields except "binary" matter, and using
direct attributes is almost always easier than setting up a custom
driver. Though you could also do trickery with:

  git -c diff.default.binary=true archive ...

if you wanted to be really clever.

I ran across this while investigating a case where somebody's zipfile
was all marked as binary (it turned out not to be related; the issue was
just that their Git was pre-4aff646d17).

I also happened to notice that zipfiles are created using the local
timezone (because they have no notion of the timezone, so we have to
pick _something_). That's probably the least-terrible option, but it was
certainly surprising to me when I tried to bit-for-bit reproduce a
zipfile from GitHub on my local machine.

 archive-zip.c  |  7 +++
 t/t5003-archive-zip.sh | 22 ++
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/archive-zip.c b/archive-zip.c
index 9db47357b0..b429a8d974 100644
--- a/archive-zip.c
+++ b/archive-zip.c
@@ -554,11 +554,18 @@ static void dos_time(time_t *time, int *dos_date, int 
*dos_time)
*dos_time = t->tm_sec / 2 + t->tm_min * 32 + t->tm_hour * 2048;
 }
 
+static int archive_zip_config(const char *var, const char *value, void *data)
+{
+   return userdiff_config(var, value);
+}
+
 static int write_zip_archive(const struct archiver *ar,
 struct archiver_args *args)
 {
int err;
 
+   git_config(archive_zip_config, NULL);
+
dos_time(>time, _date, _time);
 
zip_dir = xmalloc(ZIP_DIRECTORY_MIN_SIZE);
diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
index 14744b2a4b..55c7870997 100755
--- a/t/t5003-archive-zip.sh
+++ b/t/t5003-archive-zip.sh
@@ -64,6 +64,12 @@ check_zip() {
test_cmp_bin $original/nodiff.crlf $extracted/nodiff.crlf &&
test_cmp_bin $original/nodiff.lf   $extracted/nodiff.lf
"
+
+   test_expect_success UNZIP " validate that custom diff is unchanged " "
+   test_cmp_bin $original/custom.cr   $extracted/custom.cr &&
+   test_cmp_bin $original/custom.crlf $extracted/custom.crlf &&
+   test_cmp_bin $original/custom.lf   $extracted/custom.lf
+   "
 }
 
 test_expect_success \
@@ -78,6 +84,9 @@ test_expect_success \
  printf "text\r"   >a/nodiff.cr &&
  printf "text\r\n" >a/nodiff.crlf &&
  printf "text\n"   >a/nodiff.lf &&
+ printf "text\r"   >a/custom.cr &&
+ printf "text\r\n" >a/custom.crlf &&
+ printf "text\n"   >a/custom.lf &&
  printf "\0\r" >a/binary.cr &&
  printf "\0\r\n"   >a/binary.crlf &&
  printf "\0\n" >a/binary.lf &&
@@ -112,15 +121,20 @@ test_expect_success 'add files to repository' '
 test_expect_success 'setup export-subst and diff attributes' '
echo "a/nodiff.* -diff" >>.git/info/attributes &&
echo "a/diff.* diff" >>.git/info/attributes &&
+   echo "a/custom.* diff=custom" >>.git/info/attributes &&
+   git config diff.custom.binary true &&
echo "substfile?" export-subst >>.git/info/attributes &&
git log --max-count=1 "--pretty=format:A${SUBSTFORMAT}O" HEAD \
>a/substfile1
 '
 
-test_expect_success \
-'create bare clone' \
-'git clone --bare . bare.git &&
- cp .git/info/attributes bare.git/info/attributes'
+test_expect_success 'create bare clone' '
+   git clone --bare . bare.git &&
+   cp .git/info/attributes bare.git/info/attributes &&
+   # Recreate our changes to .git/config rather than just copying it, as
+   # we do not want to clobber core.bare or other settings.
+   git -C bare.git config diff.custom.binary true
+'
 
 test_expect_success \
 'remove ignored file' \
-- 
2.11.0.519.g31435224cf

Re: builtin difftool parsing issue

[Paul Sbarra]

> I would have expected `git difftool --submodule=diff ...` to work... What
> are the problems?

The docs for difftool state...
"git difftool is a frontend to git diff and accepts the same options
and arguments."
which could lead a user to expect passing --submodule=diff to have a
similar behavior for difftool.  It would be especially useful when
combined with --dir-diff.

Unfortunately, due to the way the left/right directories are built up,
difftool needs to handle this option itself.  Currently a file
representing the submodule directory is created that contains the
hash.

if (S_ISGITLINK(lmode) || S_ISGITLINK(rmode)) {
   strbuf_reset();
   strbuf_addf(, "Subproject commit %s", oid_to_hex());
   add_left_or_right(, src_path, buf.buf, 0);
   strbuf_reset();
   strbuf_addf(, "Subproject commit %s", oid_to_hex());
   if (!oidcmp(, ))
  strbuf_addstr(, "-dirty");
   add_left_or_right(, dst_path, buf.buf, 1);
   continue;
}

To achieve the desired behavior a diff command would need to be run
within the submodule.  A further complication is whether submodules
should be processed recursively.  I'm not sure whether or not diff
handles them recursively.  I believe the logic to parse and build up
the files would need to be factored out such that it could be called
for the super-project as well as each submodule change.

This is all out of scope for your effort as the existing (perl-based)
difftool doesn't do this either.  However, it's a feature that would
provide a significant simplification to the workflow used at the
office to review changes.

Re: [PATCH v3 00/23] Delete directories left empty after ref deletion

[Jacob Keller]

On Mon, Jan 2, 2017 at 10:14 AM, Michael Haggerty  wrote:
> On 01/02/2017 05:19 AM, Jeff King wrote:
>> On Sun, Jan 01, 2017 at 12:36:11PM -0800, Jacob Keller wrote:
>>
>>> But how likely is it to end up with differing binaries running on the
>>> exact same repository concurrently? Basically, I am trying to see
>>> whether or not we could accidentally end up causing problems by trying
>>> to race with other git processes that haven't yet been made safe
>>> against race? Is the worst case only that some git operation would
>>> fail and you would have to retry?
>>
>> Yes, I think that is the worst case.
>>
>> A more likely scenario might be something like a server accepting pushes
>> or other ref updates from both JGit and regular git (or maybe libgit2
>> and regular git).
>>
>> IMHO it's not really worth worrying about too much. Certain esoteric
>> setups might have a slightly higher chance of a pretty obscure race
>> condition happening on a very busy repository. I hate to say "eh, ship
>> it, we'll see if anybody complains". But I'd be surprised to get a
>> single report about this.
>
> I agree. I think these races would mostly affect busy hosting sites and
> result in failed updates rather than corruption. And the races can
> already occur whenever the user runs `git pack-refs`. By contrast, the
> failure to delete empty directories is more likely to affect normal users.
>
> That being said, if Junio wants to merge all but the last two patches in
> one release, then merge the last two a release or two later, I have no
> objections.
>
> Michael
>

I only wanted to make sure that the failure mode would not result in
corruption. I believe that both you and Peff have alleviated my fears.

Thanks,
Jake

[PATCH v2 2/2] t9813: avoid using pipes

[Pranit Bauva]

The exit code of the upstream in a pipe is ignored thus we should avoid
using it. By writing out the output of the git command to a file, we can
test the exit codes of both the commands.

Signed-off-by: Pranit Bauva 
---
 t/t9813-git-p4-preserve-users.sh | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t9813-git-p4-preserve-users.sh b/t/t9813-git-p4-preserve-users.sh
index 798bf2b67..9d7550ff3 100755
--- a/t/t9813-git-p4-preserve-users.sh
+++ b/t/t9813-git-p4-preserve-users.sh
@@ -118,12 +118,12 @@ test_expect_success 'not preserving user with mixed 
authorship' '
make_change_by_user usernamefile3 Derek de...@example.com &&
P4EDITOR=cat P4USER=alice P4PASSWD=secret &&
export P4EDITOR P4USER P4PASSWD &&
-   git p4 commit |\
-   grep "git author de...@example.com does not match" &&
+   git p4 commit >actual 2>&1 &&
+   grep "git author de...@example.com does not match" actual &&
 
make_change_by_user usernamefile3 Charlie char...@example.com &&
-   git p4 commit |\
-   grep "git author char...@example.com does not match" &&
+   git p4 commit >actual 2>&1 &&
+   grep "git author char...@example.com does not match" actual &&
 
make_change_by_user usernamefile3 alice al...@example.com &&
git p4 commit >actual 2>&1 &&
-- 
2.11.0

[PATCH v2 1/2] don't use test_must_fail with grep

[Pranit Bauva]

test_must_fail should only be used for testing git commands. To test the
failure of other commands use `!`.

Reported-by: Stefan Beller 
Signed-off-by: Pranit Bauva 
---
 t/t3510-cherry-pick-sequence.sh  |  6 +++---
 t/t5504-fetch-receive-strict.sh  |  2 +-
 t/t5516-fetch-push.sh|  2 +-
 t/t5601-clone.sh |  2 +-
 t/t6030-bisect-porcelain.sh  |  2 +-
 t/t7610-mergetool.sh |  2 +-
 t/t9001-send-email.sh|  2 +-
 t/t9117-git-svn-init-clone.sh| 12 ++--
 t/t9813-git-p4-preserve-users.sh |  8 
 t/t9814-git-p4-rename.sh |  6 +++---
 10 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 372307c21..0acf4b146 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -385,7 +385,7 @@ test_expect_success '--continue respects opts' '
git cat-file commit HEAD~1 >picked_msg &&
git cat-file commit HEAD~2 >unrelatedpick_msg &&
git cat-file commit HEAD~3 >initial_msg &&
-   test_must_fail grep "cherry picked from" initial_msg &&
+   ! grep "cherry picked from" initial_msg &&
grep "cherry picked from" unrelatedpick_msg &&
grep "cherry picked from" picked_msg &&
grep "cherry picked from" anotherpick_msg
@@ -426,9 +426,9 @@ test_expect_failure '--signoff is automatically propagated 
to resolved conflict'
git cat-file commit HEAD~1 >picked_msg &&
git cat-file commit HEAD~2 >unrelatedpick_msg &&
git cat-file commit HEAD~3 >initial_msg &&
-   test_must_fail grep "Signed-off-by:" initial_msg &&
+   ! grep "Signed-off-by:" initial_msg &&
grep "Signed-off-by:" unrelatedpick_msg &&
-   test_must_fail grep "Signed-off-by:" picked_msg &&
+   ! grep "Signed-off-by:" picked_msg &&
grep "Signed-off-by:" anotherpick_msg
 '
 
diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index 9b19cff72..49d3621a9 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -152,7 +152,7 @@ test_expect_success 'push with 
receive.fsck.missingEmail=warn' '
git --git-dir=dst/.git config --add \
receive.fsck.badDate warn &&
git push --porcelain dst bogus >act 2>&1 &&
-   test_must_fail grep "missingEmail" act
+   ! grep "missingEmail" act
 '
 
 test_expect_success \
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 26b2cafc4..0fc5a7c59 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1004,7 +1004,7 @@ test_expect_success 'push --porcelain' '
 test_expect_success 'push --porcelain bad url' '
mk_empty testrepo &&
test_must_fail git push >.git/bar --porcelain asdfasdfasd 
refs/heads/master:refs/remotes/origin/master &&
-   test_must_fail grep -q Done .git/bar
+   ! grep -q Done .git/bar
 '
 
 test_expect_success 'push --porcelain rejected' '
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index a43339420..4241ea5b3 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -151,7 +151,7 @@ test_expect_success 'clone --mirror does not repeat tags' '
git clone --mirror src mirror2 &&
(cd mirror2 &&
 git show-ref 2> clone.err > clone.out) &&
-   test_must_fail grep Duplicate mirror2/clone.err &&
+   ! grep Duplicate mirror2/clone.err &&
grep some-tag mirror2/clone.out
 
 '
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 5e5370feb..8c2c6eaef 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -407,7 +407,7 @@ test_expect_success 'good merge base when good and bad are 
siblings' '
test_i18ngrep "merge base must be tested" my_bisect_log.txt &&
grep $HASH4 my_bisect_log.txt &&
git bisect good > my_bisect_log.txt &&
-   test_must_fail grep "merge base must be tested" my_bisect_log.txt &&
+   ! grep "merge base must be tested" my_bisect_log.txt &&
grep $HASH6 my_bisect_log.txt &&
git bisect reset
 '
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 63d36fb28..0fe7e58cf 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -602,7 +602,7 @@ test_expect_success MKTEMP 'temporary filenames are used 
with mergetool.writeToT
test_config mergetool.myecho.trustExitCode true &&
test_must_fail git merge master &&
git mergetool --no-prompt --tool myecho -- both >actual &&
-   test_must_fail grep ^\./both_LOCAL_ actual >/dev/null &&
+   ! grep ^\./both_LOCAL_ actual >/dev/null &&
grep /both_LOCAL_ actual >/dev/null &&
git reset --hard master >/dev/null 2>&1
 '
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 3dc4a3454..0f398dd16 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -50,7 +50,7 @@ test_no_confirm () {

Re: [PATCH v3 21/23] try_remove_empty_parents(): don't accommodate consecutive slashes

[Jeff King]

On Mon, Jan 02, 2017 at 07:06:32PM +0100, Michael Haggerty wrote:

> My assumption was that only valid reference names should ever be allowed
> to be inserted into a `ref_transaction` entry. But Peff is right that
> sometimes the reference name is checked by `refname_is_safe()` rather
> than `check_refname_format()`. Let's audit this more carefully...
> 
> * `ref_transaction_add_update()` relies on its caller doing the check
>   (this fact is documented). Its callers are:
>   * `ref_transaction_update()` (the usual codepath), which checks the
> reference itself using either check_refname_format() or
> refname_is_safe() depending on what kind of update it is.
>   * `split_head_update()` passes the literal string "HEAD".
>   * `split_symref_update()` picks apart reference updates that go
> through existing symbolic references. As such I don't think they
> are an attack surface. It doesn't do any checking itself (here
> we're talking about its `referent` argument). It has only one
> caller:
> * `lock_ref_for_update()`, which gets `referent` from:
>   * `files_read_raw_ref()`, which gets the value either:
> * by reading a filesystem-level symlink's contents and
>   checking it with `check_refname_format()`, or
> * reading a symref from the filesystem. In this case, *the
>   value is not checked*.
> 
> Obviously this chain of custody is disconcertingly long and complicated.
> And the gap for symrefs should probably be fixed, even though they are
> hopefully trustworthy.

Thanks as always for a careful analysis. I agree it seems like a bug
that symlinks are checked but symrefs are not.

> I think the best thing to do is to drop this patch from the patch
> series, and address these checks in a separate series. (There are more
> known problems in this area, for example that the checks in
> `check_refname_format()` are not a strict superset of the checks in
> `refname_is_safe()`.)

Sounds like a good plan. I'd be very happy if the "superset" mismatch is
fixed. It seems like it has come up in our discussions more than once.

-Peff

Re: [PATCH 14/17] sha1_file: introduce an nth_packed_object_oid function

[Jeff King]

On Mon, Jan 02, 2017 at 07:18:58PM +0100, Michael Haggerty wrote:

> > Given that this patch only converts one caller, I'd be more inclined to
> > just have the caller do its own hashcpy. Like:
> > 
> > diff --git a/sha1_file.c b/sha1_file.c
> > index 1173071859..16345688b5 100644
> > --- a/sha1_file.c
> > +++ b/sha1_file.c
> > @@ -3769,12 +3769,14 @@ static int for_each_object_in_pack(struct 
> > packed_git *p, each_packed_object_fn c
> >  
> > for (i = 0; i < p->num_objects; i++) {
> > const unsigned char *sha1 = nth_packed_object_sha1(p, i);
> > +   struct object_id oid;
> >  
> > if (!sha1)
> > return error("unable to get sha1 of object %u in %s",
> >  i, p->pack_name);
> >  
> > -   r = cb(sha1, p, i, data);
> > +   hashcpy(oid.hash, sha1);
> > +   r = cb(, p, i, data);
> > if (r)
> > break;
> > }
> > 
> > That punts on the issue for all the other callers, but like I said, I'm
> > not quite sure if, when, and how it would make sense to convert them to
> > using a "struct oid".
> 
> Your change is not safe if any of the callback functions ("cb") tuck
> away a copy of the pointer that they are passed, expecting it to contain
> the same object id later.

I think it's generally a given that callback functions should not assume
the lifetime of parameters extend beyond the end of the callback. That
said, I didn't audit the callers (although I'm pretty sure I wrote all
of them myself in this particular case).

-Peff

Re: [PATCH 14/17] sha1_file: introduce an nth_packed_object_oid function

[Michael Haggerty]

On 01/02/2017 06:09 PM, Jeff King wrote:
> [...]
> But I think the thing that gives me the most pause is that the oid
> version of the function feels bolted-on. The nth_packed_object_sha1()
> function is there specifically to give access to the mmap'd pack-index
> data. And at least for now, that only stores sha1s, not any kind of
> struct. If and when it does learn about other hashes, I'm not sure if
> we're going to want a generic nth_packed_object_oid() function, or if
> the callers would need to handle the various cases specially.
> 
> Given that this patch only converts one caller, I'd be more inclined to
> just have the caller do its own hashcpy. Like:
> 
> diff --git a/sha1_file.c b/sha1_file.c
> index 1173071859..16345688b5 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -3769,12 +3769,14 @@ static int for_each_object_in_pack(struct packed_git 
> *p, each_packed_object_fn c
>  
>   for (i = 0; i < p->num_objects; i++) {
>   const unsigned char *sha1 = nth_packed_object_sha1(p, i);
> + struct object_id oid;
>  
>   if (!sha1)
>   return error("unable to get sha1 of object %u in %s",
>i, p->pack_name);
>  
> - r = cb(sha1, p, i, data);
> + hashcpy(oid.hash, sha1);
> + r = cb(, p, i, data);
>   if (r)
>   break;
>   }
> 
> That punts on the issue for all the other callers, but like I said, I'm
> not quite sure if, when, and how it would make sense to convert them to
> using a "struct oid".

Your change is not safe if any of the callback functions ("cb") tuck
away a copy of the pointer that they are passed, expecting it to contain
the same object id later.

Michael

Re: [PATCH v3 00/23] Delete directories left empty after ref deletion

[Michael Haggerty]

On 01/02/2017 05:19 AM, Jeff King wrote:
> On Sun, Jan 01, 2017 at 12:36:11PM -0800, Jacob Keller wrote:
> 
>> But how likely is it to end up with differing binaries running on the
>> exact same repository concurrently? Basically, I am trying to see
>> whether or not we could accidentally end up causing problems by trying
>> to race with other git processes that haven't yet been made safe
>> against race? Is the worst case only that some git operation would
>> fail and you would have to retry?
> 
> Yes, I think that is the worst case.
> 
> A more likely scenario might be something like a server accepting pushes
> or other ref updates from both JGit and regular git (or maybe libgit2
> and regular git).
> 
> IMHO it's not really worth worrying about too much. Certain esoteric
> setups might have a slightly higher chance of a pretty obscure race
> condition happening on a very busy repository. I hate to say "eh, ship
> it, we'll see if anybody complains". But I'd be surprised to get a
> single report about this.

I agree. I think these races would mostly affect busy hosting sites and
result in failed updates rather than corruption. And the races can
already occur whenever the user runs `git pack-refs`. By contrast, the
failure to delete empty directories is more likely to affect normal users.

That being said, if Junio wants to merge all but the last two patches in
one release, then merge the last two a release or two later, I have no
objections.

Michael

Re: [PATCH v3 21/23] try_remove_empty_parents(): don't accommodate consecutive slashes

[Michael Haggerty]

On 01/01/2017 06:59 AM, Jeff King wrote:
> On Sat, Dec 31, 2016 at 06:30:01PM -0800, Junio C Hamano wrote:
> 
>> Michael Haggerty  writes:
>>
>>> "refname" has already been checked by check_refname_format(), so it
>>> cannot have consecutive slashes.
>>
>> In the endgame state, this has two callers.  Both use what came in
>> the transaction->updates[] array.  Presumably "has already been
>> checked by check_refname_format()" says that whoever created entries
>> in that array must have called the function, but it would be helpful
>> to be more explicit here.
> 
> Hmm, yeah. This is called when we are deleting a ref, and I thought we
> explicitly _didn't_ do check_refname_format() when deleting, so that
> funny-named refs could be deleted. It's only is_refname_safe() that we
> must pass.
> 
> So I have no idea if that's a problem in the code or not, but it is at
> least not immediately obvious who is responsible for calling
> check_refname_format() here.

My assumption was that only valid reference names should ever be allowed
to be inserted into a `ref_transaction` entry. But Peff is right that
sometimes the reference name is checked by `refname_is_safe()` rather
than `check_refname_format()`. Let's audit this more carefully...

* `ref_transaction_add_update()` relies on its caller doing the check
  (this fact is documented). Its callers are:
  * `ref_transaction_update()` (the usual codepath), which checks the
reference itself using either check_refname_format() or
refname_is_safe() depending on what kind of update it is.
  * `split_head_update()` passes the literal string "HEAD".
  * `split_symref_update()` picks apart reference updates that go
through existing symbolic references. As such I don't think they
are an attack surface. It doesn't do any checking itself (here
we're talking about its `referent` argument). It has only one
caller:
* `lock_ref_for_update()`, which gets `referent` from:
  * `files_read_raw_ref()`, which gets the value either:
* by reading a filesystem-level symlink's contents and
  checking it with `check_refname_format()`, or
* reading a symref from the filesystem. In this case, *the
  value is not checked*.

Obviously this chain of custody is disconcertingly long and complicated.
And the gap for symrefs should probably be fixed, even though they are
hopefully trustworthy.

`refname_is_safe()` checks that its argument is either UPPER_CASE or
looks like a plausible filename under "refs/". Contrary to its
docstring, it *does not* accept strings that contain successive slashes
or "." or ".." components. It was made stricter in

e40f355 "refname_is_safe(): insist that the refname already be
normalized", 2016-04-27

, but the docstring wasn't updated at that time. I'll fix it.

I think the best thing to do is to drop this patch from the patch
series, and address these checks in a separate series. (There are more
known problems in this area, for example that the checks in
`check_refname_format()` are not a strict superset of the checks in
`refname_is_safe()`.)

Michael

Re: [PATCH v3 20/23] try_remove_empty_parents(): don't trash argument contents

[Jeff King]

On Mon, Jan 02, 2017 at 05:27:59PM +0100, Michael Haggerty wrote:

> > or something. But I doubt the single allocation is breaking the bank,
> > and it has the nice side effect that callers can pass in a const string
> > (I didn't check yet whether that enables further cleanups).
> 
> The last patch in the series passes ref_update::refname to this
> function, which is `const char *`. With your suggested change, either
> that member would have to be made non-const, or it would have to be cast
> to const at the `try_remove_empty_parents()` callsite.
> 
> Making the member non-const would be easy, though it loses a tiny bit of
> documentation and safety against misuse. Also, scribbling even
> temporarily over that member would mean that this code is not
> thread-safe (though it seems unlikely that we would ever bother making
> it multithreaded).
> 
> I think I prefer the current version because it loosens the coupling
> between this function and its callers. But I don't mind either way if
> somebody feels strongly about it.

OK, let's take what you have here, then.

> As an aside, I wonder whether we would be discussing this at all if we
> had stack-allocated strbufs that could be used without allocating heap
> memory in the usual case.

I'm not sure. We still pay the memcpy(), though I don't know how
substantial that is compared to an allocation. For these small strings,
probably not very.

-Peff

Re: [PATCH 14/17] sha1_file: introduce an nth_packed_object_oid function

[Jeff King]

On Mon, Jan 02, 2017 at 04:30:21PM +0100, Michael Haggerty wrote:

> On 01/01/2017 08:18 PM, brian m. carlson wrote:
> > There are places in the code where we would like to provide a struct
> > object_id *, yet read the hash directly from the pack.  Provide an
> > nth_packed_object_oid function that mirrors the nth_packed_object_sha1
> > function.
> > 
> > The required cast is questionable, but should be safe on all known
> > platforms.  The alternative of allocating an object as an intermediate
> > would be too inefficient and cause memory leaks.  If necessary, an
> > intermediate union could be used; this practice is allowed by GCC and
> > explicitly sanctioned by C11.  However, such a change will likely not be
> > necessary, and can be made if and when it is.
> 
> I have the feeling that this design decision has been discussed on the
> mailing list. If so, could you include a URL here?

I don't recall an explicit discussion, but I think we already do this
for similar reasons in decode_tree_entry(), where we point to the sha1s
embedded in the tree buffer (of course, you may argue that the cast
there is gross, too :) ).

I'm actually not sure how bad this cast is by the C standard. Conversion
between a struct and its first member is pretty common, and the
alignment is guaranteed by the standard. I think it probably breaks
strict aliasing rules, but then so does "struct object". This is
slightly more exotic in that it's not a struct-to-struct cast, and I
could believe that compilers treat those specially.

> The obvious alternative to allocating a new object to return to the
> caller would be to have the caller of `nth_packed_object_oid()` pass a
> `struct object_id *` argument to be filled in (by copying the hash into
> it). Aside from being legal C, this would also be a more robust step
> towards a post-SHA-1 world, where the suggested hack wouldn't work.
> 
> Of course, the question is what the callers want to do with the
> `object_id`. Are the return values of `nth_packed_object_sha1()` stored
> to other longer-lived structures that rely on the lifetime of the
> packfile mmap to keep the value valid? If so, then keeping track of the
> lifetime of the `struct object_id` could be a big chore, not to mention
> that needing to keep a 20-byte `struct object_id` around rather than a
> 8- or 4-byte pointer would also cost more RAM.

I agree that in general, copying the values out is a safer and saner
interface. There are definitely lifetime and memory-use issues to
consider (e.g., the loop in verify_packfile()). And I'd have a slight
worry that the performance impact might be noticeable, just because this
is really quite a low-level function that gets called a lot (but of
course measuring trumps everything there).

But I think the thing that gives me the most pause is that the oid
version of the function feels bolted-on. The nth_packed_object_sha1()
function is there specifically to give access to the mmap'd pack-index
data. And at least for now, that only stores sha1s, not any kind of
struct. If and when it does learn about other hashes, I'm not sure if
we're going to want a generic nth_packed_object_oid() function, or if
the callers would need to handle the various cases specially.

Given that this patch only converts one caller, I'd be more inclined to
just have the caller do its own hashcpy. Like:

diff --git a/sha1_file.c b/sha1_file.c
index 1173071859..16345688b5 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -3769,12 +3769,14 @@ static int for_each_object_in_pack(struct packed_git 
*p, each_packed_object_fn c
 
for (i = 0; i < p->num_objects; i++) {
const unsigned char *sha1 = nth_packed_object_sha1(p, i);
+   struct object_id oid;
 
if (!sha1)
return error("unable to get sha1 of object %u in %s",
 i, p->pack_name);
 
-   r = cb(sha1, p, i, data);
+   hashcpy(oid.hash, sha1);
+   r = cb(, p, i, data);
if (r)
break;
}

That punts on the issue for all the other callers, but like I said, I'm
not quite sure if, when, and how it would make sense to convert them to
using a "struct oid".

-Peff

Re: [PATCH v3 20/23] try_remove_empty_parents(): don't trash argument contents

[Michael Haggerty]

On 12/31/2016 07:40 AM, Jeff King wrote:
> On Sat, Dec 31, 2016 at 04:13:00AM +0100, Michael Haggerty wrote:
> 
>> It's bad manners and surprising and therefore error-prone.
> 
> Agreed.
> 
> I wondered while reading your earlier patch whether the
> safe_create_leading_directories() function had the same problem, but it
> carefully replaces the NUL it inserts.
> 
>> -static void try_remove_empty_parents(char *refname)
>> +static void try_remove_empty_parents(const char *refname)
>>  {
>> +struct strbuf buf = STRBUF_INIT;
>>  char *p, *q;
>>  int i;
>> -p = refname;
>> +
>> +strbuf_addstr(, refname);
> 
> I see here you just make a copy. I think it would be enough to do:
> 
>> @@ -2305,10 +2306,11 @@ static void try_remove_empty_parents(char *refname)
>>  q--;
>>  if (q == p)
>>  break;
>> -*q = '\0';
>> -if (rmdir(git_path("%s", refname)))
>> +strbuf_setlen(, q - buf.buf);
>> +if (rmdir(git_path("%s", buf.buf)))
>>  break;
> 
>   *q = '\0';
>   r = rmdir(git_path("%s", refname));
>   *q = '/';
> 
>   if (r)
>   break;
> 
> or something. But I doubt the single allocation is breaking the bank,
> and it has the nice side effect that callers can pass in a const string
> (I didn't check yet whether that enables further cleanups).

The last patch in the series passes ref_update::refname to this
function, which is `const char *`. With your suggested change, either
that member would have to be made non-const, or it would have to be cast
to const at the `try_remove_empty_parents()` callsite.

Making the member non-const would be easy, though it loses a tiny bit of
documentation and safety against misuse. Also, scribbling even
temporarily over that member would mean that this code is not
thread-safe (though it seems unlikely that we would ever bother making
it multithreaded).

I think I prefer the current version because it loosens the coupling
between this function and its callers. But I don't mind either way if
somebody feels strongly about it.

As an aside, I wonder whether we would be discussing this at all if we
had stack-allocated strbufs that could be used without allocating heap
memory in the usual case.

Michael

[PATCH v4 4/4] t7800: run both builtin and scripted difftool, for now

[Johannes Schindelin]

This is uglier than a simple

touch "$GIT_EXEC_PATH/use-builtin-difftool"

of course. But oh well.

Signed-off-by: Johannes Schindelin 
---

This patch implements the good ole' cross-validation technique
(also known as "GitHub Scientist") that I already used for my
rebase--helper work.

I am not sure whether we want to have that patch in `master`,
ever. But at least for the transition time, it may make sense.

 t/t7800-difftool.sh | 27 +++
 1 file changed, 27 insertions(+)

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index e94910c563..273ab55723 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -23,6 +23,20 @@ prompt_given ()
test "$prompt" = "Launch 'test-tool' [Y/n]? branch"
 }
 
+for use_builtin_difftool in false true
+do
+
+test_expect_success 'verify we are running the correct difftool' '
+   if test true = '$use_builtin_difftool'
+   then
+   test_must_fail ok=129 git difftool -h >help &&
+   grep "g, --gui" help
+   else
+   git difftool -h >help &&
+   grep "g|--gui" help
+   fi
+'
+
 # NEEDSWORK: lose all the PERL prereqs once legacy-difftool is retired.
 
 # Create a file on master and change it on branch
@@ -606,4 +620,17 @@ test_expect_success PERL,SYMLINKS 'difftool --dir-diff 
symlinked directories' '
)
 '
 
+test true != $use_builtin_difftool || break
+
+test_expect_success 'tear down for re-run' '
+   rm -rf * .[a-z]* &&
+   git init
+'
+
+# run as builtin difftool now
+GIT_CONFIG_PARAMETERS="'difftool.usebuiltin=true'"
+export GIT_CONFIG_PARAMETERS
+
+done
+
 test_done
-- 
2.11.0.rc3.windows.1

[PATCH v4 3/4] difftool: implement the functionality in the builtin

[Johannes Schindelin]

This patch gives life to the skeleton added in the previous patch.

The motivation for converting the difftool is that Perl scripts are not at
all native on Windows, and that `git difftool` therefore is pretty slow on
that platform, when there is no good reason for it to be slow.

In addition, Perl does not really have access to Git's internals. That
means that any script will always have to jump through unnecessary
hoops.

The current version of the builtin difftool does not, however, make full
use of the internals but instead chooses to spawn a couple of Git
processes, still, to make for an easier conversion. There remains a lot
of room for improvement, left for a later date.

Note: to play it safe, the original difftool is still called unless the
config setting difftool.useBuiltin is set to true.

The reason: this new, experimental, builtin difftool will be shipped as
part of Git for Windows v2.11.0, to allow for easier large-scale
testing, but of course as an opt-in feature.

Sadly, the speedup is more noticable on Linux than on Windows: a quick
test shows that t7800-difftool.sh runs in (2.183s/0.052s/0.108s)
(real/user/sys) in a Linux VM, down from  (6.529s/3.112s/0.644s), while
on Windows, it is (36.064s/2.730s/7.194s), down from
(47.637s/2.407s/6.863s). The culprit is most likely the overhead
incurred from *still* having to shell out to mergetool-lib.sh and
difftool--helper.sh.

Signed-off-by: Johannes Schindelin 
---
 builtin/difftool.c | 672 -
 1 file changed, 671 insertions(+), 1 deletion(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index 53870bbaf7..2115e548a5 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -11,9 +11,610 @@
  *
  * Copyright (C) 2016 Johannes Schindelin
  */
+#include "cache.h"
 #include "builtin.h"
 #include "run-command.h"
 #include "exec_cmd.h"
+#include "parse-options.h"
+#include "argv-array.h"
+#include "strbuf.h"
+#include "lockfile.h"
+#include "dir.h"
+
+static char *diff_gui_tool;
+static int trust_exit_code;
+
+static const char *const builtin_difftool_usage[] = {
+   N_("git difftool [] [ []] [--] [...]"),
+   NULL
+};
+
+static int difftool_config(const char *var, const char *value, void *cb)
+{
+   if (!strcmp(var, "diff.guitool")) {
+   diff_gui_tool = xstrdup(value);
+   return 0;
+   }
+
+   if (!strcmp(var, "difftool.trustexitcode")) {
+   trust_exit_code = git_config_bool(var, value);
+   return 0;
+   }
+
+   return git_default_config(var, value, cb);
+}
+
+static int print_tool_help(void)
+{
+   const char *argv[] = { "mergetool", "--tool-help=diff", NULL };
+   return run_command_v_opt(argv, RUN_GIT_CMD);
+}
+
+static int parse_index_info(char *p, int *mode1, int *mode2,
+   struct object_id *oid1, struct object_id *oid2,
+   char *status)
+{
+   if (*p != ':')
+   return error("expected ':', got '%c'", *p);
+   *mode1 = (int)strtol(p + 1, , 8);
+   if (*p != ' ')
+   return error("expected ' ', got '%c'", *p);
+   *mode2 = (int)strtol(p + 1, , 8);
+   if (*p != ' ')
+   return error("expected ' ', got '%c'", *p);
+   if (get_oid_hex(++p, oid1))
+   return error("expected object ID, got '%s'", p + 1);
+   p += GIT_SHA1_HEXSZ;
+   if (*p != ' ')
+   return error("expected ' ', got '%c'", *p);
+   if (get_oid_hex(++p, oid2))
+   return error("expected object ID, got '%s'", p + 1);
+   p += GIT_SHA1_HEXSZ;
+   if (*p != ' ')
+   return error("expected ' ', got '%c'", *p);
+   *status = *++p;
+   if (!*status)
+   return error("missing status");
+   if (p[1] && !isdigit(p[1]))
+   return error("unexpected trailer: '%s'", p + 1);
+   return 0;
+}
+
+/*
+ * Remove any trailing slash from $workdir
+ * before starting to avoid double slashes in symlink targets.
+ */
+static void add_path(struct strbuf *buf, size_t base_len, const char *path)
+{
+   strbuf_setlen(buf, base_len);
+   if (buf->len && buf->buf[buf->len - 1] != '/')
+   strbuf_addch(buf, '/');
+   strbuf_addstr(buf, path);
+}
+
+/*
+ * Determine whether we can simply reuse the file in the worktree.
+ */
+static int use_wt_file(const char *workdir, const char *name,
+  struct object_id *oid)
+{
+   struct strbuf buf = STRBUF_INIT;
+   struct stat st;
+   int use = 0;
+
+   strbuf_addstr(, workdir);
+   add_path(, buf.len, name);
+
+   if (!lstat(buf.buf, ) && !S_ISLNK(st.st_mode)) {
+   struct object_id wt_oid;
+   int fd = open(buf.buf, O_RDONLY);
+
+   if (fd >= 0 &&
+   !index_fd(wt_oid.hash, fd, , OBJ_BLOB, name, 0)) {
+   if (is_null_oid(oid)) {
+   

[PATCH v4 1/4] Avoid Coverity warning about unfree()d git_exec_path()

[Johannes Schindelin]

Technically, it is correct that git_exec_path() returns a possibly
malloc()ed string. Practically, it is *sometimes* not malloc()ed. So
let's just use a static variable to make it a singleton. That'll shut
Coverity up, hopefully.

Signed-off-by: Johannes Schindelin 
---
 exec_cmd.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/exec_cmd.c b/exec_cmd.c
index 19ac2146d0..587bd7eb48 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -65,6 +65,7 @@ void git_set_argv_exec_path(const char *exec_path)
 const char *git_exec_path(void)
 {
const char *env;
+   static char *system_exec_path;
 
if (argv_exec_path)
return argv_exec_path;
@@ -74,7 +75,9 @@ const char *git_exec_path(void)
return env;
}
 
-   return system_path(GIT_EXEC_PATH);
+   if (!system_exec_path)
+   system_exec_path = system_path(GIT_EXEC_PATH);
+   return system_exec_path;
 }
 
 static void add_path(struct strbuf *out, const char *path)
-- 
2.11.0.rc3.windows.1

[PATCH v4 2/4] difftool: add a skeleton for the upcoming builtin

[Johannes Schindelin]

This adds a builtin difftool that still falls back to the legacy Perl
version, which has been renamed to `legacy-difftool`.

The idea is that the new, experimental, builtin difftool immediately hands
off to the legacy difftool for now, unless the config variable
difftool.useBuiltin is set to true.

This feature flag will be used in the upcoming Git for Windows v2.11.0
release, to allow early testers to opt-in to use the builtin difftool and
flesh out any bugs.

Signed-off-by: Johannes Schindelin 
---
 .gitignore|  1 +
 Makefile  |  3 +-
 builtin.h |  1 +
 builtin/difftool.c| 63 +++
 git-difftool.perl => git-legacy-difftool.perl |  0
 git.c |  6 +++
 t/t7800-difftool.sh   |  2 +
 7 files changed, 75 insertions(+), 1 deletion(-)
 create mode 100644 builtin/difftool.c
 rename git-difftool.perl => git-legacy-difftool.perl (100%)

diff --git a/.gitignore b/.gitignore
index 6722f78f9a..ae025b 100644
--- a/.gitignore
+++ b/.gitignore
@@ -76,6 +76,7 @@
 /git-init-db
 /git-interpret-trailers
 /git-instaweb
+/git-legacy-difftool
 /git-log
 /git-ls-files
 /git-ls-remote
diff --git a/Makefile b/Makefile
index d861bd9985..8cf5bef034 100644
--- a/Makefile
+++ b/Makefile
@@ -522,7 +522,7 @@ SCRIPT_LIB += git-sh-setup
 SCRIPT_LIB += git-sh-i18n
 
 SCRIPT_PERL += git-add--interactive.perl
-SCRIPT_PERL += git-difftool.perl
+SCRIPT_PERL += git-legacy-difftool.perl
 SCRIPT_PERL += git-archimport.perl
 SCRIPT_PERL += git-cvsexportcommit.perl
 SCRIPT_PERL += git-cvsimport.perl
@@ -883,6 +883,7 @@ BUILTIN_OBJS += builtin/diff-files.o
 BUILTIN_OBJS += builtin/diff-index.o
 BUILTIN_OBJS += builtin/diff-tree.o
 BUILTIN_OBJS += builtin/diff.o
+BUILTIN_OBJS += builtin/difftool.o
 BUILTIN_OBJS += builtin/fast-export.o
 BUILTIN_OBJS += builtin/fetch-pack.o
 BUILTIN_OBJS += builtin/fetch.o
diff --git a/builtin.h b/builtin.h
index b9122bc5f4..67f80519da 100644
--- a/builtin.h
+++ b/builtin.h
@@ -60,6 +60,7 @@ extern int cmd_diff_files(int argc, const char **argv, const 
char *prefix);
 extern int cmd_diff_index(int argc, const char **argv, const char *prefix);
 extern int cmd_diff(int argc, const char **argv, const char *prefix);
 extern int cmd_diff_tree(int argc, const char **argv, const char *prefix);
+extern int cmd_difftool(int argc, const char **argv, const char *prefix);
 extern int cmd_fast_export(int argc, const char **argv, const char *prefix);
 extern int cmd_fetch(int argc, const char **argv, const char *prefix);
 extern int cmd_fetch_pack(int argc, const char **argv, const char *prefix);
diff --git a/builtin/difftool.c b/builtin/difftool.c
new file mode 100644
index 00..53870bbaf7
--- /dev/null
+++ b/builtin/difftool.c
@@ -0,0 +1,63 @@
+/*
+ * "git difftool" builtin command
+ *
+ * This is a wrapper around the GIT_EXTERNAL_DIFF-compatible
+ * git-difftool--helper script.
+ *
+ * This script exports GIT_EXTERNAL_DIFF and GIT_PAGER for use by git.
+ * The GIT_DIFF* variables are exported for use by git-difftool--helper.
+ *
+ * Any arguments that are unknown to this script are forwarded to 'git diff'.
+ *
+ * Copyright (C) 2016 Johannes Schindelin
+ */
+#include "builtin.h"
+#include "run-command.h"
+#include "exec_cmd.h"
+
+/*
+ * NEEDSWORK: this function can go once the legacy-difftool Perl script is
+ * retired.
+ *
+ * We intentionally avoid reading the config directly here, to avoid messing up
+ * the GIT_* environment variables when we need to fall back to exec()ing the
+ * Perl script.
+ */
+static int use_builtin_difftool(void) {
+   struct child_process cp = CHILD_PROCESS_INIT;
+   struct strbuf out = STRBUF_INIT;
+   int ret;
+
+   argv_array_pushl(,
+"config", "--bool", "difftool.usebuiltin", NULL);
+   cp.git_cmd = 1;
+   if (capture_command(, , 6))
+   return 0;
+   strbuf_trim();
+   ret = !strcmp("true", out.buf);
+   strbuf_release();
+   return ret;
+}
+
+int cmd_difftool(int argc, const char **argv, const char *prefix)
+{
+   /*
+* NEEDSWORK: Once the builtin difftool has been tested enough
+* and git-legacy-difftool.perl is retired to contrib/, this preamble
+* can be removed.
+*/
+   if (!use_builtin_difftool()) {
+   const char *path = mkpath("%s/git-legacy-difftool",
+ git_exec_path());
+
+   if (sane_execvp(path, (char **)argv) < 0)
+   die_errno("could not exec %s", path);
+
+   return 0;
+   }
+   prefix = setup_git_directory();
+   trace_repo_setup(prefix);
+   setup_work_tree();
+
+   die("TODO");
+}
diff --git a/git-difftool.perl b/git-legacy-difftool.perl
similarity index 100%
rename from git-difftool.perl

[PATCH v4 0/4] Show Git Mailing List: a builtin difftool

[Johannes Schindelin]

I have been working on the builtin difftool for a while now, for two
reasons:

1. Perl is really not native on Windows. Not only is there a performance
   penalty to be paid just for running Perl scripts, we also have to deal
   with the fact that users may have different Perl installations, with
   different options, and some other Perl installation may decide to set
   PERL5LIB globally, wreaking havoc with Git for Windows' Perl (which we
   have to use because almost all other Perl distributions lack the
   Subversion bindings we need for `git svn`).

2. Perl makes for a rather large reason that Git for Windows' installer
   weighs in with >30MB. While one Perl script less does not relieve us
   of that burden, it is one step in the right direction.

Changes since v3:

- made path_entry_cmp static

- fixed a few bugs identified by Coverity

- fixed overzealous status parsing that did not expect any number after
  C or R


Johannes Schindelin (4):
  Avoid Coverity warning about unfree()d git_exec_path()
  difftool: add a skeleton for the upcoming builtin
  difftool: implement the functionality in the builtin
  t7800: run both builtin and scripted difftool, for now

 .gitignore|   1 +
 Makefile  |   3 +-
 builtin.h |   1 +
 builtin/difftool.c| 733 ++
 exec_cmd.c|   5 +-
 git-difftool.perl => git-legacy-difftool.perl |   0
 git.c |   6 +
 t/t7800-difftool.sh   |  29 +
 8 files changed, 776 insertions(+), 2 deletions(-)
 create mode 100644 builtin/difftool.c
 rename git-difftool.perl => git-legacy-difftool.perl (100%)


base-commit: e05806da9ec4aff8adfed142ab2a2b3b02e33c8c
Published-As: https://github.com/dscho/git/releases/tag/builtin-difftool-v4
Fetch-It-Via: git fetch https://github.com/dscho/git builtin-difftool-v4

Interdiff vs v3:

 diff --git a/builtin/difftool.c b/builtin/difftool.c
 index 3480920fea..2115e548a5 100644
 --- a/builtin/difftool.c
 +++ b/builtin/difftool.c
 @@ -73,8 +73,10 @@ static int parse_index_info(char *p, int *mode1, int *mode2,
if (*p != ' ')
return error("expected ' ', got '%c'", *p);
*status = *++p;
 -  if (!status || p[1])
 -  return error("unexpected trailer: '%s'", p);
 +  if (!*status)
 +  return error("missing status");
 +  if (p[1] && !isdigit(p[1]))
 +  return error("unexpected trailer: '%s'", p + 1);
return 0;
  }
  
 @@ -107,7 +109,8 @@ static int use_wt_file(const char *workdir, const char 
*name,
struct object_id wt_oid;
int fd = open(buf.buf, O_RDONLY);
  
 -  if (!index_fd(wt_oid.hash, fd, , OBJ_BLOB, name, 0)) {
 +  if (fd >= 0 &&
 +  !index_fd(wt_oid.hash, fd, , OBJ_BLOB, name, 0)) {
if (is_null_oid(oid)) {
oidcpy(oid, _oid);
use = 1;
 @@ -162,7 +165,7 @@ static void add_left_or_right(struct hashmap *map, const 
char *path,
e->left[0] = e->right[0] = '\0';
hashmap_add(map, e);
}
 -  strcpy(is_right ? e->right : e->left, content);
 +  strlcpy(is_right ? e->right : e->left, content, PATH_MAX);
  }
  
  struct path_entry {
 @@ -170,7 +173,7 @@ struct path_entry {
char path[FLEX_ARRAY];
  };
  
 -int path_entry_cmp(struct path_entry *a, struct path_entry *b, void *key)
 +static int path_entry_cmp(struct path_entry *a, struct path_entry *b, void 
*key)
  {
return strcmp(a->path, key ? key : b->path);
  }
 @@ -423,17 +426,16 @@ static int run_dir_diff(const char *extcmd, int 
symlinks, const char *prefix,
struct cache_entry *ce2 =
make_cache_entry(rmode, roid.hash,
 dst_path, 0, 0);
 -  ce_mode_from_stat(ce2, rmode);
  
add_index_entry(, ce2,
ADD_CACHE_JUST_APPEND);
  
 -  add_path(, wtdir_len, dst_path);
add_path(, rdir_len, dst_path);
if (ensure_leading_directories(rdir.buf))
return error("could not create "
 "directory for '%s'",
 dst_path);
 +  add_path(, wtdir_len, dst_path);
if (symlinks) {
if (symlink(wtdir.buf, rdir.buf)) {
ret = error_errno("could not 
symlink '%s' to '%s'", wtdir.buf, 

Re: builtin difftool parsing issue

[Johannes Schindelin]

Hi Paul,

On Wed, 21 Dec 2016, Paul Sbarra wrote:

> Sadly, I haven't been able to figure out how to get the mbox file from
> this tread into gmail, but wanted to report a parsing issue I've found
> with the builtin difftool.
> 
> Original Patch:
> https://public-inbox.org/git/ac91e4818cfb5c5af6b5874662dbeb61cde1f69d.1480019834.git.johannes.schinde...@gmx.de/#t
> 
> > + *status = *++p;
> > + if (!status || p[1])
> > + return error("unexpected trailer: '%s'", p);
> > + return 0;
> 
> The p[1] null check assumes the status is only one character long, but
> git-diff's raw output format shows that a numeric value can follow in
> the copy-edit and rename-edit cases.

Thank you for the report! I fixed it locally.

> I'm looking forward to seeing the builtin difftool land.  I came across it
> while investigating adding --submodule=diff (expanding on diff's
> recent addition) support and this looks more promising then the perl
> script.  Hopefully I will make some progress.  Any tips/pointers would
> be greatly appreciated.

I would have expected `git difftool --submodule=diff ...` to work... What
are the problems?

Ciao,
Johannes

[PATCH 2/2] giteveryday: unbreak rendering with AsciiDoctor

[Johannes Schindelin]

The "giteveryday" document has a callout list that contains a code
block. This is not a problem for AsciiDoc, but AsciiDoctor sadly was
explicitly designed *not* to render this correctly [*1*]. The symptom is
an unhelpful

line 322: callout list item index: expected 1 got 12
line 325: no callouts refer to list item 1
line 325: callout list item index: expected 2 got 13
line 327: no callouts refer to list item 2

In Git for Windows, we rely on the speed improvement of AsciiDoctor (on
this developer's machine, `make -j15 html` takes roughly 30 seconds with
AsciiDoctor, 70 seconds with AsciiDoc), therefore we need a way to
render this correctly.

The easiest way out is to simplify the callout list, as suggested by
AsciiDoctor's author, even while one may very well disagree with him
that a code block hath no place in a callout list.

*1*: https://github.com/asciidoctor/asciidoctor/issues/1478

Signed-off-by: Johannes Schindelin 
---
 Documentation/giteveryday.txt | 17 +
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/Documentation/giteveryday.txt b/Documentation/giteveryday.txt
index 35473ad02f..10c8ff93c0 100644
--- a/Documentation/giteveryday.txt
+++ b/Documentation/giteveryday.txt
@@ -307,9 +307,16 @@ master or exposed as a part of a stable branch.
 <9> backport a critical fix.
 <10> create a signed tag.
 <11> make sure master was not accidentally rewound beyond that
-already pushed out.  `ko` shorthand points at the Git maintainer's
+already pushed out.
+<12> In the output from `git show-branch`, `master` should have
+everything `ko/master` has, and `next` should have
+everything `ko/next` has, etc.
+<13> push out the bleeding edge, together with new tags that point
+into the pushed history.
+
+In this example, the `ko` shorthand points at the Git maintainer's
 repository at kernel.org, and looks like this:
-+
+
 
 (in .git/config)
 [remote "ko"]
@@ -320,12 +327,6 @@ repository at kernel.org, and looks like this:
push = +refs/heads/pu
push = refs/heads/maint
 
-+
-<12> In the output from `git show-branch`, `master` should have
-everything `ko/master` has, and `next` should have
-everything `ko/next` has, etc.
-<13> push out the bleeding edge, together with new tags that point
-into the pushed history.
 
 
 Repository Administration[[ADMINISTRATION]]
-- 
2.11.0.rc3.windows.1

[PATCH 1/2] asciidoctor: fix user-manual to be built by `asciidoctor`

[Johannes Schindelin]

From: =?UTF-8?q?=EB=A7=88=EB=88=84=EC=97=98?= 

The `user-manual.txt` is designed as a `book` but the `Makefile` wants
to build it as an `article`. This seems to be a problem when building
the documentation with `asciidoctor`. Furthermore the parts *Git
Glossary* and *Appendix B* had no subsections which is not allowed when
building with `asciidoctor`. So lets add a *dummy* section.

Signed-off-by: 마누엘 
Signed-off-by: Johannes Schindelin 
---
 Documentation/Makefile| 2 +-
 Documentation/user-manual.txt | 8 
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/Documentation/Makefile b/Documentation/Makefile
index b43d66eae6..a9fb497b83 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -337,7 +337,7 @@ manpage-base-url.xsl: manpage-base-url.xsl.in
 
 user-manual.xml: user-manual.txt user-manual.conf
$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
-   $(TXT_TO_XML) -d article -o $@+ $< && \
+   $(TXT_TO_XML) -d book -o $@+ $< && \
mv $@+ $@
 
 technical/api-index.txt: technical/api-index-skel.txt \
diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt
index 5e07454572..bc29298678 100644
--- a/Documentation/user-manual.txt
+++ b/Documentation/user-manual.txt
@@ -4395,6 +4395,10 @@ itself!
 Git Glossary
 
 
+[[git-explained]]
+Git explained
+-
+
 include::glossary-content.txt[]
 
 [[git-quick-start]]
@@ -4636,6 +4640,10 @@ $ git gc
 Appendix B: Notes and todo list for this manual
 ===
 
+[[todo-list]]
+Todo list
+-
+
 This is a work in progress.
 
 The basic requirements:
-- 
2.11.0.rc3.windows.1

Re: [PATCH v3 21/21] Documentation/git-update-index: explain splitIndex.*

[Christian Couder]

On Tue, Dec 27, 2016 at 8:13 PM, Junio C Hamano  wrote:
> Christian Couder  writes:
>
>>  --split-index::
>>  --no-split-index::
>> - Enable or disable split index mode. If enabled, the index is
>> - split into two files, $GIT_DIR/index and $GIT_DIR/sharedindex..
>> - Changes are accumulated in $GIT_DIR/index while the shared
>> - index file contains all index entries stays unchanged. If
>> - split-index mode is already enabled and `--split-index` is
>> - given again, all changes in $GIT_DIR/index are pushed back to
>> - the shared index file. This mode is designed for very large
>> - indexes that take a significant amount of time to read or write.
>> + Enable or disable split index mode. If split-index mode is
>> + already enabled and `--split-index` is given again, all
>> + changes in $GIT_DIR/index are pushed back to the shared index
>> + file.
>
> In the world after this series that introduced the percentage-based
> auto consolidation, it smells strange, or even illogical, that index
> is un-split after doing this:
>
> $ git update-index --split-index
> $ git update-index --split-index
>
> Before this series, it may have been a quick and dirty way to force
> consolidating the split index without totally disabling the feature,
> i.e. it would have looked more like
>
> $ git update-index --split-index
> ... work work work to accumulate more modifications
> ... consolidate into one --- there was no other way than
> ... disabling it temporarily
> $ git update-index --no-split-index
> ... but the user likes the feature so re-enable it.
> $ git update-index --split-index
>
> which I guess is where this strange behaviour comes from.
>
> It may be something we need to fix to unconfuse the end-users after
> this series lands.  Even though "first disable and then re-enable"
> takes two commands (as opposed to one), it is far more logical.  And
> the percentage-based auto consolidation feature is meant to reduce
> the need for manual consolidation, so it probably makes sense to
> remove this illogical feature.

Yeah, I tend to agree that this feature could be removed later. Though
before removing it, I'd like to hear Duy's opinion on this as he
created the feature in the first place.

>> @@ -394,6 +390,31 @@ Although this bit looks similar to assume-unchanged 
>> bit, its goal is
>>  different from assume-unchanged bit's. Skip-worktree also takes
>>  precedence over assume-unchanged bit when both are set.
>>
>> +Split index
>> +---
>> +
>> +This mode is designed for very large indexes that take a significant
>> +amount of time to read or write.
>
> This is not a new problem, but it probably is incorrect to say "to
> read or write".  It saves time by not rewriting the whole thing but
> instead write out only the updated bits.  You'd still read the whole
> thing while populating the in-core index from the disk, and if
> anything, you'd probably spend _more_ cycles because you'd essentially
> be reading the base and then reading the delta to apply on top.

Ok, then what about:

+This mode is designed for repositories with very large indexes, and aims
+at reducing the time it takes to repeatedly write these indexes.

>> +To avoid deleting a shared index file that is still used, its mtime is
>> +updated to the current time everytime a new split index based on the
>> +shared index file is either created or read from.
>
> The same comment on the mention of "mtime" in another patch applies
> here as well.

Ok, I will use "modification time" instead of "mtime".

Thanks.

[PATCH 0/2] Fix the documentation to build with asciidoctor

[Johannes Schindelin]

The first patch in this series has been with Git for Windows for well
over a year already, the second one is a replacement for such an old
patch.

The reason why we use asciidoctor in Git for Windows is easy to see:
when building new Git for Windows packages, rendering the documentation
dominates the time by far. It takes roughly 75 seconds to compile all
the executables from scratch on my main work machine, but it takes
roughly 125 seconds to build the documentation from scratch.

Out of those 125 seconds, 45 are taken by the HTML documentation.

In the Git for Windows project, we realized a long time ago that we
could reduce the time dramatically by using asciidoctor: it takes only
15 seconds to build the HTML documentation.

Those savings come at a cost, though: asciidoctor was designed to be
much less flexible than asciidoc, in favor for maximum speed and minimal
size of the code base. And to accomodate those shortcomings, it is
unfortunately necessary to change Git's documentation sources.

So what is the big deal with those timings? It's only half a minute!

This is on a very beefy machine. Internally, I use quite a bit of
Continuous Integration to build intermediate Git for Windows versions
for testing, and those builds are backed by reasonably-sized VMs. That
makes it worth shaving that extra time off.

Side note for the curious who wonder why asciidoctor is *so much* faster
than asciidoc: my gut feeling is that the primary reason for this is,
once again, the POSIX emulation layer: asciidoc runs as a Python script,
using a Python interpreter that uses the MSYS2 runtime for path
translation and fork emulation (among other things), while asciidoctor
runs as a Ruby script, using a pure Windows Ruby interpreter (for those
remembering the nomenclature: the Python interpreter is an MSYS2 program
while the Ruby interpreter is a MINGW program). But even after that I
suspect that asciidoc was just not designed for speed, on a very
fundamental level.

Now back to the patch series: I *hope* the patches are not too
disruptive. I am very open to changing them however needed, I only care
about one result in the end: that the documentation builds correctly
(and fast) with asciidoctor.


Johannes Schindelin (1):
  giteveryday: unbreak rendering with AsciiDoctor

마누엘 (1):
  asciidoctor: fix user-manual to be built by `asciidoctor`

 Documentation/Makefile|  2 +-
 Documentation/giteveryday.txt | 17 +
 Documentation/user-manual.txt |  8 
 3 files changed, 18 insertions(+), 9 deletions(-)


base-commit: e05806da9ec4aff8adfed142ab2a2b3b02e33c8c
Published-As: https://github.com/dscho/git/releases/tag/asciidoctor-fixes-v1
Fetch-It-Via: git fetch https://github.com/dscho/git asciidoctor-fixes-v1

-- 
2.11.0.rc3.windows.1

Re: [PATCH 00/17] object_id part 6

[Michael Haggerty]

On 01/01/2017 08:18 PM, brian m. carlson wrote:
> This is another series in the continuing conversion to struct object_id.
> 
> This series converts more of the builtin directory and some of the
> refs code to use struct object_id. Additionally, it implements an
> nth_packed_object_oid function which provides a struct object_id
> version of the nth_packed_object function.
> 
> There is a small known conflict with next, but it can easily be fixed up.

I read through all of the patches, and sent a few comments. I didn't
notice any other problems.

Michael

[PATCH v3 37/38] sequencer (rebase -i): write the progress into files

[Johannes Schindelin]

For the benefit of e.g. the shell prompt, the interactive rebase not
only displays the progress for the user to see, but also writes it into
the msgnum/end files in the state directory.

Teach the sequencer this new trick.

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

diff --git a/sequencer.c b/sequencer.c
index 2c9c555ab6..b39cd21e03 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -47,6 +47,16 @@ static GIT_PATH_FUNC(rebase_path_todo, 
"rebase-merge/git-rebase-todo")
  */
 static GIT_PATH_FUNC(rebase_path_done, "rebase-merge/done")
 /*
+ * The file to keep track of how many commands were already processed (e.g.
+ * for the prompt).
+ */
+static GIT_PATH_FUNC(rebase_path_msgnum, "rebase-merge/msgnum");
+/*
+ * The file to keep track of how many commands are to be processed in total
+ * (e.g. for the prompt).
+ */
+static GIT_PATH_FUNC(rebase_path_msgtotal, "rebase-merge/end");
+/*
  * The commit message that is planned to be used for any changes that
  * need to be committed following a user interaction.
  */
@@ -1353,6 +1363,7 @@ static int read_populate_todo(struct todo_list *todo_list,
 
if (is_rebase_i(opts)) {
struct todo_list done = TODO_LIST_INIT;
+   FILE *f = fopen(rebase_path_msgtotal(), "w");
 
if (strbuf_read_file(, rebase_path_done(), 0) > 0 &&
!parse_insn_buffer(done.buf.buf, ))
@@ -1362,8 +1373,12 @@ static int read_populate_todo(struct todo_list 
*todo_list,
 
todo_list->total_nr = todo_list->done_nr
+ count_commands(todo_list);
-
todo_list_release();
+
+   if (f) {
+   fprintf(f, "%d\n", todo_list->total_nr);
+   fclose(f);
+   }
}
 
return 0;
@@ -1947,11 +1962,20 @@ static int pick_commits(struct todo_list *todo_list, 
struct replay_opts *opts)
if (save_todo(todo_list, opts))
return -1;
if (is_rebase_i(opts)) {
-   if (item->command != TODO_COMMENT)
+   if (item->command != TODO_COMMENT) {
+   FILE *f = fopen(rebase_path_msgnum(), "w");
+
+   todo_list->done_nr++;
+
+   if (f) {
+   fprintf(f, "%d\n", todo_list->done_nr);
+   fclose(f);
+   }
fprintf(stderr, "Rebasing (%d/%d)%s",
-   ++(todo_list->done_nr),
+   todo_list->done_nr,
todo_list->total_nr,
opts->verbose ? "\n" : "\r");
+   }
unlink(rebase_path_message());
unlink(rebase_path_author_script());
unlink(rebase_path_stopped_sha());
-- 
2.11.0.rc3.windows.1

[PATCH v3 38/38] sequencer (rebase -i): write out the final message

[Johannes Schindelin]

The shell script version of the interactive rebase has a very specific
final message. Teach the sequencer to print the same.

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

diff --git a/sequencer.c b/sequencer.c
index b39cd21e03..0e7d2ca5c8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2127,6 +2127,9 @@ static int pick_commits(struct todo_list *todo_list, 
struct replay_opts *opts)
}
apply_autostash(opts);
 
+   fprintf(stderr, "Successfully rebased and updated %s.\n",
+   head_ref.buf);
+
strbuf_release();
strbuf_release(_ref);
}
-- 
2.11.0.rc3.windows.1

[PATCH v3 35/38] sequencer (rebase -i): suggest --edit-todo upon unknown command

[Johannes Schindelin]

This is the same behavior as known from `git rebase -i`.

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

diff --git a/sequencer.c b/sequencer.c
index 4f37ba8d33..4792a3de3b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1314,8 +1314,12 @@ 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) {
+   if (is_rebase_i(opts))
+   return error(_("please fix this using "
+  "'git rebase --edit-todo'."));
return error(_("unusable instruction sheet: '%s'"), todo_file);
+   }
 
if (!todo_list->nr &&
(!is_rebase_i(opts) || !file_exists(rebase_path_done(
-- 
2.11.0.rc3.windows.1

[PATCH v3 31/38] sequencer: make reading author-script more elegant

[Johannes Schindelin]

Rather than abusing a strbuf to come up with an environment block, let's
just use the argv_array structure which serves the same purpose much
better.

While at it, rename the function to reflect the fact that it does not
really care exactly what environment variables are defined in said file.

Suggested-by: Jeff King 
Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 32 +++-
 1 file changed, 11 insertions(+), 21 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 41f80ea2c4..a0d0aaeaf8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -544,18 +544,17 @@ static int write_author_script(const char *message)
 }
 
 /*
- * Read the author-script file into an environment block, ready for use in
- * run_command(), that can be free()d afterwards.
+ * Read a list of environment variable assignments (such as the author-script
+ * file) into an environment block. Returns -1 on error, 0 otherwise.
  */
-static char **read_author_script(void)
+static int read_env_script(struct argv_array *env)
 {
struct strbuf script = STRBUF_INIT;
int i, count = 0;
-   char *p, *p2, **env;
-   size_t env_size;
+   char *p, *p2;
 
if (strbuf_read_file(, rebase_path_author_script(), 256) <= 0)
-   return NULL;
+   return -1;
 
for (p = script.buf; *p; p++)
if (skip_prefix(p, "'''", (const char **)))
@@ -567,19 +566,12 @@ static char **read_author_script(void)
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;
+   argv_array_push(env, p);
p += strlen(p) + 1;
}
-   env[count] = NULL;
 
-   return env;
+   return 0;
 }
 
 static const char staged_changes_advice[] =
@@ -612,14 +604,12 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
  int allow_empty, int edit, int amend,
  int cleanup_commit_message)
 {
-   char **env = NULL;
-   struct argv_array array;
+   struct argv_array env = ARGV_ARRAY_INIT, array;
int rc;
const char *value;
 
if (is_rebase_i(opts)) {
-   env = read_author_script();
-   if (!env) {
+   if (!read_env_script()) {
const char *gpg_opt = gpg_sign_opt_quoted(opts);
 
return error(_(staged_changes_advice),
@@ -655,9 +645,9 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
argv_array_push(, "--allow-empty-message");
 
rc = run_command_v_opt_cd_env(array.argv, RUN_GIT_CMD, NULL,
-   (const char *const *)env);
+   (const char *const *)env.argv);
argv_array_clear();
-   free(env);
+   argv_array_clear();
 
return rc;
 }
-- 
2.11.0.rc3.windows.1

[PATCH v3 34/38] sequencer (rebase -i): show only failed cherry-picks' output

[Johannes Schindelin]

This is the behavior of the shell script version of the interactive
rebase, by using the `output` function defined in `git-rebase.sh`.

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

diff --git a/sequencer.c b/sequencer.c
index a501dfce38..4f37ba8d33 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -433,6 +433,8 @@ static int do_recursive_merge(struct commit *base, struct 
commit *next,
o.ancestor = base ? base_label : "(empty tree)";
o.branch1 = "HEAD";
o.branch2 = next ? next_label : "(empty tree)";
+   if (is_rebase_i(opts))
+   o.buffer_output = 2;
 
head_tree = parse_tree_indirect(head);
next_tree = next ? next->tree : empty_tree();
@@ -444,6 +446,8 @@ static int do_recursive_merge(struct commit *base, struct 
commit *next,
clean = merge_trees(,
head_tree,
next_tree, base_tree, );
+   if (is_rebase_i(opts) && clean <= 0)
+   fputs(o.obuf.buf, stdout);
strbuf_release();
if (clean < 0)
return clean;
-- 
2.11.0.rc3.windows.1

[PATCH v3 36/38] sequencer (rebase -i): show the progress

[Johannes Schindelin]

The interactive rebase keeps the user informed about its progress.
If the sequencer wants to do the grunt work of the interactive
rebase, it also needs to show that progress.

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

diff --git a/sequencer.c b/sequencer.c
index 4792a3de3b..2c9c555ab6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1181,6 +1181,7 @@ struct todo_list {
struct strbuf buf;
struct todo_item *items;
int nr, alloc, current;
+   int done_nr, total_nr;
 };
 
 #define TODO_LIST_INIT { STRBUF_INIT }
@@ -1297,6 +1298,17 @@ static int parse_insn_buffer(char *buf, struct todo_list 
*todo_list)
return res;
 }
 
+static int count_commands(struct todo_list *todo_list)
+{
+   int count = 0, i;
+
+   for (i = 0; i < todo_list->nr; i++)
+   if (todo_list->items[i].command != TODO_COMMENT)
+   count++;
+
+   return count;
+}
+
 static int read_populate_todo(struct todo_list *todo_list,
struct replay_opts *opts)
 {
@@ -1339,6 +1351,21 @@ static int read_populate_todo(struct todo_list 
*todo_list,
return error(_("cannot revert during a 
cherry-pick."));
}
 
+   if (is_rebase_i(opts)) {
+   struct todo_list done = TODO_LIST_INIT;
+
+   if (strbuf_read_file(, rebase_path_done(), 0) > 0 &&
+   !parse_insn_buffer(done.buf.buf, ))
+   todo_list->done_nr = count_commands();
+   else
+   todo_list->done_nr = 0;
+
+   todo_list->total_nr = todo_list->done_nr
+   + count_commands(todo_list);
+
+   todo_list_release();
+   }
+
return 0;
 }
 
@@ -1920,6 +1947,11 @@ static int pick_commits(struct todo_list *todo_list, 
struct replay_opts *opts)
if (save_todo(todo_list, opts))
return -1;
if (is_rebase_i(opts)) {
+   if (item->command != TODO_COMMENT)
+   fprintf(stderr, "Rebasing (%d/%d)%s",
+   ++(todo_list->done_nr),
+   todo_list->total_nr,
+   opts->verbose ? "\n" : "\r");
unlink(rebase_path_message());
unlink(rebase_path_author_script());
unlink(rebase_path_stopped_sha());
-- 
2.11.0.rc3.windows.1

[PATCH v3 32/38] sequencer: use run_command() directly

[Johannes Schindelin]

Instead of using the convenience function run_command_v_opt_cd_env(), we
now use the run_command() function. The former function is simply a
wrapper of the latter, trying to make it more convenient to use.

However, we already have to construct the argv and the env parameters,
and we will need even finer control e.g. over the output of the command,
so let's just stop using the convenience function.

Based on patches and suggestions by Johannes Sixt and Jeff King.

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

diff --git a/sequencer.c b/sequencer.c
index a0d0aaeaf8..c7dc5a2ad4 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -604,12 +604,13 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
  int allow_empty, int edit, int amend,
  int cleanup_commit_message)
 {
-   struct argv_array env = ARGV_ARRAY_INIT, array;
-   int rc;
+   struct child_process cmd = CHILD_PROCESS_INIT;
const char *value;
 
+   cmd.git_cmd = 1;
+
if (is_rebase_i(opts)) {
-   if (!read_env_script()) {
+   if (read_env_script(_array)) {
const char *gpg_opt = gpg_sign_opt_quoted(opts);
 
return error(_(staged_changes_advice),
@@ -617,39 +618,34 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
}
}
 
-   argv_array_init();
-   argv_array_push(, "commit");
-   argv_array_push(, "-n");
+   argv_array_push(, "commit");
+   argv_array_push(, "-n");
 
if (amend)
-   argv_array_push(, "--amend");
+   argv_array_push(, "--amend");
if (opts->gpg_sign)
-   argv_array_pushf(, "-S%s", opts->gpg_sign);
+   argv_array_pushf(, "-S%s", opts->gpg_sign);
if (opts->signoff)
-   argv_array_push(, "-s");
+   argv_array_push(, "-s");
if (defmsg)
-   argv_array_pushl(, "-F", defmsg, NULL);
+   argv_array_pushl(, "-F", defmsg, NULL);
if (cleanup_commit_message)
-   argv_array_push(, "--cleanup=strip");
+   argv_array_push(, "--cleanup=strip");
if (edit)
-   argv_array_push(, "-e");
+   argv_array_push(, "-e");
else if (!cleanup_commit_message &&
 !opts->signoff && !opts->record_origin &&
 git_config_get_value("commit.cleanup", ))
-   argv_array_push(, "--cleanup=verbatim");
+   argv_array_push(, "--cleanup=verbatim");
 
if (allow_empty)
-   argv_array_push(, "--allow-empty");
+   argv_array_push(, "--allow-empty");
 
if (opts->allow_empty_message)
-   argv_array_push(, "--allow-empty-message");
+   argv_array_push(, "--allow-empty-message");
 
-   rc = run_command_v_opt_cd_env(array.argv, RUN_GIT_CMD, NULL,
-   (const char *const *)env.argv);
-   argv_array_clear();
-   argv_array_clear();
 
-   return rc;
+   return run_command();
 }
 
 static int is_original_commit_empty(struct commit *commit)
-- 
2.11.0.rc3.windows.1

[PATCH v3 33/38] sequencer (rebase -i): show only failed `git commit`'s output

[Johannes Schindelin]

This is the behavior of the shell script version of the interactive
rebase, by using the `output` function defined in `git-rebase.sh`.

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

diff --git a/sequencer.c b/sequencer.c
index c7dc5a2ad4..a501dfce38 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -610,6 +610,11 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
cmd.git_cmd = 1;
 
if (is_rebase_i(opts)) {
+   if (!edit) {
+   cmd.stdout_to_stderr = 1;
+   cmd.err = -1;
+   }
+
if (read_env_script(_array)) {
const char *gpg_opt = gpg_sign_opt_quoted(opts);
 
@@ -644,6 +649,19 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
if (opts->allow_empty_message)
argv_array_push(, "--allow-empty-message");
 
+   if (cmd.err == -1) {
+   /* hide stderr on success */
+   struct strbuf buf = STRBUF_INIT;
+   int rc = pipe_command(,
+ NULL, 0,
+ /* stdout is already redirected */
+ NULL, 0,
+ , 0);
+   if (rc)
+   fputs(buf.buf, stderr);
+   strbuf_release();
+   return rc;
+   }
 
return run_command();
 }
-- 
2.11.0.rc3.windows.1

[PATCH v3 29/38] sequencer (rebase -i): implement the 'drop' command

[Johannes Schindelin]

The parsing part of a 'drop' command is almost identical to parsing a
'pick', while the operation is the same as that of a 'noop'.

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

diff --git a/sequencer.c b/sequencer.c
index dd5b843a84..6e92f186ae 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -736,7 +736,8 @@ enum todo_command {
/* commands that do something else than handling a single commit */
TODO_EXEC,
/* commands that do nothing but are counted for reporting progress */
-   TODO_NOOP
+   TODO_NOOP,
+   TODO_DROP
 };
 
 static struct {
@@ -750,7 +751,8 @@ static struct {
{ 'f', "fixup" },
{ 's', "squash" },
{ 'x', "exec" },
-   { 0,   "noop" }
+   { 0,   "noop" },
+   { 'd', "drop" }
 };
 
 static const char *command_to_string(const enum todo_command command)
@@ -762,7 +764,7 @@ static const char *command_to_string(const enum 
todo_command command)
 
 static int is_noop(const enum todo_command command)
 {
-   return TODO_NOOP <= (size_t)command;
+   return TODO_NOOP <= command;
 }
 
 static int is_fixup(enum todo_command command)
-- 
2.11.0.rc3.windows.1

[PATCH v3 30/38] sequencer (rebase -i): differentiate between comments and 'noop'

[Johannes Schindelin]

In the upcoming patch, we will support rebase -i's progress
reporting. The progress skips comments but counts 'noop's.

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

diff --git a/sequencer.c b/sequencer.c
index 6e92f186ae..41f80ea2c4 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -737,7 +737,9 @@ enum todo_command {
TODO_EXEC,
/* commands that do nothing but are counted for reporting progress */
TODO_NOOP,
-   TODO_DROP
+   TODO_DROP,
+   /* comments (not counted for reporting progress) */
+   TODO_COMMENT
 };
 
 static struct {
@@ -752,12 +754,13 @@ static struct {
{ 's', "squash" },
{ 'x', "exec" },
{ 0,   "noop" },
-   { 'd', "drop" }
+   { 'd', "drop" },
+   { 0,   NULL }
 };
 
 static const char *command_to_string(const enum todo_command command)
 {
-   if ((size_t)command < ARRAY_SIZE(todo_command_info))
+   if (command < TODO_COMMENT)
return todo_command_info[command].str;
die("Unknown command: %d", command);
 }
@@ -1198,14 +1201,14 @@ static int parse_insn_line(struct todo_item *item, 
const char *bol, char *eol)
bol += strspn(bol, " \t");
 
if (bol == eol || *bol == '\r' || *bol == comment_line_char) {
-   item->command = TODO_NOOP;
+   item->command = TODO_COMMENT;
item->commit = NULL;
item->arg = bol;
item->arg_len = eol - bol;
return 0;
}
 
-   for (i = 0; i < ARRAY_SIZE(todo_command_info); i++)
+   for (i = 0; i < TODO_COMMENT; i++)
if (skip_prefix(bol, todo_command_info[i].str, )) {
item->command = i;
break;
@@ -1214,7 +1217,7 @@ static int parse_insn_line(struct todo_item *item, const 
char *bol, char *eol)
item->command = i;
break;
}
-   if (i >= ARRAY_SIZE(todo_command_info))
+   if (i >= TODO_COMMENT)
return -1;
 
if (item->command == TODO_NOOP) {
-- 
2.11.0.rc3.windows.1

Re: [PATCH 14/17] sha1_file: introduce an nth_packed_object_oid function

[Michael Haggerty]

On 01/01/2017 08:18 PM, brian m. carlson wrote:
> There are places in the code where we would like to provide a struct
> object_id *, yet read the hash directly from the pack.  Provide an
> nth_packed_object_oid function that mirrors the nth_packed_object_sha1
> function.
> 
> The required cast is questionable, but should be safe on all known
> platforms.  The alternative of allocating an object as an intermediate
> would be too inefficient and cause memory leaks.  If necessary, an
> intermediate union could be used; this practice is allowed by GCC and
> explicitly sanctioned by C11.  However, such a change will likely not be
> necessary, and can be made if and when it is.

I have the feeling that this design decision has been discussed on the
mailing list. If so, could you include a URL here?

The obvious alternative to allocating a new object to return to the
caller would be to have the caller of `nth_packed_object_oid()` pass a
`struct object_id *` argument to be filled in (by copying the hash into
it). Aside from being legal C, this would also be a more robust step
towards a post-SHA-1 world, where the suggested hack wouldn't work.

Of course, the question is what the callers want to do with the
`object_id`. Are the return values of `nth_packed_object_sha1()` stored
to other longer-lived structures that rely on the lifetime of the
packfile mmap to keep the value valid? If so, then keeping track of the
lifetime of the `struct object_id` could be a big chore, not to mention
that needing to keep a 20-byte `struct object_id` around rather than a
8- or 4-byte pointer would also cost more RAM.

As you probably can tell, I'm not psyched about straying outside of
legal C. It would be nice to see more justification.

> [...]

Michael

[PATCH v3 23/38] sequencer (rebase -i): copy commit notes at end

[Johannes Schindelin]

When rebasing commits that have commit notes attached, the interactive
rebase rewrites those notes faithfully at the end. The sequencer must
do this, too, if it wishes to do interactive rebase's job.

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

diff --git a/sequencer.c b/sequencer.c
index 95ae4bcd1e..50380a15b8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -96,6 +96,15 @@ static GIT_PATH_FUNC(rebase_path_amend, "rebase-merge/amend")
  */
 static GIT_PATH_FUNC(rebase_path_stopped_sha, "rebase-merge/stopped-sha")
 /*
+ * For the post-rewrite hook, we make a list of rewritten commits and
+ * their new sha1s.  The rewritten-pending list keeps the sha1s of
+ * commits that have been processed, but not committed yet,
+ * e.g. because they are waiting for a 'squash' command.
+ */
+static GIT_PATH_FUNC(rebase_path_rewritten_list, "rebase-merge/rewritten-list")
+static GIT_PATH_FUNC(rebase_path_rewritten_pending,
+   "rebase-merge/rewritten-pending")
+/*
  * The following files are written by git-rebase just after parsing the
  * command-line (and are only consumed, not modified, by the sequencer).
  */
@@ -850,6 +859,44 @@ static int update_squash_messages(enum todo_command 
command,
return res;
 }
 
+static void flush_rewritten_pending(void) {
+   struct strbuf buf = STRBUF_INIT;
+   unsigned char newsha1[20];
+   FILE *out;
+
+   if (strbuf_read_file(, rebase_path_rewritten_pending(), 82) > 0 &&
+   !get_sha1("HEAD", newsha1) &&
+   (out = fopen(rebase_path_rewritten_list(), "a"))) {
+   char *bol = buf.buf, *eol;
+
+   while (*bol) {
+   eol = strchrnul(bol, '\n');
+   fprintf(out, "%.*s %s\n", (int)(eol - bol),
+   bol, sha1_to_hex(newsha1));
+   if (!*eol)
+   break;
+   bol = eol + 1;
+   }
+   fclose(out);
+   unlink(rebase_path_rewritten_pending());
+   }
+   strbuf_release();
+}
+
+static void record_in_rewritten(struct object_id *oid,
+   enum todo_command next_command) {
+   FILE *out = fopen(rebase_path_rewritten_pending(), "a");
+
+   if (!out)
+   return;
+
+   fprintf(out, "%s\n", oid_to_hex(oid));
+   fclose(out);
+
+   if (!is_fixup(next_command))
+   flush_rewritten_pending();
+}
+
 static int do_pick_commit(enum todo_command command, struct commit *commit,
struct replay_opts *opts, int final_fixup)
 {
@@ -1743,6 +1790,17 @@ static int is_final_fixup(struct todo_list *todo_list)
return 1;
 }
 
+static enum todo_command peek_command(struct todo_list *todo_list, int offset)
+{
+   int i;
+
+   for (i = todo_list->current + offset; i < todo_list->nr; i++)
+   if (!is_noop(todo_list->items[i].command))
+   return todo_list->items[i].command;
+
+   return -1;
+}
+
 static const char *reflog_message(struct replay_opts *opts,
const char *sub_action, const char *fmt, ...)
 {
@@ -1801,6 +1859,9 @@ static int pick_commits(struct todo_list *todo_list, 
struct replay_opts *opts)
item->arg, item->arg_len, opts, res,
!res);
}
+   if (is_rebase_i(opts) && !res)
+   record_in_rewritten(>commit->object.oid,
+   peek_command(todo_list, 1));
if (res && is_fixup(item->command)) {
if (res == 1)
intend_to_amend();
@@ -1827,6 +1888,7 @@ static int pick_commits(struct todo_list *todo_list, 
struct replay_opts *opts)
 
if (is_rebase_i(opts)) {
struct strbuf head_ref = STRBUF_INIT, buf = STRBUF_INIT;
+   struct stat st;
 
/* Stopped in the middle, as planned? */
if (todo_list->current < todo_list->nr)
@@ -1891,6 +1953,20 @@ static int pick_commits(struct todo_list *todo_list, 
struct replay_opts *opts)
log_tree_diff_flush(_tree_opt);
}
}
+   flush_rewritten_pending();
+   if (!stat(rebase_path_rewritten_list(), ) &&
+   st.st_size > 0) {
+   struct child_process child = CHILD_PROCESS_INIT;
+
+   child.in = open(rebase_path_rewritten_list(), O_RDONLY);
+   child.git_cmd = 1;
+   argv_array_push(, "notes");
+   argv_array_push(, "copy");
+   argv_array_push(, "--for-rewrite=rebase");
+  

[PATCH v3 22/38] sequencer (rebase -i): set the reflog message consistently

[Johannes Schindelin]

We already used the same reflog message as the scripted version of rebase
-i when finishing. With this commit, we do that also for all the commands
before that.

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

diff --git a/sequencer.c b/sequencer.c
index 0d8e11f580..95ae4bcd1e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1785,6 +1785,10 @@ static int pick_commits(struct todo_list *todo_list, 
struct replay_opts *opts)
unlink(rebase_path_amend());
}
if (item->command <= TODO_SQUASH) {
+   if (is_rebase_i(opts))
+   setenv("GIT_REFLOG_ACTION", reflog_message(opts,
+   command_to_string(item->command), NULL),
+   1);
res = do_pick_commit(item->command, item->commit,
opts, is_final_fixup(todo_list));
if (item->command == TODO_EDIT) {
-- 
2.11.0.rc3.windows.1

[PATCH v3 21/38] sequencer (rebase -i): refactor setting the reflog message

[Johannes Schindelin]

This makes the code DRYer, with the obvious benefit that we can enhance
the code further in a single place.

We can also reuse the functionality elsewhere by calling this new
function.

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

diff --git a/sequencer.c b/sequencer.c
index 23161f593e..0d8e11f580 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1743,6 +1743,26 @@ static int is_final_fixup(struct todo_list *todo_list)
return 1;
 }
 
+static const char *reflog_message(struct replay_opts *opts,
+   const char *sub_action, const char *fmt, ...)
+{
+   va_list ap;
+   static struct strbuf buf = STRBUF_INIT;
+
+   va_start(ap, fmt);
+   strbuf_reset();
+   strbuf_addstr(, action_name(opts));
+   if (sub_action)
+   strbuf_addf(, " (%s)", sub_action);
+   if (fmt) {
+   strbuf_addstr(, ": ");
+   strbuf_vaddf(, fmt, ap);
+   }
+   va_end(ap);
+
+   return buf.buf;
+}
+
 static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
 {
int res = 0;
@@ -1810,6 +1830,7 @@ static int pick_commits(struct todo_list *todo_list, 
struct replay_opts *opts)
 
if (read_oneliner(_ref, rebase_path_head_name(), 0) &&
starts_with(head_ref.buf, "refs/")) {
+   const char *msg;
unsigned char head[20], orig[20];
int res;
 
@@ -1825,23 +1846,21 @@ static int pick_commits(struct todo_list *todo_list, 
struct replay_opts *opts)
res = error(_("could not read orig-head"));
goto cleanup_head_ref;
}
-   strbuf_addf(, "rebase -i (finish): %s onto ",
-   head_ref.buf);
if (!read_oneliner(, rebase_path_onto(), 0)) {
res = error(_("could not read 'onto'"));
goto cleanup_head_ref;
}
-   if (update_ref(buf.buf, head_ref.buf, head, orig,
+   msg = reflog_message(opts, "finish", "%s onto %s",
+   head_ref.buf, buf.buf);
+   if (update_ref(msg, head_ref.buf, head, orig,
REF_NODEREF, UPDATE_REFS_MSG_ON_ERR)) {
res = error(_("could not update %s"),
head_ref.buf);
goto cleanup_head_ref;
}
-   strbuf_reset();
-   strbuf_addf(,
-   "rebase -i (finish): returning to %s",
+   msg = reflog_message(opts, "finish", "returning to %s",
head_ref.buf);
-   if (create_symref("HEAD", head_ref.buf, buf.buf)) {
+   if (create_symref("HEAD", head_ref.buf, msg)) {
res = error(_("could not update HEAD to %s"),
head_ref.buf);
goto cleanup_head_ref;
-- 
2.11.0.rc3.windows.1

[PATCH v3 16/38] sequencer (rebase -i): the todo can be empty when continuing

[Johannes Schindelin]

When the last command of an interactive rebase fails, the user needs to
resolve the problem and then continue the interactive rebase. Naturally,
the todo script is empty by then. So let's not complain about that!

To that end, let's move that test out of the function that parses the
todo script, and into the more high-level function read_populate_todo().
This is also necessary by now because the lower-level parse_insn_buffer()
has no idea whether we are performing an interactive rebase or not.

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

diff --git a/sequencer.c b/sequencer.c
index a7b9ee0d04..6a840216b1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1215,8 +1215,7 @@ static int parse_insn_buffer(char *buf, struct todo_list 
*todo_list)
else if (!is_noop(item->command))
fixup_okay = 1;
}
-   if (!todo_list->nr)
-   return error(_("no commits parsed."));
+
return res;
 }
 
@@ -1240,6 +1239,10 @@ static int read_populate_todo(struct todo_list 
*todo_list,
if (res)
return error(_("unusable instruction sheet: '%s'"), todo_file);
 
+   if (!todo_list->nr &&
+   (!is_rebase_i(opts) || !file_exists(rebase_path_done(
+   return error(_("no commits parsed."));
+
if (!is_rebase_i(opts)) {
enum todo_command valid =
opts->action == REPLAY_PICK ? TODO_PICK : TODO_REVERT;
-- 
2.11.0.rc3.windows.1

[PATCH v3 17/38] sequencer (rebase -i): update refs after a successful rebase

[Johannes Schindelin]

An interactive rebase operates on a detached HEAD (to keep the reflog
of the original branch relatively clean), and updates the branch only
at the end.

Now that the sequencer learns to perform interactive rebases, it also
needs to learn the trick to update the branch before removing the
directory containing the state of the interactive rebase.

We introduce a new head_ref variable in a wider scope than necessary at
the moment, to allow for a later patch that prints out "Successfully
rebased and updated ".

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

diff --git a/sequencer.c b/sequencer.c
index 6a840216b1..80b2b2a975 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -102,6 +102,8 @@ static GIT_PATH_FUNC(rebase_path_stopped_sha, 
"rebase-merge/stopped-sha")
 static GIT_PATH_FUNC(rebase_path_gpg_sign_opt, "rebase-merge/gpg_sign_opt")
 static GIT_PATH_FUNC(rebase_path_orig_head, "rebase-merge/orig-head")
 static GIT_PATH_FUNC(rebase_path_verbose, "rebase-merge/verbose")
+static GIT_PATH_FUNC(rebase_path_head_name, "rebase-merge/head-name")
+static GIT_PATH_FUNC(rebase_path_onto, "rebase-merge/onto")
 
 static inline int is_rebase_i(const struct replay_opts *opts)
 {
@@ -1784,12 +1786,53 @@ static int pick_commits(struct todo_list *todo_list, 
struct replay_opts *opts)
}
 
if (is_rebase_i(opts)) {
-   struct strbuf buf = STRBUF_INIT;
+   struct strbuf head_ref = STRBUF_INIT, buf = STRBUF_INIT;
 
/* Stopped in the middle, as planned? */
if (todo_list->current < todo_list->nr)
return 0;
 
+   if (read_oneliner(_ref, rebase_path_head_name(), 0) &&
+   starts_with(head_ref.buf, "refs/")) {
+   unsigned char head[20], orig[20];
+   int res;
+
+   if (get_sha1("HEAD", head)) {
+   res = error(_("cannot read HEAD"));
+cleanup_head_ref:
+   strbuf_release(_ref);
+   strbuf_release();
+   return res;
+   }
+   if (!read_oneliner(, rebase_path_orig_head(), 0) ||
+   get_sha1_hex(buf.buf, orig)) {
+   res = error(_("could not read orig-head"));
+   goto cleanup_head_ref;
+   }
+   strbuf_addf(, "rebase -i (finish): %s onto ",
+   head_ref.buf);
+   if (!read_oneliner(, rebase_path_onto(), 0)) {
+   res = error(_("could not read 'onto'"));
+   goto cleanup_head_ref;
+   }
+   if (update_ref(buf.buf, head_ref.buf, head, orig,
+   REF_NODEREF, UPDATE_REFS_MSG_ON_ERR)) {
+   res = error(_("could not update %s"),
+   head_ref.buf);
+   goto cleanup_head_ref;
+   }
+   strbuf_reset();
+   strbuf_addf(,
+   "rebase -i (finish): returning to %s",
+   head_ref.buf);
+   if (create_symref("HEAD", head_ref.buf, buf.buf)) {
+   res = error(_("could not update HEAD to %s"),
+   head_ref.buf);
+   goto cleanup_head_ref;
+   }
+   strbuf_reset();
+   }
+
if (opts->verbose) {
struct rev_info log_tree_opt;
struct object_id orig, head;
@@ -1810,6 +1853,7 @@ static int pick_commits(struct todo_list *todo_list, 
struct replay_opts *opts)
}
}
strbuf_release();
+   strbuf_release(_ref);
}
 
/*
-- 
2.11.0.rc3.windows.1

[PATCH v3 24/38] sequencer (rebase -i): record interrupted commits in rewritten, too

[Johannes Schindelin]

When continuing after a `pick` command failed, we want that commit
to show up in the rewritten-list (and its notes to be rewritten), too.

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

diff --git a/sequencer.c b/sequencer.c
index 50380a15b8..d7273fd1b3 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2060,6 +2060,14 @@ int sequencer_continue(struct replay_opts *opts)
goto release_todo_list;
}
todo_list.current++;
+   } else if (file_exists(rebase_path_stopped_sha())) {
+   struct strbuf buf = STRBUF_INIT;
+   struct object_id oid;
+
+   if (read_oneliner(, rebase_path_stopped_sha(), 1) &&
+   !get_sha1_committish(buf.buf, oid.hash))
+   record_in_rewritten(, peek_command(_list, 0));
+   strbuf_release();
}
 
res = pick_commits(_list, opts);
-- 
2.11.0.rc3.windows.1

[PATCH v3 28/38] sequencer (rebase -i): allow rescheduling commands

[Johannes Schindelin]

The interactive rebase has the very special magic that a cherry-pick
that exits with a status different from 0 and 1 signifies a failure to
even record that a cherry-pick was started.

This can happen e.g. when a fast-forward fails because it would
overwrite untracked files.

In that case, we must reschedule the command that we thought we already
had at least started successfully.

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

diff --git a/sequencer.c b/sequencer.c
index 04a64cf0dc..dd5b843a84 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1915,6 +1915,12 @@ static int pick_commits(struct todo_list *todo_list, 
struct replay_opts *opts)
1);
res = do_pick_commit(item->command, item->commit,
opts, is_final_fixup(todo_list));
+   if (is_rebase_i(opts) && res < 0) {
+   /* Reschedule */
+   todo_list->current--;
+   if (save_todo(todo_list, opts))
+   return -1;
+   }
if (item->command == TODO_EDIT) {
struct commit *commit = item->commit;
if (!res)
-- 
2.11.0.rc3.windows.1

[PATCH v3 25/38] sequencer (rebase -i): run the post-rewrite hook, if needed

[Johannes Schindelin]

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

diff --git a/sequencer.c b/sequencer.c
index d7273fd1b3..43ced8db31 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1957,6 +1957,8 @@ static int pick_commits(struct todo_list *todo_list, 
struct replay_opts *opts)
if (!stat(rebase_path_rewritten_list(), ) &&
st.st_size > 0) {
struct child_process child = CHILD_PROCESS_INIT;
+   const char *post_rewrite_hook =
+   find_hook("post-rewrite");
 
child.in = open(rebase_path_rewritten_list(), O_RDONLY);
child.git_cmd = 1;
@@ -1965,6 +1967,18 @@ static int pick_commits(struct todo_list *todo_list, 
struct replay_opts *opts)
argv_array_push(, "--for-rewrite=rebase");
/* we don't care if this copying failed */
run_command();
+
+   if (post_rewrite_hook) {
+   struct child_process hook = CHILD_PROCESS_INIT;
+
+   hook.in = open(rebase_path_rewritten_list(),
+   O_RDONLY);
+   hook.stdout_to_stderr = 1;
+   argv_array_push(, post_rewrite_hook);
+   argv_array_push(, "rebase");
+   /* we don't care if this hook failed */
+   run_command();
+   }
}
 
strbuf_release();
-- 
2.11.0.rc3.windows.1

[PATCH v3 20/38] sequencer (rebase -i): allow fast-forwarding for edit/reword

[Johannes Schindelin]

The sequencer already knew how to fast-forward instead of
cherry-picking, if possible.

We want to continue to do this, of course, but in case of the 'reword'
command, we will need to call `git commit` after fast-forwarding.

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

diff --git a/sequencer.c b/sequencer.c
index 50e998acc4..23161f593e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -860,7 +860,7 @@ static int do_pick_commit(enum todo_command command, struct 
commit *commit,
const char *base_label, *next_label;
struct commit_message msg = { NULL, NULL, NULL, NULL };
struct strbuf msgbuf = STRBUF_INIT;
-   int res, unborn = 0, amend = 0, allow;
+   int res, unborn = 0, amend = 0, allow = 0;
 
if (opts->no_commit) {
/*
@@ -905,11 +905,23 @@ static int do_pick_commit(enum todo_command command, 
struct commit *commit,
else
parent = commit->parents->item;
 
+   if (get_message(commit, ) != 0)
+   return error(_("cannot get commit message for %s"),
+   oid_to_hex(>object.oid));
+
if (opts->allow_ff && !is_fixup(command) &&
((parent && !hashcmp(parent->object.oid.hash, head)) ||
-(!parent && unborn)))
-   return fast_forward_to(commit->object.oid.hash, head, unborn, 
opts);
-
+(!parent && unborn))) {
+   if (is_rebase_i(opts))
+   write_author_script(msg.message);
+   res = fast_forward_to(commit->object.oid.hash, head, unborn,
+   opts);
+   if (res || command != TODO_REWORD)
+   goto leave;
+   edit = amend = 1;
+   msg_file = NULL;
+   goto fast_forward_edit;
+   }
if (parent && parse_commit(parent) < 0)
/* TRANSLATORS: The first %s will be a "todo" command like
   "revert" or "pick", the second %s a SHA1. */
@@ -917,10 +929,6 @@ static int do_pick_commit(enum todo_command command, 
struct commit *commit,
command_to_string(command),
oid_to_hex(>object.oid));
 
-   if (get_message(commit, ) != 0)
-   return error(_("cannot get commit message for %s"),
-   oid_to_hex(>object.oid));
-
/*
 * "commit" is an existing commit.  We would want to apply
 * the difference it introduces since its first parent "prev"
@@ -1044,6 +1052,7 @@ static int do_pick_commit(enum todo_command command, 
struct commit *commit,
goto leave;
}
if (!opts->no_commit)
+fast_forward_edit:
res = run_git_commit(msg_file, opts, allow, edit, amend,
 cleanup_commit_message);
 
-- 
2.11.0.rc3.windows.1

[PATCH v3 19/38] sequencer (rebase -i): implement the 'reword' command

[Johannes Schindelin]

This is now trivial, as all the building blocks are in place: all we need
to do is to flip the "edit" switch when committing.

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

diff --git a/sequencer.c b/sequencer.c
index a2002f1c12..50e998acc4 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -718,6 +718,7 @@ enum todo_command {
TODO_PICK = 0,
TODO_REVERT,
TODO_EDIT,
+   TODO_REWORD,
TODO_FIXUP,
TODO_SQUASH,
/* commands that do something else than handling a single commit */
@@ -733,6 +734,7 @@ static struct {
{ 'p', "pick" },
{ 0,   "revert" },
{ 'e', "edit" },
+   { 'r', "reword" },
{ 'f', "fixup" },
{ 's', "squash" },
{ 'x', "exec" },
@@ -962,7 +964,9 @@ static int do_pick_commit(enum todo_command command, struct 
commit *commit,
}
}
 
-   if (is_fixup(command)) {
+   if (command == TODO_REWORD)
+   edit = 1;
+   else if (is_fixup(command)) {
if (update_squash_messages(command, commit, opts))
return -1;
amend = 1;
@@ -1771,7 +1775,8 @@ static int pick_commits(struct todo_list *todo_list, 
struct replay_opts *opts)
item->arg_len, item->arg);
} else if (res && is_rebase_i(opts))
return res | error_with_patch(item->commit,
-   item->arg, item->arg_len, opts, res, 0);
+   item->arg, item->arg_len, opts, res,
+   item->command == TODO_REWORD);
} else if (item->command == TODO_EXEC) {
char *end_of_arg = (char *)(item->arg + item->arg_len);
int saved = *end_of_arg;
-- 
2.11.0.rc3.windows.1

[PATCH v3 18/38] sequencer (rebase -i): leave a patch upon error

[Johannes Schindelin]

When doing an interactive rebase, we want to leave a 'patch' file for
further inspection by the user (even if we never tried to actually apply
that patch, since we're cherry-picking instead).

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

diff --git a/sequencer.c b/sequencer.c
index 80b2b2a975..a2002f1c12 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1769,7 +1769,9 @@ static int pick_commits(struct todo_list *todo_list, 
struct replay_opts *opts)
intend_to_amend();
return error_failed_squash(item->commit, opts,
item->arg_len, item->arg);
-   }
+   } else if (res && is_rebase_i(opts))
+   return res | error_with_patch(item->commit,
+   item->arg, item->arg_len, opts, res, 0);
} else if (item->command == TODO_EXEC) {
char *end_of_arg = (char *)(item->arg + item->arg_len);
int saved = *end_of_arg;
-- 
2.11.0.rc3.windows.1

[PATCH v3 26/38] sequencer (rebase -i): respect the rebase.autostash setting

[Johannes Schindelin]

Git's `rebase` command inspects the `rebase.autostash` config setting
to determine whether it should stash any uncommitted changes before
rebasing and re-apply them afterwards.

As we introduce more bits and pieces to let the sequencer act as
interactive rebase's backend, here is the part that adds support for
the autostash feature.

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

diff --git a/sequencer.c b/sequencer.c
index 43ced8db31..06f7cebe48 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -113,6 +113,7 @@ static GIT_PATH_FUNC(rebase_path_orig_head, 
"rebase-merge/orig-head")
 static GIT_PATH_FUNC(rebase_path_verbose, "rebase-merge/verbose")
 static GIT_PATH_FUNC(rebase_path_head_name, "rebase-merge/head-name")
 static GIT_PATH_FUNC(rebase_path_onto, "rebase-merge/onto")
+static GIT_PATH_FUNC(rebase_path_autostash, "rebase-merge/autostash")
 
 static inline int is_rebase_i(const struct replay_opts *opts)
 {
@@ -1801,6 +1802,47 @@ static enum todo_command peek_command(struct todo_list 
*todo_list, int offset)
return -1;
 }
 
+static int apply_autostash(struct replay_opts *opts)
+{
+   struct strbuf stash_sha1 = STRBUF_INIT;
+   struct child_process child = CHILD_PROCESS_INIT;
+   int ret = 0;
+
+   if (!read_oneliner(_sha1, rebase_path_autostash(), 1)) {
+   strbuf_release(_sha1);
+   return 0;
+   }
+   strbuf_trim(_sha1);
+
+   child.git_cmd = 1;
+   argv_array_push(, "stash");
+   argv_array_push(, "apply");
+   argv_array_push(, stash_sha1.buf);
+   if (!run_command())
+   printf(_("Applied autostash."));
+   else {
+   struct child_process store = CHILD_PROCESS_INIT;
+
+   store.git_cmd = 1;
+   argv_array_push(, "stash");
+   argv_array_push(, "store");
+   argv_array_push(, "-m");
+   argv_array_push(, "autostash");
+   argv_array_push(, "-q");
+   argv_array_push(, stash_sha1.buf);
+   if (run_command())
+   ret = error(_("cannot store %s"), stash_sha1.buf);
+   else
+   printf(_("Applying autostash resulted in conflicts.\n"
+   "Your changes are safe in the stash.\n"
+   "You can run \"git stash pop\" or"
+   " \"git stash drop\" at any time.\n"));
+   }
+
+   strbuf_release(_sha1);
+   return ret;
+}
+
 static const char *reflog_message(struct replay_opts *opts,
const char *sub_action, const char *fmt, ...)
 {
@@ -1980,6 +2022,7 @@ static int pick_commits(struct todo_list *todo_list, 
struct replay_opts *opts)
run_command();
}
}
+   apply_autostash(opts);
 
strbuf_release();
strbuf_release(_ref);
-- 
2.11.0.rc3.windows.1

[PATCH v3 27/38] sequencer (rebase -i): respect strategy/strategy_opts settings

[Johannes Schindelin]

The sequencer already has an idea about using different merge
strategies. We just piggy-back on top of that, using rebase -i's
own settings, when running the sequencer in interactive rebase mode.

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

diff --git a/sequencer.c b/sequencer.c
index 06f7cebe48..04a64cf0dc 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -114,6 +114,8 @@ static GIT_PATH_FUNC(rebase_path_verbose, 
"rebase-merge/verbose")
 static GIT_PATH_FUNC(rebase_path_head_name, "rebase-merge/head-name")
 static GIT_PATH_FUNC(rebase_path_onto, "rebase-merge/onto")
 static GIT_PATH_FUNC(rebase_path_autostash, "rebase-merge/autostash")
+static GIT_PATH_FUNC(rebase_path_strategy, "rebase-merge/strategy")
+static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
 
 static inline int is_rebase_i(const struct replay_opts *opts)
 {
@@ -1368,6 +1370,26 @@ static int populate_opts_cb(const char *key, const char 
*value, void *data)
return 0;
 }
 
+static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf)
+{
+   int i;
+
+   strbuf_reset(buf);
+   if (!read_oneliner(buf, rebase_path_strategy(), 0))
+   return;
+   opts->strategy = strbuf_detach(buf, NULL);
+   if (!read_oneliner(buf, rebase_path_strategy_opts(), 0))
+   return;
+
+   opts->xopts_nr = split_cmdline(buf->buf, (const char ***)>xopts);
+   for (i = 0; i < opts->xopts_nr; i++) {
+   const char *arg = opts->xopts[i];
+
+   skip_prefix(arg, "--", );
+   opts->xopts[i] = xstrdup(arg);
+   }
+}
+
 static int read_populate_opts(struct replay_opts *opts)
 {
if (is_rebase_i(opts)) {
@@ -1381,11 +1403,13 @@ static int read_populate_opts(struct replay_opts *opts)
opts->gpg_sign = xstrdup(buf.buf + 2);
}
}
-   strbuf_release();
 
if (file_exists(rebase_path_verbose()))
opts->verbose = 1;
 
+   read_strategy_opts(opts, );
+   strbuf_release();
+
return 0;
}
 
-- 
2.11.0.rc3.windows.1

[PATCH v3 07/38] sequencer (rebase -i): implement the 'exec' command

[Johannes Schindelin]

The 'exec' command is a little special among rebase -i's commands, as it
does *not* have a SHA-1 as first parameter. Instead, everything after the
`exec` command is treated as command-line to execute.

Let's reuse the arg/arg_len fields of the todo_item structure (which hold
the oneline for pick/edit commands) to point to the command-line.

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

diff --git a/sequencer.c b/sequencer.c
index b138a3906c..e9c10d7fe5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -18,6 +18,7 @@
 #include "quote.h"
 #include "trailer.h"
 #include "log-tree.h"
+#include "wt-status.h"
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
@@ -632,6 +633,8 @@ enum todo_command {
TODO_PICK = 0,
TODO_REVERT,
TODO_EDIT,
+   /* commands that do something else than handling a single commit */
+   TODO_EXEC,
/* commands that do nothing but are counted for reporting progress */
TODO_NOOP
 };
@@ -640,6 +643,7 @@ static const char *todo_command_strings[] = {
"pick",
"revert",
"edit",
+   "exec",
"noop"
 };
 
@@ -938,6 +942,12 @@ static int parse_insn_line(struct todo_item *item, const 
char *bol, char *eol)
return -1;
bol += padding;
 
+   if (item->command == TODO_EXEC) {
+   item->arg = bol;
+   item->arg_len = (int)(eol - bol);
+   return 0;
+   }
+
end_of_object_name = (char *) bol + strcspn(bol, " \t\n");
saved = *end_of_object_name;
*end_of_object_name = '\0';
@@ -1397,6 +1407,46 @@ static int error_with_patch(struct commit *commit,
return exit_code;
 }
 
+static int do_exec(const char *command_line)
+{
+   const char *child_argv[] = { NULL, NULL };
+   int dirty, status;
+
+   fprintf(stderr, "Executing: %s\n", command_line);
+   child_argv[0] = command_line;
+   status = run_command_v_opt(child_argv, RUN_USING_SHELL);
+
+   /* force re-reading of the cache */
+   if (discard_cache() < 0 || read_cache() < 0)
+   return error(_("could not read index"));
+
+   dirty = require_clean_work_tree("rebase", NULL, 1, 1);
+
+   if (status) {
+   warning(_("execution failed: %s\n%s"
+ "You can fix the problem, and then run\n"
+ "\n"
+ "  git rebase --continue\n"
+ "\n"),
+   command_line,
+   dirty ? N_("and made changes to the index and/or the "
+   "working tree\n") : "");
+   if (status == 127)
+   /* command not found */
+   status = 1;
+   } else if (dirty) {
+   warning(_("execution succeeded: %s\nbut "
+ "left changes to the index and/or the working tree\n"
+ "Commit or stash your changes, and then run\n"
+ "\n"
+ "  git rebase --continue\n"
+ "\n"), command_line);
+   status = 1;
+   }
+
+   return status;
+}
+
 static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
 {
int res = 0;
@@ -1425,6 +1475,13 @@ static int pick_commits(struct todo_list *todo_list, 
struct replay_opts *opts)
item->arg, item->arg_len, opts, res,
!res);
}
+   } else if (item->command == TODO_EXEC) {
+   char *end_of_arg = (char *)(item->arg + item->arg_len);
+   int saved = *end_of_arg;
+
+   *end_of_arg = '\0';
+   res = do_exec(item->arg);
+   *end_of_arg = saved;
} else if (!is_noop(item->command))
return error(_("unknown command %d"), item->command);
 
-- 
2.11.0.rc3.windows.1

[PATCH v3 09/38] sequencer (rebase -i): write the 'done' file

[Johannes Schindelin]

In the interactive rebase, commands that were successfully processed are
not simply discarded, but appended to the 'done' file instead. This is
used e.g. to display the current state to the user in the output of
`git status` or the progress.

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

diff --git a/sequencer.c b/sequencer.c
index ddc4d144d7..8ea3d6aa94 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -41,6 +41,12 @@ static GIT_PATH_FUNC(rebase_path, "rebase-merge")
  */
 static GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo")
 /*
+ * The rebase command lines that have already been processed. A line
+ * is moved here when it is first handled, before any associated user
+ * actions.
+ */
+static GIT_PATH_FUNC(rebase_path_done, "rebase-merge/done")
+/*
  * 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.
@@ -1296,6 +1302,23 @@ static int save_todo(struct todo_list *todo_list, struct 
replay_opts *opts)
return error_errno(_("could not write to '%s'"), todo_path);
if (commit_lock_file(_lock) < 0)
return error(_("failed to finalize '%s'."), todo_path);
+
+   if (is_rebase_i(opts)) {
+   const char *done_path = rebase_path_done();
+   int fd = open(done_path, O_CREAT | O_WRONLY | O_APPEND, 0666);
+   int prev_offset = !next ? 0 :
+   todo_list->items[next - 1].offset_in_buf;
+
+   if (fd >= 0 && offset > prev_offset &&
+   write_in_full(fd, todo_list->buf.buf + prev_offset,
+ offset - prev_offset) < 0) {
+   close(fd);
+   return error_errno(_("could not write to '%s'"),
+  done_path);
+   }
+   if (fd >= 0)
+   close(fd);
+   }
return 0;
 }
 
-- 
2.11.0.rc3.windows.1

[PATCH v3 13/38] sequencer (rebase -i): allow continuing with staged changes

[Johannes Schindelin]

When an interactive rebase is interrupted, the user may stage changes
before continuing, and we need to commit those changes in that case.

Please note that the nested "if" added to the sequencer_continue() is
not combined into a single "if" because it will be extended with an
"else" clause in a later patch in this patch series.

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

diff --git a/sequencer.c b/sequencer.c
index 9913882603..69301fecc6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1826,6 +1826,42 @@ static int continue_single_pick(void)
return run_command_v_opt(argv, RUN_GIT_CMD);
 }
 
+static int commit_staged_changes(struct replay_opts *opts)
+{
+   int amend = 0;
+
+   if (has_unstaged_changes(1))
+   return error(_("cannot rebase: You have unstaged changes."));
+   if (!has_uncommitted_changes(0))
+   return 0;
+
+   if (file_exists(rebase_path_amend())) {
+   struct strbuf rev = STRBUF_INIT;
+   unsigned char head[20], to_amend[20];
+
+   if (get_sha1("HEAD", head))
+   return error(_("cannot amend non-existing commit"));
+   if (!read_oneliner(, rebase_path_amend(), 0))
+   return error(_("invalid file: '%s'"), 
rebase_path_amend());
+   if (get_sha1_hex(rev.buf, to_amend))
+   return error(_("invalid contents: '%s'"),
+   rebase_path_amend());
+   if (hashcmp(head, to_amend))
+   return error(_("\nYou have uncommitted changes in your "
+  "working tree. Please, commit them\n"
+  "first and then run 'git rebase "
+  "--continue' again."));
+
+   strbuf_release();
+   amend = 1;
+   }
+
+   if (run_git_commit(rebase_path_message(), opts, 1, 1, amend, 0))
+   return error(_("could not commit staged changes."));
+   unlink(rebase_path_amend());
+   return 0;
+}
+
 int sequencer_continue(struct replay_opts *opts)
 {
struct todo_list todo_list = TODO_LIST_INIT;
@@ -1834,6 +1870,10 @@ int sequencer_continue(struct replay_opts *opts)
if (read_and_refresh_cache(opts))
return -1;
 
+   if (is_rebase_i(opts)) {
+   if (commit_staged_changes(opts))
+   return -1;
+   }
if (!file_exists(get_todo_path(opts)))
return continue_single_pick();
if (read_populate_opts(opts))
-- 
2.11.0.rc3.windows.1

[PATCH v3 15/38] sequencer (rebase -i): skip some revert/cherry-pick specific code path

[Johannes Schindelin]

When a cherry-pick continues without a "todo script", the intention is
simply to pick a single commit.

However, when an interactive rebase is continued without a "todo
script", it means that the last command has been completed and that we
now need to clean up.

This commit guards the revert/cherry-pick specific steps so that they
are not executed in rebase -i mode.

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

diff --git a/sequencer.c b/sequencer.c
index 52e17c8887..a7b9ee0d04 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1878,26 +1878,28 @@ int sequencer_continue(struct replay_opts *opts)
if (is_rebase_i(opts)) {
if (commit_staged_changes(opts))
return -1;
-   }
-   if (!file_exists(get_todo_path(opts)))
+   } else if (!file_exists(get_todo_path(opts)))
return continue_single_pick();
if (read_populate_opts(opts))
return -1;
if ((res = read_populate_todo(_list, opts)))
goto release_todo_list;
 
-   /* Verify that the conflict has been resolved */
-   if (file_exists(git_path_cherry_pick_head()) ||
-   file_exists(git_path_revert_head())) {
-   res = continue_single_pick();
-   if (res)
+   if (!is_rebase_i(opts)) {
+   /* Verify that the conflict has been resolved */
+   if (file_exists(git_path_cherry_pick_head()) ||
+   file_exists(git_path_revert_head())) {
+   res = continue_single_pick();
+   if (res)
+   goto release_todo_list;
+   }
+   if (index_differs_from("HEAD", 0, 0)) {
+   res = error_dirty_index(opts);
goto release_todo_list;
+   }
+   todo_list.current++;
}
-   if (index_differs_from("HEAD", 0, 0)) {
-   res = error_dirty_index(opts);
-   goto release_todo_list;
-   }
-   todo_list.current++;
+
res = pick_commits(_list, opts);
 release_todo_list:
todo_list_release(_list);
-- 
2.11.0.rc3.windows.1

[PATCH v3 14/38] sequencer (rebase -i): remove CHERRY_PICK_HEAD when no longer needed

[Johannes Schindelin]

The scripted version of the interactive rebase already does that.

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

diff --git a/sequencer.c b/sequencer.c
index 69301fecc6..52e17c8887 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1832,8 +1832,13 @@ static int commit_staged_changes(struct replay_opts 
*opts)
 
if (has_unstaged_changes(1))
return error(_("cannot rebase: You have unstaged changes."));
-   if (!has_uncommitted_changes(0))
+   if (!has_uncommitted_changes(0)) {
+   const char *cherry_pick_head = git_path("CHERRY_PICK_HEAD");
+
+   if (file_exists(cherry_pick_head) && unlink(cherry_pick_head))
+   return error(_("could not remove CHERRY_PICK_HEAD"));
return 0;
+   }
 
if (file_exists(rebase_path_amend())) {
struct strbuf rev = STRBUF_INIT;
-- 
2.11.0.rc3.windows.1

[PATCH v3 08/38] sequencer (rebase -i): learn about the 'verbose' mode

[Johannes Schindelin]

When calling `git rebase -i -v`, the user wants to see some statistics
after the commits were rebased. Let's show some.

The strbuf we use to perform that task will be used for other things
in subsequent commits, hence it is declared and initialized in a wider
scope than strictly needed here.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 28 
 sequencer.h |  1 +
 2 files changed, 29 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index e9c10d7fe5..ddc4d144d7 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -65,6 +65,8 @@ static GIT_PATH_FUNC(rebase_path_stopped_sha, 
"rebase-merge/stopped-sha")
  * 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")
+static GIT_PATH_FUNC(rebase_path_orig_head, "rebase-merge/orig-head")
+static GIT_PATH_FUNC(rebase_path_verbose, "rebase-merge/verbose")
 
 static inline int is_rebase_i(const struct replay_opts *opts)
 {
@@ -1088,6 +1090,9 @@ static int read_populate_opts(struct replay_opts *opts)
}
strbuf_release();
 
+   if (file_exists(rebase_path_verbose()))
+   opts->verbose = 1;
+
return 0;
}
 
@@ -1491,9 +1496,32 @@ static int pick_commits(struct todo_list *todo_list, 
struct replay_opts *opts)
}
 
if (is_rebase_i(opts)) {
+   struct strbuf buf = STRBUF_INIT;
+
/* Stopped in the middle, as planned? */
if (todo_list->current < todo_list->nr)
return 0;
+
+   if (opts->verbose) {
+   struct rev_info log_tree_opt;
+   struct object_id orig, head;
+
+   memset(_tree_opt, 0, sizeof(log_tree_opt));
+   init_revisions(_tree_opt, NULL);
+   log_tree_opt.diff = 1;
+   log_tree_opt.diffopt.output_format =
+   DIFF_FORMAT_DIFFSTAT;
+   log_tree_opt.disable_stdin = 1;
+
+   if (read_oneliner(, rebase_path_orig_head(), 0) &&
+   !get_sha1(buf.buf, orig.hash) &&
+   !get_sha1("HEAD", head.hash)) {
+   diff_tree_sha1(orig.hash, head.hash,
+  "", _tree_opt.diffopt);
+   log_tree_diff_flush(_tree_opt);
+   }
+   }
+   strbuf_release();
}
 
/*
diff --git a/sequencer.h b/sequencer.h
index cb21cfddee..f885b68395 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -24,6 +24,7 @@ struct replay_opts {
int allow_empty;
int allow_empty_message;
int keep_redundant_commits;
+   int verbose;
 
int mainline;
 
-- 
2.11.0.rc3.windows.1

[PATCH v3 12/38] sequencer (rebase -i): write an author-script file

[Johannes Schindelin]

When the interactive rebase aborts, it writes out an author-script file
to record the author information for the current commit. As we are about
to teach the sequencer how to perform the actions behind an interactive
rebase, it needs to write those author-script files, too.

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

diff --git a/sequencer.c b/sequencer.c
index 29b944d724..9913882603 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -483,6 +483,52 @@ static int is_index_unchanged(void)
return !hashcmp(active_cache_tree->sha1, 
head_commit->tree->object.oid.hash);
 }
 
+static int write_author_script(const char *message)
+{
+   struct strbuf buf = STRBUF_INIT;
+   const char *eol;
+   int res;
+
+   for (;;)
+   if (!*message || starts_with(message, "\n")) {
+missing_author:
+   /* Missing 'author' line? */
+   unlink(rebase_path_author_script());
+   return 0;
+   } else if (skip_prefix(message, "author ", ))
+   break;
+   else if ((eol = strchr(message, '\n')))
+   message = eol + 1;
+   else
+   goto missing_author;
+
+   strbuf_addstr(, "GIT_AUTHOR_NAME='");
+   while (*message && *message != '\n' && *message != '\r')
+   if (skip_prefix(message, " <", ))
+   break;
+   else if (*message != '\'')
+   strbuf_addch(, *(message++));
+   else
+   strbuf_addf(, "'%c'", *(message++));
+   strbuf_addstr(, "'\nGIT_AUTHOR_EMAIL='");
+   while (*message && *message != '\n' && *message != '\r')
+   if (skip_prefix(message, "> ", ))
+   break;
+   else if (*message != '\'')
+   strbuf_addch(, *(message++));
+   else
+   strbuf_addf(, "'%c'", *(message++));
+   strbuf_addstr(, "'\nGIT_AUTHOR_DATE='@");
+   while (*message && *message != '\n' && *message != '\r')
+   if (*message != '\'')
+   strbuf_addch(, *(message++));
+   else
+   strbuf_addf(, "'%c'", *(message++));
+   res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);
+   strbuf_release();
+   return res;
+}
+
 /*
  * Read the author-script file into an environment block, ready for use in
  * run_command(), that can be free()d afterwards.
@@ -935,7 +981,9 @@ static int do_pick_commit(enum todo_command command, struct 
commit *commit,
}
}
 
-   if (!opts->strategy || !strcmp(opts->strategy, "recursive") || command 
== TODO_REVERT) {
+   if (is_rebase_i(opts) && write_author_script(msg.message) < 0)
+   res = -1;
+   else 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)
-- 
2.11.0.rc3.windows.1

[PATCH v3 11/38] sequencer (rebase -i): implement the short commands

[Johannes Schindelin]

For users' convenience, most rebase commands can be abbreviated, e.g.
'p' instead of 'pick' and 'x' instead of 'exec'. Let's teach the
sequencer to handle those abbreviated commands just fine.

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

diff --git a/sequencer.c b/sequencer.c
index 6a939a10bd..29b944d724 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -678,20 +678,23 @@ enum todo_command {
TODO_NOOP
 };
 
-static const char *todo_command_strings[] = {
-   "pick",
-   "revert",
-   "edit",
-   "fixup",
-   "squash",
-   "exec",
-   "noop"
+static struct {
+   char c;
+   const char *str;
+} todo_command_info[] = {
+   { 'p', "pick" },
+   { 0,   "revert" },
+   { 'e', "edit" },
+   { 'f', "fixup" },
+   { 's', "squash" },
+   { 'x', "exec" },
+   { 0,   "noop" }
 };
 
 static const char *command_to_string(const enum todo_command command)
 {
-   if ((size_t)command < ARRAY_SIZE(todo_command_strings))
-   return todo_command_strings[command];
+   if ((size_t)command < ARRAY_SIZE(todo_command_info))
+   return todo_command_info[command].str;
die("Unknown command: %d", command);
 }
 
@@ -1087,12 +1090,16 @@ static int parse_insn_line(struct todo_item *item, 
const char *bol, char *eol)
return 0;
}
 
-   for (i = 0; i < ARRAY_SIZE(todo_command_strings); i++)
-   if (skip_prefix(bol, todo_command_strings[i], )) {
+   for (i = 0; i < ARRAY_SIZE(todo_command_info); i++)
+   if (skip_prefix(bol, todo_command_info[i].str, )) {
+   item->command = i;
+   break;
+   } else if (bol[1] == ' ' && *bol == todo_command_info[i].c) {
+   bol++;
item->command = i;
break;
}
-   if (i >= ARRAY_SIZE(todo_command_strings))
+   if (i >= ARRAY_SIZE(todo_command_info))
return -1;
 
if (item->command == TODO_NOOP) {
@@ -1287,7 +1294,7 @@ static int walk_revs_populate_todo(struct todo_list 
*todo_list,
 {
enum todo_command command = opts->action == REPLAY_PICK ?
TODO_PICK : TODO_REVERT;
-   const char *command_string = todo_command_strings[command];
+   const char *command_string = todo_command_info[command].str;
struct commit *commit;
 
if (prepare_revs(opts))
-- 
2.11.0.rc3.windows.1

[PATCH v3 10/38] sequencer (rebase -i): add support for the 'fixup' and 'squash' commands

[Johannes Schindelin]

This is a huge patch, and at the same time a huge step forward to
execute the performance-critical parts of the interactive rebase in a
builtin command.

Since 'fixup' and 'squash' are not only similar, but also need to know
about each other (we want to reduce a series of fixups/squashes into a
single, final commit message edit, from the user's point of view), we
really have to implement them both at the same time.

Most of the actual work is done by the existing code path that already
handles the "pick" and the "edit" commands; We added support for other
features (e.g. to amend the commit message) in the patches leading up to
this one, yet there are still quite a few bits in this patch that simply
would not make sense as individual patches (such as: determining whether
there was anything to "fix up" in the "todo" script, etc).

In theory, it would be possible to reuse the fast-forward code path also
for the fixup and the squash code paths, but in practice this would make
the code less readable. The end result cannot be fast-forwarded anyway,
therefore let's just extend the cherry-picking code path for now.

Since the sequencer parses the entire `git-rebase-todo` script in one go,
fixup or squash commands without a preceding pick can be reported early
(in git-rebase--interactive, we could only report such errors just before
executing the fixup/squash).

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

diff --git a/sequencer.c b/sequencer.c
index 8ea3d6aa94..6a939a10bd 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -47,6 +47,35 @@ static GIT_PATH_FUNC(rebase_path_todo, 
"rebase-merge/git-rebase-todo")
  */
 static GIT_PATH_FUNC(rebase_path_done, "rebase-merge/done")
 /*
+ * The commit message that is planned to be used for any changes that
+ * need to be committed following a user interaction.
+ */
+static GIT_PATH_FUNC(rebase_path_message, "rebase-merge/message")
+/*
+ * The file into which is accumulated the suggested commit message for
+ * squash/fixup commands. When the first of a series of squash/fixups
+ * is seen, the file is created and the commit message from the
+ * previous commit and from the first squash/fixup commit are written
+ * to it. The commit message for each subsequent squash/fixup commit
+ * is appended to the file as it is processed.
+ *
+ * The first line of the file is of the form
+ * # This is a combination of $count commits.
+ * where $count is the number of commits whose messages have been
+ * written to the file so far (including the initial "pick" commit).
+ * Each time that a commit message is processed, this line is read and
+ * updated. It is deleted just before the combined commit is made.
+ */
+static GIT_PATH_FUNC(rebase_path_squash_msg, "rebase-merge/message-squash")
+/*
+ * If the current series of squash/fixups has not yet included a squash
+ * command, then this file exists and holds the commit message of the
+ * original "pick" commit.  (If the series ends without a "squash"
+ * command, then this can be used as the commit message of the combined
+ * commit without opening the editor.)
+ */
+static GIT_PATH_FUNC(rebase_path_fixup_msg, "rebase-merge/message-fixup")
+/*
  * 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.
@@ -641,6 +670,8 @@ enum todo_command {
TODO_PICK = 0,
TODO_REVERT,
TODO_EDIT,
+   TODO_FIXUP,
+   TODO_SQUASH,
/* commands that do something else than handling a single commit */
TODO_EXEC,
/* commands that do nothing but are counted for reporting progress */
@@ -651,6 +682,8 @@ static const char *todo_command_strings[] = {
"pick",
"revert",
"edit",
+   "fixup",
+   "squash",
"exec",
"noop"
 };
@@ -667,15 +700,114 @@ static int is_noop(const enum todo_command command)
return TODO_NOOP <= (size_t)command;
 }
 
+static int is_fixup(enum todo_command command)
+{
+   return command == TODO_FIXUP || command == TODO_SQUASH;
+}
+
+static int update_squash_messages(enum todo_command command,
+   struct commit *commit, struct replay_opts *opts)
+{
+   struct strbuf buf = STRBUF_INIT;
+   int count, res;
+   const char *message, *body;
+
+   if (file_exists(rebase_path_squash_msg())) {
+   struct strbuf header = STRBUF_INIT;
+   char *eol, *p;
+
+   if (strbuf_read_file(, rebase_path_squash_msg(), 2048) <= 0)
+   return error(_("could not read '%s'"),
+   rebase_path_squash_msg());
+
+   p = buf.buf + 1;
+   eol = strchrnul(buf.buf, '\n');
+   if (buf.buf[0] != comment_line_char ||
+   (p += strcspn(p, "0123456789\n")) == eol)
+  

[PATCH v3 00/38] Teach the sequencer to act as rebase -i's backend

[Johannes Schindelin]

This marks the count down to '3': two more patch series after this
(really tiny ones) and we have a faster rebase -i.

The idea of this patch series is to teach the sequencer to understand
all of the commands in `git-rebase-todo` scripts, to execute them and to
behave pretty much very the same as `git rebase -i --continue` when
called with the newly-introduced REPLAY_INTERACTIVE_REBASE mode.

Most of these patches should be pretty much straight-forward. When not,
I tried to make a point of describing enough background in the commit
message. Please feel free to point out where my explanations fall short.

Note that even after this patch series is applied, rebase -i is still
unaffected. It will require the next patch series which introduces the
rebase--helper that essentially implements `git rebase -i --continue` by
calling the sequencer with the appropriate options.

The final patch series will move a couple of pre- and post-processing
steps into the rebase--helper/sequencer (such as expanding/shrinking the
SHA-1s, reordering the fixup!/squash! lines, etc). This might sound like
a mere add-on, but it is essential for the speed improvements: those
stupid little processing steps really dominated the execution time in my
tests.

Apart from mostly cosmetic patches (and the occasional odd bug that I
fixed promptly), I used these patches since mid May to perform all of my
interactive rebases. In mid June, I had the idea to teach rebase -i to
run *both* scripted rebase and rebase--helper and to cross-validate the
results. This slowed down all my interactive rebases since, but helped
me catch three rather obscure bugs (e.g. that git commit --fixup unfolds
long onelines and rebase -i still finds the correct original commit).

This is all only to say that I am rather confident that the current code
does the job.

Since sending out v1, I integrated all of these patch series
into Git for Windows v2.10.0, where they have been live ever since, and
used by myself (also in a Linux VM, as Git for Windows' master branch
always builds also on Linux and passes the test suite, too).

Just to reiterate why I do all this: it speeds up the interactive rebase
substantially. Even with a not yet fully builtin rebase -i, but just the
part after the user edited the `git-rebase-todo` script.

The performance test I introduced to demonstrate this (p3404) shows a
speed-up of +380% here (i.e. roughly 5x), from ~8.8 seconds to ~1.8
seconds. This is on Windows, where the performance impact of avoiding
shell scripting is most noticable.

On MacOSX and on Linux, the speed-up is less pronounced, but still
noticable, at least if you trust Travis CI, which I abused to perform
that test for me. Check for yourself (searching for "3404.2") here:
https://travis-ci.org/git/git/builds/156295227. According to those logs,
p3404 is speeded up from ~0.45 seconds to ~0.12 seconds on Linux (read:
about 3.5x) and from ~1.7 seconds to ~0.5 seconds on MacOSX (read:
almost 4x).

Changes since v2:

- fixed a TRANSLATORS: comment

- added a comment to clarify why save_todo() does not write the current
  command to git-rebase-todo

- fixed the comment for "stopped-sha" that said that a long commit name
  is stored in that file, when it is in fact an abbreviated one

- consistently put the "else" keyword on the same line as the preceding
  closing brace, if any (this adds two patches to the already large
  patch series, sorry)

- completely revamped the update_squash_messages() function to make it
  more obvious why it does not overflow its buffer, and to fix it when
  using it with localised messages

- now the sequencer code uses find_commit_subject() consistently to find
  the commit message (it does not use the subject's length returned by
  the find_commit_subject() function, of course); this adds yet another
  patch to this already large patch series, sorry.

- introduced an is_noop() function (as opposed to testing <= TODO_NOOP
  manually) to make the code less magic.

- fixed two potential leaks of get_commit_buffer()'s returned buffer

- fixed leaks in the error code paths when trying to update the ref at
  the end

- renamed read_author_script() into read_env_script() to reflect that
  it would accept any environment setting, and made it much more
  readable

- completely reworked the strategy how to suppress the output of
  successful cherry-picks: instead of introducing
  RUN_HIDE_STDERR_ON_SUCCESS, the code now uses pipe_command() to catch
  the output itself, so that it can be shown in case of error

- replaced a spawned `diff-tree` command by code using the diff functions
  directly


Johannes Schindelin (38):
  sequencer: avoid unnecessary curly braces
  sequencer: move "else" keyword onto the same line as preceding brace
  sequencer: use a helper to find the commit message
  sequencer: support a new action: 'interactive rebase'
  sequencer (rebase -i): implement the 'noop' command
  sequencer (rebase -i): implement the 'edit' command
  sequencer 

[PATCH v3 03/38] sequencer: use a helper to find the commit message

[Johannes Schindelin]

It is actually not safe to look for a commit message by looking for the
first empty line and skipping it.

The find_commit_subject() function looks more carefully, so let's use
it. Since we are interested in the entire commit message, we re-compute
the string length after verifying that the commit subject is not empty
(in which case the entire commit message would be empty, something that
should not happen but that we want to handle gracefully).

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

diff --git a/sequencer.c b/sequencer.c
index 3eededcb98..720857beda 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -703,14 +703,9 @@ static int do_pick_commit(enum todo_command command, 
struct commit *commit,
next = commit;
next_label = msg.label;
 
-   /*
-* Append the commit log message to msgbuf; it starts
-* after the tree, parent, author, committer
-* information followed by "\n\n".
-*/
-   p = strstr(msg.message, "\n\n");
-   if (p)
-   strbuf_addstr(, skip_blank_lines(p + 2));
+   /* Append the commit log message to msgbuf. */
+   if (find_commit_subject(msg.message, ))
+   strbuf_addstr(, p);
 
if (opts->record_origin) {
if (!has_conforming_footer(, NULL, 0))
-- 
2.11.0.rc3.windows.1

[PATCH v3 01/38] sequencer: avoid unnecessary curly braces

[Johannes Schindelin]

This was noticed while addressing Junio Hamano's concern that some
"else" operators were on separate lines than the preceding closing
brace.

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

diff --git a/sequencer.c b/sequencer.c
index 9adb7bbf1d..23793db08b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -632,9 +632,8 @@ static int do_pick_commit(enum todo_command command, struct 
commit *commit,
}
discard_cache();
 
-   if (!commit->parents) {
+   if (!commit->parents)
parent = NULL;
-   }
else if (commit->parents->next) {
/* Reverting or cherry-picking a merge commit */
int cnt;
-- 
2.11.0.rc3.windows.1

[PATCH v3 05/38] sequencer (rebase -i): implement the 'noop' command

[Johannes Schindelin]

The 'noop' command is probably the most boring of all rebase -i commands
to support in the sequencer.

Which makes it an excellent candidate for this first stab to add support
for rebase -i's commands to the sequencer.

For the moment, let's also treat empty lines and commented-out lines as
'noop'; We will refine that handling later in this patch series.

To make it easier to identify "classes" of todo_commands (such as:
determine whether a command is pick-like, i.e. handles a single commit),
let's enforce a certain order of said commands.

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

diff --git a/sequencer.c b/sequencer.c
index 690460bc67..84f18e64e9 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -607,14 +607,23 @@ static int allow_empty(struct replay_opts *opts, struct 
commit *commit)
return 1;
 }
 
+/*
+ * Note that ordering matters in this enum. Not only must it match the mapping
+ * below, it is also divided into several sections that matter.  When adding
+ * new commands, make sure you add it in the right section.
+ */
 enum todo_command {
+   /* commands that handle commits */
TODO_PICK = 0,
-   TODO_REVERT
+   TODO_REVERT,
+   /* commands that do nothing but are counted for reporting progress */
+   TODO_NOOP
 };
 
 static const char *todo_command_strings[] = {
"pick",
-   "revert"
+   "revert",
+   "noop"
 };
 
 static const char *command_to_string(const enum todo_command command)
@@ -624,6 +633,10 @@ static const char *command_to_string(const enum 
todo_command command)
die("Unknown command: %d", command);
 }
 
+static int is_noop(const enum todo_command command)
+{
+   return TODO_NOOP <= (size_t)command;
+}
 
 static int do_pick_commit(enum todo_command command, struct commit *commit,
struct replay_opts *opts)
@@ -879,6 +892,14 @@ static int parse_insn_line(struct todo_item *item, const 
char *bol, char *eol)
/* left-trim */
bol += strspn(bol, " \t");
 
+   if (bol == eol || *bol == '\r' || *bol == comment_line_char) {
+   item->command = TODO_NOOP;
+   item->commit = NULL;
+   item->arg = bol;
+   item->arg_len = eol - bol;
+   return 0;
+   }
+
for (i = 0; i < ARRAY_SIZE(todo_command_strings); i++)
if (skip_prefix(bol, todo_command_strings[i], )) {
item->command = i;
@@ -887,6 +908,13 @@ static int parse_insn_line(struct todo_item *item, const 
char *bol, char *eol)
if (i >= ARRAY_SIZE(todo_command_strings))
return -1;
 
+   if (item->command == TODO_NOOP) {
+   item->commit = NULL;
+   item->arg = bol;
+   item->arg_len = eol - bol;
+   return 0;
+   }
+
/* Eat up extra spaces/ tabs before object name */
padding = strspn(bol, " \t");
if (!padding)
@@ -1289,7 +1317,12 @@ static int pick_commits(struct todo_list *todo_list, 
struct replay_opts *opts)
struct todo_item *item = todo_list->items + todo_list->current;
if (save_todo(todo_list, opts))
return -1;
-   res = do_pick_commit(item->command, item->commit, opts);
+   if (item->command <= TODO_REVERT)
+   res = do_pick_commit(item->command, item->commit,
+   opts);
+   else if (!is_noop(item->command))
+   return error(_("unknown command %d"), item->command);
+
todo_list->current++;
if (res)
return res;
-- 
2.11.0.rc3.windows.1

[PATCH v3 04/38] sequencer: support a new action: 'interactive rebase'

[Johannes Schindelin]

This patch introduces a new action for the sequencer. It really does not
do a whole lot of its own right now, but lays the ground work for
patches to come. The intention, of course, is to finally make the
sequencer the work horse of the interactive rebase (the original idea
behind the "sequencer" concept).

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 36 
 sequencer.h |  3 ++-
 2 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 720857beda..690460bc67 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -30,6 +30,14 @@ static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts")
 static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
 static GIT_PATH_FUNC(git_path_abort_safety_file, "sequencer/abort-safety")
 
+static GIT_PATH_FUNC(rebase_path, "rebase-merge")
+/*
+ * The file containing rebase commands, comments, and empty lines.
+ * This file is created by "git rebase -i" then edited by the user. As
+ * the lines are processed, they are removed from the front of this
+ * file and written to the tail of 'done'.
+ */
+static GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo")
 /*
  * 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
@@ -42,19 +50,22 @@ static GIT_PATH_FUNC(rebase_path_author_script, 
"rebase-merge/author-script")
  */
 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)
 {
-   return 0;
+   return opts->action == REPLAY_INTERACTIVE_REBASE;
 }
 
 static const char *get_dir(const struct replay_opts *opts)
 {
+   if (is_rebase_i(opts))
+   return rebase_path();
return git_path_seq_dir();
 }
 
 static const char *get_todo_path(const struct replay_opts *opts)
 {
+   if (is_rebase_i(opts))
+   return rebase_path_todo();
return git_path_todo_file();
 }
 
@@ -122,7 +133,15 @@ int sequencer_remove_state(struct replay_opts *opts)
 
 static const char *action_name(const struct replay_opts *opts)
 {
-   return opts->action == REPLAY_REVERT ? N_("revert") : N_("cherry-pick");
+   switch (opts->action) {
+   case REPLAY_REVERT:
+   return N_("revert");
+   case REPLAY_PICK:
+   return N_("cherry-pick");
+   case REPLAY_INTERACTIVE_REBASE:
+   return N_("rebase -i");
+   }
+   die(_("Unknown action: %d"), opts->action);
 }
 
 struct commit_message {
@@ -364,7 +383,9 @@ static int do_recursive_merge(struct commit *base, struct 
commit *next,
 
if (active_cache_changed &&
write_locked_index(_index, _lock, COMMIT_LOCK))
-   /* TRANSLATORS: %s will be "revert" or "cherry-pick" */
+   /* TRANSLATORS: %s will be "revert", "cherry-pick" or
+* "rebase -i".
+*/
return error(_("%s: Unable to write new index file"),
_(action_name(opts)));
rollback_lock_file(_lock);
@@ -1198,6 +1219,13 @@ static int save_todo(struct todo_list *todo_list, struct 
replay_opts *opts)
const char *todo_path = get_todo_path(opts);
int next = todo_list->current, offset, fd;
 
+   /*
+* rebase -i writes "git-rebase-todo" without the currently executing
+* command, appending it to "done" instead.
+*/
+   if (is_rebase_i(opts))
+   next++;
+
fd = hold_lock_file_for_update(_lock, todo_path, 0);
if (fd < 0)
return error_errno(_("could not lock '%s'"), todo_path);
diff --git a/sequencer.h b/sequencer.h
index 7a513c576b..cb21cfddee 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -7,7 +7,8 @@ const char *git_path_seq_dir(void);
 
 enum replay_action {
REPLAY_REVERT,
-   REPLAY_PICK
+   REPLAY_PICK,
+   REPLAY_INTERACTIVE_REBASE
 };
 
 struct replay_opts {
-- 
2.11.0.rc3.windows.1

[PATCH v3 02/38] sequencer: move "else" keyword onto the same line as preceding brace

[Johannes Schindelin]

It is the current coding style of the Git project to write

if (...) {
...
} else {
...
}

instead of putting the closing brace and the "else" keyword on separate
lines.

Pointed out by Junio Hamano.

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

diff --git a/sequencer.c b/sequencer.c
index 23793db08b..3eededcb98 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1070,8 +1070,7 @@ static int create_seq_dir(void)
error(_("a cherry-pick or revert is already in progress"));
advise(_("try \"git cherry-pick (--continue | --quit | 
--abort)\""));
return -1;
-   }
-   else if (mkdir(git_path_seq_dir(), 0777) < 0)
+   } else if (mkdir(git_path_seq_dir(), 0777) < 0)
return error_errno(_("could not create sequencer directory 
'%s'"),
   git_path_seq_dir());
return 0;
-- 
2.11.0.rc3.windows.1

[PATCH v3 06/38] sequencer (rebase -i): implement the 'edit' command

[Johannes Schindelin]

This patch is a straight-forward reimplementation of the `edit`
operation of the interactive rebase command.

Well, not *quite* straight-forward: when stopping, the `edit`
command wants to write the `patch` file (which is not only the
patch, but includes the commit message and author information). To
that end, this patch requires the earlier work that taught the
log-tree machinery to respect the `file` setting of
rev_info->diffopt to write to a file stream different than stdout.

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

diff --git a/sequencer.c b/sequencer.c
index 84f18e64e9..b138a3906c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -17,6 +17,7 @@
 #include "argv-array.h"
 #include "quote.h"
 #include "trailer.h"
+#include "log-tree.h"
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
@@ -45,6 +46,20 @@ static GIT_PATH_FUNC(rebase_path_todo, 
"rebase-merge/git-rebase-todo")
  */
 static GIT_PATH_FUNC(rebase_path_author_script, "rebase-merge/author-script")
 /*
+ * When an "edit" rebase command is being processed, the SHA1 of the
+ * commit to be edited is recorded in this file.  When "git rebase
+ * --continue" is executed, if there are any staged changes then they
+ * will be amended to the HEAD commit, but only provided the HEAD
+ * commit is still the commit to be edited.  When any other rebase
+ * command is processed, this file is deleted.
+ */
+static GIT_PATH_FUNC(rebase_path_amend, "rebase-merge/amend")
+/*
+ * When we stop at a given patch via the "edit" command, this file contains
+ * the abbreviated commit name of the corresponding patch.
+ */
+static GIT_PATH_FUNC(rebase_path_stopped_sha, "rebase-merge/stopped-sha")
+/*
  * The following files are written by git-rebase just after parsing the
  * command-line (and are only consumed, not modified, by the sequencer).
  */
@@ -616,6 +631,7 @@ enum todo_command {
/* commands that handle commits */
TODO_PICK = 0,
TODO_REVERT,
+   TODO_EDIT,
/* commands that do nothing but are counted for reporting progress */
TODO_NOOP
 };
@@ -623,6 +639,7 @@ enum todo_command {
 static const char *todo_command_strings[] = {
"pick",
"revert",
+   "edit",
"noop"
 };
 
@@ -1302,9 +1319,87 @@ static int save_opts(struct replay_opts *opts)
return res;
 }
 
+static int make_patch(struct commit *commit, struct replay_opts *opts)
+{
+   struct strbuf buf = STRBUF_INIT;
+   struct rev_info log_tree_opt;
+   const char *subject, *p;
+   int res = 0;
+
+   p = short_commit_name(commit);
+   if (write_message(p, strlen(p), rebase_path_stopped_sha(), 1) < 0)
+   return -1;
+
+   strbuf_addf(, "%s/patch", get_dir(opts));
+   memset(_tree_opt, 0, sizeof(log_tree_opt));
+   init_revisions(_tree_opt, NULL);
+   log_tree_opt.abbrev = 0;
+   log_tree_opt.diff = 1;
+   log_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH;
+   log_tree_opt.disable_stdin = 1;
+   log_tree_opt.no_commit_id = 1;
+   log_tree_opt.diffopt.file = fopen(buf.buf, "w");
+   log_tree_opt.diffopt.use_color = GIT_COLOR_NEVER;
+   if (!log_tree_opt.diffopt.file)
+   res |= error_errno(_("could not open '%s'"), buf.buf);
+   else {
+   res |= log_tree_commit(_tree_opt, commit);
+   fclose(log_tree_opt.diffopt.file);
+   }
+   strbuf_reset();
+
+   strbuf_addf(, "%s/message", get_dir(opts));
+   if (!file_exists(buf.buf)) {
+   const char *commit_buffer = get_commit_buffer(commit, NULL);
+   find_commit_subject(commit_buffer, );
+   res |= write_message(subject, strlen(subject), buf.buf, 1);
+   unuse_commit_buffer(commit, commit_buffer);
+   }
+   strbuf_release();
+
+   return res;
+}
+
+static int intend_to_amend(void)
+{
+   unsigned char head[20];
+   char *p;
+
+   if (get_sha1("HEAD", head))
+   return error(_("cannot read HEAD"));
+
+   p = sha1_to_hex(head);
+   return write_message(p, strlen(p), rebase_path_amend(), 1);
+}
+
+static int error_with_patch(struct commit *commit,
+   const char *subject, int subject_len,
+   struct replay_opts *opts, int exit_code, int to_amend)
+{
+   if (make_patch(commit, opts))
+   return -1;
+
+   if (to_amend) {
+   if (intend_to_amend())
+   return -1;
+
+   fprintf(stderr, "You can amend the commit now, with\n"
+   "\n"
+   "  git commit --amend %s\n"
+   "\n"
+   "Once you are satisfied with your changes, run\n"
+   "\n"
+   "  git rebase --continue\n", gpg_sign_opt_quoted(opts));
+   } else if (exit_code)
+  

Re: [PATCH v2 05/34] sequencer (rebase -i): learn about the 'verbose' mode

[Johannes Schindelin]

Hi Junio,

On Tue, 13 Dec 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > @@ -1493,9 +1498,26 @@ static int pick_commits(struct todo_list *todo_list, 
> > struct replay_opts *opts)
> > }
> >  
> > if (is_rebase_i(opts)) {
> > +   struct strbuf buf = STRBUF_INIT;
> > +
> > /* Stopped in the middle, as planned? */
> > if (todo_list->current < todo_list->nr)
> > return 0;
> > +
> > +   if (opts->verbose) {
> > +   const char *argv[] = {
> > +   "diff-tree", "--stat", NULL, NULL
> > +   };
> > +
> > +   if (!read_oneliner(, rebase_path_orig_head(), 0))
> > +   return error(_("could not read '%s'"),
> > +   rebase_path_orig_head());
> > +   strbuf_addstr(, "..HEAD");
> > +   argv[2] = buf.buf;
> > +   run_command_v_opt(argv, RUN_GIT_CMD);
> > +   strbuf_reset();
> > +   }
> > +   strbuf_release();
> > }
> 
> It's a bit curious that the previous step avoided running a separate
> process and instead did "diff-tree -p" all in C, but this one does not.

I guess my only defence is that I tried to be a little lazy.

Fixed.
Dscho

Re: [PATCH 13/17] refs: convert each_reflog_ent_fn to struct object_id

[Michael Haggerty]

On 01/01/2017 08:18 PM, brian m. carlson wrote:
> Make each_reflog_ent_fn take two struct object_id pointers instead of
> two pointers to unsigned char.  Convert the various callbacks to use
> struct object_id as well.  Also, rename fsck_handle_reflog_sha1 to
> fsck_handle_reflog_oid.
> 
> Signed-off-by: brian m. carlson 
> ---
>  builtin/fsck.c   | 16 
>  builtin/merge-base.c |  6 +++---
>  builtin/reflog.c |  2 +-
>  reflog-walk.c|  6 +++---
>  refs.c   | 24 
>  refs.h   |  2 +-
>  refs/files-backend.c | 24 
>  revision.c   | 12 ++--
>  sha1_name.c  |  2 +-
>  wt-status.c  |  6 +++---
>  10 files changed, 50 insertions(+), 50 deletions(-)
> 
> [...]
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index f9023939d..3da3141ee 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -3113,15 +3113,15 @@ static int files_delete_reflog(struct ref_store 
> *ref_store,
>  
>  static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, 
> void *cb_data)
>  {
> - unsigned char osha1[20], nsha1[20];
> + struct object_id ooid, noid;
>   char *email_end, *message;
>   unsigned long timestamp;
>   int tz;
>  
>   /* old SP new SP name  SP time TAB msg LF */
>   if (sb->len < 83 || sb->buf[sb->len - 1] != '\n' ||
> - get_sha1_hex(sb->buf, osha1) || sb->buf[40] != ' ' ||
> - get_sha1_hex(sb->buf + 41, nsha1) || sb->buf[81] != ' ' ||
> + get_oid_hex(sb->buf, ) || sb->buf[40] != ' ' ||
> + get_oid_hex(sb->buf + 41, ) || sb->buf[81] != ' ' ||

Some magic numbers above could be converted to use constants.

>   !(email_end = strchr(sb->buf + 82, '>')) ||
>   email_end[1] != ' ' ||
>   !(timestamp = strtoul(email_end + 2, , 10)) ||
> @@ -3136,7 +3136,7 @@ static int show_one_reflog_ent(struct strbuf *sb, 
> each_reflog_ent_fn fn, void *c
>   message += 6;
>   else
>   message += 7;
> - return fn(osha1, nsha1, sb->buf + 82, timestamp, tz, message, cb_data);
> + return fn(, , sb->buf + 82, timestamp, tz, message, cb_data);

Here, too.

>  }
>  
>  static char *find_beginning_of_line(char *bob, char *scan)
> [...]
> @@ -4047,14 +4047,14 @@ static int files_reflog_expire(struct ref_store 
> *ref_store,
>*/
>   int update = (flags & EXPIRE_REFLOGS_UPDATE_REF) &&
>   !(type & REF_ISSYMREF) &&
> - !is_null_sha1(cb.last_kept_sha1);
> + !is_null_oid(_kept_oid);
>  
>   if (close_lock_file(_lock)) {
>   status |= error("couldn't write %s: %s", log_file,
>   strerror(errno));
>   } else if (update &&
>  (write_in_full(get_lock_file_fd(lock->lk),
> - sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
> + oid_to_hex(_kept_oid), 40) != 40 ||

More magic numbers above.

>   write_str_in_full(get_lock_file_fd(lock->lk), "\n") 
> != 1 ||
>   close_ref(lock) < 0)) {
>   status |= error("couldn't write %s",
> [...]

I thought it would make sense to convert `struct read_ref_at_cb` in
`refs.c` to use `struct object_id` at the same time, but I see that
would require the interface to `read_ref_at()` to change. I guess it's
important to pick your battles in this campaign :-)

Michael

Re: [PATCH v2 20/34] sequencer (rebase -i): copy commit notes at end

[Johannes Schindelin]

Hi Junio,

On Fri, 16 Dec 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > @@ -1750,6 +1797,17 @@ static int is_final_fixup(struct todo_list 
> > *todo_list)
> > return 1;
> >  }
> >  
> > +static enum todo_command peek_command(struct todo_list *todo_list, int 
> > offset)
> > +{
> > +   int i;
> > +
> > +   for (i = todo_list->current + offset; i < todo_list->nr; i++)
> > +   if (todo_list->items[i].command != TODO_NOOP)
> > +   return todo_list->items[i].command;
> 
> Makes me wonder, after having commented on 07/34 regarding the fact
> that in the end you would end up having three variants of no-op
> (i.e. NOOP, DROP and COMMENT), what definition of a "command" this
> function uses to return its result, when asked to "peek".

Well, it uses the todo_command idea of a "command"... ;-)

The only thing we do with this for now is to look whether the next command
is a fixup/squash (so that the user gets to edit the commit message just
once, for example, and also to record rewritten commits properly).

> I suspect that this will be updated in a later patch to do "< TODO_NOOP"
> instead?

Actually, no. I introduced a new function is_noop() and that is used now.

Ciao,
Dscho

Re: [PATCH v2 28/34] run_command_opt(): optionally hide stderr when the command succeeds

[Johannes Schindelin]

Hi Hannes,

On Wed, 14 Dec 2016, Johannes Sixt wrote:

> Am 14.12.2016 um 14:06 schrieb Jeff King:
> > On Wed, Dec 14, 2016 at 07:53:23AM -0500, Jeff King wrote:
> >
> > > I don't have a strong opinion on the patches under discussion, but
> > > here are a few pointers on the run-command interface:
> > > [...]
> >
> > And here is a patch representing my suggestions, on top of yours. Not
> > tested beyond "make test".
> 
> Thank you, that looks way better.
> 
> If there is agreement that this approach is preferable, I think we can
> have patches on top of the series; they would be orthogonal and do not
> have to take hostage of it. (And it looks like I won't be able to follow
> up until later this week[end].)

Seeing as the original intention was to do away with the
RUN_HIDE_STDERR_ON_SUCCESS flag, and that the sequencer-i branch *must*
include that functionality somehow, it is unfortunately not really
possible to do this on top of the patch series.

I say "unfortunately" because I feel pretty uncomfortable with replacing
something that has been tried and tested by something that still awaits
the test of time.

So the only possible course of action I see is to go the really long
route: incorporate the patches to use pipe_command() instead of
introducing a new RUN_* flag (which means basically munch up your patch
and Peff's and move it somewhere into the middle of the sequencer-i patch
series, which is exactly what I already did locally), cook the patches
beyond recognition in `next`, i.e. cook it really long to give it a really
good testing before moving the patches to `master`.

Ciao,
Johannes

Re: [PATCH v3 20/21] Documentation/config: add splitIndex.sharedIndexExpire

[Christian Couder]

On Tue, Dec 27, 2016 at 8:11 PM, Junio C Hamano  wrote:
> Christian Couder  writes:
>
>> Signed-off-by: Christian Couder 
>> ---
>>  Documentation/config.txt | 11 +++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index e0f5a77980..24e771d22e 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -2786,6 +2786,17 @@ splitIndex.maxPercentChange::
>>   than 20 percent of the total number of entries.
>>   See linkgit:git-update-index[1].
>>
>> +splitIndex.sharedIndexExpire::
>> + When the split index feature is used, shared index files with
>> + a mtime older than this time will be removed when a new shared
>
> As end-user facing documentation, it would be much better if we can
> rephrase it for those who do not know what a 'mtime' is, and it
> would be even better if we can do so without losing precision.
>
> I think "shared index files that were not modified since the time
> this variable specifies will be removed" would be understandable and
> correct enough?

Yeah, I agree it is better for end users. I will use what you suggest.

>> + index file is created. The value "now" expires all entries
>> + immediately, and "never" suppresses expiration altogether.
>> + The default value is "one.week.ago".
>> + Note that each time a new split-index file is created, the
>> + mtime of the related shared index file is updated to the
>> + current time.
>
> To match the above suggestion, "Note that a shared index file is
> considered modified (for the purpose of expiration) each time a new
> split-index file is created based on it."?

Yeah, I also agree it is better and will use that.

Re: [PATCH 09/17] builtin/merge: convert to struct object_id

[Michael Haggerty]

On 01/01/2017 08:18 PM, brian m. carlson wrote:
> Additionally convert several uses of the constant 40 into
> GIT_SHA1_HEXSZ.
> 
> Signed-off-by: brian m. carlson 
> ---
>  builtin/merge.c | 136 
> 
>  1 file changed, 68 insertions(+), 68 deletions(-)
> 
> [...]
> @@ -437,25 +437,25 @@ static void merge_name(const char *remote, struct 
> strbuf *msg)
>   strbuf_branchname(, remote);
>   remote = bname.buf;
>  
> - memset(branch_head, 0, sizeof(branch_head));
> + memset(_head, 0, sizeof(branch_head));

I think this could be

oidclr(_head);

>   remote_head = get_merge_parent(remote);
>   if (!remote_head)
>   die(_("'%s' does not point to a commit"), remote);
> [...]
> @@ -1113,9 +1113,9 @@ static struct commit_list *collect_parents(struct 
> commit *head_commit,
>  
>  int cmd_merge(int argc, const char **argv, const char *prefix)
>  {
> - unsigned char result_tree[20];
> - unsigned char stash[20];
> - unsigned char head_sha1[20];
> + struct object_id result_tree;
> + struct object_id stash;
> + struct object_id head_oid;

These could comfortably be declared on a single line now.

>   struct commit *head_commit;
>   struct strbuf buf = STRBUF_INIT;
>   const char *head_arg;
> [...]

Michael

Re: [PATCH v3 14/21] read-cache: touch shared index files when used

[Christian Couder]

On Tue, Dec 27, 2016 at 8:10 PM, Junio C Hamano  wrote:
> Christian Couder  writes:
>
>> +/*
>> + * Signal that the shared index is used by updating its mtime.
>> + *
>> + * This way, shared index can be removed if they have not been used
>> + * for some time. It's ok to fail to update the mtime if we are on a
>> + * read only file system.
>> + */
>> +void freshen_shared_index(char *base_sha1_hex)
>> +{
>> + const char *shared_index = git_path("sharedindex.%s", base_sha1_hex);
>> + check_and_freshen_file(shared_index, 1);
>
> What happens when this call fails?  The function returns 0 if the
> file did not even exist.  It also returns 0 if you cannot update its
> timestamp.

Yeah and I don't think it's a problem in either case.
If we cannot update its timestamp, it's not a problem, as we could be
on a read-only file system, and you said in a previous iteration that
we should not even warn in this case.
If the file does not exist, it could be because it has just been
deleted for a good reason, and anyway, if it is a problem that the
shared index file has been deleted, it is better addressed when we
will actually need the shared index file to read something into from
it, rather than just to update its mtime.

> Shouldn't the series be exposing freshen_file() instead _and_ taking
> its error return value seriously?

So what should we do if freshen_file() returns 0 which means that the
freshening failed?

>> +}

Re: [PATCH v3 12/21] Documentation/config: add splitIndex.maxPercentChange

[Christian Couder]

On Tue, Dec 27, 2016 at 8:09 PM, Junio C Hamano  wrote:
> Christian Couder  writes:
>
>> Signed-off-by: Christian Couder 
>> ---
>>  Documentation/config.txt | 13 +
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 221c5982c0..e0f5a77980 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -2773,6 +2773,19 @@ showbranch.default::
>>   The default set of branches for linkgit:git-show-branch[1].
>>   See linkgit:git-show-branch[1].
>>
>> +splitIndex.maxPercentChange::
>> + When the split index feature is used, this specifies the
>> + percent of entries the split index can contain compared to the
>> + whole number of entries in both the split index and the shared
>
> s/whole/total/ to match the last sentence of this section, perhaps?
> "The number of all entries" would also be OK, but "the whole number
> of entries" sounds a bit strange.

Yeah "total" is better than "whole" here. I will use "total".

Re: [PATCH] don't use test_must_fail with grep

[Pranit Bauva]

Hey Johannes,

On Sun, Jan 1, 2017 at 8:20 PM, Johannes Sixt  wrote:
> which makes me wonder: Is the message that we do expect not to occur
> actually printed on stdout? It sounds much more like an error message, i.e.,
> text that is printed on stderr. Wouldn't we need this?
>
> git p4 commit >actual 2>&1 &&
> ! grep "git author.*does not match" actual &&
>
> -- Hannes

This seems better! Since I am at it, I can remove the traces of pipes
in an another patch.

Regards,
Pranit Bauva

[PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND

[Johannes Schindelin]

From: Segev Finer 

Git for Windows has special support for the popular SSH client PuTTY:
when using PuTTY's non-interactive version ("plink.exe"), we use the -P
option to specify the port rather than OpenSSH's -p option. TortoiseGit
ships with its own, forked version of plink.exe, that adds support for
the -batch option, and for good measure we special-case that, too.

However, this special-casing of PuTTY only covers the case where the
user overrides the SSH command via the environment variable GIT_SSH
(which allows specifying the name of the executable), not
GIT_SSH_COMMAND (which allows specifying a full command, including
additional command-line options).

When users want to pass any additional arguments to (Tortoise-)Plink,
such as setting a private key, they are required to either use a shell
script named plink or tortoiseplink or duplicate the logic that is
already in Git for passing the correct style of command line arguments,
which can be difficult, error prone and annoying to get right.

This patch simply reuses the existing logic and expands it to cover
GIT_SSH_COMMAND, too.

Note: it may look a little heavy-handed to duplicate the entire
command-line and then split it, only to extract the name of the
executable. However, this is not a performance-critical code path, and
the code is much more readable this way.

Signed-off-by: Segev Finer 
Signed-off-by: Johannes Schindelin 
---
Published-As: https://github.com/dscho/git/releases/tag/putty-w-args-v1
Fetch-It-Via: git fetch https://github.com/dscho/git putty-w-args-v1


Original Pull Request:
https://github.com/git-for-windows/git/pull/1006

 connect.c| 23 ---
 t/t5601-clone.sh | 15 +++
 2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/connect.c b/connect.c
index 8cb93b0720..c81f77001b 100644
--- a/connect.c
+++ b/connect.c
@@ -772,6 +772,7 @@ struct child_process *git_connect(int fd[2], const char 
*url,
int putty = 0, tortoiseplink = 0;
char *ssh_host = hostandport;
const char *port = NULL;
+   char *ssh_argv0 = NULL;
transport_check_allowed("ssh");
get_host_and_port(_host, );
 
@@ -792,10 +793,15 @@ struct child_process *git_connect(int fd[2], const char 
*url,
}
 
ssh = get_ssh_command();
-   if (!ssh) {
-   const char *base;
-   char *ssh_dup;
-
+   if (ssh) {
+   char *split_ssh = xstrdup(ssh);
+   const char **ssh_argv;
+
+   if (split_cmdline(split_ssh, _argv))
+   ssh_argv0 = xstrdup(ssh_argv[0]);
+   free(split_ssh);
+   free((void *)ssh_argv);
+   } else {
/*
 * GIT_SSH is the no-shell version of
 * GIT_SSH_COMMAND (and must remain so for
@@ -807,8 +813,11 @@ struct child_process *git_connect(int fd[2], const char 
*url,
if (!ssh)
ssh = "ssh";
 
-   ssh_dup = xstrdup(ssh);
-   base = basename(ssh_dup);
+   ssh_argv0 = xstrdup(ssh);
+   }
+
+   if (ssh_argv0) {
+   const char *base = basename(ssh_argv0);
 
tortoiseplink = !strcasecmp(base, 
"tortoiseplink") ||
!strcasecmp(base, "tortoiseplink.exe");
@@ -816,7 +825,7 @@ struct child_process *git_connect(int fd[2], const char 
*url,
!strcasecmp(base, "plink") ||
!strcasecmp(base, "plink.exe");
 
-   free(ssh_dup);
+   free(ssh_argv0);
}
 
argv_array_push(>args, ssh);
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index a433394200..5b228e2675 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -386,6 +386,21 @@ test_expect_success 'tortoiseplink is like putty, with 
extra arguments' '
expect_ssh "-batch -P 123" myhost src
 '
 
+test_expect_success 'double quoted plink.exe in GIT_SSH_COMMAND' '
+   copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink.exe" &&
+   GIT_SSH_COMMAND="\"$TRASH_DIRECTORY/plink.exe\" -v" \
+   git clone "[myhost:123]:src" ssh-bracket-clone-plink-3 &&
+   expect_ssh "-v -P 123" myhost src
+'
+
+SQ="'"
+test_expect_success 'single quoted plink.exe in 

Re: [PATCH v3 10/21] read-cache: regenerate shared index if necessary

[Christian Couder]

On Tue, Dec 27, 2016 at 8:08 PM, Junio C Hamano  wrote:
> Christian Couder  writes:
>
>> + case 0:
>> + return 1; /* 0% means always write a new shared index */
>> + case 100:
>> + return 0; /* 100% means never write a new shared index */
>> + default:
>> + ; /* do nothing: just use the configured value */
>> + }
>
> Just like you did in 04/21, write "break" to avoid mistakes made in
> the future, i.e.
>
> default:
> break; /* just use the configured value */

Ok, I will do that.

>> +
>> + /* Count not shared entries */
>> + for (i = 0; i < istate->cache_nr; i++) {
>> + struct cache_entry *ce = istate->cache[i];
>> + if (!ce->index)
>> + not_shared++;
>> + }
>> +
>> + return istate->cache_nr * max_split < not_shared * 100;
>
> On a 32-bit arch with 2G int and more than 20 million paths in the
> index, multiplying by max_split that can come close to 100 can
> theoretically cause integer overflow, but in practice it probably
> does not matter.  Or does it?

>From a cursory look a "struct cache_entry" takes at least 80 bytes
without counting the "char name[FLEX_ARRAY]" on a 32 bit machine, so I
don't think it would be a good idea to work on a repo with 20 million
paths on a 32 bit machine, but maybe theoretically it could be a
problem.

To be safe I think I will use:

return (int64_t)istate->cache_nr * max_split < (int64_t)not_shared * 100;

[RFC PATCH 1/5] error/warning framework: prepare for l10n

[Michael J Gruber]

Currently, errors, warnings etc. are output with a fixed prefix "error: "
etc. that is not subject to l10n.

Change the call signatures of error_routine() etc. so that they receive
the prefix as first argument.

Signed-off-by: Michael J Gruber 
---
 apply.c   |  2 +-
 apply.h   |  4 ++--
 daemon.c  |  3 ++-
 fast-import.c |  4 ++--
 git-compat-util.h | 10 +-
 http-backend.c|  4 ++--
 run-command.c |  4 ++--
 usage.c   | 48 
 8 files changed, 40 insertions(+), 39 deletions(-)

diff --git a/apply.c b/apply.c
index 2ed808d429..0f93792e1c 100644
--- a/apply.c
+++ b/apply.c
@@ -112,7 +112,7 @@ void clear_apply_state(struct apply_state *state)
/* >fn_table is cleared at the end of apply_patch() */
 }
 
-static void mute_routine(const char *msg, va_list params)
+static void mute_routine(const char *prefix, const char *msg, va_list params)
 {
/* do nothing */
 }
diff --git a/apply.h b/apply.h
index b3d6783d55..56b5622868 100644
--- a/apply.h
+++ b/apply.h
@@ -100,8 +100,8 @@ struct apply_state {
 * set_error_routine() or set_warn_routine() to install muting
 * routines when in verbosity_silent mode.
 */
-   void (*saved_error_routine)(const char *err, va_list params);
-   void (*saved_warn_routine)(const char *warn, va_list params);
+   void (*saved_error_routine)(const char *prefix, const char *err, 
va_list params);
+   void (*saved_warn_routine)(const char *prefix, const char *warn, 
va_list params);
 
/* These control whitespace errors */
enum apply_ws_error_action ws_error_action;
diff --git a/daemon.c b/daemon.c
index 473e6b6b63..cd52a25001 100644
--- a/daemon.c
+++ b/daemon.c
@@ -114,8 +114,9 @@ static void loginfo(const char *err, ...)
va_end(params);
 }
 
-static void NORETURN daemon_die(const char *err, va_list params)
+static void NORETURN daemon_die(const char *prefix, const char *err, va_list 
params)
 {
+   /* no need to pass down prefix */
logreport(LOG_ERR, err, params);
exit(1);
 }
diff --git a/fast-import.c b/fast-import.c
index f561ba833b..4576f12163 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -496,13 +496,13 @@ static void end_packfile(void);
 static void unkeep_all_packs(void);
 static void dump_marks(void);
 
-static NORETURN void die_nicely(const char *err, va_list params)
+static NORETURN void die_nicely(const char *prefix, const char *err, va_list 
params)
 {
static int zombie;
char message[2 * PATH_MAX];
 
vsnprintf(message, sizeof(message), err, params);
-   fputs("fatal: ", stderr);
+   fputs(prefix, stderr);
fputs(message, stderr);
fputc('\n', stderr);
 
diff --git a/git-compat-util.h b/git-compat-util.h
index 87237b092b..f1bf0a6749 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -439,11 +439,11 @@ static inline int const_error(void)
 #define error_errno(...) (error_errno(__VA_ARGS__), const_error())
 #endif
 
-extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, 
va_list params));
-extern void set_error_routine(void (*routine)(const char *err, va_list 
params));
-extern void (*get_error_routine(void))(const char *err, va_list params);
-extern void set_warn_routine(void (*routine)(const char *warn, va_list 
params));
-extern void (*get_warn_routine(void))(const char *warn, va_list params);
+extern void set_die_routine(NORETURN_PTR void (*routine)(const char *prefix, 
const char *err, va_list params));
+extern void set_error_routine(void (*routine)(const char *prefix, const char 
*err, va_list params));
+extern void (*get_error_routine(void))(const char *prefix, const char *err, 
va_list params);
+extern void set_warn_routine(void (*routine)(const char *prefix, const char 
*warn, va_list params));
+extern void (*get_warn_routine(void))(const char *prefix, const char *warn, 
va_list params);
 extern void set_die_is_recursing_routine(int (*routine)(void));
 extern void set_error_handle(FILE *);
 
diff --git a/http-backend.c b/http-backend.c
index eef0a361f4..5c235e8b52 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -577,12 +577,12 @@ static void service_rpc(struct strbuf *hdr, char 
*service_name)
 }
 
 static int dead;
-static NORETURN void die_webcgi(const char *err, va_list params)
+static NORETURN void die_webcgi(const char *prefix, const char *err, va_list 
params)
 {
if (dead <= 1) {
struct strbuf hdr = STRBUF_INIT;
 
-   vreportf("fatal: ", err, params);
+   vreportf(prefix, err, params);
 
http_status(, 500, "Internal Server Error");
hdr_nocache();
diff --git a/run-command.c b/run-command.c
index ca905a9e80..3133bf5320 100644
--- a/run-command.c
+++ b/run-command.c
@@ -618,9 +618,9 @@ static void *run_thread(void *data)
return (void *)ret;
 }
 
-static NORETURN void die_async(const 

[RFC PATCH 0/5] Localise error headers

[Michael J Gruber]

Currently, the headers "error: ", "warning: " etc. - generated by die(),
warning() etc. - are not localized, but we feed many localized error messages
into these functions so that we produce error messages with mixed localisation.

This series introduces variants of die() etc. that use localised variants of
the headers, i.e. _("error: ") etc., and are to be fed localized messages. So,
instead of die(_("not workee")), which would produce a mixed localisation (such
as "error: geht ned"), one should use die_(_("not workee")) (resulting in
"Fehler: geht ned").

In this implementation, the gettext call for the header and the body are done
in different places (error function vs. caller) but this call pattern seems to
be the easiest variant for the caller, because the message body has to be marked
for localisation in any case, and N_() requires more letters than _(), an extra
argument to die() etc. even more than the extra "_" in the function name.

1/5 prepares the error machinery
2/5 provides new variants error_() etc.
3/5 has coccinelli rules error(_(E)) -> error_(_(E)) etc.
4/5 applies the coccinelli patches

5/5 is not to be applied to the main tree, but helps you try out the feature:
it has changes to de.po and git.pot so that e.g. "git branch" has fully 
localised
error messages (see the recipe in the commit message).

Michael J Gruber (5):
  error/warn framework: prepare for l10n
  error/warn framework: provide localized variants
  error/warn framework framework: coccinelli rules
  error/warn framework: localize
  WIP: Feature demonstration

 advice.c   |   16 +-
 apply.c|   10 +-
 apply.h|4 +-
 archive.c  |6 +-
 attr.c |3 +-
 bisect.c   |2 +-
 branch.c   |4 +-
 builtin/add.c  |   20 +-
 builtin/am.c   |   27 +-
 builtin/archive.c  |   10 +-
 builtin/blame.c|   12 +-
 builtin/branch.c   |   45 +-
 builtin/bundle.c   |4 +-
 builtin/check-ignore.c |   14 +-
 builtin/check-mailmap.c|2 +-
 builtin/checkout-index.c   |2 +-
 builtin/checkout.c |   27 +-
 builtin/clean.c|   10 +-
 builtin/clone.c|   39 +-
 builtin/column.c   |2 +-
 builtin/commit.c   |   87 +-
 builtin/config.c   |2 +-
 builtin/describe.c |6 +-
 builtin/diff.c |2 +-
 builtin/fetch.c|   21 +-
 builtin/gc.c   |3 +-
 builtin/grep.c |   14 +-
 builtin/help.c |4 +-
 builtin/index-pack.c   |   42 +-
 builtin/interpret-trailers.c   |2 +-
 builtin/log.c  |   48 +-
 builtin/merge-recursive.c  |2 +-
 builtin/merge.c|   53 +-
 builtin/mv.c   |6 +-
 builtin/notes.c|   40 +-
 builtin/pack-objects.c |4 +-
 builtin/prune.c|2 +-
 builtin/pull.c |   10 +-
 builtin/push.c |   22 +-
 builtin/remote.c   |   10 +-
 builtin/repack.c   |4 +-
 builtin/replace.c  |2 +-
 builtin/reset.c|   10 +-
 builtin/rev-list.c |2 +-
 builtin/rev-parse.c|2 +-
 builtin/revert.c   |4 +-
 builtin/rm.c   |6 +-
 builtin/shortlog.c |2 +-
 builtin/show-branch.c  |7 +-
 builtin/submodule--helper.c|9 +-
 builtin/tag.c  |   20 +-
 builtin/unpack-objects.c   |2 +-
 builtin/update-index.c |8 +-
 builtin/worktree.c |6 +-
 bundle.c   |4 +-
 config.c   |4 +-
 connect.c  |6 +-
 connected.c|2 +-
 contrib/coccinelle/errorl10n.cocci |   47 +
 daemon.c   |3 +-
 diff.c |8 +-
 dir.c  |4 +-
 fast-import.c  |4 +-
 fetch-pack.c   |   26 +-
 git-compat-util.h  |   18 +-
 help.c |2 +-
 http-backend.c |4 +-
 http.c |4 +-
 merge.c|2 +-
 notes-utils.c  |2 +-
 pathspec.c |   13 +-
 po/de.po   | 2922 
 po/git.pot | 2781 ++
 

[RFC PATCH 3/5] error/warning framework framework: coccinelli rules

[Michael J Gruber]

Provide coccinelli rules which check for error(), warning() etc. with
localised argument and create a patch to replace them with error_(),
warning_() etc. in order to fully localize them.

Signed-off-by: Michael J Gruber 
---
 contrib/coccinelle/errorl10n.cocci | 47 ++
 1 file changed, 47 insertions(+)
 create mode 100644 contrib/coccinelle/errorl10n.cocci

diff --git a/contrib/coccinelle/errorl10n.cocci 
b/contrib/coccinelle/errorl10n.cocci
new file mode 100644
index 00..d62a440644
--- /dev/null
+++ b/contrib/coccinelle/errorl10n.cocci
@@ -0,0 +1,47 @@
+@@
+expression E;
+@@
+- usage(_(E));
++ usage_(_(E));
+
+@@
+expression E;
+@@
+- usagef(_(E));
++ usagef_(_(E));
+
+@@
+expression E;
+@@
+- die(_(E));
++ die_(_(E));
+
+@@
+expression E;
+@@
+- error(_(E));
++ error_(_(E));
+
+@@
+expression E;
+@@
+- warning(_(E));
++ warning_(_(E));
+
+@@
+expression E;
+@@
+- die_errno(_(E));
++ die_errno_(_(E));
+
+@@
+expression E;
+@@
+- error_errno(_(E));
++ error_errno_(_(E));
+
+@@
+expression E;
+@@
+- warning_errno(_(E));
++ warning_errno_(_(E));
-- 
2.11.0.372.g2fcea0e476

[RFC PATCH 2/5] error/warn framework: provide localized variants

[Michael J Gruber]

Provide localized variants of error(), warning(), die() etc.
to go along with localized messages.

Signed-off-by: Michael J Gruber 
---
 git-compat-util.h |  8 ++
 usage.c   | 74 +++
 2 files changed, 82 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index f1bf0a6749..48209a6c67 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -405,13 +405,21 @@ struct strbuf;
 /* General helper functions */
 extern void vreportf(const char *prefix, const char *err, va_list params);
 extern NORETURN void usage(const char *err);
+extern NORETURN void usage_(const char *err);
 extern NORETURN void usagef(const char *err, ...) __attribute__((format 
(printf, 1, 2)));
+extern NORETURN void usagef_(const char *err, ...) __attribute__((format 
(printf, 1, 2)));
 extern NORETURN void die(const char *err, ...) __attribute__((format (printf, 
1, 2)));
+extern NORETURN void die_(const char *err, ...) __attribute__((format (printf, 
1, 2)));
 extern NORETURN void die_errno(const char *err, ...) __attribute__((format 
(printf, 1, 2)));
+extern NORETURN void die_errno_(const char *err, ...) __attribute__((format 
(printf, 1, 2)));
 extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
+extern int error_(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern int error_errno(const char *err, ...) __attribute__((format (printf, 1, 
2)));
+extern int error_errno_(const char *err, ...) __attribute__((format (printf, 
1, 2)));
 extern void warning(const char *err, ...) __attribute__((format (printf, 1, 
2)));
+extern void warning_(const char *err, ...) __attribute__((format (printf, 1, 
2)));
 extern void warning_errno(const char *err, ...) __attribute__((format (printf, 
1, 2)));
+extern void warning_errno_(const char *err, ...) __attribute__((format 
(printf, 1, 2)));
 
 #ifndef NO_OPENSSL
 #ifdef APPLE_COMMON_CRYPTO
diff --git a/usage.c b/usage.c
index 4270b04bf9..d0cfb02a64 100644
--- a/usage.c
+++ b/usage.c
@@ -105,11 +105,25 @@ void NORETURN usagef(const char *err, ...)
va_end(params);
 }
 
+void NORETURN usagef_(const char *err, ...)
+{
+   va_list params;
+
+   va_start(params, err);
+   usage_routine(_("usage: "), err, params);
+   va_end(params);
+}
+
 void NORETURN usage(const char *err)
 {
usagef("%s", err);
 }
 
+void NORETURN usage_(const char *err)
+{
+   usagef_("%s", err);
+}
+
 void NORETURN die(const char *err, ...)
 {
va_list params;
@@ -124,6 +138,20 @@ void NORETURN die(const char *err, ...)
va_end(params);
 }
 
+void NORETURN die_(const char *err, ...)
+{
+   va_list params;
+
+   if (die_is_recursing()) {
+   fputs(_("fatal: recursion detected in die handler\n"), stderr);
+   exit(128);
+   }
+
+   va_start(params, err);
+   die_routine(_("fatal: "), err, params);
+   va_end(params);
+}
+
 static const char *fmt_with_err(char *buf, int n, const char *fmt)
 {
char str_error[256], *err;
@@ -163,6 +191,22 @@ void NORETURN die_errno(const char *fmt, ...)
va_end(params);
 }
 
+void NORETURN die_errno_(const char *fmt, ...)
+{
+   char buf[1024];
+   va_list params;
+
+   if (die_is_recursing()) {
+   fputs(_("fatal: recursion detected in die_errno handler\n"),
+   stderr);
+   exit(128);
+   }
+
+   va_start(params, fmt);
+   die_routine(_("fatal: "), fmt_with_err(buf, sizeof(buf), fmt), params);
+   va_end(params);
+}
+
 #undef error_errno
 int error_errno(const char *fmt, ...)
 {
@@ -175,6 +219,17 @@ int error_errno(const char *fmt, ...)
return -1;
 }
 
+int error_errno_(const char *fmt, ...)
+{
+   char buf[1024];
+   va_list params;
+
+   va_start(params, fmt);
+   error_routine(_("error: "), fmt_with_err(buf, sizeof(buf), fmt), 
params);
+   va_end(params);
+   return -1;
+}
+
 #undef error
 int error(const char *err, ...)
 {
@@ -186,6 +241,16 @@ int error(const char *err, ...)
return -1;
 }
 
+int error_(const char *err, ...)
+{
+   va_list params;
+
+   va_start(params, err);
+   error_routine(_("error: "), err, params);
+   va_end(params);
+   return -1;
+}
+
 void warning_errno(const char *warn, ...)
 {
char buf[1024];
@@ -204,3 +269,12 @@ void warning(const char *warn, ...)
warn_routine("warning: ", warn, params);
va_end(params);
 }
+
+void warning_(const char *warn, ...)
+{
+   va_list params;
+
+   va_start(params, warn);
+   warn_routine(_("warning: "), warn, params);
+   va_end(params);
+}
-- 
2.11.0.372.g2fcea0e476

Re: Error: could not fork child process: Resource temporarily unavailable (-1)

[Johannes Schindelin]

Hi,

On Mon, 2 Jan 2017, M. Abuhena Sobuj wrote:

> My Git Bash was perfect but suddenly It stooped work for me.
> 
> I just got message
> "Error: could not fork child process: Resource temporarily unavailable (-1).
> DLL rebasing may be required. See 'rebaseall / rebase --help'."

Is this a 32-bit system? If so, simply try to reinstall Git for Windows,
that should make it work again [*1*].

If that does not solve the problem, you will want to open a ticket at
https://github.com/git-for-windows/git/issues/new

Ciao,
Johannes

Footnote *1*: https://github.com/git-for-windows/git/wiki/32-bit-issues

[PATCH v2] mingw: add a regression test for pushing to UNC paths

[Johannes Schindelin]

On Windows, there are "UNC paths" to access network (AKA shared)
folders, of the form \\server\sharename\directory. This provides a
convenient way for Windows developers to share their Git repositories
without having to have a dedicated server.

Git for Windows v2.11.0 introduced a regression where pushing to said
UNC paths no longer works, although fetching and cloning still does, as
reported here: https://github.com/git-for-windows/git/issues/979

This regression was fixed in 7814fbe3f1 (normalize_path_copy(): fix
pushing to //server/share/dir on Windows, 2016-12-14).

Let's make sure that it does not regress again, by introducing a test
that uses so-called "administrative shares": disk volumes are
automatically shared under certain circumstances, e.g.  the C: drive is
shared as \\localhost\c$. The test needs to be skipped if the current
directory is inaccessible via said administrative share, of course.

Signed-off-by: Johannes Schindelin 
---
Published-As: https://github.com/dscho/git/releases/tag/unc-paths-test-v2
Fetch-It-Via: git fetch https://github.com/dscho/git unc-paths-test-v2
Interdiff vs v1:

 diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
 index e06d230724..b195f71ea9 100755
 --- a/t/t5580-clone-push-unc.sh
 +++ b/t/t5580-clone-push-unc.sh
 @@ -40,7 +40,9 @@ test_expect_success push '
git checkout -b to-push &&
test_commit to-push &&
git push origin HEAD
 -  )
 +  ) &&
 +  rev="$(git -C clone rev-parse --verify refs/heads/to-push)" &&
 +  test "$rev" = "$(git rev-parse --verify refs/heads/to-push)"
  '
  
  test_done


 t/t5580-clone-push-unc.sh | 48 +++
 1 file changed, 48 insertions(+)
 create mode 100755 t/t5580-clone-push-unc.sh

diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
new file mode 100755
index 00..b195f71ea9
--- /dev/null
+++ b/t/t5580-clone-push-unc.sh
@@ -0,0 +1,48 @@
+#!/bin/sh
+
+test_description='various UNC path tests (Windows-only)'
+. ./test-lib.sh
+
+if ! test_have_prereq MINGW; then
+   skip_all='skipping UNC path tests, requires Windows'
+   test_done
+fi
+
+UNCPATH="$(pwd)"
+case "$UNCPATH" in
+[A-Z]:*)
+   # Use administrative share e.g. \\localhost\C$\git-sdk-64\usr\src\git
+   # (we use forward slashes here because MSYS2 and Git accept them, and
+   # they are easier on the eyes)
+   UNCPATH="//localhost/${UNCPATH%%:*}\$/${UNCPATH#?:}"
+   test -d "$UNCPATH" || {
+   skip_all='could not access administrative share; skipping'
+   test_done
+   }
+   ;;
+*)
+   skip_all='skipping UNC path tests, cannot determine current path as UNC'
+   test_done
+   ;;
+esac
+
+test_expect_success setup '
+   test_commit initial
+'
+
+test_expect_success clone '
+   git clone "file://$UNCPATH" clone
+'
+
+test_expect_success push '
+   (
+   cd clone &&
+   git checkout -b to-push &&
+   test_commit to-push &&
+   git push origin HEAD
+   ) &&
+   rev="$(git -C clone rev-parse --verify refs/heads/to-push)" &&
+   test "$rev" = "$(git rev-parse --verify refs/heads/to-push)"
+'
+
+test_done

base-commit: c69c2f50cfc0dcd4bcd014c7fd56e344a7c5522f
-- 
2.11.0.rc3.windows.1

Re: [PATCH] mingw: add a regression test for pushing to UNC paths

[Johannes Schindelin]

Hi Hannes,

On Fri, 23 Dec 2016, Johannes Sixt wrote:

> Am 23.12.2016 um 18:11 schrieb Johannes Schindelin:
> 
> > +test_expect_success setup '
> > +   test_commit initial
> > +'
> > +
> > +test_expect_success clone '
> > +   git clone "file://$UNCPATH" clone
> > +'
> > +
> > +test_expect_success push '
> > +   (
> > +   cd clone &&
> > +   git checkout -b to-push &&
> > +   test_commit to-push &&
> > +   git push origin HEAD
> > +   )
> > +'
> > +
> > +test_done
> 
> Wouldn't at a minimum
> 
> test_expect_success 'check push result' '
>   git rev-parse to-push
> '
> 
> be a good idea to make sure that the pushed commit actually arrived?

Sure.

Ciao,
Dscho

Error: could not fork child process: Resource temporarily unavailable (-1)

[M. Abuhena Sobuj]

Hello sir,
Good day.

My Git Bash was perfect but suddenly It stooped work for me.

I just got message
"Error: could not fork child process: Resource temporarily unavailable (-1).
DLL rebasing may be required. See 'rebaseall / rebase --help'."

Please help me to resolve this issue.

Thank you.

I'm Windows 10 User.

Re: [PATCH v3 08/21] Documentation/git-update-index: talk about core.splitIndex config var

[Christian Couder]

On Tue, Dec 27, 2016 at 8:07 PM, Junio C Hamano  wrote:
> Christian Couder  writes:
>
>> Signed-off-by: Christian Couder 
>> ---
>>  Documentation/git-update-index.txt | 6 ++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/Documentation/git-update-index.txt 
>> b/Documentation/git-update-index.txt
>> index 7386c93162..e091b2a409 100644
>> --- a/Documentation/git-update-index.txt
>> +++ b/Documentation/git-update-index.txt
>> @@ -171,6 +171,12 @@ may not support it yet.
>>   given again, all changes in $GIT_DIR/index are pushed back to
>>   the shared index file. This mode is designed for very large
>>   indexes that take a significant amount of time to read or write.
>> ++
>> +These options take effect whatever the value of the `core.splitIndex`
>> +configuration variable (see linkgit:git-config[1]).
>
> Doesn't the "whatever..." clause in this sentence lack its verb
> (presumably, "is", right after "variable")?

I think that it is ok to have no verb here. My preferred online
dictionary (http://www.wordreference.com/enfr/whatever) gives "We'll
try to free the hostage whatever the cost." as an example with no
verb, though I am not a native speaker.

Also note that I mostly copied what I had already written for
--(no-)untracked-cache:

--untracked-cache::
--no-untracked-cache::
Enable or disable untracked cache feature. Please use
`--test-untracked-cache` before enabling it.
+
These options take effect whatever the value of the `core.untrackedCache`
configuration variable (see linkgit:git-config[1]). But a warning is
emitted when the change goes against the configured value, as the
configured value will take effect next time the index is read and this
will remove the intended effect of the option.

>> +emitted when the change goes against the configured value, as the
>> +configured value will take effect next time the index is read and this
>> +will remove the intended effect of the option.
>
> It feels somewhat strange to see a warning in this case.

We already discussed the warning issue for --(no-)untracked-cache, and
I am just following the conclusions of that previous discussion about
--(no-)untracked-cache. See the end of this message where you
suggested the warning:

https://public-inbox.org/git/xmqqlh8cg9y2@gitster.mtv.corp.google.com/

> If the user configures the system to usually do X, and issues a
> command that explicitly does Y (or "not-X"), we do not warn for the
> command invocation if it is a single-shot operation.  That's the
> usual "command line overrides configuration" an end-user expects.
>
> I think you made this deviate from the usual end-user expectation
> because it is unpredictable when the configuration kicks in the next
> time to undo the effect of this command line request.  And I agree
> that it is a very un-nice thing to do to the users if we did not
> warn here, if we are to keep that unpredictableness.

I also think it would be strange and user unfriendly for
--untracked-cache and --split-index to behave differently, and I am
reluctant at this point to rework the way it works for
--untracked-cache.

> But stepping back a bit, we know the user's intention is that with
> "--split-index" the user wants to use the split index, even though
> the user may have configuration not to use the split index.  Don't
> we want to honor that intention?  In order to honor that intention
> without getting confused by the configured variable to tell us not
> to split, another way is to destroy that unpredictableness.  For
> that, shouldn't we automatically remove the configuration that says
> "do not split" (by perhaps set it to "do split")?  Doing so still
> may need a warning that says "we disabled your previously configured
> setting while at it", but it would be much better than a warning
> message that essentially says "we do it once, but we will disobey
> you at an unpredictable time", I would think.

I wanted to do that for --untracked-cache around one year ago but in
the following message:

https://public-inbox.org/git/xmqqsi3ckadi@gitster.mtv.corp.google.com/

you wrote the following:

"Why is it a good idea to allow an explicit operation from the
command line to muck with the configuration?  If the user wants to
change the configuration permanently, "git config" has been there
for the purpose for all the configuration variables to do so for
almost forever.  Why is it a good idea to make this variable so
special that the user does not have to use "git config" but disable
or enable it in some other way?"

It feels strange that when I do things one way, you suggest another
way, and the next time in a similar situation when I do things the way
you suggested previously, then you suggest the way I did it initially
the first time...

[PATCH] completion: complete git submodule subcommands

[Denton Liu]

Allow git submodule subcommands to be completed. This allows the
'--remote' in the command 'git submodule update --remote', for example,
to be fully completed.

Signed-off-by: Denton Liu 
---
Hi Shawn, sorry this is my first contribution to a mailing-list based
project. If I've done anything wrong, please let me know.

Thanks,

Denton Liu

---
 contrib/completion/git-completion.bash | 46 ++
 1 file changed, 35 insertions(+), 11 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 21016bf8d..941fbdfe2 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2556,17 +2556,41 @@ _git_submodule ()
__git_has_doubledash && return
 
local subcommands="add status init deinit update summary foreach sync"
-   if [ -z "$(__git_find_on_cmdline "$subcommands")" ]; then
-   case "$cur" in
-   --*)
-   __gitcomp "--quiet --cached"
-   ;;
-   *)
-   __gitcomp "$subcommands"
-   ;;
-   esac
-   return
-   fi
+   local subcommand="$(__git_find_on_cmdline "$subcommands")"
+
+   case "$subcommand,$cur" in
+   ,--*)
+   __gitcomp "--quiet"
+   ;;
+   ,*)
+   __gitcomp "$subcommands --quiet"
+   ;;
+   add,--*)
+   __gitcomp "--force --name --reference --depth"
+   ;;
+   status,--*)
+   __gitcomp "--cached --recursive"
+   ;;
+   deinit,--*)
+   __gitcomp "--force --all"
+   ;;
+   update,--*)
+   __gitcomp "
+   --init --remote --no-fetch --no-recommended-shallow
+   --recommended-shallow --force --rebase --merge 
--reference
+   --depth --recursive --jobs
+   "
+   ;;
+   summary,--*)
+   __gitcomp "--cached --files --summary-limit"
+   ;;
+   summary,*)
+   __gitcomp_nl "$(__git_refs)"
+   ;;
+   foreach,--*|sync,--*)
+   __gitcomp "--recursive"
+   ;;
+   esac
 }
 
 _git_svn ()
-- 
2.11.0

Re: [PATCH v3 06/21] t1700: add tests for core.splitIndex

[Christian Couder]

On Tue, Dec 27, 2016 at 8:04 PM, Junio C Hamano  wrote:
> Christian Couder  writes:
>
>> +test_expect_success 'set core.splitIndex config variable to true' '
>> + git config core.splitIndex true &&
>> + : >three &&
>> + git update-index --add three &&
>> + git ls-files --stage >ls-files.actual &&
>> + cat >ls-files.expect <> +100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0one
>> +100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0three
>> +100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0two
>> +EOF
>> + test_cmp ls-files.expect ls-files.actual &&
>
> It does not add much value to follow the "existing" outdated style
> like this when you are only adding new tests.  Write these like
>
> cat >ls-files.expect <<-\EOF &&
> 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0   one
> 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0   three
> 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0   two
> EOF
>
> which would give incentive to others (or yourself) to update the
> style of the existing mess ;-).

Ok, I will add a patch to update the style of the existing tests at
the beginning of the series and then use the same new style in the
tests I add in later patches.