threads / patch / 41795

patchsha1_name.c: add an option to abort on ambiguous refs

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

## tl;dr

4 messages between Mar 23, 2016 and Jun 28, 2016. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Nguyễn Thái Ngọc Duy· Mar 23, 2016, 13:30 UTC · lore

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(-)
Show changes to 3 files +14 −4

Documentation/config.txt, config.c, sha1_name.c

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
Johannes Schindelin· Mar 23, 2016, 15:45 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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

Duy Nguyen· Mar 23, 2016, 23:11 UTC · re: Johannes Schindelin · lore

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

On Wed, Mar 23, 2016 at 10:45 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 17 quoted lines
> 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
Duy Nguyen· Jun 28, 2016, 17:55 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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:
Show 73 quoted lines
> 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

← back to recent threads