You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Sahlberg <da...@gmail.com> on 2021/02/05 09:46:47 UTC
r1880192
Hi,
When going through the code for 1.14.1, I looked at r1880192. Is it
intentional to have both comments?
When I'm reading the comment and trying to understand the code I'm half
expecting to have two different (possibly nested) loops.
/* Iterate over each path with explicit mergeinfo added by the merge. */
+ /* Iterate over the paths in a parent-to-child order so that inherited
+ * mergeinfo is propagated consistently from each parent path to its
+ * children. (Issue #4862) */
It would make it easier to understand (at least for me) if it was a single
comment. (Unless I'm missing something in how the code is supposed to work
in which case I'm be happy to learn.)
Index: merge.c
===================================================================
--- merge.c (revision 1885615)
+++ merge.c (working copy)
@@ -7921,10 +7921,9 @@
if (!merge_b->paths_with_new_mergeinfo || merge_b->dry_run)
return SVN_NO_ERROR;
- /* Iterate over each path with explicit mergeinfo added by the merge. */
- /* Iterate over the paths in a parent-to-child order so that inherited
- * mergeinfo is propagated consistently from each parent path to its
- * children. (Issue #4862) */
+ /* Iterate over the paths with explicit mergeinfo added by the merge
+ * in a parent-to-child order so that inherited mergeinfo is propagated
+ * consistently from each parent path to its children. (Issue #4862) */
a = svn_sort__hash(merge_b->paths_with_new_mergeinfo,
svn_sort_compare_items_as_paths, pool);
iterpool = svn_pool_create(pool);
(Even though I stumbled on this up while reviewing 1.14.1, it is not
something that should be backported).
Kind regards,
Daniel
Re: r1880192
Posted by Julian Foad <ju...@apache.org>.
Daniel Sahlberg wrote:
> Should I have added Approved by: in the log message?
For such a trivial change? Nah.
- Julian
Re: r1880192
Posted by Daniel Sahlberg <da...@gmail.com>.
Den fre 5 feb. 2021 kl 11:35 skrev Julian Foad <ju...@apache.org>:
> +1.
>
r1886227, kept the third and fourth lines of the comment unchanged since
the suggested change was >80 chars.
Should I have added Approved by: in the log message?
/Daniel Sahlberg
Re: r1880192
Posted by Julian Foad <ju...@apache.org>.
Daniel Sahlberg wrote:
> Like this?
> /* Iterate over each path with explicit mergeinfo added by the merge.
> * Iterate in a parent-to-child order so that inherited mergeinfo is propagated
> * consistently from each parent path to its children. (Issue #4862) */
+1.
- Julian
Re: r1880192
Posted by Daniel Sahlberg <da...@gmail.com>.
Den fre 5 feb. 2021 kl 11:19 skrev Julian Foad <ju...@apache.org>:
> (Ugh, sorry for the previous blank reply.)
>
> Daniel Sahlberg wrote:
> > [...] Is it intentional to have both comments? [...] It would make it
> easier to understand (at least for me) if it was a single comment. [...]
> >
> > - /* Iterate over each path with explicit mergeinfo added by the merge.
> */
> > - /* Iterate over the paths in a parent-to-child order so that inherited
> > - * mergeinfo is propagated consistently from each parent path to its
> > - * children. (Issue #4862) */
> > + /* Iterate over the paths with explicit mergeinfo added by the merge
> > + * in a parent-to-child order so that inherited mergeinfo is
> propagated
> > + * consistently from each parent path to its children. (Issue #4862)
> */
>
> I intended it to be read as one comment containing two sentences about the
> same iteration. Feel free to adjust as you like, now I've confirmed you're
> not missing any hidden meaning. I think it would be clearest to remove the
> comment markers but keep as two sentences.
>
Like this?
/* Iterate over each path with explicit mergeinfo added by the merge.
* Iterate in a parent-to-child order so that inherited mergeinfo is
propagated
* consistently from each parent path to its children. (Issue #4862) */
Re: r1880192
Posted by Julian Foad <ju...@apache.org>.
(Ugh, sorry for the previous blank reply.)
Daniel Sahlberg wrote:
> [...] Is it intentional to have both comments? [...] It would make it easier to understand (at least for me) if it was a single comment. [...]
>
> - /* Iterate over each path with explicit mergeinfo added by the merge. */
> - /* Iterate over the paths in a parent-to-child order so that inherited
> - * mergeinfo is propagated consistently from each parent path to its
> - * children. (Issue #4862) */
> + /* Iterate over the paths with explicit mergeinfo added by the merge
> + * in a parent-to-child order so that inherited mergeinfo is propagated
> + * consistently from each parent path to its children. (Issue #4862) */
I intended it to be read as one comment containing two sentences about the same iteration. Feel free to adjust as you like, now I've confirmed you're not missing any hidden meaning. I think it would be clearest to remove the comment markers but keep as two sentences.
- Julian
Re: r1880192
Posted by Julian Foad <ju...@apache.org>.
Daniel Sahlberg wrote:
> [...] Is it intentional to have both comments? [...] It would make it easier to understand (at least for me) if it was a single comment. [...]
>
> - /* Iterate over each path with explicit mergeinfo added by the merge. */
> - /* Iterate over the paths in a parent-to-child order so that inherited
> - * mergeinfo is propagated consistently from each parent path to its
> - * children. (Issue #4862) */
> + /* Iterate over the paths with explicit mergeinfo added by the merge
> + * in a parent-to-child order so that inherited mergeinfo is propagated
> + * consistently from each parent path to its children. (Issue #4862) */
- Julian