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

Augie Fackler raf at durin42.com
Tue Oct 9 23:13:23 CDT 2012


On Oct 9, 2012, at 10:44 PM, Pierre-Yves David <pierre-yves.david at ens-lyon.org> wrote:

> 
> 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:

This patch is _so_ big I'm having trouble keeping a single pass in my head, much less the entire conversation you and I have had about it.

> - 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.

This patch, is, frankly, so large if it was almost anyone else I'd be strongly inclined to reject it as unreviewably huge. Please be patient and understand that I have fairly little time for OSS work at the moment relative to the other demands on my time.

Your reasoning is good - this reasoning should (IMO) be mentioned in the commit message that you spotted an edge case, but did not have the time to fix it. That, or in the comment you added.

> 
>> 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:"

With this knowledge in mind, I'll look at the series again with the note. I'll try to remember between now and tomorrow when I look at things again.

> 
>> 
>>> +    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
> 



More information about the Mercurial-devel mailing list