On Fri, Sep 29, 2017 at 07:08:28PM +0300, Оля Тележная wrote: > Many thanks to all of you, I am interested in every opinion. Sorry > that I wasn't in the discussion, unfortunately I got sick, that's why > I skipped all the process.
No problem. It's often reasonable to let review comments come in and think about them as a whole before responding or re-posting anyway. > > An overlong line (I can locally wrap it, so the patch does not have > > to be re-sent only to fix this alone). > I've read only about 50 characters max in commit head (and > highlighting repeats it), but there's nothing about max length of line > in commit message. Sorry, next time I will make it shorter. Usually we shoot for making things look good in an 80-column terminal, including both code and commit messages. For commit message bodies, we tend to make them a little shorter, since "git log" will indent them. 72 characters is reasonable there. And we tend to make subject lines a little shorter than that, even since they often appear with "Subject:" and "[PATCH]" prefixed. I usually go for about 60 characters, but will go over if it helps make the subject more clear. > > I had envisioned leaving mru_mark() as a wrapper for "move to the front" > > that could operate on any list. But seeing how Olga's patch takes it > > down to two trivial lines, I'd also be fine with an endgame that just > > eliminates it. > Let's add needed function to list.h directly? Yes, I think we could just call this "list_move_to_front()" or something. The fact that it's operating on a list called "packed_git_mru" is probably sufficient to make it clear that the purpose is managing recentness. > I also wanted to add > list_for_each_entry function to list.h as it's in Linux kernel. > https://www.kernel.org/doc/htmldocs/kernel-api/API-list-for-each-entry.html > It will simplify the code even more, guess that not only in MRU > related code. Maybe we need to do that in separate patch. It would be nice to have list_for_each_entry(), but unfortunately it's not portable. It relies on having a typeof operator, and we build on platforms that lack it. It was omitted in 94e99012fc (http-walker: reduce O(n) ops with doubly-linked list, 2016-07-11) for that reason. > About minor issues ( "tmp" vs "p2", variable scope, space indentation) > - fully agree, I will fix it. Thanks. > So finally I think that I need to fix that minor issues and that's > all. I have plans to rewrite (with --amend) my current commit (I think > so because I will add no new features, so it's better to have single > commit for all changes). > As I understand, Submitgit will send an update in a new thread. And I > need to say there [PATCH v2]. > Please correct me if I am wrong in any of the moments mentioned earlier. Correct. Until a patch is merged to Junio's "next" branch (at which point it is set in stone), we generally prefer to rewrite it with "--amend" (or git-rebase) to fix anything that comes up during the review. > By the way, other contributors write smth like "[PATCH v6 0/3]". What > does mean "0/3"? It's about editing separate commits in a single > patch, am I right? Right, it means multiple commits in a logical series that are meant to be applied together. Your patch is small enough that it makes as a single patch. If we wanted to do the second step of dropping mru.[ch] entirely now, then you'd probably have at least a 2-patch series. (I'm OK with not doing that second step for now, though if we are not going to polish up the mru_for_each() interface, it may make sense to make the final step sooner rather than later). -Peff
