{"thread":{"id":"50882","subject":"[RFC] TODO in read-cache.c","startedAt":"2019-04-06T11:40:54Z","lastAt":"2019-04-09T02:04:20Z","messageCount":7,"participants":["Kapil Jain","Duy Nguyen","Taylor Blau"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"373318","messageId":"CAMknYEPS68VEkUbNxeKQvVDGjzVpBXKNAi3uA04pLwN9k4ZTfA@mail.gmail.com","threadId":"50882","inReplyTo":null,"subject":"[RFC] TODO in read-cache.c","fromName":"Kapil Jain","fromEmail":"jkapil.cs@gmail.com","sentAt":"2019-04-06T11:40:41Z","receivedAt":"2019-04-06T11:40:54Z","isPatch":false,"sender":{"key":"jkapil.cs@gmail.com","avatar":null},"body":"i found some TODO tasks inside `read-cache.c` in `read_index_from()`\nfunction. which says:\n\n/*\n* TODO trace2: replace \"the_repository\" with the actual repo instance\nthat is associated with the given \"istate\".\n*/\n\nthis same TODO occurs at 4 other places in the same file.\n\nWill it be ok, if i complete this TODO by modifying the trace2's\nfunction signatures to accept `struct repository`\nand change the calls to those functions accordingly ?\n"},{"id":"373320","messageId":"CACsJy8AnXawOgC0eWKpSF7iGXAvPdP9=SZX1HePRABVdkiKs8g@mail.gmail.com","threadId":"50882","inReplyTo":"CAMknYEPS68VEkUbNxeKQvVDGjzVpBXKNAi3uA04pLwN9k4ZTfA@mail.gmail.com","subject":"Re: [RFC] TODO in read-cache.c","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-04-06T12:03:15Z","receivedAt":"2019-04-06T12:03:43Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Apr 6, 2019 at 6:42 PM Kapil Jain wrote:\n>\n> i found some TODO tasks inside `read-cache.c` in `read_index_from()`\n> function. which says:\n>\n> /*\n> * TODO trace2: replace \"the_repository\" with the actual repo instance\n> that is associated with the given \"istate\".\n> */\n>\n> this same TODO occurs at 4 other places in the same file.\n>\n> Will it be ok, if i complete this TODO by modifying the trace2's\n> function signatures to accept `struct repository`\n> and change the calls to those functions accordingly ?\n\ntrace2 API can already take 'struct repository' (the_repository is a\npointer to 'struct repository'). I'm pretty sure the purpose is to\n_not_ pass the_repository (because it implies the default repo, which\nis not always true). Which means you read-cache.c's functions need to\ntake 'struct repository *' as an argument and let the caller decide\nwhat repo they want to use.\n\nIn some cases, it will be simple. For example, if you have a look at\nrepo_read_index(), it already knows what repo it handles, so you can\njust extend read_index_from() to take 'struct repository *' and pass\n'repo' to it.\n\nBe careful though, repository and istate does not have one-to-one\nrelationship (I'll leave it to you to find out why). So you cannot\nreplace\n\n return read_index_from(repo->index, repo->index_file, repo->gitdir);\n\nin that function with\n\n return read_index_from(repo);\n\nand make read_index_from() use 'repo->index'. It will have to be\n\n return read_index_from(repo, repo->index, repo->index_file);\n-- \nDuy\n"},{"id":"373321","messageId":"CAMknYENJogZ6vxs3zxivD3TPtDnfE9DFQDTwri+eLJmTwr4zxw@mail.gmail.com","threadId":"50882","inReplyTo":"CACsJy8AnXawOgC0eWKpSF7iGXAvPdP9=SZX1HePRABVdkiKs8g@mail.gmail.com","subject":"Re: [RFC] TODO in read-cache.c","fromName":"Kapil Jain","fromEmail":"jkapil.cs@gmail.com","sentAt":"2019-04-06T12:13:56Z","receivedAt":"2019-04-06T12:14:10Z","isPatch":false,"sender":{"key":"jkapil.cs@gmail.com","avatar":null},"body":"On Sat, Apr 6, 2019 at 5:33 PM Duy Nguyen wrote:\n>\n> trace2 API can already take 'struct repository' (the_repository is a\n> pointer to 'struct repository'). I'm pretty sure the purpose is to\n> _not_ pass the_repository (because it implies the default repo, which\n> is not always true). Which means you read-cache.c's functions need to\n> take 'struct repository *' as an argument and let the caller decide\n> what repo they want to use.\n\nright, i mistyped.\n\n> In some cases, it will be simple. For example, if you have a look at\n> repo_read_index(), it already knows what repo it handles, so you can\n> just extend read_index_from() to take 'struct repository *' and pass\n> 'repo' to it.\n>\n> Be careful though, repository and istate does not have one-to-one\n> relationship (I'll leave it to you to find out why). So you cannot\n> replace\n\nshould i run all the tests after making the changes, or are there some\nspecific ones.\n"},{"id":"373322","messageId":"CACsJy8DNb+Xu_bLAGw3WHECygxMLQHkaqGhJ89SY_yGF+c20bw@mail.gmail.com","threadId":"50882","inReplyTo":"CAMknYENJogZ6vxs3zxivD3TPtDnfE9DFQDTwri+eLJmTwr4zxw@mail.gmail.com","subject":"Re: [RFC] TODO in read-cache.c","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-04-06T12:18:53Z","receivedAt":"2019-04-06T12:19:22Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Apr 6, 2019 at 7:14 PM Kapil Jain wrote:\n> > In some cases, it will be simple. For example, if you have a look at\n> > repo_read_index(), it already knows what repo it handles, so you can\n> > just extend read_index_from() to take 'struct repository *' and pass\n> > 'repo' to it.\n> >\n> > Be careful though, repository and istate does not have one-to-one\n> > relationship (I'll leave it to you to find out why). So you cannot\n> > replace\n>\n> should i run all the tests after making the changes, or are there some\n> specific ones.\n\n'make test' (with -j to speed up) should always be done for\nany kind of changes. But I'm pretty sure you'll hit plenty compiler\nerrors that will make you pause and think.\n-- \nDuy\n"},{"id":"373327","messageId":"CAMknYEMVdH9f-sxyRkfL8OtFjC993ooAf_8z0SGA07+86NB66g@mail.gmail.com","threadId":"50882","inReplyTo":"CACsJy8DNb+Xu_bLAGw3WHECygxMLQHkaqGhJ89SY_yGF+c20bw@mail.gmail.com","subject":"Re: [RFC] TODO in read-cache.c","fromName":"Kapil Jain","fromEmail":"jkapil.cs@gmail.com","sentAt":"2019-04-06T13:30:26Z","receivedAt":"2019-04-06T13:30:40Z","isPatch":false,"sender":{"key":"jkapil.cs@gmail.com","avatar":null},"body":"On Sat, Apr 6, 2019 at 5:49 PM Duy Nguyen wrote:\n>\n> On Sat, Apr 6, 2019 at 7:14 PM Kapil Jain wrote:\n> > > In some cases, it will be simple. For example, if you have a look at\n> > > repo_read_index(), it already knows what repo it handles, so you can\n> > > just extend read_index_from() to take 'struct repository *' and pass\n> > > 'repo' to it.\n> > >\n> > > Be careful though, repository and istate does not have one-to-one\n> > > relationship (I'll leave it to you to find out why). So you cannot\n> > > replace\n> >\n\nat a lot of place where, read_index_from() is called, the repo struct\nis not available, so i am passing `the_repository` in those calls.\nthis makes me wonder if this is really required, because most of the\nplaces just don't have repo.\n"},{"id":"373341","messageId":"CACsJy8DURQdR3gAB4-KDz2mbdoZcXh8-+LdmCdzjMRsPv64pQw@mail.gmail.com","threadId":"50882","inReplyTo":"CAMknYEMVdH9f-sxyRkfL8OtFjC993ooAf_8z0SGA07+86NB66g@mail.gmail.com","subject":"Re: [RFC] TODO in read-cache.c","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-04-07T03:04:54Z","receivedAt":"2019-04-07T03:05:23Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Apr 6, 2019 at 8:30 PM Kapil Jain wrote:\n>\n> On Sat, Apr 6, 2019 at 5:49 PM Duy Nguyen wrote:\n> >\n> > On Sat, Apr 6, 2019 at 7:14 PM Kapil Jain wrote:\n> > > > In some cases, it will be simple. For example, if you have a look at\n> > > > repo_read_index(), it already knows what repo it handles, so you can\n> > > > just extend read_index_from() to take 'struct repository *' and pass\n> > > > 'repo' to it.\n> > > >\n> > > > Be careful though, repository and istate does not have one-to-one\n> > > > relationship (I'll leave it to you to find out why). So you cannot\n> > > > replace\n> > >\n>\n> at a lot of place where, read_index_from() is called, the repo struct\n> is not available, so i am passing `the_repository` in those calls.\n> this makes me wonder if this is really required, because most of the\n> places just don't have repo.\n\nWe're still in a transition period where many places still assume the\ndefault repo (so yes don't have \"repo\" or \"r\" argument). Once\neverything is converted, the_repository should only appear in very few\nplaces and will be passed down as \"r\" argument to all functions.\n-- \nDuy\n"},{"id":"373447","messageId":"20190409020416.GB81620@Taylors-MBP.hsd1.wa.comcast.net","threadId":"50882","inReplyTo":"CAMknYENJogZ6vxs3zxivD3TPtDnfE9DFQDTwri+eLJmTwr4zxw@mail.gmail.com","subject":"Re: [RFC] TODO in read-cache.c","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-04-09T02:04:16Z","receivedAt":"2019-04-09T02:04:20Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Kapil,\n\nWelcome to Git! I am thrilled to see new faces on the mailing list.\n\nOn Sat, Apr 06, 2019 at 05:43:56PM +0530, Kapil Jain wrote:\n> On Sat, Apr 6, 2019 at 5:33 PM Duy Nguyen wrote:\n> >\n> > trace2 API can already take 'struct repository' (the_repository is a\n> > pointer to 'struct repository'). I'm pretty sure the purpose is to\n> > _not_ pass the_repository (because it implies the default repo, which\n> > is not always true). Which means you read-cache.c's functions need to\n> > take 'struct repository *' as an argument and let the caller decide\n> > what repo they want to use.\n>\n> right, i mistyped.\n>\n> > In some cases, it will be simple. For example, if you have a look at\n> > repo_read_index(), it already knows what repo it handles, so you can\n> > just extend read_index_from() to take 'struct repository *' and pass\n> > 'repo' to it.\n> >\n> > Be careful though, repository and istate does not have one-to-one\n> > relationship (I'll leave it to you to find out why). So you cannot\n> > replace\n>\n> should i run all the tests after making the changes, or are there some\n> specific ones.\n\nIt is a good rule of thumb to run 'make' in the testing directory (this\nis 't') at least once before sending patches to the list.\n\nGenerally when I am writing something, I will often be fixing some bug\nand either (1) have a test that I know I am trying to fix, or (2) write\na test that is initially broken, which I then aim to fix.\n\nYou can see a good example of this in the last series that I sent to the\nlist [1]. In 2/7, I introduced several failing tests, and then fixed\nthem in the later commits in the series.\n\nI also like to have a full suite of tests run on multiple platforms\nbefore sending to the list. I have my fork [2] configured with TravisCI,\nwhich runs builds on a number of different architectures/compilers for\ncompleteness.\n\nGit already has a .travis.yml in the repository, so you don't have to do\nany work other than authenticate with TravisCI.\n\nThanks,\nTaylor\n\n[1]: https://public-inbox.org/git/cover.1554435033.git.me@ttaylorr.com/\n[2]: https://github.com/ttaylorr/git\n"}]}