Somewhere along the line the commit message became so abstract it doesn't
really say anything, and mention of moving the definitions around was dropped
(such things should always be explicitly stated) but I'm just too sick and
tired of this thing to demand another round.
--
You are receiving t
Merged #649 into master.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/649#event-2352619698___
Rpm-maint mailing list
Rpm-maint
Rebase + fixes are done.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/649#issuecomment-493749504___
Rpm-maint mailing list
Rpm
pmatilai commented on this pull request.
> @@ -462,6 +430,22 @@ int readLine(rpmSpec spec, int strip)
lineType = parseLineType(s);
if (!lineType)
goto after_classification;
+
+/* check ordering of the conditional */
+if (lineType->isConditional &&
+ (spec->readSta
pmatilai commented on this pull request.
> @@ -462,6 +430,22 @@ int readLine(rpmSpec spec, int strip)
lineType = parseLineType(s);
if (!lineType)
goto after_classification;
+
+/* check ordering of the conditional */
+if (lineType->isConditional &&
+ (spec->readSta
pmatilai commented on this pull request.
> @@ -462,6 +430,22 @@ int readLine(rpmSpec spec, int strip)
lineType = parseLineType(s);
if (!lineType)
goto after_classification;
+
+/* check ordering of the conditional */
+if (lineType->isConditional &&
+ (spec->readSta
Please rebase and fix the two cosmetics mentioned above, after that I think
we're good to go finally.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/649#issuecomment-49340
pmatilai commented on this pull request.
> @@ -462,6 +430,22 @@ int readLine(rpmSpec spec, int strip)
lineType = parseLineType(s);
if (!lineType)
goto after_classification;
+
+/* check ordering of the conditional */
+if (lineType->isConditional &&
+ (spec->readSta
pmatilai commented on this pull request.
> @@ -462,6 +430,22 @@ int readLine(rpmSpec spec, int strip)
lineType = parseLineType(s);
if (!lineType)
goto after_classification;
+
+/* check ordering of the conditional */
+if (lineType->isConditional &&
+ (spec->readSta
Commit message is corrected.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/649#issuecomment-492949023___
Rpm-maint mailing list
Typo in the first sentence of the commit message: "odrering"
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/649#issuecomment-492232761__
pavlinamv commented on this pull request.
> +size_t textLen;
+const char *text;
+int withArgs;
+int isConditional;
+int wrongPrecursors;
+const char *info_text;
+} * parsedSpecLine;
+
+static struct parsedSpecLine_s const lineTypes[] = {
+{ LINE_ENDIF, LEN_AND_ST
pavlinamv commented on this pull request.
> @@ -462,6 +430,16 @@ int readLine(rpmSpec spec, int strip)
lineType = parseLineType(s);
if (!lineType)
goto after_classification;
+
+/* for a conditional check its ordering */
+if (lineType->isConditional &&
+ (spec->rea
pmatilai requested changes on this pull request.
Hmm, there seems to be some GH review-tool wonkiness here, I made several
comments on the latest round and haven't touched this because those issues
haven't been addressed. But now I noticed that those comments appear as
"pending" so I guess you
Your comments are incorporated in the new version.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/649#issuecomment-486536987___
Maybe I didn't say anything about LINE_DEFAULT specifically, just generally
about the 0 enum. The hole and the comment seem just strange now, I'd rather
see that LINE_DEFAULT 0 in the enum even if it's not used at all in the code as
that's along the general style. Nothing forces you to put that
> Hmm, why not? Having that LINE_DEFAULT kind of thing to represent all the
> normal lines seemed useful. Perhaps you misunderstood what I meant by the
> earlier comments?
Please in which comment you spoke about LINE_DEFAULT?
The implemented algorithm (added in #625) works differently and adding
Oh and BTW, I know its easy for commit messages and reality to get out of sync
when doing multiple versions. Try to review them yourself too: read it as a
story, and ask yourself does that story make sense without looking at the
actual code.
--
You are receiving this because you are subscribed
Anyway, overall looks quite nice. Bonus points if you can preserve the more
accurate error message information, it seems like one could figure the
after/without type info from the bitfield order maybe.
The biggest problem here is the commit messages :) The first commit mumbles
something about r
pmatilai commented on this pull request.
> @@ -268,6 +268,7 @@ rpmSpec newSpec(void)
spec->readStack = xcalloc(1, sizeof(*spec->readStack));
spec->readStack->next = NULL;
spec->readStack->reading = 1;
+spec->readStack->lastConditional = LINE_ENDIF;
This seems like a bit of a
pmatilai commented on this pull request.
> @@ -49,6 +50,18 @@ struct Source * next;
typedef struct Package_s * Package;
+/* Do not use number 0 for a line type */
Hmm, why not? Having that LINE_DEFAULT kind of thing to represent all the
normal lines seemed useful. Perhaps you misunderstoo
All your comments are incorporated in the new version.
First two commits are now in a single commit.The second now add a general type
of an error for inappropriate ordering of conditionals + change of
implementedTypes name.
--
You are receiving this because you are subscribed to this thread.
Re
Hmm, just realized I misplaced this comment so the context is missing:
>No problem with the change itself, but the commit message doesn't really
>convey what's going on.
On first sight the diff looks like excessive white-space changes or something,
you need to look quite closely to see that the
Well, the point is to have a specific name. lineTypes seems fairly obvious
candidate, branchTypes would be less bad, which is not to say it's a good name.
Good names are often harder than the code itself.
--
You are receiving this because you are subscribed to this thread.
Reply to this email
Please, do you have any idea what name can be used instead of implementedTypes?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/649#issuecomment-481998368___
> Another issue is that the %if-%else-%endif sanity checking now looks even
> more ad-hoc because some things look at spec->redStack->next and others at
> spec->readStack->ifStage.
BTW what I meant by that is there might be an opportunity here to unify all
those %if-related checks to use ifStag
pmatilai commented on this pull request.
> @@ -480,6 +480,13 @@ int readLine(rpmSpec spec, int strip)
ofi->fileName, ofi->lineNum);
return PART_ERROR;
}
+ if ((spec->readStack->ifStage == LINE_ELSE)) {
+ /* Got an else after %else ! */
pmatilai requested changes on this pull request.
No problem with the change itself, but the commit message doesn't really convey
what's going on.
On first sight the diff looks like excessive white-space changes or something,
you need to look quite closely to see that the whole thing is renumbere
You can view, comment on, or merge this pull request online at:
https://github.com/rpm-software-management/rpm/pull/649
-- Commit Summary --
* Enable rpmParseLineType_e to store default value
* Enable to use rpmParseLineType_e in spec.c
* Warn if %else is after %else
-- File Changes --
29 matches
Mail list logo
