[PATCH] sha1_name.c: add an option to abort on ambiguous refs

[PATCH] sha1_name.c: add an option to abort on ambiguous refs

[Nguyễn Thái Ngọc Duy]

There are cases when a warning on ambiguous refs may go unnoticed
(e.g. git-log filling up the whole screen). There are also cases when
people want to catch ambiguity early (e.g. it happens deep in some
script). In either case, aborting the program would accomplish it.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/config.txt |  3 ++-
 config.c                 |  5 ++++-
 sha1_name.c              | 10 ++++++++--
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2cd6bdd..4172f59 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -522,7 +522,8 @@ core.sharedRepository::
 
 core.warnAmbiguousRefs::
 	If true, Git will warn you if the ref name you passed it is ambiguous
-	and might match multiple refs in the repository. True by default.
+	and might match multiple refs in the repository. If set to "fatal",
+	the program will abort on ambiguous refs. True by default.
 
 core.compression::
 	An integer -1..9, indicating a default compression level.
diff --git a/config.c b/config.c
index 9ba40bc..79f1ea5 100644
--- a/config.c
+++ b/config.c
@@ -738,7 +738,10 @@ static int git_default_core_config(const char *var, const char *value)
 	}
 
 	if (!strcmp(var, "core.warnambiguousrefs")) {
-		warn_ambiguous_refs = git_config_bool(var, value);
+		if (!strcasecmp(value, "fatal"))
+			warn_ambiguous_refs = 2;
+		else
+			warn_ambiguous_refs = git_config_bool(var, value);
 		return 0;
 	}
 
diff --git a/sha1_name.c b/sha1_name.c
index 3acf221..a0f0ab9 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -480,6 +480,8 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1,
 				warning(warn_msg, len, str);
 				if (advice_object_name_warning)
 					fprintf(stderr, "%s\n", _(object_name_msg));
+				if (warn_ambiguous_refs > 1)
+					die(_("cannot continue with ambiguous refs"));
 			}
 			free(real_ref);
 		}
@@ -537,8 +539,12 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1,
 
 	if (warn_ambiguous_refs && !(flags & GET_SHA1_QUIETLY) &&
 	    (refs_found > 1 ||
-	     !get_short_sha1(str, len, tmp_sha1, GET_SHA1_QUIETLY)))
-		warning(warn_msg, len, str);
+	     !get_short_sha1(str, len, tmp_sha1, GET_SHA1_QUIETLY))) {
+		if (warn_ambiguous_refs > 1)
+			die(warn_msg, len, str);
+		else
+			warning(warn_msg, len, str);
+	}
 
 	if (reflog_len) {
 		int nth, i;
-- 
2.8.0.rc0.210.gd302cd2

Re: [PATCH] sha1_name.c: add an option to abort on ambiguous refs

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 957 bytes --]

Hi Duy,

On Wed, 23 Mar 2016, Nguyễn Thái Ngọc Duy wrote:

> There are cases when a warning on ambiguous refs may go unnoticed
> (e.g. git-log filling up the whole screen). There are also cases when
> people want to catch ambiguity early (e.g. it happens deep in some
> script). In either case, aborting the program would accomplish it.

Whenever I see a die() in code outside of builtin/*.c, I cringe. I do that
because it was *exactly* something like that that caused a serious
regression in the builtin am so that we had to resort back to spawning
separate processes *just so* that we could catch error conditions and
run certain code in that case.

Maybe there would be a way to do what you want to do that does not fly in
the face of libification? Maybe some strbuf with an atexit() that
accumulates fatal errors that might be hidden and that are then written at
the end of the program (colorfully, if isatty(2))?

Ciao,
Dscho

Re: [PATCH] sha1_name.c: add an option to abort on ambiguous refs

[Duy Nguyen]

On Wed, Mar 23, 2016 at 10:45 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Duy,
>
> On Wed, 23 Mar 2016, Nguyễn Thái Ngọc Duy wrote:
>
>> There are cases when a warning on ambiguous refs may go unnoticed
>> (e.g. git-log filling up the whole screen). There are also cases when
>> people want to catch ambiguity early (e.g. it happens deep in some
>> script). In either case, aborting the program would accomplish it.
>
> Whenever I see a die() in code outside of builtin/*.c, I cringe. I do that
> because it was *exactly* something like that that caused a serious
> regression in the builtin am so that we had to resort back to spawning
> separate processes *just so* that we could catch error conditions and
> run certain code in that case.
>
> Maybe there would be a way to do what you want to do that does not fly in
> the face of libification?

Sorry I got nothing better.

> Maybe some strbuf with an atexit() that
> accumulates fatal errors that might be hidden and that are then written at
> the end of the program (colorfully, if isatty(2))?

That sounds hacky. If you don't want do die() deep inside then the
callers have to handle it, but get_sha1() is spread everywhere. Plus I
do not want to program to succeed when there is ambiguation because
the ref git chooses may not be what I want. I want it to abort (and
almost made a patch that die() unconditionally).
-- 
Duy

Re: [PATCH] sha1_name.c: add an option to abort on ambiguous refs

[Duy Nguyen]

Bringing this up again after I've seen another person accidentally
create a refs/heads/origin/some-branch and get really confused because
"git push" reports up-to-date but the remote branch is not updated. I
don't know how he got into that situation, but I hope we should be
able to catch it and suggest a way out.

On Wed, Mar 23, 2016 at 2:30 PM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> There are cases when a warning on ambiguous refs may go unnoticed
> (e.g. git-log filling up the whole screen). There are also cases when
> people want to catch ambiguity early (e.g. it happens deep in some
> script). In either case, aborting the program would accomplish it.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Documentation/config.txt |  3 ++-
>  config.c                 |  5 ++++-
>  sha1_name.c              | 10 ++++++++--
>  3 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 2cd6bdd..4172f59 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -522,7 +522,8 @@ core.sharedRepository::
>
>  core.warnAmbiguousRefs::
>         If true, Git will warn you if the ref name you passed it is ambiguous
> -       and might match multiple refs in the repository. True by default.
> +       and might match multiple refs in the repository. If set to "fatal",
> +       the program will abort on ambiguous refs. True by default.
>
>  core.compression::
>         An integer -1..9, indicating a default compression level.
> diff --git a/config.c b/config.c
> index 9ba40bc..79f1ea5 100644
> --- a/config.c
> +++ b/config.c
> @@ -738,7 +738,10 @@ static int git_default_core_config(const char *var, const char *value)
>         }
>
>         if (!strcmp(var, "core.warnambiguousrefs")) {
> -               warn_ambiguous_refs = git_config_bool(var, value);
> +               if (!strcasecmp(value, "fatal"))
> +                       warn_ambiguous_refs = 2;
> +               else
> +                       warn_ambiguous_refs = git_config_bool(var, value);
>                 return 0;
>         }
>
> diff --git a/sha1_name.c b/sha1_name.c
> index 3acf221..a0f0ab9 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -480,6 +480,8 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1,
>                                 warning(warn_msg, len, str);
>                                 if (advice_object_name_warning)
>                                         fprintf(stderr, "%s\n", _(object_name_msg));
> +                               if (warn_ambiguous_refs > 1)
> +                                       die(_("cannot continue with ambiguous refs"));
>                         }
>                         free(real_ref);
>                 }
> @@ -537,8 +539,12 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1,
>
>         if (warn_ambiguous_refs && !(flags & GET_SHA1_QUIETLY) &&
>             (refs_found > 1 ||
> -            !get_short_sha1(str, len, tmp_sha1, GET_SHA1_QUIETLY)))
> -               warning(warn_msg, len, str);
> +            !get_short_sha1(str, len, tmp_sha1, GET_SHA1_QUIETLY))) {
> +               if (warn_ambiguous_refs > 1)
> +                       die(warn_msg, len, str);
> +               else
> +                       warning(warn_msg, len, str);
> +       }
>
>         if (reflog_len) {
>                 int nth, i;
> --
> 2.8.0.rc0.210.gd302cd2
>



-- 
Duy