Hi:
On Fri, Jan 9, 2009 at 6:49 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> I do not see anything special you do for "one commit" case in your patch,
> yet the proposed commit message keeps stressing "-1", which puzzles me.
I was trying to address Alexander's concerns he brought up previously
in the thread.
> Wouldn't it suffice to simply say something like:
>
> You need to explicitly ask for --root to obtain a patch for the root
> commit. This may have been a good way to make sure that the user
> realizes that a patch from the root commit won't be applicable to a
> history with existing data, but we should assume the user knows what
> he is doing when the user explicitly specifies a range of commits that
> includes the root commit.
>
Indeed it would. I was giving a specific case that shows what problem
this patch addresses.
> Three issues.
>
> - The "if(){" violates style by not having one SP before "(" and after ")",
> and surrounds a single statement with needless { } pair. You need one SP
> on each side of the = (assignment) as well.
>
> - Because rev.show_root_diff is a no-op for non-root commit anyway, I do not
> think you even want a conditional there.
>
> - It is a bad style to muck with rev.* while it is actively used for
> iteration (note that the above part is in a while loop that iterates over
> &rev).
Thanks for the advice. I shall adhere to it next time I submit a patch.
> I think the attached would be a better patch. We already have a
> configuration to control if we show the patch for a root commit by
> default, and we can use reuse it here. The configuration defaults to true
> these days.
I did not realize this configuration was available. The patch below
is much more elegant.
> Because the code before the hunk must check if the user said "--root
> commit" or just "commit" from the command line and behave quite
> differently by looking at rev.show_root_diff, we cannot do this assignment
> before the command line parsing like other commands in the log family.
>
> builtin-log.c | 8 ++++++++
> 1 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git c/builtin-log.c w/builtin-log.c
> index 4a02ee9..2d2c111 100644
> --- c/builtin-log.c
> +++ w/builtin-log.c
> @@ -935,6 +935,14 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> * get_revision() to do the usual traversal.
> */
> }
> +
> + /*
> + * We cannot move this anywhere earlier because we do want to
> + * know if --root was given explicitly from the comand line.
> + */
> + if (default_show_root)
> + rev.show_root_diff = 1;
> +
> if (cover_letter) {
> /* remember the range */
> int i;
>
Thanks,
Nathan Panike
--
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
