On Wed, Sep 6, 2017 at 4:08 AM, Duy Nguyen <pclouds@xxxxxxxxx> wrote: > On Thu, Aug 24, 2017 at 2:14 AM, Stefan Beller <sbeller@xxxxxxxxxx> wrote: >> On Wed, Aug 23, 2017 at 5:36 AM, Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> wrote: >>> The "submodule" argument in this function is a path, which can have >>> either '/' or '\\' as a separator. Use is_dir_sep() to support both. >>> >>> Noticed-by: Johannes Sixt <j6t@xxxxxxxx> >>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> >> >> Immediate questions that come to mind: >> * Could this go in as an independent bug fix? > > It probably could. But it may depend on other submodule changes in this series. > >> -> If so do we have any test that fails or stops failing on Windows? > > It was spotted during the review [1] so I guess no test fails on > Windows (not that I would know because I can't run tests on Windows) > >> -> If not, do we need one? > > Since I don't use Windows, I don't particularly care. And I can't add > one anyway because I can't run it. > > [1] https://public-inbox.org/git/%3Ca74cf309-fb16-2f45-8189-d1d0c655dea4@xxxxxxxx%3E/ IIRC I asked these questions as I was genuinely confused, I do not mind too much either. > >> * Assuming this is not an independent bug fix: >> Why do we need this patch in this series here? >> (I thought this is about worktrees, not submodules. >> So does this fix a potential bug that will be exposed >> later in this series, but was harmless up to now?) > > The series could probably be split in two. The first part is about > using the new refs_* API in revision.c. This helps clean up refs API a > bit, all *_submodule() functions will be one. Not strictly needed to > be part of this, it's just a continuation of my previous series that > introduces *_refs. Since submodule code is touched, this is found. > > The second part is actually fixing the prune bug. > > Should I split the series in two? I think I separated two parts in the > past (maybe I misremember) at least in the description... I had to reread the coverletter https://public-inbox.org/git/20170823123704.16518-1-pclouds@xxxxxxxxx/ to get that part. I would not be opposed to splitting the series, but I'll review an unsplit series as well. Thanks, Stefan > -- > Duy
