[Commented On] D10893: amend: add a useless initial version of `amend -r REV `

[martinvonz (Martin von Zweigbergk)]

martinvonz added a comment.


  In D10893#166668 <https://phab.mercurial-scm.org/D10893#166668>, @Alphare wrote:
  
  > In D10893#166666 <https://phab.mercurial-scm.org/D10893#166666>, @martinvonz wrote:
  >
  >> Thanks for the quick review. I sent this out early to get feedback on the idea -- particularly on the fact that we'll have `hg amend --continue` and `hg amend --abort` -- before I wasted time going too far in the wrong direction.
  >
  > Now that I think of it I'm not entirely certain what the intermediate step for amend will be used for. Will it just be in case of rebase conflicts or is there more to it?
  
  Steps:
  
  1. Create temporary commit. Once we support `hg amend -r target -i`, we'll need to create *two* temporary commits -- one to be rebased and folded in to the target, and one for the leftover changes to later be uncommitted and left in the working copy.
  2. Rebase the temporary commit on top of the target
  3. Fold the temporary commit into the target (using `rebase --collapse`)
  4. Rebase descendants on top of the new folded commit
  5. Update to the new tip commit
  6. Uncommit the temporary working copy if there was one
  
  Conflicts can happen in step 2 or step 4.
  
  >> Yes, exactly, I had intended for it to auto-rebase.
  >
  > I meant to clarify that it would not auto-rebase more than the stack (like other heads that are descendants of the amended changeset).
  
  I see. Yes, that's what I meant. It would rebase the commits between the target and the working copy parent. It would not rebase other descendants of the target. It would also not rebase descendants of the working copy parent.
  
  >> Sure, I can add a `--no-rebase` flag too. When would you want that?
  >
  > This would be for small adjustments that have no real impact on what I'm currently working on, I can just throw the amends one after the other. It's pretty niche, but not hard to implement so I thought I'd ask.
  
  I'm not sure I follow. Let's say you have this history:
  
    @ C
    |
    o B
    |
    o A
  
  You then make some change and run `hg amend -r B --no-rebase`. You get this:
  
    @ C
    |
    x B
    |
    | B'
    |/
    o A
  
  Presumably the working copy is now clean again? However, that means that you can't make changes that build on top of the previous amend.
  
  You then make some further changes in the working copy. What do you do now? If you do `hg amend -r B`, you would presumably get divergence. Should we allow `hg amend -r B'`? I had been thinking that we restrict `hg amend -r` to only amend into an ancestor just to keep it simple and because that seems like what people want in 99% of cases.
  
  >> By the way, I suppose we may want a `hg amend --stop` as well, but I probably won't implement that any time soon.
  >
  > That would be a good idea, maybe we could discuss why it's hard to implement it (if it's the case), but that shouldn't be a blocker for this patch.
  
  It might be easy and I might do it if it seems easy.
  
  >>> - Maybe plug into `hg continue/abort` and suggest using it first, then `hg amend --continue/abort`?
  
  Sorry, didn't see this comment earlier. That's what the patch currently does :) Well, there's no support for continuing, but the test case uses `hg abort` and there's no `--abort` flag yet. (I just noticed that the `hg status -v` still refers to `hg amend --continue/--abort`. I'll try to figure out how to make it not do that.)
  
  >>> As a nit, I'd say the `.t` test should be using `(known-bad-output !)` and/or `(missing-correct-output !)` to better signal what is expected than the TODO.
  >>
  >> Ah, good point. Those are new enough that I haven't gotten used to using them yet. Will update the patch.
  >
  > Thanks for updating the patch. I'm going to let a few days go by just to let people give their thoughts as this is a pretty impactful feature, but this looks good to me overall.
  
  Makes sense. I'll work on making the command actually do something useful in the meantime :)

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D10893/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D10893

To: martinvonz, #hg-reviewers
Cc: Alphare, mercurial-patches
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-patches/attachments/20210622/dd99dd00/attachment-0002.html>