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

["Ævar Arnfjörð Bjarmason" <avarab@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:

> [...]
> 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?

> +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?