[RFC] TODO in read-cache.c

[RFC] TODO in read-cache.c

[Kapil Jain]

i found some TODO tasks inside `read-cache.c` in `read_index_from()`
function. which says:

/*
* TODO trace2: replace "the_repository" with the actual repo instance
that is associated with the given "istate".
*/

this same TODO occurs at 4 other places in the same file.

Will it be ok, if i complete this TODO by modifying the trace2's
function signatures to accept `struct repository`
and change the calls to those functions accordingly ?

Re: [RFC] TODO in read-cache.c

[Duy Nguyen]

On Sat, Apr 6, 2019 at 6:42 PM Kapil Jain <jkapil.cs@gmail.com> wrote:
>
> i found some TODO tasks inside `read-cache.c` in `read_index_from()`
> function. which says:
>
> /*
> * TODO trace2: replace "the_repository" with the actual repo instance
> that is associated with the given "istate".
> */
>
> this same TODO occurs at 4 other places in the same file.
>
> Will it be ok, if i complete this TODO by modifying the trace2's
> function signatures to accept `struct repository`
> and change the calls to those functions accordingly ?

trace2 API can already take 'struct repository' (the_repository is a
pointer to 'struct repository'). I'm pretty sure the purpose is to
_not_ pass the_repository (because it implies the default repo, which
is not always true). Which means you read-cache.c's functions need to
take 'struct repository *' as an argument and let the caller decide
what repo they want to use.

In some cases, it will be simple. For example, if you have a look at
repo_read_index(), it already knows what repo it handles, so you can
just extend read_index_from() to take 'struct repository *' and pass
'repo' to it.

Be careful though, repository and istate does not have one-to-one
relationship (I'll leave it to you to find out why). So you cannot
replace

 return read_index_from(repo->index, repo->index_file, repo->gitdir);

in that function with

 return read_index_from(repo);

and make read_index_from() use 'repo->index'. It will have to be

 return read_index_from(repo, repo->index, repo->index_file);
-- 
Duy

Re: [RFC] TODO in read-cache.c

[Kapil Jain]

On Sat, Apr 6, 2019 at 5:33 PM Duy Nguyen <pclouds@gmail.com> wrote:
>
> trace2 API can already take 'struct repository' (the_repository is a
> pointer to 'struct repository'). I'm pretty sure the purpose is to
> _not_ pass the_repository (because it implies the default repo, which
> is not always true). Which means you read-cache.c's functions need to
> take 'struct repository *' as an argument and let the caller decide
> what repo they want to use.

right, i mistyped.

> In some cases, it will be simple. For example, if you have a look at
> repo_read_index(), it already knows what repo it handles, so you can
> just extend read_index_from() to take 'struct repository *' and pass
> 'repo' to it.
>
> Be careful though, repository and istate does not have one-to-one
> relationship (I'll leave it to you to find out why). So you cannot
> replace

should i run all the tests after making the changes, or are there some
specific ones.

Re: [RFC] TODO in read-cache.c

[Duy Nguyen]

On Sat, Apr 6, 2019 at 7:14 PM Kapil Jain <jkapil.cs@gmail.com> wrote:
> > In some cases, it will be simple. For example, if you have a look at
> > repo_read_index(), it already knows what repo it handles, so you can
> > just extend read_index_from() to take 'struct repository *' and pass
> > 'repo' to it.
> >
> > Be careful though, repository and istate does not have one-to-one
> > relationship (I'll leave it to you to find out why). So you cannot
> > replace
>
> should i run all the tests after making the changes, or are there some
> specific ones.

'make test' (with -j<something> to speed up) should always be done for
any kind of changes. But I'm pretty sure you'll hit plenty compiler
errors that will make you pause and think.
-- 
Duy

Re: [RFC] TODO in read-cache.c

[Kapil Jain]

On Sat, Apr 6, 2019 at 5:49 PM Duy Nguyen <pclouds@gmail.com> wrote:
>
> On Sat, Apr 6, 2019 at 7:14 PM Kapil Jain <jkapil.cs@gmail.com> wrote:
> > > In some cases, it will be simple. For example, if you have a look at
> > > repo_read_index(), it already knows what repo it handles, so you can
> > > just extend read_index_from() to take 'struct repository *' and pass
> > > 'repo' to it.
> > >
> > > Be careful though, repository and istate does not have one-to-one
> > > relationship (I'll leave it to you to find out why). So you cannot
> > > replace
> >

at a lot of place where, read_index_from() is called, the repo struct
is not available, so i am passing `the_repository` in those calls.
this makes me wonder if this is really required, because most of the
places just don't have repo.

Re: [RFC] TODO in read-cache.c

[Duy Nguyen]

On Sat, Apr 6, 2019 at 8:30 PM Kapil Jain <jkapil.cs@gmail.com> wrote:
>
> On Sat, Apr 6, 2019 at 5:49 PM Duy Nguyen <pclouds@gmail.com> wrote:
> >
> > On Sat, Apr 6, 2019 at 7:14 PM Kapil Jain <jkapil.cs@gmail.com> wrote:
> > > > In some cases, it will be simple. For example, if you have a look at
> > > > repo_read_index(), it already knows what repo it handles, so you can
> > > > just extend read_index_from() to take 'struct repository *' and pass
> > > > 'repo' to it.
> > > >
> > > > Be careful though, repository and istate does not have one-to-one
> > > > relationship (I'll leave it to you to find out why). So you cannot
> > > > replace
> > >
>
> at a lot of place where, read_index_from() is called, the repo struct
> is not available, so i am passing `the_repository` in those calls.
> this makes me wonder if this is really required, because most of the
> places just don't have repo.

We're still in a transition period where many places still assume the
default repo (so yes don't have "repo" or "r" argument). Once
everything is converted, the_repository should only appear in very few
places and will be passed down as "r" argument to all functions.
-- 
Duy

Re: [RFC] TODO in read-cache.c

[Taylor Blau]

Hi Kapil,

Welcome to Git! I am thrilled to see new faces on the mailing list.

On Sat, Apr 06, 2019 at 05:43:56PM +0530, Kapil Jain wrote:
> On Sat, Apr 6, 2019 at 5:33 PM Duy Nguyen <pclouds@gmail.com> wrote:
> >
> > trace2 API can already take 'struct repository' (the_repository is a
> > pointer to 'struct repository'). I'm pretty sure the purpose is to
> > _not_ pass the_repository (because it implies the default repo, which
> > is not always true). Which means you read-cache.c's functions need to
> > take 'struct repository *' as an argument and let the caller decide
> > what repo they want to use.
>
> right, i mistyped.
>
> > In some cases, it will be simple. For example, if you have a look at
> > repo_read_index(), it already knows what repo it handles, so you can
> > just extend read_index_from() to take 'struct repository *' and pass
> > 'repo' to it.
> >
> > Be careful though, repository and istate does not have one-to-one
> > relationship (I'll leave it to you to find out why). So you cannot
> > replace
>
> should i run all the tests after making the changes, or are there some
> specific ones.

It is a good rule of thumb to run 'make' in the testing directory (this
is 't') at least once before sending patches to the list.

Generally when I am writing something, I will often be fixing some bug
and either (1) have a test that I know I am trying to fix, or (2) write
a test that is initially broken, which I then aim to fix.

You can see a good example of this in the last series that I sent to the
list [1]. In 2/7, I introduced several failing tests, and then fixed
them in the later commits in the series.

I also like to have a full suite of tests run on multiple platforms
before sending to the list. I have my fork [2] configured with TravisCI,
which runs builds on a number of different architectures/compilers for
completeness.

Git already has a .travis.yml in the repository, so you don't have to do
any work other than authenticate with TravisCI.

Thanks,
Taylor

[1]: https://public-inbox.org/git/cover.1554435033.git.me@ttaylorr.com/
[2]: https://github.com/ttaylorr/git