# [PATCH 4/4] tests: lib-functions: trivial style cleanups 18 messages from 2020-12-01 to 2020-12-02. Participants: Felipe Contreras, Jeff King, Junio C Hamano, René Scharfe, Johannes Schindelin. Thread: https://gitlist.dev/t/54735 ## Felipe Contreras, 2020-12-01 00:46 Subject: [PATCH 4/4] tests: lib-functions: trivial style cleanups Message-ID: <20201201004649.57548-5-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/20201201004649.57548-5-felipe.contreras%40gmail.com In-Reply-To: <20201201004649.57548-1-felipe.contreras@gmail.com> ``` Signed-off-by: Felipe Contreras --- t/test-lib-functions.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 7ba3011b90..a791f2f1d6 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -955,7 +955,7 @@ test_expect_code () { # - cmp's output is not nearly as easy to read as diff -u # - not all diff versions understand "-u" -test_cmp() { +test_cmp () { eval "$GIT_TEST_CMP" '"$@"' } @@ -968,7 +968,7 @@ test_cmp() { # # test_cmp_config foo core.bar # -test_cmp_config() { +test_cmp_config () { local GD && if test "$1" = "-C" then @@ -984,7 +984,7 @@ test_cmp_config() { # test_cmp_bin - helper to compare binary files -test_cmp_bin() { +test_cmp_bin () { cmp "$@" } @@ -1418,7 +1418,7 @@ nongit () { # whitespace and put in a single packet. Note that data containing NULs must be # given on stdin, and that empty input becomes an empty packet, not a flush # packet (for that you can just print 0000 yourself). -packetize() { +packetize () { if test $# -gt 0 then packet="$*" -- 2.29.2 ``` ## Felipe Contreras, 2020-12-01 00:46 Subject: [PATCH 1/4] refspec: trivial cleanup Message-ID: <20201201004649.57548-2-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/20201201004649.57548-2-felipe.contreras%40gmail.com In-Reply-To: <20201201004649.57548-1-felipe.contreras@gmail.com> ``` 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 --- refspec.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) 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); } } } -- 2.29.2 ``` ## Felipe Contreras, 2020-12-01 00:46 Subject: [PATCH 3/4] test: completion: fix typos Message-ID: <20201201004649.57548-4-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/20201201004649.57548-4-felipe.contreras%40gmail.com In-Reply-To: <20201201004649.57548-1-felipe.contreras@gmail.com> ``` Signed-off-by: Felipe Contreras --- t/t9902-completion.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 5c01c75d40..81a728c743 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -906,7 +906,7 @@ test_expect_success '__git_refs - after --opt= - full refs' ' test_cmp expected "$actual" ' -test_expect_success '__git refs - exluding refs' ' +test_expect_success '__git refs - excluding refs' ' cat >expected <<-EOF && ^HEAD ^master @@ -922,7 +922,7 @@ test_expect_success '__git refs - exluding refs' ' test_cmp expected "$actual" ' -test_expect_success '__git refs - exluding full refs' ' +test_expect_success '__git refs - excluding full refs' ' cat >expected <<-EOF && ^refs/heads/master ^refs/heads/matching-branch -- 2.29.2 ``` ## Felipe Contreras, 2020-12-01 00:46 Subject: [PATCH 0/4] Random cleanups Message-ID: <20201201004649.57548-1-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/20201201004649.57548-1-felipe.contreras%40gmail.com ``` Just some low-hanging fruit I found. Felipe Contreras (4): refspec: trivial cleanup .gitignore: remove dangling file test: completion: fix typos tests: lib-functions: trivial style cleanups .gitignore | 1 - refspec.c | 19 ++++++++++--------- t/t9902-completion.sh | 4 ++-- t/test-lib-functions.sh | 8 ++++---- 4 files changed, 16 insertions(+), 16 deletions(-) -- 2.29.2 ``` ## Felipe Contreras, 2020-12-01 00:46 Subject: [PATCH 2/4] .gitignore: remove dangling file Message-ID: <20201201004649.57548-3-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/20201201004649.57548-3-felipe.contreras%40gmail.com In-Reply-To: <20201201004649.57548-1-felipe.contreras@gmail.com> ``` The library was removed 7 years ago on commit ae34ac126f. But not from the .gitignore file. Signed-off-by: Felipe Contreras --- .gitignore | 1 - 1 file changed, 1 deletion(-) diff --git a/.gitignore b/.gitignore index f85d02c854..7c5096aee5 100644 --- a/.gitignore +++ b/.gitignore @@ -135,7 +135,6 @@ /git-remote-ftps /git-remote-fd /git-remote-ext -/git-remote-testpy /git-repack /git-replace /git-request-pull -- 2.29.2 ``` ## Jeff King, 2020-12-01 11:43 Subject: Re: [PATCH 1/4] refspec: trivial cleanup Message-ID: URL: https://gitlist.dev/e/X8YsXjN7MQrLnytw%40coredump.intra.peff.net In-Reply-To: <20201201004649.57548-2-felipe.contreras@gmail.com> ``` 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 ``` ## Junio C Hamano, 2020-12-01 18:29 Subject: Re: [PATCH 1/4] refspec: trivial cleanup Message-ID: URL: https://gitlist.dev/e/xmqqh7p5qt0p.fsf%40gitster.c.googlers.com In-Reply-To: ``` Jeff King 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. ``` ## Junio C Hamano, 2020-12-01 18:30 Subject: Re: [PATCH 2/4] .gitignore: remove dangling file Message-ID: URL: https://gitlist.dev/e/xmqqczztqszv.fsf%40gitster.c.googlers.com In-Reply-To: <20201201004649.57548-3-felipe.contreras@gmail.com> ``` Felipe Contreras writes: > The library was removed 7 years ago on commit ae34ac126f. But not from > the .gitignore file. > > Signed-off-by: Felipe Contreras > --- Good eyes. Any automation used here, or just mark I eyeballs? > .gitignore | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/.gitignore b/.gitignore > index f85d02c854..7c5096aee5 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -135,7 +135,6 @@ > /git-remote-ftps > /git-remote-fd > /git-remote-ext > -/git-remote-testpy > /git-repack > /git-replace > /git-request-pull ``` ## Felipe Contreras, 2020-12-01 23:25 Subject: Re: [PATCH 1/4] refspec: trivial cleanup Message-ID: URL: https://gitlist.dev/e/CAMP44s3usd5EW4sA6_qapnDBVRAWg4nk5ZSxAPPQN1UygD%3DctA%40mail.gmail.com In-Reply-To: ``` On Tue, Dec 1, 2020 at 5:43 AM Jeff King 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. -- Felipe Contreras ``` ## Felipe Contreras, 2020-12-01 23:32 Subject: Re: [PATCH 2/4] .gitignore: remove dangling file Message-ID: URL: https://gitlist.dev/e/CAMP44s1sdTYxWthfHCxPdoDdmapE05gG5Rr1W3BjTfWv2pCTvg%40mail.gmail.com In-Reply-To: ``` On Tue, Dec 1, 2020 at 12:30 PM Junio C Hamano wrote: > > Felipe Contreras writes: > > > The library was removed 7 years ago on commit ae34ac126f. But not from > > the .gitignore file. > > > > Signed-off-by: Felipe Contreras > > --- > > Good eyes. Any automation used here, or just mark I eyeballs? Nope. Just switching to a very old branch prompted me to look at the .gitignore file and I stumbled upon it. -- Felipe Contreras ``` ## Jeff King, 2020-12-02 02:01 Subject: Re: [PATCH 1/4] refspec: trivial cleanup Message-ID: URL: https://gitlist.dev/e/X8b1au%2BGDihNvhIQ%40coredump.intra.peff.net In-Reply-To: ``` 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 ``` ## Jeff King, 2020-12-02 02:26 Subject: Re: [PATCH 2/4] .gitignore: remove dangling file Message-ID: URL: https://gitlist.dev/e/X8b7UMcsfaD0OFv4%40coredump.intra.peff.net In-Reply-To: ``` On Tue, Dec 01, 2020 at 10:30:12AM -0800, Junio C Hamano wrote: > Felipe Contreras writes: > > > The library was removed 7 years ago on commit ae34ac126f. But not from > > the .gitignore file. > > > > Signed-off-by: Felipe Contreras > > --- > > Good eyes. Any automation used here, or just mark I eyeballs? I was curious how hard it would be to find unused ones, and it is not too bad: find * -type f | git check-ignore --stdin -v | perl -e ' my $ignore_file = shift; while (<>) { /^\Q$ignore_file\E:(\d+)/ and $seen{$1} = 1; } open(my $fh, "<", $ignore_file); while (<$fh>) { print "$. $_" unless $seen{$.}; } ' .gitignore It turns up many hits, depending on what built cruft you have in your directory (having a crufty state is nice because it shows which patterns are used, but it also may mean you have leftover trash from old versions that doesn't get built anymore, and which we wouldn't notice). So I think a human has to apply some brain-power to the result to see which ones are plausibly created by other people's environments. You might want to revisit some of the old dist rules (do we still ever build git-core-*? "make dist" doesn't seem to. Likewise, "make rpm" and "make deb" are no more, so perhaps it's time to drop those ignores. The one below didn't take much brain-power, though. -- >8 -- Subject: [PATCH] gitignore: drop duplicate entry for git-sh-i18n This was accidentally added by e00cf070a4 (git-sh-i18n.sh: add no-op gettext() and eval_gettext() wrappers, 2011-05-14), even though an earlier commit in the same series had already done so. Signed-off-by: Jeff King --- .gitignore | 1 - 1 file changed, 1 deletion(-) diff --git a/.gitignore b/.gitignore index f85d02c854..d20d4afc1d 100644 --- a/.gitignore +++ b/.gitignore @@ -152,7 +152,6 @@ /git-sh-i18n /git-sh-i18n--envsubst /git-sh-setup -/git-sh-i18n /git-shell /git-shortlog /git-show -- 2.29.2.894.g2dadb8c6b8 ``` ## Felipe Contreras, 2020-12-02 02:27 Subject: Re: [PATCH 1/4] refspec: trivial cleanup Message-ID: URL: https://gitlist.dev/e/CAMP44s2KHv_XvQi7YiGKLZ3Tz3nvOj3ZMg%2BuZtZLnLjyW-wgNQ%40mail.gmail.com In-Reply-To: ``` On Tue, Dec 1, 2020 at 8:01 PM Jeff King 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. -- Felipe Contreras ``` ## Felipe Contreras, 2020-12-02 03:30 Subject: Re: [PATCH 2/4] .gitignore: remove dangling file Message-ID: URL: https://gitlist.dev/e/CAMP44s2kHshVBRaQ%3D%2BjMTLvvxo%2BaQyB_XP8Et%3D%2BZZt8XhpDufQ%40mail.gmail.com In-Reply-To: ``` On Tue, Dec 1, 2020 at 8:26 PM Jeff King wrote: > I was curious how hard it would be to find unused ones, and it is not > too bad: > > find * -type f | > git check-ignore --stdin -v | > perl -e ' > my $ignore_file = shift; > while (<>) { > /^\Q$ignore_file\E:(\d+)/ and $seen{$1} = 1; > } > open(my $fh, "<", $ignore_file); > while (<$fh>) { > print "$. $_" unless $seen{$.}; > } > ' .gitignore Or to use a more modern language: find * -type f | git check-ignore --stdin -v | ruby -e ' $ignore_file = ARGV.first $seen = {} $stdin.each do |e| e =~ /^#{Regexp.quote($ignore_file)}:(\d+)/ and $seen[$1.to_i - 1] = true end ARGF.each_with_index do |e, i| print "#{i} #{e}" unless $seen[i] end ' .gitignore -- Felipe Contreras ``` ## René Scharfe, 2020-12-02 11:46 Subject: [PATCH] gitignore: remove entry for git serve Message-ID: <4e30fa82-fe24-1783-6f9a-13e1dd6f4ca9@web.de> URL: https://gitlist.dev/e/4e30fa82-fe24-1783-6f9a-13e1dd6f4ca9%40web.de In-Reply-To: ``` b7ce24d095 (Turn `git serve` into a test helper, 2019-04-18) demoted git serve from a builtin command to a test helper. As a result the git-serve binary is no longer built and thus doesn't have to be ignored anymore. Signed-off-by: René Scharfe --- .gitignore | 1 - 1 file changed, 1 deletion(-) diff --git a/.gitignore b/.gitignore index f85d02c854..28c079f227 100644 --- a/.gitignore +++ b/.gitignore @@ -148,7 +148,6 @@ /git-rm /git-send-email /git-send-pack -/git-serve /git-sh-i18n /git-sh-i18n--envsubst /git-sh-setup -- 2.29.2 ``` ## Johannes Schindelin, 2020-12-02 11:03 Subject: Re: [PATCH] gitignore: remove entry for git serve Message-ID: URL: https://gitlist.dev/e/nycvar.QRO.7.76.6.2012021202490.25979%40tvgsbejvaqbjf.bet In-Reply-To: <4e30fa82-fe24-1783-6f9a-13e1dd6f4ca9@web.de> ``` Hi René, On Wed, 2 Dec 2020, René Scharfe wrote: > b7ce24d095 (Turn `git serve` into a test helper, 2019-04-18) demoted git > serve from a builtin command to a test helper. As a result the > git-serve binary is no longer built and thus doesn't have to be ignored > anymore. > > Signed-off-by: René Scharfe ACK! Dscho > --- > .gitignore | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/.gitignore b/.gitignore > index f85d02c854..28c079f227 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -148,7 +148,6 @@ > /git-rm > /git-send-email > /git-send-pack > -/git-serve > /git-sh-i18n > /git-sh-i18n--envsubst > /git-sh-setup > -- > 2.29.2 > ``` ## Jeff King, 2020-12-02 12:48 Subject: Re: [PATCH] gitignore: remove entry for git serve Message-ID: URL: https://gitlist.dev/e/X8eNGzay6yWSEoJH%40coredump.intra.peff.net In-Reply-To: <4e30fa82-fe24-1783-6f9a-13e1dd6f4ca9@web.de> ``` On Wed, Dec 02, 2020 at 12:46:01PM +0100, René Scharfe wrote: > b7ce24d095 (Turn `git serve` into a test helper, 2019-04-18) demoted git > serve from a builtin command to a test helper. As a result the > git-serve binary is no longer built and thus doesn't have to be ignored > anymore. Good catch. This shows a weakness in my detection script: I had a stale git-serve sitting around from building old versions of Git. :) I repeated the script in a "clean" directory that was a fresh clone + make, and diffed the results. git-serve was the only interesting difference I saw. So this is probably the last of the low-hanging fruit. -Peff ``` ## Junio C Hamano, 2020-12-02 22:32 Subject: Re: [PATCH] gitignore: remove entry for git serve Message-ID: URL: https://gitlist.dev/e/xmqqsg8nltzx.fsf%40gitster.c.googlers.com In-Reply-To: ``` Jeff King writes: > On Wed, Dec 02, 2020 at 12:46:01PM +0100, René Scharfe wrote: > >> b7ce24d095 (Turn `git serve` into a test helper, 2019-04-18) demoted git >> serve from a builtin command to a test helper. As a result the >> git-serve binary is no longer built and thus doesn't have to be ignored >> anymore. > > Good catch. This shows a weakness in my detection script: I had a stale > git-serve sitting around from building old versions of Git. :) > > I repeated the script in a "clean" directory that was a fresh clone + > make, and diffed the results. git-serve was the only interesting > difference I saw. So this is probably the last of the low-hanging fruit. Thanks, all. Just for simplicity, these two are also queued on the same "random cleanup" topic branch started with Felipe's four trivial clean-up patches. ```