[PATCH 0/4] Random cleanups

[PATCH 0/4] Random cleanups

[Felipe Contreras]

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

[PATCH 1/4] refspec: trivial cleanup

[Felipe Contreras]

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(-)

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

Re: [PATCH 1/4] refspec: trivial cleanup

[Jeff King]

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

Re: [PATCH 1/4] refspec: trivial cleanup

[Junio C Hamano]

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.

Re: [PATCH 1/4] refspec: trivial cleanup

[Felipe Contreras]

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.

-- 
Felipe Contreras

Re: [PATCH 1/4] refspec: trivial cleanup

[Jeff King]

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

Re: [PATCH 1/4] refspec: trivial cleanup

[Felipe Contreras]

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.

-- 
Felipe Contreras

[PATCH 2/4] .gitignore: remove dangling file

[Felipe Contreras]

The library was removed 7 years ago on commit ae34ac126f. But not from
the .gitignore file.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 .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

Re: [PATCH 2/4] .gitignore: remove dangling file

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> The library was removed 7 years ago on commit ae34ac126f. But not from
> the .gitignore file.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---

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

Re: [PATCH 2/4] .gitignore: remove dangling file

[Felipe Contreras]

On Tue, Dec 1, 2020 at 12:30 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> > The library was removed 7 years ago on commit ae34ac126f. But not from
> > the .gitignore file.
> >
> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > ---
>
> 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

Re: [PATCH 2/4] .gitignore: remove dangling file

[Jeff King]

On Tue, Dec 01, 2020 at 10:30:12AM -0800, Junio C Hamano wrote:

> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > The library was removed 7 years ago on commit ae34ac126f. But not from
> > the .gitignore file.
> >
> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > ---
> 
> 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 <peff@peff.net>
---
 .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

Re: [PATCH 2/4] .gitignore: remove dangling file

[Felipe Contreras]

On Tue, Dec 1, 2020 at 8:26 PM Jeff King <peff@peff.net> 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

[PATCH] gitignore: remove entry for git serve

[René Scharfe]

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 <l.s.r@web.de>
---
 .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

Re: [PATCH] gitignore: remove entry for git serve

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 708 bytes --]

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 <l.s.r@web.de>

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
>

Re: [PATCH] gitignore: remove entry for git serve

[Jeff King]

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

Re: [PATCH] gitignore: remove entry for git serve

[Junio C Hamano]

Jeff King <peff@peff.net> 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.

[PATCH 3/4] test: completion: fix typos

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 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

[PATCH 4/4] tests: lib-functions: trivial style cleanups

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 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