threads / discuss / 59930

Documentation/MyFirstObjectWalk: add #include "trace.h" to use trace_printf()

Subject: Documentation/MyFirstObjectWalk: add #include "trace.h" to use trace_printf()

## tl;dr

10 messages between Jun 29, 2023 and Jun 30, 2023.

replies: 9people: 3as markdown or json

Vinayak Dev· Jun 29, 2023, 16:05 UTC · lore

Hey there! I was looking through Documentation/MyFirstObjectWalk.txt, and upon building the branch containing the given code, I find that I get the error that C99 does not allow implicit function declaration where trace_printf() is encountered. However, upon including trace.h the error disappears, and the build proceeds just fine.

I did put DEVELOPER=1 in config.mak before building, but it doesn't seem to work.

Is the error pointing to a problem, or am I doing something wrong? If it is the former, I would be very happy to send a patch fixing this.

Thanks a lot! Vinayak

Emily Shaffer· Jun 29, 2023, 16:33 UTC · re: Vinayak Dev · lore

Re: Documentation/MyFirstObjectWalk: add #include "trace.h" to use trace_printf()

On Thu, Jun 29, 2023 at 9:06 AM Vinayak Dev <vinayakdev.sci@gmail.com> wrote:
Show 13 quoted lines
>
> Hey there!
> I was looking through Documentation/MyFirstObjectWalk.txt, and upon
> building the branch containing the given code, I find that I get the
> error that C99 does not allow implicit function declaration where
> trace_printf() is encountered. However, upon including trace.h the
> error disappears, and the build proceeds just fine.
>
> I did put DEVELOPER=1 in config.mak before building, but it doesn't
> seem to work.
>
> Is the error pointing to a problem, or am I doing something wrong?
> If it is the former, I would be very happy to send a patch fixing this.

Yeah, it's almost certainly stale in MyFirstObjectWalk - there was very recently a patch to clean up some headers which probably were implicitly including trace.h when I wrote this walkthrough. Patches totally welcome - and if you were working from the reference code in https://github.com/nasamuffin/git/tree/myfirstrevwalk and it's on your way to rebase and fix that too, I'm happy to update my branch accordingly too. (If you weren't, don't worry about doing the extra work, though.)

>
> Thanks a lot!
> Vinayak
Emily Shaffer· Jun 29, 2023, 16:35 UTC · re: Emily Shaffer · lore

Re: Documentation/MyFirstObjectWalk: add #include "trace.h" to use trace_printf()

On Thu, Jun 29, 2023 at 9:33 AM Emily Shaffer <nasamuffin@google.com> wrote:
Show 21 quoted lines
>
> On Thu, Jun 29, 2023 at 9:06 AM Vinayak Dev <vinayakdev.sci@gmail.com> wrote:
> >
> > Hey there!
> > I was looking through Documentation/MyFirstObjectWalk.txt, and upon
> > building the branch containing the given code, I find that I get the
> > error that C99 does not allow implicit function declaration where
> > trace_printf() is encountered. However, upon including trace.h the
> > error disappears, and the build proceeds just fine.
> >
> > I did put DEVELOPER=1 in config.mak before building, but it doesn't
> > seem to work.
> >
> > Is the error pointing to a problem, or am I doing something wrong?
> > If it is the former, I would be very happy to send a patch fixing this.
>
> Yeah, it's almost certainly stale in MyFirstObjectWalk - there was
> very recently a patch to clean up some headers which probably were
> implicitly including trace.h when I wrote this walkthrough. Patches
> totally welcome - and if you were working from the reference code in
> https://github.com/nasamuffin/git/tree/myfirstrevwalk

bah, wrong link, the tutorial points to branch `revwalk` instead of `myfirstrevwalk`, but the offer stands :)

Show 8 quoted lines
> and it's on your
> way to rebase and fix that too, I'm happy to update my branch
> accordingly too. (If you weren't, don't worry about doing the extra
> work, though.)
>
> >
> > Thanks a lot!
> > Vinayak
Vinayak Dev· Jun 29, 2023, 18:45 UTC · re: Emily Shaffer · lore

Re: Documentation/MyFirstObjectWalk: add #include "trace.h" to use trace_printf()

> On Thu, Jun 29, 2023 at 9:33 AM Emily Shaffer <nasamuffin@google.com> wrote:
Hi, thanks for replying!
Show 13 quoted lines
> > Yeah, it's almost certainly stale in MyFirstObjectWalk - there was
> > very recently a patch to clean up some headers which probably were
> > implicitly including trace.h when I wrote this walkthrough. Patches
> > totally welcome - and if you were working from the reference code in
> > https://github.com/nasamuffin/git/tree/myfirstrevwalk
>
> bah, wrong link, the tutorial points to branch `revwalk` instead of
> `myfirstrevwalk`, but the offer stands :)
>
> > and it's on your
> > way to rebase and fix that too, I'm happy to update my branch
> > accordingly too. (If you weren't, don't worry about doing the extra
> > work, though.)

Sure will! But do you mean open a PR on your fork? I have the patch ready, and would be very happy to do so, if it is accepted!

Thanks a lot! Vinayak

Emily Shaffer· Jun 29, 2023, 23:09 UTC · re: Vinayak Dev · lore

Re: Documentation/MyFirstObjectWalk: add #include "trace.h" to use trace_printf()

On Thu, Jun 29, 2023 at 11:45 AM Vinayak Dev <vinayakdev.sci@gmail.com> wrote:
Show 21 quoted lines
>
> > On Thu, Jun 29, 2023 at 9:33 AM Emily Shaffer <nasamuffin@google.com> wrote:
>
> Hi, thanks for replying!
>
> > > Yeah, it's almost certainly stale in MyFirstObjectWalk - there was
> > > very recently a patch to clean up some headers which probably were
> > > implicitly including trace.h when I wrote this walkthrough. Patches
> > > totally welcome - and if you were working from the reference code in
> > > https://github.com/nasamuffin/git/tree/myfirstrevwalk
> >
> > bah, wrong link, the tutorial points to branch `revwalk` instead of
> > `myfirstrevwalk`, but the offer stands :)
> >
> > > and it's on your
> > > way to rebase and fix that too, I'm happy to update my branch
> > > accordingly too. (If you weren't, don't worry about doing the extra
> > > work, though.)
>
> Sure will! But do you mean open a PR on your fork? I have the patch ready,
> and would be very happy to do so, if it is accepted!
Yeah, I think there are two things to fix:

First, a patch to Documentation/technical/MyFirstObjectWalk.txt fixing the snippets there. (I thought that was what you were offering to patch in your original mail, I may have been mistaken.)

Second, optionally, a rebased-and-fixed-and-your-attribution-added branch of the reference impl that I can force-push to nasamuffin/git. The more I think on it, I don't think the PR will help, since I will want to just force-push that whole branch so the commit order still functions as a learning tool. So if you have it even in a branch on your GitHub or GitLab fork, or a series of patches you'd want to mail to me, any of those are fine and I'll go ahead and rewrite my branch.

>
> Thanks a lot!
> Vinayak
Vinayak Dev· Jun 30, 2023, 05:48 UTC · re: Emily Shaffer · lore

Re: Documentation/MyFirstObjectWalk: add #include "trace.h" to use trace_printf()

On Fri, 30 Jun 2023 at 04:39, Emily Shaffer <nasamuffin@google.com> wrote:
Show 13 quoted lines
> Yeah, I think there are two things to fix:
>
> First, a patch to Documentation/technical/MyFirstObjectWalk.txt fixing
> the snippets there. (I thought that was what you were offering to
> patch in your original mail, I may have been mistaken.)
>
> Second, optionally, a rebased-and-fixed-and-your-attribution-added
> branch of the reference impl that I can force-push to nasamuffin/git.
> The more I think on it, I don't think the PR will help, since I will
> want to just force-push that whole branch so the commit order still
> functions as a learning tool. So if you have it even in a branch on
> your GitHub or GitLab fork, or a series of patches you'd want to mail
> to me, any of those are fine and I'll go ahead and rewrite my branch.

I will be pushing MyFirstObjectWalk's implementation to my Github fork, but I might need a day's time to do that(I don't want to leave behind any mistakes). You are right, a PR surely does not seem to be the best way to do this. As soon as I finish(shouldn't take too much time), I will reply with a link to the branch. Would that be alright?

Thanks a lot! Vinayak

Vinayak Dev· Jun 30, 2023, 15:39 UTC · re: Vinayak Dev · lore

Re: Documentation/MyFirstObjectWalk: add #include "trace.h" to use trace_printf()

Hi! I pulled down walken.c from https://github.com/nasamuffin/git/tree/revwalk and was able to fix the broken code. I also fixed Documentation/MyFirstObjectWalk.txt and have accordingly pushed all the changes to my fork of git:

https://github.com/vinayakdsci/git/tree/revwalk-fixed

I had to remove init_walken_defaults() as I could not trace the function init_grep_defaults() which I think has been removed since you wrote the tutorial. I also didn't find a mention of init_grep_defaults() in the tutorial, so maybe that is alright. Also, struct list_objects_filter_options is included inside of rev_info, so I don't think it requires initialisation any more.

It would be great if you are able to use this branch to rewrite your own!

Thanks a lot! Vinayak

Junio C Hamano· Jun 29, 2023, 19:28 UTC · re: Emily Shaffer · lore

Re: Documentation/MyFirstObjectWalk: add #include "trace.h" to use trace_printf()

Emily Shaffer <nasamuffin@google.com> writes:
> Yeah, it's almost certainly stale in MyFirstObjectWalk - there was
> very recently a patch to clean up some headers which probably were
> implicitly including trace.h when I wrote this walkthrough.

We are lucky that we have folks like Vinayak who tried out the examples and then bothered to spend time reporting the failure discovered. What does it take, however, for us to have a bit more automated way to prevent such a breakage that comes from API changes? Is it feasible, for example, to add a test that extracts code snippets from the MyFirstObjectWalk document and try to build the result? Alternatively, we can ship such a set of sample source files somewhere in our tree (e.g. contrib/examples?) and have such a test try to build using the current set of source files, but then we need a mechansim to ensure that the sample source files will not go out of sync with the document.

Thoughts?
Thanks.
Emily Shaffer· Jun 29, 2023, 23:06 UTC · re: Junio C Hamano · lore

Re: Documentation/MyFirstObjectWalk: add #include "trace.h" to use trace_printf()

On Thu, Jun 29, 2023 at 12:28 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 18 quoted lines
>
> Emily Shaffer <nasamuffin@google.com> writes:
>
> > Yeah, it's almost certainly stale in MyFirstObjectWalk - there was
> > very recently a patch to clean up some headers which probably were
> > implicitly including trace.h when I wrote this walkthrough.
>
> We are lucky that we have folks like Vinayak who tried out the
> examples and then bothered to spend time reporting the failure
> discovered.  What does it take, however, for us to have a bit more
> automated way to prevent such a breakage that comes from API
> changes?  Is it feasible, for example, to add a test that extracts
> code snippets from the MyFirstObjectWalk document and try to build
> the result?  Alternatively, we can ship such a set of sample source
> files somewhere in our tree (e.g. contrib/examples?) and have such
> a test try to build using the current set of source files, but then
> we need a mechansim to ensure that the sample source files will not
> go out of sync with the document.

Yeah, I remember we talked about this when MyFirstContribution and MyFirstObjectWalk went in, but never made much headway. I do very much like the idea of keeping the reference source in contrib/ as a set of patches, maybe along with a script to apply them (or a readme with the right `git am` invocation), and then checking that they still build. Checking that against the contents of the document is trickier, though, like you mentioned. Hm.

I'm interested in figuring out how to do this, but not likely to have a lot of development time available to do it. Maybe I can take a day here or there to poke at it, but if someone else is interested and beats me to it, I will not be disappointed. :)

Show 5 quoted lines
>
> Thoughts?
>
> Thanks.
>
Junio C Hamano· Jun 30, 2023, 02:21 UTC · re: Emily Shaffer · lore

Re: Documentation/MyFirstObjectWalk: add #include "trace.h" to use trace_printf()

Emily Shaffer <nasamuffin@google.com> writes:
Show 6 quoted lines
> ... I do very much
> like the idea of keeping the reference source in contrib/ as a set of
> patches, maybe along with a script to apply them (or a readme with the
> right `git am` invocation), and then checking that they still build.
> Checking that against the contents of the document is trickier,
> though, like you mentioned. Hm.

An approach to avoid two things (i.e. sample source and the code snippet in the documentation) going out of sync is not to have two of them in the first place. If we give up readability of the MFOW document in its source form, you may be able to arrange the code snippet to be incorporated by the build test and documentation at the same time.

← back to recent threads