Re: [PATCH v6 00/10] The final building block for a faster rebase -i

[Johannes Schindelin]

Hi Junio,

On Thu, 20 Jul 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > Changes since v5:
> >
> > - replaced a get_sha1() call by a get_oid() call already.
> >
> > - adjusted to hashmap API changes
> 
> Applying this to the tip of 'master' yields exactly the same result
> as merging the previous round js/rebase-i-final to the tip of
> 'master' and then applying merge-fix/js/rebase-i-final to adjust to
> the codebase, so the net effect of this reroll is none.  Which is a
> good sign, as it means there wasn't any rebase mistake and the evil
> merge we've been carrying was a good one.

Good.

> But at the same time, I prefer to avoid rebasing to newer 'master'
> until the codebase starts drifting too far apart, or until a new
> feature release is made out of newer 'master'.  This is primarily
> because I want dates on commits to mean something---namely, "this
> change hasn't seen a need to be updated for 'oops, that was wrong'
> since this date".  This use of commit dates as 'priority date'
> matters much less for a topic not in 'next', but as a general
> principle, my workflow tries to preserve commit dates for all
> topics.

By that token, commit message updates would also be inappropriate, in
particular when they came from somebody else than the patch author ;-P

As to avoiding a rebase: we can add that to the growing list of things on
which we disagree.

If the author dates really meant anything, we would also have to avoid v2,
v3, v4, ... v226 of patch series. So that flies in the face of trying to
keep the meaning of author dates.

In addition, the development flow I prefer is one that is in harmony with
the modern Continuous Integration style, where topic branches are merged
into a single, always-ready-to-release integration branch.

That means that I always work off of `master`, unless there is a good
reason to base off of `next` or even `pu`. That's to avoid merge
conflicts, to see what really gets applied.

I am *especially* adamant about rebasing to a newer upstream commit when
there are merge conflicts.

Such as is the case here.

> For the above reason, I may hold onto this patch series in my inbox
> without actually updating js/rebase-i-final topic until the current
> cycle is over; please do not mistake it as this new reroll being
> ignored.

You do as you want, of course. But please note that I will not rebase my
topic branches to an ancient revision, especially if that would cause merge
conflicts with the current `master`.

And if there should be another iteration of this wallflower patch series,
I will rebase it to the then-current `master` again [*1*].

Ciao,
Dscho

Footnote *1*: in general, I try to abide by the wishes of maintainers when
contributing code, unless those wishes are contrary to what I consider
correct software development. Like, when in Rome, I will do as the Romans
do. Except when I see them looting a parking meter.

Re: [PATCH v6 00/10] The final building block for a faster rebase -i

[Junio C Hamano]

Johannes Schindelin  writes:

> Changes since v5:
>
> - replaced a get_sha1() call by a get_oid() call already.
>
> - adjusted to hashmap API changes

Applying this to the tip of 'master' yields exactly the same result
as merging the previous round js/rebase-i-final to the tip of
'master' and then applying merge-fix/js/rebase-i-final to adjust to
the codebase, so the net effect of this reroll is none.  Which is a
good sign, as it means there wasn't any rebase mistake and the evil
merge we've been carrying was a good one.

But at the same time, I prefer to avoid rebasing to newer 'master'
until the codebase starts drifting too far apart, or until a new
feature release is made out of newer 'master'.  This is primarily
because I want dates on commits to mean something---namely, "this
change hasn't seen a need to be updated for 'oops, that was wrong'
since this date".  This use of commit dates as 'priority date'
matters much less for a topic not in 'next', but as a general
principle, my workflow tries to preserve commit dates for all
topics.

For the above reason, I may hold onto this patch series in my inbox
without actually updating js/rebase-i-final topic until the current
cycle is over; please do not mistake it as this new reroll being
ignored.

Thanks.

Re: [PATCH v6 00/10] The final building block for a faster rebase -i

[Johannes Schindelin]

Hi Stefan,

On Fri, 14 Jul 2017, Stefan Beller wrote:

> On Fri, Jul 14, 2017 at 7:44 AM, Johannes Schindelin
>  wrote:
>
> >  -static int subject2item_cmp(const struct subject2item_entry *a,
> >  -  const struct subject2item_entry *b, const void *key)
> >  +static int subject2item_cmp(const void *fndata,
> 
> This could also be named unused_fndata.

Sure. I simply took the version Junio used when he merged the previous
patch series iteration into `pu`.

The function is short enough, though, that I feel it obvious that the
parameter is unused.

Ciao,
Dscho

Re: [PATCH v6 00/10] The final building block for a faster rebase -i

[Stefan Beller]

On Fri, Jul 14, 2017 at 7:44 AM, Johannes Schindelin
 wrote:
> 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 so far.
>
> I am still unwilling to replace a compile-time safe way to pass the
> options to the revision machinery by the alternative (which I am still
> flabbergasted about) proposed by Junio. This will not change.

There is new input for this discussion, though. :)

https://public-inbox.org/git/20170706202739.6056-1-sbel...@google.com/
was sent out to gauge interest if we want to pull through with removing
all the submodule hack, such as 'add_submodule_odb' in submodule.c
which just adds the submodule objects as an alternate object store, such
that we can do some things in-process (check if a submodule has certain
commits, merge_submodule).

For one of these ('merge_submodule') I would have to add the
'struct repository' as another parameter to pass around to the
diff and revision walking machinery. But the API for these subsystems
is traditionally only operated via an array of strings instead of passing
assigning a member field to a value.

So If I were to follow the "use arrays of strings only to operate the
revision machinery" I would:
* pass a string indicating which repo to use
  (probably the path to git dir?)
* the repository objects are cached, so we can lookup e.g.
  "the_repository" via the correct string.
* use that repo object inside the revision machinery.

That sounds cumbersome and I would prefer to pass in
the repository object directory. So maybe we want to have some
other way opposed to "an array of strings" to operate the revision
machinery.

>  -static int subject2item_cmp(const struct subject2item_entry *a,
>  -  const struct subject2item_entry *b, const void *key)
>  +static int subject2item_cmp(const void *fndata,

This could also be named unused_fndata.
Please see origin/sb/hashmap-cleanup, if that makes sense as well
(have all arguments const void and cast them inside the function, such
that we can avoid the cast to hashmap_cmp_fn in hashmap_init)

Thanks,
Stefan

[PATCH v6 00/10] 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 so far.

I am still unwilling to replace a compile-time safe way to pass the
options to the revision machinery by the alternative (which I am still
flabbergasted about) proposed by Junio. This will not change.

I know that we want to concentrate on bug fixes on `master`, but this
patch series will most likely take a couple of months to get there, anyway.
So I may just as well send this iteration now. It's also not like I haven't
contributed any bug fixes lately...

Changes since v5:

- replaced a get_sha1() call by a get_oid() call already.

- adjusted to hashmap API changes


Johannes Schindelin (10):
  t3415: verify that an empty instructionFormat is handled as before
  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

 Documentation/git-rebase.txt  |  16 +-
 builtin/rebase--helper.c  |  29 ++-
 git-rebase--interactive.sh| 373 -
 sequencer.c   | 531 ++
 sequencer.h   |   8 +
 t/t3404-rebase-interactive.sh |  22 +-
 t/t3415-rebase-autosquash.sh  |  28 ++-
 7 files changed, 647 insertions(+), 360 deletions(-)


base-commit: f3da2b79be9565779e4f76dc5812c68e156afdf0
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-v6
Fetch-It-Via: git fetch https://github.com/dscho/git rebase-i-extra-v6

Interdiff vs v5:
 diff --git a/sequencer.c b/sequencer.c
 index 8713cc8d1d5..c54596f9699 100644
 --- a/sequencer.c
 +++ b/sequencer.c
 @@ -2654,7 +2654,7 @@ int skip_unnecessary_picks(void)
  
if (!read_oneliner(, rebase_path_onto(), 0))
return error(_("could not read 'onto'"));
 -  if (get_sha1(buf.buf, onto_oid.hash)) {
 +  if (get_oid(buf.buf, _oid)) {
strbuf_release();
return error(_("need a HEAD to fixup"));
}
 @@ -2756,8 +2756,9 @@ struct subject2item_entry {
char subject[FLEX_ARRAY];
  };
  
 -static int subject2item_cmp(const struct subject2item_entry *a,
 -  const struct subject2item_entry *b, const void *key)
 +static int subject2item_cmp(const void *fndata,
 +  const struct subject2item_entry *a,
 +  const struct subject2item_entry *b, const void *key)
  {
return key ? strcmp(a->subject, key) : strcmp(a->subject, b->subject);
  }
 @@ -2802,7 +2803,7 @@ int rearrange_squash(void)
 * be moved to appear after the i'th.
 */
hashmap_init(, (hashmap_cmp_fn) subject2item_cmp,
 -   todo_list.nr);
 +   NULL, todo_list.nr);
ALLOC_ARRAY(next, todo_list.nr);
ALLOC_ARRAY(tail, todo_list.nr);
ALLOC_ARRAY(subjects, todo_list.nr);
-- 
2.13.3.windows.1.13.gaf0c2223da0