Page MenuHomePhabricator

provide details about text patches
Open, Showstopper IssuesPublic

Description

Hi,

There are a lot of text-related patches up for review:

https://phab.enlightenment.org/D10542
https://phab.enlightenment.org/D10607
https://phab.enlightenment.org/D10646
https://phab.enlightenment.org/D10711
https://phab.enlightenment.org/D10715
https://phab.enlightenment.org/D10716
https://phab.enlightenment.org/D10729

None of these patches are in a stack (sidebar "Edit Related Revisions" -> parent/child revisions), none of these patches have any indication as to the order in which they should be applied, the patches do not apply in totality, and there are no repos linked to which provide the patch series in full.

This is a problem, and it is impossible for us to seriously consider patches for review (or subsequent merging) if they cannot even be applied.

The following steps should be taken to resolve this problem:

  1. Rebase all patches to successfully apply
  2. Create github repo or similar (provide link to this) where applied patches can be cloned for ease of testing
  3. Link to this repo in the description of each patch to ensure that reviewers know where to access the code
  4. Rebase this repo regularly to ensure that it is in sync with current EFL master as well as the submitted state of the patches in phabricator
  5. Create dependency chain of submitted patches in phabricator
zmike created this task.Mon, Nov 25, 5:33 AM
zmike triaged this task as Showstopper Issues priority.
zmike added a subscriber: a.srour.
ali.alzyod added a comment.EditedMon, Nov 25, 7:21 AM

@zmike
We try the most efficient way to develop and review text changes that we can think of.
Making separate patches will simplify review process instead of the big change in repository contains all changes at once. (review process will be take long time)

Patches Map: <Base> -> <Child>
D10542 -> D10646 -> D10711
D10607 -> (D10715, ,D10729)
D10716

1- Rebase all patches to successfully apply

Yes you should be able to apply them, just follow the map (apply base then childs), If you have any issue we will fix it ASAP

2- Create github repo or similar (provide link to this) where applied patches can be cloned for ease of testing

Sure, I will create one soon (I will just apply them all at once), but again no more patches for this release.

3- Link to this repo in the description of each patch to ensure that reviewers know where to access the code

We do not have any plans for more patches, these ones cover them all.

4- Rebase this repo regularly to ensure that it is in sync with current EFL master as well as the submitted state of the patches in phabricator

Hmm, I am ok with that, but this will take more time, since we already have all patches in phab.

5- Create dependency chain of submitted patches in phabricator

Patches Map: <Base> -> <Child>
D10542 -> D10646 -> D10711
D10607 -> (D10715, ,D10729)
D10716

zmike added a comment.Mon, Nov 25, 9:02 AM

Thanks for getting back to me on this.

@zmike
We try the most efficient way to develop and review text changes that we can think of.
Making separate patches will simplify review process instead of the big change in repository contains all changes at once. (review process will be take long time)

Yes, separate patches is good. Please use the features available in our patch submission system (either using phabricator web ui or git-phab) to ensure that all patches have dependency info in the patch review interface to avoid continued confusion.

Patches Map: <Base> -> <Child>
D10542 -> D10646 -> D10711
D10607 -> (D10715, ,D10729)
D10716

Please add this dependency using the specified phabricator functionality.

1- Rebase all patches to successfully apply

Yes you should be able to apply them, just follow the map (apply base then childs), If you have any issue we will fix it ASAP

2- Create github repo or similar (provide link to this) where applied patches can be cloned for ease of testing

Sure, I will create one soon (I will just apply them all at once), but again no more patches for this release.

3- Link to this repo in the description of each patch to ensure that reviewers know where to access the code

We do not have any plans for more patches, these ones cover them all.

4- Rebase this repo regularly to ensure that it is in sync with current EFL master as well as the submitted state of the patches in phabricator

Hmm, I am ok with that, but this will take more time, since we already have all patches in phab.

Ideally patches should be submitted in a stack (or multiple stacks) out of branches using git-phab, and this should be no additional effort.

If the patches do not apply to current master when I or anyone else attempts to review, they will be rejected.

5- Create dependency chain of submitted patches in phabricator

Patches Map: <Base> -> <Child>
D10542 -> D10646 -> D10711

D10542 is listed erroneously since this was already landed; the other two patches do apply.

D10607 -> (D10715, ,D10729)

This series does not apply on current master.

D10716

This patch applies.