On Mon, Mar 24, 2014 at 8:08 AM, Ashwin Jha <ajha.dev@xxxxxxxxx> wrote:
> Subject: fsck.c: modify fsck_ident() and fsck_commit()
The subject should summarize the change while being concise and
expressive. The above is a bit lacking. A better subject might be:
Subject: fsck: add missing 'const's
> In fsck_ident(): Replace argument char **ident with const char **ident
> In fsck_commit(): Replace char *buffer with const char *buffer
No need to repeat in prose what the patch itself already states more
concisely and precisely. You can drop this part.
> In both the cases, referenced memory addresses are not modified. So, it
> will be a good practice, to declare them as const.
The subject suggested above ("add missing 'const's") implies that the
referenced memory is never modified, so it's not really necessary to
explain it here too. Stating that it's good practice may be a
reasonable way to justify the change, although is also rather obvious.
I'd say that the suggested subject alone is a sufficient commit
message for this patch, though you and/or Junio might feel otherwise.
> Signed-off-by: Ashwin Jha <ajha.dev@xxxxxxxxx>
> ---
>
> Change in fsck_commit() and fsck_ident() as per the discussion with
> Eric (follow at [1]).
> [1]: http://git.661346.n2.nabble.com/PATCH-Modify-fsck-commit-Replace-memcmp-by-skip-prefix-td7606321.html
Providing a link to the previous attempt is indeed courteous to
reviewers. Since reviewer time is precious (due to this being a
high-volume mailing list), it is possible to be even more helpful by
providing more detail in your summary of changes. For instance,
instead of saying "Change in ... as per discussion", you might say:
Changes since v2 [1]:
Expanded to 2-patch series.
patch 1/1: add missing 'const's to fsck_ident() argument and
fsck_commit() 'buffer' variable so that 2/2 doesn't have to
cast-away constness from the result of skip_prefix().
patch 2/2: dropped the now unnecessary casts
The patch itself looks fine, thanks.
> fsck.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fsck.c b/fsck.c
> index 64bf279..b5f017f 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -243,7 +243,7 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)
> return retval;
> }
>
> -static int fsck_ident(char **ident, struct object *obj, fsck_error error_func)
> +static int fsck_ident(const char **ident, struct object *obj, fsck_error error_func)
> {
> char *end;
>
> @@ -284,7 +284,7 @@ static int fsck_ident(char **ident, struct object *obj, fsck_error error_func)
>
> static int fsck_commit(struct commit *commit, fsck_error error_func)
> {
> - char *buffer = commit->buffer;
> + const char *buffer = commit->buffer;
> unsigned char tree_sha1[20], sha1[20];
> struct commit_graft *graft;
> int parents = 0;
> --
> 1.9.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html