On Fri, 2016-08-26 at 08:45 +0200, Sven Eckelmann wrote:
On Donnerstag, 25. August 2016 14:39:56 CEST Jonathan Haws wrote:
Signed-off-by: Jonathan Haws jhaws@sdl.usu.edu
Added time_subtract() utility function to use for simply subtracting one timespec from another.
Please add a small changelog when you send a new revision (after the first "---"). Please also add the v2/v3/v4/... version to your [PATCH...] prefix for patches replacing an older version. These are generated automatically by git- format-patch when you add the parameter "-v 2" or "-v 3" or ....
A good example is https://patchwork.open-mesh.org/patch/16663/
Please use the format described in https://www.kernel.org/doc/Documentation/SubmittingPatches
The canonical patch subject line is: Subject: [PATCH 001/123] subsystem: summary phrase
The canonical patch message body contains the following: - A "from" line specifying the patch author (only needed if the person sending the patch is not the author). - An empty line. - The body of the explanation, line wrapped at 75 columns, which will be copied to the permanent changelog to describe this patch. - The "Signed-off-by:" lines, described above, which will also go in the changelog. - A marker line containing simply "---". - Any additional comments not suitable for the changelog. - The actual patch (diff output).
You don't seem to follow these rules yet. Instead you seem switch the Signed- off-by: line with the body of the explanation. So a git-am with -s (as the maintainers do here) would then produce this mess:
alfred: Adding time_subtract utility function Signed-off-by: Jonathan Haws jhaws@sdl.usu.edu Added time_subtract() utility function to use for simply subtracting one timespec from another. Signed-off-by: Sven Eckelmann sven@narfation.org
[...]
I've backed up to origin/master and will produce a single patch. The "signed off by:" was a misunderstanding on my part. Here is what the commit log will look like:
alfred: Externalized synchronization interval
ALFRED_INTERVAL is now externalized via the -p option (synchronization period). If specified as option, user supplied interval is used, otherwise the default interval of ALFRED_INTERVAL is used.
Does that look right?
index c5df945..884a1a7 100644 --- a/server.c +++ b/server.c @@ -372,7 +372,10 @@ int alfred_server(struct globals *globals)
while (1) { clock_gettime(CLOCK_MONOTONIC, &now);
time_diff(&now, &globals->sync_period, &now);
/* subtract the synchronization period from the
current time */
time_subtract(&now, &globals->sync_period, &now);
I'm guessing Simon meant to have the comment in the first patch. I don't get why you now introduce a time_subtract which is just a copy of time_diff (without the return value).
My reading of his response was that time_diff was meant for something completely different, thus shouldn't be used here. Thus I created time_subtract.
+void time_subtract(struct timespec *tv1, struct timespec *tv2,
- struct timespec *tvout) {
- tvout->tv_sec = tv1->tv_sec - tv2->tv_sec;
- if (tv1->tv_nsec < tv2->tv_nsec) {
tvout->tv_nsec = 1000000000 + tv1->tv_nsec - tv2-
tv_nsec;
tvout->tv_sec -= 1;
- } else {
tvout->tv_nsec = tv1->tv_nsec - tv2->tv_nsec;
- }
Have you checked the glibc documentation about this topic (search for timeval_subtract)?
https://www.gnu.org/software/libc/manual/html_node/Elapsed-Time.html
This might be relevant when you want to create an function which does handle more things than time_diff currently does. Otherwise I don't see a reason why we just have the same code twice in util.c
Yeah, I've looked at that. The code they present there essentially does the same thing. I can pull that in and have a timespec_subtract call, or I can go back to time_diff with the comment. Which would you prefer?
- return;
+}
Why a return here?
Style. I never have a function without a return, even voids. Maybe I'm a bit OCD on that point, but I just feel happier seeing it there. :) Depending on what you want to do with time_subtract, we can remove this return or replace the function.
Btw. I am seeing whitespace warnings when applying the patches. Maybe you can check them:
Applying patch #16667 using 'git am' Description: [1/2] alfred: Externalized synchronization interval Applying: alfred: Externalized synchronization interval .git/rebase-apply/patch:89: trailing whitespace. ALFRED_INTERVAL setting of 10 seconds will be used. Fractional seconds are warning: 1 line adds whitespace errors. Applying patch #16668 using 'git am' Description: [2/2] alfred: Adding time_subtract utility function Applying: alfred: Adding time_subtract utility function
I'm not sure where this would have cropped up, but I'll double check it before I submit the next patch version.
I'm new to the contributing game - I've read through the wiki, but apparently there are other details that I missed. Is there some good documentation on the typical procedure to follow and formats and whatnot to read up on? I thought I had understood everything, but apparently not. I hate to cause extra work for you maintainers simply because I can't get a simple patch right.
Thanks!