From: Thiago Farina <tfransosi@gmail.com>
Date: Sun, 24 Apr 2011 22:18:35 GMT
Subject: Re: [PATCH 2/4] diff.c: return filepair from diff_unmerge()
Message-ID: <BANLkTi=fnzRVc8Lx_Wba4yqa7u8t3xGAtQ@mail.gmail.com>
In-Reply-To: <1303678289-27627-3-git-send-email-gitster@pobox.com>

Some unrelated style comments below.

On Sun, Apr 24, 2011 at 5:51 PM, Junio C Hamano <gitster@pobox.com> wrote:
> The underlying diff_queue() returns diff_filepair so that the caller can
> further add information to it, and the helper function diff_unmerge()
> utilizes the feature itself, but does not expose it to its callers, which
> was kind of selfish.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  diff.c |   13 ++++++++-----
>  diff.h |    2 +-
>  2 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index 9a5c77c..4c34c64 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4308,20 +4308,23 @@ void diff_change(struct diff_options *options,
>                DIFF_OPT_SET(options, HAS_CHANGES);
>  }
>
> -void diff_unmerge(struct diff_options *options,
> -                 const char *path,
> -                 unsigned mode, const unsigned char *sha1)
> +struct diff_filepair *diff_unmerge(struct diff_options *options,
> +                                  const char *path,
> +                                  unsigned mode, const unsigned char *sha1)
>  {

While you are here, why not write one arg per line?

> +       struct diff_filepair *pair;
>        struct diff_filespec *one, *two;
>
>        if (options->prefix &&
>            strncmp(path, options->prefix, options->prefix_length))
> -               return;
> +               return NULL;
>
>        one = alloc_filespec(path);
>        two = alloc_filespec(path);
>        fill_filespec(one, sha1, mode);
> -       diff_queue(&diff_queued_diff, one, two)->is_unmerged = 1;
> +       pair = diff_queue(&diff_queued_diff, one, two);
> +       pair->is_unmerged = 1;
> +       return pair;
>  }
>
>  static char *run_textconv(const char *pgm, struct diff_filespec *spec,
> diff --git a/diff.h b/diff.h
> index bf2f44d..f51a8ee 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -209,7 +209,7 @@ extern void diff_change(struct diff_options *,
>                        const char *fullpath,
>                        unsigned dirty_submodule1, unsigned dirty_submodule2);
>
> -extern void diff_unmerge(struct diff_options *,
> +extern struct diff_filepair *diff_unmerge(struct diff_options *,

While you are here, why not add the argument name |options| here too?

>                         const char *path,
>                         unsigned mode,
>                         const unsigned char *sha1);