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

[Junio C Hamano]

Stefan Beller  writes:

>> Speaking of what to and not to include in the upcoming release, we
>> do want to include Stefan's off-by-one fix to the submodule-helper,
>> but that is blocked on Windows end due to the test.
>
> I'd be happy either way, i.e. we could revert that fix and make a release?
> AFAICT, Windows only has broken tests, not broken functionality with that
> submodule bug fix.

If you are referring the "trailing /. should not make difference
when resolving ../relative/path" change with "rever that fix", I
think that may be a reasonable way to proceed.  Even though that
change is a bugfix (at least from the point of view by me and j6t in
the recent discussion), it is a behaviour change that we would want
to see feedback from existing submodule users and deserves a longer
gestation period.  And that part is not yet in 'next' yet ;-)

> If we want a longer gestation period, we'd ideally merge it to master
> just after a release, such that we "cook" it in master without having
> it in any release (we had a similar discussion for the diff heuristics IIRC).

Yes.  

It would mean that we would need a separate patch that adds the
!MINGW prerequisite to some tests to what is on 'next', as the early
patches on sb/submodule-ignore-trailing-slash~ that fixes off-by-one
is the right thing to do either way.  It of course needs help from
Windows folks to validate the results.

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

[Junio C Hamano]

Johannes Schindelin  writes:

>> I prefer to cook it in 'next' sufficiently long to ensure that we hear
>> feedbacks from non-Windows users if there is any unexpected breakage.
>
> FWIW I am using the same patches not only on Windows but also in my Linux
> VM.

Thanks for a datapoint, but when I said "non-Windows users", I was
not referring you as "the" Windows user.  I am expecting that you
would hear from Windows users who got exposure to your series by its
inclusion in Git for Windows.  They are the ones that I had in mind
as "Windows users"---and not hearing breakage reported by them would
be a good sign.

The primary reason why we want to cook a new topic in 'next' is to
expose it to people with different workflows using it on different
things, and that is especially more important for a change that
affects features that are flexible and can be used in different
ways---the set of options and commands used by the original author
of the series are often different from other people's.

Any change, when it hits 'next', is expected to be sufficiently
tested by the original author [*1*], but that is only true in the
context of the original author's daily use.  Both reviews and
author's tests are not sufficient to find bugs [*2*].

Topics that touch parts of the system that are more important to
users' daily Git life deserve extra time to find any unexpected
breakage in them.  Windows users are participating in that test by
inclusion of the topic in the released version of Git for Windows.
I want to see the the test for the rest of the world done by early
adopters who run 'next' (as 'pu' is too scary for daily use).


[Footnote]

*1* And me, as topics geting ready to be in 'next' are first merged
to my private edition branch that is slightly ahead of 'next' to
be used in my everyday use, but just like the original author is
merely one user, I am also merely one user with a specific set
of workflows that is different from others'.

*2* Bug finding is not the primary purpose of the review in the
first place.  It is to find design mistakes both at the external
and internal level, and bug finding "here you have off-by-one"
is merely a side effect.  End user tests may expose the former
(e.g. the design based on a wrong assumption may not accomodate
certain workflow the original author and the reviewers failed to
consider while writing and reviewing), but no amount of test
will uncover the latter (e.g. internal API that is misdesigned
will make future enhancement unnecessarily harder).

I think it was one of the achievements of the review cycle of
this particular series that we got rid of the _entrust() thing,
for example.  That had no visible external effect that would
have been caught by cooking on 'next' or releasing it to the
public, but was the kind of thing the code review was expected
to find and fix.

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

[Junio C Hamano]

Johannes Schindelin  writes:

>> I still do not understand (note that I am not saying "I do not
>> accept"--acceptance or rejection happens after an understandable
>> explanation is given, and "do not understand" means no such
>> explanation has been given yet) your justification behind adding a
>> technical debt to reimplement the author-script parser and not
>> sharing it with "git am" in 13/27.
>
> At this point, I am most of all reluctant to introduce such a huge change,
> which surely would introduce a regression.

That is a perfectly sensible and honest answer that I can
understand, accept and even support.

You've been working on the series for several months, running with
these dozen or so lines of code, and replacing it with another dozen
or so lines of code would require you to make sure the result is
actually doing the same thing for the remainder of your series.  And
I agree that is an unnecessary risk in order to ship a working code.
The code being battle tested counts.

I cared on this point mostly because I wanted to make sure that
people later can find out why there are two functions that ought to
be doing the same thing.  

If there were a technical reason why these two must stay to be
different implementations that are potentially doing different
things, I want to see that reason described, so that those who come
later and want to refactor the helper functions into one later will
know why they are separate in the first place.

If on the other hand there isn't any technical reason why they must
stay to be different, and they are different mostly because you
happened to have written a different one in 13/27 and have been
running with it, that is also a good thing for them to know.

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

[Stefan Beller]

On Sat, Oct 22, 2016 at 10:11 AM, Junio C Hamano  wrote:

>
> There isn't enough time to include this topic in the upcoming
> release within the current https://tinyurl.com/gitCal calendar,
> however, which places the final on Nov 11th.
>
> I am wondering if it makes sense to delay 2.11 by moving the final
> by 4 weeks to Dec 9th.
>
> Thoughts?
>
> Speaking of what to and not to include in the upcoming release, we
> do want to include Stefan's off-by-one fix to the submodule-helper,
> but that is blocked on Windows end due to the test.

I'd be happy either way, i.e. we could revert that fix and make a release?
AFAICT, Windows only has broken tests, not broken functionality with that
submodule bug fix.

> I think
> everybody agreed that a longer time "right thing to do" fix is to
> address the "when base is /path/to/dir/., where is ../sub relative
> to it?" issue, but if we are to do so, it would need a longer
> gestation period once it hits 'next', as it can affect the current
> users and we may even need B/C notes in the release notes for the
> change.  Giving ourselves a few more weeks of breathing room would
> help us to make sure the fix to relative URL issue is sound, too.

If we want a longer gestation period, we'd ideally merge it to master
just after a release, such that we "cook" it in master without having
it in any release (we had a similar discussion for the diff heuristics IIRC).

So please don't let the release schedule depend on my ability to deliver a
proper patch for the submodule path issue.

Thanks,
Stefan

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

[Johannes Schindelin]

Hi Max,

On Mon, 24 Oct 2016, Max Horn wrote:

> > On 23 Oct 2016, at 11:54, Johannes Schindelin  
> > wrote:
> > 
> > On Sat, 22 Oct 2016, Junio C Hamano wrote:
> > 
> [...]
> 
> >> There isn't enough time to include this topic in the upcoming release
> >> within the current https://tinyurl.com/gitCal calendar, however,
> >> which places the final on Nov 11th.
> > 
> > More is the pity.
> > 
> > Thank you, though, for being upfront with me. I will shift my focus to
> > tasks that require my attention more urgently, then.
> 
> Junio did go on, though:
> 
> >> I am wondering if it makes sense to delay 2.11 by moving the final
> >> by 4 weeks to Dec 9th.
> 
> I was reading this as an offer to delay things to accommodate the
> integration your work into 2.11. I.e. "within the current plan, there is
> no time for this, but we could adjust the plan". But maybe I am
> misinterpreting?

There is no indication that the rebase--helper patches would make it into
2.11 even with four more weeks.

I will now focus on other things that I postponed in favor of the
interactive rebase patches. In fact, I *have* to focus on some quite
pressing tasks that I neglected over those patches.

It's not like the process would magically improve just because a release
date is pushed. To the contrary, pushing the release date to allow for the
rebase--helper to be included may very well have the counterintuitive
effect of delaying things beyond even that pushed date "because there is
now so much time left" (until there isn't). It's a variation of
[Parkinson's Law](https://en.wikipedia.org/wiki/Parkinson%27s_law) ;-)

Anyway, back to work,
Dscho

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

[Max Horn]

Hi Dscho,

> On 23 Oct 2016, at 11:54, Johannes Schindelin  
> wrote:
> 
> Hi Junio,
> 
> On Sat, 22 Oct 2016, Junio C Hamano wrote:
> 
[...]

>> There isn't enough time to include this topic in the upcoming
>> release within the current https://tinyurl.com/gitCal calendar,
>> however, which places the final on Nov 11th.
> 
> More is the pity.
> 
> Thank you, though, for being upfront with me. I will shift my focus to
> tasks that require my attention more urgently, then.

Junio did go on, though:

>> I am wondering if it makes sense to delay 2.11 by moving the final
>> by 4 weeks to Dec 9th.

I was reading this as an offer to delay things to accommodate the integration 
your work into 2.11. I.e. "within the current plan, there is no time for this, 
but we could adjust the plan". But maybe I am misinterpreting?


Cheers,
Max

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

[Johannes Schindelin]

Hi Junio,

On Sun, 23 Oct 2016, Johannes Schindelin wrote:

> On Sat, 22 Oct 2016, Junio C Hamano wrote:
> 
> > Johannes Schindelin  writes:
> > 
> > > This patch series marks the '4' in the countdown to speed up rebase -i
> > > by implementing large parts in C (read: there will be three more patch
> > > series after that before the full benefit hits git.git: sequencer-i,
> > > rebase--helper and rebase-i-extra).
> > > ...
> > > It would be *really* nice if we could get this patch series at least
> > > into `next` soon, as it gets late and later for the rest of the
> > > patches to make it into `master` in time for v2.11 (and it is not for
> > > lack of trying on my end...).
> > 
> > This "countdown 4" step can affect cherry-pick and revert, even

Oh, I forgot to comment on this tidbit of your mail, sorry.

This *is* the countdown 4, as the remaining 3 patch series depend on each
other in the order I sent them out.

Ciao,
Dscho

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

[Johannes Schindelin]

Hi Junio,

On Sat, 22 Oct 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > This patch series marks the '4' in the countdown to speed up rebase -i
> > by implementing large parts in C (read: there will be three more patch
> > series after that before the full benefit hits git.git: sequencer-i,
> > rebase--helper and rebase-i-extra).
> > ...
> > It would be *really* nice if we could get this patch series at least
> > into `next` soon, as it gets late and later for the rest of the
> > patches to make it into `master` in time for v2.11 (and it is not for
> > lack of trying on my end...).
> 
> This "countdown 4" step can affect cherry-pick and revert, even
> though we were careful to review changes to the sequencer.c code.

As I pointed out in another mail in this thread: we should not fall into
the trap of overrating review.

In the case of the rebase--helper patches, so far the review mainly
resulted in more work for me (having to change spellings elsewhere, for
example), not in improving the changes I intended to introduce into
git.git's code.

Sure, there has been the occasional improvement, but it certainly feels as
if I spent about 80% of the work after each -v1 iteration on things that
have positively nothing at all to do with accelerating rebase -i.

> I prefer to cook it in 'next' sufficiently long to ensure that we hear
> feedbacks from non-Windows users if there is any unexpected breakage.

FWIW I am using the same patches not only on Windows but also in my Linux
VM.

> There isn't enough time to include this topic in the upcoming
> release within the current https://tinyurl.com/gitCal calendar,
> however, which places the final on Nov 11th.

More is the pity.

Thank you, though, for being upfront with me. I will shift my focus to
tasks that require my attention more urgently, then.

Ciao,
Dscho

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

[Johannes Schindelin]

Hi Junio,

On Fri, 21 Oct 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> I still do not understand (note that I am not saying "I do not
> accept"--acceptance or rejection happens after an understandable
> explanation is given, and "do not understand" means no such
> explanation has been given yet) your justification behind adding a
> technical debt to reimplement the author-script parser and not
> sharing it with "git am" in 13/27.

At this point, I am most of all reluctant to introduce such a huge change,
which surely would introduce a regression.

This is what happened a couple of times to me, most recently with the
hide-dot-gitdir patch series that worked flawlessly for years, had to be
dramatically changed during review to enter git.git, and introduced the
major regression that `core.hideDotFiles = gitDirOnly` was broken.

The lesson I learned: review should not be valued more than the test of
time. This lesson has been reinforced by all the regressions that have not
been caught by review nor the test suite running on Linux only.

It would be a different matter if I still had the cross-validator in place
(which I did when I sent out v1 of this patch series) and tons of time to
spend on accommodating your wishes, however I may disagree with them. And
in this instance, I thought I made clear that I disagree, and why:
Internally, git-am and git-rebase-i handle the author-script very
differently. That may change at some stage in the future, and it would be
a good time then and there to take care of unifying this code. Currently,
not so much, as the only excuse to use the same parser would be that they
both read the same file, while they have to do very different things with
the parsed output (in fact, your suggestion would ask the parser in the
sequencer to rip apart the information into key/value pairs, only to
re-glue them back together when they are used as the environment variables
as which rebase-i treats the contents of the author-script file).

So no, at this point I am not willing to risk introducing breakages in
code that has been proven to work in practice.

Ciao,
Dscho

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

[Junio C Hamano]

Johannes Schindelin  writes:

> This patch series marks the '4' in the countdown to speed up rebase -i
> by implementing large parts in C (read: there will be three more patch
> series after that before the full benefit hits git.git: sequencer-i,
> rebase--helper and rebase-i-extra).
> ...
> It would be *really* nice if we could get this patch series at least into
> `next` soon, as it gets late and later for the rest of the patches to make
> it into `master` in time for v2.11 (and it is not for lack of trying on my
> end...).

This "countdown 4" step can affect cherry-pick and revert, even
though we were careful to review changes to the sequencer.c code.  I
prefer to cook it in 'next' sufficiently long to ensure that we hear
feedbacks from non-Windows users if there is any unexpected breakage.

There isn't enough time to include this topic in the upcoming
release within the current https://tinyurl.com/gitCal calendar,
however, which places the final on Nov 11th.

I am wondering if it makes sense to delay 2.11 by moving the final
by 4 weeks to Dec 9th.

Thoughts?

Speaking of what to and not to include in the upcoming release, we
do want to include Stefan's off-by-one fix to the submodule-helper,
but that is blocked on Windows end due to the test.  I think
everybody agreed that a longer time "right thing to do" fix is to
address the "when base is /path/to/dir/., where is ../sub relative
to it?" issue, but if we are to do so, it would need a longer
gestation period once it hits 'next', as it can affect the current
users and we may even need B/C notes in the release notes for the
change.  Giving ourselves a few more weeks of breathing room would
help us to make sure the fix to relative URL issue is sound, too.

As to "countdown 3" and below steps, I am guessing that some of them
can start cooking in 'next' before 2.11, but even with lengthened
schedule, it is likely that they need to cook there beyond the end
of this cycle, unless they are truly trivial changes that do not
even need any reviews.

Thanks.

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

[Junio C Hamano]

Johannes Schindelin  writes:

> Changes vs v4:

I still do not understand (note that I am not saying "I do not
accept"--acceptance or rejection happens after an understandable
explanation is given, and "do not understand" means no such
explanation has been given yet) your justification behind adding a
technical debt to reimplement the author-script parser and not
sharing it with "git am" in 13/27.

As I pointed out, I think 22/27 is out of place in this series.

But other than these two points, the changes since v4 look minimum,
and they all looked good to me.

Thanks.

[PATCH v5 00/27] Prepare the sequencer for the upcoming rebase -i patches

[Johannes Schindelin]

This patch series marks the '4' in the countdown to speed up rebase -i
by implementing large parts in C (read: there will be three more patch
series after that before the full benefit hits git.git: sequencer-i,
rebase--helper and rebase-i-extra). It is based on the `libify-sequencer`
patch series I submitted earlier.

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

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

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

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

I have been working on this since early February, whenever time allowed,
and it is time to put it into the users' hands. To that end, I already
integrated the whole shebang into Git for Windows 2.10.0 and 2.10.1
where it has been running without complaints (and some quite positive
feedback).

It would be *really* nice if we could get this patch series at least into
`next` soon, as it gets late and later for the rest of the patches to make
it into `master` in time for v2.11 (and it is not for lack of trying on my
end...).

Changes vs v4:

- clarified in a code comment that read_oneliner() is lenient when it
  comes to multi-line files: it still reads the entire file, but strips
  off only the final EOL (if any).

- rephrased a commit message to stop judging Junio's taste ;-)

- changed "strip LF" to "skip LF", as requested.

- rephrased a commit message to talk about pluggin memory leaks instead
  of stating that the memory is now eventually released.

- dropped the "sequencer: do not try to commit when there were merge
  conflicts" patch that appears to be no longer necessary.

- split and modified the commit refactoring write_message() according to
  Junio's suggestions.


Johannes Schindelin (27):
  sequencer: use static initializers for replay_opts
  sequencer: use memoized sequencer directory path
  sequencer: avoid unnecessary indirection
  sequencer: future-proof remove_sequencer_state()
  sequencer: plug memory leaks for the option values
  sequencer: future-proof read_populate_todo()
  sequencer: refactor the code to obtain a short commit name
  sequencer: completely revamp the "todo" script parsing
  sequencer: strip CR from the todo script
  sequencer: avoid completely different messages for different actions
  sequencer: get rid of the subcommand field
  sequencer: remember the onelines when parsing the todo file
  sequencer: prepare for rebase -i's commit functionality
  sequencer: introduce a helper to read files written by scripts
  sequencer: allow editing the commit message on a case-by-case basis
  sequencer: support amending commits
  sequencer: support cleaning up commit messages
  sequencer: left-trim lines read from the script
  sequencer: stop releasing the strbuf in write_message()
  sequencer: roll back lock file if write_message() failed
  sequencer: refactor write_message() to take a pointer/length
  sequencer: teach write_message() to append an optional LF
  sequencer: remove overzealous assumption in rebase -i mode
  sequencer: mark action_name() for translation
  sequencer: quote filenames in error messages
  sequencer: start error messages consistently with lower case
  sequencer: mark all error messages for translation

 builtin/commit.c  |   2 +-
 builtin/revert.c  |  46 ++-
 sequencer.c   | 680 --
 sequencer.h   |  23 +-
 t/t3501-revert-cherry-pick.sh |   2 +-
 5 files changed, 492 insertions(+), 261 deletions(-)


base-commit: 659889482ac63411daea38b2c3d127842ea04e4d
Published-As: https://github.com/dscho/git/releases/tag/prepare-sequencer-v5
Fetch-It-Via: git fetch https://github.com/dscho/git prepare-sequencer-v5

Interdiff vs v4:

 diff --git a/sequencer.c b/sequencer.c
 index 1cf70f7..a61fe76 100644
 --- a/sequencer.c
 +++ b/sequencer.c
 @@ -234,8 +234,8 @@ static void print_advice(int show_hint, struct replay_opts 
*opts)
}
  }
  
 -static int write_with_lock_file(const char *filename,
 -  const void *buf, size_t len, int append_eol)
 +static int write_message(const void *buf, size_t len, const char *filename,
 +   int append_eol)
  {
static struct lock_file msg_file;
  
 @@ -258,16 +258,12 @@ static int write_with_lock_file(const char *filename,
return 0;
  }
  
 -static int write_message(struct strbuf *msgbuf, const char *filename)
 -{
 -  int res = write_with_lock_file(filename,