Re: [PATCH 2/3] builtin/apply: make gitdiff_verify_name() return void
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 22, 2016, 21:25 UTC
- Message-ID
- <xmqq37rigq7o.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <1458680322-17681-3-git-send-email-chriscool@tuxfamily.org>
Christian Couder <christian.couder@gmail.com> writes:
Show 7 quoted lines
> As the value returned by gitdiff_verify_name() is put into the > same variable that is passed as a parameter to this function, > it is simpler to pass the address of the variable and have > gitdiff_verify_name() change the variable itself. > > Signed-off-by: Christian Couder <chriscool@tuxfamily.org> > ---
This change makes the function less useful by restricting the callers--only the ones that wants in-place update can call it after this change, while the old function signature allowed a caller to pass one variable as orig, receive the result in another variable (and probably compare them).
It does not matter very much for this file scope static helper either way, and I would probably say the same thing if the patch were in reverse (i.e. if the patch were loosening the restriction), but I cannot offhand see why this is an improvement. Puzzled...
Show 67 quoted lines
> builtin/apply.c | 26 +++++++++++++-------------
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index 465f954..4cafdaf 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -925,37 +925,37 @@ static int gitdiff_hdrend(const char *line, struct patch *patch)
> #define DIFF_OLD_NAME 0
> #define DIFF_NEW_NAME 1
>
> -static char *gitdiff_verify_name(const char *line, int isnull, char *orig_name, int side)
> +static void gitdiff_verify_name(const char *line, int isnull, char **name, int side)
> {
> - if (!orig_name && !isnull)
> - return find_name(line, NULL, p_value, TERM_TAB);
> + if (!*name && !isnull) {
> + *name = find_name(line, NULL, p_value, TERM_TAB);
> + return;
> + }
>
> - if (orig_name) {
> - int len = strlen(orig_name);
> + if (*name) {
> + int len = strlen(*name);
> char *another;
> if (isnull)
> die(_("git apply: bad git-diff - expected /dev/null, got %s on line %d"),
> - orig_name, linenr);
> + *name, linenr);
> another = find_name(line, NULL, p_value, TERM_TAB);
> - if (!another || memcmp(another, orig_name, len + 1))
> + if (!another || memcmp(another, *name, len + 1))
> die((side == DIFF_NEW_NAME) ?
> _("git apply: bad git-diff - inconsistent new filename on line %d") :
> _("git apply: bad git-diff - inconsistent old filename on line %d"), linenr);
> free(another);
> - return orig_name;
> } else {
> /* expect "/dev/null" */
> if (memcmp("/dev/null", line, 9) || line[9] != '\n')
> die(_("git apply: bad git-diff - expected /dev/null on line %d"), linenr);
> - return NULL;
> }
> }
>
> static int gitdiff_oldname(const char *line, struct patch *patch)
> {
> char *orig = patch->old_name;
> - patch->old_name = gitdiff_verify_name(line, patch->is_new, patch->old_name,
> - DIFF_OLD_NAME);
> + gitdiff_verify_name(line, patch->is_new, &patch->old_name,
> + DIFF_OLD_NAME);
> if (orig != patch->old_name)
> free(orig);
> return 0;
> @@ -964,8 +964,8 @@ static int gitdiff_oldname(const char *line, struct patch *patch)
> static int gitdiff_newname(const char *line, struct patch *patch)
> {
> char *orig = patch->new_name;
> - patch->new_name = gitdiff_verify_name(line, patch->is_delete, patch->new_name,
> - DIFF_NEW_NAME);
> + gitdiff_verify_name(line, patch->is_delete, &patch->new_name,
> + DIFF_NEW_NAME);
> if (orig != patch->new_name)
> free(orig);
> return 0;