Re: [PATCH] general style: replaces memcmp() with proper starts_with()

Jeff King [off-list ref] writes:

So I think the whole function could use some refactoring to handle
corner cases better.  I'll try to take a look tomorrow, but please
feel free if somebody else wants to take a crack at it.

Yup, thanks.

From what I can gather, there seems to be opposition to specific

pieces of this patch.

The following area is clearly the most controversial:

 static inline int standard_header_field(const char *field, size_t len)
 {
-    return ((len == 4 && !memcmp(field, "tree ", 5)) ||
-            (len == 6 && !memcmp(field, "parent ", 7)) ||
-            (len == 6 && !memcmp(field, "author ", 7)) ||
-            (len == 9 && !memcmp(field, "committer ", 10)) ||
-            (len == 8 && !memcmp(field, "encoding ", 9)));
+    return ((len == 4 && starts_with(field, "tree ")) ||
+            (len == 6 && starts_with(field, "parent ")) ||
+            (len == 6 && starts_with(field, "author ")) ||
+            (len == 9 && starts_with(field, "committer ")) ||
+            (len == 8 && starts_with(field, "encoding ")));

I am happy to submit a version of this patch excluding this section
(and/or others), but it seems I've stumbled into a more fundamental
conversation about the place for helper functions in general (and
about refactoring skip_prefix()). I am working on this particular
change as a microproject, #14 on the list [1], and am not as familiar
with the conventions of the Git codebase as many of you on this list
are.

Junio said:

The result after the conversion, however, still have the same magic
numbers, but one less of them each.  Doesn't it make it harder to
later spot the patterns to come up with a better abstraction that
does not rely on the magic number?

It is _not_ my goal to make the code harder to maintain down the road.
So, at this point, which hunks (if any) are worth patching?

Quint


[1]: http://git.github.io/SoC-2014-Microprojects.html

On Wed, Mar 12, 2014 at 11:33:50PM -0400, Quint Guvernator wrote:

It is _not_ my goal to make the code harder to maintain down the road.
So, at this point, which hunks (if any) are worth patching?

This discussion ended up encompassing a lot of other related cleanups. I
hope we didn't scare you away. :)

My understanding is that you were approaching this as a micro-project
for GSoC. I'd love it if you want to pick up and run with some of the
ideas discussed here. But as far as a microproject goes, I think it
would make sense to identify one or two no-brainer improvement spots by
hand, and submit a patch with just those (and I think Junio gave some
good guidelines in his reply).

-Peff

2014-03-14 0:57 GMT-04:00 Jeff King [off-list ref]:

This discussion ended up encompassing a lot of other related cleanups. I
hope we didn't scare you away. :)

I don't think you could; this community is much more accepting than
other software communities around the web. The fact that I received
constructive feedback rather than a lecture when formatting issues
slipped my mind (i.e. forgetting [PATCH v2]) is reason enough to stick
around!

My understanding is that you were approaching this as a micro-project
for GSoC. I'd love it if you want to pick up and run with some of the
ideas discussed here. But as far as a microproject goes, I think it
would make sense to identify one or two no-brainer improvement spots by
hand, and submit a patch with just those (and I think Junio gave some
good guidelines in his reply).

I agree with trying to push a few uncontroversial changes through. I'd
love to take a deeper look at these helper functions and related
cleanups…perhaps it would be worth it to identify a few key areas to
work on in addition to a main GSoC project? In fact, the project I'm
looking to take on (rebase --interactive) also involves code cleanup
and might not take all summer, so I could see how those could work
well together in a proposal.

I'll be re-reading this thread and working on this patch over the
weekend to try to identify the more straightforward hunks I could
submit in a patch.

Thanks Peff and everyone else for your help.
Quint