git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v3 1/1] general style: replaces memcmp() with starts_with()

From
Quint Guvernator <quintus.public@gmail.com>
Date
Mar 17, 2014, 20:36 UTC
Message-ID
<CALs4jVFFf2cBU920jRcyh1v9KAxkSS+LnycfYo4quuqjNRsp2g@mail.gmail.com>
In-Reply-To
<532722ED.1010500@alum.mit.edu>
2014-03-17 12:29 GMT-04:00 Michael Haggerty <mhagger@alum.mit.edu>:
Show 6 quoted lines
> This hunk uses the magic number "11" a couple lines later.  Junio (I
> think rightly) objected [1] to rewrites in these circumstances because
> they make it even less obvious where the constant came from and thereby
> make the complete elimination of the hard-coded constant *more* difficult.
>
> [1] http://article.gmane.org/gmane.comp.version-control.git/244005

Sure, I can understand that. I'll look through the hunks again with more context in the diff to try to look for more cases where magic numbers are used more than once. One of the goals of this revision is to minimize that, see the commit message.

Show 7 quoted lines
> In any open-source project it is important to optimize for the time of
> the reviewer, because code-review is a relatively tedious task and is
> therefore often the bottleneck for progress.  Therefore I suggest that
> you turn your approach on its head.  Don't "remove the most
> objectionable hunks" but rather *include only the best hunks*--the ones
> that you can stand behind 100%, which you think are an unambiguous
> improvement, and that the reviewer can accept without reservations.
...
Show 7 quoted lines
> It would be much better for you to submit only a handful of changes (or
> even only one!) that is perfect, rather than throwing a bunch at the
> wall and asking the reviewer to pick between the good and the bad.  As
> you gain experience and learn about the preferences of the Git project,
> you will get a better feel for the boundary between
> acceptable/unacceptable patches, and *then* you will be able to start
> submitting patches closer to the boundary.

I see. For v4, I will be more discerning as to what I include. I will try to keep the scope of future patches down and err on the side of caution when I review my own changes before submitting. Thank you for the *pointers.

I'm going to give v4 a shot. It should be on the mailing list in an hour or so.
Quint
Previous: Michael Haggerty
Message 4 of 4 in “general style: replaces memcmp() with starts_with()”
  1. 0/1 general style: replaces memcmp() with starts_with()Quint Guvernator, Mar 15, 2014
  2. 1/1 general style: replaces memcmp() with starts_with()Quint Guvernator, Mar 15, 2014
  3. Michael HaggertyMar 17, 2014
  4. Quint GuvernatorMar 17, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.