patchdoc: set actual revdate for manpages

manpages expect the date of the last revision, if that is not found DocBook Stylesheets go through a series of hacks to generate one with the format `%d/%d/%Y` which is not ideal.

In addition to this format not being standard, different tools generate dates with different formats.

There's no need for any confusion if we specify the revision date, so let's do so.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---

This patch requires [1] to actually work, and has a simple conflict with [2], so it's written on top of both.

[1] https://lore.kernel.org/git/20230323221523.52472-1-felipe.contreras@gmail.com/ [2] https://lore.kernel.org/git/20230408001829.11031-1-felipe.contreras@gmail.com/

 Documentation/Makefile | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

On Thu, Apr 13, 2023 at 01:47:22AM -0600, Felipe Contreras wrote:

That seems like a good goal, and should reduce our asciidoc/asciidoctor diff considerably.

I wasted a bit of time trying this out, so let me elaborate on "actually work" for the benefit of other reviewers. Without the patch in [1] (which is 8806120de6c on fc/remove-header-workarounds-for-asciidoc), this patch works as advertised with asciidoctor, but has no effect with asciidoc. The reason is that asciidoc puts the <date> tags in the header, and the custom header removed by 8806120de6c suppresses asciidoc's default header entirely (so a workaround would be to include the <date> tags in our custom header, but I don't see any reason not to just build on top of 8806120de6c, as you did here).

What will/should this do in a distribution tarball, where we won't have a Git repository at all? I think we'll just end up with a blank date in the xml file, though it looks like docbook turns that into today's date in the result.

That's not _too_ bad, but feels a bit inconsistent (and it uses the format you're trying to get rid of!).

It would be nicer to populate the date variable in that case, like we do for GIT_VERSION. I think that could look something like this:

Jeff King wrote:

Yes, I also "wasted" a bit of time comparing the custom header introduced in 7ef195ba3e (Documentation: Add version information to man pages, 2007-03-25) to what is in asciidoc and the difference is simple:

Felipe Contreras <felipe.contreras@gmail.com> writes:

Seeing Peff's change, it sounds so small a thing that it feels a bit unnatural not to do that in the same series, at least to me.

Having said that, I think that "we make progress one step at a time" is perfectly acceptable and may even be preferred, as long as the formatted manpages from the tarball would not change between with and without this patch. That way, we make the output from a repository better while keeping the output from a tarball extract intact, and make the latter match the former in a later effort.

So, I "wasted" (not really---this was a fruitful validation that is a part of a review process) some time to play with this on top of 'seen' to see how well the tarball extract fares. We get an error message from "git show" complaining about "not a git repository" but that is to be expected ("sh GIT-VERSION-GEN" does not share the problem, though).

At least with the versions of toolchain picked by default in my environment, I seem to be getting identical "04/14/2023" in a directory extracted out of a tarball taken from 'seen' (with and without this patch) in the formatted manpages, because we do not have any record in the input either way.

Formatted output from a repository working tree changes from "04/14/2023" to "2023-04-13". The value change may be intended, but I am not sure if the format change was intended or even welcome. If we want to correct the date format, it can totally be done in a separate patch, or a separate series even, with some justification in the proposed log message, I think.

Thanks.

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

Forgot to mention another thing. While it may be a good idea to tie the datestamp etched in the formatted result to that of the source material, rather than the date the formatter happened to have been run, the committer date is more appropriate than the author date for that purpose, as the former is the date that the change made on the latter date (which is earlier) has become a part of the whole, from which the formatted result was produced.

It may not make a big practical difference:

 * For an individual who is trying out the changes just made, the
   committer time and the author time are likely identical.

 * For a release process, what is at the tip of the released branch
   is likely be the release notes and version bump, recorded by the
   releaser, and again the committer time and the author time are
   likely identical.

 * For results of a pull request, the times are likely identical for
   the merge commit.

but from the philosophical standpoint, it does matter. Using the committer time would give us one more justification to use one single timestamp from the commit, when people complain "this manual page, as opposed to the other one that was changed in the latest release, has not seen any need to be updated for the past 3 years, yet the formatted output for these two manual pages carry the same date".

Junio C Hamano wrote:

Or _any_ practical difference.

Agreed that committer == author in these cases.

I prefer to focus on the real rather than hypothetical (or philosophical).

I do create my own releases (e.g. 2.40.0+fc1) and a real issue with the version script (rather than philosphical) is that it only considers annotated tags, so I have to carry a patch that adds `--tags` to the `git describe` command.

Shouldn't dealing with real issues of real people have a higher priority than philosophical issues?

For the record, I do agree the committer date feels more proper, but it doesn't make any real difference, I just wonder about the priorities.

Cheers.

Junio C Hamano wrote:

It should be a small thing, but the GIT-VERSION-GEN script has not been updated since 2008 and has a lot of issues (from my point of view).

I think it should be cleaned up first (I already sent a patch series for that), *then* it would be trivial to add another field.

But that would add another (unncessary) dependency to this series.

I live in the vast majority of countries where MM/DD/YYYY makes absolutely no sense, so I think it's a plus if any man pages have a sane date.

I'm pretty sure I'm not alone on this, but some might disagree.

Sure, but in order to do that we would need a way to convert the sane standard git dates to USA-centric dates, and I'm not so sure the tidyness of having both dates wrong is worth it.

Would you consider something like this more welcome?

  GIT_DATE := $(shell git show --quiet --pretty='%as' | sed -e 's|^\(.\+\)-\(.\+\)-\(.\+\)$|\2/\3/\1|'

On Fri, Apr 14, 2023 at 09:46:19AM -0700, Junio C Hamano wrote:

The word "wasted" is appearing in a lot of people's emails in this thread. ;) So let me just clarify that in the original I meant that I spent time puzzling over why it was not working, which was a waste because Felipe had already said it required the other patch.

It definitely would have helped if he had explained _why_ the other patch was necessary. But to be fair, the fault was at least as much mine for not heeding what he did say.

But reviewers reproducing and tinkering is most certainly not a waste of time in general, and I didn't mean to imply it was (or that the patch itself was a waste of time).

I think the change is welcome and intended. I would not mind seeing the two changes (format change, and using commit date versus "now") conceptually split, but I think it's much more tangled. Asciidoctor is already producing y-m-d dates, and python asciidoc is using m/d/y. Changing the latter requires passing in not just a format but the actual date. If we want it to be the current date, then we have to get that from somewhere, which may introduce portability questions (e.g., can we rely on "date"?).

So doing it all in one patch, though this conversation may indicate that the commit message could do a better job of explaining the goal and implications.

-Peff

Jeff King <peff@peff.net> writes:

I can read both formats, and if we are moving in the direction to be consistent between AsciiDoc and asciidoctor, that is good. As long as the design decision is documented well in the proposed commit log message, I would not complain ;-).

Thanks.

On Fri, Apr 14, 2023 at 06:40:05AM -0600, Felipe Contreras wrote:

Sometimes, as long as it is not making the status quo worse in the meantime. In this case, if the tarball build continues to use the current date (which it appears to), then it's not changing. It does become inconsistent with the output you get from building in-repo, which is unfortunate. But that might be acceptable.

If so, it would make sense to document that decision in the commit message. We'd also want to suppress stderr from the Git invocation so that tarball builders don't see a confusing error message. And probably we should avoid using the "git show" porcelain in a script. I think:

  git rev-list -1 --no-commit-header --format=%as HEAD

would do what you want (or %cs; I don't have a strong opinion there).

Yeah, that should be fine. The original could probably be using "read" in the first place. In general it's worth micro-optimizing this script because it runs on every single "make" invocation, though this code path in particular is only run for tarball builders (who presumably build a lot less than developers).

It might also be worth just putting the two fields in two separate files, if there's any question of spaces in the version field.

-Peff

Jeff King <peff@peff.net> writes:

I agree building on what is in-flight instead of making duplicate effort makes sense.

I did wonder the same "what about tarball" thing while reading this last night and then stopped, and it is good to see somebody else brought it up ;-). I like the "version" approach, as that is already an established way to deal with the same "the builder somehow wants to force the identifying string to a certain value in the build product".

Are we sure historical GIT_VERSION strings never had a SP in it? I would be very surprised if we did, but the correctness of the approach depends on that assumption.

Thanks.

Junio C Hamano wrote:

Why would that matter?

Supposing a hypothetical tarball with a version '0.99 foo', it would have a corresponding GIT-VERSION-GEN (which didn't check for such file until 1.1.1), and it would know how to parse it.

In other words: all three are tied together: the 'version' file, the GIT-VERSION-GEN script, and the tarball that has them both.

Felipe Contreras <felipe.contreras@gmail.com> writes:

Ah, that is true. What I should have worried more about was the distribution package maintainers who may set their own version by writing it in the "version" file themselves.

Thanks.

Junio C Hamano wrote:

In reality they don't do that. I checked Arch Linux, Fedora, and Debian, and all of them leave the version alone.

It does make sense becasue for example in Arch Linux `2.40.0-1` means version `2.40.0`, release `1`. I believe all packaging systems use the same semantical distinction.

I asked ChatGPT, and this is what it said about `-1`:

  -1 is the package release number. This number is used by the package
  maintainers to indicate how many times the package has been built and
  released. This number is typically incremented each time a new build
  of the package is released, even if the underlying software has not
  changed.

So it makes sense for `git --version` to return the upstream version, not the package release number, and that's why packagers don't mess with that.

Either way, even though I don't think it matters in practice, I generally prefer to separate fields with tabs, or even newlines. If you consider this an issue, we could do:

  printf '%s\n' $(GIT_VERSION) $(GIT_DATE) >version

  read -d'' VN DN <version

Cheers.