[Spice-devel] [PATCH RFC] Add HACKING file

[Marc-André Lureau]

So much of this is quite irrelevant to the discussion at hand about
unreviewed commit rule.

We already have a cooding style:
http://www.spice-space.org/docs/spice_style.pdf although as always,
it's arguable and we haven't been following too strictly (thankfully)

In general I don't think we need that document, because we follow very
common participation rules. Having strict rules makes contributions
more difficult and that's really not what we are after at this point.

About unreview commit, this is a good summary of what we already
apply. Bu the problem will remain that deciding whether a change
"trivial" is subjective. And you removed some part related to make
check, I am not sure why. I beleive my autogen.sh fix was obvious and
fixed an obvious build problem and didn't required all this fuss. Many
other parts of Spice would deserve that attention, not an autogen.sh
fix.

On Mon, Dec 8, 2014 at 2:56 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
> This is libvirt's HACKING file with some parts not relevant to SPICE
> removed. It contains some git advice, some C coding style rules, and
> an attempt at defining what a trivial patch is.
> ---
> Hey,
>
> After the latest thread about trivial patches, I realized that libvirt actually
> has a definition of what patches can be pushed without review in their HACKING
> file. As it would be useful for us to have an agreed on definition for that, I
> went ahead and created a HACKING file. As the libvirt file had more useful bits,
> I tried to keep those as well (and also removed quite a bit of stuff).
>
>
> Christophe
>
>
>  HACKING     | 394 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  Makefile.am |   1 +
>  2 files changed, 395 insertions(+)
>  create mode 100644 HACKING
>
> diff --git a/HACKING b/HACKING
> new file mode 100644
> index 0000000..2a18038
> --- /dev/null
> +++ b/HACKING
> @@ -0,0 +1,394 @@
> +                         Contributor guidelines
> +                         ======================
> +
> +
> +
> +General tips for contributing patches
> +=====================================
> +(1) Discuss any large changes on the mailing list first. Post patches early and
> +listen to feedback.
> +
> +
> +
> +(2) Official upstream repository is kept in git ("http://cgit.freedesktop.org/spice/spice/")
> +and is browsable along with other SPICE-related repositories (e.g.
> +spice-protocol) online <http://cgit.freedesktop.org/spice/>.
> +
> +
> +
> +(3) Post patches in unified diff format, with git rename detection enabled. You
> +need a one-time setup of:
> +
> +  git config diff.renames true
> +
> +After that, a command similar to this should work:
> +
> +  diff -urp spice.orig/ spice.modified/ > spice-myfeature.patch
> +
> +or:
> +
> +  git diff > spice-myfeature.patch
> +
> +Also, for code motion patches, you may find that "git diff --patience"
> +provides an easier-to-read patch. However, the usual workflow of spice
> +developer is:
> +
> +  git checkout master
> +  git pull
> +  git checkout -t origin -b workbranch
> +  Hack, committing any changes along the way
> +
> +When you want to post your patches:
> +
> +  git pull --rebase
> +  (fix any conflicts)
> +  git send-email --cover-letter --no-chain-reply-to --annotate \
> +                 --to=spice-devel at lists.freedesktop.org master
> +
> +(Note that the "git send-email" subcommand may not be in the main git package
> +and using it may require installation of a separate package, for example the
> +"git-email" package in Fedora.) For a single patch you can omit
> +"--cover-letter", but a series of two or more patches needs a cover letter. If
> +you get tired of typing "--to=spice-devel at lists.freedesktop.org" designation you can set
> +it in git config:
> +
> +  git config sendemail.to spice-devel at lists.freedesktop.org
> +
> +Please follow this as close as you can, especially the rebase and git
> +send-email part, as it makes life easier for other developers to review your
> +patch set. One should avoid sending patches as attachments, but rather send
> +them in email body along with commit message. If a developer is sending
> +another version of the patch (e.g. to address review comments), they are advised
> +to note differences to previous versions after the "---" line in the patch so
> +that it helps reviewers but doesn't become part of git history. Moreover, such
> +patch needs to be prefixed correctly with "--subject-prefix=PATCHv2" appended
> +to "git send-email" (substitute "v2" with the correct version if needed
> +though).
> +
> +
> +
> +(4) In your commit message, make the summary line reasonably short (60 characters
> +is typical), followed by a blank line, followed by any longer description of
> +why your patch makes sense. If the patch fixes a regression, and you know what
> +commit introduced the problem, mentioning that is useful. If the patch
> +resolves a bugzilla report, mentioning the URL of the bug number is useful;
> +but also summarize the issue rather than making all readers follow the link.
> +You can use 'git shortlog -30' to get an idea of typical summary lines.
> +SPICE does not currently attach any meaning to Signed-off-by: lines, so it
> +is up to you if you want to include or omit them in the commit message.
> +
> +
> +
> +(5) Split large changes into a series of smaller patches, self-contained if
> +possible, with an explanation of each patch and an explanation of how the
> +sequence of patches fits together. Moreover, please keep in mind that it's
> +required to be able to compile cleanly after each patch. A feature does not
> +have to work until the end of a series, but intermediate patches must compile
> +and not cause test-suite failures (this is to preserve the usefulness of "git
> +bisect", among other things).
> +
> +
> +
> +(6) Make sure your patches apply against SPICE GIT. Developers only follow GIT
> +and don't care much about released versions.
> +
> +
> +
> +There is more on this subject, including lots of links to background reading
> +on the subject, on Richard Jones' guide to working with open source projects
> +<http://people.redhat.com/rjones/how-to-supply-code-to-open-source-projects/>.
> +
> +
> +Code indentation
> +================
> +SPICE's C source code generally adheres to some basic code-formatting
> +conventions. The existing code base is not totally consistent on this front,
> +but we do prefer that contributed code be formatted similarly. In short, use
> +spaces-not-TABs for indentation, use 4 spaces for each indentation level, and
> +other than that, follow the K&R style.
> +
> +
> +Code formatting (especially for new code)
> +=========================================
> +
> +Most of the code base prefers to stick to C89 syntax unless there is a
> +compelling reason otherwise. For example, it is preferable to use "/* */"
> +comments rather than "//". Also, when declaring local variables, the prevailing
> +style has been to declare them at the beginning of a scope, rather than
> +immediately before use.
> +
> +
> +Bracket spacing
> +===============
> +The keywords "if", "for", "while", and "switch" must have a single space
> +following them before the opening bracket. E.g.
> +
> +      if(foo)   // Bad
> +      if (foo)  // Good
> +
> +Function implementations must not have any whitespace between the function name
> +and the opening bracket. E.g.
> +
> +      int foo (int wizz)  // Bad
> +      int foo(int wizz)   // Good
> +
> +Function calls must not have any whitespace between the function name and the
> +opening bracket. E.g.
> +
> +      bar = foo (wizz);  // Bad
> +      bar = foo(wizz);   // Good
> +
> +Function typedefs must not have any whitespace between the closing bracket of
> +the function name and opening bracket of the arg list. E.g.
> +
> +      typedef int (*foo) (int wizz);  // Bad
> +      typedef int (*foo)(int wizz);   // Good
> +
> +There must not be any whitespace immediately following any opening bracket, or
> +immediately prior to any closing bracket. E.g.
> +
> +      int foo( int wizz );  // Bad
> +      int foo(int wizz);    // Good
> +
> +
> +Commas
> +======
> +Commas should always be followed by a space or end of line, and never have
> +leading space.
> +
> +      call(a,b ,c);// Bad
> +      call(a, b, c); // Good
> +
> +When declaring an enum or using a struct initializer that occupies more than
> +one line, use a trailing comma. That way, future edits to extend the list only
> +have to add a line, rather than modify an existing line to add the
> +intermediate comma.
> +
> +      enum {
> +          VALUE_ONE,
> +          VALUE_TWO // Bad
> +      };
> +      enum {
> +          VALUE_THREE,
> +          VALUE_FOUR, // Good
> +      };
> +
> +
> +Semicolons
> +==========
> +Semicolons should never have a space beforehand. Inside the condition of a
> +"for" loop, there should always be a space or line break after each semicolon,
> +except for the special case of an infinite loop (although more infinite loops
> +use "while"). While not enforced, loop counters generally use post-increment.
> +
> +      for (i = 0 ;i < limit ; ++i) { // Bad
> +      for (i = 0; i < limit; i++) { // Good
> +      for (;;) { // ok
> +      while (1) { // Better
> +
> +Empty loop bodies are better represented with curly braces and a comment,
> +although use of a semicolon is not currently rejected.
> +
> +      while ((rc = waitpid(pid, &st, 0) == -1) &&
> +             errno == EINTR); // ok
> +      while ((rc = waitpid(pid, &st, 0) == -1) &&
> +             errno == EINTR) { // Better
> +          /* nothing */
> +      }
> +
> +
> +Preprocessor
> +============
> +Macros defined with an ALL_CAPS name should generally be assumed to be unsafe
> +with regards to arguments with side-effects (that is, MAX(a++, b--) might
> +increment a or decrement b too many or too few times). Exceptions to this rule
> +are explicitly documented for macros in viralloc.h and virstring.h.
> +
> +For variadic macros, stick with C99 syntax:
> +
> +  #define vshPrint(_ctl, ...) fprintf(stdout, __VA_ARGS__)
> +
> +Use parenthesis when checking if a macro is defined, and use indentation to
> +track nesting:
> +
> +  #if defined(HAVE_POSIX_FALLOCATE) && !defined(HAVE_FALLOCATE)
> +  # define fallocate(a, ignored, b, c) posix_fallocate(a, b, c)
> +  #endif
> +
> +
> +C types
> +=======
> +Use the right type.
> +
> +Scalars
> +-------
> +- If you're using "int" or "long", odds are good that there's a better type.
> +
> +- If a variable is counting something, be sure to declare it with an unsigned
> +type.
> +
> +- If it's memory-size-related, use "size_t" (use "ssize_t" only if required).
> +
> +- If it's file-size related, use uintmax_t, or maybe "off_t".
> +
> +- If it's file-offset related (i.e., signed), use "off_t".
> +
> +- If it's just counting small numbers use "unsigned int"; (on all but oddball
> +embedded systems, you can assume that that type is at least four bytes wide).
> +
> +- In the unusual event that you require a specific width, use a standard type
> +like "int32_t", "uint32_t", "uint64_t", etc.
> +
> +- While using "bool" is good for readability, it comes with minor caveats:
> +
> +-- Don't use "bool" in places where the type size must be constant across all
> +systems, like public interfaces and on-the-wire protocols.
> +
> +-- Don't compare a bool variable against the literal, "true", since a value with
> +a logical non-false value need not be "1". I.e., don't write "if (seen ==
> +true) ...". Rather, write "if (seen)...".
> +
> +
> +
> +
> +
> +Of course, take all of the above with a grain of salt. If you're about to use
> +some system interface that requires a type like "size_t", "pid_t" or "off_t",
> +use matching types for any corresponding variables.
> +
> +Also, if you try to use e.g., "unsigned int" as a type, and that conflicts
> +with the signedness of a related variable, sometimes it's best just to use the
> +*wrong* type, if 'pulling the thread' and fixing all related variables would
> +be too invasive.
> +
> +Finally, while using descriptive types is important, be careful not to go
> +overboard. If whatever you're doing causes warnings, or requires casts, then
> +reconsider or ask for help.
> +
> +Pointers
> +--------
> +Ensure that all of your pointers are 'const-correct'. Unless a pointer is used
> +to modify the pointed-to storage, give it the "const" attribute. That way, the
> +reader knows up-front that this is a read-only pointer. Perhaps more
> +importantly, if we're diligent about this, when you see a non-const pointer,
> +you're guaranteed that it is used to modify the storage it points to, or it is
> +aliased to another pointer that is.
> +
> +
> +Include files
> +=============
> +There are now quite a large number of include files, both SPICE internal and
> +external, and system includes. To manage all this complexity it's best to
> +stick to the following general plan for all *.c source files:
> +
> +  /*
> +   * Copyright notice
> +   * ....
> +   * ....
> +   * ....
> +   *
> +   */
> +
> +  #include <config.h>             Must come first in every file.
> +
> +  #include <stdio.h>              Any system includes you need.
> +  #include <string.h>
> +  #include <limits.h>
> +
> +  #if WITH_NUMACTL                Some system includes aren't supported
> +  # include <numa.h>              everywhere so need these #if guards.
> +  #endif
> +
> +  #include "util.h"               Any SPICE internal header files.
> +  #include "buf.h"
> +
> +  static int
> +  myInternalFunc()                The actual code.
> +  {
> +      ...
> +
> +
> +Use of goto
> +===========
> +The use of goto is not forbidden, and goto is widely used throughout SPICE.
> +While the uncontrolled use of goto will quickly lead to unmaintainable code,
> +there is a place for it in well structured code where its use increases
> +readability and maintainability. In general, if goto is used for error
> +recovery, it's likely to be ok, otherwise, be cautious or avoid it all
> +together.
> +
> +The typical use of goto is to jump to cleanup code in the case of a long list
> +of actions, any of which may fail and cause the entire operation to fail. In
> +this case, a function will have a single label at the end of the function.
> +It's almost always ok to use this style.
> +
> +There are a couple of signs that a particular use of goto is not ok:
> +
> +- You're using multiple labels. If you find yourself using multiple labels,
> +you're strongly encouraged to rework your code to eliminate all but one of
> +them.
> +
> +- The goto jumps back up to a point above the current line of code being
> +executed. Please use some combination of looping constructs to re-execute code
> +instead; it's almost certainly going to be more understandable by others. One
> +well-known exception to this rule is restarting an i/o operation following
> +EINTR.
> +
> +- The goto jumps down to an arbitrary place in the middle of a function followed
> +by further potentially failing calls. You should almost certainly be using a
> +conditional and a block instead of a goto. Perhaps some of your function's
> +logic would be better pulled out into a helper function.
> +
> +
> +
> +Although SPICE does not encourage the Linux kernel wind/unwind style of
> +multiple labels, there's a good general discussion of the issue archived at
> +KernelTrap <http://kerneltrap.org/node/553/2131>
> +
> +When using goto, please use one of these standard labels if it makes sense:
> +
> +      error: A path only taken upon return with an error code
> +    cleanup: A path taken upon return with success code + optional error
> +  no_memory: A path only taken upon return with an OOM error code
> +      retry: If needing to jump upwards (e.g., retry on EINTR)
> +
> +Top-level labels should be indented by one space (putting them on the
> +beginning of the line confuses function context detection in git):
> +
> +int foo()
> +{
> +    /* ... do stuff ... */
> + cleanup:
> +    /* ... do other stuff ... */
> +}
> +
> +
> +SPICE committer guidelines
> +==========================
> +The AUTHORS files indicates the list of people with commit access right who
> +can actually merge the patches.
> +
> +The general rule for committing a patch is to make sure it has been reviewed
> +properly in the mailing-list first, usually if a couple of people gave an ACK
> +or +1 to a patch and nobody raised an objection on the list it should be good
> +to go. If the patch touches a part of the code where you're not the main
> +maintainer, or where you do not have a very clear idea of how things work,
> +it's better to wait for a more authoritative feedback though. Before
> +committing, please also rebuild locally, and make sure you don't raise errors.
> +Try to look for warnings too; for example, compile with
> +
> +  make CFLAGS="-Werror"
> +
> +which adds -Werror to compile flags, so no warnings get missed
> +
> +An exception to 'review and approval on the list first' is fixing failures to
> +build:
> +
> +- if a recently committed patch breaks compilation on a platform or for a given
> +driver, then it's fine to commit a minimal fix directly without getting the
> +review feedback first
> +
> +- fixes for documentation and code comments can be managed in the same way, but
> +still make sure they get reviewed if non-trivial.
> +
> +The patch should still be sent to the list (or tell what the fix was if trivial)
> diff --git a/Makefile.am b/Makefile.am
> index cada94d..5e552b0 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -15,6 +15,7 @@ DISTCHECK_CONFIGURE_FLAGS =                   \
>         $(NULL)
>
>  EXTRA_DIST =                                   \
> +       HACKING                                 \
>         build-aux/git-version-gen               \
>         .version                                \
>         $(NULL)
> --
> 2.1.0
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel



-- 
Marc-André Lureau