From: John Cai <johncai86@gmail.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: Jeff King <peff@peff.net>, Junio C Hamano <gitster@pobox.com>,
git@vger.kernel.org
Subject: Re: [PATCH v3 4/4] Documentation: add lint-fsck-msgids
Date: Thu, 27 Oct 2022 23:02:14 -0400 [thread overview]
Message-ID: <08A5BC44-24D9-4C8F-A61A-41983A13553A@gmail.com> (raw)
In-Reply-To: <221026.8635badbz5.gmgdl@evledraar.gmail.com>
Hi Ævar,
On 26 Oct 2022, at 7:35, Ævar Arnfjörð Bjarmason wrote:
> On Wed, Oct 26 2022, Jeff King wrote:
>
>> On Wed, Oct 26, 2022 at 04:43:32AM +0200, Ævar Arnfjörð Bjarmason wrote:
>>
>>>
>>> On Tue, Oct 25 2022, Junio C Hamano wrote:
>>>
>>>> During the initial development of the fsck-msgids.txt feature, it
>>>> has become apparent that it is very much error prone to make sure
>>>> the description in the documentation file are sorted and correctly
>>>> match what is in the fsck.h header file.
>>>
>>> I have local fixes for the same issues in the list of advice in our
>>> docs, some of it's missing, wrong, out of date etc.
>>>
>>> I tried to quickly adapt the generation script I had for that, which
>>> works nicely, and by line count much shorter than the lint :)
>>
>> Yeah, my instinct here was to generate rather than lint. If you make a
>> mistake and the linter hits you over the head, that is better than
>> quietly letting your mistake go. But better still is making it
>> impossible to make in the first place.
>>
>> The downside is added complexity to the build, but it doesn't seem too
>> bad in this case.
>
> Yeah, it's not, I have local patches to generate advice-{type,config}.h,
> and builtin.h. This is a quick POC to do it for fsck-msgids.h.
>
> I see I forgot the .gitignore entry, so it's a rough POC :)
>
>> (I had a similar thought after getting hit on the head by the recent
>> t0450-txt-doc-vs-help.sh).
>
> Sorry about that. FWIW I've wanted to assert that for a while, and to do
> it by e.g. having the doc *.txt blurbs generated from running "$buildin
> -h" during the build.
If we wanted to go this route of generating the docs from the code (which sounds
like a better long term solution), how would this work? Would we print out the
list of message ids in builtin/fsck.c and write it to
Documentation/fsck-msgids.txt ?
>
> But when I suggested that I gathered That Junio wasn't up for that
> approach, it does have its downsides thorugh. E.g. to build the docs
> you'd now have to compile C code, and e.g. that git-scm.com publisher
> written in Ruby would have to compile the code to generate its docs.
>
> Or we could do it the other way around, and scape the *.txt for the *.c
> generation, but then we need to run a new script for building
> builtin/*.o. Also possible, and I think eventually both are better than
> what t0450-txt-doc-vs-help.sh's doing now, but that was a less invasive
> change than both...
>
>>> Having to exhaustively list every *.c file that uses fsck.h is a bit of
>>> a bother, but we have the same with the other generated *.h's, so it
>>> shouldn't be too bad.
>>
>> It feels like this could be made much shorter by having a separate
>> fsck-msgs.h and not including it from fsck.h. Only fsck.c and mktag.c
>> need the actual list. It would probably have to stop being an "enum",
>> though.
>
> Yes, that would make it shorter, but C doesn't have forward decls of
> enums, so we'd need to make it "int", ....
>
>> Another alternative is to generate the doc from the code, rather than
>> the other way around.
>
> *nod* :)
>
>>> +# Unfortunately our dependency management of generated headers used
>>> +# from within other headers suck, so we'll need to list every user of
>>> +# fsck.h here, but not too bad...
>>> +FSCK_MSGIDS_H_BUILTINS = fsck index-pack mktag receive-pack unpack-objects
>>> +$(foreach f,$(FSCK_MSGIDS_H_BUILTINS:%=builtin/%),$f.sp $f.s $f.o): fsck-msgids.h
>>> +FSCK_MSGIDS_H_LIBS = fetch-pack fsck
>>> +$(foreach f,$(FSCK_MSGIDS_H_LIBS),$f.sp $f.s $f.o): fsck-msgids.h
>>
>> I don't understand the "used within other headers" part here. Computed
>> dependencies will get this right. It's only the initial build (before we
>> have any computed dependencies) that needs to know that the C files in
>> question depend on the generated file. But that is true whether they do
>> so directly or indirectly.
>
> I forgot the full story, but luckily I wrote it down in the WIP commits
> :) FWIW if you want to scour that it's mainly:
>
> https://github.com/avar/git/commit/a00f1cb9ea5 # add advice-type.h
> https://github.com/avar/git/commit/9e080923a11 # generate 'struct advice
>
> Also, before generating builtin.h I've got e.g. this:
>
> https://github.com/avar/git/commit/5a5360d0134 # just check with 'make' that any file is sorted
>
> To actualy generate it, very WIP:
>
> http://github.com/avar/git/commit/cf1d02fa6b2
>
> Anyway, in partial summary, why (and sorry this got so long):
>
> * Yes, once we build e.g. fsck.o we have the full dependency tree,
> yay....
> * ...but only for gcc & clang, but we support other compilers.
> * ...for other compilers (or gcc & clang without the dep detection
> enabled) what should this do:
>
> make file-that-does-not-use-generated-header.o
>
> It sucks a bit to have e.g. every *.o depend on command-list.h, when
> we only use it in 1-2 places, ditto the other headers.
>
> The approach I took to this was to manually list headers that had
> "small uses" (1-10), but say that all of *.h dependend on running the
> *.sh to generate some "widely used" headers.
>
> E.g. if we generate builtin.h we'll either need that, or add
> builtin/%.o as a dependency, with a manual listing of the handful of
> uses outside of that subdirectory.
>
> * .... or just not care about any of that, i.e. to have all of our *.sh
> run on the "first build" no matter what, which certainly simplifies
> things, but once you have e.g. 5-10 generated headers doing e.g.:
>
> make grep.o
>
> and having it build command-list.h or whatever is a bit noisy, but
> it's a trade-off of manually maintaining deps v.s. not.
>
> * Our "COMPUTED_HEADER_DEPENDENCIES" only happens when you build the
> *.o, but we have *.sp and *.s targets too. I have some local changes
> *to generalize that, so we e.g. get proper dependencies for building
> **.s files.
>
> * We also have e.g. "make hdr-check", which works just fine now, but
> it's becausue we have no *.h file that uses another generated *.h
> file.
>
> Actually, I suspect the posted WIP change is broken in that regard
> (but doing this for real on my topic branch works), i.e. we need to
> start caring about deps for those auxiliary targets.
>
> * I may be wrong (I'm just looking at some old "here be dragons" note
> of mine), but I think there's also an edge case where the dependency
> graph of .depend.d/* is correct *once you've always had it*, but if a
> file now e.g. depends on foo.h, and I make foo.h include a new
> foo-generated.h things go boom.
>
> That issue (it if even exists, sorry, I'm blanking on the details)
> was solvable by just doing a "make clean", and building again,
> i.e. once we had re-built once we were OK, it was only a problem with
> an older checkout with an existing build pulling new sources.
>
> Still, I wanted to not make that case annoying either if I added a
> generated header...
next prev parent reply other threads:[~2022-10-28 3:02 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-24 15:00 [PATCH 0/2] Document fsck msg ids John Cai via GitGitGadget
2022-10-24 15:00 ` [PATCH 1/2] fsck: remove the unused BAD_TAG_OBJECT John Cai via GitGitGadget
2022-10-24 16:57 ` Junio C Hamano
2022-10-24 18:16 ` Junio C Hamano
2022-10-24 18:33 ` John Cai
2022-10-24 23:39 ` Jeff King
2022-10-24 15:00 ` [PATCH 2/2] fsck: document msg-id John Cai via GitGitGadget
2022-10-24 17:33 ` Junio C Hamano
2022-10-25 9:41 ` Ævar Arnfjörð Bjarmason
2022-10-25 16:07 ` Junio C Hamano
2022-10-24 18:51 ` [PATCH 0/2] Document fsck msg ids Junio C Hamano
2022-10-25 3:17 ` [PATCH v2 " John Cai via GitGitGadget
2022-10-25 3:17 ` [PATCH v2 1/2] fsck: remove the unused BAD_TAG_OBJECT John Cai via GitGitGadget
2022-10-25 3:17 ` [PATCH v2 2/2] fsck: document msg-id John Cai via GitGitGadget
2022-10-25 4:30 ` [PATCH v2 0/2] Document fsck msg ids Junio C Hamano
2022-10-25 4:40 ` Junio C Hamano
2022-10-25 5:12 ` [PATCH] Documentation: add lint-fsck-msgids Junio C Hamano
2022-10-25 22:42 ` [PATCH v3 0/4] document fsck error message ids Junio C Hamano
2022-10-25 22:42 ` [PATCH v3 1/4] fsck: remove the unused BAD_TAG_OBJECT Junio C Hamano
2022-10-25 22:42 ` [PATCH v3 2/4] fsck: remove the unused MISSING_TREE_OBJECT Junio C Hamano
2022-10-25 22:42 ` [PATCH v3 3/4] fsck: document msg-id Junio C Hamano
2022-10-25 22:42 ` [PATCH v3 4/4] Documentation: add lint-fsck-msgids Junio C Hamano
2022-10-26 2:43 ` Ævar Arnfjörð Bjarmason
2022-10-26 5:34 ` Jeff King
2022-10-26 6:46 ` Junio C Hamano
2022-10-26 11:35 ` Ævar Arnfjörð Bjarmason
2022-10-28 1:23 ` Jeff King
2022-10-28 2:04 ` Ævar Arnfjörð Bjarmason
2022-10-28 5:32 ` Jeff King
2022-10-28 10:41 ` Ævar Arnfjörð Bjarmason
2022-10-28 3:02 ` John Cai [this message]
2022-10-28 3:11 ` Ævar Arnfjörð Bjarmason
2022-10-28 5:32 ` Junio C Hamano
2022-10-28 5:37 ` Jeff King
2022-10-28 5:35 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=08A5BC44-24D9-4C8F-A61A-41983A13553A@gmail.com \
--to=johncai86@gmail.com \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).