[FR] Support multiple repositories associated with the same ML

[FR] Support multiple repositories associated with the same ML

[Ihor Radchenko]

Hi,

I am currently using piem to work with Org mode ML and quite enjoying
it. However, I just ran into an issue related to the fact that Org ML is
used to manage multiple repositories.

Org mode ML is an official mailing list for Org mode itself,
org-contrib, worg, and main Org website repositories. We accept patches
for all these projects in the same ML. Running piem-am when I configure
piem-inboxes to point to the same ML for different repos only considers
the first matching repo and does not query me where to apply the patch.

Could it be solved?

Best,
Ihor

Re: [FR] Support multiple repositories associated with the same ML

[Kyle Meyer]

Hi Ihor,

Ihor Radchenko writes:

> Hi,
>
> I am currently using piem to work with Org mode ML and quite enjoying
> it.

I'm glad to hear that (and thank you for all of the Org work you've been
doing).

> However, I just ran into an issue related to the fact that Org ML is
> used to manage multiple repositories.
>
> Org mode ML is an official mailing list for Org mode itself,
> org-contrib, worg, and main Org website repositories. We accept patches
> for all these projects in the same ML. Running piem-am when I configure
> piem-inboxes to point to the same ML for different repos only considers
> the first matching repo and does not query me where to apply the patch.

Right, I'd like piem to cover this use case.  Here's a relevant to-do
comment in piem.el:

  ;; TODO: Decide how to deal with inboxes that map to more than one
  ;; coderepo.  This is important to support for people that want to
  ;; use a catchall inbox for small projects which they don't think
  ;; (yet) need a dedicated address.
  (defcustom piem-inboxes nil

I'll plan to take a look at this soon (within the next week or two).

Applying the same patch multiple times (was: [FR] Support multiple repositories associated with the same ML)

[Ihor Radchenko]

Kyle Meyer <kyle@kyleam.com> writes:

>   ;; TODO: Decide how to deal with inboxes that map to more than one
>   ;; coderepo.  This is important to support for people that want to
>   ;; use a catchall inbox for small projects which they don't think
>   ;; (yet) need a dedicated address.
>   (defcustom piem-inboxes nil
>
> I'll plan to take a look at this soon (within the next week or two).

Thanks!

I also noticed one minor annoyance when I return to work on the
previously applied patch.

My starting point is the patch-containing email. Then, I run piem-am and
use the default automatically generated branch.

Expected behaviour: Silently switch to the already existing branch
unless that branch had modifications to the patch in the email. In the
latter case, either throw an error or show the changes interactively.

Observed behaviour: Because the branch already exists and the patch is
already applied, git am throws an error.

I think a more useful approach would be something like:
1. piem creates a throwaway branch and apply the patch in the email
2. piem rebases the existing branch onto that throwaway branch

That way, I can start working on a patch from email, possibly add some
followup commits and still be able to return to this WIP branch starting
from the patch email.

Best,
Ihor

Re: Applying the same patch multiple times

[Kyle Meyer]

Ihor Radchenko writes:

> I also noticed one minor annoyance when I return to work on the
> previously applied patch.
>
> My starting point is the patch-containing email. Then, I run piem-am and
> use the default automatically generated branch.

Interesting, using the email as the staring point for jumping to a
branch isn't a use case that I've considered, but, yeah, I can see why
that's appealing.

> Expected behaviour: Silently switch to the already existing branch
> unless that branch had modifications to the patch in the email. In the
> latter case, either throw an error or show the changes interactively.

Hmm, this makes a strong assumption about the patch to branch name
mapping, but the branch name offered in the prompt is just a (hopefully
helpful) suggestion/starting point.

To implement this feature, I'm not seeing a way around needing to
maintain the mapping.  The first thing that comes to mind is that the
Git configuration for the branch could be a good place to keep it (say
branch.$branch.piem-mid).  That would allow generating the mid->branch
mapping for a coderepo and has the nice feature that it's cleaned up by
Git when the branch is deleted.  There are other wrinkles here, but I
think the overall approach would probably be workable.

> Observed behaviour: Because the branch already exists and the patch is
> already applied, git am throws an error.

Odd.  I expected the git-checkout call upstream of git-am to fail
(saying something like "fatal: a branch named 'blah' already exists").
Testing quickly, that's what I see on my end.

(Either way, that's of course not your desired behavior.)

> I think a more useful approach would be something like:
> 1. piem creates a throwaway branch and apply the patch in the email
> 2. piem rebases the existing branch onto that throwaway branch

I'm not quite connecting this (1)...

> That way, I can start working on a patch from email, possibly add some
> followup commits and still be able to return to this WIP branch starting
> from the patch email.

...to this (2).  2 more or less aligns with how I understood the rest of
your email, but I don't see where 1 comes in.  If you already applied a
patch from a particular message, that patch isn't changing, so wouldn't
you just want to jump to the associated branch, even if you amended the
given commit and/or added commits on top?

Perhaps the answer is that these are updated patches from later
messages.  I'd say those ought to land on a separate branch, though I
know that unfortunately some people aren't good about bumping the reroll
count of their patches, in which case the suggested branch name won't be
different.

Thanks for your feedback.

Re: Applying the same patch multiple times

[Ihor Radchenko]

Kyle Meyer <kyle@kyleam.com> writes:

> Ihor Radchenko writes:
>> My starting point is the patch-containing email. Then, I run piem-am and
>> use the default automatically generated branch.
>
> Interesting, using the email as the staring point for jumping to a
> branch isn't a use case that I've considered, but, yeah, I can see why
> that's appealing.

Strictly speaking, I do not start from email. Rather I capture an Org
heading from email and assign a TODO keyword. The heading is linked back
to the email. Hence, when I start working on the patch, I jump from the
heading to linked email and then to the working branch.

>> Observed behaviour: Because the branch already exists and the patch is
>> already applied, git am throws an error.
>
> Odd.  I expected the git-checkout call upstream of git-am to fail
> (saying something like "fatal: a branch named 'blah' already exists").
> Testing quickly, that's what I see on my end.
> (Either way, that's of course not your desired behavior.)

To clarify, by error I meant "branch already exists". So, it is the same
on my end.

>> I think a more useful approach would be something like:
>> 1. piem creates a throwaway branch and apply the patch in the email
>> 2. piem rebases the existing branch onto that throwaway branch
>
> I'm not quite connecting this (1)...
>
>> That way, I can start working on a patch from email, possibly add some
>> followup commits and still be able to return to this WIP branch starting
>> from the patch email.
>
> ...to this (2).  2 more or less aligns with how I understood the rest of
> your email, but I don't see where 1 comes in.  If you already applied a
> patch from a particular message, that patch isn't changing, so wouldn't
> you just want to jump to the associated branch, even if you amended the
> given commit and/or added commits on top?

Let me clarify what I had in mind.

Consider that I start working on a patch at some point. I apply it to a
new branch using piem-am. Then, I happen to make changes to the patch
and run out of time to work. The changes are left on the git branch.

Days or weeks later I return to my work on the patch. Again, the
starting point is the patch email, not the git branch. I do not exactly
recall whether I did any changes to the patch. I again run piem-am. I
wish that piem-am reminds me about the old WIP state of the patch.

One way to remind me about my local amendments is highlighting that
there is a difference between the original patch in the email and the
working branch. This difference can be shown e.g. by rebase conflict.
This is where my (1) and (2) are coming from.

Similar approach might be used to work with updated versions of the
patch. Updates are rarely done with reroll count + interdiff.
Highligting the difference between previous and current patches in the
thread would be helpful. Though this second idea is probably more
complicated compared to the above.

Best,
Ihor

[PATCH] piem-inboxes: Support mapping inbox to multiple coderepos

[Kyle Meyer]

An inbox may regularly receive patches for multiple repos.  Allow an
inbox's :coderepo to be a list, and teach piem-inbox-coderepo to
select a target repo by prompting the caller with the configured list.
---
 Documentation/piem.texi |  5 ++++-
 TODO                    |  4 ----
 piem.el                 | 46 +++++++++++++++++++++++++++--------------
 tests/piem-tests.el     | 29 ++++++++++++++++++++++++++
 4 files changed, 64 insertions(+), 20 deletions(-)

diff --git a/Documentation/piem.texi b/Documentation/piem.texi
index fc9f795b..26962a4b 100644
--- a/Documentation/piem.texi
+++ b/Documentation/piem.texi
@@ -163,7 +163,10 @@ Registering inboxes
 that contains code related to that archive (in the example above, a
 local clone of @url{https://git.kernel.org/pub/scm/git/git.git/}).  This
 information is required to apply patches from an archive to a local code
-repository (@pxref{Applying patches}).
+repository (@pxref{Applying patches}).  To map an inbox to more than one
+repository (e.g., if the inbox receives patches for multiple projects,
+or if you use a few dedicated Git worktrees for different types of
+patches), set @code{:coderepo} to a list of locations.
 
 @findex piem-merged-inboxes
 @vindex piem-get-inboxes-from-config
diff --git a/TODO b/TODO
index 97d2d7f6..e6a90ea6 100644
--- a/TODO
+++ b/TODO
@@ -2,10 +2,6 @@ A (sparse and likely stale) to-do list for piem                   -*-outline-*-
 
 * general
 
-** piem-inboxes: support mapping inbox to multiple coderepos
-A catchall inbox may be used for small projects without a dedicated
-address.
-
 ** "report inbox" command for debugging
 
 ** "show message ID at point" command
diff --git a/piem.el b/piem.el
index 22d82120..f91e3e64 100644
--- a/piem.el
+++ b/piem.el
@@ -58,10 +58,6 @@ (defgroup piem ()
   :link '(info-link "(piem)Top")
   :group 'tools)
 
-;; TODO: Decide how to deal with inboxes that map to more than one
-;; coderepo.  This is important to support for people that want to
-;; use a catchall inbox for small projects which they don't think
-;; (yet) need a dedicated address.
 (defcustom piem-inboxes nil
   "List of public-inbox-archived projects.
 
@@ -77,6 +73,12 @@ (defcustom piem-inboxes nil
       differs from public-inbox-config's coderepo, which is a link to
       another section that must point to the repository's git
       directory.
+
+      To map an inbox to multiple repositories, you can set this
+      to a list of strings.  When a function (e.g., `piem-am')
+      needs a single coderepo, you will be prompted to select a
+      repository, with the first value in this list offered as
+      the default.
   :url
       A URL hosting HTTPS archives.  This value must end with a slash.
   :maildir
@@ -100,8 +102,7 @@ (defcustom piem-inboxes nil
   ;; `piem--merge-config-inboxes' so that the value can also be set in
   ;; ~/.public-inbox/config.
   :type '(alist :key-type string
-                :value-type
-                (plist :value-type string))
+                :value-type plist)
   :set (lambda (var val)
          (set var val)
          (when (fboundp 'piem-clear-merged-inboxes)
@@ -398,16 +399,23 @@ (defun piem--merge-config-inboxes ()
              (let* ((inbox-name (match-string 1 key))
                     (inbox-item (assoc inbox-name cfg-inboxes))
                     (prop-name (intern (concat ":" (match-string 2 key))))
-                    (prop-pair (list prop-name (car val))))
-               (when-let ((coderepo
+                    (prop-pair (list prop-name
+                                     (if (eq prop-name :coderepo)
+                                         val
+                                       (car val)))))
+               (when-let ((coderepos
                            (and (eq prop-name :coderepo)
-                                (car (gethash
-                                      (format "coderepo.%s.dir" (car val))
-                                      pi-cfg)))))
+                                (mapcar
+                                 (lambda (v)
+                                   (car (gethash (format "coderepo.%s.dir" v)
+                                                 pi-cfg)))
+                                 val))))
                  (setq prop-pair
                        (list :coderepo
-                             (replace-regexp-in-string
-                              "/\\.git/?\\'" "" coderepo))))
+                             (mapcar
+                              (lambda (r)
+                                (replace-regexp-in-string "/\\.git/?\\'" "" r))
+                              coderepos))))
                (if inbox-item
                    (setcdr inbox-item (nconc prop-pair (cdr inbox-item)))
                  (push (cons inbox-name prop-pair) cfg-inboxes)))))
@@ -516,8 +524,16 @@ (defun piem-inbox-get (key &optional inbox)
 
 (defun piem-inbox-coderepo (&optional inbox)
   "Return the code repository of current buffer's inbox."
-  (when-let ((repo (piem-inbox-get :coderepo inbox)))
-    (file-name-as-directory (expand-file-name repo))))
+  (when-let ((inbox (or inbox (piem-inbox)))
+             (repos (piem-inbox-get :coderepo inbox)))
+    (when (stringp repos)
+      (setq repos (list repos)))
+    (let ((repo (if (= (length repos) 1)
+                    (car repos)
+                  (completing-read "Code repository: " repos
+                                   nil t nil nil (car repos)))))
+      (unless (string-blank-p repo)
+        (file-name-as-directory (expand-file-name repo))))))
 
 (defun piem-inbox-maildir-directory (&optional inbox)
   "Return the maildir for INBOX.
diff --git a/tests/piem-tests.el b/tests/piem-tests.el
index 444ed5f7..79d8591e 100644
--- a/tests/piem-tests.el
+++ b/tests/piem-tests.el
@@ -49,6 +49,21 @@ (defvar piem-tests-sample-pi-config "
           dir = /code/foo/.git
 ")
 
+(defvar piem-tests-sample-pi-config-multiple-coderepos "
+[publicinbox \"foo\"]
+        address = foo@example.com
+        url = https://example.com/foo
+        inboxdir = /inboxes/foo
+        coderepo = foo.git
+        coderepo = bar.git
+
+[coderepo \"foo.git\"]
+          dir = /code/foo/.git
+
+[coderepo \"bar.git\"]
+          dir = /code/bar/.git
+")
+
 (ert-deftest piem-merged-inboxes:from-config-disabled ()
   (let ((piem-get-inboxes-from-config nil)
         (piem-inboxes nil))
@@ -78,6 +93,20 @@ (ert-deftest piem-merged-inboxes:from-config ()
       (piem-clear-merged-inboxes)
       (should-not (piem-inbox-get :address "foo")))))
 
+(ert-deftest piem-merged-inboxes:from-config-multiple-coderepos ()
+  (piem-clear-merged-inboxes)
+  (let ((piem-get-inboxes-from-config t)
+        (piem-inboxes nil))
+    (piem-tests-with-pi-config piem-tests-sample-pi-config-multiple-coderepos
+      (cl-letf (((symbol-function 'completing-read)
+                 (lambda (&rest _) "/code/foo")))
+        (should (equal (piem-inbox-coderepo "foo")
+                       "/code/foo/")))
+      (cl-letf (((symbol-function 'completing-read)
+                 (lambda (&rest _) "/code/bar")))
+        (should (equal (piem-inbox-coderepo "foo")
+                       "/code/bar/"))))))
+
 (ert-deftest piem-merged-inboxes:override-config ()
   (piem-clear-merged-inboxes)
   (let ((piem-get-inboxes-from-config t)

base-commit: 6f196480a040c0f57bb0a11ecae97e5b427a9d0f
-- 
2.35.1

Re: [PATCH] piem-inboxes: Support mapping inbox to multiple coderepos

[Ihor Radchenko]

Kyle Meyer <kyle@kyleam.com> writes:

> An inbox may regularly receive patches for multiple repos.  Allow an
> inbox's :coderepo to be a list, and teach piem-inbox-coderepo to
> select a target repo by prompting the caller with the configured list.

Thanks!

> @@ -100,8 +102,7 @@ (defcustom piem-inboxes nil
>    ;; `piem--merge-config-inboxes' so that the value can also be set in
>    ;; ~/.public-inbox/config.
>    :type '(alist :key-type string
> -                :value-type
> -                (plist :value-type string))
> +                :value-type plist)

:value-type plist seems wrong, unless I miss something. Even a list of
strings will not necessarily match this value type when there are e.g. 3
repos associated with an inbox.

> -               (when-let ((coderepo
> +               (when-let ((coderepos
>                             (and (eq prop-name :coderepo)
> -                                (car (gethash
> -                                      (format "coderepo.%s.dir" (car val))
> -                                      pi-cfg)))))
> +                                (mapcar
> +                                 (lambda (v)
> +                                   (car (gethash (format "coderepo.%s.dir" v)
> +                                                 pi-cfg)))
> +                                 val))))

Does it mean that :coderepo "a single string" format is no longer supported?

Best,
Ihor

Re: [PATCH] piem-inboxes: Support mapping inbox to multiple coderepos

[Kyle Meyer]

Ihor Radchenko writes:

> Kyle Meyer <kyle@kyleam.com> writes:
[...]
>> @@ -100,8 +102,7 @@ (defcustom piem-inboxes nil
>>    ;; `piem--merge-config-inboxes' so that the value can also be set in
>>    ;; ~/.public-inbox/config.
>>    :type '(alist :key-type string
>> -                :value-type
>> -                (plist :value-type string))
>> +                :value-type plist)
>
> :value-type plist seems wrong, unless I miss something. Even a list of
> strings will not necessarily match this value type when there are e.g. 3
> repos associated with an inbox.

Hmm, aren't you mapping at the wrong level?  This type is supposed to
match an alist like this:

  (("git"
    :url "https://lore.kernel.org/git/"
    :address "git@@vger.kernel.org"
    :listid "git.vger.kernel.org"
    :coderepo "~/src/git/")
   ...)

And it looks to me like it does:

  (alist :key-type string               => "git"
         :value-type plist)             => (:url ...)

Before I was specifying that the plist values should be strings, but,
given that :coderepo can now be a list of strings, this patch loosens
the plist type to its default value type (sexp).

>> -               (when-let ((coderepo
>> +               (when-let ((coderepos
>>                             (and (eq prop-name :coderepo)
>> -                                (car (gethash
>> -                                      (format "coderepo.%s.dir" (car val))
>> -                                      pi-cfg)))))
>> +                                (mapcar
>> +                                 (lambda (v)
>> +                                   (car (gethash (format "coderepo.%s.dir" v)
>> +                                                 pi-cfg)))
>> +                                 val))))
>
> Does it mean that :coderepo "a single string" format is no longer supported?

A string value is still supported.  This mapcar is working with the
values returned by piem--git-config-list, and that returns all values as
a list of strings.  (Notice in the pre-image above its `(car val)`.)

Re: [PATCH] piem-inboxes: Support mapping inbox to multiple coderepos

[Ihor Radchenko]

Kyle Meyer <kyle@kyleam.com> writes:

>> :value-type plist seems wrong, unless I miss something. Even a list of
>> strings will not necessarily match this value type when there are e.g. 3
>> repos associated with an inbox.
>
> Hmm, aren't you mapping at the wrong level?  This type is supposed to
> match an alist like this:

>> Does it mean that :coderepo "a single string" format is no longer supported?
>
> A string value is still supported.  This mapcar is working with the
> values returned by piem--git-config-list, and that returns all values as
> a list of strings.  (Notice in the pre-image above its `(car val)`.)

Thanks for the clarifications! You are indeed right.

Best,
Ihor

Re: [PATCH] piem-inboxes: Support mapping inbox to multiple coderepos

[Kyle Meyer]

Ihor Radchenko writes:

> Thanks for the clarifications! You are indeed right.

Thanks for confirming.  Pushed (2bf9c383).

Re: Applying the same patch multiple times

[Kyle Meyer]

Ihor Radchenko writes:

> Let me clarify what I had in mind.
> [...]

Thanks for the additional details.  I'll have to think more about this,
but I've added an entry to the TODO file.