On Tue, Sep 03, 2013 at 02:36:39PM -0400, Richard Hansen wrote:
> I have a patch submission question: Is it OK that I didn't use the
> '--in-reply-to' argument to 'git send-email' when I sent the v3 reroll?
> Should I have marked it as a reply to the v2 email? Or should I have
> marked it as a reply to your review of the v2 email?
I generally prefer if they are kept in the same thread, for two reasons:
1. People reading v3 may want to refer back to v2. You can provide a
link to the previous discussion in the archive, but in-reply-to is
a machine-readable way of doing the same thing (so it also works in
people's MUAs without having to jump out to the archive).
2. People reading v2 may be doing so after v3 has been published, and
of course v2 cannot have linked to v3 at the time it was written.
If the reader has a threaded MUA (or uses gmane), then the two are
grouped together, and they can easily see that reading v2 carefully
may be a waste of time, as it is already obsolete.
Between replying to the review versus the original patch, I don't have a
preference (any decent reader should group the whole thread, which is
enough to accomplish either of the above two).
> Done. I see by your reply that my fear of going a bit overboard in the
> test was justified. :) I don't mind rerolling if you'd prefer a
> simpler test.
I think what you have is fine (modulo dropping the "&& true", which I
somehow failed to notice on first read). It is more thorough, and I do
not think it hurts the readability of the end result (i.e., I can still
tell what the point of the test is). But I am happy either way (I only
mentioned the simpler version because you asked).
> For future reference, is there a preference for putting tests of a new
> feature in a separate commit? In the same commit? Doesn't really matter?
I usually put them in the same commit for a small enhancement or fix
like this. Seeing the test along with the change often helps the reader
understand what the patch is doing by providing a concrete example.
Sometimes for a set of changes that needs a large set of refactoring
patches, I'll introduce a group of related failing tests at the
beginning (using test_expect_failure), and then mark them as
expect_success as they are fixed by individual commits.
So you can use your judgement about how the split will communicate to
the reviewer (and later readers of the commit history). But the one
thing you _can't_ do is introduce a failing test and mark it as
expect_success. We try to keep "make test" successful in each commit,
which lets people bisect the history more easily.
> > I wonder if we could do even better with:
> >
> > const char *x;
> > ...
> > if ((x = skip_prefix(sp, commit_type)) && *x == '}')
> >
> > which avoids the magic lengths altogether
>
> Not bad, especially since skip_prefix() already exists.
I'll post a follow-up patch in a second.
> There's also this awkward approach, which would avoid strlen() altogether:
>
> commit.h:
>
> extern const char *commit_type;
> extern const size_t commit_type_len;
>
> commit.c:
>
> const char commit_type_array[] = "commit";
> const char *commit_type = &commit_type_array[0];
> const size_t commit_type_len = sizeof(commit_type_array) - 1;
That is a little manual and awkward for my tastes. skip_prefix() is
an inline specifically so that the compiler _can_ optimize out the
strlen in such cases, without us having to resort to writing the code
ourselves. So if we cared about the strlen here (and I don't think we
do), it would be cleaner to simply convert commit_type to a macro,
static string literal, or static constant array.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html