Re: [PATCH 10/16] fast-import: use skip_prefix for parsing input

On Fri, Jun 20, 2014 at 1:45 AM, Jeff King [off-list ref] wrote:

On Thu, Jun 19, 2014 at 11:19:09PM -0400, Eric Sunshine wrote:

quoted

quoted

-               if (starts_with(command_buf.buf, "M "))
-                       file_change_m(b);
-               else if (starts_with(command_buf.buf, "D "))
-                       file_change_d(b);
-               else if (starts_with(command_buf.buf, "R "))
-                       file_change_cr(b, 1);
-               else if (starts_with(command_buf.buf, "C "))
-                       file_change_cr(b, 0);
-               else if (starts_with(command_buf.buf, "N "))
-                       note_change_n(b, &prev_fanout);
+               const char *v;

This declaration of 'v' shadows the 'v' added by patch 8/16 earlier in
the function.

Thanks.  I reordered the patches before sending, so when this one was
originally written, there was no "v" at the top-level of the function.
I think we can just drop this interior one. The point of the short "v"
is that it can be used as a temporary value for prefix matches, so I
think we can just reuse the same one.

Agreed. The intended usage of 'v' is clear enough and the code simple
enough that confusion is unlikely.

I tried compiling with -Wshadow (which I don't usually do), but we're
not even close to compiling clean there. Some of them are legitimately
confusing (e.g., try figuring out "end" in parse_rev_note). But others
look just annoying (e.g., complaining that a local "usage" conflicts
with the global function). I don't know if we want to put effort into
being -Wshadow clean or not.

I just happened to notice the shadowing declaration while reading the
patch, but don't feel strongly about existing cases. It makes sense to
clean up confusing cases, such 'end' in parse_rev_note(), when working
on that code (just as with style cleanups), but thus far nobody has
been complaining about existing shadowed variables, so global cleanup
would likely be considered churn.