| Message ID | 20201201004649.57548-2-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | bfded8757032ea7095ed688a1065c9767016e509 |
| Headers | show |
| Series | Random cleanups | expand |
On Mon, Nov 30, 2020 at 06:46:46PM -0600, Felipe Contreras wrote: > We can remove one level of indentation and make the code clearer. So why > not do so? I know your question was rhetorical, but a good reason not to do so in general is that the existing pattern reveals some intent. E.g., it sometimes is the case that with the _current_ code we can return early from a function or loop, but that is not inherent to what the code is doing, and the early return or continue makes it harder to understand that. I don't think that is the case here, though. The continue actually expresses the intent more clearly than the existing code. So the patch looks good to me (as do the others in the series). -Peff
Jeff King <peff@peff.net> writes: > On Mon, Nov 30, 2020 at 06:46:46PM -0600, Felipe Contreras wrote: > >> We can remove one level of indentation and make the code clearer. So why >> not do so? > > I know your question was rhetorical, but a good reason not to do so in > general is that the existing pattern reveals some intent. E.g., it > sometimes is the case that with the _current_ code we can return early > from a function or loop, but that is not inherent to what the code is > doing, and the early return or continue makes it harder to understand > that. > > I don't think that is the case here, though. The continue actually > expresses the intent more clearly than the existing code. > > So the patch looks good to me (as do the others in the series). Yup, the patch text (eh, the source with the patch applied) looks good. I'd agree with your hintand would take rhetorical question out of the log message while queuing. Thanks.
On Tue, Dec 1, 2020 at 5:43 AM Jeff King <peff@peff.net> wrote: > > On Mon, Nov 30, 2020 at 06:46:46PM -0600, Felipe Contreras wrote: > > > We can remove one level of indentation and make the code clearer. So why > > not do so? > > I know your question was rhetorical, but a good reason not to do so in > general is that the existing pattern reveals some intent. E.g., it > sometimes is the case that with the _current_ code we can return early > from a function or loop, but that is not inherent to what the code is > doing, and the early return or continue makes it harder to understand > that. Yes, but even in those cases it arguably helps readability. Anyway, I prefer to argue on the tangible rather than hypotheticals because the hypotheticals are infinite. In this particular case there's no particular reason to just continue if there's no prefix. Cheers.
On Tue, Dec 01, 2020 at 05:25:53PM -0600, Felipe Contreras wrote: > > I know your question was rhetorical, but a good reason not to do so in > > general is that the existing pattern reveals some intent. E.g., it > > sometimes is the case that with the _current_ code we can return early > > from a function or loop, but that is not inherent to what the code is > > doing, and the early return or continue makes it harder to understand > > that. > > Yes, but even in those cases it arguably helps readability. > > Anyway, I prefer to argue on the tangible rather than hypotheticals > because the hypotheticals are infinite. > > In this particular case there's no particular reason to just continue > if there's no prefix. Right, which I already agreed with. I hesitated on responding at all, because you and I have not had a good history of agreeing on commit messages. But my comment was primarily for other readers on the list. I do not want people blindly applying a rule like "less indentation is good" without thinking about the code overall (I don't know whether you thought about it or not, but it was not apparent from your commit message). -Peff
On Tue, Dec 1, 2020 at 8:01 PM Jeff King <peff@peff.net> wrote: > On Tue, Dec 01, 2020 at 05:25:53PM -0600, Felipe Contreras wrote: > > In this particular case there's no particular reason to just continue > > if there's no prefix. > > Right, which I already agreed with. > > I hesitated on responding at all, because you and I have not had a good > history of agreeing on commit messages. We don't have to agree. Having a different opinion is fine. In fact, that's precisely what keeps a project healthy: diversity of opinion. Do not hesitate. If you feel it must be said, say it. Or at least; don't hesitate on my behalf. I don't take disagreements personally. > But my comment was primarily for > other readers on the list. I do not want people blindly applying a rule > like "less indentation is good" without thinking about the code overall > (I don't know whether you thought about it or not, but it was not > apparent from your commit message). I see. I did think about it, but I couldn't find an instance in which such similar change could be contestable. "Why not do so?" was my succinct way of saying that: I cannot find any of the typical reasons why we might want to keep an indented condition. Sure, there's possibly better ways to state that, but I couldn't think of any in the limited time I devoted to the commit message. Maybe: "There's no reason to keep the main conditional branch indented; we can invert the condition and not continue". Cheers.
diff --git a/refspec.c b/refspec.c index c49347c2d7..d4823dbff9 100644 --- a/refspec.c +++ b/refspec.c @@ -272,15 +272,16 @@ void refspec_ref_prefixes(const struct refspec *rs, else if (item->src && !item->exact_sha1) prefix = item->src; - if (prefix) { - if (item->pattern) { - const char *glob = strchr(prefix, '*'); - strvec_pushf(ref_prefixes, "%.*s", - (int)(glob - prefix), - prefix); - } else { - expand_ref_prefix(ref_prefixes, prefix); - } + if (!prefix) + continue; + + if (item->pattern) { + const char *glob = strchr(prefix, '*'); + strvec_pushf(ref_prefixes, "%.*s", + (int)(glob - prefix), + prefix); + } else { + expand_ref_prefix(ref_prefixes, prefix); } } }
We can remove one level of indentation and make the code clearer. So why not do so? No functional changes. Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- refspec.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)