[trivial PATCH 12/22] [otp_mppe.c] eliminate build warnings
Signed-off-by: Wang Tinggong <wangtinggong@gmail.com> --- src/modules/rlm_otp/otp_mppe.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/modules/rlm_otp/otp_mppe.c b/src/modules/rlm_otp/otp_mppe.c index f31ef9f..c5a12c8 100644 --- a/src/modules/rlm_otp/otp_mppe.c +++ b/src/modules/rlm_otp/otp_mppe.c @@ -351,7 +351,7 @@ otp_mppe(REQUEST *request, otp_pwe_t pwe, const otp_option_t *opt, (2 * sizeof(mppe_key)) + 1]; #else /* 0 */ /* 0x ( ASCII(mppe_key) ) \0 */ - unsigned char mppe_key_string[2 + (2 * sizeof(MasterKey)) + 1]; + char mppe_key_string[2 + (2 * sizeof(MasterKey)) + 1]; #endif /* else !0 */ /* Generate the master session key. */ -- 1.6.0.6
That's incorrect. The type must be unsigned char. I didn't look at any other changes, maybe others have problems also. On February 2, 2010 1:28:45 AM +0800 Wang Tinggong <wangtinggong@gmail.com> wrote:
Signed-off-by: Wang Tinggong <wangtinggong@gmail.com> --- src/modules/rlm_otp/otp_mppe.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/modules/rlm_otp/otp_mppe.c b/src/modules/rlm_otp/otp_mppe.c index f31ef9f..c5a12c8 100644 --- a/src/modules/rlm_otp/otp_mppe.c +++ b/src/modules/rlm_otp/otp_mppe.c @@ -351,7 +351,7 @@ otp_mppe(REQUEST *request, otp_pwe_t pwe, const otp_option_t *opt, (2 * sizeof(mppe_key)) + 1]; #else /* 0 */ /* 0x ( ASCII(mppe_key) ) \0 */ - unsigned char mppe_key_string[2 + (2 * sizeof(MasterKey)) + 1]; + char mppe_key_string[2 + (2 * sizeof(MasterKey)) + 1]; #endif /* else !0 */
/* Generate the master session key. */
Doesn't it defeat the purpose of git vs cvs (having changesets) to have these patches as individual commits vs a single changeset???? Or is it a quirk of the way they are mailed that they look like separate changes. -frank
Frank Cusack wrote:
Doesn't it defeat the purpose of git vs cvs (having changesets) to have these patches as individual commits vs a single changeset????
Or is it a quirk of the way they are mailed that they look like separate changes.
If they're mailed as separate changes, they will be applied as separate changes. I'm inclined to have more small changes, because it makes any later merge *much* easier. Having a few large changes makes merges annoying. i.e. Ask me how many local "git" trees I have with changes that aren't pushed back. It's non-negligible. Alan DeKok.
On February 1, 2010 8:37:27 PM +0100 Alan DeKok <aland@deployingradius.com> wrote:
Frank Cusack wrote:
Doesn't it defeat the purpose of git vs cvs (having changesets) to have these patches as individual commits vs a single changeset????
Or is it a quirk of the way they are mailed that they look like separate changes.
If they're mailed as separate changes, they will be applied as separate changes.
I'm inclined to have more small changes, because it makes any later merge *much* easier. Having a few large changes makes merges annoying.
My experience is the opposite, at least for some values of "large". But it is always better to group a set of patches that comprise a single logical change into a single changeset. That makes change review sane, and that happens much more frequently than merging. Now of course if one patch in a changeset is bad, yes that does invalidate the entire changeset, but the fix for that is still easy: make the single correction in the branch, then do a single merge which includes both of the branch's changesets. Just for a trivial example, if you change the type of a struct member in a header file, the c files which reference that struct and depend on it's type being known must be included in the same changeset. -frank
On February 1, 2010 4:23:13 PM -0500 Frank Cusack <fcusack@fcusack.com> wrote:
On February 1, 2010 8:37:27 PM +0100 Alan DeKok <aland@deployingradius.com> wrote:
I'm inclined to have more small changes, because it makes any later merge *much* easier. Having a few large changes makes merges annoying.
My experience is the opposite, at least for some values of "large".
I have to amend that a bit. It's always bad to have a single change be "large", regardless if it is to a single file or a changeset that comprises multiple files. Not because it may be difficult to merge, but because it is difficult to review. So I don't disagree with you. But that's a different thing than combining multiple individual patches into a single change. Grouping 22 1-line patches into a single changeset is not large. OTOH submitting them as 22 individual patches belies a naive understanding of source control, and actually makes merging that much more difficult, or at least time consuming. -frank
Frank Cusack wrote:
Grouping 22 1-line patches into a single changeset is not large. OTOH submitting them as 22 individual patches belies a naive understanding of source control, and actually makes merging that much more difficult, or at least time consuming.
I also prefer changes to function prototypes to be in the same patch as changes to the functions... otherwise the build breaks. Alan DeKok.
--On Tuesday, February 02, 2010 12:01:04 AM +0100 Alan DeKok <aland@deployingradius.com> wrote:
Frank Cusack wrote:
Grouping 22 1-line patches into a single changeset is not large. OTOH submitting them as 22 individual patches belies a naive understanding of source control, and actually makes merging that much more difficult, or at least time consuming.
I also prefer changes to function prototypes to be in the same patch as changes to the functions... otherwise the build breaks.
If you'll allow me to borrow the soapbox for a moment... The guiding principal I've often seen and used is typically expressed as "one change per patch; one patch per change". That is, the fundamental property of interest is not size, but the number of logically separate changes which are part of the same patch. The correct number is always exactly one. Multiple related edits like changing a function's prototype, making the corresponding change to its definition, and updating all of the call sites are really a single change, and should be part of the same patch, and thus the same commit. The same goes for a group of edits spread across the tree that introduce a new feature or fix a bug. If they're part of one logical change, they should be part of one commit. A good rule to use here is to combine things that are syntactically or semantically interdependent, such that applying only part of the change results in a tree that doesn't build or that no longer works correctly. The flip side is that unrelated things should generally not be part of the same patch. Don't fix a bug and add a feature in the same patch. Or fix two bugs, or add two features. That goes triple for keeping cosmetic/stylistic changes such as a reindent separate from warning elimination, and both separate from actual functional changes. Putting unrelated changes in the same patch makes that patch harder to review, makes it harder to accept one change but not the other (which may result in the whole thing being rejected), and makes it harder to track down what happened if one of those changes introduced a bug. "git bisect" is a powerful tool for tracking down when a problem was introduced, but can't split the difference between two changes that were part of the same commit. There is, of course, some grey here. Some projects prefer to get things like reindent or warning elimination in larger chunks, arguing that such changes are related even if not interdependent, and are actually easier to merge and review in large batches. Alan has clearly indicated his preference to be the opposite, and around here, he gets to decide. Similarly, it may often be possible to take a large set of changes to add a complex new feature, and break it down into smaller parts, each building on the last. Such things are often easier to review and test, which is always a good thing. -- Jeffrey T. Hutzelman (N3NHS) <jhutz+@cmu.edu> Carnegie Mellon University - Pittsburgh, PA
Jeffrey Hutzelman wrote:
The guiding principal I've often seen and used is typically expressed as "one change per patch; one patch per change". That is, the fundamental property of interest is not size, but the number of logically separate changes which are part of the same patch. The correct number is always exactly one.
I agree. I haven't always done that myself, but it's a good idea.
There is, of course, some grey here. Some projects prefer to get things like reindent or warning elimination in larger chunks, arguing that such changes are related even if not interdependent, and are actually easier to merge and review in large batches. Alan has clearly indicated his preference to be the opposite, and around here, he gets to decide.
:) I've always wondered why git doesn't have advisory information for patches. Like "created by running 'indent options'". That way when a merge fails, it can try to run indent on the branch to be merged, and *then* merge the patch.
Similarly, it may often be possible to take a large set of changes to add a complex new feature, and break it down into smaller parts, each building on the last. Such things are often easier to review and test, which is always a good thing.
And should likely go on a development branch, too. (i.e. no fast-forward merge.) That way you have the best of both worlds: "diff master branch" to get the whole feature, and then a set of smaller patches which make merges easier. If it's OK with you, I'll take your email with minor edits && put it on git.freeradius.org as "how to do patches". :) Alan DeKok.
--On Tuesday, February 02, 2010 07:57:00 AM +0100 Alan DeKok <aland@deployingradius.com> wrote:
:) I've always wondered why git doesn't have advisory information for patches. Like "created by running 'indent options'". That way when a merge fails, it can try to run indent on the branch to be merged, and *then* merge the patch.
That's a good idea. Such information would also make it easier for reviewers to verify large mechanically-generated patches.
If it's OK with you, I'll take your email with minor edits && put it on git.freeradius.org as "how to do patches". :)
Sure, go ahead.
participants (4)
-
Alan DeKok -
Frank Cusack -
Jeffrey Hutzelman -
Wang Tinggong