On 23-01-27 03:18PM, Stefan Sperling wrote:
> On Fri, Jan 27, 2023 at 10:50:26PM +1100, Mark Jamsek wrote:
> > New diff below with your improvements to the documentation plus regress.
> 
> Thanks, ok stsp
> 
> We can proceed on this in-tree. There are some minor things I've
> noticed during review and testing but it will be easier to keep
> working on top of your current diff.

Ok, great! Thanks :)

This has been committed; I'll get the commit coverage tests done soon.

There's just one item I need to address, however, because I completely
flubbed it when I responded to your feedback here and I want to bring it
to your attention:

>> Reading this, I am wondering if we should tweak the behaviour regarding
>> removal after commit: Would it be better to keep a ref around if there
>> is no overlap between the cherrypicked commit's list of changed paths
>> and the newly created commit's list of changed paths?
>
>Yes, I agree, and we already do this! Great minds think alike :)
>
>I described that part of the implementation wrong by missing a word:
>(s/references/matched references).

The code was doing this...at some point, just not by the time I sent the
diff!

The four lines of code in lookup_logmsg_ref() that add refs to the queue
for removal have now been moved into the 'add_logmsg' block just
a couple lines up so that now we really do only add _matched_ refs to
the queue for removal :)

diff /home/mark/src/got
commit - f75f2784458c2318b073e238d937e06d1b4459e5
path + /home/mark/src/got
blob - 891db152d602c780bf3bde6fc8ea118567686019
file + got/got.c
--- got/got.c
+++ got/got.c
@@ -8944,13 +8944,13 @@ lookup_logmsg_ref(char **logmsg, struct got_reflist_he
                                goto done;
                        if (!added_logmsg)
                                ++added_logmsg;
+
+                       err = got_reflist_entry_dup(&re_match, re);
+                       if (err)
+                               goto done;
+                       TAILQ_INSERT_HEAD(matched_refs, re_match, entry);
                }

-               err = got_reflist_entry_dup(&re_match, re);
-               if (err)
-                       goto done;
-               TAILQ_INSERT_HEAD(matched_refs, re_match, entry);
-
                got_object_commit_close(commit);
                commit = NULL;
                free(id);

I really made a mess of that and should've confirmed what I thought
I remembered rather than trusting my memory before replying.  And
I really hope I didn't make you question your own sanity like I just did
after running some final pre-commit tests!

The committed code should indeed now do that which you asked and
I mistakenly said it did regarding the above quoted exchange :)

And this aspect of logmsg refs will be covered in the commit and revert
coverage test too.

-- 
Mark Jamsek <fnc.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68