Re: [PATCH v2 06/10] log: add default decoration filter

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Aug 4, 2022, 07:08 UTC
Message-ID: <220804.86iln8e9hl.gmgdl@evledraar.gmail.com>
In-Reply-To: <bec532fb8c63b3ae784d442f438687a4f0bbad37.1659122979.git.gitgitgadget@gmail.com>

On Fri, Jul 29 2022, Derrick Stolee via GitGitGadget wrote:

> From: Derrick Stolee <derrickstolee@github.com>

Thanks for the re-roll in general, there's a lot of good stuff here & I hope I find more time to comment on it in more detail, but just focusing on things that would be hard to back out of once changed:

Show 24 quoted lines

> [...]
> Another alternative would be to exclude the known namespaces that are
> not intended to be shown. This would reduce the visible effect of the
> change for expert users who use their own custom ref namespaces. The
> implementation change would be very simple to swap due to our use of
> ref_namespaces:
>
> 	int i;
> 	struct string_list *exclude = decoration_filter->exclude_ref_pattern;
>
> 	/*
> 	 * No command-line or config options were given, so
> 	 * populate with sensible defaults.
> 	 */
> 	for (i = 0; i < NAMESPACE__COUNT; i++) {
> 		if (ref_namespaces[i].decoration)
> 			continue;
>
> 		string_list_append(exclude, ref_namespaces[i].ref);
> 	}
>
> The main downside of this approach is that we expect to add new hidden
> namespaces in the future, and that means that Git versions will be less
> stable in how they behave as those namespaces are added.

I see that as a feature, and not a downside. If we simply explain this in the documentation as e.g.:

	When adding decorations git will by default exclude certain
	"internal" ref namespaces that it treats specially, such as
	refs/magical-1/*, refs/magical-2/* (or whatever). Other such
	special namespaces may be reserved in the future.

There's no lack of "stability", because the ref hiding only act on what's known to be something we can ignore, because our git version knows about it.

If git v2.40 knows about refs/magical-1/* but not refs/magical-2/*, but git v2.50 knows about both it's not a lack of stability that v2.40 shows one decorated by default, but v2.50 shows neither.

To v2.40 one of them isn't a magical "I know what this is, it's internal & I can hide it" ref.

I'm aware that we disagree, and some of this was discussed in v1. I'm not intending to just repeat what I said before.

But it's not just that I disagree, I genuinely don't get your POV here. We:

 * Know that we have (admittedly probably rare) in-the-wild use of
   non-standard and custom namespaces, and that this series would change
   long-standing log output.

   If I could see a good reason to change the existing "log" behavior
   here I'd probably be sold on it. We try not to change existing output
   in general, but this part isn't "plumbing", and arguably not a very
   "stable" part of the log output either (decorations being optional
   etc).

   But it does rub me the wrong way to sell a change in the name of
   "stability" when it's from the outset doing the exact opposite,
   to....

 * ... are willing to make that one-time change so that we can have
   stability for users that are relying on "decorate" working
   consistently across versions once we're in the new world order,
   because we *might* add new magical refs.

Since the latter group of users don't exist today by definition (this having not been integrated) why isn't it a better win-win solution to give those users some --decorate-only-known-refs option/config?

They'd get their "stability" going forward, and without the needless breaking of existing behavior, no?

Show 14 quoted lines

> +test_expect_success 'log --decorate does not include things outside filter' '
> +	reflist="refs/prefetch refs/rebase-merge refs/bundle" &&
> +
> +	for ref in $reflist
> +	do
> +		git update-ref $ref/fake HEAD || return 1
> +	done &&
> +
> +	git log --decorate=full --oneline >actual &&
> +
> +	for ref in $reflist
> +	do
> +		! grep $ref/fake actual || return 1
> +	done

I haven't tested, but isn't that last for-loop replacable with:

	! grep /fake actual

Or do we have other "/fake" refs we want to include?