Re: [PATCH v2 1/2] diff: consolidate diff algorithm option parsing

From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: "John Cai" <johncai86@gmail.com>,
	"John Cai via GitGitGadget" <gitgitgadget@gmail.com>,
	git@vger.kernel.org, "Eric Sunshine" <sunshine@sunshineco.com>,
	"Phillip Wood" <phillip.wood123@gmail.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Elijah Newren" <newren@gmail.com>
Subject: Re: [PATCH v2 1/2] diff: consolidate diff algorithm option parsing
Date: Wed, 15 Feb 2023 21:14:25 -0500	[thread overview]
Message-ID: <Y+2RgdCDrDC1N9Y1@coredump.intra.peff.net> (raw)
In-Reply-To: <xmqqmt5ev6ln.fsf@gitster.g>

On Wed, Feb 15, 2023 at 03:42:12PM -0800, Junio C Hamano wrote:

> And I think that is a reasonable way to use callback to do "more
> than just setting a bit".  Even in that case, I am not sure if it is
> a good idea to share the same callback that has conditional code
> that only is relevant to the "patience" case, though.

I have to admit I did a double-take at the string comparisons with
opt->long. I guess we can rely on it, since it is coming from the
options struct (and is not the string that the user wrote on the command
line!). But it still feels a little error-prone, just because there's no
help from the type system or the compiler.

I'd have expected it with individual callbacks like:

  int handle_patience(...)
  {
	do_patience_specific_stuff();
	do_shared_stuff(PATIENCE_DIFF);
  }

  int handle_histogram(...)
  {
	do_shared_stuff(HISTOGRAM_DIFF);
  }

and so on. That's a bit more verbose, but the call stack reflects the
flow we expect.

I can live with it either way, though.

-Peff