[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH 02/12] Convert starts_with() to skip_prefix() for option parsing
From:       Johannes Sixt <j.sixt () viscovery ! net>
Date:       2013-12-20 6:51:00
Message-ID: 52B3E8D4.1030805 () viscovery ! net
[Download RAW message or body]

Am 12/18/2013 15:53, schrieb Nguyễn Thái Ngọc Duy:
> The code that's not converted to use parse_options() often does
> 
>   if (!starts_with(arg, "foo=")) {
>      value = atoi(arg + 4);
>   }
> 
> This patch removes those magic numbers with skip_prefix()
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/fetch-pack.c     | 13 +++++----
>  builtin/index-pack.c     | 17 +++++------
>  builtin/ls-remote.c      |  9 +++---
>  builtin/mailinfo.c       |  5 ++--
>  builtin/reflog.c         |  9 +++---
>  builtin/rev-parse.c      | 41 +++++++++++++-------------
>  builtin/send-pack.c      | 18 ++++++------
>  builtin/unpack-objects.c |  5 ++--
>  builtin/update-ref.c     | 21 +++++++-------
>  daemon.c                 | 75 ++++++++++++++++++++++++------------------------
>  diff.c                   | 49 +++++++++++++++----------------
>  git.c                    | 13 +++++----
>  merge-recursive.c        | 13 +++++----
>  revision.c               | 60 +++++++++++++++++++-------------------
>  upload-pack.c            |  5 ++--
>  15 files changed, 182 insertions(+), 171 deletions(-)
> 
> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index 8b8978a2..2df1423 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -47,13 +47,14 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
>  
>  	for (i = 1; i < argc && *argv[i] == '-'; i++) {
>  		const char *arg = argv[i];
> +		const char *optarg;
>  
> -		if (starts_with(arg, "--upload-pack=")) {
> -			args.uploadpack = arg + 14;
> +		if ((optarg = skip_prefix(arg, "--upload-pack=")) != NULL) {
> +			args.uploadpack = optarg;

Quite frankly, I do not think this is an improvement. The old code is
*MUCH* easier to understand because "starts_with" is clearly a predicate
that is either true or false, but the code with "skip_prefix" is much
heavier on the eye with its extra level of parenthesis. That it removes a
hard-coded constant does not count much IMHO because it is very clear
where the value comes from.

-- Hannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]