git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Оля Тележная" <olyatelezhnaya@gmail.com>
To: Christian Couder <christian.couder@gmail.com>,
	Jeff King <peff@peff.net>,
	gitster@pobox.com
Cc: git <git@vger.kernel.org>
Subject: Re: [PATCH Outreachy] mru: use double-linked list from list.h
Date: Fri, 29 Sep 2017 19:08:28 +0300	[thread overview]
Message-ID: <CAL21BmkcVSEhEK+tAE-RNVabb0pnokYwbagueUrp9giZ3zqT8A@mail.gmail.com> (raw)
In-Reply-To: <CAP8UFD1-9dYSX-VKZSPN9Ei75V8mGC-wusieL45ArxxJ08tO9Q@mail.gmail.com>

Hi everyone,
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.
I want to reply to the main moments and also ask some questions.

>> Simplify mru.c, mru.h and related code by reusing the double-linked list implementation from list.h instead of a custom one.
> 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.

About many different opinions how to improve the code: I agree with
the idea that my commit is a middle step to get rid of MRU at all. If
we really need to add initializer/mru_for_each/smth_else - it's
absolutely not a problem, but as it was said, not sure that we need
it.
It really looks that using list implementation from list.h directly
won't be worse.

> 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? 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.

About minor issues ( "tmp" vs "p2", variable scope, space indentation)
- fully agree, I will fix it.

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.

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?

Thank you one more time!
Olga

  reply	other threads:[~2017-09-29 16:08 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-09-27 10:18 [PATCH] [Outreachy] cleanup: use list.h in mru.h and mru.c Оля Тележная
2017-09-27 11:30 ` Christian Couder
2017-09-28  8:38   ` [PATCH Outreachy] mru: use double-linked list from list.h Olga Telezhnaya
2017-09-28 11:03     ` Junio C Hamano
2017-09-28 20:47       ` Jeff King
2017-09-28 21:56         ` Junio C Hamano
2017-09-28 22:19           ` Jeff King
2017-09-28 21:04     ` Jeff King
2017-09-28 22:42     ` Jeff King
2017-09-29  7:18       ` Christian Couder
2017-09-29  7:23         ` Jeff King
2017-09-29 11:50           ` Christian Couder
2017-09-29 16:08             ` Оля Тележная [this message]
2017-09-29 20:38               ` Оля Тележная
2017-09-29 23:40                 ` Jeff King
2017-09-30 18:09                   ` Оля Тележная
2017-10-02  8:22                     ` Jeff King
2017-09-29 23:37               ` Jeff King
2017-09-30  0:07                 ` Junio C Hamano
2017-09-30 17:51     ` [PATCH v2 " Olga Telezhnaya
2017-10-02  8:20       ` Jeff King
2017-10-02  9:37         ` Оля Тележная
2017-10-03 10:10           ` Jeff King
2017-11-08  1:44         ` Junio C Hamano
2017-11-08  4:22           ` Jeff King
2017-11-10 11:51             ` Оля Тележная

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAL21BmkcVSEhEK+tAE-RNVabb0pnokYwbagueUrp9giZ3zqT8A@mail.gmail.com \
    --to=olyatelezhnaya@gmail.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).