| Message ID | 340c0f46f74acd641945fceba5ab5feac011ae60.1660718028.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fixups for cw/submodule-merge-messages | expand |
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Elijah Newren <newren@gmail.com> > > Commit 4057523a40 ("submodule merge: update conflict error message", > 2022-08-04) added a sub_flag variable that is used to store a value from > enum conflict_and_info_types, but initializes it with an invalid value > of -1. The code may never set it to a valid value, and use the invalid > one. This can be surprising when reading over the code at first, but it > was intentional. Add a comment making it clear that it is okay to be > using an invalid value, due to how it is used later. The current code uses -1 as the "suggest the default course of action", so -1 is very much a "valid value" from the viewpoint of the code that suggests how to resolve. It indeed is an invalid value from the viewpoint of those who maintain conflict_and_info_types enum. > Signed-off-by: Elijah Newren <newren@gmail.com> > --- > merge-ort.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/merge-ort.c b/merge-ort.c > index 67159fc6ef9..0a935a8135f 100644 > --- a/merge-ort.c > +++ b/merge-ort.c > @@ -1886,7 +1886,7 @@ cleanup: > const char *abbrev; > > util = xmalloc(sizeof(*util)); > - util->flag = sub_flag; > + util->flag = sub_flag; /* May still be -1 */ > util->abbrev = NULL; > if (!sub_not_initialized) { > abbrev = repo_find_unique_abbrev(&subrepo, b, DEFAULT_ABBREV); This new comment may be a slight improvement, but a valid value of sub_flag is used only to signal the situation where the code does not know what to suggest, which feels backwards for longer-term code evolution. Presumably, we would use the util->flag field to store which of the known cases we know what to suggest as we know better. I wonder if we should initialize the variable to the most generic CONFLICT_SUBMODULE_FAILED_TO_MERGE instead of -1. The value would mean "use the default suggestion", and the two known unworkable values (not-initialized and history-not-available) are currently handled according to what these two values mean. We may later add more specialization based on other CONFLICT_SUBMODULE_* values. Thanks.
On Wed, Aug 17, 2022 at 2:45 PM Junio C Hamano <gitster@pobox.com> wrote: > > "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Elijah Newren <newren@gmail.com> > > > > Commit 4057523a40 ("submodule merge: update conflict error message", > > 2022-08-04) added a sub_flag variable that is used to store a value from > > enum conflict_and_info_types, but initializes it with an invalid value > > of -1. The code may never set it to a valid value, and use the invalid > > one. This can be surprising when reading over the code at first, but it > > was intentional. Add a comment making it clear that it is okay to be > > using an invalid value, due to how it is used later. > > The current code uses -1 as the "suggest the default course of > action", so -1 is very much a "valid value" from the viewpoint of > the code that suggests how to resolve. It indeed is an invalid > value from the viewpoint of those who maintain conflict_and_info_types > enum. > > > Signed-off-by: Elijah Newren <newren@gmail.com> > > --- > > merge-ort.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/merge-ort.c b/merge-ort.c > > index 67159fc6ef9..0a935a8135f 100644 > > --- a/merge-ort.c > > +++ b/merge-ort.c > > @@ -1886,7 +1886,7 @@ cleanup: > > const char *abbrev; > > > > util = xmalloc(sizeof(*util)); > > - util->flag = sub_flag; > > + util->flag = sub_flag; /* May still be -1 */ > > util->abbrev = NULL; > > if (!sub_not_initialized) { > > abbrev = repo_find_unique_abbrev(&subrepo, b, DEFAULT_ABBREV); > > This new comment may be a slight improvement, but a valid value of > sub_flag is used only to signal the situation where the code does > not know what to suggest, which feels backwards for longer-term code > evolution. Presumably, we would use the util->flag field to store > which of the known cases we know what to suggest as we know better. > > I wonder if we should initialize the variable to the most generic > CONFLICT_SUBMODULE_FAILED_TO_MERGE instead of -1. The value would > mean "use the default suggestion", and the two known unworkable > values (not-initialized and history-not-available) are currently > handled according to what these two values mean. We may later add > more specialization based on other CONFLICT_SUBMODULE_* values. I like that; I'll make the switch.
diff --git a/merge-ort.c b/merge-ort.c index 67159fc6ef9..0a935a8135f 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -1886,7 +1886,7 @@ cleanup: const char *abbrev; util = xmalloc(sizeof(*util)); - util->flag = sub_flag; + util->flag = sub_flag; /* May still be -1 */ util->abbrev = NULL; if (!sub_not_initialized) { abbrev = repo_find_unique_abbrev(&subrepo, b, DEFAULT_ABBREV);