Re: [PATCH Outreachy] mru: use double-linked list from list.h

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.

quoted

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.

quoted

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