threads / patch / 59226

v2, 3 parts{apply,alias}: convert pre-processor macros to enums

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

## tl;dr

8 messages between Feb 10, 2023 and Feb 15, 2023. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Vinayak Dev· Feb 10, 2023, 17:13 UTC · lore
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
Vinayak Dev· Feb 10, 2023, 17:13 UTC · re: Vinayak Dev · lore

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

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(-)
Show changes to 2 files +24 −7

alias.c, apply.c

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
Vinayak Dev· Feb 10, 2023, 17:13 UTC · re: Vinayak Dev · lore

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

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(-)
Show changes to alias.c +3 −8
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
Vinayak Dev· Feb 10, 2023, 17:13 UTC · re: Vinayak Dev · lore

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

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(-)
Show changes to apply.c +2 −8
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
Junio C Hamano· Feb 10, 2023, 22:01 UTC · re: Vinayak Dev · lore

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

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.
Vinayak Dev· Feb 11, 2023, 04:00 UTC · re: Junio C Hamano · lore

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

On Sat, 11 Feb 2023 at 03:31, Junio C Hamano <gitster@pobox.com> wrote:
Show 22 quoted lines
>
> 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

Eric Sunshine· Feb 15, 2023, 00:04 UTC · re: Vinayak Dev · lore

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

On Fri, Feb 10, 2023 at 11:00 PM Vinayak Dev <vinayakdev.sci@gmail.com> wrote:
Show 23 quoted lines
> 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.
Vinayak Dev· Feb 15, 2023, 08:19 UTC · re: Eric Sunshine · lore

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

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:
Show 16 quoted lines
> > 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

← back to recent threads