[PATCH] docs: include "trace.h" in MyFirstObjectWalk.txt

[PATCH] docs: include "trace.h" in MyFirstObjectWalk.txt

[Vinayak Dev]

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)
 {
@@ -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).

base-commit: a9e066fa63149291a55f383cfa113d8bdbdaa6b3
-- 
2.41.0

Re: [PATCH] docs: include "trace.h" in MyFirstObjectWalk.txt

[Junio C Hamano]

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.

Re: [PATCH] docs: include "trace.h" in MyFirstObjectWalk.txt

[Vinayak Dev]

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

Re: [PATCH] docs: include "trace.h" in MyFirstObjectWalk.txt

[Junio C Hamano]

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.

Re: [PATCH] docs: include "trace.h" in MyFirstObjectWalk.txt

[Vinayak Dev]

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