Re: [PATCH v2 2/3] docs: clarify cmd_psuh signature and explain UNUSED macro

K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:

The above is not quite easy to reason about. It is more like we wrote this document, and then later tightened the default compiler warnings for developer builds. So "omitted" may technically be correct, but it was more like "did not use it, because there was no need".

    The sample program, as written, would not build for at least two
    reasons:

    - Since this document was first written, the calling convention
      to subcommand implementation has changed, and now cmd_psuh()
      needs to accept the third parameter, repository.

    - These days, compiler warning options for developers include
      one that detects and complains about unused parameters, so
      ones that are deliberately unused have to be marked as such.

After such observation on the status quo and description of the problem you are going to solve, you give an order to the code base to fix it, perhaps like:

    Update the old-style examples to adjust to the current
    practices, with explanations as needed.

To recap, the usual way to compose a log message of this project is to

 - Give an observation on how the current system works in the
   present tense (so no need to say "Currently X is Y", or
   "Previously X was Y" to describe the state before your change;
   just "X is Y" is enough), and discuss what you perceive as a
   problem in it.

 - Propose a solution (optional---often, problem description
   trivially leads to an obvious solution in reader's minds).

 - Give commands to the codebase to "become like so".

in this order.

I do not quite understand. Why do we need a new one here? Wouldn't it be easier to read for a newcomer if you just give the last one and explain what UNUSED are for? Perhaps like

    ... matching the style and signature:

    ----
    int cmd_psuh(int argc UNUSED, const char **argv UNUSED,
	         const char *prefix UNUSED, struct repository *repo UNUSED)
    ----

    A few things to note:

    * A subcommand implementation takes its command line arguments
      in `int argc` + `const char **argv`, like `main()` would

    * It also takes two extra parameters, `prefix` and `repo`.  What
      they mean will not be discussed until much later.

    * Because this first example will not use any of the parameters,
      your compiler will give warnings on unused parameters.  As the
      list of these four parameters is mandated by the API to add
      new built-in commands, you cannot omit them.  Instead, you add
      `UNUSED` to each of them to tell the compiler that you _know_
      you are not (yet) using it.

Take a special note on the last one. There may be multiple ways to squelch warnings, but it is worth telling your readers that use of UNUSED is the right way.

I'll stop here for this patch.

Thanks.