Re: [PATCH v7 00/20] submodule: convert the rest of 'update' to C

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:

> On Thu, Feb 17 2022, Glen Choo wrote:
>
>> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
>>
>>> On Thu, Feb 10 2022, Glen Choo wrote:
>>>
>>>> Atharva Raykar (6):
>>>>   submodule--helper: get remote names from any repository
>>>>   submodule--helper: refactor get_submodule_displaypath()
>>>>   submodule--helper: allow setting superprefix for init_submodule()
>>>>   submodule--helper: run update using child process struct
>>>>   builtin/submodule--helper.c: reformat designated initializers
>>>>   submodule: move core cmd_update() logic to C
>>>>
>>>> Glen Choo (11):
>>>>   submodule--helper: remove update-module-mode
>>>>   submodule--helper: reorganize code for sh to C conversion
>>>>   submodule--helper run-update-procedure: remove --suboid
>>>>   submodule--helper run-update-procedure: learn --remote
>>>>   submodule--helper: remove ensure-core-worktree
>>>>   submodule--helper update-clone: learn --init
>>>>   submodule--helper: move functions around
>>>>   submodule--helper: reduce logic in run_update_procedure()
>>>>   fixup! submodule--helper run-update-procedure: remove --suboid
>>>>   fixup! submodule--helper run-update-procedure: learn --remote
>>>>   fixup! submodule: move core cmd_update() logic to C
>>>>
>>>> Ævar Arnfjörð Bjarmason (3):
>>>>   builtin/submodule--helper.c: rename option variables to "opt"
>>>>   submodule--helper: don't use bitfield indirection for parse_options()
>>>>   submodule tests: test for init and update failure output
>>>
>>> I think sending a version of this with the fixups squashed in as a v8
>>> would be good, and perhaps addressing some of my comments.
>>>
>>> I don't know if my suggested split-up of "prep fixes" into another
>>> series would be a good thing to pursue overall, perhaps Junio will chime
>>> in on how he'd be most comfortable in merging this down. I'd think
>>> splitting such trivial fixes into their own series be easier to review,
>>> but perhaps not.
>>
>> Combing through the patches again, I couldn't really convince myself
>> that the patch 4..9 prep fixes make sense as obvious standalone fixes,
>> except maybe:
>>
>> - patch 4 submodule--helper: run update using child process struct
>> - patch 8 submodule tests: test for init and update failure output
>> - patch 9:  087bf43aba submodule--helper: remove update-module-mode
>>
>> But, since the 'final' patch (ignoring the fixup!-s) is consuming a huge
>> chunk of the work anyway, here's an alternative patch organization with
>> the fixup!-s squashed:
>>
>> = Move 'easy' and 'obviously correct' code from sh->C
>> - patches 8-9   Cleanup and introduce tests
>> - patches 1-4   Refactor existing functions, which enables..
>> - patches 10-14 Move 'obviously correct' pieces of logic from sh-> C
>>
>> = Finalize move from sh->C
>> i.e. combine "run-update-procedure" and "update-clone" into "update"
>> - patches 5,7     Cleanup and prep
>> - patches 6,15-16 Shrinking the diff
>> - patch 17        Implement "git submodule--helper update" 
>>
>> I'll send this if there are no objections :)
>
> Yes that sounds good, or rather, I haven't re-looked at that in detail,
> but I think if you think it makes sense we should go for it.
>
> Or rather, we should really be aiming to produce a patch series that
> makes sense in its current iteration, as opposed to optimizing for a
> diff against some ad-hoc re-roll I produced a few versions ago :)

Agreed, makes sense.

> Thanks again for working on this & picking this up. It's great to see
> progress in this area!

Thanks to you too for getting the ball rolling and lending me your
thoughts :)




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux