[v2,0/3] {apply,alias}: convert pre-processor macros to enums

Message

Vinayak Dev Feb. 10, 2023, 5:13 p.m. UTC

Revert changes to alias.c, and change variable types in apply.c

It is needed to revert changes to alias.c as the changed #define macros were actually returned values,
so it is not right to change their types to enums.

Also, SPLIT_CMDLINE_BAD_ENDING is returned after changing to negative sign,
breaking the validity of the conversion to enum. 

The changes to apply.c are actually useful only if the variable types are enum,
so change the type of variable int patch_method and function argument int side to enums as well.

Changes are in accordance to Eric's review of v1:
https://lore.kernel.org/git/CADE8NappDSaZrMLeqak4is59oL=[e-mail]/T/

Vinayak Dev (2):
  Revert added enum to #define
  Change types of int patch_method and int side to enum

Vinayak Dev (1):
  Change #define to enum in apply.c and alias.c

 alias.c |  1 +
 apply.c | 17 +++++++++++------
 2 files changed, 12 insertions(+), 6 deletions(-)

Range-diff against v1:
1:  f64b1af2a5 = 1:  eaaa09dfb1 Change #define to enum in apply.c and alias.c
-:  ---------- > 2:  86e27e7f64 Revert added enum to #define
-:  ---------- > 3:  17233eb3fe Change types of int patch_method and int side to enum

Comments

Junio C Hamano Feb. 10, 2023, 10:01 p.m. UTC | #1

Vinayak Dev <[e-mail]> writes:

> Revert changes to alias.c, and change variable types in apply.c

When you send out a rerolled series, you do not have to show your
past mistakes.  This [v2] is structured as a three patch series
whose first one makes a similar mess as what [v1] did, the second
one and the third one then revert some parts of that earlier mess.

That is not what you want to show your reviewers, and more
importantly, that is not what we want to record in our history.
Rerolling a series is your chance to pretend that you are much
better programmer than you who wrote the [v1] patch.  The review
exchange is to help you do that.  Please take advantage of that.

You may find "git rebase -i" is a useful tool to help you pretend
that you got to the ideal end result without these "I tried this
first, which was wrong in these points, which I correct in a
subsequent step" steps.

Thanks.

Vinayak Dev Feb. 11, 2023, 4 a.m. UTC | #2

On Sat, 11 Feb 2023 at 03:31, Junio C Hamano <[e-mail]> wrote:
>
> Vinayak Dev <[e-mail]> writes:
>
> > Revert changes to alias.c, and change variable types in apply.c
>
> When you send out a rerolled series, you do not have to show your
> past mistakes.  This [v2] is structured as a three patch series
> whose first one makes a similar mess as what [v1] did, the second
> one and the third one then revert some parts of that earlier mess.
>
> That is not what you want to show your reviewers, and more
> importantly, that is not what we want to record in our history.
> Rerolling a series is your chance to pretend that you are much
> better programmer than you who wrote the [v1] patch.  The review
> exchange is to help you do that.  Please take advantage of that.
>
> You may find "git rebase -i" is a useful tool to help you pretend
> that you got to the ideal end result without these "I tried this
> first, which was wrong in these points, which I correct in a
> subsequent step" steps.
>
> Thanks.

OK, I will keep that in mind before sending subsequent patches.
Should I re-send [v2] after making corrections for this mistake?
That would make the corrections more obvious and the mistakes less.

Thanks a lot!
Vinayak

Eric Sunshine Feb. 15, 2023, 12:04 a.m. UTC | #3

On Fri, Feb 10, 2023 at 11:00 PM Vinayak Dev <[e-mail]> wrote:
> On Sat, 11 Feb 2023 at 03:31, Junio C Hamano <[e-mail]> wrote:
> > Vinayak Dev <[e-mail]> writes:
> > > Revert changes to alias.c, and change variable types in apply.c
> >
> > When you send out a rerolled series, you do not have to show your
> > past mistakes.  This [v2] is structured as a three patch series
> > whose first one makes a similar mess as what [v1] did, the second
> > one and the third one then revert some parts of that earlier mess.
> >
> > That is not what you want to show your reviewers, and more
> > importantly, that is not what we want to record in our history.
> > Rerolling a series is your chance to pretend that you are much
> > better programmer than you who wrote the [v1] patch.  The review
> > exchange is to help you do that.  Please take advantage of that.
> >
> > You may find "git rebase -i" is a useful tool to help you pretend
> > that you got to the ideal end result without these "I tried this
> > first, which was wrong in these points, which I correct in a
> > subsequent step" steps.
>
> OK, I will keep that in mind before sending subsequent patches.
> Should I re-send [v2] after making corrections for this mistake?
> That would make the corrections more obvious and the mistakes less.

You should send a v3 which completely replaces v1 and v2.

To prepare v3, use the "squash" (or "fixup") command of `git rebase
-i` to squash all three patches from v2 into a single patch, so that
v3 consists of just one patch. The squashed patch should contain only
changes to "apply.c"; specifically, changing #define to `enum`, and
changing the variable declarations from `int` to `enum`.

You can also update the commit message of the squashed patch so that
it explains the reason for the patch: specifically, the debugger will
display the values symbolically rather than as mere numbers.

Finally, proofread the commit message and the patch itself, and resubmit as v3.

Vinayak Dev Feb. 15, 2023, 8:19 a.m. UTC | #4

On Wed, 15 Feb 2023 at 05:34, Eric Sunshine <[e-mail]> wrote:
>
> On Fri, Feb 10, 2023 at 11:00 PM Vinayak Dev <[e-mail]> wrote:

> > Should I re-send [v2] after making corrections for this mistake?
> > That would make the corrections more obvious and the mistakes less.
>
> You should send a v3 which completely replaces v1 and v2.
>
> To prepare v3, use the "squash" (or "fixup") command of `git rebase
> -i` to squash all three patches from v2 into a single patch, so that
> v3 consists of just one patch. The squashed patch should contain only
> changes to "apply.c"; specifically, changing #define to `enum`, and
> changing the variable declarations from `int` to `enum`.
>
> You can also update the commit message of the squashed patch so that
> it explains the reason for the patch: specifically, the debugger will
> display the values symbolically rather than as mere numbers.
>
> Finally, proofread the commit message and the patch itself, and resubmit as v3.

Ok! I have absolutely understood your points. I will roll out v3 with
appropriate
changes, as you suggest.

Thanks a lot!
 Vinayak