[RFC/PATCH 2/2] WIP xdiff: markup duplicates differently

[Stefan Beller]

When moving code (e.g. a function is moved to another part of the file or
to a different file), the review process is different than reviewing new
code. When reviewing moved code we are only interested in the diff as
where there are differences in the moved code, e.g. namespace changes.

However the inner part of these moved texts should not change.
To aid a developer reviewing such code, emit it with a different prefix
than the usual +,- to indicate it is overlapping code.

Examples from recent history:
git show e28eae3184b26d3cf3293e69403babb5c575342c
git show bc9204d4ef6e0672389fdfb0d398fa9a39dba3d5
git show 8465541e8ce8eaf16e66ab847086779768c18f2d

This doesn't work yet, but we should make this patch series work
to ignore white space changes:
9d1ca1dac0ebfd6e17d73e33b2d173926c139c2d

Signed-off-by: Stefan Beller 
---
 xdiff/xdiff.h |   1 +
 xdiff/xemit.c | 128 +-
 2 files changed, 127 insertions(+), 2 deletions(-)

diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index 7423f77..0744e01 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -45,6 +45,7 @@ extern "C" {
 
 #define XDL_EMIT_FUNCNAMES (1 << 0)
 #define XDL_EMIT_FUNCCONTEXT (1 << 2)
+#define XDL_EMIT_DUPLICATE (1 << 3)
 
 #define XDL_MMB_READONLY (1 << 0)
 
diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index b52b4b9..4abafae 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -22,6 +22,9 @@
 
 #include "xinclude.h"
 
+#include "git-compat-util.h"
+#include "hashmap.h"
+
 static long xdl_get_rec(xdfile_t *xdf, long ri, char const **rec) {
 
*rec = xdf->recs[ri]->ptr;
@@ -158,12 +161,133 @@ static int is_empty_rec(xdfile_t *xdf, long ri)
return !len;
 }
 
+struct hashmap *duplicates_added;
+struct hashmap *duplicates_removed;
+
+struct dup_entry {
+   struct hashmap_entry ent;
+   xdfile_t *xdf;
+   long index;
+};
+
+static int dup_entry_cmp(const struct dup_entry *a,
+  const struct dup_entry *b,
+  const void *unused)
+{
+   int d = XDL_MIN(strcspn(a->xdf->recs[a->index]->ptr, "\n"),
+   strcspn(b->xdf->recs[b->index]->ptr, "\n"));
+
+   if (!strncmp(a->xdf->recs[a->index]->ptr,
+   b->xdf->recs[b->index]->ptr,
+   d))
+   return 0;
+   return 1;
+}
+
+struct dup_entry *prepare_entry(xdfile_t *xdf, long ri)
+{
+   long range_start = XDL_MAX(0, ri - 2);
+   long range_end = XDL_MIN(xdf->nrec, ri + 2);
+   long hash = 0;
+   int i;
+   struct dup_entry *ret = xmalloc(sizeof(*ret));
+
+   for (i = range_start; i < range_end; i++)
+   hash ^= memhash(xdf->recs[i]->ptr, xdf->recs[i]->size);
+
+   ret->ent.hash = hash;
+   ret->xdf = xdf;
+   ret->index = ri;
+   return ret;
+}
+
+int add_removal(xdfile_t *xdf, long ri)
+{
+   hashmap_add(duplicates_removed, prepare_entry(xdf, ri));
+   return 0;
+}
+
+int add_addition(xdfile_t *xdf, long ri)
+{
+   hashmap_add(duplicates_added, prepare_entry(xdf, ri));
+   return 0;
+}
+
+int xdl_markup_duplicates(xdfenv_t *xe, xdchange_t *xscr,
+ xdemitconf_t const *xecfg)
+{
+   long s1, s2;
+   xdchange_t *xch, *xche;
+
+   duplicates_added = xmalloc(sizeof(*duplicates_added));
+   duplicates_removed = xmalloc(sizeof(*duplicates_removed));
+   hashmap_init(duplicates_added, (hashmap_cmp_fn)dup_entry_cmp, 0);
+   hashmap_init(duplicates_removed, (hashmap_cmp_fn)dup_entry_cmp, 0);
+
+   for (xch = xscr; xch; xch = xche->next) {
+   xche = xdl_get_hunk(, xecfg);
+   if (!xch)
+   break;
+
+   for (s1 = xch->i1, s2 = xch->i2;; xch = xch->next) {
+
+   /*
+* Removes lines from the first file.
+*/
+   for (s1 = xch->i1; s1 < xch->i1 + xch->chg1; s1++)
+   if (add_removal(>xdf1, s1) < 0)
+   return -1;
+
+   /*
+* Adds lines from the second file.
+*/
+   for (s2 = xch->i2; s2 < xch->i2 + xch->chg2; s2++)
+   if (add_addition(>xdf2, s2) < 0)
+   return -1;
+
+   if (xch == xche)
+   break;
+   s1 = xch->i1 + xch->chg1;
+   s2 = xch->i2 + xch->chg2;
+   }
+   }
+   return 0;
+}
+
+static int xdl_check_and_emit_record(xdfile_t *xdf, long ri,
+char *pre, xdemitcb_t *ecb,
+int duplicate_handling)
+{
+   const char *hacked_pre = pre;
+
+   if (duplicate_handling) {
+   struct dup_entry *keydata = prepare_entry(xdf, 

[RFC/PATCH 0/2] Color moved code differently

[Stefan Beller]

When moving code (e.g. a function is moved to another part of the file or
to a different file), the review process is different than reviewing new
code. When reviewing moved code we are only interested in the diff as
where there are differences in the moved code, e.g. namespace changes.

However the inner part of these moved texts should not change.
To aid a developer reviewing such code, we'll color pure moved stuff
differently.

A line is colored differently if that line and the surroundign 2 lines
appear as-is in the opposite part of the diff.

Example:
http://i.imgur.com/ay84q0q.png

Or apply these patches and 
git show e28eae3184b26d3cf3293e69403babb5c575342c
git show bc9204d4ef6e0672389fdfb0d398fa9a39dba3d5
git show 8465541e8ce8eaf16e66ab847086779768c18f2d

The code quality sucks though, hence RFC.

Thanks,
Stefan

Stefan Beller (2):
  diff.c: emit duplicate lines with a different color
  WIP xdiff: markup duplicates differently

 diff.c|  26 
 diff.h|   4 +-
 xdiff/xdiff.h |   1 +
 xdiff/xemit.c | 128 +-
 4 files changed, 156 insertions(+), 3 deletions(-)

-- 
2.10.0.rc2.23.gf336a1a.dirty

[PATCH 1/2] diff.c: emit duplicate lines with a different color

[Stefan Beller]

Color is WIP, I just make space for a different case.

Signed-off-by: Stefan Beller 
---
 diff.c | 26 ++
 diff.h |  4 +++-
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index 534c12e..57d435c 100644
--- a/diff.c
+++ b/diff.c
@@ -52,6 +52,8 @@ static char diff_colors[][COLOR_MAXLEN] = {
GIT_COLOR_YELLOW,   /* COMMIT */
GIT_COLOR_BG_RED,   /* WHITESPACE */
GIT_COLOR_NORMAL,   /* FUNCINFO */
+   GIT_COLOR_BLUE, /* NEW DUPLICATE */
+   GIT_COLOR_CYAN, /* OLD DUPLICATE */
 };
 
 static int parse_diff_color_slot(const char *var)
@@ -541,6 +543,14 @@ static void emit_add_line(const char *reset,
  DIFF_FILE_NEW, WSEH_NEW, '+');
 }
 
+static void emit_add_line_dup(const char *reset,
+ struct emit_callback *ecbdata,
+ const char *line, int len)
+{
+   emit_line_checked(reset, ecbdata, line, len,
+ DIFF_FILE_DUPLICATE_NEW, WSEH_NEW, '+');
+}
+
 static void emit_del_line(const char *reset,
  struct emit_callback *ecbdata,
  const char *line, int len)
@@ -549,6 +559,14 @@ static void emit_del_line(const char *reset,
  DIFF_FILE_OLD, WSEH_OLD, '-');
 }
 
+static void emit_del_line_dup(const char *reset,
+ struct emit_callback *ecbdata,
+ const char *line, int len)
+{
+   emit_line_checked(reset, ecbdata, line, len,
+ DIFF_FILE_DUPLICATE_OLD, WSEH_OLD, '-');
+}
+
 static void emit_context_line(const char *reset,
  struct emit_callback *ecbdata,
  const char *line, int len)
@@ -1300,6 +1318,10 @@ static void fn_out_consume(void *priv, char *line, 
unsigned long len)
}
 
switch (line[0]) {
+   case '*':
+   ecbdata->lno_in_postimage++;
+   emit_add_line_dup(reset, ecbdata, line + 1, len - 1);
+   break;
case '+':
ecbdata->lno_in_postimage++;
emit_add_line(reset, ecbdata, line + 1, len - 1);
@@ -1308,6 +1330,10 @@ static void fn_out_consume(void *priv, char *line, 
unsigned long len)
ecbdata->lno_in_preimage++;
emit_del_line(reset, ecbdata, line + 1, len - 1);
break;
+   case '~':
+   ecbdata->lno_in_preimage++;
+   emit_del_line_dup(reset, ecbdata, line + 1, len - 1);
+   break;
case ' ':
ecbdata->lno_in_postimage++;
ecbdata->lno_in_preimage++;
diff --git a/diff.h b/diff.h
index 7883729..d500f0e 100644
--- a/diff.h
+++ b/diff.h
@@ -189,7 +189,9 @@ enum color_diff {
DIFF_FILE_NEW = 5,
DIFF_COMMIT = 6,
DIFF_WHITESPACE = 7,
-   DIFF_FUNCINFO = 8
+   DIFF_FUNCINFO = 8,
+   DIFF_FILE_DUPLICATE_NEW = 9,
+   DIFF_FILE_DUPLICATE_OLD = 10
 };
 const char *diff_get_color(int diff_use_color, enum color_diff ix);
 #define diff_get_color_opt(o, ix) \
-- 
2.10.0.rc2.23.gf336a1a.dirty

[PATCH] xdiff: remove unneeded declarations

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 xdiff/xemit.c | 9 -
 1 file changed, 9 deletions(-)

diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index 49aa16f..b52b4b9 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -22,15 +22,6 @@
 
 #include "xinclude.h"
 
-
-
-
-static long xdl_get_rec(xdfile_t *xdf, long ri, char const **rec);
-static int xdl_emit_record(xdfile_t *xdf, long ri, char const *pre, xdemitcb_t 
*ecb);
-
-
-
-
 static long xdl_get_rec(xdfile_t *xdf, long ri, char const **rec) {
 
*rec = xdf->recs[ri]->ptr;
-- 
2.10.0.rc2.22.g25cb54d.dirty

Re: [PATCH] sequencer: support folding in rfc2822 footer

[Junio C Hamano]

Jonathan Tan  writes:

> Sample-field: multiple-line field body
>  that causes a blank line below

I am not sure this is unconditionally good, or may cause problems to
those with workflows you did not consider when you wrote this patch.

Not being too lenient here historically has been a deliberate
decision to avoid misidentification of non "footers".  Does Git
itself produce some folded footer line?  If Git itself produced such
folded lines, I'd be a lot more receptive to this change, but I do
not think that is the case here.

A slightly related tangent.  An unconditionally good change you
could make is to allow folding of in-body headers.  I.e. you can
have e.g.

-- >8 --
Subject: [PATCH] sequencer: support in-body headers that are
 folded according to RFC2822 rules

The first paragraph after the above long title begins
here...

in the body of the msssage, and I _think_ we do not fold it properly
when applying such a patch.  We should, as that is something that
appears in format-patch output (i.e. something Git itself produces,
unlike the folded "footer").

Re: Fixup of a fixup not working right

[Junio C Hamano]

"Philip Oakley"  writes:

> As I understand this it's implied by design. The issue is that the
> rebase is looking for that named commit within its current rebase
> range, and can't find it, so ignores it.
>
> There is a separate issue that all the fixup! fixup! messages are
> essentially treated as being concatenations of the original fixup!, no
> matter how many time the fiup is present.

They can be handled separately, but they come from the same "design"
that could be improved.  When the "original" is not in the range to
be rebased for whatever reason (including the most likely one, i.e.
it has already graduated to become part of the public history), the
best thing the user could do at that point may be, as you suggested
to Robert in your message, to turn the "fixup! original" that did
not make in time before "original" hit the public record into a
standalone "fix original" follow-up change, and then to squash
subsequent "fixup! fixup! original" (and other "fixup! original",
too) into that commit.  And a good direction forward may be to see
if "rebase -i" can be taught to be more helpful for the user who
wants to do that.

Perhaps a change like this to "rebase -i":

 - The search for "original" when handling "pick fixup! original",
   when it does not find "original", could turn it into "reword
   fixup! original" without changing its position in the instruction
   sequence.

 - The search for "original" when handling "pick fixup! fixup!
   original", could be (probably unconditionally) changed to look
   for "fixup! original" to amend, instead of looking for "original"
   as the current code (this is your "separate issue").  The same
   "if the commit to be amended is not found, turn it into reword"
   rule from the above applies to this one, too.

may be an improvement?

Re: [PATCH 2/2] connect: know that zero-ID is not a ref

[Jeff King]

On Fri, Sep 02, 2016 at 07:03:30PM -0700, Shawn Pearce wrote:

> > Is it useful for upload-pack? If we have no refs, there's traditionally
> > been nothing to fetch. Perhaps that's something that could change,
> > though. For example, there could be a capability to allow fetching
> > arbitrary sha1s (we have allowTIPSH1InWant and allowReachableSHA1InWant,
> > which obviously both require some refs, but allowArbitrarySHA1 does not
> > seem outside the realm of possibility).
> 
> Its exactly these sort of extra capabilities. We run JGit in modes
> where "out of band" (e.g. URL or higher level protocol framing like an
> undocumented HTTP header) allows the fetch-pack client to say "do not
> send me advertisements, but I want to learn your capabilities". The
> fetch-pack client typically activates the allow-reachable-sha1-in-want
> feature and names specific SHA-1s it wants.

So it sounds like you _could_ enable this only in out-of-band mode,
without loss of functionality.

I don't particularly care either way (I do not run any JGit servers
myself), but if we do it in Git, I think that is the path we should go
for maximum compatibility (and then if we jump to protocol v2, we get to
shed all of the compatibility cruft).

> This allows the fetch-pack client to bypass a very large advertisement
> if it wants only a specific SHA-1 and doesn't care about the ref name
> its bound to, or reachable through.

Yep, definitely a useful thing.

> This is also perhaps a stepping stone towards "client speaks first".
> If we can later standardize an HTTP query parameter or extra HTTP
> header, the server may be able to avoid sending a lot of ref
> advertisements, but would still need to advertise capabilities.

Yes, but that is a big enough jump that we can design it to look like
whatever we want, not shoe-horning it into a pretend ref. Older clients
will be left behind either way, we will need a transition plan, etc.

-Peff

What's cooking in git.git (Sep 2016, #01; Fri, 2)

[Junio C Hamano]

Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.  The ones marked with '.' do not appear in any of
the integration branches, but I am still holding onto them.

Git 2.10 has been tagged.  In a few days, many topics that have been
cooking in 'next' would be merged to 'master'; as usual, I plan to
keep the tip of 'next' closed for a week or so and encourage people
to pay more attention to possible regressions than to show their
shiny new toys, and then open the new cycle by rebuilding 'next'
after that.

You can find the changes described here in the integration branches
of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--
[New Topics]

* jc/submodule-anchor-git-dir (2016-09-01) 1 commit
 - submodule: avoid auto-discovery in prepare_submodule_repo_env()

 Having a submodule whose ".git" repository is somehow corrupt
 caused a few commands that recurse into submodules loop forever.

 Will merge to 'next'.


* jc/forbid-symbolic-ref-d-HEAD (2016-09-02) 1 commit
 - symbolic-ref -d: do not allow removal of HEAD

 "git symbolic-ref -d HEAD" happily removes the symbolic ref, but
 the resulting repository becomes an invalid one.  Teach the command
 to forbid removal of HEAD.

 Will merge to 'next'.

--
[Stalled]

* jc/bundle (2016-03-03) 6 commits
 - index-pack: --clone-bundle option
 - Merge branch 'jc/index-pack' into jc/bundle
 - bundle v3: the beginning
 - bundle: keep a copy of bundle file name in the in-core bundle header
 - bundle: plug resource leak
 - bundle doc: 'verify' is not about verifying the bundle

 The beginning of "split bundle", which could be one of the
 ingredients to allow "git clone" traffic off of the core server
 network to CDN.

 While I think it would make it easier for people to experiment and
 build on if the topic is merged to 'next', I am at the same time a
 bit reluctant to merge an unproven new topic that introduces a new
 file format, which we may end up having to support til the end of
 time.  It is likely that to support a "prime clone from CDN", it
 would need a lot more than just "these are the heads and the pack
 data is over there", so this may not be sufficient.

 Will discard.


* jc/blame-reverse (2016-06-14) 2 commits
 - blame: dwim "blame --reverse OLD" as "blame --reverse OLD.."
 - blame: improve diagnosis for "--reverse NEW"

 It is a common mistake to say "git blame --reverse OLD path",
 expecting that the command line is dwimmed as if asking how lines
 in path in an old revision OLD have survived up to the current
 commit.

 Has been waiting for positive responses without seeing any.

 Will discard.


* jc/attr (2016-05-25) 18 commits
 - attr: support quoting pathname patterns in C style
 - attr: expose validity check for attribute names
 - attr: add counted string version of git_attr()
 - attr: add counted string version of git_check_attr()
 - attr: retire git_check_attrs() API
 - attr: convert git_check_attrs() callers to use the new API
 - attr: convert git_all_attrs() to use "struct git_attr_check"
 - attr: (re)introduce git_check_attr() and struct git_attr_check
 - attr: rename function and struct related to checking attributes
 - attr.c: plug small leak in parse_attr_line()
 - attr.c: tighten constness around "git_attr" structure
 - attr.c: simplify macroexpand_one()
 - attr.c: mark where #if DEBUG ends more clearly
 - attr.c: complete a sentence in a comment
 - attr.c: explain the lack of attr-name syntax check in parse_attr()
 - attr.c: update a stale comment on "struct match_attr"
 - attr.c: use strchrnul() to scan for one line
 - commit.c: use strchrnul() to scan for one line
 (this branch is used by jc/attr-more, sb/pathspec-label and 
sb/submodule-default-paths.)

 The attributes API has been updated so that it can later be
 optimized using the knowledge of which attributes are queried.

 I wanted to polish this topic further to make the attribute
 subsystem thread-ready, but because other topics depend on this
 topic and they do not (yet) need it to be thread-ready.

 As the authors of topics that depend on this seem not in a hurry,
 let's discard this and dependent topics and restart them some other
 day.

 Will discard.


* jc/attr-more (2016-06-09) 8 commits
 - attr.c: outline the future plans by heavily commenting
 - attr.c: always pass check[] to collect_some_attrs()
 - attr.c: introduce empty_attr_check_elems()
 - attr.c: correct ugly hack for git_all_attrs()
 - attr.c: rename a local variable check
 - fixup! d5ad6c13
 - attr.c: pass struct git_attr_check down the callchain
 - attr.c: add push_stack() helper
 (this branch uses jc/attr; is tangled with sb/pathspec-label and 
sb/submodule-default-paths.)

 The beginning of long and tortuous journey to clean-up attribute
 subsystem implementation.

 Needs 

A note from the maintainer

[Junio C Hamano]

Welcome to the Git development community.

This message is written by the maintainer and talks about how Git
project is managed, and how you can work with it.

* Mailing list and the community

The development is primarily done on the Git mailing list. Help
requests, feature proposals, bug reports and patches should be sent to
the list address .  You don't have to be
subscribed to send messages.  The convention on the list is to keep
everybody involved on Cc:, so it is unnecessary to say "Please Cc: me,
I am not subscribed".

Before sending patches, please read Documentation/SubmittingPatches
and Documentation/CodingGuidelines to familiarize yourself with the
project convention.

If you sent a patch and you did not hear any response from anybody for
several days, it could be that your patch was totally uninteresting,
but it also is possible that it was simply lost in the noise.  Please
do not hesitate to send a reminder message in such a case.  Messages
getting lost in the noise may be a sign that those who can evaluate
your patch don't have enough mental/time bandwidth to process them
right at the moment, and it often helps to wait until the list traffic
becomes calmer before sending such a reminder.

The list archive is available at a few public sites:

http://public-inbox.org/git/
http://marc.info/?l=git
http://www.spinics.net/lists/git/

For those who prefer to read it over NNTP:

nntp://news.gmane.org/gmane.comp.version-control.git
nntp://news.public-inbox.org/inbox.comp.version-control.git

are available.

When you point at a message in a mailing list archive, using its
message ID is often the most robust (if not very friendly) way to do
so, like this:


http://public-inbox.org/git/pine.lnx.4.58.0504150753440.7...@ppc970.osdl.org

Often these web interfaces accept the message ID with enclosing <>
stripped (like the above example to point at one of the most important
message in the Git list).

Some members of the development community can sometimes be found on
the #git and #git-devel IRC channels on Freenode.  Their logs are
available at:

http://colabti.org/irclogger/irclogger_log/git
http://colabti.org/irclogger/irclogger_log/git-devel

There is a volunteer-run newsletter to serve our community ("Git Rev
News" http://git.github.io/rev_news/rev_news.html).

Git is a member project of software freedom conservancy, a non-profit
organization (https://sfconservancy.org/).  To reach a committee of
liaisons to the conservancy, contact them at .


* Reporting bugs

When you think git does not behave as you expect, please do not stop
your bug report with just "git does not work".  "I used git in this
way, but it did not work" is not much better, neither is "I used git
in this way, and X happend, which is broken".  It often is that git is
correct to cause X happen in such a case, and it is your expectation
that is broken. People would not know what other result Y you expected
to see instead of X, if you left it unsaid.

Please remember to always state

 - what you wanted to achieve;

 - what you did (the version of git and the command sequence to reproduce
   the behavior);

 - what you saw happen (X above);

 - what you expected to see (Y above); and

 - how the last two are different.

See http://www.chiark.greenend.org.uk/~sgtatham/bugs.html for further
hints.

If you think you found a security-sensitive issue and want to disclose
it to us without announcing it to wider public, please contact us at
our security mailing list .  This is
a closed list that is limited to people who need to know early about
vulnerabilities, including:

  - people triaging and fixing reported vulnerabilities
  - people operating major git hosting sites with many users
  - people packaging and distributing git to large numbers of people

where these issues are discussed without risk of the information
leaking out before we're ready to make public announcements.


* Repositories and documentation.

My public git.git repositories are at:

  git://git.kernel.org/pub/scm/git/git.git/
  https://kernel.googlesource.com/pub/scm/git/git
  git://repo.or.cz/alt-git.git/
  https://github.com/git/git/
  git://git.sourceforge.jp/gitroot/git-core/git.git/
  git://git-core.git.sourceforge.net/gitroot/git-core/git-core/

A few web interfaces are found at:

  http://git.kernel.org/cgit/git/git.git
  https://kernel.googlesource.com/pub/scm/git/git
  http://repo.or.cz/w/alt-git.git

Preformatted documentation from the tip of the "master" branch can be
found in:

  git://git.kernel.org/pub/scm/git/git-{htmldocs,manpages}.git/
  git://repo.or.cz/git-{htmldocs,manpages}.git/
  https://github.com/gitster/git-{htmldocs,manpages}.git/

The manual pages formatted in HTML for the tip of 'master' can be
viewed online at:

  https://git.github.io/htmldocs/git.html


* How various branches are used.

There are four 

[ANNOUNCE] Git v2.10.0

[Junio C Hamano]

The latest feature release Git v2.10.0 is now available at the
usual places.  It is comprised of 639 non-merge commits since
v2.9.0, contributed by 76 people, 22 of which are new faces.

The tarballs are found at:

https://www.kernel.org/pub/software/scm/git/

The following public repositories all have a copy of the 'v2.10.0'
tag and the 'master' branch that the tag points at:

  url = https://kernel.googlesource.com/pub/scm/git/git
  url = git://repo.or.cz/alt-git.git
  url = git://git.sourceforge.jp/gitroot/git-core/git.git
  url = git://git-core.git.sourceforge.net/gitroot/git-core/git-core
  url = https://github.com/gitster/git

New contributors whose contributions weren't in v2.9.0 are as follows.
Welcome to the Git development community!

  Alexander Hirsch, Andreas Brauchli, Andrew Oakley, Antoine Queru,
  Ben Wijen, Christopher Layne, Dave Nicolson, David Glasser, Ed
  Maste, Heiko Becker, Ingo Brückl, Jonathan Tan, Jordan DE GEA,
  Josef Kufner, Keith McGuigan, Kevin Willford, LE Manh Cuong,
  Michael Stahl, Parker Moore, Peter Colberg, Tom Russello,
  and William Duclot.

Returning contributors who helped this release are as follows.
Thanks for your continued support.

  Alex Henrie, Alfred Perlstein, Armin Kunaschik, brian m. carlson,
  Changwoo Ryu, Charles Bailey, Chris Packham, Christian Couder,
  David A. Greene, David Aguilar, David Kastrup, David Turner,
  Edward Thomson, Elia Pinto, Eric Sunshine, Eric Wong, Heiko
  Voigt, Jacob Keller, Jean-Noel Avila, Jeff King, Jiang Xin,
  Joey Hess, Johannes Schindelin, Johannes Sixt, John Keeping,
  Jonathan Nieder, Josh Triplett, Junio C Hamano, Lars Schneider,
  Lars Vogel, Linus Torvalds, Lukas Fleischer, Matthieu Moy,
  Mehul Jain, Michael Haggerty, Michael J Gruber, Mike Hommey,
  Nguyễn Thái Ngọc Duy, Nicolas Pitre, Orgad Shaneh, Patrick
  Steinhardt, Peter Krefting, Pranit Bauva, Ramsay Jones, René
  Scharfe, Ronald Wampler, Stefan Beller, SZEDER Gábor, Thomas
  Braun, Thomas Gummerer, Torsten Bögershausen, Trần Ngọc
  Quân, Vasco Almeida, and Ville Skyttä.



Git 2.10 Release Notes
==

Backward compatibility notes


Updates since v2.9
--

UI, Workflows & Features

 * "git pull --rebase --verify-signature" learned to warn the user
   that "--verify-signature" is a no-op when rebasing.

 * An upstream project can make a recommendation to shallowly clone
   some submodules in the .gitmodules file it ships.

 * "git worktree add" learned that '-' can be used as a short-hand for
   "@{-1}", the previous branch.

 * Update the funcname definition to support css files.

 * The completion script (in contrib/) learned to complete "git
   status" options.

 * Messages that are generated by auto gc during "git push" on the
   receiving end are now passed back to the sending end in such a way
   that they are shown with "remote: " prefix to avoid confusing the
   users.

 * "git add -i/-p" learned to honor diff.compactionHeuristic
   experimental knob, so that the user can work on the same hunk split
   as "git diff" output.

 * "upload-pack" allows a custom "git pack-objects" replacement when
   responding to "fetch/clone" via the uploadpack.packObjectsHook.
   (merge b738396 jk/upload-pack-hook later to maint).

 * Teach format-patch and mailsplit (hence "am") how a line that
   happens to begin with "From " in the e-mail message is quoted with
   ">", so that these lines can be restored to their original shape.
   (merge d9925d1 ew/mboxrd-format-am later to maint).

 * "git repack" learned the "--keep-unreachable" option, which sends
   loose unreachable objects to a pack instead of leaving them loose.
   This helps heuristics based on the number of loose objects
   (e.g. "gc --auto").
   (merge e26a8c4 jk/repack-keep-unreachable later to maint).

 * "log --graph --format=" learned that "%>|(N)" specifies the width
   relative to the terminal's left edge, not relative to the area to
   draw text that is to the right of the ancestry-graph section.  It
   also now accepts negative N that means the column limit is relative
   to the right border.

 * A careless invocation of "git send-email directory/" after editing
   0001-change.patch with an editor often ends up sending both
   0001-change.patch and its backup file, 0001-change.patch~, causing
   embarrassment and a minor confusion.  Detect such an input and
   offer to skip the backup files when sending the patches out.
   (merge 531220b jc/send-email-skip-backup later to maint).

 * "git submodule update" that drives many "git clone" could
   eventually hit flaky servers/network conditions on one of the
   submodules; the command learned to retry the attempt.

 * The output coloring scheme learned two new attributes, italic and
   strike, in addition to existing bold, reverse, etc.

 * "git log" learns log.showSignature configuration variable, and a
   command line 

Re: [PATCH 2/2] connect: know that zero-ID is not a ref

[Shawn Pearce]

On Fri, Sep 2, 2016 at 1:13 PM, Jeff King  wrote:
>
> Hmm. So since this is backwards-compatible, I'm not overly concerned
> with changing the client. But I wonder if you considered that the
> documentation is wrong, and that JGit should stop sending the extra
> capabilities line?

No, JGit needs to keep sending the capabilities^{} line in upload-pack
if there were no refs advertised to the client. (If it advertises at
least one ref it does not send this capabilities^{} line.)

> In either case, there will still be breakage until _somebody_ upgrades
> (with your patch, until clients upgrade; with a JGit patch, until the
> server upgrades). So perhaps an interesting question would be: if we
> were writing this now, what is the correct behavior?
>
> For pushing, it is obviously useful to send capabilities even though
> there are no refs (because the client is going to send _you_ something).
> And that is why "capabilities^{}" exists; it is a receive-pack feature
> (that is actually implemented!), and the documentation (which came after
> the implementation) blindly mentioned it for upload-pack, as well.
>
> Is it useful for upload-pack? If we have no refs, there's traditionally
> been nothing to fetch. Perhaps that's something that could change,
> though. For example, there could be a capability to allow fetching
> arbitrary sha1s (we have allowTIPSH1InWant and allowReachableSHA1InWant,
> which obviously both require some refs, but allowArbitrarySHA1 does not
> seem outside the realm of possibility).

Its exactly these sort of extra capabilities. We run JGit in modes
where "out of band" (e.g. URL or higher level protocol framing like an
undocumented HTTP header) allows the fetch-pack client to say "do not
send me advertisements, but I want to learn your capabilities". The
fetch-pack client typically activates the allow-reachable-sha1-in-want
feature and names specific SHA-1s it wants.

This allows the fetch-pack client to bypass a very large advertisement
if it wants only a specific SHA-1 and doesn't care about the ref name
its bound to, or reachable through.


This is also perhaps a stepping stone towards "client speaks first".
If we can later standardize an HTTP query parameter or extra HTTP
header, the server may be able to avoid sending a lot of ref
advertisements, but would still need to advertise capabilities.

Re: [PATCH v2 2/2] connect: advertized capability is not a ref

[Jeff King]

On Fri, Sep 02, 2016 at 04:51:45PM -0700, Jonathan Nieder wrote:

> > I'd be more interested in the pain of this transition if there was a
> > concrete use case for "hide literally all refs, but still allow
> > fetches". Is that a thing that people do?
> 
> Sure, it is a thing that people do.  For example I've seen replication
> systems that learn what SHA-1s to fetch out-of-band and then use this
> approach to avoid the overhead of a long ref advertisement.

I know that's how those features work. I was more wondering if it ever
comes up that somebody actually has hidden refs, but _no_ non-hidden
ones. Do the systems you've seen hide all the refs?

> However, that is not my motivation.  My motivation is being able to
> extend the protocol in the future.  The capabilities line has been
> important for that historically.

Sure, I agree it's a nice move forward for compatibility. But that
argues for teaching the clients to handle it (for the future), and then
turning it on in the server only when it becomes useful (i.e., the "in a
year or so" can become "when we find a use for it").

In a similar vein, I'd think that a config to enable this in upload-pack
today could have an "auto" mode, which enables it _only_ when you know
something productive might come of it (namely that you have hidden refs,
one of the uploadpack.allow* features is enabled, and the ref
advertisement is empty). Then requests which could not benefit from it
at all do not have to pay the potential compatibility cost.

> Do you have any objection to the server gaining support for this
> guarded by a configuration option?  Then when the time comes to
> consider flipping the default we can use real-world statistics about
> what git client versions people use to make an informed decision.

Guarded by config, no. It's the flipping of the default I care more
about. The config is not necessary in the meantime for getting
real-world statistics; you can add the config and flip the default as
one unit at any time (the thing that is time-critical is teaching the
client to handle _both_ cases gracefully).

The config is useful in the meantime if there are people who could
benefit from it immediately, and don't mind paying the compatibility
cost. With an "auto" as I described above, using that as the default
seems like a decent interim behavior (i.e., why would you set up such a
repository if you didn't expect most clients to use the allowTipSHA1
feature?). I'd still probably give some lag between shipping the client,
and flipping the server default to "auto".

I hoped to share some numbers on what versions people are currently
using against GitHub, to get a sense of how far back most people are.
But I haven't been actively involved in keeping those numbers for a
while, and I'm not sure what we have readily stored. I did show some
numbers a few years ago[1], and it looks like about 2/3 of people were
within 6-12 months of the latest version, but the rest was a long tail.

I don't know if that will have changed with the advent of more client
versions (e.g., lots more people are using libgit2 now via GUI clients,
Visual Studio, etc; how does it fare with the proposed change?).

-Peff

[1] http://public-inbox.org/git/20120531114801.ga21...@sigill.intra.peff.net/

Re: Bug Report: Too many untracked files (gitignore)

[Brian Levinstein]

The patterns in question do contain a slash, although they don't start
with a slash.

I tried changing it to "!/.vim/colors/*" as you recommended, with no
change in behavior. I even tried adding a leading slash to every
pattern in gitignore, with no effect.

Removing the line with "!/.vim/colors/*" still fixes the problem.

Brian Levinstein
blevinst...@gmail.com | bpl...@virginia.edu
(703) 673-8711
Google | Software Engineer
University of Virginia | MS Commerce 2014
University of Virginia | BS Computer Science 2013
Alpha Tau Omega | Delta Chapter
http://www.linkedin.com/pub/brian-levinstein/14/620/6ba
https://github.com/blevinstein


On Fri, Sep 2, 2016 at 4:58 PM, Stefan Beller  wrote:
> On Fri, Sep 2, 2016 at 4:06 PM, Brian Levinstein  
> wrote:
>> The relevant repo is here:
>> https://github.com/blevinstein/dotfiles
>>
>> My gitignore file looks like this:
>> https://github.com/blevinstein/dotfiles/blob/2400ca8642a7b454a2bfc54e8402343d008836aa/.gitignore
>> It basically ignores all files, except for specifically whitelisted
>> files. However, when I run "git status" (git version
>> 2.8.0.rc3.226.g39d4020), I see the following untracked files:
>>
>> #   .bash_history
>> #   .bash_logout
>> #   .cache/
>> #   [private]
>> #   [private]
>> #   .profile
>> #   .viminfo
>> #   dev/
>
> For the specific files to be exclued, I'd recommend starting with a slash, 
> e.g.
>
> !/.bashrc
> !/.vim/colors/*
>
> If the pattern does not contain a slash /, Git treats it as a shell
> glob pattern and checks
> for a match against the pathname relative to the location of the
> .gitignore file (relative
> to the toplevel of the work tree if not from a .gitignore file).
>
> See the notes section of https://git-scm.com/docs/gitignore
>
> So I do not quite see the bug?
>
> Stefan

Re: [PATCH v2 2/2] connect: advertized capability is not a ref

[Jonathan Nieder]

Stefan Beller wrote:
> On Fri, Sep 2, 2016 at 4:35 PM, Jeff King  wrote:

>> I'd be more interested in the pain of this transition if there was a
>> concrete use case for "hide literally all refs, but still allow
>> fetches". Is that a thing that people do?
[...]
> Not to derail the discussion to much, but let's talk about protocol v2
> for a second:

Uh oh. ;-)

> One of the ideas we floated around for protocol v2 would be exactly
> that: the server advertises only a small portion (could be just master
> or no branch eventually) with a capability "v2" and then the client
> selects that capability and after that there comes protocol 2.

Sounds scary to me.  What would happen when I try to clone a
repository with a v1 client?  I'd see nothing.  I'd want at least a
"master" branch with a README file (or an ERR packet?) saying "please
update your client".

> The advantage of this approach would be have a functional
> v1 server still running, but the meat is found only in v2: e.g. via
> v2 you can obtain all pull requests/changes or even wiki/meta
> information stuff that would be too large to advertise in v1.

This sounds less scary, but it doesn't answer the question Peff
raised.  Wouldn't it still be typical to advertise at least one ref,
which can contain a capabilities line?

However, another idea I think you've mentioned before on-list about
changing the ref advertisement could answer it.  Suppose that I always
include the ref advertisement in my first reply, but I provide a
capability saying that further requests to this server can use a
different mechanism that skips the long advertisement.

Normally that would work great --- I only pay the cost of the large
advertisement once, and from then on I can cache what the server told
me about how it prefers to be contacted.  Except what happens if this
was a new repository and my first contact with the server was to clone
that empty repository?

In that case, getting capabilities with the ref advertisement would
benefit me.

Likewise for other capabilities that may come with such an empty
fetch: for example the server could tell which unborn branch the HEAD
symref points to.

Thanks,
Jonathan

Re: Bug Report: Too many untracked files (gitignore)

[Stefan Beller]

On Fri, Sep 2, 2016 at 4:06 PM, Brian Levinstein  wrote:
> The relevant repo is here:
> https://github.com/blevinstein/dotfiles
>
> My gitignore file looks like this:
> https://github.com/blevinstein/dotfiles/blob/2400ca8642a7b454a2bfc54e8402343d008836aa/.gitignore
> It basically ignores all files, except for specifically whitelisted
> files. However, when I run "git status" (git version
> 2.8.0.rc3.226.g39d4020), I see the following untracked files:
>
> #   .bash_history
> #   .bash_logout
> #   .cache/
> #   [private]
> #   [private]
> #   .profile
> #   .viminfo
> #   dev/

For the specific files to be exclued, I'd recommend starting with a slash, e.g.

!/.bashrc
!/.vim/colors/*

If the pattern does not contain a slash /, Git treats it as a shell
glob pattern and checks
for a match against the pathname relative to the location of the
.gitignore file (relative
to the toplevel of the work tree if not from a .gitignore file).

See the notes section of https://git-scm.com/docs/gitignore

So I do not quite see the bug?

Stefan

Re: [PATCH v2 2/2] connect: advertized capability is not a ref

[Jonathan Nieder]

Jeff King wrote:
> On Fri, Sep 02, 2016 at 03:06:12PM -0700, Jonathan Tan wrote:

>  But combining hideRefs with
> allowTipSHA1InWant could trigger this case.

Yes.

[...]
> I'd be more interested in the pain of this transition if there was a
> concrete use case for "hide literally all refs, but still allow
> fetches". Is that a thing that people do?

Sure, it is a thing that people do.  For example I've seen replication
systems that learn what SHA-1s to fetch out-of-band and then use this
approach to avoid the overhead of a long ref advertisement.

However, that is not my motivation.  My motivation is being able to
extend the protocol in the future.  The capabilities line has been
important for that historically.

Do you have any objection to the server gaining support for this
guarded by a configuration option?  Then when the time comes to
consider flipping the default we can use real-world statistics about
what git client versions people use to make an informed decision.

Thanks,
Jonathan

Re: [PATCH v2 2/2] connect: advertized capability is not a ref

[Stefan Beller]

On Fri, Sep 2, 2016 at 4:35 PM, Jeff King  wrote:
> I'd be more interested in the pain of this transition if there was a
> concrete use case for "hide literally all refs, but still allow
> fetches". Is that a thing that people do?
>

Not yet, I would think.

Not to derail the discussion to much, but let's talk about protocol v2
for a second:

One of the ideas we floated around for protocol v2 would be exactly
that: the server advertises only a small portion (could be just master
or no branch eventually) with a capability "v2" and then the client
selects that capability and after that there comes protocol 2.

The advantage of this approach would be have a functional
v1 server still running, but the meat is found only in v2: e.g. via
v2 you can obtain all pull requests/changes or even wiki/meta
information stuff that would be too large to advertise in v1.

Re: [PATCH v2 2/2] connect: advertized capability is not a ref

[Jeff King]

On Fri, Sep 02, 2016 at 03:06:12PM -0700, Jonathan Tan wrote:

> The cause is that, since v3.1.0.201309270735-rc1~22 (Advertise capabilities
> with no refs in upload service., 2013-08-08), JGit's ref advertisement 
> includes
> a ref named capabilities^{} to advertise its capabilities on, while git's ref
> advertisement is empty in this case. This allows the client to learn about the
> server's capabilities and is needed, for example, for fetch-by-sha1 to work
> when no refs are advertised.

So does JGit actually have a fetch-by-sha1 that works in such a case
(obviously the repository has to have _some_ objects, so is this a
feature where the server hides some of the refs?).

I was thinking we did not have one in git (i.e., we have nothing to
allow fetching arbitrary sha1s). But combining hideRefs with
allowTipSHA1InWant could trigger this case.

> This also affects "ls-remote". For example, against an empty repository served
> by JGit:
> 
>   $ git ls-remote git://localhost/tmp/empty
>   capabilities^{}
> 
> Git advertises the same capabilities^{} ref in its ref advertisement for push
> but since it never remembered to do so for fetch, the client forgot to handle
> this case. Handle it.

As you can probably guess from my previous emails in this thread, I
don't think it is "never remembered to do so". It is more like "never
intended to do so and was documented incorrectly".

That doesn't make things clear cut, of course. But I think the real
story is more like (I dug a little in the history, as you'll see, but
didn't look for conversations in the list archive, so take this with the
appropriate grain of salt):

  0. Upload-pack existed without this capabilities^{} trick for some time.

  1. Receive-pack learned the capabilities^{} trick, and send-pack on
 the client side learned to accept it (this looks like it came along
 with the first capability in cfee10a (send-pack/receive-pack: allow
 errors to be reported back to pusher., 2005-12-25).

  2. Later, b31222c (Update packfile transfer protocol documentation,
 2009-11-03) tried to document the upload-pack and receive-pack
 protocols, but mistakenly documented both as having
 capabilities^{}.

  3. In ae1f469 (Advertise capabilities with no refs in upload service.,
 2013-08-08), JGit started sending these in its upload-pack
 equivalent, according to the documentation from (3).

So now we are in a state where JGit behavior does not match Git
behavior. Since JGit versions have existed in the wild for a few years,
it's a good idea for all clients to be liberal and accept both cases.

> In this aspect, JGit is compliant with the specification in pack-protocol.txt.
> Because git client versions without this fix are expected to exist in the wild
> for a while, we should not change the server to always send the 
> capabilities^{}
> line when there are no refs to advertise yet.  A transition will take multiple
> steps:
> 
>  1. This patch, which updates the client
> 
>  2. Update pack-protocol to clarify that both server behaviors must be
> tolerated.

These two seem like obvious improvements.

>  3. Add a configuration variable to allow git upload-pack to advertise
> capabilities when there are no refs to advertise.  Leave it disabled
> by default since git clients can't be counted on to have this patch (1)
> yet.
> 
>  4. After a year or so, flip the default for that server configuration
> variable to true.

I think "a year or so" is not nearly long enough, as this is not a
backwards-compatible change. The only thing that mitigates it is that an
older client doesn't barf totally, but just generates funny output.

I'd be more interested in the pain of this transition if there was a
concrete use case for "hide literally all refs, but still allow
fetches". Is that a thing that people do?

-Peff

Bug Report: Too many untracked files (gitignore)

[Brian Levinstein]

The relevant repo is here:
https://github.com/blevinstein/dotfiles

My gitignore file looks like this:
https://github.com/blevinstein/dotfiles/blob/2400ca8642a7b454a2bfc54e8402343d008836aa/.gitignore
It basically ignores all files, except for specifically whitelisted
files. However, when I run "git status" (git version
2.8.0.rc3.226.g39d4020), I see the following untracked files:

#   .bash_history
#   .bash_logout
#   .cache/
#   [private]
#   [private]
#   .profile
#   .viminfo
#   dev/

I can fix this by removing the following line from my gitignore:

!.vim/colors/*
after which all the untracked files disappear. I also tried changing
that line to:

!.vim/colors/twilight256.vim

but it had no effect.

The same effect can be achieved with any directory name starting with a period:

!.tmux/asdf

!.vim/asdf

where .tmux and .vim are real directories. It does not seem to matter
whether the "asdf" subdirectory exists at all.

Brian Levinstein

blevinst...@gmail.com | bpl...@virginia.edu
(703) 673-8711
Google | Software Engineer
University of Virginia | MS Commerce 2014
University of Virginia | BS Computer Science 2013
Alpha Tau Omega | Delta Chapter
http://www.linkedin.com/pub/brian-levinstein/14/620/6ba
https://github.com/blevinstein

Re: [PATCH v2 2/2] connect: advertized capability is not a ref

[Jonathan Nieder]

Jonathan Tan wrote:

> Signed-off-by: Jonathan Tan 
> ---
>  connect.c|  3 +++
>  t/t5512-ls-remote.sh | 39 +++
>  2 files changed, 42 insertions(+)

Reviewed-by: Jonathan Nieder 

Thanks.

Re: Fixup of a fixup not working right

[Philip Oakley]

From: "Robert Dailey" 

Suppose I have a branch with 4 commits, in the following order (as you
might see during interactive rebase):

pick 123 Original Change
pick 789 fixup! Original Change
pick 456 Some Other Thing
pick abc fixup! fixup! Original Change

However, let's say the first commit is already pushed upstream on a
topic branch. Since there are multiple developers on this topic
branch, I do not want to rebase right now. Instead, I want to document
future fixups via fixup commits and then when we're ready to merge, do
a final rebase prior to the merge to master to clean things up after
we're all done collaborating.

For this specific situation, since the first commit is already pushed,
I want to perform a fixup on the 1st fixup commit. When I perform an
interactive rebase against upstream topic, I get the following:

pick 789 fixup! Original Change
pick 456 Some Other Thing
pick abc fixup! fixup! Original Change

The tip commit (abc in this case) is not marked as a fixup. What I
expect to see is:

pick 789 fixup! Original Change
fixup abc fixup! fixup! Original Change
pick 456 Some Other Thing

Is this by design, or a defect? I assumed that Git would only look at
the first occurrence of "fixup!" and treat everything else after as
the commit description to match. But it seems in this case that it
stops at the last occurrence of "fixup!", which would explain why it
isn't matching in the interactive rebase. I haven't looked at the
code, though.


As I understand this it's implied by design. The issue is that the rebase is 
looking for that named commit within its current rebase range, and can't 
find it, so ignores it.


There is a separate issue that all the fixup! fixup! messages are 
essentially treated as being concatenations of the original fixup!, no 
matter how many time the fiup is present.


In the mean time you should reword those commit messages as being 
'bug-fixes' as they are (you say) already upstream and hence published. You 
can make the first as a bug-fix and the following ones a fixup!s.




Thoughts? Also I'm perfectly willing to accept feedback involving me
just using the feature wrong or as not intended. Thanks in advance.

Re: [PATCH 2/2] connect: know that zero-ID is not a ref

[Jonathan Tan]

On 09/02/2016 01:13 PM, Jeff King wrote:

On Fri, Sep 02, 2016 at 10:15:39AM -0700, Jonathan Tan wrote:

(git-daemon should probably also be changed to serve zero IDs, but such
a change can be considered independently from this change; even if both
the client and server changes were made in one commit, it is nearly
impossible that all Git installations are updated at the same time - an
updated client would still need to deal with unupdated servers and vice
versa.)


I'm really not sure what you mean here. How does git-daemon enter into
this at all?


I was comparing the behavior of git daemon and jgit daemon - when 
serving the same repository, the former does not send the zero ID and 
capabilities^{} line, whereas the latter does; and I was stating that 
git daemon's behavior should be changed to JGit's behavior, but not 
necessarily immediately.


(In one of the replies to that email, Jonathan Nieder has suggested a 
more detailed transition plan.)



diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 819b9dd..c6f8b6f 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -207,5 +207,27 @@ test_expect_success 'ls-remote --symref omits filtered-out 
matches' '
test_cmp expect actual
 '

+test_lazy_prereq GIT_DAEMON '
+   test_have_prereq JGIT &&
+   test_tristate GIT_TEST_GIT_DAEMON &&
+   test "$GIT_TEST_GIT_DAEMON" != false
+'


GIT_DAEMON depends on JGIT? Should this really be the JGIT_DAEMON
prerequisite?


The JGIT line shouldn't be there - thanks for catching this.

[PATCH v2 2/2] connect: advertized capability is not a ref

[Jonathan Tan]

When cloning an empty repository served by standard git, "git clone" produces
the following reassuring message:

$ git clone git://localhost/tmp/empty
Cloning into 'empty'...
warning: You appear to have cloned an empty repository.
Checking connectivity... done.

Meanwhile when cloning an empty repository served by JGit, the output is more
haphazard:

$ git clone git://localhost/tmp/empty
Cloning into 'empty'...
Checking connectivity... done.
warning: remote HEAD refers to nonexistent ref, unable to checkout.

This is a common command to run immediately after creating a remote repository
as preparation for adding content to populate it and pushing. The warning is
confusing and needlessly worrying.

The cause is that, since v3.1.0.201309270735-rc1~22 (Advertise capabilities
with no refs in upload service., 2013-08-08), JGit's ref advertisement includes
a ref named capabilities^{} to advertise its capabilities on, while git's ref
advertisement is empty in this case. This allows the client to learn about the
server's capabilities and is needed, for example, for fetch-by-sha1 to work
when no refs are advertised.

This also affects "ls-remote". For example, against an empty repository served
by JGit:

$ git ls-remote git://localhost/tmp/empty
capabilities^{}

Git advertises the same capabilities^{} ref in its ref advertisement for push
but since it never remembered to do so for fetch, the client forgot to handle
this case. Handle it.

In this aspect, JGit is compliant with the specification in pack-protocol.txt.
Because git client versions without this fix are expected to exist in the wild
for a while, we should not change the server to always send the capabilities^{}
line when there are no refs to advertise yet.  A transition will take multiple
steps:

 1. This patch, which updates the client

 2. Update pack-protocol to clarify that both server behaviors must be
tolerated.

 3. Add a configuration variable to allow git upload-pack to advertise
capabilities when there are no refs to advertise.  Leave it disabled
by default since git clients can't be counted on to have this patch (1)
yet.

 4. After a year or so, flip the default for that server configuration
variable to true.

Signed-off-by: Jonathan Tan 
---
 connect.c|  3 +++
 t/t5512-ls-remote.sh | 39 +++
 2 files changed, 42 insertions(+)

diff --git a/connect.c b/connect.c
index 722dc3f..0c2221e 100644
--- a/connect.c
+++ b/connect.c
@@ -165,6 +165,9 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t 
src_len,
continue;
}
 
+   if (!strcmp(name, "capabilities^{}"))
+   continue;
+
if (!check_ref(name, flags))
continue;
ref = alloc_ref(buffer + GIT_SHA1_HEXSZ + 1);
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 819b9dd..2de52f5 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -207,5 +207,44 @@ test_expect_success 'ls-remote --symref omits filtered-out 
matches' '
test_cmp expect actual
 '
 
+test_lazy_prereq GIT_DAEMON '
+   test_tristate GIT_TEST_GIT_DAEMON &&
+   test "$GIT_TEST_GIT_DAEMON" != false
+'
+
+JGIT_DAEMON_PORT=${JGIT_DAEMON_PORT-${this_test#t}}
+
+# This test spawns a daemon, so run it only if the user would be OK with
+# testing with git-daemon.
+test_expect_success PIPE,JGIT,GIT_DAEMON 'indicate no refs in 
standards-compliant empty remote' '
+   JGIT_DAEMON_PID= &&
+   git init --bare empty.git &&
+   touch empty.git/git-daemon-export-ok &&
+   mkfifo jgit_daemon_output &&
+   {
+   jgit daemon --port="$JGIT_DAEMON_PORT" . >jgit_daemon_output &
+   JGIT_DAEMON_PID=$!
+   } &&
+   test_when_finished kill "$JGIT_DAEMON_PID" &&
+   {
+   read line &&
+   case $line in
+   Exporting*)
+   ;;
+   *)
+   echo "Expected: Exporting" &&
+   false;;
+   esac &&
+   read line &&
+   case $line in
+   "Listening on"*)
+   ;;
+   *)
+   echo "Expected: Listening on" &&
+   false;;
+   esac
+   } 

[PATCH v2 1/2] tests: move test_lazy_prereq JGIT to test-lib.sh

[Jonathan Tan]

This enables JGIT to be used as a prereq in invocations of
test_expect_success (and other functions) in other test scripts.

Signed-off-by: Jonathan Tan 
---
 t/t5310-pack-bitmaps.sh | 4 
 t/test-lib.sh   | 4 
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 3893afd..1e376ea 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -158,10 +158,6 @@ test_expect_success 'pack with missing parent' '
git pack-objects --stdout --revs /dev/null
 '
 
-test_lazy_prereq JGIT '
-   type jgit
-'
-
 test_expect_success JGIT 'we can read jgit bitmaps' '
git clone . compat-jgit &&
(
diff --git a/t/test-lib.sh b/t/test-lib.sh
index d731d66..c9c1037 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1072,6 +1072,10 @@ test_lazy_prereq NOT_ROOT '
test "$uid" != 0
 '
 
+test_lazy_prereq JGIT '
+   type jgit
+'
+
 # SANITY is about "can you correctly predict what the filesystem would
 # do by only looking at the permission bits of the files and
 # directories?"  A typical example of !SANITY is running the test
-- 
2.8.0.rc3.226.g39d4020

[PATCH v2 0/2] handle empty spec-compliant remote repos correctly

[Jonathan Tan]

Thanks - I've updated the following:
o better patch description as suggested by Jonathan Nieder
o checking for capabilities^{} - all such lines are ignored without any further
  checks, similar to the behavior for .have 
o minor fix to test_lazy_prereq GIT_DAEMON
o test waits for output of `jgit daemon` (instead of sleeping)

Jonathan Tan (2):
  tests: move test_lazy_prereq JGIT to test-lib.sh
  connect: advertized capability is not a ref

 connect.c   |  3 +++
 t/t5310-pack-bitmaps.sh |  4 
 t/t5512-ls-remote.sh| 39 +++
 t/test-lib.sh   |  4 
 4 files changed, 46 insertions(+), 4 deletions(-)

-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 01/34] sequencer: support a new action: 'interactive rebase'

[Kevin Daudt]

On Wed, Aug 31, 2016 at 10:54:02AM +0200, Johannes Schindelin wrote:
> @@ -43,16 +51,20 @@ 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();
>  }

This patch fails to apply for me because function is_rebase_i has never
been introduced before (no record of it anywhere). Currently, only
IS_REBASE_I macro is present.

Re: [PATCH 6/9] rebase -i: check for missing commits in the rebase--helper

[Dennis Kaarsemaker]

On vr, 2016-09-02 at 18:23 +0200, Johannes Schindelin wrote:
> In particular on Windows, where shell scripts are even more expensive
> than on MacOSX or Linux, it makes sense to move a loop that forks
> Git at least once for every line in the todo list into a builtin.

Heh, this was the one thing that made me hesitate sending the
suggestion about rebase-helper --edit-todo, but with this bit already
moved, I think rebase-helper --edit-todo makes even more sense to do.

D.

Re: [PATCH 4/9] rebase -i: also expand/collapse the SHA-1s via the rebase--helper

[Dennis Kaarsemaker]

On vr, 2016-09-02 at 18:23 +0200, Johannes Schindelin wrote:
> This is crucial to improve performance on Windows, as the speed is now
> mostly dominated by the SHA-1 transformation (because it spawns a new
> rev-parse process for *every* line, and spawning processes is pretty
> slow from Git for Windows' MSYS2 Bash).

I see these functions only used as part of an shorten-edit-expand
sequence. Why not do a git rebase-helper --edit-todo instead? Saves
another few process spawnings.

Something for yet another later followup patch?

D.

Re: [PATCH 2/2] connect: know that zero-ID is not a ref

[Jeff King]

On Fri, Sep 02, 2016 at 10:15:39AM -0700, Jonathan Tan wrote:

> connect.c, when processing packfiles, treats a zero ID (with
> `capabilities^{}` in place of the refname) as an actual ref instead of a
> placeholder for a capability declaration, contrary to the specification
> in Reference Discovery in Documentation/technical/pack-protocol.txt.
> This is an issue when interacting with Git implementations that follow
> this specification. For example, `ls-remote` (against a git://
> repository served by such a Git implementation) will report a ref when
> there are none, and `clone` (against something similar) will fail its
> checkout.
> 
> Make connect.c follow the specification with respect to this, while
> maintaining compatibility with existing implementations that do not
> serve the zero ID when a repository has no refs.

Hmm. So since this is backwards-compatible, I'm not overly concerned
with changing the client. But I wonder if you considered that the
documentation is wrong, and that JGit should stop sending the extra
capabilities line?

In either case, there will still be breakage until _somebody_ upgrades
(with your patch, until clients upgrade; with a JGit patch, until the
server upgrades). So perhaps an interesting question would be: if we
were writing this now, what is the correct behavior?

For pushing, it is obviously useful to send capabilities even though
there are no refs (because the client is going to send _you_ something).
And that is why "capabilities^{}" exists; it is a receive-pack feature
(that is actually implemented!), and the documentation (which came after
the implementation) blindly mentioned it for upload-pack, as well.

Is it useful for upload-pack? If we have no refs, there's traditionally
been nothing to fetch. Perhaps that's something that could change,
though. For example, there could be a capability to allow fetching
arbitrary sha1s (we have allowTIPSH1InWant and allowReachableSHA1InWant,
which obviously both require some refs, but allowArbitrarySHA1 does not
seem outside the realm of possibility).

I'll review the rest assuming that this is the direction we want to
go...

> (git-daemon should probably also be changed to serve zero IDs, but such
> a change can be considered independently from this change; even if both
> the client and server changes were made in one commit, it is nearly
> impossible that all Git installations are updated at the same time - an
> updated client would still need to deal with unupdated servers and vice
> versa.)

I'm really not sure what you mean here. How does git-daemon enter into
this at all?

> + if (is_null_oid(_oid)) {
> + if (strcmp(name, "capabilities^{}"))
> + warning("zero object ID received that is not 
> accompanied by a "
> + "capability declaration, ignoring and 
> continuing anyway");
> + continue;
> + }

I agree with Shawn that this should be looking for "capabilities^{}" as
the name of the first entry (and the warning can go away; if we see any
other null sha1, it just gets handled in the usual error code paths).

> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
> index 819b9dd..c6f8b6f 100755
> --- a/t/t5512-ls-remote.sh
> +++ b/t/t5512-ls-remote.sh
> @@ -207,5 +207,27 @@ test_expect_success 'ls-remote --symref omits 
> filtered-out matches' '
>   test_cmp expect actual
>  '
>  
> +test_lazy_prereq GIT_DAEMON '
> + test_have_prereq JGIT &&
> + test_tristate GIT_TEST_GIT_DAEMON &&
> + test "$GIT_TEST_GIT_DAEMON" != false
> +'

GIT_DAEMON depends on JGIT? Should this really be the JGIT_DAEMON
prerequisite?

> +# This test spawns a daemon, so run it only if the user would be OK with
> +# testing with git-daemon.
> +test_expect_success JGIT,GIT_DAEMON 'indicate no refs in standards-compliant 
> empty remote' '
> + JGIT_DAEMON_PID= &&
> + git init --bare empty.git &&
> + touch empty.git/git-daemon-export-ok &&
> + {
> + jgit daemon --port="$JGIT_DAEMON_PORT" . &
> + JGIT_DAEMON_PID=$!
> + } &&
> + test_when_finished kill "$JGIT_DAEMON_PID" &&
> + sleep 1 && # allow jgit daemon some time to set up

We definitely need something more robust than this "sleep". This test is
going to fail racily when the system is under load.

-Peff

Re: [PATCH 2/2] connect: know that zero-ID is not a ref

[Shawn Pearce]

On Fri, Sep 2, 2016 at 12:56 PM, Stefan Beller  wrote:
> On Fri, Sep 2, 2016 at 12:39 PM, Shawn Pearce  wrote:
>> On Fri, Sep 2, 2016 at 10:15 AM, Jonathan Tan  
>> wrote:
>>>
>>> +   if (is_null_oid(_oid)) {
>>> +   if (strcmp(name, "capabilities^{}"))
>>
>> Its not the zero ID that is special, its the "capabilities^{}" name
>> that is special when its the first entry in the stream. In the wire
>> protocol a "x^{}" line is a modifier to a prior "x" line to add a
>> peeled object to the prior line. But if we see "^{}" on the first line
>> that is non-sense, there is no prior line to modify with this
>> identifier.
>>
>> Further ^{} is used here because its invalid in a name. A server
>> really cannot have a reference that ends with the sequence ^{}. And a
>> server should not have a reference named "capabilities" without a
>> "refs/" prefix on it.
>>
>> So the entire "capabilities^{}" on the first line is a bunch of
>> contradictions that violate a number of things about the protocol,
>> which is why clients should ignore it.
>>
>> I think the test should be about:
>>
>>   !*list && !strcmp(name, "capabilities^{}")
>>
>>> +   warning("zero object ID received that is 
>>> not accompanied by a "
>>> +   "capability declaration, ignoring 
>>> and continuing anyway");
>>
>> Annoyingly a zero object ID is sort of possible; with a probability of
>> 1/2^160 or something. Its just a very very unlikely value. Slightly
>> stronger to test against the known invalid name.
>
> That ship has sailed long ago I would think?
> There are tests for null sha1 in the refs code, e.g. for
> deletion/creation of a branch
> we consider the null sha1 as the counterpart.
>
> So while it may be possible to have SHA1 producing a "0"x40, you
> cannot e.g. push it?

Indeed, you are correct.

I'm just stating the JGit client behavior is to look at
"capabilities^{}" in the first line as special, not the SHA-1.

[PATCH] sequencer: support folding in rfc2822 footer

[Jonathan Tan]

When running `git cherry-pick -x`, a blank line would be inserted if a line in
the footer was broken into multiple lines (which is inconsistent with its
behavior if no lines were broken). For example, this command would produce:

---
Sample-field: no blank line below
(cherry picked from commit ...)
---

but would also produce:

---
Sample-field: multiple-line field body
 that causes a blank line below

(cherry picked from commit ...)
---

This, among other things, trips up tools that look for the last paragraph
(including sequencer.c itself).

RFC 2822 allows field bodies to be split into multiple lines, especially (but
not only) to deal with the line-width limitations described in the
specification, referring to this as "folding".

Teach sequencer.c to treat split and unsplit field bodies in the same way (that
is, to not include the blank line).

Signed-off-by: Jonathan Tan 
---
 sequencer.c  | 19 +--
 t/t3511-cherry-pick-x.sh | 19 +++
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 3804fa9..411dd50 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -26,10 +26,20 @@ static GIT_PATH_FUNC(git_path_opts_file, SEQ_OPTS_FILE)
 static GIT_PATH_FUNC(git_path_seq_dir, SEQ_DIR)
 static GIT_PATH_FUNC(git_path_head_file, SEQ_HEAD_FILE)
 
-static int is_rfc2822_line(const char *buf, int len)
+static int is_rfc2822_line(const char *buf, int len,
+  int allow_folding_continuation)
 {
int i;
 
+   /*
+* Section 2.2.3 of RFC 2822 allows field bodies to continue onto
+* multiple lines, referred to as "folding". Such continuation lines
+* start with whitespace.
+*/
+   if (allow_folding_continuation)
+   if (len && (buf[0] == ' ' || buf[0] == '\t'))
+   return 1;
+
for (i = 0; i < len; i++) {
int ch = buf[i];
if (ch == ':')
@@ -64,6 +74,7 @@ static int has_conforming_footer(struct strbuf *sb, struct 
strbuf *sob,
int len = sb->len - ignore_footer;
const char *buf = sb->buf;
int found_sob = 0;
+   int allow_folding_continuation = 0;
 
/* footer must end with newline */
if (!len || buf[len - 1] != '\n')
@@ -92,7 +103,11 @@ static int has_conforming_footer(struct strbuf *sb, struct 
strbuf *sob,
; /* do nothing */
k++;
 
-   found_rfc2822 = is_rfc2822_line(buf + i, k - i - 1);
+   found_rfc2822 = is_rfc2822_line(buf + i,
+   k - i - 1,
+   allow_folding_continuation);
+   allow_folding_continuation = 1;
+
if (found_rfc2822 && sob &&
!strncmp(buf + i, sob->buf, sob->len))
found_sob = k;
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index 9cce5ae..1a50339 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -36,6 +36,11 @@ mesg_with_cherry_footer="$mesg_with_footer_sob
 (cherry picked from commit da39a3ee5e6b4b0d3255bfef95601890afd80709)
 Tested-by: C.U. Thor "
 
+mesg_with_folding_footer="$mesg_no_footer
+
+Field: This is a very long field body
+ that is continued onto another line"
+
 mesg_unclean="$mesg_one_line
 
 
@@ -68,6 +73,8 @@ test_expect_success setup '
git reset --hard initial &&
test_commit "$mesg_with_cherry_footer" foo b mesg-with-cherry-footer &&
git reset --hard initial &&
+   test_commit "$mesg_with_folding_footer" foo b mesg-with-folding-footer 
&&
+   git reset --hard initial &&
test_config commit.cleanup verbatim &&
test_commit "$mesg_unclean" foo b mesg-unclean &&
test_unconfig commit.cleanup &&
@@ -234,6 +241,18 @@ test_expect_success 'cherry-pick -x -s treats "(cherry 
picked from..." line as p
test_cmp expect actual
 '
 
+test_expect_success 'cherry-pick -x does not insert blank line when folding 
footer is found' '
+   pristine_detach initial &&
+   sha1=$(git rev-parse mesg-with-folding-footer^0) &&
+   git cherry-pick -x mesg-with-folding-footer &&
+   cat <<-EOF >expect &&
+   $mesg_with_folding_footer
+   (cherry picked from commit $sha1)
+   EOF
+   git log -1 --pretty=format:%B >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'cherry-pick preserves commit message' '
pristine_detach initial &&
printf "$mesg_unclean" >expect &&
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 2/2] connect: know that zero-ID is not a ref

[Stefan Beller]

On Fri, Sep 2, 2016 at 12:39 PM, Shawn Pearce  wrote:
> On Fri, Sep 2, 2016 at 10:15 AM, Jonathan Tan  
> wrote:
>>
>> +   if (is_null_oid(_oid)) {
>> +   if (strcmp(name, "capabilities^{}"))
>
> Its not the zero ID that is special, its the "capabilities^{}" name
> that is special when its the first entry in the stream. In the wire
> protocol a "x^{}" line is a modifier to a prior "x" line to add a
> peeled object to the prior line. But if we see "^{}" on the first line
> that is non-sense, there is no prior line to modify with this
> identifier.
>
> Further ^{} is used here because its invalid in a name. A server
> really cannot have a reference that ends with the sequence ^{}. And a
> server should not have a reference named "capabilities" without a
> "refs/" prefix on it.
>
> So the entire "capabilities^{}" on the first line is a bunch of
> contradictions that violate a number of things about the protocol,
> which is why clients should ignore it.
>
> I think the test should be about:
>
>   !*list && !strcmp(name, "capabilities^{}")
>
>> +   warning("zero object ID received that is not 
>> accompanied by a "
>> +   "capability declaration, ignoring 
>> and continuing anyway");
>
> Annoyingly a zero object ID is sort of possible; with a probability of
> 1/2^160 or something. Its just a very very unlikely value. Slightly
> stronger to test against the known invalid name.

That ship has sailed long ago I would think?
There are tests for null sha1 in the refs code, e.g. for
deletion/creation of a branch
we consider the null sha1 as the counterpart.

So while it may be possible to have SHA1 producing a "0"x40, you
cannot e.g. push it?

Re: git add -p—splitting hunks, limit is too large

[Christian Neukirchen]

Jeff King  writes:

> Splitting to single lines means you need to remember to add the matched
> pairs, which might be arbitrarily far apart.  That's not really any
> different than dumping the hunk in your editor, but I find there that
> it's easy to rearrange and group things as appropriate.

My main use case for this would be to split a plain addition into
several small additions.  Which would be much easier with the
menu-driven approach.

(Mostly I just use magit, but sometimes I want to do this on machines
without emacs set up.)

-- 
Christian Neukirchen    http://chneukirchen.org

Re: [PATCH 2/2] connect: know that zero-ID is not a ref

[Shawn Pearce]

On Fri, Sep 2, 2016 at 10:15 AM, Jonathan Tan  wrote:
>
> +   if (is_null_oid(_oid)) {
> +   if (strcmp(name, "capabilities^{}"))

Its not the zero ID that is special, its the "capabilities^{}" name
that is special when its the first entry in the stream. In the wire
protocol a "x^{}" line is a modifier to a prior "x" line to add a
peeled object to the prior line. But if we see "^{}" on the first line
that is non-sense, there is no prior line to modify with this
identifier.

Further ^{} is used here because its invalid in a name. A server
really cannot have a reference that ends with the sequence ^{}. And a
server should not have a reference named "capabilities" without a
"refs/" prefix on it.

So the entire "capabilities^{}" on the first line is a bunch of
contradictions that violate a number of things about the protocol,
which is why clients should ignore it.

I think the test should be about:

  !*list && !strcmp(name, "capabilities^{}")

> +   warning("zero object ID received that is not 
> accompanied by a "
> +   "capability declaration, ignoring and 
> continuing anyway");

Annoyingly a zero object ID is sort of possible; with a probability of
1/2^160 or something. Its just a very very unlikely value. Slightly
stronger to test against the known invalid name.

Re: [PATCH 2/2] connect: know that zero-ID is not a ref

[Jonathan Nieder]

Hi,

Jonathan Tan wrote:

> connect.c, when processing packfiles, treats a zero ID (with
> `capabilities^{}` in place of the refname) as an actual ref instead of a
> placeholder for a capability declaration, contrary to the specification
> in Reference Discovery in Documentation/technical/pack-protocol.txt.
> This is an issue when interacting with Git implementations that follow
> this specification. For example, `ls-remote` (against a git://
> repository served by such a Git implementation) will report a ref when
> there are none, and `clone` (against something similar) will fail its
> checkout.

For example, this means I get the following confusing message when
cloning an empty repository served by "jgit daemon":

 $ git clone https://googlers.googlesource.com/jrn/empty
 Cloning into 'empty'...
 Checking connectivity... done.
 warning: remote HEAD refers to nonexistent ref, unable to checkout.

 $

It also means that standard "git daemon" is not able to advertise
capabilities on a fetch from a repository without advertising some
refs, so I cannot fetch-by-sha1 from a C git server unless some refs
are advertised.

This fix should solve the former problem and paves the way for the
latter to be solved once a year or two passes and people's clients are
up to date (or earlier if a server operator chooses).

> Make connect.c follow the specification with respect to this, while
> maintaining compatibility with existing implementations that do not
> serve the zero ID when a repository has no refs.

This description focuses on the code.  When deciding whether to apply
the patch (or whether to revert it if it comes up while trying to
investigate another problem), I am likely to wonder "what effect will
the patch have on me?" instead of "what standards does it follow?"

Flipping the emphasis, we could say something like

connect: treat ref advertisement with capabilities^{} line as one with 
no refs

When cloning an empty repository served by standard git, "git clone"
produces the following reassuring message:

$ git clone git://localhost/tmp/empty
Cloning into 'empty'...
warning: You appear to have cloned an empty repository.
Checking connectivity... done.

Meanwhile when cloning an empty repository served by JGit, the output
is more haphazard:

$ git clone git://localhost/tmp/empty
Cloning into 'empty'...
Checking connectivity... done.
warning: remote HEAD refers to nonexistent ref, unable to 
checkout.

This is a common command to run immediately after creating a remote
repository as preparation for adding content to populate it and pushing.
The warning is confusing and needlessly worrying.

The cause is that, since v3.1.0.201309270735-rc1~22 (Advertise 
capabilities
with no refs in upload service., 2013-08-08), JGit's ref advertisement
includes a ref named capabilities^{} to advertise its refs on, while 
git's
ref advertisement is empty in this case.  This allows the client to 
learn
about the server's capabilities and is need, for example, for 
fetch-by-sha1
to work when no refs are advertised.

Git advertises the same capabilities^{} ref in its ref advertisement for
push but since it never remembered to do so for fetch, the client forgot
to handle this case. Handle it.

So now you can see the same friendly message whether the server runs C
git or JGit.

The pack-protocol.txt documentation gives some guidance about the 
expected
server behavior: what JGit does is currently required, since a 
list-of-refs
is required to be non-empty.

  advertised-refs  =  (no-refs / list-of-refs)
  *shallow
  flush-pkt

  no-refs  =  PKT-LINE(zero-id SP "capabilities^{}"
  NUL capability-list)

  list-of-refs =  first-ref *other-ref

Because git client versions without this fix are expected to exist in 
the
wild for a while, we should not change the server to always send the
capabilities^{} line when there are no refs to advertise yet.  A 
transition
will take multiple steps:

 1. This patch, which updates the client

 2. Update pack-protocol to clarify that both server behaviors must be
tolerated.

 3. Add a configuration variable to allow git upload-pack to advertise
capabilities when there are no refs to advertise.  Leave it disabled
by default since git clients can't be counted on to have this patch 
(1)
yet.

 4. After a year or so, flip the default for that server configuration
variable to true.

[...]
> --- a/connect.c
> +++ 

Fixup of a fixup not working right

[Robert Dailey]

Suppose I have a branch with 4 commits, in the following order (as you
might see during interactive rebase):

pick 123 Original Change
pick 789 fixup! Original Change
pick 456 Some Other Thing
pick abc fixup! fixup! Original Change

However, let's say the first commit is already pushed upstream on a
topic branch. Since there are multiple developers on this topic
branch, I do not want to rebase right now. Instead, I want to document
future fixups via fixup commits and then when we're ready to merge, do
a final rebase prior to the merge to master to clean things up after
we're all done collaborating.

For this specific situation, since the first commit is already pushed,
I want to perform a fixup on the 1st fixup commit. When I perform an
interactive rebase against upstream topic, I get the following:

pick 789 fixup! Original Change
pick 456 Some Other Thing
pick abc fixup! fixup! Original Change

The tip commit (abc in this case) is not marked as a fixup. What I
expect to see is:

pick 789 fixup! Original Change
fixup abc fixup! fixup! Original Change
pick 456 Some Other Thing

Is this by design, or a defect? I assumed that Git would only look at
the first occurrence of "fixup!" and treat everything else after as
the commit description to match. But it seems in this case that it
stops at the last occurrence of "fixup!", which would explain why it
isn't matching in the interactive rebase. I haven't looked at the
code, though.

Thoughts? Also I'm perfectly willing to accept feedback involving me
just using the feature wrong or as not intended. Thanks in advance.

If a branch moves a submodule, "merge --ff[-only]" succeeds while "merge --no-ff" fails with conflicts

[Dakota Hawkins]

Below is a simple reproduction of the issue.

The _real_ problem is that this is how our pull request merges work,
they're not allowed to do fast-forward merges. To work around this we
are having to split this up into two pull requests/merges: One that
copies the submodules to the new location and includes any fixes
required to support the move, and a second that removes the old
locations.

## Setup steps
git clone 
https://github.com/dakotahawkins/submodule-move-merge-bug-main-repo.git
cd submodule-move-merge-bug-main-repo
## How it was initially constructed
# git submodule add ../submodule-move-merge-bug-submodule-repo.git
./submodule-location-1
# git commit -m "Added submodule in its initial location"
# git push
# git checkout -b move-submodule
# git mv ./submodule-location-1 ./submodule-location-2
# git commit -m "Moved submodule"
# git push --set-upstream origin move-submodule
git branch move-submodule origin/move-submodule

## Test fast-forward merge, this will work
git checkout -b merge-ff-test master # warning: unable to rmdir
submodule-location-2: Directory not empty
rm -rf ./submodule-location-2
git merge --ff-only move-submodule

## Test no-fast-forward merge, this will fail with conflicts:
git checkout -b merge-no-ff-test master
git merge --no-ff move-submodule
# Auto-merging submodule-location-2
# Adding as submodule-location-2~move-submodule instead
# Automatic merge failed; fix conflicts and then commit the result.
git status
# On branch merge-no-ff-test
# You have unmerged paths.
#   (fix conflicts and run "git commit")
#   (use "git merge --abort" to abort the merge)
#
# Changes to be committed:
#
# modified:   .gitmodules
# deleted:submodule-location-1
#
# Unmerged paths:
#   (use "git add ..." to mark resolution)
#
# added by us: submodule-location-2
#
# fatal: Not a git repository: 'submodule-location-1/.git'
# Submodule changes to be committed:
#
# * submodule-location-1 07fec24...000:

Re: git add -p—splitting hunks, limit is too large

[Jeff King]

On Fri, Sep 02, 2016 at 03:36:58PM +0100, Beau Martinez wrote:

> I'd like to inquire as to why `git add -p` can only split hunks so
> much. The limit is too large; why can't you split until each hunk is
> only a line? I often have to run `edit` and split them manually
> myself.

There's some previous discussion in this thread:

  http://public-inbox.org/git/200805232221.45406.tr...@student.ethz.ch/t/#u

and further back, this message:

  http://public-inbox.org/git/7vbq8v7cdx@gitster.siamese.dyndns.org/

I think one problem is that in a given contiguous hunk, not all of the
lines are independent, because edits are represented as a pair of -/+
lines. E.g., if the preimage is:

  one
  two
  four

and the postimage is:

  one
  two modified
  three
  four

your diff will be:

   one
  -two
  +two modified
  +three
   four

The ideal split is two groups:

  -two
  +two modified

  +three

So you could possibly achieve that by specifying the exact line to split
at. But let's imagine "two" was the missing item, and we modified
"three". Then your diff is:

   one
  -three
  +two
  +three modified
   four

Now the related lines are non-adjacent! I don't think there's a general
solution, and of course it can get arbitrarily complicated, with many
interleaved pairs. I don't think we can rely on figuring out which lines
form a pair. In this toy example it's obvious, but in real diffs the
lines might not bear any resemblance.

Splitting to single lines means you need to remember to add the matched
pairs, which might be arbitrarily far apart.  That's not really any
different than dumping the hunk in your editor, but I find there that
it's easy to rearrange and group things as appropriate.

> I'd like to contribute a patch to change it, although my C is rusty.
> Are there resources that will help me to do this?

The good news (or maybe the bad) is that "add -p" is implemented
entirely in Perl. :) It's in git-add--interactive.perl.

-Peff

[PATCH 2/2] connect: know that zero-ID is not a ref

[Jonathan Tan]

connect.c, when processing packfiles, treats a zero ID (with
`capabilities^{}` in place of the refname) as an actual ref instead of a
placeholder for a capability declaration, contrary to the specification
in Reference Discovery in Documentation/technical/pack-protocol.txt.
This is an issue when interacting with Git implementations that follow
this specification. For example, `ls-remote` (against a git://
repository served by such a Git implementation) will report a ref when
there are none, and `clone` (against something similar) will fail its
checkout.

Make connect.c follow the specification with respect to this, while
maintaining compatibility with existing implementations that do not
serve the zero ID when a repository has no refs.

(git-daemon should probably also be changed to serve zero IDs, but such
a change can be considered independently from this change; even if both
the client and server changes were made in one commit, it is nearly
impossible that all Git installations are updated at the same time - an
updated client would still need to deal with unupdated servers and vice
versa.)

The test uses JGit's daemon feature, which is specification-compliant.

Signed-off-by: Jonathan Tan 
---
 connect.c|  7 +++
 t/t5512-ls-remote.sh | 22 ++
 2 files changed, 29 insertions(+)

diff --git a/connect.c b/connect.c
index 722dc3f..d4a58de 100644
--- a/connect.c
+++ b/connect.c
@@ -165,6 +165,13 @@ struct ref **get_remote_heads(int in, char *src_buf, 
size_t src_len,
continue;
}
 
+   if (is_null_oid(_oid)) {
+   if (strcmp(name, "capabilities^{}"))
+   warning("zero object ID received that is not 
accompanied by a "
+   "capability declaration, ignoring and 
continuing anyway");
+   continue;
+   }
+
if (!check_ref(name, flags))
continue;
ref = alloc_ref(buffer + GIT_SHA1_HEXSZ + 1);
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 819b9dd..c6f8b6f 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -207,5 +207,27 @@ test_expect_success 'ls-remote --symref omits filtered-out 
matches' '
test_cmp expect actual
 '
 
+test_lazy_prereq GIT_DAEMON '
+   test_have_prereq JGIT &&
+   test_tristate GIT_TEST_GIT_DAEMON &&
+   test "$GIT_TEST_GIT_DAEMON" != false
+'
+
+JGIT_DAEMON_PORT=${JGIT_DAEMON_PORT-${this_test#t}}
+
+# This test spawns a daemon, so run it only if the user would be OK with
+# testing with git-daemon.
+test_expect_success JGIT,GIT_DAEMON 'indicate no refs in standards-compliant 
empty remote' '
+   JGIT_DAEMON_PID= &&
+   git init --bare empty.git &&
+   touch empty.git/git-daemon-export-ok &&
+   {
+   jgit daemon --port="$JGIT_DAEMON_PORT" . &
+   JGIT_DAEMON_PID=$!
+   } &&
+   test_when_finished kill "$JGIT_DAEMON_PID" &&
+   sleep 1 && # allow jgit daemon some time to set up
+   test_expect_code 2 git ls-remote --exit-code 
git://localhost:$JGIT_DAEMON_PORT/empty.git
+'
 
 test_done
-- 
2.8.0.rc3.226.g39d4020

[PATCH 1/2] tests: move test_lazy_prereq JGIT to test-lib.sh

[Jonathan Tan]

This enables JGIT to be used as a prereq in invocations of
test_expect_success (and other functions) in other test scripts.

Signed-off-by: Jonathan Tan 
---
 t/t5310-pack-bitmaps.sh | 4 
 t/test-lib.sh   | 4 
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 3893afd..1e376ea 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -158,10 +158,6 @@ test_expect_success 'pack with missing parent' '
git pack-objects --stdout --revs /dev/null
 '
 
-test_lazy_prereq JGIT '
-   type jgit
-'
-
 test_expect_success JGIT 'we can read jgit bitmaps' '
git clone . compat-jgit &&
(
diff --git a/t/test-lib.sh b/t/test-lib.sh
index d731d66..c9c1037 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1072,6 +1072,10 @@ test_lazy_prereq NOT_ROOT '
test "$uid" != 0
 '
 
+test_lazy_prereq JGIT '
+   type jgit
+'
+
 # SANITY is about "can you correctly predict what the filesystem would
 # do by only looking at the permission bits of the files and
 # directories?"  A typical example of !SANITY is running the test
-- 
2.8.0.rc3.226.g39d4020

[PATCH 0/2] handle empty spec-compliant remote repos correctly

[Jonathan Tan]

This issue was noticed when trying to clone an empty repository served by a
server that uses the JGit library.

Reference Discovery in Documentation/technical/pack-protocol.txt dictates that
servers should send a zero ID when there are no refs in the queried repository,
and implementations like JGit do, but the C client does not handle them
correctly (treating them as an actual ref and subsequently returning incorrect
responses or errors).

These patches fix those while maintaining backwards compatibility with existing
implementations that do not send the zero ID in such a case.

Jonathan Tan (2):
  tests: move test_lazy_prereq JGIT to test-lib.sh
  connect: know that zero-ID is not a ref

 connect.c   |  7 +++
 t/t5310-pack-bitmaps.sh |  4 
 t/t5512-ls-remote.sh| 22 ++
 t/test-lib.sh   |  4 
 4 files changed, 33 insertions(+), 4 deletions(-)

-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 07/34] sequencer (rebase -i): add support for the 'fixup' and 'squash' commands

[Dennis Kaarsemaker]

On vr, 2016-09-02 at 16:22 +0200, Johannes Schindelin wrote:
> I hope this clarifies why I am not so concerned about some issues
> such as translation, or commit messages, or grammar, and more so
> about others, such as incorrect code.

It does, thanks!

D.

Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests

[Matthieu Moy]

Johannes Schindelin  writes:

> Hi Ævar,
>
> On Fri, 2 Sep 2016, Ævar Arnfjörð Bjarmason wrote:
>
>> On Wed, Aug 31, 2016 at 5:05 PM, Johannes Schindelin
>>  wrote:
>>
>> > The biggest problem with Strawberry Perl is that it is virtually
>> > impossible to build the Subversion-Perl bindings using the Git for
>> > Windows SDK when using Strawberry Perl.
>> >
>> > Which pretty much precludes it from being used in Git for Windows.
>> >
>> > And then there are the path issues... Git's Perl scripts are pretty
>> > certain that they live in a POSIX-y environment. Which MSYS2 Perl
>> > provides. Strawberry Perl not.
>> 
>> This might be me missing the point, and I'm really just trying to be
>> helpful here and make "prove" work for you because it's awesome, but
>> as far as just you running this for development purposes does any of
>> this SVN stuff matter? I.e. you can build Git itself not with
>> Strawberry, but just use Strawberry to get a working copy of "prove".
>
> Yes, the SVN stuff matters, because of the many t9*svn* tests (which, BTW
> take a substantial time to run). So if I run the test suite, I better do
> it with a perl.exe in the PATH that can run the SVN tests. Otherwise I
> might just as well not bother with running the entire test suite...

Maybe something like

\path\to\strawberry-perl\perl.exe \path\to\prove ...

without changing the PATH would work. I wouldn't call that convenient
though.

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

[PATCH 0/9] The final building block for a faster rebase -i

[Johannes Schindelin]

This patch series reimplements the expensive pre- and post-processing of
the todo script in C.

And it concludes the work I did to accelerate rebase -i.


Johannes Schindelin (9):
  rebase -i: generate the script via rebase--helper
  rebase -i: remove useless indentation
  rebase -i: do not invent onelines when expanding/collapsing SHA-1s
  rebase -i: also expand/collapse the SHA-1s via the rebase--helper
  t3404: relax rebase.missingCommitsCheck tests
  rebase -i: check for missing commits in the rebase--helper
  rebase -i: skip unnecessary picks using the rebase--helper
  t3415: test fixup with wrapped oneline
  rebase -i: rearrange fixup/squash lines using the rebase--helper

 builtin/rebase--helper.c  |  29 ++-
 git-rebase--interactive.sh| 362 -
 sequencer.c   | 514 ++
 sequencer.h   |   7 +
 t/t3404-rebase-interactive.sh |  22 +-
 t/t3415-rebase-autosquash.sh  |  16 +-
 6 files changed, 614 insertions(+), 336 deletions(-)

Based-On: rebase--helper at https://github.com/dscho/git
Fetch-Base-Via: git fetch https://github.com/dscho/git rebase--helper
Published-As: https://github.com/dscho/git/releases/tag/rebase-i-extra-v1
Fetch-It-Via: git fetch https://github.com/dscho/git rebase-i-extra-v1

-- 
2.9.3.windows.3

base-commit: 4c39918f42eb8228ea4241073f86f2ac851f4636

[PATCH 4/9] rebase -i: also expand/collapse the SHA-1s via the rebase--helper

[Johannes Schindelin]

This is crucial to improve performance on Windows, as the speed is now
mostly dominated by the SHA-1 transformation (because it spawns a new
rev-parse process for *every* line, and spawning processes is pretty
slow from Git for Windows' MSYS2 Bash).

Signed-off-by: Johannes Schindelin 
---
 builtin/rebase--helper.c   | 10 +++-
 git-rebase--interactive.sh |  4 ++--
 sequencer.c| 59 ++
 sequencer.h|  2 ++
 4 files changed, 72 insertions(+), 3 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 821058d..9444c8d 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -13,7 +13,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
struct replay_opts opts = REPLAY_OPTS_INIT;
int keep_empty = 0;
enum {
-   CONTINUE = 1, ABORT, MAKE_SCRIPT
+   CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -24,6 +24,10 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
ABORT),
OPT_CMDMODE(0, "make-script", ,
N_("make rebase script"), MAKE_SCRIPT),
+   OPT_CMDMODE(0, "shorten-sha1s", ,
+   N_("shorten SHA-1s in the todo list"), SHORTEN_SHA1S),
+   OPT_CMDMODE(0, "expand-sha1s", ,
+   N_("expand SHA-1s in the todo list"), EXPAND_SHA1S),
OPT_END()
};
 
@@ -42,5 +46,9 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
return !!sequencer_remove_state();
if (command == MAKE_SCRIPT && argc > 1)
return !!sequencer_make_script(keep_empty, stdout, argc, argv);
+   if (command == SHORTEN_SHA1S && argc == 1)
+   return !!transform_todo_ids(1);
+   if (command == EXPAND_SHA1S && argc == 1)
+   return !!transform_todo_ids(0);
usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 0eb5583..f642ec2 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -764,11 +764,11 @@ transform_todo_ids () {
 }
 
 expand_todo_ids() {
-   transform_todo_ids
+   git rebase--helper --expand-sha1s
 }
 
 collapse_todo_ids() {
-   transform_todo_ids --short
+   git rebase--helper --shorten-sha1s
 }
 
 # Rearrange the todo list that has both "pick sha1 msg" and
diff --git a/sequencer.c b/sequencer.c
index 43e078a..ee4fdb0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2391,3 +2391,62 @@ int sequencer_make_script(int keep_empty, FILE *out,
strbuf_release();
return 0;
 }
+
+
+int transform_todo_ids(int shorten_sha1s)
+{
+   const char *todo_file = rebase_path_todo();
+   struct todo_list todo_list = TODO_LIST_INIT;
+   int fd, res, i;
+   FILE *out;
+
+   strbuf_reset(_list.buf);
+   fd = open(todo_file, O_RDONLY);
+   if (fd < 0)
+   return error_errno(_("Could not open '%s'"), todo_file);
+   if (strbuf_read(_list.buf, fd, 0) < 0) {
+   close(fd);
+   return error(_("Could not read %s."), todo_file);
+   }
+   close(fd);
+
+   res = parse_insn_buffer(todo_list.buf.buf, _list);
+   if (res) {
+   todo_list_release(_list);
+   return error(_("Unusable instruction sheet: %s"), todo_file);
+   }
+
+   out = fopen(todo_file, "w");
+   if (!out) {
+   todo_list_release(_list);
+   return error(_("Unable to open '%s' for writing"), todo_file);
+   }
+   for (i = 0; i < todo_list.nr; i++) {
+   struct todo_item *item = todo_list.items + i;
+   int bol = item->offset_in_buf;
+   const char *p = todo_list.buf.buf + bol;
+   int eol = i + 1 < todo_list.nr ?
+   todo_list.items[i + 1].offset_in_buf :
+   todo_list.buf.len;
+
+   if (item->command >= TODO_EXEC && item->command != TODO_DROP)
+   fwrite(p, eol - bol, 1, out);
+   else {
+   int eoc = strcspn(p, " \t");
+   const char *sha1 = shorten_sha1s ?
+   short_commit_name(item->commit) :
+   oid_to_hex(>commit->object.oid);
+
+   if (!eoc) {
+   p += strspn(p, " \t");
+   eoc = strcspn(p, " \t");
+   }
+
+   fprintf(out, "%.*s %s %.*s\n",
+   eoc, p, sha1, item->arg_len, item->arg);
+   }
+   }
+   

[PATCH 3/9] rebase -i: do not invent onelines when expanding/collapsing SHA-1s

[Johannes Schindelin]

To avoid problems with short SHA-1s that become non-unique during the
rebase, we rewrite the todo script with short/long SHA-1s before and
after letting the user edit the script. Since SHA-1s are not intuitive
for humans, rebase -i also provides the onelines (commit message
subjects) in the script, purely for the user's convenience.

It is very possible to generate a todo script via different means than
rebase -i and then to let rebase -i run with it; In this case, these
onelines are not required.

And this is where the expand/collapse machinery has a bug: it *expects*
that oneline, and failing to find one reuses the previous SHA-1 as
"oneline".

It was most likely an oversight, and made implementation in the (quite
limiting) shell script language less convoluted. However, we are about
to reimplement performance-critical parts in C (and due to spawning a
git.exe process for every single line of the todo script, the
expansion/collapsing of the SHA-1s *is* performance-hampering on
Windows), therefore let's fix this bug to make cross-validation with the
C version of that functionality possible.

Signed-off-by: Johannes Schindelin 
---
 git-rebase--interactive.sh | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 5df5850..0eb5583 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -750,7 +750,12 @@ transform_todo_ids () {
;;
*)
sha1=$(git rev-parse --verify --quiet "$@" ${rest%%[
 ]*}) &&
-   rest="$sha1 ${rest#*[]}"
+   if test "a$rest" = "a${rest#*[   ]}"
+   then
+   rest=$sha1
+   else
+   rest="$sha1 ${rest#*[]}"
+   fi
;;
esac
printf '%s\n' "$command${rest:+ }$rest"
-- 
2.9.3.windows.3

[PATCH 7/9] rebase -i: skip unnecessary picks using the rebase--helper

[Johannes Schindelin]

In particular on Windows, where shell scripts are even more expensive
than on MacOSX or Linux, it makes sense to move a loop that forks
Git at least once for every line in the todo list into a builtin.

Note: The original code did not try to skip unnecessary picks of root
commits but punts instead (probably --root was not considered common
enough of a use case to bother optimizing). We do the same, for now.

Signed-off-by: Johannes Schindelin 
---
 builtin/rebase--helper.c   |  6 +++-
 git-rebase--interactive.sh | 41 ++---
 sequencer.c| 90 ++
 sequencer.h|  1 +
 4 files changed, 99 insertions(+), 39 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index e706eac..de3ccd9 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -14,7 +14,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
int keep_empty = 0;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S,
-   CHECK_TODO_LIST
+   CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -31,6 +31,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
N_("expand SHA-1s in the todo list"), EXPAND_SHA1S),
OPT_CMDMODE(0, "check-todo-list", ,
N_("check the todo list"), CHECK_TODO_LIST),
+   OPT_CMDMODE(0, "skip-unnecessary-picks", ,
+   N_("skip unnecessary picks"), SKIP_UNNECESSARY_PICKS),
OPT_END()
};
 
@@ -55,5 +57,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
return !!transform_todo_ids(0);
if (command == CHECK_TODO_LIST && argc == 1)
return !!check_todo_list();
+   if (command == SKIP_UNNECESSARY_PICKS && argc == 1)
+   return !!skip_unnecessary_picks();
usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 02a7698..a34ebdc 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -703,43 +703,6 @@ do_rest () {
done
 }
 
-# skip picking commits whose parents are unchanged
-skip_unnecessary_picks () {
-   fd=3
-   while read -r command rest
-   do
-   # fd=3 means we skip the command
-   case "$fd,$command" in
-   3,pick|3,p)
-   # pick a commit whose parent is current $onto -> skip
-   sha1=${rest%% *}
-   case "$(git rev-parse --verify --quiet "$sha1"^)" in
-   "$onto"*)
-   onto=$sha1
-   ;;
-   *)
-   fd=1
-   ;;
-   esac
-   ;;
-   3,"$comment_char"*|3,)
-   # copy comments
-   ;;
-   *)
-   fd=1
-   ;;
-   esac
-   printf '%s\n' "$command${rest:+ }$rest" >&$fd
-   done <"$todo" >"$todo.new" 3>>"$done" &&
-   mv -f "$todo".new "$todo" &&
-   case "$(peek_next_command)" in
-   squash|s|fixup|f)
-   record_in_rewritten "$onto"
-   ;;
-   esac ||
-   die "$(gettext "Could not skip unnecessary pick commands")"
-}
-
 transform_todo_ids () {
while read -r command rest
do
@@ -1165,7 +1128,9 @@ git rebase--helper --check-todo-list || {
 
 expand_todo_ids
 
-test -d "$rewritten" || test -n "$force_rebase" || skip_unnecessary_picks
+test -d "$rewritten" || test -n "$force_rebase" ||
+onto="$(git rebase--helper --skip-unnecessary-picks)" ||
+die "Could not skip unnecessary pick commands"
 
 checkout_onto
 if test -z "$rebase_root" && test ! -d "$rewritten"
diff --git a/sequencer.c b/sequencer.c
index 0c82925..6cc94c9 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2574,3 +2574,93 @@ int check_todo_list(void)
 
return res;
 }
+
+/* skip picking commits whose parents are unchanged */
+int skip_unnecessary_picks(void)
+{
+   const char *todo_file = rebase_path_todo();
+   struct strbuf buf = STRBUF_INIT;
+   struct todo_list todo_list = TODO_LIST_INIT;
+   struct object_id onto_oid, *oid = _oid, *parent_oid;
+   int fd, i;
+
+   if (!read_oneliner(, rebase_path_onto(), 0))
+   return error("Could not read 'onto'");
+   if (get_sha1(buf.buf, onto_oid.hash)) {
+   strbuf_release();
+   return error("Need a HEAD to fixup");
+   }
+   strbuf_release();
+
+   fd = open(todo_file, 

[PATCH 2/9] rebase -i: remove useless indentation

[Johannes Schindelin]

The commands used to be indented, and it is nice to look at, but when we
transform the SHA-1s, the indentation is removed. So let's do away with it.

For the moment, at least: when we will use the upcoming rebase--helper
to transform the SHA-1s, we *will* keep the indentation and can
reintroduce it. Yet, to be able to validate the rebase--helper against
the output of the current shell script version, we need to remove the
extra indentation.

Signed-off-by: Johannes Schindelin 
---
 git-rebase--interactive.sh | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 01c9fec..5df5850 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -146,13 +146,13 @@ reschedule_last_action () {
 append_todo_help () {
gettext "
 Commands:
- p, pick = use commit
- r, reword = use commit, but edit the commit message
- e, edit = use commit, but stop for amending
- s, squash = use commit, but meld into previous commit
- f, fixup = like \"squash\", but discard this commit's log message
- x, exec = run command (the rest of the line) using shell
- d, drop = remove commit
+p, pick = use commit
+r, reword = use commit, but edit the commit message
+e, edit = use commit, but stop for amending
+s, squash = use commit, but meld into previous commit
+f, fixup = like \"squash\", but discard this commit's log message
+x, exec = run command (the rest of the line) using shell
+d, drop = remove commit
 
 These lines can be re-ordered; they are executed from top to bottom.
 " | git stripspace --comment-lines >>"$todo"
-- 
2.9.3.windows.3

[PATCH 9/9] rebase -i: rearrange fixup/squash lines using the rebase--helper

[Johannes Schindelin]

This operation has quadratic complexity, which is especially painful
on Windows, where shell scripts are *already* slow (mainly due to the
overhead of the POSIX emulation layer).

Let's reimplement this with linear complexity (using a hash map to
match the commits' subject lines) for the common case; Sadly, the
fixup/squash feature's design neglected performance considerations,
allowing arbitrary prefixes (read: `fixup! hell` will match the
commit subject `hello world`), which means that we are stuck with
quadratic performance in the worst case.

The reimplemented logic also happens to fix a bug where commented-out
lines (representing empty patches) were dropped by the previous code.

Signed-off-by: Johannes Schindelin 
---
 builtin/rebase--helper.c |   6 +-
 git-rebase--interactive.sh   |  90 +---
 sequencer.c  | 197 +++
 sequencer.h  |   1 +
 t/t3415-rebase-autosquash.sh |   2 +-
 5 files changed, 205 insertions(+), 91 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index de3ccd9..e6591f0 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -14,7 +14,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
int keep_empty = 0;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S,
-   CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS
+   CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -33,6 +33,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
N_("check the todo list"), CHECK_TODO_LIST),
OPT_CMDMODE(0, "skip-unnecessary-picks", ,
N_("skip unnecessary picks"), SKIP_UNNECESSARY_PICKS),
+   OPT_CMDMODE(0, "rearrange-squash", ,
+   N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
OPT_END()
};
 
@@ -59,5 +61,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
return !!check_todo_list();
if (command == SKIP_UNNECESSARY_PICKS && argc == 1)
return !!skip_unnecessary_picks();
+   if (command == REARRANGE_SQUASH && argc == 1)
+   return !!rearrange_squash();
usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index a34ebdc..68a6e6a 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -734,94 +734,6 @@ collapse_todo_ids() {
git rebase--helper --shorten-sha1s
 }
 
-# Rearrange the todo list that has both "pick sha1 msg" and
-# "pick sha1 fixup!/squash! msg" appears in it so that the latter
-# comes immediately after the former, and change "pick" to
-# "fixup"/"squash".
-#
-# Note that if the config has specified a custom instruction format
-# each log message will be re-retrieved in order to normalize the
-# autosquash arrangement
-rearrange_squash () {
-   format=$(git config --get rebase.instructionFormat)
-   # extract fixup!/squash! lines and resolve any referenced sha1's
-   while read -r pick sha1 message
-   do
-   test -z "${format}" || message=$(git log -n 1 --format="%s" 
${sha1})
-   case "$message" in
-   "squash! "*|"fixup! "*)
-   action="${message%%!*}"
-   rest=$message
-   prefix=
-   # skip all squash! or fixup! (but save for later)
-   while :
-   do
-   case "$rest" in
-   "squash! "*|"fixup! "*)
-   prefix="$prefix${rest%%!*},"
-   rest="${rest#*! }"
-   ;;
-   *)
-   break
-   ;;
-   esac
-   done
-   printf '%s %s %s %s\n' "$sha1" "$action" "$prefix" 
"$rest"
-   # if it's a single word, try to resolve to a full sha1 
and
-   # emit a second copy. This allows us to match on both 
message
-   # and on sha1 prefix
-   if test "${rest#* }" = "$rest"; then
-   fullsha="$(git rev-parse -q --verify "$rest" 
2>/dev/null)"
-   if test -n "$fullsha"; then
-   # prefix the action to uniquely 
identify this line as
-   # intended for full sha1 match
-

[PATCH 8/9] t3415: test fixup with wrapped oneline

[Johannes Schindelin]

The `git commit --fixup` command unwraps wrapped onelines when
constructing the commit message, without wrapping the result.

We need to make sure that `git rebase --autosquash` keeps handling such
cases correctly, in particular since we are about to move the autosquash
handling into the rebase--helper.

Signed-off-by: Johannes Schindelin 
---
 t/t3415-rebase-autosquash.sh | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index 48346f1..9fd629a 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -304,4 +304,18 @@ test_expect_success 'extra spaces after fixup!' '
test $base = $parent
 '
 
+test_expect_success 'wrapped original subject' '
+   if test -d .git/rebase-merge; then git rebase --abort; fi &&
+   base=$(git rev-parse HEAD) &&
+   echo "wrapped subject" >wrapped &&
+   git add wrapped &&
+   test_tick &&
+   git commit --allow-empty -m "$(printf "To\nfixup")" &&
+   test_tick &&
+   git commit --allow-empty -m "fixup! To fixup" &&
+   git rebase -i --autosquash --keep-empty HEAD~2 &&
+   parent=$(git rev-parse HEAD^) &&
+   test $base = $parent
+'
+
 test_done
-- 
2.9.3.windows.3

[PATCH 5/9] t3404: relax rebase.missingCommitsCheck tests

[Johannes Schindelin]

These tests were a bit anal about the *exact* warning/error message
printed by git rebase. But those messages are intended for the *end
user*, therefore it does not make sense to test so rigidly for the
*exact* wording.

In the following, we will reimplement the missing commits check in
the sequencer, with slightly different words.

So let's just test for the parts in the warning/error message that
we *really* care about, nothing more, nothing less.

Signed-off-by: Johannes Schindelin 
---
 t/t3404-rebase-interactive.sh | 22 --
 1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 597e94e..a18759e 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1215,20 +1215,13 @@ test_expect_success 'rebase -i respects 
rebase.missingCommitsCheck = error' '
test B = $(git cat-file commit HEAD^ | sed -ne \$p)
 '
 
-cat >expect expect 

[PATCH 6/9] rebase -i: check for missing commits in the rebase--helper

[Johannes Schindelin]

In particular on Windows, where shell scripts are even more expensive
than on MacOSX or Linux, it makes sense to move a loop that forks
Git at least once for every line in the todo list into a builtin.

Signed-off-by: Johannes Schindelin 
---
 builtin/rebase--helper.c   |   7 +-
 git-rebase--interactive.sh | 164 ++---
 sequencer.c| 124 ++
 sequencer.h|   1 +
 4 files changed, 136 insertions(+), 160 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 9444c8d..e706eac 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -13,7 +13,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
struct replay_opts opts = REPLAY_OPTS_INIT;
int keep_empty = 0;
enum {
-   CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S
+   CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S,
+   CHECK_TODO_LIST
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -28,6 +29,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
N_("shorten SHA-1s in the todo list"), SHORTEN_SHA1S),
OPT_CMDMODE(0, "expand-sha1s", ,
N_("expand SHA-1s in the todo list"), EXPAND_SHA1S),
+   OPT_CMDMODE(0, "check-todo-list", ,
+   N_("check the todo list"), CHECK_TODO_LIST),
OPT_END()
};
 
@@ -50,5 +53,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
return !!transform_todo_ids(1);
if (command == EXPAND_SHA1S && argc == 1)
return !!transform_todo_ids(0);
+   if (command == CHECK_TODO_LIST && argc == 1)
+   return !!check_todo_list();
usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index f642ec2..02a7698 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -880,96 +880,6 @@ add_exec_commands () {
mv "$1.new" "$1"
 }
 
-# Check if the SHA-1 passed as an argument is a
-# correct one, if not then print $2 in "$todo".badsha
-# $1: the SHA-1 to test
-# $2: the line number of the input
-# $3: the input filename
-check_commit_sha () {
-   badsha=0
-   if test -z "$1"
-   then
-   badsha=1
-   else
-   sha1_verif="$(git rev-parse --verify --quiet $1^{commit})"
-   if test -z "$sha1_verif"
-   then
-   badsha=1
-   fi
-   fi
-
-   if test $badsha -ne 0
-   then
-   line="$(sed -n -e "${2}p" "$3")"
-   warn "$(eval_gettext "\
-Warning: the SHA-1 is missing or isn't a commit in the following line:
- - \$line")"
-   warn
-   fi
-
-   return $badsha
-}
-
-# prints the bad commits and bad commands
-# from the todolist in stdin
-check_bad_cmd_and_sha () {
-   retval=0
-   lineno=0
-   while read -r command rest
-   do
-   lineno=$(( $lineno + 1 ))
-   case $command in
-   "$comment_char"*|''|noop|x|exec)
-   # Doesn't expect a SHA-1
-   ;;
-   "$cr")
-   # Work around CR left by "read" (e.g. with Git for
-   # Windows' Bash).
-   ;;
-   pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
-   if ! check_commit_sha "${rest%%[]*}" "$lineno" 
"$1"
-   then
-   retval=1
-   fi
-   ;;
-   *)
-   line="$(sed -n -e "${lineno}p" "$1")"
-   warn "$(eval_gettext "\
-Warning: the command isn't recognized in the following line:
- - \$line")"
-   warn
-   retval=1
-   ;;
-   esac
-   done <"$1"
-   return $retval
-}
-
-# Print the list of the SHA-1 of the commits
-# from stdin to stdout
-todo_list_to_sha_list () {
-   git stripspace --strip-comments |
-   while read -r command sha1 rest
-   do
-   case $command in
-   "$comment_char"*|''|noop|x|"exec")
-   ;;
-   *)
-   long_sha=$(git rev-list --no-walk "$sha1" 2>/dev/null)
-   printf "%s\n" "$long_sha"
-   ;;
-   esac
-   done
-}
-
-# Use warn for each line in stdin
-warn_lines () {
-   while read -r line
-   do
-   warn " - $line"
-   done
-}
-
 # Switch to the branch in $into and notify it in the reflog
 

[PATCH 1/9] rebase -i: generate the script via rebase--helper

[Johannes Schindelin]

The first step of an interactive rebase is to generate the so-called "todo
script", to be stored in the state directory as "git-rebase-todo" and to
be edited by the user.

Originally, we adjusted the output of `git log ` using a simple
sed script. Over the course of the years, the code became more
complicated. We now use shell scripting to edit the output of `git log`
conditionally, depending whether to keep "empty" commits (i.e. commits
that do not change any files).

On platforms where shell scripting is not native, this can be a serious
drag. And it opens the door for incompatibilities between platforms when
it comes to shell scripting or to Unix-y commands.

Let's just re-implement the todo script generation in plain C, using the
revision machinery directly.

This is substantially faster, improving the speed relative to the
shell script version of the interactive rebase from 2x to 3x on Windows.

Note that the rearrange_squash() function in git-rebase--interactive
relied on the fact that we set the "format" variable to the config setting
rebase.instructionFormat. Relying on a side effect like this is no good,
hence we explicitly perform that assignment (possibly again) in
rearrange_squash().

Signed-off-by: Johannes Schindelin 
---
 builtin/rebase--helper.c   |  8 +++-
 git-rebase--interactive.sh | 44 +++-
 sequencer.c| 44 
 sequencer.h|  2 ++
 4 files changed, 76 insertions(+), 22 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index ca1ebb2..821058d 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -11,15 +11,19 @@ static const char * const builtin_rebase_helper_usage[] = {
 int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 {
struct replay_opts opts = REPLAY_OPTS_INIT;
+   int keep_empty = 0;
enum {
-   CONTINUE = 1, ABORT
+   CONTINUE = 1, ABORT, MAKE_SCRIPT
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
+   OPT_BOOL(0, "keep-empty", _empty, N_("keep empty 
commits")),
OPT_CMDMODE(0, "continue", , N_("continue rebase"),
CONTINUE),
OPT_CMDMODE(0, "abort", , N_("abort rebase"),
ABORT),
+   OPT_CMDMODE(0, "make-script", ,
+   N_("make rebase script"), MAKE_SCRIPT),
OPT_END()
};
 
@@ -36,5 +40,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
return !!sequencer_continue();
if (command == ABORT && argc == 1)
return !!sequencer_remove_state();
+   if (command == MAKE_SCRIPT && argc > 1)
+   return !!sequencer_make_script(keep_empty, stdout, argc, argv);
usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 022766b..01c9fec 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -775,6 +775,7 @@ collapse_todo_ids() {
 # each log message will be re-retrieved in order to normalize the
 # autosquash arrangement
 rearrange_squash () {
+   format=$(git config --get rebase.instructionFormat)
# extract fixup!/squash! lines and resolve any referenced sha1's
while read -r pick sha1 message
do
@@ -1203,26 +1204,27 @@ else
revisions=$onto...$orig_head
shortrevisions=$shorthead
 fi
-format=$(git config --get rebase.instructionFormat)
-# the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to 
parse
-git rev-list $merges_option --format="%m%H ${format:-%s}" \
-   --reverse --left-right --topo-order \
-   $revisions ${restrict_revision+^$restrict_revision} | \
-   sed -n "s/^>//p" |
-while read -r sha1 rest
-do
-
-   if test -z "$keep_empty" && is_empty_commit $sha1 && ! is_merge_commit 
$sha1
-   then
-   comment_out="$comment_char "
-   else
-   comment_out=
-   fi
+if test t != "$preserve_merges"
+then
+   git rebase--helper --make-script ${keep_empty:+--keep-empty} \
+   $revisions ${restrict_revision+^$restrict_revision} >"$todo"
+else
+   format=$(git config --get rebase.instructionFormat)
+   # the 'rev-list .. | sed' requires %m to parse; the instruction 
requires %H to parse
+   git rev-list $merges_option --format="%m%H ${format:-%s}" \
+   --reverse --left-right --topo-order \
+   $revisions ${restrict_revision+^$restrict_revision} | \
+   sed -n "s/^>//p" |
+   while read -r sha1 rest
+   do
+
+   if test -z "$keep_empty" && is_empty_commit $sha1 && ! 
is_merge_commit $sha1
+   then
+   

git add -p—splitting hunks, limit is too large

[Beau Martinez]

Hi git developers and community,

I'd like to inquire as to why `git add -p` can only split hunks so
much. The limit is too large; why can't you split until each hunk is
only a line? I often have to run `edit` and split them manually
myself.

I'd like to contribute a patch to change it, although my C is rusty.
Are there resources that will help me to do this?

Thank you for your time.

Beau

Re: Git in Outreachy December-March?

[Christian Couder]

On Fri, Sep 2, 2016 at 11:02 AM, Jeff King  wrote:
>
> I'm happy to act as admin. We will need a few things:
>
>   - to arrange funding for the stipend. GitHub offered to cover this
> last time, and if we are interested, I can see if this is still the
> case. We can also cover it out of the Git project money.
>
>   - mentor volunteers. This is similar in scope to GSoC, but I don't
> want to just assume that people who volunteered for GSoC would still
> be available

I would be happy to co-mentor as well as for GSoC.

>   - projects. We can pull from the ideas that were not selected for the
> 2016 GSoC, but we may need to update or add to it.

Yeah, maybe we could also update the micro-project page
(https://github.com/git/git.github.io/blob/master/SoC-2016-Microprojects.md).

Re: [PATCH 07/34] sequencer (rebase -i): add support for the 'fixup' and 'squash' commands

[Johannes Schindelin]

Hi Dennis,

On Fri, 2 Sep 2016, Dennis Kaarsemaker wrote:

> On vr, 2016-09-02 at 09:13 +0200, Johannes Schindelin wrote:
> 
> > As Git for Windows does not ship with translations (for multiple
> > reasons), it would not be a regression.
> 
> I'm confused, how does "git for windows does not ship with
> translations" translate to "this is not a regression"? Is this patch
> series only meant to be for git for windows and not go into git.git
> itself?

Oh, I thought I had clarified my plan... The timeline is:

- I submit the remaining rebase--helper patch series for review (last week
  and this one),

- I publish a preview of Git for Windows v2.10.0 that already uses these
  patches (done: 
https://github.com/git-for-windows/git/releases/tag/v2.9.3.windows.3)

- once upstream Git v2.10.0 is released (possibly today, after my work
  hours), I perform a final "Git garden shears" run (read: rebase Git for
  Windows' patches, retaining the branch structure) on top of v2.10.0 and
  release Git for Windows v2.10.0, tagged as v2.10.0.windows.1 in
  https://github.com/git-for-windows/git (due to time zone differences
  relative to Junio, the most likely time for this release would be
  some time around noon tomorrow, given that the release engineering takes
  roughly 2-4 hours, running unsupervised for the most part).

- as far as Git for Windows is concerned, l10n is not really an issue yet:
  the installer is released without any localizations.

- After releasing Git for Windows v2.10.0, I will pay a lot of attention
  to feedback. Not only to hear a lot of praise, but also to catch any
  possible regressions. Not that I expect anything dramatic to happen
  because I really tested this as thoroughly as I can: not a single one of
  my interactive rebases since mid May has been performed without
  involving the rebase--helper. In the three cases where I *did* find a
  regression, I solved it immediately, of course.

- After releasing Git for Windows v2.10.0, I will have a nice beer. Or
  three.

- Then I will leisurely try to address the l10n issues.

- Then, I will send out the current iterations of the patch series that
  are in flight.

- I have the entire week to address concerns with Git for Windows as well
  as with the patch series (where the former takes precedence, of course).

- The second half of September, I will relax from this marathon that
  started in early February. Meaning: I will be mostly offline.

I hope this clarifies why I am not so concerned about some issues such as
translation, or commit messages, or grammar, and more so about others,
such as incorrect code.

Ciao,
Dscho

Re: [PATCH 00/22] Prepare the sequencer for the upcoming rebase -i patches

[Johannes Schindelin]

Hi Kuba,

On Fri, 2 Sep 2016, Jakub Narębski wrote:

> W dniu 29.08.2016 o 10:03, Johannes Schindelin pisze:
> 
> > This patch series marks the  '4' in the countdown to speed up rebase -i
> > by implementing large parts in C. It is based on the `libify-sequencer`
> > patch series that I submitted last week.
> 
> Which of those got reviewed (and perhaps accepted), and which of those
> needs review still?  What is subject of their cover letter?

Most of the patch series I sent before last week got accepted. Only one
got rejected, IIRC, and replaced by a better solution (3727318 (Merge
branch 'jk/test-send-sh-x-trace-elsewhere', 2016-05-17)).

The patch series I submitted as part of my rebase--helper work that were
accepted:

b232439 (Merge branch 'js/t3404-typofix', 2016-05-17)
7b02771 (Merge branch 'js/perf-rebase-i', 2016-05-23)
3437017 (Merge branch 'js/perf-on-apple', 2016-07-06)
62e5e83 (Merge branch 'js/find-commit-subject-ignore-leading-blanks', 
2016-07-11)
c510926 (Merge branch 'js/sign-empty-commit-fix', 2016-07-13)
6c35952 (Merge branch 'js/t3404-grammo-fix', 2016-07-13)
63641fb (Merge branch 'js/log-to-diffopt-file', 2016-07-19)
3d55eea (Merge branch 'js/am-call-theirs-theirs-in-fallback-3way', 2016-07-19)
c97268c (Merge branch 'js/rebase-i-tests', 2016-07-28)
1a5f1a3 (Merge branch 'js/am-3-merge-recursive-direct', 2016-08-10)

You will note that I broke out a couple of patch series that do not
strictly have anything to do with the rebase--helper, such as
perf-on-apple. Nevertheless, they were part of a 99-strong patch series
that was my initial working rebase--helper, which I have used ever since
to perform all of my interactive rebases.

There are still a couple of patch series in flight. Let me list them by
the tags created by my mail-patch-series.sh script:

https://github.com/dscho/git/releases/tag/libify-sequencer-v2
https://github.com/dscho/git/releases/tag/require-clean-work-tree-v1
https://github.com/dscho/git/releases/tag/prepare-sequencer-v1
https://github.com/dscho/git/releases/tag/sequencer-i-v1
https://github.com/dscho/git/releases/tag/rebase--helper-v1

These tags all contain links to the cover letter as stored on
public-inbox.org, identified by the Message-ID.

Please note that the first four of this batch of five already saw
substantial work-after-review, thanks in part to your helpful comments.
You may appreciate the fact that a link of the form

https://github.com/dscho/git/compare/libify-sequencer-v2...libify-sequencer

shows you where I am at, although it cannot give you a real interdiff
because I rebased to a newer version of upstream/master in the meantime.

Finally, there is one last patch series that I did not yet submit: the
'rebase-i-extra' patch series. However, as I continuously update the
overall 'interactive-rebase' branch thicket (and have done so since the
very beginning of my work on the rebase--helper), it is relatively easy to
see what is left:

https://github.com/dscho/git/compare/rebase--helper...interactive-rebase

BTW thanks for making me dig out all of this information (it did take a
while to uncover it...), as I am so totally going to use that in a blog
post.

> > The reason to split these two patch series is simple: to keep them at a
> > sensible size.
> 
> That's good.

Thanks. I really try to be sensible with other people's time.

Even more so after being so offended by the talk at the most recent Git
Merge that stated that some people deliberately waste contributors' time
because they value their own time so much more. I am *really* offended by
that.

As a maintainer of Git for Windows, I do everything in my power to strike
a sensible balance between how much time I spend on improving the software
and how much time I ask others to do so.

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

The problem with --root is that it *creates* an initial commit. It is
empty, and will be amended. It would most likely not be a lot of work, but
I really wanted this work to be incremental, focusing on the most
important aspects first.

In fact, I do hope that somebody with the need for --root will take the
baton and run with it.

> > The end game of this patch series is a git-rebase--helper that makes
> > rebase -i 5x faster on Windows (according to t/perf/p3404). Travis
> > says that even MacOSX and Linux benefit (4x and 3x, respectively).
> 
> So do I understand correctly that end goal for *this* series is to move
> most of processing to git-rebase--helper, but full builtin-ification
> (and retiring git-rebase.sh to contrib/examples/) would have to wait for
> later?

Oh yes!

Retiring git-rebase.sh is *far, far, far* in the future. We really missed
the boat a 

Re: [GIT PULL] l10n updates for 2.10.0 round 2

[Jiang Xin]

Hi Junio,

Another update comes, please pull.

The following changes since commit 5b18e70009487bb156cac18546d6f91105338f4c:

  A few more fixes before the final 2.10 (2016-08-31 10:21:05 -0700)

are available in the git repository at:

  git://github.com/git-l10n/git-po tags/l10n-2.10.0-rnd2.2

for you to fetch changes up to e8e349249c86550d3505c4abfac28caf3d13df46:

  Merge branch 'master' of https://github.com/vnwildman/git
(2016-09-02 21:29:48 +0800)


l10n-2.10.0-rnd2.2


Jiang Xin (1):
  Merge branch 'master' of https://github.com/vnwildman/git

Trần Ngọc Quân (1):
  l10n: Updated Vietnamese translation for v2.10.0-rc2 (2757t)

 po/vi.po | 691 +--
 1 file changed, 317 insertions(+), 374 deletions(-)

2016-09-02 10:32 GMT+08:00 Junio C Hamano :
> Trần Ngọc Quân  writes:
>
>> On 31/08/2016 21:14, Jiang Xin wrote:
>>> Hi Junio,
>>>
>>> Would you please pull the following git l10n updates.
>> Please wait! Jiang Xin probably missing pull my one commit[1].
>>
>> [1]
>> 
>
> Jiang, I do not mind another update from you before the final.
>
> Thanks.

Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests

[Johannes Schindelin]

Hi Ævar,

On Fri, 2 Sep 2016, Ævar Arnfjörð Bjarmason wrote:

> On Wed, Aug 31, 2016 at 5:05 PM, Johannes Schindelin
>  wrote:
>
> > The biggest problem with Strawberry Perl is that it is virtually
> > impossible to build the Subversion-Perl bindings using the Git for
> > Windows SDK when using Strawberry Perl.
> >
> > Which pretty much precludes it from being used in Git for Windows.
> >
> > And then there are the path issues... Git's Perl scripts are pretty
> > certain that they live in a POSIX-y environment. Which MSYS2 Perl
> > provides. Strawberry Perl not.
> 
> This might be me missing the point, and I'm really just trying to be
> helpful here and make "prove" work for you because it's awesome, but
> as far as just you running this for development purposes does any of
> this SVN stuff matter? I.e. you can build Git itself not with
> Strawberry, but just use Strawberry to get a working copy of "prove".

Yes, the SVN stuff matters, because of the many t9*svn* tests (which, BTW
take a substantial time to run). So if I run the test suite, I better do
it with a perl.exe in the PATH that can run the SVN tests. Otherwise I
might just as well not bother with running the entire test suite...

Ciao,
Dscho

Re: [PATCH 07/34] sequencer (rebase -i): add support for the 'fixup' and 'squash' commands

[Dennis Kaarsemaker]

On vr, 2016-09-02 at 09:13 +0200, Johannes Schindelin wrote:

> As Git for Windows does not ship with translations (for multiple
> reasons), it would not be a regression.

I'm confused, how does "git for windows does not ship with
translations" translate to "this is not a regression"? Is this patch
series only meant to be for git for windows and not go into git.git
itself?

D.

Re: [PATCH 00/22] Prepare the sequencer for the upcoming rebase -i patches

[Jakub Narębski]

W dniu 29.08.2016 o 10:03, Johannes Schindelin pisze:

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

Which of those got reviewed (and perhaps accepted), and which of those
needs review still?  What is subject of their cover letter?

> 
> The patches in this series merely prepare the sequencer code for the
> next patch series that actually teaches the sequencer to run an
> interactive rebase.
> 
> The reason to split these two patch series is simple: to keep them at a
> sensible size.

That's good.

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

I can understand --preserve-merges, but what is the problem with --root?

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

So do I understand correctly that end goal for *this* series is to move
most of processing to git-rebase--helper, but full builtin-ification
(and retiring git-rebase.sh to contrib/examples/) would have to wait
for later?

[...]

I'd like here to summarize the discussion (my review, Dennis review,
Johannes Sixt and Junio comments).

If there are no comments, it means no problems or minor changes.

> Johannes Schindelin (22):
>   sequencer: use static initializers for replay_opts
There is no need for putting zeros in static initializer.  Commit
message expanded.

>   sequencer: use memoized sequencer directory path
>   sequencer: avoid unnecessary indirection
>   sequencer: future-proof remove_sequencer_state()
Leftover unrelated chunk removed.

>   sequencer: allow the sequencer to take custody of malloc()ed data
Is introducing new *_entrust() mechanism (which needs docs, at least
as comments) worth it, instead of just strdup everything and free?
If it is: naming of function parameter + example in commit message.

>   sequencer: release memory that was allocated when reading options
See above.

>   sequencer: future-proof read_populate_todo()
Possibly mention which functions were not future-proofed because
of planned for the subsequent patch full rewrite.

>   sequencer: remove overzealous assumption
Overzealous assumptions, or a worthy check?  Perhaps just remove check
for rebase -i in future commit, and keep test.  Perhaps remove test
temporarily.

>   sequencer: completely revamp the "todo" script parsing
This removes check; it should return if it was worthy.  Some discussion
about eager versus lazy parsing of commits, but IMHO it should be left
for later, if considered worth it.

>   sequencer: avoid completely different messages for different actions
Fix l10n or drop (and not introduce lego translation).

>   sequencer: get rid of the subcommand field
>   sequencer: refactor the code to obtain a short commit name
Explain reason behind this change in the commit mesage.

>   sequencer: remember the onelines when parsing the todo file
Lazy or eager again; "exec", "noop" and --preserve-merges.

>   sequencer: prepare for rebase -i's commit functionality
Add helper function, possibly extract helper function.  Rephrase block
comment.

"[PATCH] am: refactor read_author_script()" from Junio.

>   sequencer: introduce a helper to read files written by scripts
Perhaps add why not use open + strbuf_getline to commit message...

>   sequencer: prepare for rebase -i's GPG settings
Possibly fixes bug.  Use *_entrust() or strdup to not leak memory
(and to not crash when freeing memory).

>   sequencer: allow editing the commit message on a case-by-case basis
Enhance the commit message.

>   sequencer: support amending commits
>   sequencer: support cleaning up commit messages
>   sequencer: remember do_recursive_merge()'s return value
>   sequencer: left-trim the lines read from the script
>   sequencer: refactor write_message()
Enhance the commit message.  Quote path in messages while at it.


HTH

> 
>  builtin/commit.c|   2 +-
>  builtin/revert.c|  42 ++-
>  sequencer.c | 573 
> +++-
>  sequencer.h |  27 +-
>  t/t3510-cherry-pick-sequence.sh |  11 -
>  5 files changed, 428 insertions(+), 227 deletions(-)
> 
> Based-On: libify-sequencer at https://github.com/dscho/git
> Fetch-Base-Via: git fetch https://github.com/dscho/git libify-sequencer
> Published-As: https://github.com/dscho/git/releases/tag/prepare-sequencer-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git prepare-sequencer-v1

An unrelated question: Dscho, how are you generating above lines?

-- 
Jakub Narębski
 

Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests

[Ævar Arnfjörð Bjarmason]

On Wed, Aug 31, 2016 at 5:05 PM, Johannes Schindelin
 wrote:
> Hi Ævar,
>
> On Wed, 31 Aug 2016, Ævar Arnfjörð Bjarmason wrote:
>
>> I haven't used it myself (or any Windows thing) but people say good
>> things about http://strawberryperl.com
>
> Ah yes. This comes up frequently. Many a Git for Windows user pointed me
> into that direction.
>
> The biggest problem with Strawberry Perl is that it is virtually
> impossible to build the Subversion-Perl bindings using the Git for Windows
> SDK when using Strawberry Perl.
>
> Which pretty much precludes it from being used in Git for Windows.
>
> And then there are the path issues... Git's Perl scripts are pretty
> certain that they live in a POSIX-y environment. Which MSYS2 Perl
> provides. Strawberry Perl not.

This might be me missing the point, and I'm really just trying to be
helpful here and make "prove" work for you because it's awesome, but
as far as just you running this for development purposes does any of
this SVN stuff matter? I.e. you can build Git itself not with
Strawberry, but just use Strawberry to get a working copy of "prove".

Re: Git in Outreachy December-March?

[Pranit Bauva]

Probably off-topic.

On Fri, Sep 2, 2016 at 2:32 PM, Jeff King  wrote:
> As some of you may recall, we signed up to participate in Outreachy for
> the May-August session, but did not end up selecting an intern. The
> original thread with details is here:
>
>   http://public-inbox.org/git/20160308224625.ga29...@sigill.intra.peff.net/
>
> There's another session that runs from December to March. If we want to
> participate, we need to sign up in the next few days.
>
> I'm happy to act as admin. We will need a few things:
>
>   - to arrange funding for the stipend. GitHub offered to cover this
> last time, and if we are interested, I can see if this is still the
> case. We can also cover it out of the Git project money.
>
>   - mentor volunteers. This is similar in scope to GSoC, but I don't
> want to just assume that people who volunteered for GSoC would still
> be available
>
>   - projects. We can pull from the ideas that were not selected for the
> 2016 GSoC, but we may need to update or add to it.

I have a few friends who too did GSoC along with me but in different
orgs. Their orgs have a separate channel (slack or IRC) for
GSoC/Outreachy communications. In that channel is that all potential
mentors are added and students too are pointed to it. In that channel
the very basic doubts are covered. Let's say I am online and probably
the student's mentor is currently unavailable, so he/she can post a
trivial doubt there for someone to respond quickly. It takes off a
little load from the list and the mentor as well. I am aware that
there exists a channel named #git-devel but unfortunately its not
really active. I will be wiling to help other students with their
early days!

These days a lot of my fellow students don't really use IRC for
communication and see you can see there were really less number of
people inquiring  about GSoC in #git-devel. We can prefer slack or any
other alternative.

Regards,
Pranit Bauva

Re: bug: 'core.logallrefupdates' is not set by default in non-bare repository

[Jeff King]

On Fri, Sep 02, 2016 at 11:01:54AM +0200, Dennis Kaarsemaker wrote:

> Well, 'git init' is a valid operation to run inside an existing repo to
> reinitialize some bits, so we definitely need to not ignore the config
> once we're sure we're not creating a new repo.

Good point, I hadn't considered re-initializing.

For the follow-up patch I sent, where we check
startup_info->have_repository, I think the right thing would probably be
to call setup_git_directory() after seeing we are in a re-init case.
Probably even the "gently" form, as I think you can "re-init" a
partially corrupted repository.

> > > @@ -500,7 +506,6 @@ int cmd_init_db(int argc, const char **argv, const 
> > > char *prefix)
> > >    * and we know shared_repository should always 
> > > be 0;
> > >    * but just in case we play safe.
> > >    */
> > > - saved = get_shared_repository();
> > >   set_shared_repository(0);
> > >   switch 
> > > (safe_create_leading_directories_const(argv[0])) {
> > >   case SCLD_OK:
> > I don't know if anybody cares about being able to set core.sharedRepository
> > from ~/.gitconfig. It didn't work until v2.9.0 anyway (when I moved it
> > out of the repository-format check), but it seems like you _should_ be
> > able to set it and have it Just Work.
> > 
> > And in that case, this "we know shared_repository should always be 0" is
> > not true, and we would want to keep doing the save/set-to-0/restore
> > dance here.
> 
> We don't need to save if we throw away the cache below.

Yeah, you're right. Though I somehow lost my train of thought between
the two paragraphs. I was thinking that we would want to actually
respect the ~/.gitconfig setting for sharedrepository. Which would
actually mean _dropping_ the save/zero/restore entirely, and just using
the value we get from the config. But I guess the point here is to avoid
s_c_l_d creating "shared" leading directories that are outside any
repository. I could see an argument either way on whether that is the
right thing to do when core.sharedRepository is set in ~/.gitconfig.

-Peff

Git in Outreachy December-March?

[Jeff King]

As some of you may recall, we signed up to participate in Outreachy for
the May-August session, but did not end up selecting an intern. The
original thread with details is here:

  http://public-inbox.org/git/20160308224625.ga29...@sigill.intra.peff.net/

There's another session that runs from December to March. If we want to
participate, we need to sign up in the next few days.

I'm happy to act as admin. We will need a few things:

  - to arrange funding for the stipend. GitHub offered to cover this
last time, and if we are interested, I can see if this is still the
case. We can also cover it out of the Git project money.

  - mentor volunteers. This is similar in scope to GSoC, but I don't
want to just assume that people who volunteered for GSoC would still
be available

  - projects. We can pull from the ideas that were not selected for the
2016 GSoC, but we may need to update or add to it.

-Peff

Re: bug: 'core.logallrefupdates' is not set by default in non-bare repository

[Dennis Kaarsemaker]

On vr, 2016-09-02 at 04:04 -0400, Jeff King wrote:
> On Wed, Aug 31, 2016 at 05:32:33PM +0200, Dennis Kaarsemaker wrote:
> 
> > 
> > > 
> > > We may need to do something like turn off the
> > > need_shared_repository_from_config in init-db, since I think it would
> > > not want to ever read from the default config sources in most of its
> > > code-paths (OTOH, it should in theory respect core.sharedRepository
> > > in ~/.gitconfig, so maybe there is another more elegant way of
> > > handling this).
> > I would go even further and say that git init should completely ignore
> > the config of a repository you happen to be in when creating a new
> > repository.
> Hmm. I'd think we would already be avoiding that, because we shouldn't
> be calling setup_git_directory(). But some of the lazy-loaded setup is a
> bit overzealous, and we blindly look at ".git/config". If we try the
> same operation from a subdir of an existing repo, we _don't_ end up
> confused. Eek.

Yikes. Didnt' dig that deep, but that sounds wrong :)

> So I actually wonder if that is the root of the bug. In your patch, you
> disable config reading when we chdir to the new repo:
> 
> > 
> > diff --git a/builtin/init-db.c b/builtin/init-db.c
> > index 3a45f0b..d0fd3dc 100644
> > --- a/builtin/init-db.c
> > +++ b/builtin/init-db.c
> > @@ -493,6 +493,12 @@ int cmd_init_db(int argc, const char **argv, const 
> > char *prefix)
> >     int mkdir_tried = 0;
> >     retry:
> >     if (chdir(argv[0]) < 0) {
> > +   /*
> > +    * We're creating a new repository. If we're already in 
> > another
> > +    * repository, ignore its config
> > +    */
> > +   ignore_repo_config = 1;
> > +   git_config_clear();
> But I think we should go further and avoid ever looking at the original
> repository in the first place. I.e., I would say that "git init" should
> never ever behave differently if run in an existing repo versus outside
> of one.

Well, 'git init' is a valid operation to run inside an existing repo to
reinitialize some bits, so we definitely need to not ignore the config
once we're sure we're not creating a new repo.

> So really we ought to be setting ignore_repo_config from the very start
> of cmd_init(), and then re-enabling it once we are "inside" the new
> repo.  The git_config_clear() should in theory come once we are
> "inside", as well; we may have cached system/global config, and
> need to flush so we read them anew along with the new local config.

That's why I git_config_clear() twice.

> OTOH, since there shouldn't be anything interesting in the new
> repo-level config, I'm not sure that's really necessary. The rest of
> "init" can probably proceed without caring.

Except when running 'git init' to re-init existing repo.

> I also wonder if there are other things besides config which might
> accidentally read from .git (because they call git_pathdup(), and it
> just blindly looks in ".git" if nobody called setup_git_directory()). So
> it would be nice to have some flag for "do not ever lazy-call
> setup_git_env; we do not care about any repository".  But I think that's
> ahrd; functions like git_pathdup() are always expected to return _some_
> value, so what would they say? The best we could do is return
> "/does-not-exist/" or something, but that is awfully hacky.
> 
> > 
> > @@ -500,7 +506,6 @@ int cmd_init_db(int argc, const char **argv, const char 
> > *prefix)
> >      * and we know shared_repository should always 
> > be 0;
> >      * but just in case we play safe.
> >      */
> > -   saved = get_shared_repository();
> >     set_shared_repository(0);
> >     switch 
> > (safe_create_leading_directories_const(argv[0])) {
> >     case SCLD_OK:
> I don't know if anybody cares about being able to set core.sharedRepository
> from ~/.gitconfig. It didn't work until v2.9.0 anyway (when I moved it
> out of the repository-format check), but it seems like you _should_ be
> able to set it and have it Just Work.
> 
> And in that case, this "we know shared_repository should always be 0" is
> not true, and we would want to keep doing the save/set-to-0/restore
> dance here.

We don't need to save if we throw away the cache below.

> > @@ -524,6 +528,11 @@ int cmd_init_db(int argc, const char **argv, const 
> > char *prefix)
> >     } else if (0 < argc) {
> >     usage(init_db_usage[0]);
> >     }
> > +
> > +   need_shared_repository_from_config = 1;
> > +   ignore_repo_config = 0;
> > +   git_config_clear();
> This is the part I think we could actually skip. The only thing we might
> not have loaded is the "config" we just wrote to the new repository. But
> I don't think we have to care about what is in it.

We do, because this is also called for existing repos.

> 

Re: bug: 'core.logallrefupdates' is not set by default in non-bare repository

[Jeff King]

On Fri, Sep 02, 2016 at 04:04:16AM -0400, Jeff King wrote:

> So here's the minimal fix that seems to work for me:
> 
> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index 3a45f0b..56e7b9a 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c

I also wonder if "clone" should be doing something similar. Or, for that
matter, things like git-daemon that operate outside of a repo. They work
now because they do not happen to trigger any library calls which look
at config under the hood.

Traditionally these were supposed to just use git_config_early(), but
that's really not possible when the config calls are happening behind
the scenes (e.g., when lazy-loading the config cache). And so we
eventually got rid of git_config_early() entirely.

But I wonder if we could enforce that concept automatically for config.

The simple patch below does fix this case:

diff --git a/config.c b/config.c
index 0dfed68..b62bb40 100644
--- a/config.c
+++ b/config.c
@@ -1289,7 +1289,7 @@ static int do_git_config_sequence(config_fn_t fn, void 
*data)
int ret = 0;
char *xdg_config = xdg_config_home("config");
char *user_config = expand_user_path("~/.gitconfig");
-   char *repo_config = git_pathdup("config");
+   char *repo_config = startup_info->have_repository ? 
git_pathdup("config") : NULL;
 
current_parsing_scope = CONFIG_SCOPE_SYSTEM;
if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0))

but it causes a few test failures. Some of those are arguably
reasonable, though. E.g., several of the diff tests use "git diff
--no-index" and expect to read local config. But "--no-index" explicitly
_avoids_ setting up the git repository, so the current code just falls
back to reading ".git/config". Which means it works when you are at the
top-level of a repository, but not in a subdir!

So I think this patch is an improvement; if we have not set up the
repository, then we should not be reading its config! (It's another
question of whether --no-index should try setup_git_directory_gently(),
but then this patch would just do the right thing).

I think "hash-object" without "-w" is in the same boat. It does not even
bother looking for a git dir, but we assume that it can see config like
core.autocrlf. It works in the top-level, but not elsewhere:

  $ git init
  $ git config core.autocrlf true
  $ printf 'foo\r\n' >file
  $ git hash-object file
  257cc5642cb1a054f08cc83f2d943e56fd3ebe99
  $ mkdir subdir
  $ cd subdir
  $ git hash-object ../file
  e48b03ece74f47d1ae20075200c64aeaa01a9cdb

So there is definitely some cleanup work, but it seems like it would be
fixing a bunch of bugs.

Some of the other failures are not so obvious. In particular, t7006
tests the core.pager settings that are looked up before we set up the
git directory, and those are now broken. OTOH, I suspect that doing it
_correctly_ would fix all of the known breakages like:

  not ok 46 - git -p true - core.pager overrides PAGER from subdirectory

They are hitting that same subdirectory problem mentioned above.

-Peff

[PATCH 1/2] Add a builtin helper for interactive rebases

[Johannes Schindelin]

Git's interactive rebase is still implemented as a shell script, despite
its complexity. This implies that it suffers from the portability point
of view, from lack of expressibility, and of course also from
performance. The latter issue is particularly serious on Windows, where
we pay a hefty price for relying so much on POSIX.

Unfortunately, being such a huge shell script also means that we missed
the train when it would have been relatively easy to port it to C, and
instead piled feature upon feature onto that poor script that originally
never intended to be more than a slightly pimped cherry-pick in a loop.

To open the road toward better performance (in addition to all the other
benefits of C over shell scripts), let's just start *somewhere*.

The approach taken here is to add a builtin helper that at first intends
to take care of the parts of the interactive rebase that are most
affected by the performance penalties mentioned above.

In particular, after we spent all those efforts on preparing the sequencer
to process rebase -i's git-rebase-todo scripts, we implement the `git
rebase -i --continue` functionality as a new builtin, git-rebase--helper.

Once that is in place, we can work gradually on tackling the rest of the
technical debt.

Note that the rebase--helper needs to learn about the transient
--ff/--no-ff options of git-rebase, as the corresponding flag is not
persisted to, and re-read from, the state directory.

Signed-off-by: Johannes Schindelin 
---
 .gitignore   |  1 +
 Makefile |  1 +
 builtin.h|  1 +
 builtin/rebase--helper.c | 40 
 git.c|  1 +
 5 files changed, 44 insertions(+)
 create mode 100644 builtin/rebase--helper.c

diff --git a/.gitignore b/.gitignore
index 05cb58a..a9b8c96 100644
--- a/.gitignore
+++ b/.gitignore
@@ -114,6 +114,7 @@
 /git-read-tree
 /git-rebase
 /git-rebase--am
+/git-rebase--helper
 /git-rebase--interactive
 /git-rebase--merge
 /git-receive-pack
diff --git a/Makefile b/Makefile
index d96ecb7..980e1dc 100644
--- a/Makefile
+++ b/Makefile
@@ -919,6 +919,7 @@ BUILTIN_OBJS += builtin/prune.o
 BUILTIN_OBJS += builtin/pull.o
 BUILTIN_OBJS += builtin/push.o
 BUILTIN_OBJS += builtin/read-tree.o
+BUILTIN_OBJS += builtin/rebase--helper.o
 BUILTIN_OBJS += builtin/receive-pack.o
 BUILTIN_OBJS += builtin/reflog.o
 BUILTIN_OBJS += builtin/remote.o
diff --git a/builtin.h b/builtin.h
index 6b95006..2e5de14 100644
--- a/builtin.h
+++ b/builtin.h
@@ -102,6 +102,7 @@ extern int cmd_prune_packed(int argc, const char **argv, 
const char *prefix);
 extern int cmd_pull(int argc, const char **argv, const char *prefix);
 extern int cmd_push(int argc, const char **argv, const char *prefix);
 extern int cmd_read_tree(int argc, const char **argv, const char *prefix);
+extern int cmd_rebase__helper(int argc, const char **argv, const char *prefix);
 extern int cmd_receive_pack(int argc, const char **argv, const char *prefix);
 extern int cmd_reflog(int argc, const char **argv, const char *prefix);
 extern int cmd_remote(int argc, const char **argv, const char *prefix);
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
new file mode 100644
index 000..ca1ebb2
--- /dev/null
+++ b/builtin/rebase--helper.c
@@ -0,0 +1,40 @@
+#include "builtin.h"
+#include "cache.h"
+#include "parse-options.h"
+#include "sequencer.h"
+
+static const char * const builtin_rebase_helper_usage[] = {
+   N_("git rebase--helper []"),
+   NULL
+};
+
+int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
+{
+   struct replay_opts opts = REPLAY_OPTS_INIT;
+   enum {
+   CONTINUE = 1, ABORT
+   } command = 0;
+   struct option options[] = {
+   OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
+   OPT_CMDMODE(0, "continue", , N_("continue rebase"),
+   CONTINUE),
+   OPT_CMDMODE(0, "abort", , N_("abort rebase"),
+   ABORT),
+   OPT_END()
+   };
+
+   git_config(git_default_config, NULL);
+
+   opts.action = REPLAY_INTERACTIVE_REBASE;
+   opts.allow_ff = 1;
+   opts.allow_empty = 1;
+
+   argc = parse_options(argc, argv, NULL, options,
+   builtin_rebase_helper_usage, PARSE_OPT_KEEP_ARGV0);
+
+   if (command == CONTINUE && argc == 1)
+   return !!sequencer_continue();
+   if (command == ABORT && argc == 1)
+   return !!sequencer_remove_state();
+   usage_with_options(builtin_rebase_helper_usage, options);
+}
diff --git a/git.c b/git.c
index 0f1937f..26b4ad3 100644
--- a/git.c
+++ b/git.c
@@ -451,6 +451,7 @@ static struct cmd_struct commands[] = {
{ "pull", cmd_pull, RUN_SETUP | NEED_WORK_TREE },
{ "push", cmd_push, RUN_SETUP },
{ "read-tree", cmd_read_tree, RUN_SETUP },
+   { "rebase--helper", cmd_rebase__helper, 

[PATCH 2/2] rebase -i: use the rebase--helper builtin

[Johannes Schindelin]

Now that the sequencer learned to process a "normal" interactive rebase,
we use it. The original shell script is still used for "non-normal"
interactive rebases, i.e. when --root or --preserve-merges was passed.

Please note that the --root option (via the $squash_onto variable) needs
special handling only for the very first command, hence it is still okay
to use the helper upon continue/skip.

Also please note that the --no-ff setting is volatile, i.e. when the
interactive rebase is interrupted at any stage, there is no record of
it. Therefore, we have to pass it from the shell script to the
rebase--helper.

Signed-off-by: Johannes Schindelin 
---
 git-rebase--interactive.sh | 13 +
 1 file changed, 13 insertions(+)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 7e558b0..022766b 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -1059,6 +1059,10 @@ git_rebase__interactive () {
 
 case "$action" in
 continue)
+   if test ! -d "$rewritten"
+   then
+   exec git rebase--helper ${force_rebase:+--no-ff} --continue
+   fi
# do we have anything to commit?
if git diff-index --cached --quiet HEAD --
then
@@ -1118,6 +1122,10 @@ first and then run 'git rebase --continue' again.")"
 skip)
git rerere clear
 
+   if test ! -d "$rewritten"
+   then
+   exec git rebase--helper ${force_rebase:+--no-ff} --continue
+   fi
do_rest
return 0
;;
@@ -1307,6 +1315,11 @@ expand_todo_ids
 test -d "$rewritten" || test -n "$force_rebase" || skip_unnecessary_picks
 
 checkout_onto
+if test -z "$rebase_root" && test ! -d "$rewritten"
+then
+   require_clean_work_tree "rebase"
+   exec git rebase--helper ${force_rebase:+--no-ff} --continue
+fi
 do_rest
 
 }
-- 
2.9.3.windows.3

[PATCH 0/2] Let the sequencer handle the grunt work of rebase -i

[Johannes Schindelin]

After all of these patch series y'all had to review, this is finally the
one that switches things over.

Please note that it does not (yet) handle the `git rebase -i --root`
invocation; I tried to focus on the common case, and I rarely use --root
myself.

Please note also that --preserve-merges is *not* handled.

The way I designed --preserve-merges is totally stupid and idiotic and I
do not want to spend any further time on it. You cannot "pick" merges
and hope to be able to reorder commits, for example.

And please finally note that this pair of patches does not yet yield the
full speed improvement that I promised earlier. After these patches, the
time is dominated by pre- and post-processing the todo script, at least
on Windows, so there is another patch series that ports those bits and
pieces into the rebase--helper, too.


Johannes Schindelin (2):
  Add a builtin helper for interactive rebases
  rebase -i: use the rebase--helper builtin

 .gitignore |  1 +
 Makefile   |  1 +
 builtin.h  |  1 +
 builtin/rebase--helper.c   | 40 
 git-rebase--interactive.sh | 13 +
 git.c  |  1 +
 6 files changed, 57 insertions(+)
 create mode 100644 builtin/rebase--helper.c

Based-On: sequencer-i at https://github.com/dscho/git
Fetch-Base-Via: git fetch https://github.com/dscho/git sequencer-i
Published-As: https://github.com/dscho/git/releases/tag/rebase--helper-v1
Fetch-It-Via: git fetch https://github.com/dscho/git rebase--helper-v1

-- 
2.9.3.windows.3

base-commit: bbec81903b5e46c481fdc0cfe6f10166423526f1

Re: bug: 'core.logallrefupdates' is not set by default in non-bare repository

[Jeff King]

On Wed, Aug 31, 2016 at 05:32:33PM +0200, Dennis Kaarsemaker wrote:

> > We may need to do something like turn off the
> > need_shared_repository_from_config in init-db, since I think it would
> > not want to ever read from the default config sources in most of its
> > code-paths (OTOH, it should in theory respect core.sharedRepository
> > in ~/.gitconfig, so maybe there is another more elegant way of
> > handling this).
> 
> I would go even further and say that git init should completely ignore
> the config of a repository you happen to be in when creating a new
> repository.

Hmm. I'd think we would already be avoiding that, because we shouldn't
be calling setup_git_directory(). But some of the lazy-loaded setup is a
bit overzealous, and we blindly look at ".git/config". If we try the
same operation from a subdir of an existing repo, we _don't_ end up
confused. Eek.

So I actually wonder if that is the root of the bug. In your patch, you
disable config reading when we chdir to the new repo:

> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index 3a45f0b..d0fd3dc 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -493,6 +493,12 @@ int cmd_init_db(int argc, const char **argv, const char 
> *prefix)
>   int mkdir_tried = 0;
>   retry:
>   if (chdir(argv[0]) < 0) {
> + /*
> +  * We're creating a new repository. If we're already in 
> another
> +  * repository, ignore its config
> +  */
> + ignore_repo_config = 1;
> + git_config_clear();

But I think we should go further and avoid ever looking at the original
repository in the first place. I.e., I would say that "git init" should
never ever behave differently if run in an existing repo versus outside
of one.

So really we ought to be setting ignore_repo_config from the very start
of cmd_init(), and then re-enabling it once we are "inside" the new
repo.  The git_config_clear() should in theory come once we are
"inside", as well; we may have cached system/global config, and
need to flush so we read them anew along with the new local config.

OTOH, since there shouldn't be anything interesting in the new
repo-level config, I'm not sure that's really necessary. The rest of
"init" can probably proceed without caring.

I also wonder if there are other things besides config which might
accidentally read from .git (because they call git_pathdup(), and it
just blindly looks in ".git" if nobody called setup_git_directory()). So
it would be nice to have some flag for "do not ever lazy-call
setup_git_env; we do not care about any repository".  But I think that's
ahrd; functions like git_pathdup() are always expected to return _some_
value, so what would they say? The best we could do is return
"/does-not-exist/" or something, but that is awfully hacky.

> @@ -500,7 +506,6 @@ int cmd_init_db(int argc, const char **argv, const char 
> *prefix)
>    * and we know shared_repository should always 
> be 0;
>    * but just in case we play safe.
>    */
> - saved = get_shared_repository();
>   set_shared_repository(0);
>   switch 
> (safe_create_leading_directories_const(argv[0])) {
>   case SCLD_OK:

I don't know if anybody cares about being able to set core.sharedRepository
from ~/.gitconfig. It didn't work until v2.9.0 anyway (when I moved it
out of the repository-format check), but it seems like you _should_ be
able to set it and have it Just Work.

And in that case, this "we know shared_repository should always be 0" is
not true, and we would want to keep doing the save/set-to-0/restore
dance here.

> @@ -524,6 +528,11 @@ int cmd_init_db(int argc, const char **argv, const char 
> *prefix)
>   } else if (0 < argc) {
>   usage(init_db_usage[0]);
>   }
> +
> + need_shared_repository_from_config = 1;
> + ignore_repo_config = 0;
> + git_config_clear();

This is the part I think we could actually skip. The only thing we might
not have loaded is the "config" we just wrote to the new repository. But
I don't think we have to care about what is in it.

> diff --git a/config.c b/config.c
> index 0dfed68..2df0189 100644
> --- a/config.c
> +++ b/config.c
> @@ -1304,7 +1304,7 @@ static int do_git_config_sequence(config_fn_t fn, void 
> *data)
>   ret += git_config_from_file(fn, user_config, data);
>  
>   current_parsing_scope = CONFIG_SCOPE_REPO;
> - if (repo_config && !access_or_die(repo_config, R_OK, 0))
> + if (repo_config && !ignore_repo_config && !access_or_die(repo_config, 
> R_OK, 0))
>   ret += git_config_from_file(fn, repo_config, data);

We probably want to intercept the call to git_pathdup() earlier than
this, if the point is not to touch any of the 

Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests

[Johannes Schindelin]

Hi Junio,

On Thu, 1 Sep 2016, Junio C Hamano wrote:

> Hopefully that [patch removing the - suffix] would help making
> Dscho's "what are the failed tests?" logic simpler.

Of course.

It also makes sure that those 2 hours I spent on writing and perfecting
the sed magic were spent in vain... ;-)

Ciao,
Dscho

Re: [PATCH 27/34] sequencer (rebase -i): differentiate between comments and 'noop'

[Johannes Schindelin]

Hi Dennis,

On Thu, 1 Sep 2016, Dennis Kaarsemaker wrote:

> /*
>  * 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 {
>   /* All commands that handle commits */
>   TODO_PICK,
>   ...
>   /* All commands that do something else than pick */
>   TODO_EXEC,
>   ...
>   /* All commands that do nothing but are counted for reporting progress 
> */
>   TODO_NOOP,
>   ...
>   /* Comments, which are not counted
>   TODO_COMMENT
> }

I like it! Changed accordingly.

Thanks!
Dscho

Re: [PATCH 07/34] sequencer (rebase -i): add support for the 'fixup' and 'squash' commands

[Johannes Schindelin]

Hi Junio,

On Thu, 1 Sep 2016, Junio C Hamano wrote:

> For those who were not paying attention on the 'master' front during
> this pre-release period [*1*], I have to point out that the scripted
> Porcelain has been updated to lose the Anglo-centric st/nd/rd/th and
> this series would want to get updated to match.
> 
> 
> [Footnote]
> 
> *1* Why weren't you?  Repent! ;-)

I tried to. But, you know, I was kinda busy with a couple of patch series.

In any case, I changed the code this morning. Can't say that I like those
forced last-minute changes.

Ciao,
Johannes

Re: [PATCH 07/34] sequencer (rebase -i): add support for the 'fixup' and 'squash' commands

[Johannes Schindelin]

Hi Dennis,

On Thu, 1 Sep 2016, Dennis Kaarsemaker wrote:

> On do, 2016-09-01 at 17:17 +0200, Johannes Schindelin wrote:
>
> > And I see that the beautiful ordinal computation was given up in favor
> > of a lousy "#1", "#2", "#3", etc (it used to be "1st", "2nd", "3rd"
> > etc).
> > 
> > In any case, translation is not my main concern until v2.10.0, so I'll
> > take care of this after that release.
> 
> Hmm, not sure if I agree with that. I'd see it as a regression to lose
> the i18n there.

As Git for Windows does not ship with translations (for multiple reasons),
it would not be a regression.

Having said that, I see that having a different text than the current
rebase -i can be seen as a regression, so I changed that.

Thanks for the review!
Dscho

Re: bitmap creation failed

[Jeff King]

On Fri, Sep 02, 2016 at 12:04:54PM +0530, Arumuga wrote:

> So I understand now, the following.
> 
> 1. reducing the pack file size will increase the clone time
> 2. Single pack file is expected to better use bitmap feature.
> 
> Am i correct ?

Yes, on both.

-Peff