patch, 4 partsSHA1DC fixes & fully moving to a git.git submodule

We've now had a couple of Git releases where we've used sha1collisiondetection/ if it's checked out, but have gracefully fallen back on sha1dc/ if it's not there.

This series makes it a hard requirement, without 4/4 you'll still be able to do NO_DC_SHA1_SUBMODULE=UnfortunatelyYes, but with it even the ability to do that is removed, i.e. we're fully on the submodule (unless you have it as an external library).

1/4 should be destined straight for inclusion since it's a bugfix to the existing logic, and 2/4 could tag along with it, but none of this is urgent, so I'd figured I'd sent it all as one series and see what people think.

Ævar Arnfjörð Bjarmason (4):
  Makefile: don't error out under DC_SHA1_EXTERNAL if
    DC_SHA1_SUBMODULE=auto
  sha1dc_git.h: re-arrange an ifdef chain for a subsequent change
  Makefile: use the sha1collisiondetection submodule by default
  sha1dc: remove in favor of using sha1collisiondetection as a submodule

 Makefile              |   29 +-
 sha1dc/.gitattributes |    1 -
 sha1dc/LICENSE.txt    |   30 -
 sha1dc/sha1.c         | 1900 -------------------------------------------------
 sha1dc/sha1.h         |  110 ---
 sha1dc/ubc_check.c    |  372 ----------
 sha1dc/ubc_check.h    |   52 --
 sha1dc_git.h          |    6 +-
 8 files changed, 14 insertions(+), 2486 deletions(-)
 delete mode 100644 sha1dc/.gitattributes
 delete mode 100644 sha1dc/LICENSE.txt
 delete mode 100644 sha1dc/sha1.c
 delete mode 100644 sha1dc/sha1.h
 delete mode 100644 sha1dc/ubc_check.c
 delete mode 100644 sha1dc/ubc_check.h

Fix a logic error in the initial introduction of DC_SHA1_EXTERNAL. If git.git has a sha1collisiondetection submodule checked out the logic to set DC_SHA1_SUBMODULE=auto would interact badly with the check for whether DC_SHA1_SUBMODULE was set.

It would error out, meaning that there's no way to build git with DC_SHA1_EXTERNAL=YesPlease without deinit-ing the submodule.

Instead, adjust the logic to only fire if the variable is to something else than "auto" which would mean it's a mistake on the part of whoever's building git, not just the Makefile tripping over its own logic.

1. 3964cbbb5c ("sha1dc: allow building with the external sha1dc
   library", 2017-08-15)
2. cac87dc01d ("sha1collisiondetection: automatically enable when
   submodule is populated", 2017-07-01)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 2 ++
 1 file changed, 2 insertions(+)

On Tue, Nov 28, 2017 at 09:32:11PM +0000, Ævar Arnfjörð Bjarmason wrote:

This all makes sense, and I agree your patch is an improvement.

One minor whitespace nit:

The indentation here is funky. Unfortunately I think $(error) can't be tab-indented, so it has to either stay at the left-most side, or we have to use spaces.

But the ifneq/endif pair can be indented. Ordinarily I'd say it's fine to keep it at the outermost (because of the weird error indent), but note that we're _inside_ an already-indented ifdef/endif pair. We should either stay inside there, or we should put the outer one to the left for consistency.

-Peff

A subsequent change will change the semantics of DC_SHA1_SUBMODULE in a way that would require moving these checks around, so start by moving them around without any functional changes.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 sha1dc_git.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

On Tue, Nov 28, 2017 at 09:32:12PM +0000, Ævar Arnfjörð Bjarmason wrote:

OK. This flips the priority, but we're assuming that the Makefile doesn't allow you to actually set both flags (at least not easily). And I think that is the case, even after your loosening of the error for "auto" in the previous patch, because the whole DC_SHA1_SUBMODULE code-path is in the "else" block for DC_SHA1_EXTERNAL. Good.

-Peff

Change the build process so that instead of needing to supply DC_SHA1_SUBMODULE=YesPlease to use the sha1collisiondetection submodule instead of the copy of the same code shipped in the sha1dc directory, it uses the submodule by default unless NO_DC_SHA1_SUBMODULE=UnfortunatelyYes is supplied.

This reverses the logic added by me in 86cfd61e6b ("sha1dc: optionally use sha1collisiondetection as a submodule", 2017-07-01). Git has now shipped with the submodule in git.git for two major releases, if we're ever going to migrate to fully using it instead of perpetually maintaining both sha1collisiondetection and the sha1dc directory this is a logical first step.

This change removes the "auto" logic Junio added in cac87dc01d ("sha1collisiondetection: automatically enable when submodule is populated", 2017-07-01), I feel that automatically falling back to using sha1dc would defeat the point, which is to smoke out any remaining users of git.git who have issues cloning the submodule for whatever reason.

Instead the Makefile will emit an error if the contents of the submodule aren't checked out (line-wrapped. GNU make emits this all on one line):

    Makefile:1031: *** The sha1collisiondetection submodule is not
    checked out. Please make it available, either by cloning with
    --recurse-submodules, or by running "git submodule update
    --init". If you can't use it for whatever reason you can define
    NO_DC_SHA1_SUBMODULE=UnfortunatelyYes.  Stop.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile     | 32 +++++++++++++++++++-------------
 sha1dc_git.h |  2 +-
 2 files changed, 20 insertions(+), 14 deletions(-)

On Tue, Nov 28, 2017 at 09:32:13PM +0000, Ævar Arnfjörð Bjarmason wrote:

I'm not sure how I feel about this. I see your point that there's no real value in maintaining two systems indefinitely. At the same time, I wonder how much value the submodule strategy is actually bringing us.

IOW, are we agreed that the path forward is to get everybody using the submodule?

It seems like it's going to cause some minor pain for CI and packaging systems that now need to care about submodules (so at least flipping the switch, but maybe also dealing with having a network dependency for the build that was not already there).

I'll admit I'm more sensitive to this than most people, since I happen to maintain a fork of Git that I run through an internal CI system. And that CI otherwise depends only on the locally-held fork, not any external resources. But I'm probably in a fairly unique situation there.

-Peff

[Forgot to CC Stefan & Brandon who were involved in previous discussions]

On Tue, Dec 05 2017, Jeff King jotted:

In no particular order:

 * I don't feel strongly about 2-4/4 in this series. I just hacked this
   up because it occurred to me that I'd left this sha1dc stuff in some
   in-between state and we'd talked about eventually moving forward with
   this.

   We've had two releases with the submodule being purely optional, if
   we're going to keep it it seems logical to start at least using it by
   default.

   The main thing I want is for the answer to "why do we have the same
   code twice in git.git" to not be "Ævar added a submodule and never
   followed up" :)

 * The main benefit of doing 3-4/4 is to get the git project itself to
   dogfood submodules & have the entire community enjoy the resulting
   fixes that'll come out of that. Not that it's a big bother for us to
   maintain the sha1dc/ & sha1collisiondetection/ directories and we
   need to have a submodule for our own use.

 * We'll never find out whether submodules are a hassle for downstream
   git.git consumers without something like 3/4, where you'll need to at
   least supply NO_DC_SHA1_SUBMODULE=UnfortunatelyYes so we'll get
   people coming out of the woodworks complaining that we've broken
   their workflows, right now with "auto" they won't even notice.

 * The network dependency for internal builds is a bit of a pain, but
   with this 3/4 you can still just supply
   NO_DC_SHA1_SUBMODULE=UnfortunatelyYes and it'll work. With 4/4 and a
   hard dependency it won't be so easy, you'll need to clone
   sha1collisiondetection internally somewhere and use url.X.insteadOf=Y
   to rewrite the submodule URL.

 * I forgot to note: Since git-archive doesn't include submodules
   (missing that feature) 4/4 is blocking on either just hacking the
   "make dist" target in git.git to monkeypatch it into the tarball (we
   already do this for other stuff), or making git-archive support a
   --recurse-submodules option.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Good.

With a need for a patch like 1/4, I suspect two release cycles is way too short for making a move like 2-4/4, though.

On Tue, Dec 05 2017, Junio C. Hamano jotted:

You're conflating two unrelated things, which to be fair I'm confusingly doing by submitting all this together.

1) Since 2.14 we've had the "auto" rule and
   DC_SHA1_SUBMODULE=[YesPlease|auto], so we'll prefer the submodule if
   it's there. So we've been testing if the mere presence of a
   .gitmodules breaks something for someone, seems like it doesn't.

2) Then in the 2.15 release Takashi Iwai submitted a feature to link to
   an external SHA1DC. This is used in the SuSE 2.15 package here:
   http://download.opensuse.org/tumbleweed/repo/src-oss/suse/src/

   However, as you'll see if you extract that package they don't run
   into that bug, because they're building it from a tarball which has
   an empty sha1collisiondetection/ directory as noted in my
   87bmjdscdr.fsf@evledraar.booking.com.

   Takashi *would* run into an error with my 1/4 if he was building from
   git.git, or if "make dist" included sha1collisiondetection/, but I
   don't see a reason to hold anything back back on that account. The
   only users of DC_SHA1_EXTERNAL=YesPlease are going to be packagers
   who know what they're doing, and if we start erroring out for them on
   this obscure option that's going to be trivially solved.

I don't see why this obscure edge case with #2 should keep us from deciding whatever we'd decide with #1. They're really unrelated, #2 practically speaking only impacts tarball consumers, #1 impacts git.git users.

It seems logical to me if we're going to move forward with #1 at all by first making the submodule the default & then depending on how that turns out making it a hard dependency, we'd do it now.

We'll learn nothing new by shipping a 2.16 with DC_SHA1_SUBMODULE=auto that we haven't already learned in 2.14 & 2.15.

On Tue, Dec 05, 2017 at 02:02:50AM -0500, Jeff King wrote:

To add to this point, package systems such as Alpinelinux and Archlinux (and probably others) work with released tarballs, not cloned repositories. For them, there is no easy way to get the submodule contents (the release tarballs would not contain it).

Once we would switch over to submodules only (because we do not want to maintain 2 separate systems), it would be a lot of hassle for those projects to get the sha1collisiondetection contents.

That's in my opinion a bigger disadvantage of submodules, commands like git archive do not support it, making it harder to get self-contained tarballs.

Perpahs there is a good solution to that problem, but then I'd like to hear it.

Kevin.

On Sat, Dec 09, 2017 at 01:30:14PM +0100, Kevin Daudt wrote:

I missed the v2 Ævar sent. I see that there `make dist` is adjusted to include sha1collisiondetection, so that would at least solve this problem.

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

I think it makes sense to drop 'auto' if we were to go this route. I do not think the right value for NO_DC_SHA1_SUBMODULE should be "unfortunately yes"; it should be spelled NoThanks or something. It's not like an external reason "unfortunately" prevents you from using the code from the submodule---the person sets it deliberately and by choice.

Likewise here.

Here's v2 as promised. Comments per-patch.

Ævar Arnfjörð Bjarmason (5):
  Makefile: don't error out under DC_SHA1_EXTERNAL if
    DC_SHA1_SUBMODULE=auto

Fixed indenting.

  Makefile: under "make dist", include the sha1collisiondetection
    submodule

NEW: Change "make dist" to include the sha1collisiondetection/ dir in
the tarball Junio's going to build when he makes releases, right now
we just ship an empty directory.

  sha1dc_git.h: re-arrange an ifdef chain for a subsequent change

No changes, trivial rewording of commit message.

  Makefile: use the sha1collisiondetection submodule by default

s/NO_DC_SHA1_SUBMODULE=UnfortunatelyYes/NO_DC_SHA1_SUBMODULE=NoThanks/ as requested by Junio.

Fix up wording of comment describing NO_DC_SHA1_SUBMODULE

Fix indenting.

  sha1dc: remove in favor of using sha1collisiondetection as a submodule

Reword & expand commit message.

Don't die if both NO_DC_SHA1_SUBMODULE=Y and DC_SHA1_EXTERNAL=Y are provided.

 Makefile              |   42 +-
 sha1dc/.gitattributes |    1 -
 sha1dc/LICENSE.txt    |   30 -
 sha1dc/sha1.c         | 1900 -------------------------------------------------
 sha1dc/sha1.h         |  110 ---
 sha1dc/ubc_check.c    |  372 ----------
 sha1dc/ubc_check.h    |   52 --
 sha1dc_git.h          |    6 +-
 8 files changed, 27 insertions(+), 2486 deletions(-)
 delete mode 100644 sha1dc/.gitattributes
 delete mode 100644 sha1dc/LICENSE.txt
 delete mode 100644 sha1dc/sha1.c
 delete mode 100644 sha1dc/sha1.h
 delete mode 100644 sha1dc/ubc_check.c
 delete mode 100644 sha1dc/ubc_check.h

Fix a logic error in the initial introduction of DC_SHA1_EXTERNAL. If git.git has a sha1collisiondetection submodule checked out the logic to set DC_SHA1_SUBMODULE=auto would interact badly with the check for whether DC_SHA1_SUBMODULE was set.

It would error out, meaning that there's no way to build git with DC_SHA1_EXTERNAL=YesPlease without deinit-ing the submodule.

Instead, adjust the logic to only fire if the variable is to something else than "auto" which would mean it's a mistake on the part of whoever's building git, not just the Makefile tripping over its own logic.

1. 3964cbbb5c ("sha1dc: allow building with the external sha1dc
   library", 2017-08-15)
2. cac87dc01d ("sha1collisiondetection: automatically enable when
   submodule is populated", 2017-07-01)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 2 ++
 1 file changed, 2 insertions(+)

A subsequent change will change the semantics of DC_SHA1_SUBMODULE in a way that would require moving these checks around, so start by moving them around without any functional changes to reduce the size of the subsequent patch.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 sha1dc_git.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Include the sha1collisiondetection submodule when running "make dist". Even though we've been shipping the sha1collisiondetection submodule[1] and using it by default if it's checked out[2] anyone downloading git as a tarball would just get an empty sha1collisiondetection/ directory.

Doing this automatically is a feature that's missing from git-archive, but in the meantime let's bundle this up into the tarball we ship. This ensures that the DC_SHA1_SUBMODULE flag does what's intended even in an unpacked tarball, and more importantly means we're building the exact same code from the same paths from git.git and from the tarball.

I am not including all the files in the submodule, only the ones git actually needs (and the licenses), only including some files like this would be a useful feature if git-archive ever adds the ability to bundle up submodules.

1. commit 86cfd61e6b ("sha1dc: optionally use sha1collisiondetection
   as a submodule", 2017-07-01)
2. cac87dc01d ("sha1collisiondetection: automatically enable when
   submodule is populated", 2017-07-01)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

While I can see that you are not including everything, but I do not see _why_ you chose to do so and hardcode the burden of maintaining the list of files we need to copy in the Makefile.

This is much better than shipping a tarball that would not build at the endgame stage, of course ;-)

Thanks.

On Fri, Dec 08 2017, Junio C. Hamano jotted:

I started by trying to come up with something generic which would handle future submodules, i.e.:

    git submodule foreach 'git ls-files'

However, unlike the C programs ./git-submodule will bark about missing shell stuff when not installed, and the "dist" target already use ./git-*.

Between that and someone using git.git probably never running sha1collisiondetection/ itself, it seemed fine just to hardcode the couple of things we needed, which are very unlikely to change.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

I am not all that interested in that. The hardcoded list I felt disturbing was not "what are the submodules we want to include?", but "what are the files from sha1dc submodule we use?".

IOW, I was more wondering about these selective copies:

... + @cp sha1collisiondetection/LICENSE.txt \ + $(GIT_TARNAME)/sha1collisiondetection/ + @cp sha1collisiondetection/LICENSE.txt \ + $(GIT_TARNAME)/sha1collisiondetection/ + @cp sha1collisiondetection/lib/sha1.[ch] \ + $(GIT_TARNAME)/sha1collisiondetection/lib/ + @cp sha1collisiondetection/lib/ubc_check.[ch] \ + $(GIT_TARNAME)/sha1collisiondetection/lib/ ...

As we are assuming that DC_SHA1_SUBMODULE users are not doing the make dist from a tarball extract, wouldn't something along the lines of

	git -C sha1collisiondetection archive HEAD | \
	$(TAR) Cxf $(GIT_TARNAME)/sha1collisiondetection -

be a better way to go?

Change the build process so that instead of needing to supply DC_SHA1_SUBMODULE=YesPlease to use the sha1collisiondetection submodule instead of the copy of the same code shipped in the sha1dc directory, it uses the submodule by default unless NO_DC_SHA1_SUBMODULE=NoThanks is supplied.

This reverses the logic added by me in 86cfd61e6b ("sha1dc: optionally use sha1collisiondetection as a submodule", 2017-07-01). Git has now shipped with the submodule in git.git for two major releases, if we're ever going to migrate to fully using it instead of perpetually maintaining both sha1collisiondetection and the sha1dc directory this is a logical first step.

This change removes the "auto" logic Junio added in cac87dc01d ("sha1collisiondetection: automatically enable when submodule is populated", 2017-07-01), I feel that automatically falling back to using sha1dc would defeat the point, which is to smoke out any remaining users of git.git who have issues cloning the submodule for whatever reason.

Instead the Makefile will emit an error if the contents of the submodule aren't checked out (line-wrapped. GNU make emits this all on one line):

    Makefile:1031: *** The sha1collisiondetection submodule is not
    checked out. Please make it available, either by cloning with
    --recurse-submodules, or by running "git submodule update
    --init". If you can't use it for whatever reason you can define
    NO_DC_SHA1_SUBMODULE=NoThanks.  Stop.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile     | 34 ++++++++++++++++++++--------------
 sha1dc_git.h |  2 +-
 2 files changed, 21 insertions(+), 15 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Sounds OK.  

But I actually do not mind to (and may even prefer to) have an endgame opposite of what this series tries to lead us to. We've tried to have this as submodule, we've seen that the arrangement works, and now we declare victory and get rid of the submodule.

That endgame allows us not force people to grab an essential part of the codebase as an external dependency from another place, which feels quite bad, especially when their primary interest is not in dogfooding submodule but in building a working version of Git.

Removing one and keeping the other between the two will make the distribution more streamlined by removing the build-time knob we need to tweak, but the one that gets removed does not have to be the in-tree one that allows people to "git clone" from just one place.

Junio C Hamano <gitster@pobox.com> writes:

Perhaps this may deserve a bit more explanation.

I wouldn't be so against "let's do submodule-only" if this were not SHA-1 implementation but something like gitk and git-gui. An optional part of a system that it is safe to leave to individual builders if they want to fetch and use that part *is* an ideal target to bind as a submodule to the system.

It's just the "default SHA-1 implementation" is at the far opposite end of the spectrum from "an optional part", and our use of submodule to bind this code is definitely *not* because it makes sense to use submodule in that context; it is because developers (not necessarily those who consume Git sourcecode) *wanted* to use submodule there, regardless of the real merit of doing so.

And that is why I do not feel entirely happy with the step 4/5 and also I feel moderately strongly against the step 5/5. These two steps have a "developer first" smell that disgusts me.

On Fri, Dec 08, 2017 at 03:21:23PM -0800, Junio C Hamano wrote:

Thanks for writing this out. I had a vague feeling that I didn't quite like going the submodule-only direction, but I was having trouble putting into words. It's exactly this.

-Peff

On Fri, Dec 08 2017, Junio C. Hamano jotted:

I don't think we can say we tried without having this 4/5 (5/5 not needed) in a couple of releases. Without this series we always smoothly fall back to the non-submodule, and only use it if you opt in.

So all we've really tested so far is:

 * CI systems that consume git.git and provide --recurse-submodules to
   git-clone by default.

 * People on this list that have gone out of their way to test by
   manually toggling the the flag.

As noted previously my two motivations are:

 1) That we decide what we want to do with this, ultimately I don't
    really mind which way we go.

 2) That if we go with the submodule by default, it should be understood
    that one of the main benefits is us *actually* dogfooding it and
    subsequently improving it for all git users.

What you're describing here is a great example of #2, and also a way of making my point above that we really haven't tried submodules in git.git yet, since you're just worrying about this issue now that using it would the default.

This is a UX issue with submodules that I agree sucks and there's no reason for why it couldn't be solved. E.g. one solution is that submodules could have something like:

    [submodule "sha1collisiondetection"]
            path = sha1collisiondetection
            url = https://github.com/cr-marcstevens/sha1collisiondetection.git
            branch = master
            localbranch = sha1collisiondetection/master

Where the localbranch would be git.git's own copy in a branch of the the sha1collisiondetection/ commit. Then when you update the ref sha1collisiondetection/ points to it would also update the sha1collisiondetection/master branch (and warn/die when you push git.git master but not that branch).

This would solve offer a solution to this submodule UX issue, but more importantly I think the likelyhood of such a patch (and others) actually being written is going to go up *significantly* if the git project itself is dogfooding the feature, with exhibit A being that you're already annoyed by it :)

On Fri, Dec 08, 2017 at 10:29:56PM +0000, Ævar Arnfjörð Bjarmason wrote:

Is this commit missing from the mailing list because the e-mail is too large?

On Tue, Nov 28, 2017 at 09:32:14PM +0000, Ævar Arnfjörð Bjarmason wrote:

diff.orderFile, I think.

I wonder if that might run afoul of programs which try to apply patches in order. I think "git apply" is smart enough to apply all deletions (regardless of order) before any creations (so it cannot be fooled by adding "foo" followed by deleting "foo/bar"). So maybe it's fine.

-Peff