{"thread":{"id":"59589","subject":"[PATCH] doc: set actual revdate for manpages","startedAt":"2023-04-13T07:47:58Z","lastAt":"2023-04-14T22:06:19Z","messageCount":14,"participants":["Felipe Contreras","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"475267","messageId":"20230413074722.71260-1-felipe.contreras@gmail.com","threadId":"59589","inReplyTo":null,"subject":"[PATCH] doc: set actual revdate for manpages","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-04-13T07:47:22Z","receivedAt":"2023-04-13T07:47:58Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"manpages expect the date of the last revision, if that is not found\nDocBook Stylesheets go through a series of hacks to generate one with\nthe format `%d/%d/%Y` which is not ideal.\n\nIn addition to this format not being standard, different tools generate\ndates with different formats.\n\nThere's no need for any confusion if we specify the revision date, so\nlet's do so.\n\nSigned-off-by: Felipe Contreras \n---\n\nThis patch requires [1] to actually work, and has a simple conflict with\n[2], so it's written on top of both.\n\n[1] https://lore.kernel.org/git/20230323221523.52472-1-felipe.contreras@gmail.com/\n[2] https://lore.kernel.org/git/20230408001829.11031-1-felipe.contreras@gmail.com/\n\n Documentation/Makefile | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/Makefile b/Documentation/Makefile\nindex 3133ea3182..b629176d7d 100644\n--- a/Documentation/Makefile\n+++ b/Documentation/Makefile\n@@ -144,13 +144,16 @@ man5dir = $(mandir)/man5\n man7dir = $(mandir)/man7\n # DESTDIR =\n \n+GIT_DATE := $(shell git show --quiet --pretty='%as')\n+\n ASCIIDOC = asciidoc\n ASCIIDOC_EXTRA =\n ASCIIDOC_HTML = xhtml11\n ASCIIDOC_DOCBOOK = docbook\n ASCIIDOC_CONF = -f asciidoc.conf\n ASCIIDOC_COMMON = $(ASCIIDOC) $(ASCIIDOC_EXTRA) $(ASCIIDOC_CONF) \\\n-\t\t-amanmanual='Git Manual' -amansource='Git $(GIT_VERSION)'\n+\t\t-amanmanual='Git Manual' -amansource='Git $(GIT_VERSION)' \\\n+\t\t-arevdate='$(GIT_DATE)'\n ASCIIDOC_DEPS = asciidoc.conf GIT-ASCIIDOCFLAGS\n TXT_TO_HTML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_HTML)\n TXT_TO_XML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_DOCBOOK)\n-- \n2.40.0+fc1\n\n"},{"id":"475344","messageId":"20230414070449.GA540206@coredump.intra.peff.net","threadId":"59589","inReplyTo":"20230413074722.71260-1-felipe.contreras@gmail.com","subject":"Re: [PATCH] doc: set actual revdate for manpages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-14T07:04:49Z","receivedAt":"2023-04-14T07:04:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 13, 2023 at 01:47:22AM -0600, Felipe Contreras wrote:\n\n> manpages expect the date of the last revision, if that is not found\n> DocBook Stylesheets go through a series of hacks to generate one with\n> the format `%d/%d/%Y` which is not ideal.\n> \n> In addition to this format not being standard, different tools generate\n> dates with different formats.\n> \n> There's no need for any confusion if we specify the revision date, so\n> let's do so.\n\nThat seems like a good goal, and should reduce our asciidoc/asciidoctor\ndiff considerably.\n\n> This patch requires [1] to actually work, and has a simple conflict with\n> [2], so it's written on top of both.\n> \n> [1] https://lore.kernel.org/git/20230323221523.52472-1-felipe.contreras@gmail.com/\n> [2] https://lore.kernel.org/git/20230408001829.11031-1-felipe.contreras@gmail.com/\n\nI wasted a bit of time trying this out, so let me elaborate on \"actually\nwork\" for the benefit of other reviewers. Without the patch in [1]\n(which is 8806120de6c on fc/remove-header-workarounds-for-asciidoc),\nthis patch works as advertised with asciidoctor, but has no effect with\nasciidoc. The reason is that asciidoc puts the tags in the\nheader, and the custom header removed by 8806120de6c suppresses\nasciidoc's default header entirely (so a workaround would be to include\nthe tags in our custom header, but I don't see any reason not to\njust build on top of 8806120de6c, as you did here).\n\n> diff --git a/Documentation/Makefile b/Documentation/Makefile\n> index 3133ea3182..b629176d7d 100644\n> --- a/Documentation/Makefile\n> +++ b/Documentation/Makefile\n> @@ -144,13 +144,16 @@ man5dir = $(mandir)/man5\n> man7dir = $(mandir)/man7\n> # DESTDIR =\n> \n> +GIT_DATE := $(shell git show --quiet --pretty='%as')\n\nWhat will/should this do in a distribution tarball, where we won't have\na Git repository at all? I think we'll just end up with a blank date in\nthe xml file, though it looks like docbook turns that into today's date\nin the result.\n\nThat's not _too_ bad, but feels a bit inconsistent (and it uses the\nformat you're trying to get rid of!).\n\nIt would be nicer to populate the date variable in that case, like we do\nfor GIT_VERSION. I think that could look something like this:\n\ndiff --git a/Documentation/Makefile b/Documentation/Makefile\nindex 2ccc3a9bc9..307634a94f 100644\n--- a/Documentation/Makefile\n+++ b/Documentation/Makefile\n@@ -144,8 +144,6 @@ man5dir = $(mandir)/man5\n man7dir = $(mandir)/man7\n # DESTDIR =\n \n-GIT_DATE := $(shell git show --quiet --pretty='%as')\n-\n ASCIIDOC = asciidoc\n ASCIIDOC_EXTRA =\n ASCIIDOC_HTML = xhtml11\ndiff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN\nindex 9a1111af9b..14903bd261 100755\n--- a/GIT-VERSION-GEN\n+++ b/GIT-VERSION-GEN\n@@ -10,7 +10,8 @@ LF='\n # then try git-describe, then default.\n if test -f version\n then\n-\tVN=$(cat version) || VN=\"$DEF_VER\"\n+\tVN=$(cut -d\" \" -f1 version) || VN=\"$DEF_VER\"\n+\tDN=$(cut -d\" \" -f2 version) || DN=\"\"\n elif test -d ${GIT_DIR:-.git} -o -f .git &&\n \tVN=$(git describe --match \"v[0-9]*\" HEAD 2>/dev/null) &&\n \tcase \"$VN\" in\n@@ -22,19 +23,22 @@ elif test -d ${GIT_DIR:-.git} -o -f .git &&\n \tesac\n then\n \tVN=$(echo \"$VN\" | sed -e 's/-/./g');\n+\tDN=$(git log -1 --format=%as HEAD)\n else\n \tVN=\"$DEF_VER\"\n+\tDN=\"\"\n fi\n \n VN=$(expr \"$VN\" : v*'\\(.*\\)')\n \n if test -r $GVF\n then\n-\tVC=$(sed -e 's/^GIT_VERSION = //' <$GVF)\n+\tVC=$(sed -ne 's/^GIT_VERSION = //p' <$GVF)\n else\n \tVC=unset\n fi\n test \"$VN\" = \"$VC\" || {\n \techo >&2 \"GIT_VERSION = $VN\"\n \techo \"GIT_VERSION = $VN\" >$GVF\n+\techo \"GIT_DATE = $DN\" >>$GVF\n }\ndiff --git a/Makefile b/Makefile\nindex 60ab1a8b4f..fa8db1943c 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3573,7 +3573,7 @@ endif\n dist: git-archive$(X) configure\n \t@$(RM) -r .dist-tmp-dir\n \t@mkdir .dist-tmp-dir\n-\t@echo $(GIT_VERSION) > .dist-tmp-dir/version\n+\t@echo $(GIT_VERSION) $(GIT_DATE) > .dist-tmp-dir/version\n \t@$(MAKE) -C git-gui TARDIR=../.dist-tmp-dir/git-gui dist-version\n \t./git-archive --format=tar \\\n \t\t$(GIT_ARCHIVE_EXTRA_FILES) \\\n-- \n2.40.0.515.gdfb9e78b42\n\n"},{"id":"475375","messageId":"643949a57396c_5b7e62948d@chronos.notmuch","threadId":"59589","inReplyTo":"20230414070449.GA540206@coredump.intra.peff.net","subject":"Re: [PATCH] doc: set actual revdate for manpages","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-04-14T12:40:05Z","receivedAt":"2023-04-14T12:40:52Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Jeff King wrote:\n> On Thu, Apr 13, 2023 at 01:47:22AM -0600, Felipe Contreras wrote:\n> \n> > manpages expect the date of the last revision, if that is not found\n> > DocBook Stylesheets go through a series of hacks to generate one with\n> > the format `%d/%d/%Y` which is not ideal.\n> > \n> > In addition to this format not being standard, different tools generate\n> > dates with different formats.\n> > \n> > There's no need for any confusion if we specify the revision date, so\n> > let's do so.\n> \n> That seems like a good goal, and should reduce our asciidoc/asciidoctor\n> diff considerably.\n> \n> > This patch requires [1] to actually work, and has a simple conflict with\n> > [2], so it's written on top of both.\n> > \n> > [1] https://lore.kernel.org/git/20230323221523.52472-1-felipe.contreras@gmail.com/\n> > [2] https://lore.kernel.org/git/20230408001829.11031-1-felipe.contreras@gmail.com/\n> \n> I wasted a bit of time trying this out, so let me elaborate on \"actually\n> work\" for the benefit of other reviewers. Without the patch in [1]\n> (which is 8806120de6c on fc/remove-header-workarounds-for-asciidoc),\n> this patch works as advertised with asciidoctor, but has no effect with\n> asciidoc. The reason is that asciidoc puts the tags in the\n> header, and the custom header removed by 8806120de6c suppresses\n> asciidoc's default header entirely (so a workaround would be to include\n> the tags in our custom header, but I don't see any reason not to\n> just build on top of 8806120de6c, as you did here).\n\nYes, I also \"wasted\" a bit of time comparing the custom header introduced in\n7ef195ba3e (Documentation: Add version information to man pages, 2007-03-25) to\nwhat is in asciidoc and the difference is simple:\n\n--- a/Documentation/asciidoc.conf\n+++ b/Documentation/asciidoc.conf\n@@ -56,6 +56,9 @@ ifdef::backend-docbook[]\n [header]\n template::[header-declarations]\n \n+\n+template::[docinfo]\n+\n \n {mantitle}\n {manvolnum}\n\nThis is of course not needed if we get rid of that custom header, but I sent\nthe patch anyway for reference in a semi-unrelated patch series [1].\n\nBut to me the rationale is simple: the information isn't there because we are\noverriding the asciidoc header and not putting it there.\n\n> > diff --git a/Documentation/Makefile b/Documentation/Makefile\n> > index 3133ea3182..b629176d7d 100644\n> > --- a/Documentation/Makefile\n> > +++ b/Documentation/Makefile\n> > @@ -144,13 +144,16 @@ man5dir = $(mandir)/man5\n> > man7dir = $(mandir)/man7\n> > # DESTDIR =\n> > \n> > +GIT_DATE := $(shell git show --quiet --pretty='%as')\n> \n> What will/should this do in a distribution tarball, where we won't have\n> a Git repository at all? I think we'll just end up with a blank date in\n> the xml file, though it looks like docbook turns that into today's date\n> in the result.\n> \n> That's not _too_ bad, but feels a bit inconsistent (and it uses the\n> format you're trying to get rid of!).\n> \n> It would be nicer to populate the date variable in that case, like we do\n> for GIT_VERSION. I think that could look something like this:\n\nIndeed, that would be better, but that totally can be done in a separate patch,\nor a separate series even.\n\nThe perfect doesn't have to be the enemy of the good, and that can be done\nlater.\n\nIn the meantime something is better than nothing.\n\nFor the record, the GIT-VERSION-GEN script can be simplified a lot, I had a\npatch from 2013 around, cleaned it up, and sent it a new series, so it should\nbe easier to implement this on top of that [2].\n\n> diff --git a/Documentation/Makefile b/Documentation/Makefile\n> index 2ccc3a9bc9..307634a94f 100644\n> --- a/Documentation/Makefile\n> +++ b/Documentation/Makefile\n> @@ -144,8 +144,6 @@ man5dir = $(mandir)/man5\n> man7dir = $(mandir)/man7\n> # DESTDIR =\n> \n> -GIT_DATE := $(shell git show --quiet --pretty='%as')\n> -\n> ASCIIDOC = asciidoc\n> ASCIIDOC_EXTRA =\n> ASCIIDOC_HTML = xhtml11\n> diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN\n> index 9a1111af9b..14903bd261 100755\n> --- a/GIT-VERSION-GEN\n> +++ b/GIT-VERSION-GEN\n> @@ -10,7 +10,8 @@ LF='\n> # then try git-describe, then default.\n> if test -f version\n> then\n> -\tVN=$(cat version) || VN=\"$DEF_VER\"\n> +\tVN=$(cut -d\" \" -f1 version) || VN=\"$DEF_VER\"\n> +\tDN=$(cut -d\" \" -f2 version) || DN=\"\"\n\nAlthough this works, I have an issue with doing multiple unnecessary forks.\n\nThis does the same, no?\n\n read VN DN writes:\n\n> ... The reason is that asciidoc puts the tags in the\n> header, and the custom header removed by 8806120de6c suppresses\n> asciidoc's default header entirely (so a workaround would be to include\n> the tags in our custom header, but I don't see any reason not to\n> just build on top of 8806120de6c, as you did here).\n\nI agree building on what is in-flight instead of making duplicate\neffort makes sense.\n\n>> diff --git a/Documentation/Makefile b/Documentation/Makefile\n>> index 3133ea3182..b629176d7d 100644\n>> --- a/Documentation/Makefile\n>> +++ b/Documentation/Makefile\n>> @@ -144,13 +144,16 @@ man5dir = $(mandir)/man5\n>> man7dir = $(mandir)/man7\n>> # DESTDIR =\n>> \n>> +GIT_DATE := $(shell git show --quiet --pretty='%as')\n>\n> What will/should this do in a distribution tarball, where we won't have\n> a Git repository at all? I think we'll just end up with a blank date in\n> the xml file, though it looks like docbook turns that into today's date\n> in the result.\n>\n> That's not _too_ bad, but feels a bit inconsistent (and it uses the\n> format you're trying to get rid of!).\n>\n> It would be nicer to populate the date variable in that case, like we do\n> for GIT_VERSION. I think that could look something like this:\n\nI did wonder the same \"what about tarball\" thing while reading this\nlast night and then stopped, and it is good to see somebody else\nbrought it up ;-). I like the \"version\" approach, as that is\nalready an established way to deal with the same \"the builder\nsomehow wants to force the identifying string to a certain value in\nthe build product\".\n\n\n> diff --git a/Documentation/Makefile b/Documentation/Makefile\n> index 2ccc3a9bc9..307634a94f 100644\n> --- a/Documentation/Makefile\n> +++ b/Documentation/Makefile\n> @@ -144,8 +144,6 @@ man5dir = $(mandir)/man5\n> man7dir = $(mandir)/man7\n> # DESTDIR =\n> \n> -GIT_DATE := $(shell git show --quiet --pretty='%as')\n> -\n> ASCIIDOC = asciidoc\n> ASCIIDOC_EXTRA =\n> ASCIIDOC_HTML = xhtml11\n> diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN\n> index 9a1111af9b..14903bd261 100755\n> --- a/GIT-VERSION-GEN\n> +++ b/GIT-VERSION-GEN\n> @@ -10,7 +10,8 @@ LF='\n> # then try git-describe, then default.\n> if test -f version\n> then\n> -\tVN=$(cat version) || VN=\"$DEF_VER\"\n> +\tVN=$(cut -d\" \" -f1 version) || VN=\"$DEF_VER\"\n> +\tDN=$(cut -d\" \" -f2 version) || DN=\"\"\n\nAre we sure historical GIT_VERSION strings never had a SP in it?\nI would be very surprised if we did, but the correctness of the\napproach depends on that assumption.\n\nThanks.\n"},{"id":"475389","messageId":"64397d3d67d21_5e3ce294a6@chronos.notmuch","threadId":"59589","inReplyTo":"xmqq5y9yv601.fsf@gitster.g","subject":"Re: [PATCH] doc: set actual revdate for manpages","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-04-14T16:20:13Z","receivedAt":"2023-04-14T16:20:28Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Jeff King writes:\n\n> > diff --git a/Documentation/Makefile b/Documentation/Makefile\n> > index 2ccc3a9bc9..307634a94f 100644\n> > --- a/Documentation/Makefile\n> > +++ b/Documentation/Makefile\n> > @@ -144,8 +144,6 @@ man5dir = $(mandir)/man5\n> > man7dir = $(mandir)/man7\n> > # DESTDIR =\n> > \n> > -GIT_DATE := $(shell git show --quiet --pretty='%as')\n> > -\n> > ASCIIDOC = asciidoc\n> > ASCIIDOC_EXTRA =\n> > ASCIIDOC_HTML = xhtml11\n> > diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN\n> > index 9a1111af9b..14903bd261 100755\n> > --- a/GIT-VERSION-GEN\n> > +++ b/GIT-VERSION-GEN\n> > @@ -10,7 +10,8 @@ LF='\n> > # then try git-describe, then default.\n> > if test -f version\n> > then\n> > -\tVN=$(cat version) || VN=\"$DEF_VER\"\n> > +\tVN=$(cut -d\" \" -f1 version) || VN=\"$DEF_VER\"\n> > +\tDN=$(cut -d\" \" -f2 version) || DN=\"\"\n> \n> Are we sure historical GIT_VERSION strings never had a SP in it?\n> I would be very surprised if we did, but the correctness of the\n> approach depends on that assumption.\n\nWhy would that matter?\n\nSupposing a hypothetical tarball with a version '0.99 foo', it would have a\ncorresponding GIT-VERSION-GEN (which didn't check for such file until 1.1.1),\nand it would know how to parse it.\n\nIn other words: all three are tied together: the 'version' file, the\nGIT-VERSION-GEN script, and the tarball that has them both.\n\n-- \nFelipe Contreras\n"},{"id":"475390","messageId":"xmqqildys97o.fsf@gitster.g","threadId":"59589","inReplyTo":"643949a57396c_5b7e62948d@chronos.notmuch","subject":"Re: [PATCH] doc: set actual revdate for manpages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-14T16:46:19Z","receivedAt":"2023-04-14T16:46:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n>> > diff --git a/Documentation/Makefile b/Documentation/Makefile\n>> > index 3133ea3182..b629176d7d 100644\n>> > --- a/Documentation/Makefile\n>> > +++ b/Documentation/Makefile\n>> > @@ -144,13 +144,16 @@ man5dir = $(mandir)/man5\n>> > man7dir = $(mandir)/man7\n>> > # DESTDIR =\n>> > \n>> > +GIT_DATE := $(shell git show --quiet --pretty='%as')\n>> \n>> What will/should this do in a distribution tarball, where we won't have\n>> a Git repository at all? I think we'll just end up with a blank date in\n>> the xml file, though it looks like docbook turns that into today's date\n>> in the result.\n>> \n>> That's not _too_ bad, but feels a bit inconsistent (and it uses the\n>> format you're trying to get rid of!).\n>> \n>> It would be nicer to populate the date variable in that case, like we do\n>> for GIT_VERSION. I think that could look something like this:\n>\n> Indeed, that would be better, but that totally can be done in a separate patch,\n> or a separate series even.\n\nSeeing Peff's change, it sounds so small a thing that it feels a bit\nunnatural not to do that in the same series, at least to me.\n\nHaving said that, I think that \"we make progress one step at a time\"\nis perfectly acceptable and may even be preferred, as long as the\nformatted manpages from the tarball would not change between with\nand without this patch. That way, we make the output from a\nrepository better while keeping the output from a tarball extract\nintact, and make the latter match the former in a later effort.\n\nSo, I \"wasted\" (not really---this was a fruitful validation that is\na part of a review process) some time to play with this on top of\n'seen' to see how well the tarball extract fares. We get an error\nmessage from \"git show\" complaining about \"not a git repository\" but\nthat is to be expected (\"sh GIT-VERSION-GEN\" does not share the\nproblem, though).\n\nAt least with the versions of toolchain picked by default in my\nenvironment, I seem to be getting identical \"04/14/2023\" in a\ndirectory extracted out of a tarball taken from 'seen' (with and\nwithout this patch) in the formatted manpages, because we do not\nhave any record in the input either way.\n\nFormatted output from a repository working tree changes from\n\"04/14/2023\" to \"2023-04-13\". The value change may be intended, but\nI am not sure if the format change was intended or even welcome. If\nwe want to correct the date format, it can totally be done in a\nseparate patch, or a separate series even, with some justification\nin the proposed log message, I think.\n\nThanks.\n"},{"id":"475392","messageId":"xmqqa5zas7xf.fsf@gitster.g","threadId":"59589","inReplyTo":"xmqqildys97o.fsf@gitster.g","subject":"Re: [PATCH] doc: set actual revdate for manpages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-14T17:14:04Z","receivedAt":"2023-04-14T17:14:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Formatted output from a repository working tree changes from\n> \"04/14/2023\" to \"2023-04-13\". The value change may be intended,...\n\nForgot to mention another thing. While it may be a good idea to tie\nthe datestamp etched in the formatted result to that of the source\nmaterial, rather than the date the formatter happened to have been\nrun, the committer date is more appropriate than the author date for\nthat purpose, as the former is the date that the change made on the\nlatter date (which is earlier) has become a part of the whole, from\nwhich the formatted result was produced.\n\nIt may not make a big practical difference:\n\n * For an individual who is trying out the changes just made, the\n committer time and the author time are likely identical.\n\n * For a release process, what is at the tip of the released branch\n is likely be the release notes and version bump, recorded by the\n releaser, and again the committer time and the author time are\n likely identical.\n\n * For results of a pull request, the times are likely identical for\n the merge commit.\n\nbut from the philosophical standpoint, it does matter. Using the\ncommitter time would give us one more justification to use one\nsingle timestamp from the commit, when people complain \"this manual\npage, as opposed to the other one that was changed in the latest\nrelease, has not seen any need to be updated for the past 3 years,\nyet the formatted output for these two manual pages carry the same\ndate\".\n"},{"id":"475394","messageId":"xmqqv8hyqsc6.fsf@gitster.g","threadId":"59589","inReplyTo":"64397d3d67d21_5e3ce294a6@chronos.notmuch","subject":"Re: [PATCH] doc: set actual revdate for manpages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-14T17:36:09Z","receivedAt":"2023-04-14T17:36:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n>> Are we sure historical GIT_VERSION strings never had a SP in it?\n>> I would be very surprised if we did, but the correctness of the\n>> approach depends on that assumption.\n>\n> Why would that matter?\n\nAh, that is true. What I should have worried more about was the\ndistribution package maintainers who may set their own version by\nwriting it in the \"version\" file themselves.\n\nThanks.\n\n"},{"id":"475403","messageId":"64399ac5685a4_5f77329495@chronos.notmuch","threadId":"59589","inReplyTo":"xmqqildys97o.fsf@gitster.g","subject":"Re: [PATCH] doc: set actual revdate for manpages","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-04-14T18:26:13Z","receivedAt":"2023-04-14T18:26:19Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Felipe Contreras writes:\n> \n> >> > diff --git a/Documentation/Makefile b/Documentation/Makefile\n> >> > index 3133ea3182..b629176d7d 100644\n> >> > --- a/Documentation/Makefile\n> >> > +++ b/Documentation/Makefile\n> >> > @@ -144,13 +144,16 @@ man5dir = $(mandir)/man5\n> >> > man7dir = $(mandir)/man7\n> >> > # DESTDIR =\n> >> > \n> >> > +GIT_DATE := $(shell git show --quiet --pretty='%as')\n> >> \n> >> What will/should this do in a distribution tarball, where we won't have\n> >> a Git repository at all? I think we'll just end up with a blank date in\n> >> the xml file, though it looks like docbook turns that into today's date\n> >> in the result.\n> >> \n> >> That's not _too_ bad, but feels a bit inconsistent (and it uses the\n> >> format you're trying to get rid of!).\n> >> \n> >> It would be nicer to populate the date variable in that case, like we do\n> >> for GIT_VERSION. I think that could look something like this:\n> >\n> > Indeed, that would be better, but that totally can be done in a separate patch,\n> > or a separate series even.\n> \n> Seeing Peff's change, it sounds so small a thing that it feels a bit\n> unnatural not to do that in the same series, at least to me.\n\nIt should be a small thing, but the GIT-VERSION-GEN script has not been\nupdated since 2008 and has a lot of issues (from my point of view).\n\nI think it should be cleaned up first (I already sent a patch series for\nthat), *then* it would be trivial to add another field.\n\nBut that would add another (unncessary) dependency to this series.\n\n> Having said that, I think that \"we make progress one step at a time\"\n> is perfectly acceptable and may even be preferred, as long as the\n> formatted manpages from the tarball would not change between with\n> and without this patch. That way, we make the output from a\n> repository better while keeping the output from a tarball extract\n> intact, and make the latter match the former in a later effort.\n> \n> So, I \"wasted\" (not really---this was a fruitful validation that is\n> a part of a review process) some time to play with this on top of\n> 'seen' to see how well the tarball extract fares. We get an error\n> message from \"git show\" complaining about \"not a git repository\" but\n> that is to be expected (\"sh GIT-VERSION-GEN\" does not share the\n> problem, though).\n> \n> At least with the versions of toolchain picked by default in my\n> environment, I seem to be getting identical \"04/14/2023\" in a\n> directory extracted out of a tarball taken from 'seen' (with and\n> without this patch) in the formatted manpages, because we do not\n> have any record in the input either way.\n> \n> Formatted output from a repository working tree changes from\n> \"04/14/2023\" to \"2023-04-13\". The value change may be intended, but\n> I am not sure if the format change was intended or even welcome.\n\nI live in the vast majority of countries where MM/DD/YYYY makes\nabsolutely no sense, so I think it's a plus if any man pages have a sane\ndate.\n\nI'm pretty sure I'm not alone on this, but some might disagree.\n\n> If we want to correct the date format, it can totally be done in a\n> separate patch, or a separate series even, with some justification in\n> the proposed log message, I think.\n\nSure, but in order to do that we would need a way to convert the sane\nstandard git dates to USA-centric dates, and I'm not so sure the\ntidyness of having both dates wrong is worth it.\n\nWould you consider something like this more welcome?\n\n GIT_DATE := $(shell git show --quiet --pretty='%as' | sed -e 's|^\\(.\\+\\)-\\(.\\+\\)-\\(.\\+\\)$|\\2/\\3/\\1|'\n\n-- \nFelipe Contreras\n"},{"id":"475405","messageId":"64399e28eed60_5f7732949f@chronos.notmuch","threadId":"59589","inReplyTo":"xmqqa5zas7xf.fsf@gitster.g","subject":"Re: [PATCH] doc: set actual revdate for manpages","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-04-14T18:40:40Z","receivedAt":"2023-04-14T18:40:47Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > Formatted output from a repository working tree changes from\n> > \"04/14/2023\" to \"2023-04-13\". The value change may be intended,...\n> \n> Forgot to mention another thing. While it may be a good idea to tie\n> the datestamp etched in the formatted result to that of the source\n> material, rather than the date the formatter happened to have been\n> run, the committer date is more appropriate than the author date for\n> that purpose, as the former is the date that the change made on the\n> latter date (which is earlier) has become a part of the whole, from\n> which the formatted result was produced.\n> \n> It may not make a big practical difference:\n\nOr _any_ practical difference.\n\n> * For an individual who is trying out the changes just made, the\n> committer time and the author time are likely identical.\n> \n> * For a release process, what is at the tip of the released branch\n> is likely be the release notes and version bump, recorded by the\n> releaser, and again the committer time and the author time are\n> likely identical.\n> \n> * For results of a pull request, the times are likely identical for\n> the merge commit.\n\nAgreed that committer == author in these cases.\n\n> but from the philosophical standpoint, it does matter.\n\nI prefer to focus on the real rather than hypothetical (or\nphilosophical).\n\nI do create my own releases (e.g. 2.40.0+fc1) and a real issue with the\nversion script (rather than philosphical) is that it only considers\nannotated tags, so I have to carry a patch that adds `--tags` to the\n`git describe` command.\n\nShouldn't dealing with real issues of real people have a higher priority\nthan philosophical issues?\n\nFor the record, I do agree the committer date feels more proper, but it\ndoesn't make any real difference, I just wonder about the priorities.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"475407","messageId":"6439a28c34fe7_5f77329440@chronos.notmuch","threadId":"59589","inReplyTo":"xmqqv8hyqsc6.fsf@gitster.g","subject":"Re: [PATCH] doc: set actual revdate for manpages","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-04-14T18:59:24Z","receivedAt":"2023-04-14T18:59:29Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Felipe Contreras writes:\n> \n> >> Are we sure historical GIT_VERSION strings never had a SP in it?\n> >> I would be very surprised if we did, but the correctness of the\n> >> approach depends on that assumption.\n> >\n> > Why would that matter?\n> \n> Ah, that is true. What I should have worried more about was the\n> distribution package maintainers who may set their own version by\n> writing it in the \"version\" file themselves.\n\nIn reality they don't do that. I checked Arch Linux, Fedora, and Debian,\nand all of them leave the version alone.\n\nIt does make sense becasue for example in Arch Linux `2.40.0-1` means\nversion `2.40.0`, release `1`. I believe all packaging systems use the\nsame semantical distinction.\n\nI asked ChatGPT, and this is what it said about `-1`:\n\n -1 is the package release number. This number is used by the package\n maintainers to indicate how many times the package has been built and\n released. This number is typically incremented each time a new build\n of the package is released, even if the underlying software has not\n changed.\n\nSo it makes sense for `git --version` to return the upstream version,\nnot the package release number, and that's why packagers don't mess with\nthat.\n\nEither way, even though I don't think it matters in practice, I\ngenerally prefer to separate fields with tabs, or even newlines. If you\nconsider this an issue, we could do:\n\n printf '%s\\n' $(GIT_VERSION) $(GIT_DATE) >version\n\n read -d'' VN DN > It would be nicer to populate the date variable in that case, like we do\n> > for GIT_VERSION. I think that could look something like this:\n> \n> Indeed, that would be better, but that totally can be done in a separate patch,\n> or a separate series even.\n> \n> The perfect doesn't have to be the enemy of the good, and that can be done\n> later.\n> \n> In the meantime something is better than nothing.\n\nSometimes, as long as it is not making the status quo worse in the\nmeantime. In this case, if the tarball build continues to use the\ncurrent date (which it appears to), then it's not changing. It does\nbecome inconsistent with the output you get from building in-repo, which\nis unfortunate. But that might be acceptable.\n\nIf so, it would make sense to document that decision in the commit\nmessage. We'd also want to suppress stderr from the Git invocation so\nthat tarball builders don't see a confusing error message. And probably\nwe should avoid using the \"git show\" porcelain in a script. I think:\n\n git rev-list -1 --no-commit-header --format=%as HEAD\n\nwould do what you want (or %cs; I don't have a strong opinion there).\n\n> > --- a/GIT-VERSION-GEN\n> > +++ b/GIT-VERSION-GEN\n> > @@ -10,7 +10,8 @@ LF='\n> > # then try git-describe, then default.\n> > if test -f version\n> > then\n> > -\tVN=$(cat version) || VN=\"$DEF_VER\"\n> > +\tVN=$(cut -d\" \" -f1 version) || VN=\"$DEF_VER\"\n> > +\tDN=$(cut -d\" \" -f2 version) || DN=\"\"\n> \n> Although this works, I have an issue with doing multiple unnecessary forks.\n> \n> This does the same, no?\n> \n> read VN DN So, I \"wasted\" (not really---this was a fruitful validation that is\n> a part of a review process) some time to play with this on top of\n\nThe word \"wasted\" is appearing in a lot of people's emails in this\nthread. ;) So let me just clarify that in the original I meant that I\nspent time puzzling over why it was not working, which was a waste\nbecause Felipe had already said it required the other patch.\n\nIt definitely would have helped if he had explained _why_ the other\npatch was necessary. But to be fair, the fault was at least as much mine\nfor not heeding what he did say.\n\nBut reviewers reproducing and tinkering is most certainly not a waste of\ntime in general, and I didn't mean to imply it was (or that the patch\nitself was a waste of time).\n\n> Formatted output from a repository working tree changes from\n> \"04/14/2023\" to \"2023-04-13\". The value change may be intended, but\n> I am not sure if the format change was intended or even welcome. If\n> we want to correct the date format, it can totally be done in a\n> separate patch, or a separate series even, with some justification\n> in the proposed log message, I think.\n\nI think the change is welcome and intended. I would not mind seeing the\ntwo changes (format change, and using commit date versus \"now\")\nconceptually split, but I think it's much more tangled. Asciidoctor is\nalready producing y-m-d dates, and python asciidoc is using m/d/y.\nChanging the latter requires passing in not just a format but the actual\ndate. If we want it to be the current date, then we have to get that\nfrom somewhere, which may introduce portability questions (e.g., can we\nrely on \"date\"?).\n\nSo doing it all in one patch, though this conversation may indicate that\nthe commit message could do a better job of explaining the goal and\nimplications.\n\n-Peff\n"},{"id":"475428","messageId":"xmqqmt3ap19m.fsf@gitster.g","threadId":"59589","inReplyTo":"20230414214508.GB640081@coredump.intra.peff.net","subject":"Re: [PATCH] doc: set actual revdate for manpages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-14T22:06:13Z","receivedAt":"2023-04-14T22:06:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So doing it all in one patch, though this conversation may indicate that\n> the commit message could do a better job of explaining the goal and\n> implications.\n\nI can read both formats, and if we are moving in the direction to be\nconsistent between AsciiDoc and asciidoctor, that is good. As long\nas the design decision is documented well in the proposed commit log\nmessage, I would not complain ;-).\n\nThanks.\n"}]}