On Fri, Jan 28 2022, Elijah Newren wrote:

Hi Ævar,

On Thu, Jan 27, 2022 at 11:54 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
[...]

quoted

quoted

quoted

Technically, before commit 3050b6dfc75d, repo-settings would treat any

fetch.negotiationAlgorithm value other than "skipping" or "noop" as a
request for "default", but I think it probably makes more sense to
ignore such broken requests and leave fetch.negotiationAlgorithm with
the default value rather than the value of "default".  (If that sounds
confusing, note that "default" is usually the default value, but when
feature.experimental=true, "skipping" is the default value.)

[...]
    A long sidenote about naming things "default":

    Many years ago, in the Gnome community, there was a huge fight that
    erupted, in part due to confusion over "default". There was a journalist
    who had been a designer in a past life, who had a little friction with
    the rest of the community, but intended well and generally improved
    things. At some point, they suggested some changes to improve the
    "default" theme (and they were a nice improvement), but not being a
    developer the changes weren't communicated in the form of a patch. And
    the changes accidentally got applied to the wrong theme: the default one
    (yes, there was a theme named "default" which was not the default
    theme). Now, basically no one used the default theme because it was so
    hideously ugly. I think we suffered from a case of not being able to
    change the default (again?) because no one could get an agreement on
    what the default should be. Who did actually use the default theme,
    though? The person writing the release notes (though they only used it
    for taking screenshots to include in the release notes, and otherwise
    used some other theme). So, with people under pressure for an imminent
    release, there were screenshots that looked like garbage, and
    investigation eventually uncovered that it was due to changes that were
    meant for the "default" theme having accidentally been applied to the
    default theme. It could have just been an amusing story if not for the
    other unfortunate factors happening around the same time and the heated
    and protracted flamewars that erupted.

    Don't name settings/themes/things "default" if it describes something
    specific, since someone may come along and decide that something else
    should be the default, and then you're stuck with a non-default
    "default". Sadly, the name was already picked and documented so for
    backward compatibility we need to support it...

Funny story, I think this is only going to bite us if we don't switch
the default over along with promoting this out of feature.experimental.

I.e. =default should always be equivalent to not declaring that config
at all anywhere, and not drift to being a reference to some name that
happens to be "default", as in the GNOME case.

No, we have the same problem as the Gnome case.  See this part of the
documentation for fetch.negotiationAlgorithm:

"""
    The default is "default" which instructs Git to use the
    default algorithm that never skips commits (unless the server has
    acknowledged it or one of its descendants).
"""

features.experimental turns on "skipping" as the default behavior, and
that text clearly rules out the possibility that "default" could be
used to mean "skipping".  So, if that experimental feature graduates,
then the default behavior of fetch.negotiationAlgorithm will NOT be
the "default" behavior of fetch.negotationAlgorithm.

I see what you mean, and I'm aware that I'm debating this with a native
speaker :)

FWIW I didn't read it that way, earlier it discusses "skipping", and
here it's describing what the default is. But especially since you'd
have no reason to set this except to "reset to default" I didn't take it
to be a promise that the default wouldn't change.

I.e. maybe we'll just make it "skipping" and drop the current "default"
code, or we'll give the current "default" a name at that point.

But I do see how us not having a name for the "defult" complicates that
view of the world. For grep.patternType we've got the same thing, but
"default" there is "basic", so that's a bit different.

I do read log.date's "default" as being the sort of GNOME case you're
describing however. But I don't think we'd ever change the default
there, a date format is too subjective, whereas an internal algorithm is
liable to change.

But I think we should just change this to make it explicit (separate
from this narrow bugfix). Maybe "exhaustive" would be a good permanent
name for the default algorithm?

quoted

In our case it's more of a story about the inconsistencies in our config
space, i.e. some values you can't reset at all, some take empty values
to do so, others "default" etc.
[...]

Since it's the same as the preceding test, maybe we can squash this in
to avoid the duplication? This works for me.
[...]
-       rm -f trace &&
-       cp -r client clientv2 &&
-       GIT_TRACE_PACKET="$(pwd)/trace" git -C clientv2 -c protocol.version=2 \
+test_expect_success 'use ref advertisement to prune "have" lines sent' '
+       test_negotiation_algorithm_default \
                -c feature.experimental=true \
-               -c fetch.negotiationAlgorithm=default \
-               fetch origin server_has both_have_2 &&
-       grep "have $(git -C client rev-parse client_has)" trace &&
-       grep "have $(git -C client rev-parse both_have_2)" trace &&
-       ! grep "have $(git -C client rev-parse both_have_2^)" trace
+               -c fetch.negotiationAlgorithm=default

I think you accidentally dropped one of the two tests by turning it
into a function and then only calling it for the latter usage and not
the former, but I get your idea.  It makes sense; I'll make the
change.

Ah yes, oops. Yes we should clearly have the non-"-c [...]" case too
(and it's what I was aiming for) :)