patch, 4 partstests: lib-functions: trivial style cleanups

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

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/test-lib-functions.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

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

On Mon, Nov 30, 2020 at 06:46:46PM -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.

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:

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:

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:

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:

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.

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.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t9902-completion.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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

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

Good eyes.  Any automation used here, or just mark I eyeballs?

On Tue, Dec 1, 2020 at 12:30 PM Junio C Hamano <gitster@pobox.com> wrote:

Nope. Just switching to a very old branch prompted me to look at the .gitignore file and I stumbled upon it.

On Tue, Dec 01, 2020 at 10:30:12AM -0800, Junio C Hamano 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

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

On Tue, Dec 1, 2020 at 8:26 PM Jeff King <peff@peff.net> wrote:

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

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

Hi René,

On Wed, 2 Dec 2020, René Scharfe wrote:

ACK! Dscho

On Wed, Dec 02, 2020 at 12:46:01PM +0100, René Scharfe wrote:

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

Jeff King <peff@peff.net> writes:

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.