| Message ID | 20230629185238.58961-1-vinayakdev.sci@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | docs: include "trace.h" in MyFirstObjectWalk.txt | expand |
Vinayak Dev <vinayakdev.sci@gmail.com> writes: [jc: including Elijah, who owns a few topics of the recent past that shuffled header files, to recipients]. > In Documentation/MyFirstObjectWalk.txt, the function > trace_printf() is used to enable trace output. > However, the file "trace.h" is not included, which > produces an error when the code from the tutorial is > compiled, with the compiler complaining that the > function is not defined before usage. Therefore, add > an include for "trace.h" in the tutorial. > > Signed-off-by: Vinayak Dev <vinayakdev.sci@gmail.com> > --- > Documentation/MyFirstObjectWalk.txt | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt > index eee513e86f..c3a23eb100 100644 > --- a/Documentation/MyFirstObjectWalk.txt > +++ b/Documentation/MyFirstObjectWalk.txt > @@ -41,6 +41,7 @@ Open up a new file `builtin/walken.c` and set up the command handler: > */ > > #include "builtin.h" > +#include "trace.h" > > int cmd_walken(int argc, const char **argv, const char *prefix) > { OK. > @@ -49,12 +50,13 @@ int cmd_walken(int argc, const char **argv, const char *prefix) > } > ---- > > -NOTE: `trace_printf()` differs from `printf()` in that it can be turned on or > -off at runtime. For the purposes of this tutorial, we will write `walken` as > -though it is intended for use as a "plumbing" command: that is, a command which > -is used primarily in scripts, rather than interactively by humans (a "porcelain" > -command). So we will send our debug output to `trace_printf()` instead. When > -running, enable trace output by setting the environment variable `GIT_TRACE`. > +NOTE: `trace_printf()`, defined in `trace.h`, differs from `printf()` in > +that it can be turned on or off at runtime. For the purposes of this > +tutorial, we will write `walken` as though it is intended for use as > +a "plumbing" command: that is, a command which is used primarily in > +scripts, rather than interactively by humans (a "porcelain" command). > +So we will send our debug output to `trace_printf()` instead. > +When running, enable trace output by setting the environment variable `GIT_TRACE`. All of the above may be good currently, but as soon as somebody does another round of header shuffling, we risk a very similar breakage. It is a good time to think about ways to avoid that, while the pain is fresh in our mind. One "cop out" thing we can do to limit the damage may be to loosen the text in the "NOTE:", so that it does *NOT* mention exact header files the API functions appear and let the readers learn from the source themselves, with "git grep" helping their way. Or tone down "defined in X" somehow to hint that these details may change. More effective things that would involve higher implementation cost (but will reduce maintenance cost) would be to actually make sure that those who update the API will notice that they are breaking these samples when they develop their series. In https://lore.kernel.org/git/xmqq1qhu9ifp.fsf@gitster.g/, I've floated some strawman ideas but people may be able to invent better ways.
On Fri, 30 Jun 2023 at 02:43, Junio C Hamano <gitster@pobox.com> wrote: > Hi, thanks for replying! > All of the above may be good currently, but as soon as somebody does > another round of header shuffling, we risk a very similar breakage. > It is a good time to think about ways to avoid that, while the pain > is fresh in our mind. > > One "cop out" thing we can do to limit the damage may be to loosen > the text in the "NOTE:", so that it does *NOT* mention exact header > files the API functions appear and let the readers learn from the > source themselves, with "git grep" helping their way. Or tone down > "defined in X" somehow to hint that these details may change. That is a good suggestion, but wouldn't the same argument apply to including trace.h itself? It makes it necessary for the code to work that any API changes must involve changing the included headers. Either way, I would be happy to fix my mistake. Should I send out a V2? > More effective things that would involve higher implementation cost > (but will reduce maintenance cost) would be to actually make sure > that those who update the API will notice that they are breaking > these samples when they develop their series. > > In https://lore.kernel.org/git/xmqq1qhu9ifp.fsf@gitster.g/, I've > floated some strawman ideas but people may be able to invent better > ways. I do want to be a part of the discussion and the solution, so if I am able to find a reasonable way(or to implement a solution), I would inform everyone for sure. Thanks a lot! Vinayak
Vinayak Dev <vinayakdev.sci@gmail.com> writes: > That is a good suggestion, but wouldn't the same argument apply to > including trace.h itself? It makes it necessary for the code to work > that any API changes must involve changing the included headers. Exactly. That is exactly what I meant by "tone down 'defined in X' somehow to hint that these details may change". IOW, it may allow us to "cop out" of the problem to say "You'd add a new file like so, and call this API function, and include headers necessary to do so. The codebase may have evolved since this tutorial was written, so some details (like names of the API functions and in which header files the functions are declared) may be different in the code you have." > Either way, I would be happy to fix my mistake. Should I send out a V2? Oh, sorry to see you took it that way. I think the change you sent as-is is a fine fix for the immediate problem. Everything you quoted above is "while the issue is fresh on our minds, what are the follow-up improvements we can make" material, so there is no "mistake" to fix.
On Fri, 30 Jun 2023 at 21:55, Junio C Hamano <gitster@pobox.com> wrote: Hello again, and thanks for replying! > Vinayak Dev <vinayakdev.sci@gmail.com> writes: > > > That is a good suggestion, but wouldn't the same argument apply to > > including trace.h itself? It makes it necessary for the code to work > > that any API changes must involve changing the included headers. > > Exactly. That is exactly what I meant by "tone down 'defined in X' > somehow to hint that these details may change". > > IOW, it may allow us to "cop out" of the problem to say "You'd add a > new file like so, and call this API function, and include headers > necessary to do so. The codebase may have evolved since this > tutorial was written, so some details (like names of the API > functions and in which header files the functions are declared) may > be different in the code you have." > > > Either way, I would be happy to fix my mistake. Should I send out a V2? > > Oh, sorry to see you took it that way. I think the change you sent > as-is is a fine fix for the immediate problem. Everything you > quoted above is "while the issue is fresh on our minds, what are the > follow-up improvements we can make" material, so there is no "mistake" > to fix. That is certainly nice to hear! However, I must admit that I did make a mistake in the patch in that I missed to include "hex.h" down in the file. This is needed because the tutorial uses the function oid_to_hex(), which is defined there. I found this while fixing the code in Emily's branch to which the tutorial points. I have fixed that there, so maybe if you think a V2 would be alright, I would be happy to send it fixing both the problems. Thanks a lot! Vinayak
diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt index eee513e86f..c3a23eb100 100644 --- a/Documentation/MyFirstObjectWalk.txt +++ b/Documentation/MyFirstObjectWalk.txt @@ -41,6 +41,7 @@ Open up a new file `builtin/walken.c` and set up the command handler: */ #include "builtin.h" +#include "trace.h" int cmd_walken(int argc, const char **argv, const char *prefix) { @@ -49,12 +50,13 @@ int cmd_walken(int argc, const char **argv, const char *prefix) } ---- -NOTE: `trace_printf()` differs from `printf()` in that it can be turned on or -off at runtime. For the purposes of this tutorial, we will write `walken` as -though it is intended for use as a "plumbing" command: that is, a command which -is used primarily in scripts, rather than interactively by humans (a "porcelain" -command). So we will send our debug output to `trace_printf()` instead. When -running, enable trace output by setting the environment variable `GIT_TRACE`. +NOTE: `trace_printf()`, defined in `trace.h`, differs from `printf()` in +that it can be turned on or off at runtime. For the purposes of this +tutorial, we will write `walken` as though it is intended for use as +a "plumbing" command: that is, a command which is used primarily in +scripts, rather than interactively by humans (a "porcelain" command). +So we will send our debug output to `trace_printf()` instead. +When running, enable trace output by setting the environment variable `GIT_TRACE`. Add usage text and `-h` handling, like all subcommands should consistently do (our test suite will notice and complain if you fail to do so).
In Documentation/MyFirstObjectWalk.txt, the function trace_printf() is used to enable trace output. However, the file "trace.h" is not included, which produces an error when the code from the tutorial is compiled, with the compiler complaining that the function is not defined before usage. Therefore, add an include for "trace.h" in the tutorial. Signed-off-by: Vinayak Dev <vinayakdev.sci@gmail.com> --- Documentation/MyFirstObjectWalk.txt | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) base-commit: a9e066fa63149291a55f383cfa113d8bdbdaa6b3