Re: [PATCH 00/17] tests: access bare repositories explicitly

[Junio C Hamano <gitster@pobox.com>]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> Shoudln't there be a patch [01/18] before everything else that
>> updates Documentation/BreakingChanges.adoc to propose the default
>> change?
>
> Well, yes and no. There should be an update to BreakingChanges to propose
> that default change, but obviously it should not only be a documentation
> change: It should also adjust `Documentation/config/safe.adoc` to mention
> the intended change of behavior, and it should introduce
> `WITH_BREAKING_HANGES`-specific conditional code in `setup.c`.

Ah, very true.

And of course we'd need some tests conditional to breaking-changes
prerequisite in the meantime, to make sure that the default is to
allow "all" before breaking changes, and with the prereq the test
checks the new behaviour is to deny by default.

As to the changes to existing tests (i.e., this patch series started
doing), they have a bit of balancing act.

 * Most importantly, they need to make sure their indivial
   operations, when working on a bare repository is allowed in any
   unspecified means, continue to behave sensibly.  "git log -p" in
   a bare repository should still produce patches, "git bisect" in a
   bare repository should work and assume --no-checkout, etc.

 * Then all of them would need to be somehow told that working on a
   bare repository is allowed in suitable way.  For most of then it
   should be done by setting safe.bareRepository in the global
   scope, just like many tests specify the default branch name
   upfront just once (this is a tangent, but we should find a way to
   get rid of GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME variable support
   in our binary and instead set it up in the test environment's
   ~/.gitconfig), some may add --git-dir=., etc.  But the primary
   focus with this change is to preserve what individual operation
   each test wants to validate (see above).

 * There may need to be some tests behind the WITH_BREAKING_CHANGES
   prerequisite that makes sure that the access to the repository is
   blocked without the configuration loosening the condition, but
   they hopefully would be minimum---unlike "how this particular
   command should operate in a bare repository?" that vary per
   command, "no command should work in a bare repository unless ..."
   that is enforced in the setup phase do not have to validate each
   and every command.

> And, crucially, it should pass the test suite under WITH_BREAKING_CHANGES.
>
> To do that, a lot more needs to happen than just the 17 patches in this
> here patch series. Most of the additional changes are quite mechanical,
> and can be validated relatively easily despite their sheer number. And
> only at the very end of those changes can that `safe.bareRepository`
> change even be proposed.

I agree with all the flow you propose here.  And for that process, I
think the early parts of the effort (i.e., there ~20 patches) that
prepare existing tests that make sure the operations of individual
commands in bare repositories should be kept to bare minimum by
freezing the world to allow bare repositories, just like initial
branch names are frozen to either 'master' or 'main' in many test
scripts.  Adding "--git-dir=." (regardless of the "-C there") you
proposed in the other thread is a valid way (but it may be tedious
to do and verify); doing "git config --global safe.bareRepository
all" upfront, if it works, might be simpler.  Any approach that
would achieve what we want is fine.

> I plan on proposing that patch in due time, to start the discussion
> whether or not it is a good idea to change the default of
> `safe.bareRepository` in Git 3.0.

Yup, I do not think it is a bad thing in the longer term, even
though I suspect it might be a bit more disruptive than others
we have in the breaking changes document.


Thanks for working on this.