# [PATCH 0/3] builtin/apply: simplify some gitdiff_* functions 8 messages from 2016-03-22 to 2016-03-22. Participants: Christian Couder, Junio C Hamano. Thread: https://gitlist.dev/t/41786 ## Christian Couder, 2016-03-22 20:58 Subject: [PATCH 0/3] builtin/apply: simplify some gitdiff_* functions Message-ID: <1458680322-17681-1-git-send-email-chriscool@tuxfamily.org> URL: https://gitlist.dev/e/1458680322-17681-1-git-send-email-chriscool%40tuxfamily.org ``` While working on libifying "git apply" it appeared that some gitdiff_* functions are unnecessarily complex. Christian Couder (3): builtin/apply: get rid of useless 'name' variable builtin/apply: make gitdiff_verify_name() return void builtin/apply: simplify gitdiff_{old,new}name() builtin/apply.c | 39 +++++++++++++++------------------------ 1 file changed, 15 insertions(+), 24 deletions(-) -- 2.8.0.rc4.1.g302de0d.dirty ``` ## Christian Couder, 2016-03-22 20:58 Subject: [PATCH 1/3] builtin/apply: get rid of useless 'name' variable Message-ID: <1458680322-17681-2-git-send-email-chriscool@tuxfamily.org> URL: https://gitlist.dev/e/1458680322-17681-2-git-send-email-chriscool%40tuxfamily.org In-Reply-To: <1458680322-17681-1-git-send-email-chriscool@tuxfamily.org> ``` While at it put an 'else' on the same line as the previous '}'. Signed-off-by: Christian Couder --- builtin/apply.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/builtin/apply.c b/builtin/apply.c index 42c610e..465f954 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -931,22 +931,19 @@ static char *gitdiff_verify_name(const char *line, int isnull, char *orig_name, return find_name(line, NULL, p_value, TERM_TAB); if (orig_name) { - int len; - const char *name; + int len = strlen(orig_name); char *another; - name = orig_name; - len = strlen(name); if (isnull) - die(_("git apply: bad git-diff - expected /dev/null, got %s on line %d"), name, linenr); + die(_("git apply: bad git-diff - expected /dev/null, got %s on line %d"), + orig_name, linenr); another = find_name(line, NULL, p_value, TERM_TAB); - if (!another || memcmp(another, name, len + 1)) + if (!another || memcmp(another, orig_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 { + } 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); -- 2.8.0.rc4.1.g302de0d.dirty ``` ## Christian Couder, 2016-03-22 20:58 Subject: [PATCH 2/3] builtin/apply: make gitdiff_verify_name() return void Message-ID: <1458680322-17681-3-git-send-email-chriscool@tuxfamily.org> URL: https://gitlist.dev/e/1458680322-17681-3-git-send-email-chriscool%40tuxfamily.org In-Reply-To: <1458680322-17681-1-git-send-email-chriscool@tuxfamily.org> ``` 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 --- 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; -- 2.8.0.rc4.1.g302de0d.dirty ``` ## Christian Couder, 2016-03-22 20:58 Subject: [PATCH 3/3] builtin/apply: simplify gitdiff_{old,new}name() Message-ID: <1458680322-17681-4-git-send-email-chriscool@tuxfamily.org> URL: https://gitlist.dev/e/1458680322-17681-4-git-send-email-chriscool%40tuxfamily.org In-Reply-To: <1458680322-17681-1-git-send-email-chriscool@tuxfamily.org> ``` After the previous simplifications, it is easy to see that there is no need to free the original string passed to gitdiff_verify_name(), because this string can be changed only when it is NULL. Signed-off-by: Christian Couder --- builtin/apply.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/builtin/apply.c b/builtin/apply.c index 4cafdaf..9cfa9f4 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -953,21 +953,15 @@ static void gitdiff_verify_name(const char *line, int isnull, char **name, int s static int gitdiff_oldname(const char *line, struct patch *patch) { - char *orig = patch->old_name; gitdiff_verify_name(line, patch->is_new, &patch->old_name, DIFF_OLD_NAME); - if (orig != patch->old_name) - free(orig); return 0; } static int gitdiff_newname(const char *line, struct patch *patch) { - char *orig = patch->new_name; gitdiff_verify_name(line, patch->is_delete, &patch->new_name, DIFF_NEW_NAME); - if (orig != patch->new_name) - free(orig); return 0; } -- 2.8.0.rc4.1.g302de0d.dirty ``` ## Junio C Hamano, 2016-03-22 21:20 Subject: Re: [PATCH 1/3] builtin/apply: get rid of useless 'name' variable Message-ID: URL: https://gitlist.dev/e/xmqq7fgugqfp.fsf%40gitster.mtv.corp.google.com In-Reply-To: <1458680322-17681-2-git-send-email-chriscool@tuxfamily.org> ``` Christian Couder writes: > While at it put an 'else' on the same line as the previous '}'. > > Signed-off-by: Christian Couder > --- > builtin/apply.c | 13 +++++-------- > 1 file changed, 5 insertions(+), 8 deletions(-) > > diff --git a/builtin/apply.c b/builtin/apply.c > index 42c610e..465f954 100644 > --- a/builtin/apply.c > +++ b/builtin/apply.c > @@ -931,22 +931,19 @@ static char *gitdiff_verify_name(const char *line, int isnull, char *orig_name, > return find_name(line, NULL, p_value, TERM_TAB); > > if (orig_name) { > - int len; > - const char *name; > + int len = strlen(orig_name); > char *another; > - name = orig_name; > - len = strlen(name); > if (isnull) > - die(_("git apply: bad git-diff - expected /dev/null, got %s on line %d"), name, linenr); > + die(_("git apply: bad git-diff - expected /dev/null, got %s on line %d"), > + orig_name, linenr); > another = find_name(line, NULL, p_value, TERM_TAB); > - if (!another || memcmp(another, name, len + 1)) > + if (!another || memcmp(another, orig_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 { > + } 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); Looks correct; back when 1e3f6b6e (git-apply: more consistency checks on gitdiff filenames, 2005-05-23) introduced this function, the variable "name" was in the outer scope, and a subsequent update narrowed its scope at ea56188a (Update git-apply to use C-style quoting for funny pathnames., 2005-10-16) but it could have removed the variable at the same time. ``` ## Junio C Hamano, 2016-03-22 21:25 Subject: Re: [PATCH 2/3] builtin/apply: make gitdiff_verify_name() return void Message-ID: URL: https://gitlist.dev/e/xmqq37rigq7o.fsf%40gitster.mtv.corp.google.com In-Reply-To: <1458680322-17681-3-git-send-email-chriscool@tuxfamily.org> ``` Christian Couder writes: > 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 > --- 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... > 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; ``` ## Junio C Hamano, 2016-03-22 21:35 Subject: Re: [PATCH 3/3] builtin/apply: simplify gitdiff_{old,new}name() Message-ID: URL: https://gitlist.dev/e/xmqqy49afb6k.fsf%40gitster.mtv.corp.google.com In-Reply-To: <1458680322-17681-4-git-send-email-chriscool@tuxfamily.org> ``` Christian Couder writes: > After the previous simplifications, it is easy to see that > there is no need to free the original string passed to > gitdiff_verify_name(), because this string can be changed > only when it is NULL. > > Signed-off-by: Christian Couder > --- I do not think you need either 1/3 or 2/3 to see that (I actually think it is easier to see this without 2/3). The caller passes patch->old_name and - if that is NULL, then we will either get NULL or a new string from find_name(); either way, we do not have to worry about calling free() on the original NULL; - if that is not NULL, then the only possible value returned from the function is itself (otherwise it will die()), so we won't be calling free() in this code. so I agree with the conclusion, i.e. the conditional free() can go from these places. > builtin/apply.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/builtin/apply.c b/builtin/apply.c > index 4cafdaf..9cfa9f4 100644 > --- a/builtin/apply.c > +++ b/builtin/apply.c > @@ -953,21 +953,15 @@ static void gitdiff_verify_name(const char *line, int isnull, char **name, int s > > static int gitdiff_oldname(const char *line, struct patch *patch) > { > - char *orig = patch->old_name; > gitdiff_verify_name(line, patch->is_new, &patch->old_name, > DIFF_OLD_NAME); > - if (orig != patch->old_name) > - free(orig); > return 0; > } > > static int gitdiff_newname(const char *line, struct patch *patch) > { > - char *orig = patch->new_name; > gitdiff_verify_name(line, patch->is_delete, &patch->new_name, > DIFF_NEW_NAME); > - if (orig != patch->new_name) > - free(orig); > return 0; > } ``` ## Christian Couder, 2016-03-22 22:10 Subject: Re: [PATCH 2/3] builtin/apply: make gitdiff_verify_name() return void Message-ID: URL: https://gitlist.dev/e/CAP8UFD13ueLG1xdV7QYpiZLx9DZHgdrPcpi9zLc7NAh2BLd08w%40mail.gmail.com In-Reply-To: ``` On Tue, Mar 22, 2016 at 10:25 PM, Junio C Hamano wrote: > Christian Couder writes: > >> 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 >> --- > > > 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... In my opinion it is an improvement because: - the feature of allowing a caller to pass one variable as orig and receive the result in another is useless here and just makes it more difficult to understand what the code is doing because you first have to realize that in both calls the argument that is passed is also assigned to, and - using up the return value to return a result prevents from using it to signal an error instead of die()ing, so it makes libifying the function more difficult. But if you prefer I can just resend a series with only patchs 1/3 and 3/3. And we can discuss later in the context of libifying "git apply" if this patch makes sense. ```