[PATCH 1 of 5] histedit: simplify computation of `newchildren` during --continue

[Pierre-Yves David]

On 10 oct. 2012, at 01:25, Augie Fackler wrote:

>> def bootstrapcontinue(ui, repo, parentctx, existing, replacemap, rules,
>>                      tmpnodes, created, replaced, opts):
>> -    currentparent, wantnull = repo.dirstate.parents()
>> -    # existing is the list of revisions initially considered by
>> -    # histedit. Here we use it to list new changesets, descendants
>> -    # of parentctx without an 'existing' changeset in-between. We
>> -    # also have to exclude 'existing' changesets which were
>> -    # previously dropped.
>> -    descendants = set(c.node() for c in
>> -            repo.set('(%d::) - %d', parentctx, parentctx))
>> -    notdropped = set(n for n in existing if n in descendants and
>> -            (n not in replacemap or replacemap[n] in descendants))
>> -    # Discover any nodes the user has added in the interim. We can
>> -    # miss changesets which were dropped and recreated the same.
>> -    newchildren = list(c.node() for c in repo.set(
>> -        'sort(%ln - (%ln or %ln::))', descendants, existing, notdropped))
>>    action, currentnode = rules.pop(0)
>> +    # is there any new commit between the expected parent and "."
>> +    # XXX does not take non linear new change in account (but previous
>> +    # XXX implementation didn't used them anyway
> 
> I'd still like this reworked into an abort since we're going to (arguably) be broken if the user does this. I'd rather be overly defensive than let someone start depending on weird broken behavior.

That won't happen from me this cycle. As said before fixing is is non trivial. I do not think we have the necessary data to detect them right now. So tackling this will requires:

- actually decide what the proper behavior isn
- changing written and read datan
- adding testcase,

Therefore I do not plan to work on this quicky:

- This is not a regression,
- this is not the purpose of this series,
- the freeze is near and I have more critical stuff to attend to,
- this is non trivial,
- this change have not direct relation with the current series

I agree that is a non-perfect, but practicality beat purity. The fact that the core of this series is getting stuck on this is a strong demotivation for me. I available to discuss this on IRC if needed.

> Actually, looking at the code further, it looks like you are considering that case? In that case, rework the comment thus:
> """We currently can't handle nonlinear new changes. This isn't a regression, but defend against it since it could screw up obsolete markers."""

I do not take them in account at all.

> (I normally use XXX as a marker for something that's really wrong and shouldn't be finalized as stated.)

I turned that into "note:"

> 
>> +    newchildren = [c.node() for c in repo.set('(%d::.)', parentctx)]
>> +    if not newchildren:
>> +        # `parentctxnode` should match but no result. This means that
>> +        # currentnode is not a descendant from parentctxnode.
>> +        msg = _('working directory parent is not a descendant of %s')
>> +        hint = _('update to proper revision and run "hg histedit '
>> +                 '--continue" again')
> 
> Please describe for the user what a "proper revision" is. "A descendant of %s" seems reasonable to me.

%s is actually a valid destination. "A descendant of %s" does not make is clean". I changed the message for:

  (update to 055a42cdd887 or descendant and run "hg histedit --continue" again)

Feel free to change it when queuing the series.

-- 
Pierre-Yves David