1 patch for repository http://darcs.net/screened:

patch 0f939713867ee7f22f842f1dc0de3e3fa4b7aff6
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Nov  3 19:46:46 CET 2020
  * refactor askAboutDepends and updatePatchHeader

  My immediate goal with this was to avoid making repository requests inside
  updatePatchHeader. Thus AskAboutDeps now contains a PatchSet instead of a
  Repository. But for the amend command the patchset we pass in is not the
  recorded patches but rather the recorded patches minus the selected patch.
  So we have to reconstruct this patchset. This is best done by not throwing
  away patches in the first place, but instead return unselected patches from
  filterNotInRemote as well as withSelectedPatchFromList. Incidentally this
  fixes a problem when we de-select the latest clean tag: before this patch
  amend would print all patches in the repo. This is now avoided by calling
  contextPatches when --not-in-remote is /not/ in effect.

Attachments

• patch-preview.txt
• refactor-askaboutdepends-and-updatepatchheader.dpatch

> My immediate goal with this was to avoid making repository
> requests inside updatePatchHeader. Thus AskAboutDeps now
> contains a PatchSet instead of a Repository. But for the
> amend command the patchset we pass in is not the recorded
> patches but rather the recorded patches minus the selected
> patch. So we have to reconstruct this patchset.

I'm having trouble understanding this motivation. Before,
askAboutDepends took a repository, read all the patches from it,
then filtered out the patch being worked out along with its implicit
dependencies.

After, it still does this filtering and at least filtering out the
implicit dependencies needs to be done somewhere.

So what do we gain from pre-filtering the PatchSet in AskAboutDeps
to remove the patch, instead of just consistently calculating it by
reading the repo? i.e. move the repo read operation out of
updatePatchHeader, but otherwise leave the logic unchanged.

Now that you put it this way I am having trouble understanding it 
myself. I'll have to play with this to see if there is some hidden 
catch that I failed to mention in the patch comment.

That said, even if it turns out (and I think it will) that simply 
taking the readPatches out of AskAboutDepends works correctly, there 
is still the issue of amend spamming you with all patches in the 
repo's history when you deselect the latest clean tag.

I think what happened here is that I mixed up two changes that were 
actually (semantically) independent and then tried to rationalize 
that when I wrote the patch comment.

> there is still the issue of amend spamming you with all
> patches in the repo's history when you deselect the latest
> clean tag.

Makes sense. Is this part of the behaviour worth a test?

Adding a test makes sense, will do. I would love to rebase this one 
into a sequence of separate patches, but there are already 20 or so 
in screened depending on it. Sorry for the mess!

I still think I'll follow up with a further refactor where 
askAboutDepends doesn't receive a PatchSet for the full context but 
rather a plain RL or FL. The idea being that we have to call 
contextPatches only once; actually, I now think this was my original 
intention when I conflated the two independent changes.

Following up on review. I haven't screened them yet, to be able to
amend if is something is still amiss.

The tests are in issue2293-laziness.sh because that happened to be the first
that failed when I tested if the new version works. The fix was to remove
the extra complication in amend (introduced by the problematic patch). It
then occurred to me that this is the perfect place to test that
--ask-depends does not offer, nor even touch, any patches not covered by the
the latest tag.

2 patches for repository http://darcs.net/screened:

patch a798c7b5ef6331922db9aa03190d76c64bfbe82a
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jun 26 14:35:47 CEST 2023
  * add tests for --ask-deps to issue2293-laziness.sh

  These additional tests ensure that we never try to offer any patches covered
  by the latest tag.

patch 5089f3fee53578439a99d8ccf78714036bfbcba6
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jun 26 17:37:36 CEST 2023
  * revisit askAboutDepends after review of 0f939713

  This again changes AskAboutDepends to contain only an RL of the candidates
  to select from. The extra complication in the amend command to re-construct
  a suitable PatchSet is now gone: we have the required patch sequence readily
  available, since it is the same as what remains after selecting the patch to
  amend. Also added haddocks for askAboutDepends, including a TODO item about
  a rather annoying limitation.

Attachments

• add-tests-for-__ask_deps-to-issue2293_laziness_sh.dpatch
• patch-preview.txt

This is really just one additional patch and it is not for screened yet. I
am sending it to illustrate what I meant in my previous message. The
remaining coercion, an unsafeCoerceU in the amend command, is needed because
commuting the rename through pending and working leaves us with a different
wU for the repository.

2 patches for repository http://darcs.net/screened:

patch 5089f3fee53578439a99d8ccf78714036bfbcba6
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jun 26 17:37:36 CEST 2023
  * revisit askAboutDepends after review of 0f939713

  This again changes AskAboutDepends to contain only an RL of the candidates
  to select from. The extra complication in the amend command to re-construct
  a suitable PatchSet is now gone: we have the required patch sequence readily
  available, since it is the same as what remains after selecting the patch to
  amend. Also added haddocks for askAboutDepends, including a TODO item about
  a rather annoying limitation.

patch f5f8d93cc49e104e93dbc900b6ce066c6418f7a1
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jun 27 16:36:41 CEST 2023
  * WIP properly track context changes when we rename a patch

Attachments

• patch-preview.txt
• revisit-askaboutdepends-after-review-of-0f939713.dpatch

I mixed up tickets again, sorry!

Please ignore my last message and the patch I sent. Will re-send my 
follow-up. I tried to remove my message with the "remove" button but 
that fails with "Invalid request" :-(

Following up on review as announced previously. Not yet screened.

1 patch for repository http://darcs.net/screened:

patch 5089f3fee53578439a99d8ccf78714036bfbcba6
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jun 26 17:37:36 CEST 2023
  * revisit askAboutDepends after review of 0f939713

  This again changes AskAboutDepends to contain only an RL of the candidates
  to select from. The extra complication in the amend command to re-construct
  a suitable PatchSet is now gone: we have the required patch sequence readily
  available, since it is the same as what remains after selecting the patch to
  amend. Also added haddocks for askAboutDepends, including a TODO item about
  a rather annoying limitation.

Attachments

• patch-preview.txt
• revisit-askaboutdepends-after-review-of-0f939713.dpatch

Sigh, I had already sent my follow-up, it's in file9510 "refactor-
askaboutdepends-and-updatepatchheader.dpatch". Please ignore 
everything I sent after msg23439.

>   * add tests for --ask-deps to issue2293-laziness.sh

This test looks fine.

> # note: the last 'y' here is for the hijack prompt

As an aside, I wonder why the test triggers hijack prompts, but not
enough to dig into it :-)

I've reviewed the combination of these two and it looks fine:

>   * refactor askAboutDepends and updatePatchHeader
>   * revisit askAboutDepends after review of 0f939713

> As an aside, I wonder why the test triggers hijack prompts

It's because the patches in the packed test repo have you as author 
and not "tester".