| Message ID | 20230207150359.177641-1-five231003@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [GSoC] commit: warn the usage of reverse_commit_list() helper | expand |
On Tue, Feb 07 2023, Kousik Sanagavarapu wrote: > The helper function reverse_commit_list() has destructive behavior when > used to reverse a list in-place. Warn about this behavior. > > Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com> > --- > > This patch has been sent based on the confusion that can be caused while > using the reverse_commit_list() helper function. One example of this is > a recent patch that I submitted[1] where the use of this function broke > try_merge_strategy() in merge. > > It is also based on the discussions[2] there that I send this patch. > > [1]: https://lore.kernel.org/git/20230202165137.118741-1-five231003@gmail.com/ > [2]: https://lore.kernel.org/git/xmqqmt5uo9ea.fsf@gitster.g/ > > commit.h | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/commit.h b/commit.h > index fa39202fa6..9dba07748f 100644 > --- a/commit.h > +++ b/commit.h > @@ -198,7 +198,12 @@ void commit_list_sort_by_date(struct commit_list **list); > /* Shallow copy of the input list */ > struct commit_list *copy_commit_list(struct commit_list *list); > > -/* Modify list in-place to reverse it, returning new head; list will be tail */ > +/* > + * Modify list in-place to reverse it, returning new head; list will be tail. > + * > + * NOTE! The reversed list is constructed using the elements of the original > + * list, hence losing the original list. > + */ > struct commit_list *reverse_commit_list(struct commit_list *list); Junio can clarify, but I understood from his original comment on this suggesting a comment that he wasn't aware of the existing documentation. I think it's better just to chuck this up to an understandable one-off mistake, if we're going to update the docs here I don't really see what's being added by this addition. It seems to me that this is just rephrasing what's being said more succinctly with "modifies in-place", it's understood that any function which does that is going to schred the input data for its own purposes. If that wording is thought to be too technical or obscure wouldn't we be better off with replacing the existing wording with something using less jargon, rather than keeping the jargon & adding a rephrasing of it? Having said that, I think the existing version is fine, and we could just ascribe the issue that prompted this to a one-off mistake :) I think if you want to pursue this, a much better improvement here would be to show what the user *should* do. E.g. show one code example of using the API in-place, and then the preferred pattern if one wants to produce a new reversed commit list, while retaining the original (presumably just copy_commit_list() followed by reverse_commit_list()).
Kousik Sanagavarapu <five231003@gmail.com> writes: > -/* Modify list in-place to reverse it, returning new head; list will be tail */ > +/* > + * Modify list in-place to reverse it, returning new head; list will be tail. > + * > + * NOTE! The reversed list is constructed using the elements of the original > + * list, hence losing the original list. > + */ After re-reading the original, I realize that "in-place" is good enough clue to say that this is destructive.
On Tue, 7 Feb 2023 at 23:35, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > >[...] > > Having said that, I think the existing version is fine, and we could > just ascribe the issue that prompted this to a one-off mistake :) I understand it now. Thanks. > I think if you want to pursue this, a much better improvement here would > be to show what the user *should* do. > > E.g. show one code example of using the API in-place, and then the > preferred pattern if one wants to produce a new reversed commit list, > while retaining the original (presumably just copy_commit_list() > followed by reverse_commit_list()). Following the response by Junio, I think it's better off that I leave it this way? Thanks, Kousik
On Wed, 8 Feb 2023 at 00:23, Junio C Hamano <gitster@pobox.com> wrote: > > After re-reading the original, I realize that "in-place" is good > enough clue to say that this is destructive. I see. Thanks for the review. Kousik
diff --git a/commit.h b/commit.h index fa39202fa6..9dba07748f 100644 --- a/commit.h +++ b/commit.h @@ -198,7 +198,12 @@ void commit_list_sort_by_date(struct commit_list **list); /* Shallow copy of the input list */ struct commit_list *copy_commit_list(struct commit_list *list); -/* Modify list in-place to reverse it, returning new head; list will be tail */ +/* + * Modify list in-place to reverse it, returning new head; list will be tail. + * + * NOTE! The reversed list is constructed using the elements of the original + * list, hence losing the original list. + */ struct commit_list *reverse_commit_list(struct commit_list *list); void free_commit_list(struct commit_list *list);
The helper function reverse_commit_list() has destructive behavior when used to reverse a list in-place. Warn about this behavior. Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com> --- This patch has been sent based on the confusion that can be caused while using the reverse_commit_list() helper function. One example of this is a recent patch that I submitted[1] where the use of this function broke try_merge_strategy() in merge. It is also based on the discussions[2] there that I send this patch. [1]: https://lore.kernel.org/git/20230202165137.118741-1-five231003@gmail.com/ [2]: https://lore.kernel.org/git/xmqqmt5uo9ea.fsf@gitster.g/ commit.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)