Re: [PATCH v4 3/3] ident.c: cleanup wrt ident's source
On Thu, Feb 04, 2016 at 02:42:55PM -0800, Junio C Hamano wrote:
> Dan Aloni writes:
>
> > +static int ident_source_is_sufficient(enum ident_source source)
> > +{
> > + switch (source) {
> > + case IDENT_SOURCE_CONFIG:
> > + case IDENT_SOURCE_ENVIRONMENT:
> > + return 1;
>
> Without adding these two lines here:
>
> default:
> break;
>
> I get this build failure:
>
> ident.c: In function 'ident_source_is_sufficient':
> ident.c:444:2: error: enumeration value 'IDENT_SOURCE_UNKNOWN' not handled in
> switch [-Werror=switch]
> switch (source) {
> ^
> ident.c:444:2: error: enumeration value 'IDENT_SOURCE_GUESSED' not handled in
> switch [-Werror=switch]
> ident.c:444:2: error: enumeration value 'IDENT_SOURCE_GUESSED_BOGUS' not
> handled in switch [-Werror=switch]
My fault - must have left over CFLAGS="-O0 -g" while debugging, so it
got missed. Will fix.
>[..]
> ident.c: In function 'ident_is_sufficient':
> ident.c:456:6: error: variable 'name' set but not used
> [-Werror=unused-but-set-variable]
> int name, email;
> ^
> cc1: all warnings being treated as errors
> make: *** [ident.o] Error 1
Ditto.
--
Dan Aloni
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/3] ident.c: cleanup wrt ident's source
Dan Aloni writes:
> +static int ident_source_is_sufficient(enum ident_source source)
> +{
> + switch (source) {
> + case IDENT_SOURCE_CONFIG:
> + case IDENT_SOURCE_ENVIRONMENT:
> + return 1;
Without adding these two lines here:
default:
break;
I get this build failure:
ident.c: In function 'ident_source_is_sufficient':
ident.c:444:2: error: enumeration value 'IDENT_SOURCE_UNKNOWN' not handled in
switch [-Werror=switch]
switch (source) {
^
ident.c:444:2: error: enumeration value 'IDENT_SOURCE_GUESSED' not handled in
switch [-Werror=switch]
ident.c:444:2: error: enumeration value 'IDENT_SOURCE_GUESSED_BOGUS' not
handled in switch [-Werror=switch]
> +static int ident_is_sufficient(enum ident_person person)
> {
> + const char *str_name, *str_email;
> + int name, email;
> +
> + switch (person) {
> + case IDENT_PERSON_COMMITTER:
> + str_name = getenv("GIT_COMMITTER_NAME");
> + str_email = getenv("GIT_COMMITTER_EMAIL");
> + break;
> + case IDENT_PERSON_AUTHOR:
> + str_name = getenv("GIT_AUTHOR_NAME");
> + str_email = getenv("GIT_AUTHOR_EMAIL");
> + break;
> + default:
> + die("invalid parameter to ident_is_sufficient()");
> + }
> +
> + name = !!str_name || ident_source_is_sufficient(source_of_default_name);
> + email = !!str_email ||
> ident_source_is_sufficient(source_of_default_email);
> +
> #ifndef WINDOWS
> - return (user_ident_explicitly_given & IDENT_MAIL_GIVEN);
> + return email;
> #else
> - return (user_ident_explicitly_given == IDENT_ALL_GIVEN);
> + return name && email;
> #endif
> }
It appears that str_name and name are unconditionally computed even
though it does not affect the outcome of the whole thing. When
building for !WINDOWS, I get
ident.c: In function 'ident_is_sufficient':
ident.c:456:6: error: variable 'name' set but not used
[-Werror=unused-but-set-variable]
int name, email;
^
cc1: all warnings being treated as errors
make: *** [ident.o] Error 1
as the result of this.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/3] ident.c: cleanup wrt ident's source
Dan Aloni writes:
> * Condense the variables that tells where we got the user's
>ident into single enum, instead of a collection of booleans.
> * Have {committer,author}_ident_sufficiently_given directly
>probe the environment and the afformentioned enum instead of
>relying on git_{committer,author}_info to do so.
That looks quite different from how we write our proposed log
messages.
>
> Signed-off-by: Dan Aloni
> ---
> ident.c | 122
>
> 1 file changed, 77 insertions(+), 45 deletions(-)
>
> diff --git a/ident.c b/ident.c
> index 1216079d0b0d..b9aad38e0621 100644
> --- a/ident.c
> +++ b/ident.c
> @@ -10,17 +10,19 @@
> static struct strbuf git_default_name = STRBUF_INIT;
> static struct strbuf git_default_email = STRBUF_INIT;
> static struct strbuf git_default_date = STRBUF_INIT;
> -static int default_email_is_bogus;
> -static int default_name_is_bogus;
> +
> +enum ident_source {
> + IDENT_SOURCE_UNKNOWN = 0,
> + IDENT_SOURCE_CONFIG,
> + IDENT_SOURCE_ENVIRONMENT,
> + IDENT_SOURCE_GUESSED,
> + IDENT_SOURCE_GUESSED_BOGUS,
> +};
No trailing comma after the last enum (some compliers choke on this
IIRC).
I skimmed the remainder of the patch but I am no the fence--I cannot
quite see how this improves the readability of the result.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 3/3] ident.c: cleanup wrt ident's source
* Condense the variables that tells where we got the user's
ident into single enum, instead of a collection of booleans.
* Have {committer,author}_ident_sufficiently_given directly
probe the environment and the afformentioned enum instead of
relying on git_{committer,author}_info to do so.
Signed-off-by: Dan Aloni
---
ident.c | 122
1 file changed, 77 insertions(+), 45 deletions(-)
diff --git a/ident.c b/ident.c
index 1216079d0b0d..b9aad38e0621 100644
--- a/ident.c
+++ b/ident.c
@@ -10,17 +10,19 @@
static struct strbuf git_default_name = STRBUF_INIT;
static struct strbuf git_default_email = STRBUF_INIT;
static struct strbuf git_default_date = STRBUF_INIT;
-static int default_email_is_bogus;
-static int default_name_is_bogus;
+
+enum ident_source {
+ IDENT_SOURCE_UNKNOWN = 0,
+ IDENT_SOURCE_CONFIG,
+ IDENT_SOURCE_ENVIRONMENT,
+ IDENT_SOURCE_GUESSED,
+ IDENT_SOURCE_GUESSED_BOGUS,
+};
static int ident_use_config_only;
-#define IDENT_NAME_GIVEN 01
-#define IDENT_MAIL_GIVEN 02
-#define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
-static int committer_ident_explicitly_given;
-static int author_ident_explicitly_given;
-static int ident_config_given;
+static enum ident_source source_of_default_email = IDENT_SOURCE_UNKNOWN;
+static enum ident_source source_of_default_name = IDENT_SOURCE_UNKNOWN;
#ifdef NO_GECOS_IN_PWENT
#define get_gecos(ignored) "&"
@@ -28,7 +30,7 @@ static int ident_config_given;
#define get_gecos(struct_passwd) ((struct_passwd)->pw_gecos)
#endif
-static struct passwd *xgetpwuid_self(int *is_bogus)
+static struct passwd *xgetpwuid_self(enum ident_source *source)
{
struct passwd *pw;
@@ -41,9 +43,13 @@ static struct passwd *xgetpwuid_self(int *is_bogus)
fallback.pw_gecos = "Unknown";
#endif
pw = &fallback;
- if (is_bogus)
- *is_bogus = 1;
+ if (source)
+ *source = IDENT_SOURCE_GUESSED_BOGUS;
+ } else {
+ if (source)
+ *source = IDENT_SOURCE_GUESSED;
}
+
return pw;
}
@@ -120,26 +126,26 @@ static int canonical_name(const char *host, struct strbuf
*out)
return status;
}
-static void add_domainname(struct strbuf *out, int *is_bogus)
+static void add_domainname(struct strbuf *out, enum ident_source *source)
{
char buf[1024];
if (gethostname(buf, sizeof(buf))) {
warning("cannot get host name: %s", strerror(errno));
strbuf_addstr(out, "(none)");
- *is_bogus = 1;
+ *source = IDENT_SOURCE_GUESSED_BOGUS;
return;
}
if (strchr(buf, '.'))
strbuf_addstr(out, buf);
else if (canonical_name(buf, out) < 0) {
strbuf_addf(out, "%s.(none)", buf);
- *is_bogus = 1;
+ *source = IDENT_SOURCE_GUESSED_BOGUS;
}
}
static void copy_email(const struct passwd *pw, struct strbuf *email,
- int *is_bogus)
+ enum ident_source *source)
{
/*
* Make up a fake email address
@@ -150,13 +156,13 @@ static void copy_email(const struct passwd *pw, struct
strbuf *email,
if (!add_mailname_host(email))
return; /* read from "/etc/mailname" (Debian) */
- add_domainname(email, is_bogus);
+ add_domainname(email, source);
}
const char *ident_default_name(void)
{
if (!git_default_name.len) {
- copy_gecos(xgetpwuid_self(&default_name_is_bogus),
&git_default_name);
+ copy_gecos(xgetpwuid_self(&source_of_default_name),
&git_default_name);
strbuf_trim(&git_default_name);
}
return git_default_name.buf;
@@ -169,11 +175,12 @@ const char *ident_default_email(void)
if (email && email[0]) {
strbuf_addstr(&git_default_email, email);
- committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
- author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
- } else
- copy_email(xgetpwuid_self(&default_email_is_bogus),
- &git_default_email, &default_email_is_bogus);
+ source_of_default_email = IDENT_SOURCE_ENVIRONMENT;
+ } else {
+ copy_email(xgetpwuid_self(&source_of_default_email),
+ &git_default_email,
&source_of_default_email);
+ }
+
strbuf_trim(&git_default_email);
}
return git_default_email.buf;
@@ -353,12 +360,13 @@ const char *fmt_ident(const char *name, const char *email,
if (!name) {
name = ident_default_name();
using_default = 1;
-
