dune pkg: improve behavior when available git is too old

[mbarbin]

When trying the dune developer preview I had to do a manual upgrade of git. The git that's packaged by ubuntu 20.04.6 LTS is too old for dune pkg.

$ /usr/bin/git --version
git version 2.25.1

dune pkg lock resulted in an internal error for me, displaying the "I must not crash" poem.

$ dune pkg lock
Internal error, please report upstream including the contents of _build/log.
Description:
  ("git returned non-zero exit code",
   { exit code = 129
   ; dir = External "/home/mathieu/.cache/dune/git-repo"
   ; git = External "/usr/bin/git"
   ; args =
       [ "fetch"
       ; "--no-write-fetch-head"
       ; "https://github.com/ocaml-dune/opam-overlays.git"
       ; "9bad4501e6dd4feb7b61b48d29812241d1d711f9"
       ]
   ; output = []
   })
Raised at Stdune__Code_error.raise in file
  "otherlibs/stdune/src/code_error.ml", line 10, characters 30-62
Called from Fiber__Core.O.(>>|).(fun) in file "vendor/fiber/src/core.ml",
  line 253, characters 36-41
Called from Fiber__Scheduler.exec in file "vendor/fiber/src/scheduler.ml",
  line 76, characters 8-11
Re-raised at Stdune__Exn.raise_with_backtrace in file
  "otherlibs/stdune/src/exn.ml" (inlined), line 38, characters 27-56
Called from Stdune__Exn_with_backtrace.reraise in file
  "otherlibs/stdune/src/exn_with_backtrace.ml", line 20, characters 33-71
Called from Fiber__Scheduler.exec in file "vendor/fiber/src/scheduler.ml",
  line 76, characters 8-11

I must not crash.  Uncertainty is the mind-killer. Exceptions are the
little-death that brings total obliteration.  I will fully express my cases. 
Execution will pass over me and through me.  And when it has gone past, I
will unwind the stack along its path.  Where the cases are handled there will
be nothing.  Only I will remain.

I think in this git version, git fetch doesn't know the --no-write-fetch-head flag.

It would be nice if dune warned you somehow, instead of throwing some internal error. What's the minimal version that dune pkg requires?

Upgrading git to 2.46.2 fixed it, I am not blocked on this.

[rgrinberg]

Verifying that this flag is available and erroring out if it's absent sounds like the right thing to do.

[Leonidas-from-XIV]

We could also drop the flag, the difference between writing the fetch head and not doing it is marginal.

[rgrinberg]

It's not marginal. It's what allows us to write git commands concurrently without a lock.

[Leonidas-from-XIV]

If concurrent git processes overwrite the FETCH_HEAD in an way that produces races, is it really an issue? We never use the fetch head.

[rgrinberg]

IIRC, they wouldn't overwrite but error out saying that the git lock is already taken and the process is unable to write

[Leonidas-from-XIV]

Ah, ok. I've tried looking at the man page and it unhelpfully only states:

       --[no-]write-fetch-head
           Write the list of remote refs fetched in the FETCH_HEAD file directly under $GIT_DIR. This is the default. Passing --no-write-fetch-head from the command line tells Git not to write the file. Under --dry-run option, the file is never written.

No mention of index.lock but then again the whole manpage is extremely vague on how concurrency-safe it is.

[Leonidas-from-XIV]

From a glance of Git's fetch.c it seems that the FETCH_HEAD handling is really just about writing the file in append mode, without any difference to locking:

https://github.com/git/git/blob/e9356ba3ea2a6754281ff7697b3e5a1697b21e24/builtin/fetch.c#L1058-L1062

(see also open_fetch_head and do_fetch where it is used; the difference I see is whether fetch_head is NULL or not; in the latter case the operations on the fetch head turn into no-ops)

[rgrinberg]

Hmm, I guess there was some other reason. Maybe some other command that we run reads FETCH_HEAD and at the very least expects it to be not corrupted?

Is there a particular old version of git that you'd like to support? I personally don't really see the point. I'd rather lean on newer version of git further to make this feature work better rather than being bogged by compatibility right off the start.

At the very least, corrupting the FETCH_HEAD has the possibility of making various git debugging commands fail inside this repo.

[mbarbin]

Thank you for looking into it. Just an aside, when I wrote:

I think in this git version, git fetch doesn't know the --no-write-fetch-head flag.

this was what I concluded from a quick glance at the --help page, but I wasn't very thorough. I did not look into git changelogs nor did I verify this hypothesis with certainty. Just mentioning it, as I would feel embarrassed if I steered you into chasing the wrong thing and you just took my word for it.

From a naive user's standpoint, I think if dune is clear about its git dependency requirement, that seems fine to me. Something like a page in the doc that lists the system dependencies with their version constraints?

[Leonidas-from-XIV]

Is there a particular old version of git that you'd like to support?

Not specifically; I generally think that supporting whatever is available in current stable/LTS versions is a fair starting point to be usable for a large amount of users.

That said, Ubuntu 20.04 is the previous-previous-LTS and even from a Debian stable perspective the version to support would be Git 2.39. I'd prefer to avoid launching an extra process per run to see if the flag is supported as for a growing amount of users that will always be "yes" and making users pay the price seems wasteful.

@mbarbin Yeah, it is probably --no-write-fetch-head. It matches the error code 129 for unknown arguments. 20.04 ships with 2.25 and according to https://github.com/git/git/commit/887952b8c680626f4721cb5fa57704478801aca4 the feature was added in Git 2.29. If you feel like, you can remove the argument and recompile to check.

[rgrinberg]

If launching a process is wasteful, writing and overwriting this file is even more wasteful.

Supporting git from ubuntu's LTS would be a fair policy IMO. Going older would make me weary, we've already wasted so much time supporting older versions of OCaml. I'd hate to repeat this mistake.

[Leonidas-from-XIV]

In such case we should just document that you need Git 2.29+ which came out about four years ago and I assume has made its way into the most common distributions.