# [RFC] TODO in read-cache.c 7 messages from 2019-04-06 to 2019-04-09. Participants: Kapil Jain, Duy Nguyen, Taylor Blau. Thread: https://gitlist.dev/t/50882 ## Kapil Jain, 2019-04-06 11:40 Subject: [RFC] TODO in read-cache.c Message-ID: URL: https://gitlist.dev/e/CAMknYEPS68VEkUbNxeKQvVDGjzVpBXKNAi3uA04pLwN9k4ZTfA%40mail.gmail.com ``` 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 ? ``` ## Duy Nguyen, 2019-04-06 12:03 Subject: Re: [RFC] TODO in read-cache.c Message-ID: URL: https://gitlist.dev/e/CACsJy8AnXawOgC0eWKpSF7iGXAvPdP9%3DSZX1HePRABVdkiKs8g%40mail.gmail.com In-Reply-To: ``` On Sat, Apr 6, 2019 at 6:42 PM Kapil Jain 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 ``` ## Kapil Jain, 2019-04-06 12:13 Subject: Re: [RFC] TODO in read-cache.c Message-ID: URL: https://gitlist.dev/e/CAMknYENJogZ6vxs3zxivD3TPtDnfE9DFQDTwri%2BeLJmTwr4zxw%40mail.gmail.com In-Reply-To: ``` On Sat, Apr 6, 2019 at 5:33 PM Duy Nguyen 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. ``` ## Duy Nguyen, 2019-04-06 12:18 Subject: Re: [RFC] TODO in read-cache.c Message-ID: URL: https://gitlist.dev/e/CACsJy8DNb%2BXu_bLAGw3WHECygxMLQHkaqGhJ89SY_yGF%2Bc20bw%40mail.gmail.com In-Reply-To: ``` On Sat, Apr 6, 2019 at 7:14 PM Kapil Jain 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 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 ``` ## Kapil Jain, 2019-04-06 13:30 Subject: Re: [RFC] TODO in read-cache.c Message-ID: URL: https://gitlist.dev/e/CAMknYEMVdH9f-sxyRkfL8OtFjC993ooAf_8z0SGA07%2B86NB66g%40mail.gmail.com In-Reply-To: ``` On Sat, Apr 6, 2019 at 5:49 PM Duy Nguyen wrote: > > On Sat, Apr 6, 2019 at 7:14 PM Kapil Jain 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. ``` ## Duy Nguyen, 2019-04-07 03:04 Subject: Re: [RFC] TODO in read-cache.c Message-ID: URL: https://gitlist.dev/e/CACsJy8DURQdR3gAB4-KDz2mbdoZcXh8-%2BLdmCdzjMRsPv64pQw%40mail.gmail.com In-Reply-To: ``` On Sat, Apr 6, 2019 at 8:30 PM Kapil Jain wrote: > > On Sat, Apr 6, 2019 at 5:49 PM Duy Nguyen wrote: > > > > On Sat, Apr 6, 2019 at 7:14 PM Kapil Jain 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 ``` ## Taylor Blau, 2019-04-09 02:04 Subject: Re: [RFC] TODO in read-cache.c Message-ID: <20190409020416.GB81620@Taylors-MBP.hsd1.wa.comcast.net> URL: https://gitlist.dev/e/20190409020416.GB81620%40Taylors-MBP.hsd1.wa.comcast.net In-Reply-To: ``` 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 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 ```