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

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

[Vinayak Dev]

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

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

[Emily Shaffer]

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 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

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

[Emily Shaffer]

On Thu, Jun 29, 2023 at 9:33 AM Emily Shaffer <nasamuffin@google.com> wrote:
>
> 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 :)

> 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

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

[Vinayak Dev]

> 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!

Thanks a lot!
Vinayak

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

[Emily Shaffer]

On Thu, Jun 29, 2023 at 11:45 AM Vinayak Dev <vinayakdev.sci@gmail.com> wrote:
>
> > 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

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

[Vinayak Dev]

On Fri, 30 Jun 2023 at 04:39, Emily Shaffer <nasamuffin@google.com> wrote:
> 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

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

[Vinayak Dev]

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

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

[Junio C Hamano]

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.

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

[Emily Shaffer]

On Thu, Jun 29, 2023 at 12:28 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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. :)

>
> Thoughts?
>
> Thanks.
>

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

[Junio C Hamano]

Emily Shaffer <nasamuffin@google.com> writes:

> ... 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.