2016-08-25 14:09 GMT+03:00 Pino Toscano <ptoscano@redhat.com>:> On Wednesday, 24 August 2016 23:59:53 CEST Matteo Cafasso wrote: > > The find_inode API allows the User to search all the entries referring > > to a given inode and returns a tsk_dirent structure for each of them. > > > > As I didn't want to change unrelated code, there is a little bit > > of code duplication at the moment. Plan is to refactor the logic > > in a dedicated set of patches. > > The general idea looks ok, but I'd rather see the duplication dealt > with sooner than later. >In the previous submissions, non related changes were rejected therefore I thought that was the custom. Moreover I'll add another API find_block (block_number --> tsk_dirents referring to it) and I think is easier to refactor the code once all the use cases are in place as the picture gets more clear.> Thanks, > -- > Pino Toscano > _______________________________________________ > Libguestfs mailing list > Libguestfs@redhat.com > https://www.redhat.com/mailman/listinfo/libguestfs >
On Thursday, 25 August 2016 16:05:47 CEST NoxDaFox wrote:> 2016-08-25 14:09 GMT+03:00 Pino Toscano <ptoscano@redhat.com>: > > > On Wednesday, 24 August 2016 23:59:53 CEST Matteo Cafasso wrote: > > > The find_inode API allows the User to search all the entries referring > > > to a given inode and returns a tsk_dirent structure for each of them. > > > > > > As I didn't want to change unrelated code, there is a little bit > > > of code duplication at the moment. Plan is to refactor the logic > > > in a dedicated set of patches. > > > > The general idea looks ok, but I'd rather see the duplication dealt > > with sooner than later. > > > In the previous submissions, non related changes were rejected therefore I > thought that was the custom.I don't see how the two cases are the same: what was "rejected" was a single patch contaning both additions documented in the commit message, and unrelated changes such as formatting fixes. I remember to have said that it's a no-go as *single* patch, but they can be sent (and integrated) as different commits. In this case, refactoring and de-duplication of code should be done in different commits as well.> Moreover I'll add another API find_block (block_number --> tsk_dirents > referring to it) and I think is easier to refactor the code once all the > use cases are in place as the picture gets more clear.More reasons to refactor *before*: since you already planned more APIs, it's easier to just refactor one in advance with all the common code needed, and use it later. Also, it eases a lot the review of the patches, since it will be less code added. -- Pino Toscano
2016-08-25 16:12 GMT+03:00 Pino Toscano <ptoscano@redhat.com>:> On Thursday, 25 August 2016 16:05:47 CEST NoxDaFox wrote: > > 2016-08-25 14:09 GMT+03:00 Pino Toscano <ptoscano@redhat.com>: > > > > > On Wednesday, 24 August 2016 23:59:53 CEST Matteo Cafasso wrote: > > > > The find_inode API allows the User to search all the entries > referring > > > > to a given inode and returns a tsk_dirent structure for each of them. > > > > > > > > As I didn't want to change unrelated code, there is a little bit > > > > of code duplication at the moment. Plan is to refactor the logic > > > > in a dedicated set of patches. > > > > > > The general idea looks ok, but I'd rather see the duplication dealt > > > with sooner than later. > > > > > In the previous submissions, non related changes were rejected therefore > I > > thought that was the custom. > > I don't see how the two cases are the same: what was "rejected" was a > single patch contaning both additions documented in the commit message, > and unrelated changes such as formatting fixes. I remember to have said > that it's a no-go as *single* patch, but they can be sent (and > integrated) as different commits. >Then I totally misunderstood.> > In this case, refactoring and de-duplication of code should be done in > different commits as well. >> > Moreover I'll add another API find_block (block_number --> tsk_dirents > > referring to it) and I think is easier to refactor the code once all the > > use cases are in place as the picture gets more clear. > > More reasons to refactor *before*: since you already planned more APIs, > it's easier to just refactor one in advance with all the common code > needed, and use it later. Also, it eases a lot the review of the > patches, since it will be less code added. >Seems reasonable, I'll proceed and re-submit then.> > -- > Pino Toscano > _______________________________________________ > Libguestfs mailing list > Libguestfs@redhat.com > https://www.redhat.com/mailman/listinfo/libguestfs >