Re: [PATCH 2/5] check-ref-format: Refactor to make --branch code more common

[Ian Jackson]

Michael Haggerty writes ("Re: [PATCH 2/5] check-ref-format: Refactor to make 
--branch code more common"):
> On 11/04/2016 08:13 PM, Ian Jackson wrote:
> >  static int normalize = 0;
> > +static int check_branch = 0;
> >  static int flags = 0;
> >  
> >  static int check_one_ref_format(const char *refname)
> >  {
> > +   int got;
> 
> `got` is an unusual name for this variable, and I don't really
> understand what the word means in this context. Is there a reason not to
> use the more usual `err`?

I have no real opinion about the name of this variable.  `err' is a
fine name too.

> > +   if (check_branch && (flags || normalize))
> 
> Is there a reason not to allow `--normalize` with `--branch`?
> (Currently, `git check-ref-format --branch` *does* allow input like
> `refs/heads/foo`.)

It was like that when I found it :-).  I wasn't sure why this
restriction was there so I left it alone.

Looking at it again: AFAICT from the documentation --branch is a
completely different mode.  The effect of --normalize is not to do
additional work, but simply to produce additional output.  It really
means --print-normalized.  --branch already prints output, but AFAICT
it does not collapse slashes.  This seems like a confusing collection
of options.  But, sorting that out is beyond the scope of what I was
trying to do.

In my series I have at least managed not to make any of this any
worse, I think: the --stdin option I introduce applies to both modes
equally, and doesn't make future improvements to the conflict between
--branch and --normalize any harder.

(In _this_ patch, certainly, allowing --normalize with --branch would
be wrong, since _this_ patch is just refactoring.)

Thanks,
Ian.

-- 
Ian Jackson <ijack...@chiark.greenend.org.uk>   These opinions are my own.

If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
a private address which bypasses my fierce spamfilter.

Re: [PATCH 2/5] check-ref-format: Refactor to make --branch code more common

[Michael Haggerty]

On 11/04/2016 08:13 PM, Ian Jackson wrote:
> We are going to want to permit other options with --branch.
> 
> So, replace the special case with just an entry for --branch in the
> parser for ordinary options, and check for option compatibility at the
> end.
> 
> No overall functional change.
> 
> Signed-off-by: Ian Jackson 
> ---
>  builtin/check-ref-format.c | 17 +
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
> index 4d56caa..f12c19c 100644
> --- a/builtin/check-ref-format.c
> +++ b/builtin/check-ref-format.c
> @@ -49,13 +49,19 @@ static int check_ref_format_branch(const char *arg)
>  }
>  
>  static int normalize = 0;
> +static int check_branch = 0;
>  static int flags = 0;
>  
>  static int check_one_ref_format(const char *refname)
>  {
> + int got;

`got` is an unusual name for this variable, and I don't really
understand what the word means in this context. Is there a reason not to
use the more usual `err`?

> +
>   if (normalize)
>   refname = collapse_slashes(refname);
> - if (check_refname_format(refname, flags))
> + got = check_branch
> + ? check_ref_format_branch(refname)
> + : check_refname_format(refname, flags);
> + if (got)
>   return 1;
>   if (normalize)
>   printf("%s\n", refname);
> @@ -68,9 +74,6 @@ int cmd_check_ref_format(int argc, const char **argv, const 
> char *prefix)
>   if (argc == 2 && !strcmp(argv[1], "-h"))
>   usage(builtin_check_ref_format_usage);
>  
> - if (argc == 3 && !strcmp(argv[1], "--branch"))
> - return check_ref_format_branch(argv[2]);
> -
>   for (i = 1; i < argc && argv[i][0] == '-'; i++) {
>   if (!strcmp(argv[i], "--normalize") || !strcmp(argv[i], 
> "--print"))
>   normalize = 1;
> @@ -80,9 +83,15 @@ int cmd_check_ref_format(int argc, const char **argv, 
> const char *prefix)
>   flags &= ~REFNAME_ALLOW_ONELEVEL;
>   else if (!strcmp(argv[i], "--refspec-pattern"))
>   flags |= REFNAME_REFSPEC_PATTERN;
> + else if (!strcmp(argv[i], "--branch"))
> + check_branch = 1;
>   else
>   usage(builtin_check_ref_format_usage);
>   }
> +
> + if (check_branch && (flags || normalize))

Is there a reason not to allow `--normalize` with `--branch`?
(Currently, `git check-ref-format --branch` *does* allow input like
`refs/heads/foo`.)

But note that simply allowing `--branch --normalize` without changing
`check_one_ref_format()` would mean generating *two* lines of output per
reference, so something else would have to change, too.

> + usage(builtin_check_ref_format_usage);
> +
>   if (! (i == argc - 1))
>   usage(builtin_check_ref_format_usage);
>  
> 

Michael

[PATCH 2/5] check-ref-format: Refactor to make --branch code more common

[Ian Jackson]

We are going to want to permit other options with --branch.

So, replace the special case with just an entry for --branch in the
parser for ordinary options, and check for option compatibility at the
end.

No overall functional change.

Signed-off-by: Ian Jackson 
---
 builtin/check-ref-format.c | 17 +
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index 4d56caa..f12c19c 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -49,13 +49,19 @@ static int check_ref_format_branch(const char *arg)
 }
 
 static int normalize = 0;
+static int check_branch = 0;
 static int flags = 0;
 
 static int check_one_ref_format(const char *refname)
 {
+   int got;
+
if (normalize)
refname = collapse_slashes(refname);
-   if (check_refname_format(refname, flags))
+   got = check_branch
+   ? check_ref_format_branch(refname)
+   : check_refname_format(refname, flags);
+   if (got)
return 1;
if (normalize)
printf("%s\n", refname);
@@ -68,9 +74,6 @@ int cmd_check_ref_format(int argc, const char **argv, const 
char *prefix)
if (argc == 2 && !strcmp(argv[1], "-h"))
usage(builtin_check_ref_format_usage);
 
-   if (argc == 3 && !strcmp(argv[1], "--branch"))
-   return check_ref_format_branch(argv[2]);
-
for (i = 1; i < argc && argv[i][0] == '-'; i++) {
if (!strcmp(argv[i], "--normalize") || !strcmp(argv[i], 
"--print"))
normalize = 1;
@@ -80,9 +83,15 @@ int cmd_check_ref_format(int argc, const char **argv, const 
char *prefix)
flags &= ~REFNAME_ALLOW_ONELEVEL;
else if (!strcmp(argv[i], "--refspec-pattern"))
flags |= REFNAME_REFSPEC_PATTERN;
+   else if (!strcmp(argv[i], "--branch"))
+   check_branch = 1;
else
usage(builtin_check_ref_format_usage);
}
+
+   if (check_branch && (flags || normalize))
+   usage(builtin_check_ref_format_usage);
+
if (! (i == argc - 1))
usage(builtin_check_ref_format_usage);
 
-- 
2.10.1