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

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

[Vinayak Dev]

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=X1wJOj2eCLLjaCKyrnoK9PQ@mail.gmail.com/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
-- 
2.39.1

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

[Vinayak Dev]

From: vinayakdsci <vinayakdev.sci@gmail.com>

Changes made to alias.c and apply.c in v1, requiring refactoring.

Signed-off-by: Vinayak Dev <vinayakdev.sci@gmail.com>

---
 alias.c | 12 +++++++++---
 apply.c | 19 +++++++++++++++----
 2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/alias.c b/alias.c
index 00abde0817..61ef2c0c54 100644
--- a/alias.c
+++ b/alias.c
@@ -44,9 +44,15 @@ void list_aliases(struct string_list *list)
 	read_early_config(config_alias_cb, &data);
 }
 
-#define SPLIT_CMDLINE_BAD_ENDING 1
-#define SPLIT_CMDLINE_UNCLOSED_QUOTE 2
-#define SPLIT_CMDLINE_ARGC_OVERFLOW 3
+/* #define SPLIT_CMDLINE_BAD_ENDING 1 */
+/* #define SPLIT_CMDLINE_UNCLOSED_QUOTE 2 */
+/* #define SPLIT_CMDLINE_ARGC_OVERFLOW 3 */
+enum split_cmdline_error {
+	SPLIT_CMDLINE_BAD_ENDING = 1,
+	SPLIT_CMDLINE_UNCLOSED_QUOTE,
+	SPLIT_CMDLINE_ARGC_OVERFLOW
+};
+
 static const char *split_cmdline_errors[] = {
 	N_("cmdline ends with \\"),
 	N_("unclosed quote"),
diff --git a/apply.c b/apply.c
index 5eec433583..1e9cf2f4f2 100644
--- a/apply.c
+++ b/apply.c
@@ -205,8 +205,13 @@ struct fragment {
  * or deflated "literal".
  */
 #define binary_patch_method leading
-#define BINARY_DELTA_DEFLATED	1
-#define BINARY_LITERAL_DEFLATED 2
+/* #define BINARY_DELTA_DEFLATED   1 */
+/* #define BINARY_LITERAL_DEFLATED 2 */
+
+enum binary_type_deflated {
+	BINARY_DELTA_DEFLATED = 1,
+	BINARY_LITERAL_DEFLATED
+};
 
 static void free_fragment_list(struct fragment *list)
 {
@@ -918,8 +923,14 @@ static int gitdiff_hdrend(struct gitdiff_data *state UNUSED,
  * their names against any previous information, just
  * to make sure..
  */
-#define DIFF_OLD_NAME 0
-#define DIFF_NEW_NAME 1
+
+/* #define DIFF_OLD_NAME 0 */
+/* #define DIFF_NEW_NAME 1 */
+
+enum diff_name {
+	DIFF_OLD_NAME = 0,
+	DIFF_NEW_NAME
+};
 
 static int gitdiff_verify_name(struct gitdiff_data *state,
 			       const char *line,
-- 
2.39.1

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

[Vinayak Dev]

Revert the changes made to alias.c in v1.

Signed-off-by: Vinayak Dev <vinayakdev.sci@gmail.com>

---
 alias.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/alias.c b/alias.c
index 61ef2c0c54..deb871d59c 100644
--- a/alias.c
+++ b/alias.c
@@ -44,14 +44,9 @@ void list_aliases(struct string_list *list)
 	read_early_config(config_alias_cb, &data);
 }
 
-/* #define SPLIT_CMDLINE_BAD_ENDING 1 */
-/* #define SPLIT_CMDLINE_UNCLOSED_QUOTE 2 */
-/* #define SPLIT_CMDLINE_ARGC_OVERFLOW 3 */
-enum split_cmdline_error {
-	SPLIT_CMDLINE_BAD_ENDING = 1,
-	SPLIT_CMDLINE_UNCLOSED_QUOTE,
-	SPLIT_CMDLINE_ARGC_OVERFLOW
-};
+#define SPLIT_CMDLINE_BAD_ENDING 1
+#define SPLIT_CMDLINE_UNCLOSED_QUOTE 2
+#define SPLIT_CMDLINE_ARGC_OVERFLOW 3
 
 static const char *split_cmdline_errors[] = {
 	N_("cmdline ends with \\"),
-- 
2.39.1

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

[Vinayak Dev]

Change variables int side and int patch_method to enum in apply.c.
This allows for proper listing of enum constants during debugging.

Signed-off-by: Vinayak Dev <vinayakdev.sci@gmail.com>

---
 apply.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/apply.c b/apply.c
index 1e9cf2f4f2..8612bd44c8 100644
--- a/apply.c
+++ b/apply.c
@@ -205,9 +205,6 @@ struct fragment {
  * or deflated "literal".
  */
 #define binary_patch_method leading
-/* #define BINARY_DELTA_DEFLATED   1 */
-/* #define BINARY_LITERAL_DEFLATED 2 */
-
 enum binary_type_deflated {
 	BINARY_DELTA_DEFLATED = 1,
 	BINARY_LITERAL_DEFLATED
@@ -924,9 +921,6 @@ static int gitdiff_hdrend(struct gitdiff_data *state UNUSED,
  * to make sure..
  */
 
-/* #define DIFF_OLD_NAME 0 */
-/* #define DIFF_NEW_NAME 1 */
-
 enum diff_name {
 	DIFF_OLD_NAME = 0,
 	DIFF_NEW_NAME
@@ -936,7 +930,7 @@ static int gitdiff_verify_name(struct gitdiff_data *state,
 			       const char *line,
 			       int isnull,
 			       char **name,
-			       int side)
+			       enum diff_name side)
 {
 	if (!*name && !isnull) {
 		*name = find_name(state->root, line, NULL, state->p_value, TERM_TAB);
@@ -1921,7 +1915,7 @@ static struct fragment *parse_binary_hunk(struct apply_state *state,
 	int llen, used;
 	unsigned long size = *sz_p;
 	char *buffer = *buf_p;
-	int patch_method;
+	enum binary_type_deflated patch_method;
 	unsigned long origlen;
 	char *data = NULL;
 	int hunk_size = 0;
-- 
2.39.1

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

[Junio C Hamano]

Vinayak Dev <vinayakdev.sci@gmail.com> 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.

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

[Vinayak Dev]

On Sat, 11 Feb 2023 at 03:31, Junio C Hamano <gitster@pobox.com> wrote:
>
> Vinayak Dev <vinayakdev.sci@gmail.com> 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

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

[Eric Sunshine]

On Fri, Feb 10, 2023 at 11:00 PM Vinayak Dev <vinayakdev.sci@gmail.com> wrote:
> On Sat, 11 Feb 2023 at 03:31, Junio C Hamano <gitster@pobox.com> wrote:
> > Vinayak Dev <vinayakdev.sci@gmail.com> 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.

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

[Vinayak Dev]

On Wed, 15 Feb 2023 at 05:34, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Fri, Feb 10, 2023 at 11:00 PM Vinayak Dev <vinayakdev.sci@gmail.com> 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