date:20160204

[PATCH v6 2/3] ident: add user.useConfigOnly boolean for when ident shouldn't be guessed

2016-02-04 Thread Dan Aloni

It used to be that:

   git config --global user.email "(none)"

was a viable way for people to force themselves to set user.email in
each repository.  This was helpful for people with more than one
email address, targeting different email addresses for different
clones, as it barred git from creating commit unless the user.email
config was set in the per-repo config to the correct email address.

A recent change, 19ce497c (ident: keep a flag for bogus
default_email, 2015-12-10), however declared that an explicitly
configured user.email is not bogus, no matter what its value is, so
this hack no longer works.

Provide the same functionality by adding a new configuration
variable user.useConfigOnly; when this variable is set, the
user must explicitly set user.email configuration.

Signed-off-by: Dan Aloni 
Helped-by: Jeff King 
Signed-off-by: Junio C Hamano 
Cc: Eric Sunshine 
---
 Documentation/config.txt  |  9 
 ident.c   | 16 +
 t/t9904-per-repo-email.sh | 58 +++
 3 files changed, 83 insertions(+)
 create mode 100755 t/t9904-per-repo-email.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 02bcde6bb596..25cf7ce4e83a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2821,6 +2821,15 @@ user.name::
Can be overridden by the 'GIT_AUTHOR_NAME' and 'GIT_COMMITTER_NAME'
environment variables.  See linkgit:git-commit-tree[1].
 
+user.useConfigOnly::
+   This instruct Git to avoid trying to guess defaults for 'user.email'
+   and 'user.name' other than strictly from environment or config.
+   If you have multiple email addresses that you would like to set
+   up per repository, you may want to set this to 'true' in the global
+   config, and then Git would prompt you to set user.email separately,
+   in each of the cloned repositories.
+   Defaults to `false`.
+
 user.signingKey::
If linkgit:git-tag[1] or linkgit:git-commit[1] is not selecting the
key you want it to automatically when creating a signed tag or
diff --git a/ident.c b/ident.c
index f3a431f738cc..9bd6ac69bfe8 100644
--- a/ident.c
+++ b/ident.c
@@ -13,11 +13,14 @@ static struct strbuf git_default_date = STRBUF_INIT;
 static int default_email_is_bogus;
 static int default_name_is_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;
 
 #ifdef NO_GECOS_IN_PWENT
 #define get_gecos(ignored) "&"
@@ -354,6 +357,9 @@ const char *fmt_ident(const char *name, const char *email,
fputs(env_hint, stderr);
die("unable to auto-detect name (got '%s')", 
name);
}
+   if (strict && ident_use_config_only &&
+   !(ident_config_given & IDENT_NAME_GIVEN))
+   die("user.useConfigOnly set but no name given");
}
if (!*name) {
struct passwd *pw;
@@ -373,6 +379,9 @@ const char *fmt_ident(const char *name, const char *email,
fputs(env_hint, stderr);
die("unable to auto-detect email address (got '%s')", 
email);
}
+   if (strict && ident_use_config_only
+   && !(ident_config_given & IDENT_MAIL_GIVEN))
+   die("user.useConfigOnly set but no mail given");
}
 
strbuf_reset(&ident);
@@ -446,6 +455,11 @@ int author_ident_sufficiently_given(void)
 
 int git_ident_config(const char *var, const char *value, void *data)
 {
+   if (!strcmp(var, "user.useconfigonly")) {
+   ident_use_config_only = git_config_bool(var, value);
+   return 0;
+   }
+
if (!strcmp(var, "user.name")) {
if (!value)
return config_error_nonbool(var);
@@ -453,6 +467,7 @@ int git_ident_config(const char *var, const char *value, 
void *data)
strbuf_addstr(&git_default_name, value);
committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
author_ident_explicitly_given |= IDENT_NAME_GIVEN;
+   ident_config_given |= IDENT_NAME_GIVEN;
return 0;
}
 
@@ -463,6 +478,7 @@ int git_ident_config(const char *var, const char *value, 
void *data)
strbuf_addstr(&git_default_email, value);
committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+   ident_config_given |= IDENT_MAIL_GIVEN;
return 0;
}
 
diff --git a/t/t9904-per-repo-email.sh b/t/t9904-per-repo-email.sh
new file mode 100755
index 00

[PATCH v6 1/3] fmt_ident: refactor strictness checks

2016-02-04 Thread Dan Aloni

From: Jeff King 

This function has evolved quite a bit over time, and as a
result, the logic for "is this an OK ident" has been
sprinkled throughout. This ends up with a lot of redundant
conditionals, like checking want_name repeatedly. Worse,
we want to know in many cases whether we are using the
"default" ident, and we do so by comparing directly to the
global strbuf, which violates the abstraction of the
ident_default_* functions.

Let's reorganize the function into a hierarchy of
conditionals to handle similar cases together. The only
case that doesn't just work naturally for this is that of an
empty name, where our advice is different based on whether
we came from ident_default_name() or not. We can use a
simple flag to cover this case.

Signed-off-by: Jeff King 
---
 ident.c | 46 --
 1 file changed, 24 insertions(+), 22 deletions(-)

diff --git a/ident.c b/ident.c
index 3da555634290..f3a431f738cc 100644
--- a/ident.c
+++ b/ident.c
@@ -345,32 +345,34 @@ const char *fmt_ident(const char *name, const char *email,
int want_date = !(flag & IDENT_NO_DATE);
int want_name = !(flag & IDENT_NO_NAME);
 
-   if (want_name && !name)
-   name = ident_default_name();
-   if (!email)
-   email = ident_default_email();
-
-   if (want_name && !*name) {
-   struct passwd *pw;
-
-   if (strict) {
-   if (name == git_default_name.buf)
+   if (want_name) {
+   int using_default = 0;
+   if (!name) {
+   name = ident_default_name();
+   using_default = 1;
+   if (strict && default_name_is_bogus) {
fputs(env_hint, stderr);
-   die("empty ident name (for <%s>) not allowed", email);
+   die("unable to auto-detect name (got '%s')", 
name);
+   }
+   }
+   if (!*name) {
+   struct passwd *pw;
+   if (strict) {
+   if (using_default)
+   fputs(env_hint, stderr);
+   die("empty ident name (for <%s>) not allowed", 
email);
+   }
+   pw = xgetpwuid_self(NULL);
+   name = pw->pw_name;
}
-   pw = xgetpwuid_self(NULL);
-   name = pw->pw_name;
-   }
-
-   if (want_name && strict &&
-   name == git_default_name.buf && default_name_is_bogus) {
-   fputs(env_hint, stderr);
-   die("unable to auto-detect name (got '%s')", name);
}
 
-   if (strict && email == git_default_email.buf && default_email_is_bogus) 
{
-   fputs(env_hint, stderr);
-   die("unable to auto-detect email address (got '%s')", email);
+   if (!email) {
+   email = ident_default_email();
+   if (strict && default_email_is_bogus) {
+   fputs(env_hint, stderr);
+   die("unable to auto-detect email address (got '%s')", 
email);
+   }
}
 
strbuf_reset(&ident);
-- 
2.5.0

--
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 v6 3/3] ident: cleanup wrt ident's source

2016-02-04 Thread Dan Aloni

This change condenses the variables that tells where we got the user's
ident into single enum, instead of a collection of booleans.

In addtion, also 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 
Helped-by: Jeff King 
Helped-by: Junio C Hamano 
---
 ident.c | 126 
 1 file changed, 80 insertions(+), 46 deletions(-)

diff --git a/ident.c b/ident.c
index 9bd6ac69bfe8..9bb05912b59a 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_

[PATCH v6] ident: add user.useConfigOnly boolean for when ident shouldn't be guessed

2016-02-04 Thread Dan Aloni


Change between v5 -> v6:

* Removed trailing comma in 'enum ident_person'.

v5: http://article.gmane.org/gmane.comp.version-control.git/285544
--
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

Best regards! Dear Sir or Madam! - please help our school site - пожалуйста, помогите нашему школьному сайту - будь ласка, допоможіть нашому шкільному сайту http://bilokaminsky-nvk.pp.ua/

2016-02-04 Thread admin

Best regards! Dear Sir or Madam! 
please at the top of any page of site click once on the advertising banner,
so that we could pay for hosting our school site,
Thank you

ad...@bilokaminsky-nvk.pp.ua
http://bilokaminsky-nvk.pp.ua/

добрий день,
просимо на будь-якій сторінці вгорі один раз натиснути на рекламний банер,
щоб ми змогли оплатити хостинг нашого шкільного сайту,
Дякуємо

добрый день, 
просим на любой странице вверху один раз нажать на рекламный баннер,
чтобы мы смогли оплатить хостинг нашего школьного сайта,
спасибо

Sorry if this letter has caused you inconvenience. Your address is taken from 
public sources.
To unsubscribe, please send us a a message to our email address.

ad...@bilokaminsky-nvk.pp.ua
http://bilokaminsky-nvk.pp.ua/
--
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 v5 3/3] ident: cleanup wrt ident's source

2016-02-04 Thread Dan Aloni

This change condenses the variables that tells where we got the user's
ident into single enum, instead of a collection of booleans.

In addtion, also 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 
Helped-by: Jeff King 
Helped-by: Junio C Hamano 
---
 ident.c | 126 
 1 file changed, 80 insertions(+), 46 deletions(-)

diff --git a/ident.c b/ident.c
index 9bd6ac69bfe8..725d0aeb7da4 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_

[PATCH v5 2/3] ident: add user.useConfigOnly boolean for when ident shouldn't be guessed

2016-02-04 Thread Dan Aloni

It used to be that:

   git config --global user.email "(none)"

was a viable way for people to force themselves to set user.email in
each repository.  This was helpful for people with more than one
email address, targeting different email addresses for different
clones, as it barred git from creating commit unless the user.email
config was set in the per-repo config to the correct email address.

A recent change, 19ce497c (ident: keep a flag for bogus
default_email, 2015-12-10), however declared that an explicitly
configured user.email is not bogus, no matter what its value is, so
this hack no longer works.

Provide the same functionality by adding a new configuration
variable user.useConfigOnly; when this variable is set, the
user must explicitly set user.email configuration.

Signed-off-by: Dan Aloni 
Helped-by: Jeff King 
Signed-off-by: Junio C Hamano 
Cc: Eric Sunshine 
---
 Documentation/config.txt  |  9 
 ident.c   | 16 +
 t/t9904-per-repo-email.sh | 58 +++
 3 files changed, 83 insertions(+)
 create mode 100755 t/t9904-per-repo-email.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 02bcde6bb596..25cf7ce4e83a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2821,6 +2821,15 @@ user.name::
Can be overridden by the 'GIT_AUTHOR_NAME' and 'GIT_COMMITTER_NAME'
environment variables.  See linkgit:git-commit-tree[1].
 
+user.useConfigOnly::
+   This instruct Git to avoid trying to guess defaults for 'user.email'
+   and 'user.name' other than strictly from environment or config.
+   If you have multiple email addresses that you would like to set
+   up per repository, you may want to set this to 'true' in the global
+   config, and then Git would prompt you to set user.email separately,
+   in each of the cloned repositories.
+   Defaults to `false`.
+
 user.signingKey::
If linkgit:git-tag[1] or linkgit:git-commit[1] is not selecting the
key you want it to automatically when creating a signed tag or
diff --git a/ident.c b/ident.c
index f3a431f738cc..9bd6ac69bfe8 100644
--- a/ident.c
+++ b/ident.c
@@ -13,11 +13,14 @@ static struct strbuf git_default_date = STRBUF_INIT;
 static int default_email_is_bogus;
 static int default_name_is_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;
 
 #ifdef NO_GECOS_IN_PWENT
 #define get_gecos(ignored) "&"
@@ -354,6 +357,9 @@ const char *fmt_ident(const char *name, const char *email,
fputs(env_hint, stderr);
die("unable to auto-detect name (got '%s')", 
name);
}
+   if (strict && ident_use_config_only &&
+   !(ident_config_given & IDENT_NAME_GIVEN))
+   die("user.useConfigOnly set but no name given");
}
if (!*name) {
struct passwd *pw;
@@ -373,6 +379,9 @@ const char *fmt_ident(const char *name, const char *email,
fputs(env_hint, stderr);
die("unable to auto-detect email address (got '%s')", 
email);
}
+   if (strict && ident_use_config_only
+   && !(ident_config_given & IDENT_MAIL_GIVEN))
+   die("user.useConfigOnly set but no mail given");
}
 
strbuf_reset(&ident);
@@ -446,6 +455,11 @@ int author_ident_sufficiently_given(void)
 
 int git_ident_config(const char *var, const char *value, void *data)
 {
+   if (!strcmp(var, "user.useconfigonly")) {
+   ident_use_config_only = git_config_bool(var, value);
+   return 0;
+   }
+
if (!strcmp(var, "user.name")) {
if (!value)
return config_error_nonbool(var);
@@ -453,6 +467,7 @@ int git_ident_config(const char *var, const char *value, 
void *data)
strbuf_addstr(&git_default_name, value);
committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
author_ident_explicitly_given |= IDENT_NAME_GIVEN;
+   ident_config_given |= IDENT_NAME_GIVEN;
return 0;
}
 
@@ -463,6 +478,7 @@ int git_ident_config(const char *var, const char *value, 
void *data)
strbuf_addstr(&git_default_email, value);
committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+   ident_config_given |= IDENT_MAIL_GIVEN;
return 0;
}
 
diff --git a/t/t9904-per-repo-email.sh b/t/t9904-per-repo-email.sh
new file mode 100755
index 00

[PATCH v5] ident: add user.useConfigOnly boolean for when ident shouldn't be guessed

2016-02-04 Thread Dan Aloni


Changes between v4 -> v5:

 * Fixes for compiler warnings under standard compilation.
 * Revised commit messages according to remarks.
 * Cleanups and fixes of the added test script.
 * Indentation and styling fixes.

v4: http://thread.gmane.org/gmane.comp.version-control.git/285441
--
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 v5 1/3] fmt_ident: refactor strictness checks

2016-02-04 Thread Dan Aloni

From: Jeff King 

This function has evolved quite a bit over time, and as a
result, the logic for "is this an OK ident" has been
sprinkled throughout. This ends up with a lot of redundant
conditionals, like checking want_name repeatedly. Worse,
we want to know in many cases whether we are using the
"default" ident, and we do so by comparing directly to the
global strbuf, which violates the abstraction of the
ident_default_* functions.

Let's reorganize the function into a hierarchy of
conditionals to handle similar cases together. The only
case that doesn't just work naturally for this is that of an
empty name, where our advice is different based on whether
we came from ident_default_name() or not. We can use a
simple flag to cover this case.

Signed-off-by: Jeff King 
---
 ident.c | 46 --
 1 file changed, 24 insertions(+), 22 deletions(-)

diff --git a/ident.c b/ident.c
index 3da555634290..f3a431f738cc 100644
--- a/ident.c
+++ b/ident.c
@@ -345,32 +345,34 @@ const char *fmt_ident(const char *name, const char *email,
int want_date = !(flag & IDENT_NO_DATE);
int want_name = !(flag & IDENT_NO_NAME);
 
-   if (want_name && !name)
-   name = ident_default_name();
-   if (!email)
-   email = ident_default_email();
-
-   if (want_name && !*name) {
-   struct passwd *pw;
-
-   if (strict) {
-   if (name == git_default_name.buf)
+   if (want_name) {
+   int using_default = 0;
+   if (!name) {
+   name = ident_default_name();
+   using_default = 1;
+   if (strict && default_name_is_bogus) {
fputs(env_hint, stderr);
-   die("empty ident name (for <%s>) not allowed", email);
+   die("unable to auto-detect name (got '%s')", 
name);
+   }
+   }
+   if (!*name) {
+   struct passwd *pw;
+   if (strict) {
+   if (using_default)
+   fputs(env_hint, stderr);
+   die("empty ident name (for <%s>) not allowed", 
email);
+   }
+   pw = xgetpwuid_self(NULL);
+   name = pw->pw_name;
}
-   pw = xgetpwuid_self(NULL);
-   name = pw->pw_name;
-   }
-
-   if (want_name && strict &&
-   name == git_default_name.buf && default_name_is_bogus) {
-   fputs(env_hint, stderr);
-   die("unable to auto-detect name (got '%s')", name);
}
 
-   if (strict && email == git_default_email.buf && default_email_is_bogus) 
{
-   fputs(env_hint, stderr);
-   die("unable to auto-detect email address (got '%s')", email);
+   if (!email) {
+   email = ident_default_email();
+   if (strict && default_email_is_bogus) {
+   fputs(env_hint, stderr);
+   die("unable to auto-detect email address (got '%s')", 
email);
+   }
}
 
strbuf_reset(&ident);
-- 
2.5.0

--
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 2/3] Add user.useConfigOnly boolean for when ident shouldn't be guessed

2016-02-04 Thread Dan Aloni

On Thu, Feb 04, 2016 at 01:53:25PM -0800, Junio C Hamano wrote:
>[..]
> "by adding a new configuration variable" is a bit weak.  Help the
> reader by mentioning what it is called and what it does in the same
> sentence.
> 
> Perhaps like this?
> 
> -- >8 --
>[..]
>

Looks good, I'll just take that :)

> ident: add user.useConfigOnly boolean for when ident shouldn't be guessed
>[..]
> > }
> > +   if (strict && ident_use_config_only && !(ident_config_given & 
> > IDENT_MAIL_GIVEN))
> > +   die("user.useConfigOnly set but no mail given");
> > }
> 
> By folding the line just like you did for "name" above, you do not
> have to worry about an overlong line here.

Consistency is important. Will fix here too, though it got fixed later
in the cleanup.

>[..]
> > +git add foo &&
> > +EDITOR=: VISUAL=: git commit -m foo &&
> 
> What is the point of these one-shot assignments to the environment
> variables?
> 
> "git commit -m " does not invoke the editor unless given "-e",
> and EDITOR=: is done early in test-lib.sh already, so I am puzzled.
> 
> Besides, if you are worried about some stray environment variable,
> overriding EDITOR and VISUAL would not guard you against a stray
> GIT_EDITOR, which takes the precedence, I think.

Being new to this testing framework, I tried learning the trade from
other tests. Maybe I goofed, or the other tests need cleaning?

> 
> > +   # Setup a likely user.useConfigOnly use case
> > +   unset GIT_AUTHOR_NAME &&
> > +   unset GIT_AUTHOR_EMAIL &&
> 
> Doesn't unset fail when the variable is not set (we have sane_unset
> helper for that)?

Sure.

>[..] 
> > +test_expect_success 'fails committing if clone email is not set, but EMAIL 
> > set' '
> > +prepare && about_to_commit &&
> > +
> > +   EMAIL=t...@fail.com EDITOR=: VISUAL=: test_must_fail git commit -m msg
> 
> This is a good place to use the "test_must_fail env" pattern, i.e.
> 
>   test_must_fail env EMAIL=t...@fail.com git commit -m msg
> 
> I would think.

Yes, and the fixed test still passes. Will resubmit the patches.

-- 
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

2016-02-04 Thread Dan Aloni

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 v2 24/25] upload-pack: make check_reachable_object() return unreachable list if asked

2016-02-04 Thread Eric Sunshine

On Thu, Feb 4, 2016 at 4:04 AM, Nguyễn Thái Ngọc Duy  wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/upload-pack.c b/upload-pack.c
> @@ -451,8 +451,16 @@ static int is_our_ref(struct object *o)
> -static int check_unreachable(struct object_array *src)
> +/*
> + * If reachable is NULL, return 1 if there is no unreachable object,
> + * zero otherwise.
> + *
> + * If reachable is not NULL, it's filled with reachable objects.
> + * Return value is irrelevant. The caller has to compare reachable and
> + * src to find out if there's any unreachable object.
> + */

This dual-purpose function serving up two entirely orthogonal modes
feel strange. Would it make sense to split this  into two functions,
one for each paragraph in the above documentation? (Of course, they
could share common implementation behind-the-scenes as needed.)

More below...

> +static int check_unreachable(struct object_array *reachable,
> +struct object_array *src)
>  {
> static const char *argv[] = {
> "rev-list", "--stdin", NULL,
> @@ -507,9 +522,31 @@ static int check_unreachable(struct object_array *src)
>  * The commits out of the rev-list are not ancestors of
>  * our ref.
>  */
> -   i = read_in_full(cmd.out, namebuf, 1);
> -   if (i)
> -   return 0;
> +   if (!reachable) {
> +   i = read_in_full(cmd.out, namebuf, 1);
> +   if (i)
> +   return 0;
> +   } else {
> +   while ((i = read_in_full(cmd.out, namebuf, 41)) == 41) {
> +   struct object_id sha1;
> +
> +   if (namebuf[40] != '\n' || get_oid_hex(namebuf, 
> &sha1))
> +   break;
> +
> +   o = lookup_object(sha1.hash);
> +   if (o && o->type == OBJ_COMMIT) {
> +   o->flags &= ~TMP_MARK;
> +   }
> +   }
> +   for (i = get_max_object_index(); 0 < i; ) {
> +   o = get_indexed_object(--i);

Perhaps code this as:

for (i = get_max_object_index(); 0 < i; i--) {
o = get_indexed_object(i - 1);

in order to keep the loop control within the 'for' itself?

> +   if (o && o->type == OBJ_COMMIT &&
> +   (o->flags & TMP_MARK)) {
> +   add_object_array(o, NULL, reachable);
> +   o->flags &= ~TMP_MARK;
> +   }
> +   }
> +   }
--
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 v2 21/25] fetch: define shallow boundary with --shallow-exclude

2016-02-04 Thread Eric Sunshine

On Thu, Feb 4, 2016 at 4:03 AM, Nguyễn Thái Ngọc Duy  wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> @@ -109,6 +109,16 @@ int cmd_fetch_pack(int argc, const char **argv, const 
> char *prefix)
> +   if (skip_prefix(arg, "--shallow-exclude=", &value)) {
> +   static struct string_list *deepen_not;
> +   if (!deepen_not) {
> +   deepen_not = xmalloc(sizeof(*deepen_not));
> +   string_list_init(deepen_not, 1);
> +   args.deepen_not = deepen_not;
> +   }
> +   string_list_append(deepen_not, value);
> +   continue;
> +   }

Hmm, can't this be simplified to:

if (skip_prefix(arg, "--shallow-exclude=", &value)) {
if (!args.deepen_not) {
args.deepen_not = xmalloc(sizeof(*args.deepen_not));
string_list_init(args.deepen_not, 1);
}
string_list_append(args.deepen_not, value);
continue;
}

Or, perhaps even better, declare it as plain 'struct string_list
deepen_not' in struct fetch_pack_args, rather than as a pointer, and
then in cmd_fetch_pack():

memset(&args, 0, sizeof(args));
args.uploadpack = "git-upload-pack";
string_list_init(&args.deepen_not, 1);

...

if (skip_prefix(arg, "--shallow-exclude=", &value)) {
string_list_append(args.deepen_not, value);
continue;
}
--
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 v2 20/25] upload-pack: support define shallow boundary by excluding revisions

2016-02-04 Thread Eric Sunshine

On Thu, Feb 4, 2016 at 4:03 AM, Nguyễn Thái Ngọc Duy  wrote:
> @@ -732,7 +743,7 @@ static void receive_needs(void)
> if (depth == 0 && !deepen_rev_list && shallows.nr == 0)
> return;
> if (depth > 0 && deepen_rev_list)
> -   die("--depth and --shallow-since cannot be used together");
> +   die("--depth and --shallow-since (or --shallow-exclude) 
> cannot be used together");

Also, does this change belong in patch 21/25?

> if (depth > 0)
> deepen(depth, &shallows);
> else if (deepen_rev_list) {
--
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 v2 20/25] upload-pack: support define shallow boundary by excluding revisions

2016-02-04 Thread Eric Sunshine

On Thu, Feb 4, 2016 at 4:03 AM, Nguyễn Thái Ngọc Duy  wrote:
> This should allow the user to say "create a shallow clone of this branch
> after version ".
>
> deepen-not cannot be used with deepen the same way deepen-since cannot
> be used with deepen.

As written, this is a bit of a brain twister and required several
re-reads to digest. Perhaps it might be clearer if rephrased like
this:

Like deepen-since, deepen-not cannot be used with deepen.

or:

Like deepen-since, deepen-not is incompatible with deepen.

> But deepen-not can be mixed with deepen-since. The
> result is exactly how you do the command "git rev-list --since=... --not
> ref".
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/Documentation/technical/protocol-capabilities.txt 
> b/Documentation/technical/protocol-capabilities.txt
> @@ -188,6 +188,15 @@ specific time, instead of depth. Internally it's 
> equivalent of doing
> +deepen-not
> +--
> +
> +This capability adds "deepen-not" command to fetch-pack/upload-pack
> +protocol so the client can request shallow clones that are cut at a
> +specific revision, instead of depth. Internally it's equivalent of
> +doing "rev-list --not " on the server side. "deepen-not"
> +cannot be used with "deepen", but can be used with "deepen-since".

This description, on the other hand, is easy to grasp.

> diff --git a/upload-pack.c b/upload-pack.c
> @@ -678,6 +679,16 @@ static void receive_needs(void)
> +   if (skip_prefix(line, "deepen-not ", &arg)) {
> +   char *ref = NULL;
> +   unsigned char sha1[20];
> +   if (expand_ref(arg, strlen(arg), sha1, &ref) != 1)
> +   die("Ambiguous deepen-not: %s", line);

With just a few exceptions, die() invocations in upload-pack.c prefix
the message with "git upload-pack:" and start the actual diagnostic
with lowercase, so perhaps:

die("git upload-pack: ambiguous deepen-not: %s", line);

> +   string_list_append(&deepen_not, ref);
> +   free(ref);
> +   deepen_rev_list = 1;
> +   continue;
> +   }
> @@ -746,6 +757,13 @@ static void receive_needs(void)
> struct object *o = want_obj.objects[i].item;
> argv_array_push(&av, oid_to_hex(&o->oid));
> }
> +   if (deepen_not.nr) {
> +   argv_array_push(&av, "--not");
> +   for (i = 0; i < deepen_not.nr; i++) {
> +   struct string_list_item *s = deepen_not.items 
> + i;
> +   argv_array_push(&av, s->string);
> +   }

The documentation for rev-list --not says it "Reverses the meaning ...
up to the next --not", so would it make sense to add a final:

argv_array_push(&av, "--not");

here to future-proof against some change pushing more arguments onto
'av' following this code?

> +   }
> deepen_by_rev_list(av.argc, av.argv, &shallows);
> argv_array_clear(&av);
> }
--
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 v2 13/25] fetch-pack: use a separate flag for fetch in deepening mode

2016-02-04 Thread Eric Sunshine

On Thu, Feb 4, 2016 at 4:03 AM, Nguyễn Thái Ngọc Duy  wrote:
> The shallow repo could be deepened or shortened when then user gives

s/then/the/

> --depth. But in future that won't be the only way to deepen/shorten a
> repo. Stop relying on args->depth in this mode. Future deepening
> methods can simply set this flag on instead of updating all these if
> expressions.
>
> The new name "deepen" was chosen after the command to define shallow
> boundary in pack protocol. New commands also follow this tradition.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
--
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 v2 12/25] fetch-pack: use a common function for verbose printing

2016-02-04 Thread Eric Sunshine

On Thu, Feb 4, 2016 at 4:03 AM, Nguyễn Thái Ngọc Duy  wrote:
> This reduces the number of "if (verbose)" which makes it a bit easier
> to read imo. It also makes it easier to redirect all these printouts,
> to a file for example.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/fetch-pack.c b/fetch-pack.c
> @@ -810,39 +814,32 @@ static struct ref *do_fetch_pack(struct fetch_pack_args 
> *args,
> else if (server_supports("side-band")) {
> -   if (args->verbose)
> -   fprintf(stderr, "Server supports side-band\n");
> +   print_verbose(args, "Server supports side-band");
> use_sideband = 1;
> }
> if (server_supports("allow-tip-sha1-in-want")) {
> -   if (args->verbose)
> -   fprintf(stderr, "Server supports 
> allow-tip-sha1-in-want\n");
> +   print_verbose(args, "Server supports allow-tip-sha1-in-want");
> allow_unadvertised_object_request |= ALLOW_TIP_SHA1;
> }
> if (server_supports("allow-reachable-sha1-in-want")) {
> -   if (args->verbose)
> -   fprintf(stderr, "Server supports 
> allow-reachable-sha1-in-want\n");
> +   print_verbose(args, "Server supports 
> allow-reachable-sha1-in-want\n");

I think you want to drop the newline here as you did with all the
other call-sites since print_verbose() adds its own.

> allow_unadvertised_object_request |= ALLOW_REACHABLE_SHA1;
> }
> if (!server_supports("thin-pack"))
--
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: [PATCHv8 6/9] fetching submodules: respect `submodule.fetchJobs` config option

2016-02-04 Thread Junio C Hamano

Stefan Beller  writes:

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 586840d..5aa1c2d 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -37,7 +37,7 @@ static int prune = -1; /* unspecified */
>  static int all, append, dry_run, force, keep, multiple, update_head_ok, 
> verbosity;
>  static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
>  static int tags = TAGS_DEFAULT, unshallow, update_shallow;
> -static int max_children = 1;
> +static int max_children = -1;

This makes sense as you would later be doing "If left unspecified,
i.e. -1, then fall back to blah" ...

g> diff --git a/submodule-config.c b/submodule-config.c
> index 2841c5e..339b59d 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -32,6 +32,7 @@ enum lookup_type {
>  
>  static struct submodule_cache cache;
>  static int is_cache_init;
> +static unsigned long parallel_jobs = -1;

... but I do not think this does.  For one thing, you would not be
doing "If parallel_jobs < -1, then that is unspecified yet" for the
unsigned long variable, and for another, I do not think you would be
behaving differently for the first time (i.e. "unspecified yet" case).

I think you want to initialize this to 1, if your "not configured at
all" default is supposed to be 1.

> @@ -233,6 +234,13 @@ static int parse_generic_submodule_config(const char 
> *key,
> const char *value,
> struct parse_config_parameter *me)
>  {
> + if (!strcmp(key, "fetchjobs")) {
> + if (!git_parse_ulong(value, ¶llel_jobs)) {
> + warning(_("Error parsing submodule.fetchJobs; 
> Defaulting to 1."));
> + parallel_jobs = 1;

Hmph, this is not a die() because...?  Not a rhetorical question.

> +unsigned long config_parallel_submodules(void)
> +{
> + return parallel_jobs;
> +}

It is not a crime to make this return "int", as the code that
eventually uses it will assign it to a variable that will be
passed to run_processes_parallel() that takes an "int".

In fact, returning "int" would be preferrable.  You are causing
truncation somewhere in the callchain anyway.  If the truncation
bothers you, this function or immediately after calling
git_parse_ulong() would be a good place to do a range check.  The
variable parallel_jobs has to stay "unsigned long" in any case as
you are calling git_parse_ulong().

> diff --git a/submodule.c b/submodule.c
> index b83939c..eb7d54b 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -751,6 +751,11 @@ int fetch_populated_submodules(const struct argv_array 
> *options,
>   argv_array_push(&spf.args, "--recurse-submodules-default");
>   /* default value, "--submodule-prefix" and its value are added later */
>  
> + if (max_parallel_jobs < 0)
> + max_parallel_jobs = config_parallel_submodules();
> + if (max_parallel_jobs < 0)
> + max_parallel_jobs = 1;
> +
>   calculate_changed_submodule_paths();
>   run_processes_parallel(max_parallel_jobs,
>  get_next_submodule,
--
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: [PATCHv8 2/9] submodule-config: drop check against NULL

2016-02-04 Thread Jonathan Nieder

Stefan Beller wrote:

> Adhere to the common coding style of Git and not check explicitly
> for NULL throughout the file. There are still other occurrences in the
> code base but that is usually inside of conditions with side effects.
>
> Signed-off-by: Stefan Beller 
> ---
>  submodule-config.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)

Thanks.
Reviewed-by: Jonathan Nieder 
--
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: [PATCHv8 1/9] submodule-config: keep update strategy around

2016-02-04 Thread Jonathan Nieder

Hi,

It's been a while since I looked at this series.  Hopefully I can
come at it with some fresh eyes.  Thanks for your perseverance.

Stefan Beller wrote:

> We need the submodule update strategies in a later patch.

This description doesn't explain what the patch will do or why
parse_config didn't already retain the value.  If I look back
at this patch later and want to understand why it was written,
what would I want to know?

It could say

Currently submodule..update is only handled by git-submodule.sh.
C code will start to need to make use of that value as more of the
functionality of git-submodule.sh moves into library code in C.

Add the update field to 'struct submodule' and populate it so it can
be read as sm->update.

[...]
> +++ b/submodule-config.c
> @@ -311,6 +312,16 @@ static int parse_config(const char *var, const char 
> *value, void *data)
>   free((void *) submodule->url);
>   submodule->url = xstrdup(value);
>   }
> + } else if (!strcmp(item.buf, "update")) {
> + if (!value)
> + ret = config_error_nonbool(var);
> + else if (!me->overwrite && submodule->update != NULL)
> + warn_multiple_config(me->commit_sha1, submodule->name,
> +  "update");
> + else {
> + free((void *) submodule->update);
> + submodule->update = xstrdup(value);
> + }

(not about this patch) This code is repetitive.  Would there be a way
to share code between the parsing of different per-submodule settings?

[...]
> --- a/submodule-config.h
> +++ b/submodule-config.h
> @@ -14,6 +14,7 @@ struct submodule {
>   const char *url;
>   int fetch_recurse;
>   const char *ignore;
> + const char *update;

gitmodules(5) tells me the only allowed values are checkout, rebase,
merge, and none.  I wouldn't know at a glance how to match against
those in calling code.  Can this be an enum?

Thanks,
Jonathan
--
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 00/12] ref-filter: use parsing functions

2016-02-04 Thread Eric Sunshine

Karthik Nayak  writes:
> This series cleans up populate_value() in ref-filter, by moving out
> the parsing part of atoms to separate parsing functions. This ensures
> that parsing is only done once and also improves the modularity of the
> code.
>
> v1: http://thread.gmane.org/gmane.comp.version-control.git/281180
> v2: http://thread.gmane.org/gmane.comp.version-control.git/282563
> v3: http://thread.gmane.org/gmane.comp.version-control.git/283350
>
> Changes:
> * The parsing functions now take the arguments of the atom as
> function parameteres, instead of parsing it inside the fucntion.
> * Rebased on top of pu:jk/list-tag-2.7-regression
> * In strbuf use a copylen variable rather than using multiplication
> to perform a logical operation.
> * Code movement for easier review and general improvement.
> * Use COLOR_MAXLEN as the maximum size for the color variable.
> * Small code changes.
> * Documentation changes.
> * Fixed incorrect style of test (t6302).

v4 is a nice improvement. With the retirement of match_atom_name() and
its misleading and confusing use in the parsers, overall the parsers
are now more concise, straightforward, and easier (in fact, dead
simple) to comprehend.

As most of my review comments this round were relatively minor, and
there don't seem to be any major problems, hopefully this series will
wrap up with v5. 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

Re: [PATCH v4 11/12] ref-filter: introduce contents_atom_parser()

2016-02-04 Thread Eric Sunshine

On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak  wrote:
> Introduce contents_atom_parser() which will parse the '%(contents)'
> atom and store information into the 'used_atom' structure based on the
> modifiers used along with the atom. Also introduce body_atom_parser()
> and subject_atom_parser() for parsing atoms '%(body)' and '%(subject)'
> respectively.

These latter two parsers are conceptually distinct from introduction
of the %(contents) parser, thus could be done in a follow-on patch or
two (though I don't care strongly enough to insist upon it).

> Signed-off-by: Karthik Nayak 
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -66,6 +70,38 @@ static void remote_ref_atom_parser(struct used_atom *atom, 
> const char *arg)
> +static void body_atom_parser(struct used_atom *atom, const char *arg)
> +{
> +   if (arg)
> +   die("%%(body) atom does not take arguments");

None of the other error messages bothers saying "atom" literally
following a %(foo). For consistency, this likely should say merely:

%(body) does not take arguments

> +   atom->u.contents.option = C_BODY_DEP;
> +}
> +
> +static void subject_atom_parser(struct used_atom *atom, const char *arg)
> +{
> +   if (arg)
> +   die("%%(subject) atom does not take arguments");

Ditto.

> +   atom->u.contents.option = C_SUB;
> +}
> @@ -733,19 +763,15 @@ static void grab_sub_body_contents(struct atom_value 
> *val, int deref, struct obj
>
> for (i = 0; i < used_atom_cnt; i++) {
> const char *name = used_atom[i].name;
> +   struct used_atom *atom = &used_atom[i];

Not a big deal, but if you re-order these two lines, then the second,
which extracts name, could do so from the variable declared by the
first:

struct used_atom *atom = &used_atom[i];
const char *name = atom->name;

> struct atom_value *v = &val[i];
> -   const char *valp = NULL;
> if (!!deref != (*name == '*'))
> continue;
> if (deref)
> name++;
> if (strcmp(name, "subject") &&
> strcmp(name, "body") &&
> -   strcmp(name, "contents") &&
> -   strcmp(name, "contents:subject") &&
> -   strcmp(name, "contents:body") &&
> -   strcmp(name, "contents:signature") &&
> -   !starts_with(name, "contents:lines="))
> +   !starts_with(name, "contents"))
> continue;

This changes behavior in that it will also now match
"contentsanything", whereas the original was much more strict. Is that
desirable? (Genuine question.)

> if (!subpos)
> find_subpos(buf, sz,
--
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: git grep argument parser bug

2016-02-04 Thread Duy Nguyen

On Fri, Feb 5, 2016 at 5:02 AM, Dylan Grafmyre  wrote:
> In both ubuntu versions of git 1.9.1 and 2.7.0
>
> git grep '-test'
> git grep '--help'

(Un)Quoting is done by shell and stripped out before "git" is
executed. We just don't see them.

> Or any other expressions literal leading with a single dash or double
> dash get interpreted as argument flags and not as search expressions.
>
> What I expect is grep results for the literal strings "-test" and "--help"
> What i get is git help output informing of wrong argument usage, or
> accidentally turning on flags I didn't expect.
>
> Work around; for afflicted users terminating argument parsing with `
> -- ` works as it should.
>
> git grep -- '-test'

Or you can do "git grep -e --test". UNIX grep faces the same problem,
and also has -e to deal with it.

> Confirmed on two Ubuntu [based] systems, Ubuntu Server 14.03 and
> LinuxMint 17.3 [Cinnamon]
-- 
Duy
--
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 10/12] ref-filter: introduce remote_ref_atom_parser()

2016-02-04 Thread Eric Sunshine

On Sun, Jan 31, 2016 at 11:12:54PM +0530, Karthik Nayak wrote:
> Introduce remote_ref_atom_parser() which will parse the '%(upstream)'
> and '%(push)' atoms and store information into the 'used_atom'
> structure based on the modifiers used along with the corresponding
> atom.
> 
> Signed-off-by: Karthik Nayak 
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -50,6 +52,20 @@ static void color_atom_parser(struct used_atom *atom, 
> const char *color_value)
> +static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
> +{
> + if (!arg) {
> + atom->u.remote_ref = RR_NORMAL;
> + } else if (!strcmp(arg, "short"))

Style: drop unnecessary braces

> + atom->u.remote_ref = RR_SHORTEN;
> + else if (!strcmp(arg, "track"))
> + atom->u.remote_ref = RR_TRACK;
> + else if (!strcmp(arg, "trackshort"))
> + atom->u.remote_ref = RR_TRACKSHORT;
> + else
> + die(_("unrecognized format: %%(%s)"), atom->name);
> +}
--
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 v2 13/25] fetch-pack: use a separate flag for fetch in deepening mode

2016-02-04 Thread Junio C Hamano

Nguyễn Thái Ngọc Duy   writes:

> The shallow repo could be deepened or shortened when then user gives
> --depth. But in future that won't be the only way to deepen/shorten a
> repo. Stop relying on args->depth in this mode. Future deepening
> methods can simply set this flag on instead of updating all these if
> expressions.
>
> The new name "deepen" was chosen after the command to define shallow
> boundary in pack protocol. New commands also follow this tradition.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---

OK.  Up to here things look more-or-less sensible overall.

Thanks.

>  fetch-pack.c | 14 --
>  fetch-pack.h |  1 +
>  2 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 16917f9..e947514 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -197,7 +197,7 @@ enum ack_type {
>  
>  static void consume_shallow_list(struct fetch_pack_args *args, int fd)
>  {
> - if (args->stateless_rpc && args->depth > 0) {
> + if (args->stateless_rpc && args->deepen) {
>   /* If we sent a depth we will get back "duplicate"
>* shallow and unshallow commands every time there
>* is a block of have lines exchanged.
> @@ -348,7 +348,7 @@ static int find_common(struct fetch_pack_args *args,
>   packet_buf_flush(&req_buf);
>   state_len = req_buf.len;
>  
> - if (args->depth > 0) {
> + if (args->deepen) {
>   char *line;
>   const char *arg;
>   unsigned char sha1[20];
> @@ -557,7 +557,7 @@ static void filter_refs(struct fetch_pack_args *args,
>   }
>  
>   if (!keep && args->fetch_all &&
> - (!args->depth || !starts_with(ref->name, "refs/tags/")))
> + (!args->deepen || !starts_with(ref->name, "refs/tags/")))
>   keep = 1;
>  
>   if (keep) {
> @@ -627,7 +627,7 @@ static int everything_local(struct fetch_pack_args *args,
>   }
>   }
>  
> - if (!args->depth) {
> + if (!args->deepen) {
>   for_each_ref(mark_complete_oid, NULL);
>   for_each_alternate_ref(mark_alternate_complete, NULL);
>   commit_list_sort_by_date(&complete);
> @@ -813,6 +813,8 @@ static struct ref *do_fetch_pack(struct fetch_pack_args 
> *args,
>  
>   if ((args->depth > 0 || is_repository_shallow()) && 
> !server_supports("shallow"))
>   die("Server does not support shallow clients");
> + if (args->depth > 0)
> + args->deepen = 1;
>   if (server_supports("multi_ack_detailed")) {
>   print_verbose(args, "Server supports multi_ack_detailed");
>   multi_ack = 2;
> @@ -873,7 +875,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args 
> *args,
>  
>   if (args->stateless_rpc)
>   packet_flush(fd[1]);
> - if (args->depth > 0)
> + if (args->deepen)
>   setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
>   NULL);
>   else if (si->nr_ours || si->nr_theirs)
> @@ -940,7 +942,7 @@ static void update_shallow(struct fetch_pack_args *args,
>   int *status;
>   int i;
>  
> - if (args->depth > 0 && alternate_shallow_file) {
> + if (args->deepen && alternate_shallow_file) {
>   if (*alternate_shallow_file == '\0') { /* --unshallow */
>   unlink_or_warn(git_path_shallow());
>   rollback_lock_file(&shallow_lock);
> diff --git a/fetch-pack.h b/fetch-pack.h
> index bb7fd76..4d0adb0 100644
> --- a/fetch-pack.h
> +++ b/fetch-pack.h
> @@ -25,6 +25,7 @@ struct fetch_pack_args {
>   unsigned self_contained_and_connected:1;
>   unsigned cloning:1;
>   unsigned update_shallow:1;
> + unsigned deepen:1;
>  };
>  
>  /*
--
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 v2 12/25] fetch-pack: use a common function for verbose printing

2016-02-04 Thread Junio C Hamano

Nguyễn Thái Ngọc Duy   writes:

> This reduces the number of "if (verbose)" which makes it a bit easier
> to read imo. It also makes it easier to redirect all these printouts,
> to a file for example.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---

>  fetch-pack.c | 88 
> +---
>  1 file changed, 42 insertions(+), 46 deletions(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 01e34b6..16917f9 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -50,6 +50,21 @@ static int non_common_revs, multi_ack, use_sideband;
>  #define ALLOW_REACHABLE_SHA1 02
>  static unsigned int allow_unadvertised_object_request;
>  
> +__attribute__((format (printf, 2, 3)))
> +static inline void print_verbose(const struct fetch_pack_args *args,
> +  const char *fmt, ...)
> +{
> + va_list params;
> +
> + if (!args->verbose)
> + return;
> +
> + va_start(params, fmt);
> + vfprintf(stderr, fmt, params);
> + va_end(params);
> + fputc('\n', stderr);
> +}

This does "print if told to be verbose".  Sounds more like 'tracef',
'reportf', 'debugf', etc.

I am debating myself if it is a good idea to omit the final "\n" on
the calling side and add one unconditionally here, but the benefit
of brevity on the many callsites outweigh the reduced flexibility, I
guess.

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

Re: [PATCH v4 09/12] ref-filter: align: introduce long-form syntax

2016-02-04 Thread Eric Sunshine

On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak  wrote:
> Introduce optional prefixes "width=" and "position=" for the align atom
> so that the atom can be used as "%(align:width=,position=)".
>
> Add Documentation and tests for the same.
>
> Signed-off-by: Karthik Nayak 
> ---
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> @@ -133,6 +133,48 @@ test_expect_success 'right alignment' '
> +cat >expect <<-\EOF
> +|   refname is refs/heads/master   |refs/heads/master
> +|refname is refs/heads/side|refs/heads/side
> +| refname is refs/odd/spot |refs/odd/spot
> +| refname is refs/tags/double-tag  |refs/tags/double-tag
> +|refname is refs/tags/four |refs/tags/four
> +| refname is refs/tags/one |refs/tags/one
> +| refname is refs/tags/signed-tag  |refs/tags/signed-tag
> +|refname is refs/tags/three|refs/tags/three
> +| refname is refs/tags/two |refs/tags/two
> +EOF
> +
> +test_align_permutations() {
> +   while read -r option
> +   do
> +   test_expect_success "align:$option" '
> +   git for-each-ref --format="|%(align:$option)refname is 
> %(refname)%(end)|%(refname)" >actual &&
> +   test_cmp expect actual
> +   '

I think I mentioned this in the last round: The two lines following
test_expect_success() are actually the test body, thus should be
indented one more level. (Not necessarily worth a re-roll, though...)

> +   done
> +}
> +
> +test_align_permutations <<-\EOF
> +   middle,42
> +   42,middle
> +   position=middle,42
> +   42,position=middle
> +   middle,width=42
> +   width=42,middle
> +   position=middle,width=42
> +   width=42,position=middle
> +EOF
> +
> +# Last one wins (silently) when multiple arguments of the same type are given
> +
> +test_align_permutations <<-\EOF
> +   32,width=42,middle
> +   width=30,42,middle
> +   width=42,position=right,middle
> +   42,right,position=middle
> +EOF
> +
>  # Individual atoms inside %(align:...) and %(end) must not be quoted.
>
>  test_expect_success 'alignment with format quote' "
> --
> 2.7.0
--
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 v2 11/25] fetch-pack: use skip_prefix() instead of starts_with() when possible

2016-02-04 Thread Junio C Hamano

Nguyễn Thái Ngọc Duy   writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---

OK, with the same comment on "when possible".  I would have reused
the same "arg" without inventing a separate variable "value" if I
were doing this conversion, but either way would be OK.

>  builtin/fetch-pack.c | 13 +++--
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index 9b2a514..0482077 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -58,13 +58,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 *value;
>  
> - if (starts_with(arg, "--upload-pack=")) {
> - args.uploadpack = arg + 14;
> + if (skip_prefix(arg, "--upload-pack=", &value)) {
> + args.uploadpack = value;
>   continue;
>   }
> - if (starts_with(arg, "--exec=")) {
> - args.uploadpack = arg + 7;
> + if (skip_prefix(arg, "--exec=", &value)) {
> + args.uploadpack = value;
>   continue;
>   }
>   if (!strcmp("--quiet", arg) || !strcmp("-q", arg)) {
> @@ -100,8 +101,8 @@ int cmd_fetch_pack(int argc, const char **argv, const 
> char *prefix)
>   args.verbose = 1;
>   continue;
>   }
> - if (starts_with(arg, "--depth=")) {
> - args.depth = strtol(arg + 8, NULL, 0);
> + if (skip_prefix(arg, "--depth=", &value)) {
> + args.depth = strtol(value, NULL, 0);
>   continue;
>   }
>   if (!strcmp("--no-progress", arg)) {
--
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: git 2.7.0 crashes when top-down memory allocation preference is set

2016-02-04 Thread Johannes Schindelin

Hi,

On Thu, 4 Feb 2016, Klinger, Xia wrote:

> Thanks for confirming it. I hope a fix is available soon. I am using a
> very old version of Git at the moment to work around this issue, which
> doesn't comply to the requirement of our Stash Git Server from
> Atlassian.

If you would please stop top-posting? And yes, I worked on a fix for part
of the day.

Ciao,
Johannes
--
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 08/12] ref-filter: introduce align_atom_parser()

2016-02-04 Thread Eric Sunshine

On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak  wrote:
> Introduce align_atom_parser() which will parse an 'align' atom and
> store the required alignment position and width in the 'used_atom'
> structure for further usage in populate_value().
>
> Since this patch removes the last usage of match_atom_name(), remove
> the function from ref-filter.c.
>
> Signed-off-by: Karthik Nayak 
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -55,6 +61,37 @@ static align_type parse_align_position(const char *s)
> +static void align_atom_parser(struct used_atom *atom, const char *arg)
> +{
> +   struct align *align = &atom->u.align;
> +   struct strbuf **s, **to_free;
> +   unsigned int width = ~0U;
> +
> +   if (!arg)
> +   die(_("expected format: %%(align:,)"));
> +   s = to_free = strbuf_split_str_omit_term(arg, ',', 0);
> +
> +   align->position = ALIGN_LEFT;
> +
> +   while (*s) {
> +   int position;
> +   arg = s[0]->buf;

It's confusing to see 'arg' being re-used here for a different
purpose, and leads the reader to wonder if this is done because the
s[0]->buf might be needed outside the loop (when, in fact, it isn't).
It would be better to declare a new variable here in the scope of the
'while' loop to hold this value.

(I might have named the result of the strbuf split 'tokens' or even
short-and-sweet 'v' -- for vector -- and then used 's' for the name of
the new variable here in the 'while' loop, but these name suggestions
aren't particularly important; it is important to declare a new
variable here -- whatever you name it -- rather than re-using 'arg'.)

> +
> +   if (!strtoul_ui(arg, 10, &width))
> +   ;
> +   else if ((position = parse_align_position(arg)) >= 0)
> +   align->position = position;
> +   else
> +   die(_("unrecognized %%(align) argument: %s"), arg);
> +   s++;
> +   }
> +
> +   if (width == ~0U)
> +   die(_("positive width expected with the %%(align) atom"));
> +   align->width = width;
> +   strbuf_list_free(to_free);
> +}
--
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 v2 09/25] upload-pack: tighten number parsing at "deepen" lines

2016-02-04 Thread Junio C Hamano

Nguyễn Thái Ngọc Duy   writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---

Hmm, so "deepen 10-by-the-way-let-me-tell-you-something-else" was an
acceptable input that some (third-party) version of "git fetch"
could have used, but now we are rejecting it.

That "let me tell you something else" part wouldn't have reached
outside this code, so it is inconceivable that anybody would relied
on that looseness as a "feature", so the only practical risk would
be if somebody wrote a protocol driver, mumbling "on the Internet,
the end of line is CRLF, just like SMTP does", that sends a "deepen
10".  We used not to notice, but now we reject such a
reimplementation of Git.

>  upload-pack.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/upload-pack.c b/upload-pack.c
> index 257ad48..9f14933 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -641,9 +641,9 @@ static void receive_needs(void)
>   continue;
>   }
>   if (skip_prefix(line, "deepen ", &arg)) {
> - char *end;
> + char *end = NULL;
>   depth = strtol(arg, &end, 0);
> - if (end == arg || depth <= 0)
> + if (!end || *end || depth <= 0)
>   die("Invalid deepen: %s", line);
>   continue;
>   }
--
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 v2 08/25] upload-pack: use skip_prefix() instead of starts_with() when possible

2016-02-04 Thread Junio C Hamano

Nguyễn Thái Ngọc Duy   writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---

The result is certainly easier to read.  "when possible" is not
quite a right way to look at it, though.  I think the patch does it
when it makes sense (i.e. when the result becomes easier to read),
which is much better ;-)

I initially thought that the name "arg" was too bland, but we can
think of these as pointing at an argument on each line that begins
with a command (e.g. "have", "shallow", etc.), and when viewed that
way, calling the second token on the line an "arg" makes sense.



>  upload-pack.c | 26 +++---
>  1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/upload-pack.c b/upload-pack.c
> index bfb7985..257ad48 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -276,7 +276,7 @@ static void create_pack_file(void)
>   die("git upload-pack: %s", abort_msg);
>  }
>  
> -static int got_sha1(char *hex, unsigned char *sha1)
> +static int got_sha1(const char *hex, unsigned char *sha1)
>  {
>   struct object *o;
>   int we_knew_they_have = 0;
> @@ -382,6 +382,8 @@ static int get_common_commits(void)
>  
>   for (;;) {
>   char *line = packet_read_line(0, NULL);
> + const char *arg;
> +
>   reset_timeout();
>  
>   if (!line) {
> @@ -403,8 +405,8 @@ static int get_common_commits(void)
>   got_other = 0;
>   continue;
>   }
> - if (starts_with(line, "have ")) {
> - switch (got_sha1(line+5, sha1)) {
> + if (skip_prefix(line, "have ", &arg)) {
> + switch (got_sha1(arg, sha1)) {
>   case -1: /* they have what we do not */
>   got_other = 1;
>   if (multi_ack && ok_to_give_up()) {
> @@ -616,14 +618,16 @@ static void receive_needs(void)
>   const char *features;
>   unsigned char sha1_buf[20];
>   char *line = packet_read_line(0, NULL);
> + const char *arg;
> +
>   reset_timeout();
>   if (!line)
>   break;
>  
> - if (starts_with(line, "shallow ")) {
> + if (skip_prefix(line, "shallow ", &arg)) {
>   unsigned char sha1[20];
>   struct object *object;
> - if (get_sha1_hex(line + 8, sha1))
> + if (get_sha1_hex(arg, sha1))
>   die("invalid shallow line: %s", line);
>   object = parse_object(sha1);
>   if (!object)
> @@ -636,19 +640,19 @@ static void receive_needs(void)
>   }
>   continue;
>   }
> - if (starts_with(line, "deepen ")) {
> + if (skip_prefix(line, "deepen ", &arg)) {
>   char *end;
> - depth = strtol(line + 7, &end, 0);
> - if (end == line + 7 || depth <= 0)
> + depth = strtol(arg, &end, 0);
> + if (end == arg || depth <= 0)
>   die("Invalid deepen: %s", line);
>   continue;
>   }
> - if (!starts_with(line, "want ") ||
> - get_sha1_hex(line+5, sha1_buf))
> + if (!skip_prefix(line, "want ", &arg) ||
> + get_sha1_hex(arg, sha1_buf))
>   die("git upload-pack: protocol error, "
>   "expected to get sha, not '%s'", line);
>  
> - features = line + 45;
> + features = arg + 40;
>  
>   if (parse_feature_request(features, "multi_ack_detailed"))
>   multi_ack = 2;
--
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 v2 07/25] upload-pack: move "unshallow" sending code out of deepen()

2016-02-04 Thread Junio C Hamano

Nguyễn Thái Ngọc Duy   writes:

> Also add some more comments in this code because it takes too long to
> understand what it does (to me, who should be familiar enough to
> understand this code well!)
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---

The diff is harder to read than necessary (not your fault), but
splitting this into a separate helper makes sense to me.

Thanks.

>  upload-pack.c | 39 ++-
>  1 file changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/upload-pack.c b/upload-pack.c
> index ee5d20b..bfb7985 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -552,20 +552,10 @@ static void send_shallow(struct commit_list *result)
>   }
>  }
>  
> -static void deepen(int depth, const struct object_array *shallows)
> +static void send_unshallow(const struct object_array *shallows)
>  {
> - struct commit_list *result = NULL;
>   int i;
> - if (depth == INFINITE_DEPTH && !is_repository_shallow())
> - for (i = 0; i < shallows->nr; i++) {
> - struct object *object = shallows->objects[i].item;
> - object->flags |= NOT_SHALLOW;
> - }
> - else
> - result = get_shallow_commits(&want_obj, depth,
> -  SHALLOW, NOT_SHALLOW);
> - send_shallow(result);
> - free_commit_list(result);
> +
>   for (i = 0; i < shallows->nr; i++) {
>   struct object *object = shallows->objects[i].item;
>   if (object->flags & NOT_SHALLOW) {
> @@ -573,7 +563,13 @@ static void deepen(int depth, const struct object_array 
> *shallows)
>   packet_write(1, "unshallow %s",
>oid_to_hex(&object->oid));
>   object->flags &= ~CLIENT_SHALLOW;
> - /* make sure the real parents are parsed */
> + /*
> +  * We want to _register_ "object" as shallow, but we
> +  * also need to traverse object's parents to deepen a
> +  * shallow clone. Unregister it for now so we can
> +  * parse and add the parents to the want list, then
> +  * re-register it.
> +  */
>   unregister_shallow(object->oid.hash);
>   object->parsed = 0;
>   parse_commit_or_die((struct commit *)object);
> @@ -588,6 +584,23 @@ static void deepen(int depth, const struct object_array 
> *shallows)
>   /* make sure commit traversal conforms to client */
>   register_shallow(object->oid.hash);
>   }
> +}
> +
> +static void deepen(int depth, const struct object_array *shallows)
> +{
> + struct commit_list *result = NULL;
> + int i;
> + if (depth == INFINITE_DEPTH && !is_repository_shallow())
> + for (i = 0; i < shallows->nr; i++) {
> + struct object *object = shallows->objects[i].item;
> + object->flags |= NOT_SHALLOW;
> + }
> + else
> + result = get_shallow_commits(&want_obj, depth,
> +  SHALLOW, NOT_SHALLOW);
> + send_shallow(result);
> + free_commit_list(result);
> + send_unshallow(shallows);
>   packet_flush(1);
>  }
--
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 v2 06/25] upload-pack: remove unused variable "backup"

2016-02-04 Thread Junio C Hamano

Nguyễn Thái Ngọc Duy   writes:

> After the last patch, "result" and "backup" are the same. "result" used
> to move, but the movement is now contained in send_shallow(). Delete
> this redundant variable.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---

OK.  05 and 06 squashed together would also have been readable, but
either way is fine; it is not worth merging them into one.

>  upload-pack.c | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/upload-pack.c b/upload-pack.c
> index 0eb9a0b..ee5d20b 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -554,7 +554,7 @@ static void send_shallow(struct commit_list *result)
>  
>  static void deepen(int depth, const struct object_array *shallows)
>  {
> - struct commit_list *result = NULL, *backup = NULL;
> + struct commit_list *result = NULL;
>   int i;
>   if (depth == INFINITE_DEPTH && !is_repository_shallow())
>   for (i = 0; i < shallows->nr; i++) {
> @@ -562,11 +562,10 @@ static void deepen(int depth, const struct object_array 
> *shallows)
>   object->flags |= NOT_SHALLOW;
>   }
>   else
> - backup = result =
> - get_shallow_commits(&want_obj, depth,
> - SHALLOW, NOT_SHALLOW);
> + result = get_shallow_commits(&want_obj, depth,
> +  SHALLOW, NOT_SHALLOW);
>   send_shallow(result);
> - free_commit_list(backup);
> + free_commit_list(result);
>   for (i = 0; i < shallows->nr; i++) {
>   struct object *object = shallows->objects[i].item;
>   if (object->flags & NOT_SHALLOW) {
--
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 v2 04/25] upload-pack: move shallow deepen code out of receive_needs()

2016-02-04 Thread Junio C Hamano

Nguyễn Thái Ngọc Duy   writes:

> This is a prep step for further refactoring. Besides reindentation and
> s/shallows\./shallows->/g, no other changes are expected.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---

Good.

>  upload-pack.c | 99 
> +++
>  1 file changed, 52 insertions(+), 47 deletions(-)
>
> diff --git a/upload-pack.c b/upload-pack.c
> index b3f6653..97ed620 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -538,6 +538,55 @@ error:
>   }
>  }
>  
> +static void deepen(int depth, const struct object_array *shallows)
> +{
> + struct commit_list *result = NULL, *backup = NULL;
> + int i;
> + if (depth == INFINITE_DEPTH && !is_repository_shallow())
> + for (i = 0; i < shallows->nr; i++) {
> + struct object *object = shallows->objects[i].item;
> + object->flags |= NOT_SHALLOW;
> + }
> + else
> + backup = result =
> + get_shallow_commits(&want_obj, depth,
> + SHALLOW, NOT_SHALLOW);
> + while (result) {
> + struct object *object = &result->item->object;
> + if (!(object->flags & (CLIENT_SHALLOW|NOT_SHALLOW))) {
> + packet_write(1, "shallow %s",
> +  oid_to_hex(&object->oid));
> + register_shallow(object->oid.hash);
> + shallow_nr++;
> + }
> + result = result->next;
> + }
> + free_commit_list(backup);
> + for (i = 0; i < shallows->nr; i++) {
> + struct object *object = shallows->objects[i].item;
> + if (object->flags & NOT_SHALLOW) {
> + struct commit_list *parents;
> + packet_write(1, "unshallow %s",
> +  oid_to_hex(&object->oid));
> + object->flags &= ~CLIENT_SHALLOW;
> + /* make sure the real parents are parsed */
> + unregister_shallow(object->oid.hash);
> + object->parsed = 0;
> + parse_commit_or_die((struct commit *)object);
> + parents = ((struct commit *)object)->parents;
> + while (parents) {
> + add_object_array(&parents->item->object,
> +  NULL, &want_obj);
> + parents = parents->next;
> + }
> + add_object_array(object, NULL, &extra_edge_obj);
> + }
> + /* make sure commit traversal conforms to client */
> + register_shallow(object->oid.hash);
> + }
> + packet_flush(1);
> +}
> +
>  static void receive_needs(void)
>  {
>   struct object_array shallows = OBJECT_ARRAY_INIT;
> @@ -630,53 +679,9 @@ static void receive_needs(void)
>  
>   if (depth == 0 && shallows.nr == 0)
>   return;
> - if (depth > 0) {
> - struct commit_list *result = NULL, *backup = NULL;
> - int i;
> - if (depth == INFINITE_DEPTH && !is_repository_shallow())
> - for (i = 0; i < shallows.nr; i++) {
> - struct object *object = 
> shallows.objects[i].item;
> - object->flags |= NOT_SHALLOW;
> - }
> - else
> - backup = result =
> - get_shallow_commits(&want_obj, depth,
> - SHALLOW, NOT_SHALLOW);
> - while (result) {
> - struct object *object = &result->item->object;
> - if (!(object->flags & (CLIENT_SHALLOW|NOT_SHALLOW))) {
> - packet_write(1, "shallow %s",
> - oid_to_hex(&object->oid));
> - register_shallow(object->oid.hash);
> - shallow_nr++;
> - }
> - result = result->next;
> - }
> - free_commit_list(backup);
> - for (i = 0; i < shallows.nr; i++) {
> - struct object *object = shallows.objects[i].item;
> - if (object->flags & NOT_SHALLOW) {
> - struct commit_list *parents;
> - packet_write(1, "unshallow %s",
> - oid_to_hex(&object->oid));
> - object->flags &= ~CLIENT_SHALLOW;
> - /* make sure the real parents are parsed */
> - unregister_shallow(object->oid.hash);
> - object->parsed = 0;
> - parse_commit_or_die((struct commit *)object);
> - par

Re: [PATCH v2 03/25] transport-helper.c: do not send null option to remote helper

2016-02-04 Thread Junio C Hamano

Nguyễn Thái Ngọc Duy   writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---

This is even more strange.  Are the current callers broken and some
sends value==NULL for an option that is not is_bool, resulting in
a call to quote_c_style() with NULL?  I somehow find it hard to
believe as that would lead to an immediate segfault.

Assuming that no current caller passes NULL to value when is_bool is
not in effect, there needs an explanation why future new callers may
need to do so.  An alternative for a valueless option could be to
send "option name\n" instead of the usual "option name value\n", but
without such an explanation, readers cannot tell why not sending
anything about "name", which is what this patch chooses to implement,
is a better idea.

>  transport-helper.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/transport-helper.c b/transport-helper.c
> index 35023da..2e78c4d 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -309,8 +309,12 @@ static int set_helper_option(struct transport *transport,
>   strbuf_addf(&buf, "option %s ", name);
>   if (is_bool)
>   strbuf_addstr(&buf, value ? "true" : "false");
> - else
> + else if (value)
>   quote_c_style(value, &buf, NULL, 0);
> + else {
> + strbuf_release(&buf);
> + return 0;
> + }
>   strbuf_addch(&buf, '\n');
>  
>   ret = strbuf_set_helper_option(data, &buf);
--
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 v2 02/25] transport-helper.c: refactor set_helper_option()

2016-02-04 Thread Junio C Hamano

Nguyễn Thái Ngọc Duy   writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---

Just like other patches, this is thin on the rationale.  You are
planning to invent another function that prepares the requrest in a
strbuf and allow it to call this helper to perform a single round
trip with the other side?  If that is the case, the split is very
sensible, but you need to say something about "why".

>  transport-helper.c | 37 +++--
>  1 file changed, 23 insertions(+), 14 deletions(-)
>
> diff --git a/transport-helper.c b/transport-helper.c
> index a6bff8b..35023da 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -260,6 +260,28 @@ static const char *boolean_options[] = {
>   TRANS_OPT_FOLLOWTAGS,
>   };
>  
> +static int strbuf_set_helper_option(struct helper_data *data,
> + struct strbuf *buf)
> +{
> + int ret;
> +
> + sendline(data, buf);
> + if (recvline(data, buf))
> + exit(128);
> +
> + if (!strcmp(buf->buf, "ok"))
> + ret = 0;
> + else if (starts_with(buf->buf, "error")) {
> + ret = -1;
> + } else if (!strcmp(buf->buf, "unsupported"))
> + ret = 1;
> + else {
> + warning("%s unexpectedly said: '%s'", data->name, buf->buf);
> + ret = 1;
> + }
> + return ret;
> +}
> +
>  static int set_helper_option(struct transport *transport,
> const char *name, const char *value)
>  {
> @@ -291,20 +313,7 @@ static int set_helper_option(struct transport *transport,
>   quote_c_style(value, &buf, NULL, 0);
>   strbuf_addch(&buf, '\n');
>  
> - sendline(data, &buf);
> - if (recvline(data, &buf))
> - exit(128);
> -
> - if (!strcmp(buf.buf, "ok"))
> - ret = 0;
> - else if (starts_with(buf.buf, "error")) {
> - ret = -1;
> - } else if (!strcmp(buf.buf, "unsupported"))
> - ret = 1;
> - else {
> - warning("%s unexpectedly said: '%s'", data->name, buf.buf);
> - ret = 1;
> - }
> + ret = strbuf_set_helper_option(data, &buf);
>   strbuf_release(&buf);
>   return ret;
>  }
--
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

Merging branches with smudge filter

2016-02-04 Thread Leonardo

Hi, everybody. I'm new to git and I'd like to keep track of some codes
we have here in our company. They have some sensitive information I
would like to keep private. After some googling, I found some
solutions that encrypt/decrypt the files using filters as they're
committed/checked out. I've been using this approach and it suits my
needs. Now I need to merge two branches, but the process is failing
for two files in particular. First of all, here's my config file:

[filter "openssl"]
clean = openssl enc -aes-256-cbc -a -nosalt -pass pass:password
smudge = openssl enc -d -aes-256-cbc -a -nosalt -pass pass:password
required

For these two files I mentioned, decryption fails. Then I did this:

smudge = openssl enc -d -aes-256-cbc -a -nosalt -pass
pass:password || tee -a /out.txt

Based on the output, it seems like the input data is corrupted, but I
don't get it. These files are regular source code like any other file.
I can also check out both branches individually, so I'm assuming the
stored blob is fine. Every other file is perfectly merged. What should
I do?

Thank you.
--
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 v2 01/25] remote-curl.c: convert fetch_git() to use argv_array

2016-02-04 Thread Junio C Hamano

Nguyễn Thái Ngọc Duy   writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy 

Yay, it's about time ;-)

> diff --git a/remote-curl.c b/remote-curl.c
> index c704857..3ac5b6c 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -726,37 +726,31 @@ static int fetch_git(struct discovery *heads,
>   struct rpc_state rpc;
>   struct strbuf preamble = STRBUF_INIT;
>   char *depth_arg = NULL;
> - int argc = 0, i, err;
> - const char *argv[17];
> + int i, err;
> + struct argv_array args = ARGV_ARRAY_INIT;
>  
> - argv[argc++] = "fetch-pack";
> - argv[argc++] = "--stateless-rpc";
> - argv[argc++] = "--stdin";
> - argv[argc++] = "--lock-pack";
> + argv_array_pushl(&args, "fetch-pack", "--stateless-rpc",
> +  "--stdin", "--lock-pack",
> +  NULL);

A lone NULL after a reasonably short line looks somewhat irritating,
but if you are planning to add more on that short line in the later
patch, it is OK.  Let's keep reading.

The remainder of this patch looked obviously correct.

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

Re: [PATCH v3 00/20] refs backend rebase on pu

2016-02-04 Thread David Turner

On Wed, 2016-02-03 at 21:58 -0500, Jeff King wrote:
> On Thu, Feb 04, 2016 at 01:54:44AM +, Ramsay Jones wrote:
> 
> > > They were working for me as-of the time I sent them.  I guess
> > > something
> > > must have broken since.  I'll rebase, test, and send a new
> > > series.
> > 
> > I didn't spend too long looking at it, but I think this interacts
> > with
> > Jeff's patch a2d5156c ("resolve_gitlink_ref: ignore non-repository
> > paths",
> > 22-01-2016) which introduces the new test in 't3000-ls-files
> > -others.sh'
> > which fails for me.
> > 
> > The change which Jeff made to resolve_gitlink_ref() is effectively
> > side-stepped
> > by the call to check_submodule_backend() in the new
> > resolve_gitlink_ref().
> > (Jeff's change is now in the 'files' backend version of
> > resolve_gitlink_ref()).
> 
> Yeah. The check_submodule_backend() function calls
> strbuf_git_path_submodule(), which unconditionally requires that the
> path be an actual submodule (the irony being that we are using it to
> find out whether we have a submodule!). So I don't think there's a
> conflict between our code, so much as that the new code has the same
> bug
> I fixed in a2d5156c (and we didn't notice earlier, because there was
> no
> test).
> 
> The solution in a2d5156 is to use is_nonbare_repository_dir() before
> assuming we have a submodule. I think check_submodule_backend() would
> want to do the same thing. This is the minimal fix:
> 
> diff --git a/refs.c b/refs.c
> index 3d4c0a6..7f86c49 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -313,9 +313,8 @@ static void check_submodule_backend(const char
> *submodule)
>   if (!submodule)
>   goto done;
>  
> - strbuf_git_path_submodule(&sb, submodule, "%s", "");
> -
> - if (!is_git_directory(sb.buf))
> + strbuf_addstr(&sb, submodule);
> + if (!is_nonbare_repository_dir(&sb))
>   goto done;
>  
>   strbuf_reset(&sb);

+ one other place, yeah, that does fix it.

> That gets the test passing. But I noticed a few other things, some
> related and some unrelated, while looking at this function:
> 
>   - in files_resolve_gitlink_ref, if we do find a submodule, we cache
> the result with the ref_cache code. But here, we would read the
> .git
> file repeatedly (and in fact, it happens twice per call, as
> submodule_git_path has to read it itself).
> 
> I don't know if it would be worth adding any kind of caching at
> this
> layer.  It may be that we don't typically resolve more than one
> ref
> (or do more than one for_each_ref iteration) for a submodule, so
> the
> cache is pointless. I didn't implement it specifically in
> a2d5156,
> it just came for free with the existing ref_cache code.

I'm going to punt on this for now; we can fix it if it becomes a
problem.

>   - check_submodule_backend knows whether we have a submodule at all
> and
> is worth proceeding, but does not tell its callers. So we'll end
> up
> in the backend files_resolve_gitlink_ref and make the same check.
> It's probably worth moving this logic to the outer layer so each
> backend doesn't have to implement it (and then the check in
> files_resolve_gitlink_ref can actually go away).

Fixed, thanks.

  - for the common case of submodule==NULL (i.e., the main repository),
> check_submodule_backend should be a noop, but it allocates and
> frees
> the submodule_storage_backend string. Probably not a huge deal,
> but
> it can be easily bumped down, and the first "goto done" turned
> into
> a "return".

Fixed, thanks.

>   - if the submodule does have a backend configured, we leak the
> memory
> for the default string. I think the submodule_backend() config
> callback needs to free() the previous value.

Fixed.

>   - the config callback unconditionally dereferences "value", which
> will
> segfault if the submodule's extensions.refstorage is a pure
> boolean
> like:
> 
> [extensions]
>   refstorage

Fixed.

> That should never happen, of course, but we should be checking
> and
> dying for "!value" rather than segfaulting. Using
> git_config_string() will do this for you.
> 
> Hope that helps.

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

Re: [PATCH v4 3/3] ident.c: cleanup wrt ident's source

2016-02-04 Thread Junio C Hamano

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 06/12] ref-filter: introduce color_atom_parser()

2016-02-04 Thread Eric Sunshine

On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak  wrote:
> Introduce color_atom_parser() which will parse a "color" atom and
> store its color in the "used_atom" structure for further usage in
> populate_value().
>
> Signed-off-by: Karthik Nayak 
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -29,10 +29,21 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } 
> cmp_type;
>  static struct used_atom {
> const char *name;
> cmp_type type;
> +   union {
> +   char color[COLOR_MAXLEN];
> +   } u;
>  } *used_atom;
>  static int used_atom_cnt, need_tagged, need_symref;
>  static int need_color_reset_at_eol;
>
> +static void color_atom_parser(struct used_atom *atom, const char 
> *color_value)
> +{
> +   if (!color_value)
> +   die(_("expected format: %%(color:)"));
> +   if (color_parse(color_value, atom->u.color) < 0)
> +   die(_("invalid color value: %s"), atom->u.color);

Shouldn't this be:

die(_("invalid color value: %s"), color_value);

?

> +}
--
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: git grep argument parser bug

2016-02-04 Thread Junio C Hamano

Dylan Grafmyre  writes:

> In both ubuntu versions of git 1.9.1 and 2.7.0
>
> git grep '-test'
> git grep '--help'
>
> Or any other expressions literal leading with a single dash or double
> dash get interpreted as argument flags and not as search expressions.
> ...
> What I expect is grep results for the literal strings "-test" and "--help"

I think you'd need to adjust your expectations, then ;-).

This is how "grep" (not "git grep") works, and you use "-e", i.e.

grep -e "-anything that begins with a dash" FILES...

to disambiguate.  If you are scripting, if you do not know what you
have in a variable, and if you are looking for the value in that
variable, you would always do

grep -e "$variable" FILES...

not

grep "$variable" FILES...

I think all of the above is pretty much the standard practice and
"git grep" merely follows suit, i.e.

git grep -e "-help"

is designed to work that way to match users' expectations.
--
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

[PATCHv8 3/9] submodule-config: remove name_and_item_from_var

2016-02-04 Thread Stefan Beller

`name_and_item_from_var` does not provide the proper abstraction
we need here in a later patch. As we have only a single call site
for `name_and_item_from_var`, just inline it. Instead of using
strbufs, use direct char *.

Signed-off-by: Stefan Beller 
---
 submodule-config.c | 40 +++-
 1 file changed, 11 insertions(+), 29 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index 6d01941..c08ee7f 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -161,22 +161,6 @@ static struct submodule *cache_lookup_name(struct 
submodule_cache *cache,
return NULL;
 }
 
-static int name_and_item_from_var(const char *var, struct strbuf *name,
- struct strbuf *item)
-{
-   const char *subsection, *key;
-   int subsection_len, parse;
-   parse = parse_config_key(var, "submodule", &subsection,
-   &subsection_len, &key);
-   if (parse < 0 || !subsection)
-   return 0;
-
-   strbuf_add(name, subsection, subsection_len);
-   strbuf_addstr(item, key);
-
-   return 1;
-}
-
 static struct submodule *lookup_or_create_by_name(struct submodule_cache 
*cache,
const unsigned char *gitmodules_sha1, const char *name)
 {
@@ -251,18 +235,19 @@ static int parse_config(const char *var, const char 
*value, void *data)
 {
struct parse_config_parameter *me = data;
struct submodule *submodule;
-   struct strbuf name = STRBUF_INIT, item = STRBUF_INIT;
-   int ret = 0;
+   int subsection_len, ret = 0;
+   const char *subsection, *key;
 
-   /* this also ensures that we only parse submodule entries */
-   if (!name_and_item_from_var(var, &name, &item))
+   if (parse_config_key(var, "submodule", &subsection,
+&subsection_len, &key) < 0 || !subsection)
return 0;
 
+   subsection = xmemdupz(subsection, subsection_len);
submodule = lookup_or_create_by_name(me->cache,
 me->gitmodules_sha1,
-name.buf);
+subsection);
 
-   if (!strcmp(item.buf, "path")) {
+   if (!strcmp(key, "path")) {
if (!value)
ret = config_error_nonbool(var);
else if (!me->overwrite && submodule->path)
@@ -275,7 +260,7 @@ static int parse_config(const char *var, const char *value, 
void *data)
submodule->path = xstrdup(value);
cache_put_path(me->cache, submodule);
}
-   } else if (!strcmp(item.buf, "fetchrecursesubmodules")) {
+   } else if (!strcmp(key, "fetchrecursesubmodules")) {
/* when parsing worktree configurations we can die early */
int die_on_error = is_null_sha1(me->gitmodules_sha1);
if (!me->overwrite &&
@@ -286,7 +271,7 @@ static int parse_config(const char *var, const char *value, 
void *data)
submodule->fetch_recurse = parse_fetch_recurse(
var, value,
die_on_error);
-   } else if (!strcmp(item.buf, "ignore")) {
+   } else if (!strcmp(key, "ignore")) {
if (!value)
ret = config_error_nonbool(var);
else if (!me->overwrite && submodule->ignore)
@@ -302,7 +287,7 @@ static int parse_config(const char *var, const char *value, 
void *data)
free((void *) submodule->ignore);
submodule->ignore = xstrdup(value);
}
-   } else if (!strcmp(item.buf, "url")) {
+   } else if (!strcmp(key, "url")) {
if (!value) {
ret = config_error_nonbool(var);
} else if (!me->overwrite && submodule->url) {
@@ -312,7 +297,7 @@ static int parse_config(const char *var, const char *value, 
void *data)
free((void *) submodule->url);
submodule->url = xstrdup(value);
}
-   } else if (!strcmp(item.buf, "update")) {
+   } else if (!strcmp(key, "update")) {
if (!value)
ret = config_error_nonbool(var);
else if (!me->overwrite && submodule->update)
@@ -324,9 +309,6 @@ static int parse_config(const char *var, const char *value, 
void *data)
}
}
 
-   strbuf_release(&name);
-   strbuf_release(&item);
-
return ret;
 }
 
-- 
2.7.0.rc0.41.gd102984.dirty

--
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

[PATCHv8 2/9] submodule-config: drop check against NULL

2016-02-04 Thread Stefan Beller

Adhere to the common coding style of Git and not check explicitly
for NULL throughout the file. There are still other occurrences in the
code base but that is usually inside of conditions with side effects.

Signed-off-by: Stefan Beller 
---
 submodule-config.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index 4239b0e..6d01941 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -265,7 +265,7 @@ static int parse_config(const char *var, const char *value, 
void *data)
if (!strcmp(item.buf, "path")) {
if (!value)
ret = config_error_nonbool(var);
-   else if (!me->overwrite && submodule->path != NULL)
+   else if (!me->overwrite && submodule->path)
warn_multiple_config(me->commit_sha1, submodule->name,
"path");
else {
@@ -289,7 +289,7 @@ static int parse_config(const char *var, const char *value, 
void *data)
} else if (!strcmp(item.buf, "ignore")) {
if (!value)
ret = config_error_nonbool(var);
-   else if (!me->overwrite && submodule->ignore != NULL)
+   else if (!me->overwrite && submodule->ignore)
warn_multiple_config(me->commit_sha1, submodule->name,
"ignore");
else if (strcmp(value, "untracked") &&
@@ -305,7 +305,7 @@ static int parse_config(const char *var, const char *value, 
void *data)
} else if (!strcmp(item.buf, "url")) {
if (!value) {
ret = config_error_nonbool(var);
-   } else if (!me->overwrite && submodule->url != NULL) {
+   } else if (!me->overwrite && submodule->url) {
warn_multiple_config(me->commit_sha1, submodule->name,
"url");
} else {
@@ -315,7 +315,7 @@ static int parse_config(const char *var, const char *value, 
void *data)
} else if (!strcmp(item.buf, "update")) {
if (!value)
ret = config_error_nonbool(var);
-   else if (!me->overwrite && submodule->update != NULL)
+   else if (!me->overwrite && submodule->update)
warn_multiple_config(me->commit_sha1, submodule->name,
 "update");
else {
-- 
2.7.0.rc0.41.gd102984.dirty

--
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

[PATCHv8 1/9] submodule-config: keep update strategy around

2016-02-04 Thread Stefan Beller

We need the submodule update strategies in a later patch.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 submodule-config.c | 11 +++
 submodule-config.h |  1 +
 2 files changed, 12 insertions(+)

diff --git a/submodule-config.c b/submodule-config.c
index afe0ea8..4239b0e 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -194,6 +194,7 @@ static struct submodule *lookup_or_create_by_name(struct 
submodule_cache *cache,
 
submodule->path = NULL;
submodule->url = NULL;
+   submodule->update = NULL;
submodule->fetch_recurse = RECURSE_SUBMODULES_NONE;
submodule->ignore = NULL;
 
@@ -311,6 +312,16 @@ static int parse_config(const char *var, const char 
*value, void *data)
free((void *) submodule->url);
submodule->url = xstrdup(value);
}
+   } else if (!strcmp(item.buf, "update")) {
+   if (!value)
+   ret = config_error_nonbool(var);
+   else if (!me->overwrite && submodule->update != NULL)
+   warn_multiple_config(me->commit_sha1, submodule->name,
+"update");
+   else {
+   free((void *) submodule->update);
+   submodule->update = xstrdup(value);
+   }
}
 
strbuf_release(&name);
diff --git a/submodule-config.h b/submodule-config.h
index 9061e4e..f9e2a29 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -14,6 +14,7 @@ struct submodule {
const char *url;
int fetch_recurse;
const char *ignore;
+   const char *update;
/* the sha1 blob id of the responsible .gitmodules file */
unsigned char gitmodules_sha1[20];
 };
-- 
2.7.0.rc0.41.gd102984.dirty

--
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

[PATCHv8 9/9] clone: allow an explicit argument for parallel submodule clones

2016-02-04 Thread Stefan Beller

Just pass it along to "git submodule update", which may pick reasonable
defaults if you don't specify an explicit number.

Signed-off-by: Stefan Beller 
---
 Documentation/git-clone.txt |  6 +-
 builtin/clone.c | 19 +--
 t/t7406-submodule-update.sh | 15 +++
 3 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 6bf000d..6db7b6d 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -14,7 +14,7 @@ SYNOPSIS
  [-o ] [-b ] [-u ] [--reference ]
  [--dissociate] [--separate-git-dir ]
  [--depth ] [--[no-]single-branch]
- [--recursive | --recurse-submodules] [--] 
+ [--recursive | --recurse-submodules] [--jobs ] [--] 
  []
 
 DESCRIPTION
@@ -221,6 +221,10 @@ objects from the source repository into a pack in the 
cloned repository.
The result is Git repository can be separated from working
tree.
 
+-j ::
+--jobs ::
+   The number of submodules fetched at the same time.
+   Defaults to the `submodule.fetchJobs` option.
 
 ::
The (possibly remote) repository to clone from.  See the
diff --git a/builtin/clone.c b/builtin/clone.c
index a0b3cd9..b004fb4 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -50,6 +50,7 @@ static int option_progress = -1;
 static struct string_list option_config;
 static struct string_list option_reference;
 static int option_dissociate;
+static int max_jobs = -1;
 
 static struct option builtin_clone_options[] = {
OPT__VERBOSITY(&option_verbosity),
@@ -72,6 +73,8 @@ static struct option builtin_clone_options[] = {
N_("initialize submodules in the clone")),
OPT_BOOL(0, "recurse-submodules", &option_recursive,
N_("initialize submodules in the clone")),
+   OPT_INTEGER('j', "jobs", &max_jobs,
+   N_("number of submodules cloned in parallel")),
OPT_STRING(0, "template", &option_template, N_("template-directory"),
   N_("directory from which templates will be used")),
OPT_STRING_LIST(0, "reference", &option_reference, N_("repo"),
@@ -95,10 +98,6 @@ static struct option builtin_clone_options[] = {
OPT_END()
 };
 
-static const char *argv_submodule[] = {
-   "submodule", "update", "--init", "--recursive", NULL
-};
-
 static const char *get_repo_path_1(struct strbuf *path, int *is_bundle)
 {
static char *suffix[] = { "/.git", "", ".git/.git", ".git" };
@@ -724,8 +723,16 @@ static int checkout(void)
err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1),
   sha1_to_hex(sha1), "1", NULL);
 
-   if (!err && option_recursive)
-   err = run_command_v_opt(argv_submodule, RUN_GIT_CMD);
+   if (!err && option_recursive) {
+   struct argv_array args = ARGV_ARRAY_INIT;
+   argv_array_pushl(&args, "submodule", "update", "--init", 
"--recursive", NULL);
+
+   if (max_jobs != -1)
+   argv_array_pushf(&args, "--jobs=%d", max_jobs);
+
+   err = run_command_v_opt(args.argv, RUN_GIT_CMD);
+   argv_array_clear(&args);
+   }
 
return err;
 }
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 7fd5142..090891e 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -786,4 +786,19 @@ test_expect_success 'submodule update can be run in 
parallel' '
 grep "9 tasks" trace.out
)
 '
+
+test_expect_success 'git clone passes the parallel jobs config on to 
submodules' '
+   test_when_finished "rm -rf super4" &&
+   GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 7 . 
super4 &&
+   grep "7 tasks" trace.out &&
+   rm -rf super4 &&
+   git config --global submodule.fetchJobs 8 &&
+   GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules . super4 &&
+   grep "8 tasks" trace.out &&
+   rm -rf super4 &&
+   GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 9 . 
super4 &&
+   grep "9 tasks" trace.out &&
+   rm -rf super4
+'
+
 test_done
-- 
2.7.0.rc0.41.gd102984.dirty

--
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

[PATCHv8 7/9] git submodule update: have a dedicated helper for cloning

2016-02-04 Thread Stefan Beller

This introduces a new helper function in git submodule--helper
which takes care of cloning all submodules, which we want to
parallelize eventually.

Some tests (such as empty URL, update_mode=none) are required in the
helper to make the decision for cloning. These checks have been
moved into the C function as well (no need to repeat them in the
shell script).

As we can only access the stderr channel from within the parallel
processing engine, we need to reroute the error message for
specified but initialized submodules to stderr. As it is an error
message, this should have gone to stderr in the first place, so it
is a bug fix along the way.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 builtin/submodule--helper.c | 236 
 git-submodule.sh|  45 +++--
 t/t7400-submodule-basic.sh  |   4 +-
 3 files changed, 249 insertions(+), 36 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f4c3eff..90f9dc6 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -255,6 +255,241 @@ static int module_clone(int argc, const char **argv, 
const char *prefix)
return 0;
 }
 
+static int git_submodule_config(const char *var, const char *value, void *cb)
+{
+   return parse_submodule_config_option(var, value);
+}
+
+struct submodule_update_clone {
+   /* states */
+   int count;
+   int print_unmatched;
+   /* configuration */
+   int quiet;
+   const char *reference;
+   const char *depth;
+   const char *update;
+   const char *recursive_prefix;
+   const char *prefix;
+   struct module_list list;
+   struct string_list projectlines;
+   struct pathspec pathspec;
+};
+#define SUBMODULE_UPDATE_CLONE_INIT {0, 0, 0, NULL, NULL, NULL, NULL, NULL, 
MODULE_LIST_INIT, STRING_LIST_INIT_DUP}
+
+static void fill_clone_command(struct child_process *cp, int quiet,
+  const char *prefix, const char *path,
+  const char *name, const char *url,
+  const char *reference, const char *depth)
+{
+   cp->git_cmd = 1;
+   cp->no_stdin = 1;
+   cp->stdout_to_stderr = 1;
+   cp->err = -1;
+   argv_array_push(&cp->args, "submodule--helper");
+   argv_array_push(&cp->args, "clone");
+   if (quiet)
+   argv_array_push(&cp->args, "--quiet");
+
+   if (prefix)
+   argv_array_pushl(&cp->args, "--prefix", prefix, NULL);
+
+   argv_array_pushl(&cp->args, "--path", path, NULL);
+   argv_array_pushl(&cp->args, "--name", name, NULL);
+   argv_array_pushl(&cp->args, "--url", url, NULL);
+   if (reference)
+   argv_array_push(&cp->args, reference);
+   if (depth)
+   argv_array_push(&cp->args, depth);
+}
+
+static int update_clone_get_next_task(struct child_process *cp,
+ struct strbuf *err,
+ void *pp_cb,
+ void **pp_task_cb)
+{
+   struct submodule_update_clone *pp = pp_cb;
+
+   for (; pp->count < pp->list.nr; pp->count++) {
+   const struct submodule *sub = NULL;
+   const struct cache_entry *ce = pp->list.entries[pp->count];
+   struct strbuf displaypath_sb = STRBUF_INIT;
+   struct strbuf sb = STRBUF_INIT;
+   const char *displaypath = NULL;
+   const char *update_module = NULL;
+   char *url = NULL;
+   int needs_cloning = 0;
+
+   if (ce_stage(ce)) {
+   if (pp->recursive_prefix)
+   strbuf_addf(err,
+   "Skipping unmerged submodule %s/%s\n",
+   pp->recursive_prefix, ce->name);
+   else
+   strbuf_addf(err,
+   "Skipping unmerged submodule %s\n",
+   ce->name);
+   goto cleanup_and_continue;
+   }
+
+   sub = submodule_from_path(null_sha1, ce->name);
+
+   if (pp->recursive_prefix)
+   displaypath = relative_path(pp->recursive_prefix,
+   ce->name, &displaypath_sb);
+   else
+   displaypath = ce->name;
+
+   if (pp->update)
+   update_module = pp->update;
+   if (!update_module)
+   update_module = sub->update;
+   if (!update_module)
+   update_module = "checkout";
+   if (!strcmp(update_module, "none")) {
+   strbuf_addf(err, "Skipping submodule '%s'\n",
+   displaypath);
+   goto cle

[PATCHv8 6/9] fetching submodules: respect `submodule.fetchJobs` config option

2016-02-04 Thread Stefan Beller

This allows to configure fetching and updating in parallel
without having the command line option.

This moved the responsibility to determine how many parallel processes
to start from builtin/fetch to submodule.c as we need a way to communicate
"The user did not specify the number of parallel processes in the command
line options" in the builtin fetch. The submodule code takes care of
the precedence (CLI > config > default)

Signed-off-by: Stefan Beller 
---
 Documentation/config.txt|  6 ++
 builtin/fetch.c |  2 +-
 submodule-config.c  | 13 +
 submodule-config.h  |  2 ++
 submodule.c |  5 +
 t/t5526-fetch-submodules.sh | 14 ++
 6 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2d06b11..3b02732 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2646,6 +2646,12 @@ submodule..ignore::
"--ignore-submodules" option. The 'git submodule' commands are not
affected by this setting.
 
+submodule.fetchJobs::
+   Specifies how many submodules are fetched/cloned at the same time.
+   A positive integer allows up to that number of submodules fetched
+   in parallel. A value of 0 will give some reasonable default.
+   If unset, it defaults to 1.
+
 tag.sort::
This variable controls the sort ordering of tags when displayed by
linkgit:git-tag[1]. Without the "--sort=" option provided, the
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 586840d..5aa1c2d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -37,7 +37,7 @@ static int prune = -1; /* unspecified */
 static int all, append, dry_run, force, keep, multiple, update_head_ok, 
verbosity;
 static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 static int tags = TAGS_DEFAULT, unshallow, update_shallow;
-static int max_children = 1;
+static int max_children = -1;
 static const char *depth;
 static const char *upload_pack;
 static struct strbuf default_rla = STRBUF_INIT;
diff --git a/submodule-config.c b/submodule-config.c
index 2841c5e..339b59d 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -32,6 +32,7 @@ enum lookup_type {
 
 static struct submodule_cache cache;
 static int is_cache_init;
+static unsigned long parallel_jobs = -1;
 
 static int config_path_cmp(const struct submodule_entry *a,
   const struct submodule_entry *b,
@@ -233,6 +234,13 @@ static int parse_generic_submodule_config(const char *key,
  const char *value,
  struct parse_config_parameter *me)
 {
+   if (!strcmp(key, "fetchjobs")) {
+   if (!git_parse_ulong(value, ¶llel_jobs)) {
+   warning(_("Error parsing submodule.fetchJobs; 
Defaulting to 1."));
+   parallel_jobs = 1;
+   }
+   }
+
return 0;
 }
 
@@ -479,3 +487,8 @@ void submodule_free(void)
cache_free(&cache);
is_cache_init = 0;
 }
+
+unsigned long config_parallel_submodules(void)
+{
+   return parallel_jobs;
+}
diff --git a/submodule-config.h b/submodule-config.h
index f9e2a29..bab1e8d 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -27,4 +27,6 @@ const struct submodule *submodule_from_path(const unsigned 
char *commit_sha1,
const char *path);
 void submodule_free(void);
 
+unsigned long config_parallel_submodules(void);
+
 #endif /* SUBMODULE_CONFIG_H */
diff --git a/submodule.c b/submodule.c
index b83939c..eb7d54b 100644
--- a/submodule.c
+++ b/submodule.c
@@ -751,6 +751,11 @@ int fetch_populated_submodules(const struct argv_array 
*options,
argv_array_push(&spf.args, "--recurse-submodules-default");
/* default value, "--submodule-prefix" and its value are added later */
 
+   if (max_parallel_jobs < 0)
+   max_parallel_jobs = config_parallel_submodules();
+   if (max_parallel_jobs < 0)
+   max_parallel_jobs = 1;
+
calculate_changed_submodule_paths();
run_processes_parallel(max_parallel_jobs,
   get_next_submodule,
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 1241146..954d0e4 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -471,4 +471,18 @@ test_expect_success "don't fetch submodule when newly 
recorded commits are alrea
test_i18ncmp expect.err actual.err
 '
 
+test_expect_success 'fetching submodules respects parallel settings' '
+   git config fetch.recurseSubmodules true &&
+   (
+   cd downstream &&
+   GIT_TRACE=$(pwd)/trace.out git fetch --jobs 7 &&
+   grep "7 tasks" trace.out &&
+   git config submodule.fetchJobs 8 &&
+   GIT_TRACE=$(pwd)/trace.out git fetch &&
+   grep "8 tasks" trace.out &&
+   GIT_T

[PATCHv8 4/9] submodule-config: slightly simplify lookup_or_create_by_name

2016-02-04 Thread Stefan Beller

No need for a strbuf, when all we use it for, is duplicating a string.

Signed-off-by: Stefan Beller 
---
 submodule-config.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index c08ee7f..e375730 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -165,7 +165,6 @@ static struct submodule *lookup_or_create_by_name(struct 
submodule_cache *cache,
const unsigned char *gitmodules_sha1, const char *name)
 {
struct submodule *submodule;
-   struct strbuf name_buf = STRBUF_INIT;
 
submodule = cache_lookup_name(cache, gitmodules_sha1, name);
if (submodule)
@@ -173,9 +172,7 @@ static struct submodule *lookup_or_create_by_name(struct 
submodule_cache *cache,
 
submodule = xmalloc(sizeof(*submodule));
 
-   strbuf_addstr(&name_buf, name);
-   submodule->name = strbuf_detach(&name_buf, NULL);
-
+   submodule->name = xstrdup(name);
submodule->path = NULL;
submodule->url = NULL;
submodule->update = NULL;
-- 
2.7.0.rc0.41.gd102984.dirty

--
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

[PATCHv8 8/9] submodule update: expose parallelism to the user

2016-02-04 Thread Stefan Beller

Expose possible parallelism either via the "--jobs" CLI parameter or
the "submodule.fetchJobs" setting.

By having the variable initialized to -1, we make sure 0 can be passed
into the parallel processing machine, which will then pick as many parallel
workers as there are CPUs.

Signed-off-by: Stefan Beller 
---
 Documentation/git-submodule.txt |  7 ++-
 builtin/submodule--helper.c | 18 ++
 git-submodule.sh|  9 +
 t/t7406-submodule-update.sh | 12 
 4 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 1572f05..13adebf 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -16,7 +16,7 @@ SYNOPSIS
 'git submodule' [--quiet] deinit [-f|--force] [--] ...
 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
  [-f|--force] [--rebase|--merge] [--reference ]
- [--depth ] [--recursive] [--] [...]
+ [--depth ] [--recursive] [--jobs ] [--] [...]
 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) ]
  [commit] [--] [...]
 'git submodule' [--quiet] foreach [--recursive] 
@@ -377,6 +377,11 @@ for linkgit:git-clone[1]'s `--reference` and `--shared` 
options carefully.
clone with a history truncated to the specified number of revisions.
See linkgit:git-clone[1]
 
+-j ::
+--jobs ::
+   This option is only valid for the update command.
+   Clone new submodules in parallel with as many jobs.
+   Defaults to the `submodule.fetchJobs` option.
 
 ...::
Paths to submodule(s). When specified this will restrict the command
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 90f9dc6..7e01b85 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -433,6 +433,7 @@ static int update_clone_task_finished(int result,
 
 static int update_clone(int argc, const char **argv, const char *prefix)
 {
+   int max_jobs = -1;
struct string_list_item *item;
struct submodule_update_clone pp = SUBMODULE_UPDATE_CLONE_INIT;
 
@@ -453,6 +454,8 @@ static int update_clone(int argc, const char **argv, const 
char *prefix)
OPT_STRING(0, "depth", &pp.depth, "",
   N_("Create a shallow clone truncated to the "
  "specified number of revisions")),
+   OPT_INTEGER('j', "jobs", &max_jobs,
+   N_("parallel jobs")),
OPT__QUIET(&pp.quiet, N_("do't print cloning progress")),
OPT_END()
};
@@ -474,10 +477,17 @@ static int update_clone(int argc, const char **argv, 
const char *prefix)
gitmodules_config();
/* Overlay the parsed .gitmodules file with .git/config */
git_config(git_submodule_config, NULL);
-   run_processes_parallel(1, update_clone_get_next_task,
- update_clone_start_failure,
- update_clone_task_finished,
- &pp);
+
+   if (max_jobs < 0)
+   max_jobs = config_parallel_submodules();
+   if (max_jobs < 0)
+   max_jobs = 1;
+
+   run_processes_parallel(max_jobs,
+  update_clone_get_next_task,
+  update_clone_start_failure,
+  update_clone_task_finished,
+  &pp);
 
if (pp.print_unmatched) {
printf("#unmatched\n");
diff --git a/git-submodule.sh b/git-submodule.sh
index 9f554fb..10c5af9 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -645,6 +645,14 @@ cmd_update()
--depth=*)
depth=$1
;;
+   -j|--jobs)
+   case "$2" in '') usage ;; esac
+   jobs="--jobs=$2"
+   shift
+   ;;
+   --jobs=*)
+   jobs=$1
+   ;;
--)
shift
break
@@ -670,6 +678,7 @@ cmd_update()
${update:+--update "$update"} \
${reference:+--reference "$reference"} \
${depth:+--depth "$depth"} \
+   ${jobs:+$jobs} \
"$@" | {
err=
while read mode sha1 stage just_cloned sm_path
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index dda3929..7fd5142 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -774,4 +774,16 @@ test_expect_success 'submodule update --recursive drops 
module name before recur
 test_i18ngrep "Submodule path .deeper/submodule/subsubmodule.: checked 
out" actual
)
 '
+
+test_expect_success 'submodule update can be run in parallel' '
+   (cd super2 &&
+GI

[PATCHv8 0/9] Expose submodule parallelism to the user

2016-02-04 Thread Stefan Beller

This replaces sb/submodule-parallel-update.
(which is based on sb/submodule-parallel-fetch,
but this series also applies cleanly on master)

* using a "more" correct version of parsing the section title
* fixed the memleaks, free-after-use in
  "git submodule update: have a dedicated helper for cloning"
* reworded documentation.
* another additional patch snuck in, which makes code a bit more readable
  (0004-submodule-config-slightly-simplify-lookup_or_create_.patch)

Thanks,
Stefan

Interdiff to v7:

diff --git a/Documentation/config.txt b/Documentation/config.txt
index eda3276..3b02732 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2647,11 +2647,10 @@ submodule..ignore::
affected by this setting.
 
 submodule.fetchJobs::
-   This is used to determine how many submodules will be
-   fetched/cloned at the same time. Specifying a positive integer
-   allows up to that number of submodules being fetched in parallel.
-   This is used in fetch and clone operations only. A value of 0 will
-   give some reasonable configuration. It defaults to 1.
+   Specifies how many submodules are fetched/cloned at the same time.
+   A positive integer allows up to that number of submodules fetched
+   in parallel. A value of 0 will give some reasonable default.
+   If unset, it defaults to 1.
 
 tag.sort::
This variable controls the sort ordering of tags when displayed by
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 8002187..7e01b85 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -312,33 +312,31 @@ static int update_clone_get_next_task(struct 
child_process *cp,
 
for (; pp->count < pp->list.nr; pp->count++) {
const struct submodule *sub = NULL;
-   const char *displaypath = NULL;
const struct cache_entry *ce = pp->list.entries[pp->count];
+   struct strbuf displaypath_sb = STRBUF_INIT;
struct strbuf sb = STRBUF_INIT;
+   const char *displaypath = NULL;
const char *update_module = NULL;
char *url = NULL;
int needs_cloning = 0;
 
if (ce_stage(ce)) {
if (pp->recursive_prefix)
-   strbuf_addf(err, "Skipping unmerged submodule 
%s/%s\n",
+   strbuf_addf(err,
+   "Skipping unmerged submodule %s/%s\n",
pp->recursive_prefix, ce->name);
else
-   strbuf_addf(err, "Skipping unmerged submodule 
%s\n",
+   strbuf_addf(err,
+   "Skipping unmerged submodule %s\n",
ce->name);
-   continue;
+   goto cleanup_and_continue;
}
 
sub = submodule_from_path(null_sha1, ce->name);
-   if (!sub) {
-   strbuf_addf(err, "BUG: internal error managing 
submodules. "
-   "The cache could not locate '%s'", 
ce->name);
-   pp->print_unmatched = 1;
-   continue;
-   }
 
if (pp->recursive_prefix)
-   displaypath = relative_path(pp->recursive_prefix, 
ce->name, &sb);
+   displaypath = relative_path(pp->recursive_prefix,
+   ce->name, &displaypath_sb);
else
displaypath = ce->name;
 
@@ -349,14 +347,15 @@ static int update_clone_get_next_task(struct 
child_process *cp,
if (!update_module)
update_module = "checkout";
if (!strcmp(update_module, "none")) {
-   strbuf_addf(err, "Skipping submodule '%s'\n", 
displaypath);
-   continue;
+   strbuf_addf(err, "Skipping submodule '%s'\n",
+   displaypath);
+   goto cleanup_and_continue;
}
 
/*
 * Looking up the url in .git/config.
-* We must not fall back to .gitmodules as we only want to 
process
-* configured submodules.
+* We must not fall back to .gitmodules as we only want
+* to process configured submodules.
 */
strbuf_reset(&sb);
strbuf_addf(&sb, "submodule.%s.url", sub->name);
@@ -367,9 +366,11 @@ static int update_clone_get_next_task(struct child_process 
*cp,
 * path have been specified
 */
if (pp->pathspec.nr)
-   strbuf_addf(err, _("Submodule path '%s' not 
initializ

[PATCHv8 5/9] submodule-config: introduce parse_generic_submodule_config

2016-02-04 Thread Stefan Beller

This rewrites parse_config to distinguish between configs specific to
one submodule and configs which apply generically to all submodules.
We do not have generic submodule configs yet, but the next patch will
introduce "submodule.fetchJobs".

Signed-off-by: Stefan Beller 
---
 submodule-config.c | 45 +++--
 1 file changed, 35 insertions(+), 10 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index e375730..2841c5e 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -228,18 +228,22 @@ struct parse_config_parameter {
int overwrite;
 };
 
-static int parse_config(const char *var, const char *value, void *data)
+static int parse_generic_submodule_config(const char *key,
+ const char *var,
+ const char *value,
+ struct parse_config_parameter *me)
 {
-   struct parse_config_parameter *me = data;
-   struct submodule *submodule;
-   int subsection_len, ret = 0;
-   const char *subsection, *key;
-
-   if (parse_config_key(var, "submodule", &subsection,
-&subsection_len, &key) < 0 || !subsection)
-   return 0;
+   return 0;
+}
 
-   subsection = xmemdupz(subsection, subsection_len);
+static int parse_specific_submodule_config(const char *subsection,
+  const char *key,
+  const char *var,
+  const char *value,
+  struct parse_config_parameter *me)
+{
+   int ret = 0;
+   struct submodule *submodule;
submodule = lookup_or_create_by_name(me->cache,
 me->gitmodules_sha1,
 subsection);
@@ -309,6 +313,27 @@ static int parse_config(const char *var, const char 
*value, void *data)
return ret;
 }
 
+static int parse_config(const char *var, const char *value, void *data)
+{
+   struct parse_config_parameter *me = data;
+   int subsection_len, ret;
+   const char *subsection, *key;
+   char *sub;
+
+   if (parse_config_key(var, "submodule", &subsection,
+&subsection_len, &key) < 0)
+   return 0;
+
+   if (!subsection)
+   return parse_generic_submodule_config(key, var, value, me);
+
+   sub = xmemdupz(subsection, subsection_len);
+   ret = parse_specific_submodule_config(sub, key,
+ var, value, me);
+   free(sub);
+   return ret;
+}
+
 static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,
  unsigned char *gitmodules_sha1)
 {
-- 
2.7.0.rc0.41.gd102984.dirty

--
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

git grep argument parser bug

2016-02-04 Thread Dylan Grafmyre

In both ubuntu versions of git 1.9.1 and 2.7.0

git grep '-test'
git grep '--help'

Or any other expressions literal leading with a single dash or double
dash get interpreted as argument flags and not as search expressions.

What I expect is grep results for the literal strings "-test" and "--help"
What i get is git help output informing of wrong argument usage, or
accidentally turning on flags I didn't expect.

Work around; for afflicted users terminating argument parsing with `
-- ` works as it should.

git grep -- '-test'

Confirmed on two Ubuntu [based] systems, Ubuntu Server 14.03 and
LinuxMint 17.3 [Cinnamon]

-- 
Dylan Grafmyre
Associate System Administrator

Email: dyl...@conversica.com
Office: +1 (888) 778 1004
Web:   www.conversica.com
--
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 6/8] git submodule update: have a dedicated helper for cloning

2016-02-04 Thread Junio C Hamano

Stefan Beller  writes:

> I do not think that is a better place as not every consumer of module_list
> (and module_list_compute as the nested function) will need to use the
> submodule caching API.

Ah, if that is the case, then OK.

I was imagining that module-list may someday would return a list of
"struct" instances each of which describes a submodule and has .name
and .path fields, or something, instead of just being a "here is the
path to the submodule, go look it up yourself" interface.  But we
are not there (at least not yet--and I am not saying we agreed that
we would eventually want to go there)

--
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 2/3] Add user.useConfigOnly boolean for when ident shouldn't be guessed

2016-02-04 Thread Junio C Hamano

Dan Aloni  writes:

> Previously, before 5498c57cdd63, many people did the following:
>
>git config --global user.email "(none)"
>
> This was helpful for people with more than one email address,
> targeting different email addresses for different clones.
> as it barred git from creating commit unless the user.email
> config was set in the per-repo config to the correct email
> address.
>
> This commit provides the same functionality by adding a new
> configuration variable.

Thanks.  I'd rather cite an individual commit, not a merge of a
topic, without forcing people to run "git show" only to see the
title of the commit.  Also I'd avoid "was it that really that many
people did so?" discussion by not saying "many people" altogether.

"by adding a new configuration variable" is a bit weak.  Help the
reader by mentioning what it is called and what it does in the same
sentence.

Perhaps like this?

-- >8 --
ident: add user.useConfigOnly boolean for when ident shouldn't be guessed

It used to be that:

   git config --global user.email "(none)"

was a viable way for people to force themselves to set user.email in
each repository.  This was helpful for people with more than one
email address, targeting different email addresses for different
clones, as it barred git from creating commit unless the user.email
config was set in the per-repo config to the correct email address.

A recent change, 19ce497c (ident: keep a flag for bogus
default_email, 2015-12-10), however declared that an explicitly
configured user.email is not bogus, no matter what its value is, so
this hack no longer works.

Provide the same functionality by adding a new configuration
variable user.useConfigOnly; when this variable is set, the
user must explicitly set user.email configuration.

Signed-off-by: Dan Aloni 
Helped-by: Jeff King 
Signed-off-by: Junio C Hamano 
-- 8< --

The logic in the patch is very cleanly written, partly thanks to the
previous step that was a real clean-up.

> @@ -354,6 +357,9 @@ const char *fmt_ident(const char *name, const char *email,
>   fputs(env_hint, stderr);
>   die("unable to auto-detect name (got '%s')", 
> name);
>   }
> + if (strict && ident_use_config_only &&
> + !(ident_config_given & IDENT_NAME_GIVEN))
> + die("user.useConfigOnly set but no name given");
>   }
>   if (!*name) {
>   struct passwd *pw;
> @@ -373,6 +379,8 @@ const char *fmt_ident(const char *name, const char *email,
>   fputs(env_hint, stderr);
>   die("unable to auto-detect email address (got '%s')", 
> email);
>   }
> + if (strict && ident_use_config_only && !(ident_config_given & 
> IDENT_MAIL_GIVEN))
> + die("user.useConfigOnly set but no mail given");
>   }

By folding the line just like you did for "name" above, you do not
have to worry about an overlong line here.

> diff --git a/t/t9904-per-repo-email.sh b/t/t9904-per-repo-email.sh
> new file mode 100755
> index ..9522a640951b
> --- /dev/null
> +++ b/t/t9904-per-repo-email.sh
> @@ -0,0 +1,58 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2016 Dan Aloni
> +#
> +
> +test_description='per-repo forced setting of email address'
> +
> +. ./test-lib.sh
> +
> +prepare () {
> +# Have a non-empty repository
> +rm -fr .git
> +git init
> + echo "Initial" >foo &&

By attempting to reply to a patch, one discovers that the patch has
mixed indentation style ;-)  Indent with tabs.

> +git add foo &&
> +EDITOR=: VISUAL=: git commit -m foo &&

What is the point of these one-shot assignments to the environment
variables?

"git commit -m " does not invoke the editor unless given "-e",
and EDITOR=: is done early in test-lib.sh already, so I am puzzled.

Besides, if you are worried about some stray environment variable,
overriding EDITOR and VISUAL would not guard you against a stray
GIT_EDITOR, which takes the precedence, I think.

> + # Setup a likely user.useConfigOnly use case
> + unset GIT_AUTHOR_NAME &&
> + unset GIT_AUTHOR_EMAIL &&

Doesn't unset fail when the variable is not set (we have sane_unset
helper for that)?

> + test_unconfig --global user.name &&
> + test_unconfig --global user.email &&
> + test_config user.name "test" &&
> + test_unconfig user.email &&
> + test_config_global user.useConfigOnly true
> +}
> +
> +about_to_commit () {
> + echo "Second" >>foo &&
> + git add foo
> +}
> +
> +test_expect_success 'fails committing if clone email is not set' '
> +prepare && about_to_commit &&
> +
> + EDITOR=: VISUAL=: test_must_fail git commit -m msg

test_must_fail, being a shell function, does not follow the usual
"With '

Re: [PATCH v3 00/20] refs backend rebase on pu

2016-02-04 Thread David Turner

On Thu, 2016-02-04 at 17:09 +0700, Duy Nguyen wrote:
> On Wed, Feb 3, 2016 at 3:08 AM, David Turner <
> dtur...@twopensource.com> wrote:
> > Are there any more reviews on this?  I do have some changes from
> > this
> > set, but they're pretty minor so I don't want to post a new one
> > (unless
> > folks would rather see those changes before reviewing).  Let me
> > know.
> 
> I think you need to keep "refs" directory back somehow. Without it
> (e.g. "git init --ref-storage=lmdb" does not create "refs"),
> is_git_directory() from _old_ git versions fails to recognize this is
> a good repository. So instead of dying on finding an unsupported
> repository, setup code keeps on looking for another repository in
> parent directories. If I accidentally run "git add something" with an
> old binary, "something" could be added to a wrong repository. Very
> confusing.

I went ahead and changed this.  Doing so seems to cause old versions of
git to recognize this as a git repo with an unknown version, which is
what we want.  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

Re: [PATCH v4 3/3] ident.c: cleanup wrt ident's source

2016-02-04 Thread Junio C Hamano

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

Re: [PATCH v3 00/20] refs backend rebase on pu

2016-02-04 Thread David Turner

On Thu, 2016-02-04 at 20:39 +, Ramsay Jones wrote:
> 
> On 04/02/16 20:25, David Turner wrote:
> > On Thu, 2016-02-04 at 18:42 +0700, Duy Nguyen wrote:
> > > On Wed, Feb 3, 2016 at 3:08 AM, David Turner <
> > > dtur...@twopensource.com> wrote:
> > > > Are there any more reviews on this?  I do have some changes
> > > > from
> > > > this
> > > > set, but they're pretty minor so I don't want to post a new one
> > > > (unless
> > > > folks would rather see those changes before reviewing).  Let me
> > > > know.
> > > 
> > > Last note. Since this is new code, maybe you can wrap
> > > translatable
> > > strings with _(), basically any visible string that machines do
> > > not
> > > need to recognize.
> > 
> > Fixed, thanks. 
> 
> Another minor point, could you please squash this in:
> 
> diff --git a/refs.c b/refs.c
> index 3d4c0a6..4858d94 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -17,7 +17,6 @@ static const char split_transaction_fail_warning[]
> =
>  /*
>   * We always have a files backend and it is the default.
>   */
> -struct ref_storage_be refs_be_files;
>  static struct ref_storage_be *the_refs_backend = &refs_be_files;
>  /*
>   * List of all available backends
> 
> The above (on Linux, anyway) is a 'tentative definition' of the
> refs_be_files variable and so a common symbol definition is issued
> in refs.o. This then gets 'combined' with the *actual* symbol
> definition in  refs/files-backend.c. So everything 'works', except
> that I have used some unix (let alone non-unix) systems which would
> not output a common symbol for the above and would fail to link
> with a 'multiple symbol definition' error.
> 
> [Also note that an external declaration is already in effect from
> the refs/refs-internal.h header file! ;-) ]
> 
> ATB,
> Ramsay Jones

Fixed, 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

Re: [PATCH v3 00/20] refs backend rebase on pu

2016-02-04 Thread Ramsay Jones

On 04/02/16 20:25, David Turner wrote:
> On Thu, 2016-02-04 at 18:42 +0700, Duy Nguyen wrote:
>> On Wed, Feb 3, 2016 at 3:08 AM, David Turner <
>> dtur...@twopensource.com> wrote:
>>> Are there any more reviews on this?  I do have some changes from
>>> this
>>> set, but they're pretty minor so I don't want to post a new one
>>> (unless
>>> folks would rather see those changes before reviewing).  Let me
>>> know.
>>
>> Last note. Since this is new code, maybe you can wrap translatable
>> strings with _(), basically any visible string that machines do not
>> need to recognize.
> 
> Fixed, thanks. 

Another minor point, could you please squash this in:

diff --git a/refs.c b/refs.c
index 3d4c0a6..4858d94 100644
--- a/refs.c
+++ b/refs.c
@@ -17,7 +17,6 @@ static const char split_transaction_fail_warning[] =
 /*
  * We always have a files backend and it is the default.
  */
-struct ref_storage_be refs_be_files;
 static struct ref_storage_be *the_refs_backend = &refs_be_files;
 /*
  * List of all available backends

The above (on Linux, anyway) is a 'tentative definition' of the
refs_be_files variable and so a common symbol definition is issued
in refs.o. This then gets 'combined' with the *actual* symbol
definition in  refs/files-backend.c. So everything 'works', except
that I have used some unix (let alone non-unix) systems which would
not output a common symbol for the above and would fail to link
with a 'multiple symbol definition' error.

[Also note that an external declaration is already in effect from
the refs/refs-internal.h header file! ;-) ]

ATB,
Ramsay Jones
--
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 v2 7/7] convert.c: simplify text_stat

2016-02-04 Thread Junio C Hamano

tbo...@web.de writes:

>  static int convert_is_binary(unsigned long size, const struct text_stat 
> *stats)
>  {
> - if (stats->cr != stats->crlf)
> + if (stats->lonecr)
>   return 1;

This is an equivalent conversion, but...

> @@ -266,8 +267,8 @@ static int crlf_to_git(const char *path, const char *src, 
> size_t len,
>  
>   check_safe_crlf(path, crlf_action, &stats, checksafe);
>  
> - /* Optimization: No CR? Nothing to convert, regardless. */
> - if (!stats.cr)
> + /* Optimization: No CRLF? Nothing to convert, regardless. */
> + if (!stats.crlf)
>   return 0;

... this is not.  In fact this is even better, as we used to try the
remainder of the function when we saw a lone CR, but we no longer
do.  I am of course assuming that the original turned into a no-op
if we had a lone CR in the input--otherwise this patch changes the
behaviour.

However, I see this comment after the function returns with the
above optimization:

   /*
* If we guessed, we already know we rejected a file with
* lone CR, and we can strip a CR without looking at what
* follow it.
*/

So the code around there used to have a reason to worry about a lone
CR (i.e. it didn't want to lose them).  With the change in this hunk
for the "optimization", it no longer is necessary to do so, i.e. we
know we do not have a lone CR so every CR can be stripped because it
must be followed by LF, isn't it?

But I do not see a matching change to simplify that here.  Am I
following the current code incorrectly or something?

Puzzled...

>   /*
> @@ -315,21 +316,16 @@ static int crlf_to_worktree(const char *path, const 
> char *src, size_t len,
>   gather_stats(src, len, &stats);
>  
>   /* No LF? Nothing to convert, regardless. */
> - if (!stats.lf)
> - return 0;
> -
> - /* Was it already in CRLF format? */
> - if (stats.lf == stats.crlf)
> + if (!stats.lonelf)

Doesn't the comment above need adjustment?

>   return 0;

Otherwise, the output side looks correct to me.
--
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 v3 00/20] refs backend rebase on pu

2016-02-04 Thread David Turner

On Thu, 2016-02-04 at 18:42 +0700, Duy Nguyen wrote:
> On Wed, Feb 3, 2016 at 3:08 AM, David Turner <
> dtur...@twopensource.com> wrote:
> > Are there any more reviews on this?  I do have some changes from
> > this
> > set, but they're pretty minor so I don't want to post a new one
> > (unless
> > folks would rather see those changes before reviewing).  Let me
> > know.
> 
> Last note. Since this is new code, maybe you can wrap translatable
> strings with _(), basically any visible string that machines do not
> need to recognize.

Fixed, 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

Re: [PATCH 6/8] git submodule update: have a dedicated helper for cloning

2016-02-04 Thread Stefan Beller

On Wed, Feb 3, 2016 at 4:54 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> On Wed, Feb 3, 2016 at 3:24 PM, Junio C Hamano  wrote:
>>> Stefan Beller  writes:
>>>
 + if (ce_stage(ce)) {
 + if (pp->recursive_prefix)
 + strbuf_addf(err, "Skipping unmerged 
 submodule %s/%s\n",
 + pp->recursive_prefix, ce->name);
>>
>> As a side question: Do we care about proper visual directory
>> separators in Windows?
>
> You know I do not do Windows ;-)  I'll leave the question for others
> to answer (I am trying not to be one of the the only small number of
> people who review code around here).
>
>> I never run into this BUG after having proper initialization, so maybe it's 
>> not
>> worth carrying this code around. (We have many other places where
>> submodule_from_{path, name} is used unchecked, so why would this place
>> be special?)
>
> That is why I wondered if moudule_list() is a better place to do so.
> That is where the list of everybody works on come from.

I do not think that is a better place as not every consumer of module_list
(and module_list_compute as the nested function) will need to use the
submodule caching API. So these consumers are not interested in possible
bugs in the submodule cache API nor do they want the performance hit
which comes from checking unrelated stuff in there.

As said, I only saw this bug when the cache was not initialized properly,
and then such a bug is to be expected. I'd rather remove it in a reroll.
--
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 v2 4/7] convert.c: Use text_eol_is_crlf()

2016-02-04 Thread Junio C Hamano

tbo...@web.de writes:

> From: Torsten Bögershausen 
>
> Add a helper function to find out, which line endings
> text files should get at checkout, depending on
> core.autocrlf and core.eol
>
> Signed-off-by: Torsten Bögershausen 
> ---
>  convert.c | 25 +
>  1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/convert.c b/convert.c
> index d0c8c66..9ffd043 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -149,6 +149,19 @@ const char *get_wt_convert_stats_ascii(const char *path)
>   return ret;
>  }
>  
> +static int text_eol_is_crlf(void)
> +{
> + if (auto_crlf == AUTO_CRLF_TRUE)
> + return 1;
> + else if (auto_crlf == AUTO_CRLF_INPUT)
> + return 0;
> + if (core_eol == EOL_CRLF)
> + return 1;
> + if (core_eol == EOL_UNSET && EOL_NATIVE == EOL_CRLF)
> + return 1;
> + return 0;
> +}
> +
>  static enum eol output_eol(enum crlf_action crlf_action)
>  {
>   switch (crlf_action) {
> @@ -164,12 +177,7 @@ static enum eol output_eol(enum crlf_action crlf_action)
>   /* fall through */
>   case CRLF_TEXT:
>   case CRLF_AUTO:
> - if (auto_crlf == AUTO_CRLF_TRUE)
> - return EOL_CRLF;
> - else if (auto_crlf == AUTO_CRLF_INPUT)
> - return EOL_LF;
> - else if (core_eol == EOL_UNSET)
> - return EOL_NATIVE;
> + return text_eol_is_crlf() ? EOL_CRLF : EOL_LF;
>   }
>   return core_eol;
>  }
> @@ -1378,8 +1386,9 @@ struct stream_filter *get_stream_filter(const char 
> *path, const unsigned char *s
>   (crlf_action == CRLF_GUESS && auto_crlf == AUTO_CRLF_FALSE))
>   filter = cascade_filter(filter, &null_filter_singleton);
>  
> - else if (output_eol(crlf_action) == EOL_CRLF &&
> -  !(crlf_action == CRLF_AUTO || crlf_action == CRLF_GUESS))
> + else if ((crlf_action == CRLF_AUTO || crlf_action == CRLF_GUESS))
> + ;
> + else if (output_eol(crlf_action) == EOL_CRLF)
>   filter = cascade_filter(filter, lf_to_crlf_filter());
>  
>   return filter;

I am a bit slow today so let me talk this change through aloud.

The condition under which we called cascade_filter() used to be that
output_eol(crlf_action) yields EOL_CRLF and crlf_action is not set
to one of these two values.  Now, we say if crlf_action is one of
these two values, we wouldn't call it, and then we check
output_eol().

So they do the same thing, even though it smells that this change is
outside the scope of "Add a helper and use it" theme of the patch.

While I do not think this new "split" conditions particularly easier
to read compared to the previous one, if you plan to do something
different in later patches when crlf_action is set to specific
values, ignoring what output_eol() says, a patch to implement such a
change would become easier to understand what is going on with this
preparatory rewrite.  So if such a preparatory rewrite is coming (I
haven't checked 5-7/7 yet), perhaps have this hunk as a single patch
that is separate from "add a helper text_eol_is_crlf()" patch.

By the way, your new 1/7 has s/: Remove/: remove/ applied to the
subject, but not other ones like this one.  Intended, or you forgot
to do s/: Use/: use/ here?

--
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 v3 15/20] init: allow alternate backends to be set for new repos

2016-02-04 Thread David Turner

On Thu, 2016-02-04 at 16:48 +0700, Duy Nguyen wrote:
> (picking a random series version because I didn't follow it closely)
> 
> On Thu, Jan 14, 2016 at 11:26 PM, David Turner <
> dtur...@twopensource.com> wrote:
> > +   if (requested_ref_storage_backend)
> > +   ref_storage_backend =
> > requested_ref_storage_backend;
> > +   if (strcmp(ref_storage_backend, "files")) {
> > +   git_config_set("extensions.refStorage",
> > ref_storage_backend);
> > +   git_config_set("core.repositoryformatversion",
> > ref_storage_backend);
> > +#ifdef USE_LIBLMDB
> > +   register_ref_storage_backend(&refs_be_lmdb);
> > +#endif
> > +   set_ref_storage_backend(ref_storage_backend);
> > +   repo_version = 1;
> > +   }
> > +
> > +   if (refs_init_db(&err, shared_repository))
> > +   die("failed to set up refs db: %s", err.buf);
> > +
> 
> I was surprised that "git init --ref-storage=lmdb abc" ran
> successfully even on non-lmdb build. Of course running any commands
> in
> the new repo will fail, suggesting to rebuild with lmdb. But should
> "git init" fail in the first place? I see Thomas' comment about this
> block, but not sure why it still passes for me. It does not catch
> unrecognized backend names either.

This was broken in this patchset and is fixed in the set I've got in
-progress (with a test).

> Also it would be nice if we could hide #ifdef USE_LIBLMDB (here and
> in
> config.c) away, maybe in refs directory. I imagine a new backend
> would
> need the same set of #ifdef. Spreading them across files does not
> sound ideal.

Good point. Moved to a single one in refs.c.  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

Re: [PATCH v2 1/7] t0027: Add tests for get_stream_filter()

2016-02-04 Thread Junio C Hamano

tbo...@web.de writes:

> + while test "$#" != 0
> + do
> + case "$1" in
> + auto)echo '*.txt text=auto' ;;
> + i) echo '* ident' ;;
> + text)echo '*.txt text' ;;
> + -text) echo '*.txt -text' ;;
> + crlf)  echo '*.txt eol=crlf' ;;
> + lf)echo '*.txt eol=lf' ;;
> + "") echo '' ;;
> + *)
> + echo >&2 invalid attribute: $attr
> + exit 1
> + ;;

Perhaps use the right style as you are rewriting pretty much
everything in this helper function?  case/esac aligns with the
labels of case arms, and commands under each arm are indented one
level down, i.e.

case "$var" in
$arm1)  ... do short thing ...;;
$arm2)
... do a lot of
... things
;;
esac


I still do not see the value of accepting a cryptic 'i' (or an empty
string) to this thing, though.  Also what $attr does the error
message refer to now?  Perhaps it wants to refer to "$1" (quoted)
instead?
--
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 14/15] git-am.sh: replace using expr for arithmetic operations with the equivalent shell builtin

2016-02-04 Thread Stefan Beller

On which version did you base your patches?

git-am.sh is no more since 2015-08-04,
(it was moved to contrib/examples/git-am.sh as part of the rewrite of am in C)
--
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 0/3] Fix $((...)) coding style

2016-02-04 Thread Junio C Hamano

Junio C Hamano  writes:

> One thing I was wondering about the $(( ... )) syntax while reading
> this thread was about the SP around the expression, i.e.
>
>   var=$(( $term1 * $term2 + $term3 ))
>
> vs
>
>   var=$(($term1 * $term2 + $term3))
>
> I personally do not have strong preference between the two, but I
> have a vague impression that we preferred the former because
> somebody in the past gave us a good explanation why we should.
>
> "git grep" however seems to tell us that we are not clearly decided
> between the two, so I probably am misremembering it and there is no
> preference either way.

One thing that is somewhat related is that we would want to avoid
writing things like this by mistake:

var=$((cmd1 && cmd2) | cmd3)

which is not meant to be an arithmetic expansion, but is a command
substitution that happens to have a subshell at the head of the
pipeline.  I think bash gets this right, but some shells (e.g. dash)
tries to parse this as an arithmetic expansion upon seeing "$((" and
fails to parse it, waiting forever until it sees the matching "))".
So we write it like this to make it safer:

var=$( (cmd1 && cmd2) | cmd3 )

Perhaps having a SP after $(( of a real arithmetic expression, i.e.

var=$(( ... anything ... ))

makes it easier to tell these two apart?  I dunno.
--
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 0/3] Fix $((...)) coding style

2016-02-04 Thread Junio C Hamano

John Keeping  writes:

> I avoided quoting CodingGuidelines in my previous message, but it says:
>
>  - Fixing style violations while working on a real change as a
>preparatory clean-up step is good, but otherwise avoid useless code
>churn for the sake of conforming to the style.
>
>"Once it _is_ in the tree, it's not really worth the patch noise to
>go and fix it up."
>Cf. http://article.gmane.org/gmane.linux.kernel/943020
>
>> Also, we *did* document the policy in the CodingGuidelines:
>> 
>>  As for more concrete guidelines, just imitate the existing code
>
> This is the first point I made.  To take one example, in
> git-filter-branch.sh there are five occurrences of the sequence "$((";
> your patch changes four of them to remove spaces.  If we standardise on
> having spaces only one needs to change:

I agree that our codebase seems to have a moderately strong
preference for having SP around binary operators, i.e.

$term1 * $term2 + $term3

over not having one:

$term1*$term2+$term3

and I think that is OK to declare it as our style, primarily because
that is how we encourage to write our expressions in C, as you
mentioned earlier.

One thing I was wondering about the $(( ... )) syntax while reading
this thread was about the SP around the expression, i.e.

var=$(( $term1 * $term2 + $term3 ))

vs

var=$(($term1 * $term2 + $term3))

I personally do not have strong preference between the two, but I
have a vague impression that we preferred the former because
somebody in the past gave us a good explanation why we should.

"git grep" however seems to tell us that we are not clearly decided
between the two, so I probably am misremembering it and there is no
preference either way.

In any case, it is good to catch a patch that mixes style changes
and other things during a review, and it also is good to clean up
the style before you start working on a specific part of the code
to make gradual improvement without stepping on others' toes.

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

Re: [PATCH 14/15] git-am.sh: replace using expr for arithmetic operations with the equivalent shell builtin

2016-02-04 Thread Eric Sunshine

On Thu, Feb 4, 2016 at 2:33 PM, Junio C Hamano  wrote:
> As pointed out already, quoting of "$this" inside the arithmetic
> expansion would not work very well, so [14/15] needs fixing.
>
> I do not see 01/15 thru 13/15 here, by the way.  Is it just me?

I didn't receive them either, and they don't seem to be on gmane...
--
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 14/15] git-am.sh: replace using expr for arithmetic operations with the equivalent shell builtin

2016-02-04 Thread Junio C Hamano

As pointed out already, quoting of "$this" inside the arithmetic
expansion would not work very well, so [14/15] needs fixing.

I do not see 01/15 thru 13/15 here, by the way.  Is it just me?

--
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 v3 19/20] refs: add LMDB refs backend

2016-02-04 Thread David Turner

On Thu, 2016-02-04 at 16:58 +0700, Duy Nguyen wrote:
> On Thu, Jan 14, 2016 at 11:26 PM, David Turner <
> dtur...@twopensource.com> wrote:
> > diff --git a/refs/lmdb-backend.c b/refs/lmdb-backend.c
> > new file mode 100644
> > index 000..470c79f
> > --- /dev/null
> > +++ b/refs/lmdb-backend.c
> > @@ -0,0 +1,2051 @@
> > +/*
> > + * This file implements a lmdb backend for refs.
> > + *
> > + * The design of this backend relies on lmdb's write lock -- that
> > is, any
> > + * write transaction blocks all other writers.  Thus, as soon as a
> > ref
> > + * transaction is opened, we know that any values we read won't
> > + * change out from under us, and we have a fully-consistent view
> > of the
> > + * database.
> > + *
> > + * We store the content of refs including the trailing \0 so that
> > + * standard C string functions can handle them.  Just like struct
> > + * strbuf.
> > + */
> > +#include "../cache.h"
> > +#include 
> > +#include "../object.h"
> > +#include "../refs.h"
> > +#include "refs-internal.h"
> > +#include "../tag.h"
> > +#include "../lockfile.h"
> > +
> > +static struct trace_key db_trace = TRACE_KEY_INIT(LMDB);
> 
> Super nit:
> 
> CC refs/lmdb-backend.o
> refs/lmdb-backend.c:22:25: cảnh báo: ‘db_trace’ defined but not used
> [-Wunused-variable]

Fixed, 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

Re: [PATCH v3 13/20] refs: resolve symbolic refs first

2016-02-04 Thread David Turner

On Thu, 2016-02-04 at 02:37 -0500, Jeff King wrote:
> On Thu, Jan 14, 2016 at 11:26:10AM -0500, David Turner wrote:
> 
> > +static int dereference_symrefs(struct ref_transaction
> > *transaction,
> > +  struct strbuf *err)
> > +{
> > +   int i;
> > +   int nr = transaction->nr;
> > +
> > +   for (i = 0; i < nr; i++) {
> > +   struct ref_update *update = transaction
> > ->updates[i];
> > +   const char *resolved;
> > +   unsigned char sha1[20];
> > +   int resolve_flags = 0;
> > +   int mustexist = (update->old_sha1 &&
> > +!is_null_sha1(update->old_sha1));
> 
> Coverity complains about this last line, as "update->old_sha1" is an
> array. I think you want to check "update->flags & REF_HAVE_OLD"
> instead?

Yeah, that's right.  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

Re: COPYING tabs vs whitespaces

2016-02-04 Thread Junio C Hamano

Petr Stodulka  writes:

> I found that license file COPYING is different as compared with
> http://www.gnu.org/licenses/gpl-2.0.txt If I pass over with
> Linus's preamble, change is only about whitespaces - tabs
> vs. space.  Probably it's minor non-essential change, but some
> projects do this change, so rather I ask about that.

Interesting.  I cannot quite connect "some projects do this change"
and "so rather I ask".  Are you asking why this project changed it?

After running "diff" between the two, I think the changes are only
on the indented section title lines, and "git blame" tells us that
the section title lines in the copy we have has always been that way
since Linus added to it at 075b845a (Add a COPYING notice, making it
explicit that the license is GPLv2., 2005-04-11).

So, perhaps the copy Linus got originally had leading runs of spaces
that are multiples-of-8-long unexpanded to leading tabs back then,
or perhaps he did that unexpanding himself.

--
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] git-completion.bash: always swallow error output of for-each-ref

2016-02-04 Thread Junio C Hamano

Johannes Schindelin  writes:

>> $ time git for-each-ref --format='%(refname:short)' refs/tags >/dev/null
>> 
>> real0m0.009s
>> user0m0.004s
>> sys 0m0.004s
>
> And the timings in the ticket I mentioned above are not pretty small:
> 0.055s vs 1.341s
>
>> The upcoming refname:strip does much better:
>> 
>> $ time git for-each-ref --format='%(refname:strip=2)' refs/tags >/dev/null
>> 
>> real0m0.004s
>> user0m0.000s
>> sys 0m0.004s
>
> This is funny: after reading the commit message at
> https://github.com/git/git/commit/0571979b it eludes me why strip=2 should
> be so much faster than short...

"short" tries to ensure that the result is not ambiguous within the
repository, so when asked to shorten refs/heads/foo, it needs to
check if refs/tags/foo exists.  "strip=2" textually strips two
levels from the top without worrying about ambiguity across
different hierarchies.

--
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

git worktree (was: [PATCH v3 1/6] worktree: new repo extension to manage worktree behaviors)

2016-02-04 Thread Stefan Monnier

>> As a heavy user of the git-new-worktree "hack / script", is there
>> something I can do to help "get more experience"?
> You can try out "git worktree" command (in "lab" environment) and see
> what's missing, what use cases of yours it does not support.

Cool, didn't know about it, and it's even already in Debian testing!
Using it right now.  So far so good,


Stefan

--
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 v2 3/7] convert.c: Remove input_crlf_action()

2016-02-04 Thread tboegi

From: Torsten Bögershausen 

Integrate the code of input_crlf_action() into convert_attrs(),
so that ca.crlf_action is always valid after calling convert_attrs().
Keep a copy of crlf_action in attr_action, this is needed for
get_convert_attr_ascii().

Remove eol_attr from struct conv_attrs, as it is now used temporally.

Signed-off-by: Torsten Bögershausen 
---
 convert.c | 37 ++---
 1 file changed, 14 insertions(+), 23 deletions(-)

diff --git a/convert.c b/convert.c
index a24c2a2..d0c8c66 100644
--- a/convert.c
+++ b/convert.c
@@ -746,21 +746,10 @@ static int git_path_check_ident(struct git_attr_check 
*check)
return !!ATTR_TRUE(value);
 }
 
-static enum crlf_action input_crlf_action(enum crlf_action text_attr, enum eol 
eol_attr)
-{
-   if (text_attr == CRLF_BINARY)
-   return CRLF_BINARY;
-   if (eol_attr == EOL_LF)
-   return CRLF_INPUT;
-   if (eol_attr == EOL_CRLF)
-   return CRLF_CRLF;
-   return text_attr;
-}
-
 struct conv_attrs {
struct convert_driver *drv;
-   enum crlf_action crlf_action;
-   enum eol eol_attr;
+   enum crlf_action attr_action; /* What attr says */
+   enum crlf_action crlf_action; /* When no attr is set, use core.autocrlf 
*/
int ident;
 };
 
@@ -782,16 +771,23 @@ static void convert_attrs(struct conv_attrs *ca, const 
char *path)
}
 
if (!git_check_attr(path, NUM_CONV_ATTRS, ccheck)) {
-   ca->crlf_action = git_path_check_crlf( ccheck + 4);
+   enum eol eol_attr;
+   ca->crlf_action = git_path_check_crlf(ccheck + 4);
if (ca->crlf_action == CRLF_GUESS)
ca->crlf_action = git_path_check_crlf(ccheck + 0);
+   ca->attr_action = ca->crlf_action;
ca->ident = git_path_check_ident(ccheck + 1);
ca->drv = git_path_check_convert(ccheck + 2);
-   ca->eol_attr = git_path_check_eol(ccheck + 3);
+   if (ca->crlf_action == CRLF_BINARY)
+   return;
+   eol_attr = git_path_check_eol(ccheck + 3);
+   if (eol_attr == EOL_LF)
+   ca->crlf_action = CRLF_INPUT;
+   else if (eol_attr == EOL_CRLF)
+   ca->crlf_action = CRLF_CRLF;
} else {
ca->drv = NULL;
ca->crlf_action = CRLF_GUESS;
-   ca->eol_attr = EOL_UNSET;
ca->ident = 0;
}
 }
@@ -818,11 +814,9 @@ int would_convert_to_git_filter_fd(const char *path)
 const char *get_convert_attr_ascii(const char *path)
 {
struct conv_attrs ca;
-   enum crlf_action crlf_action;
 
convert_attrs(&ca, path);
-   crlf_action = input_crlf_action(ca.crlf_action, ca.eol_attr);
-   switch (crlf_action) {
+   switch (ca.attr_action) {
case CRLF_GUESS:
return "";
case CRLF_BINARY:
@@ -861,7 +855,6 @@ int convert_to_git(const char *path, const char *src, 
size_t len,
src = dst->buf;
len = dst->len;
}
-   ca.crlf_action = input_crlf_action(ca.crlf_action, ca.eol_attr);
ret |= crlf_to_git(path, src, len, dst, ca.crlf_action, checksafe);
if (ret && dst) {
src = dst->buf;
@@ -882,7 +875,6 @@ void convert_to_git_filter_fd(const char *path, int fd, 
struct strbuf *dst,
if (!apply_filter(path, NULL, 0, fd, dst, ca.drv->clean))
die("%s: clean filter '%s' failed", path, ca.drv->name);
 
-   ca.crlf_action = input_crlf_action(ca.crlf_action, ca.eol_attr);
crlf_to_git(path, dst->buf, dst->len, dst, ca.crlf_action, checksafe);
ident_to_git(path, dst->buf, dst->len, dst, ca.ident);
 }
@@ -912,7 +904,6 @@ static int convert_to_working_tree_internal(const char 
*path, const char *src,
 * is a smudge filter.  The filter might expect CRLFs.
 */
if (filter || !normalizing) {
-   ca.crlf_action = input_crlf_action(ca.crlf_action, ca.eol_attr);
ret |= crlf_to_worktree(path, src, len, dst, ca.crlf_action);
if (ret) {
src = dst->buf;
@@ -1381,7 +1372,7 @@ struct stream_filter *get_stream_filter(const char *path, 
const unsigned char *s
if (ca.ident)
filter = ident_filter(sha1);
 
-   crlf_action = input_crlf_action(ca.crlf_action, ca.eol_attr);
+   crlf_action = ca.crlf_action;
 
if ((crlf_action == CRLF_BINARY) || (crlf_action == CRLF_INPUT) ||
(crlf_action == CRLF_GUESS && auto_crlf == AUTO_CRLF_FALSE))
-- 
2.7.0.303.g2c4f448.dirty

--
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 v2 6/7] convert.c: Refactor crlf_action

2016-02-04 Thread tboegi

From: Torsten Bögershausen 

Refactor the determination and usage of crlf_action.
Today, when no attributes are set on a file,
crlf_action is set to CRLF_GUESS, and later, CRLF_GUESS is used as an
indication that core.autocrlf is not false and that some automatic eol
conversion is needed.
Make more clear, what is what, by defining:

- CRLF_UNDEFINED : No attributes set. Temparally used, until core.autocrlf
   and core.eol is evaluated and one of CRLF_BINARY,
   CRLF_AUTO_INPUT or CRLF_AUTO_CRLF is selected
- CRLF_BINARY: No processing of line endings.
- CRLF_TEXT  : attribute "text" is set, line endings are processed.
- CRLF_TEXT_INPUT: attribute "input" or "eol=lf" is set. This implies text.
- CRLF_TEXT_CRLF : attribute "eol=crlf" is set. This implies text.
- CRLF_AUTO  : attribute "auto" is set.
- CRLF_AUTO_INPUT: No attributes, core.autocrlf=input
- CRLF_AUTO_CRLF : No attributes, core.autocrlf=true

Signed-off-by: Torsten Bögershausen 
---
 convert.c | 77 ++-
 1 file changed, 46 insertions(+), 31 deletions(-)

diff --git a/convert.c b/convert.c
index 4e33db1..dd9646a 100644
--- a/convert.c
+++ b/convert.c
@@ -19,12 +19,14 @@
 #define CONVERT_STAT_BITS_BIN   0x4
 
 enum crlf_action {
-   CRLF_GUESS = -1,
-   CRLF_BINARY = 0,
+   CRLF_UNDEFINED,
+   CRLF_BINARY,
CRLF_TEXT,
-   CRLF_INPUT,
-   CRLF_CRLF,
-   CRLF_AUTO
+   CRLF_TEXT_INPUT,
+   CRLF_TEXT_CRLF,
+   CRLF_AUTO,
+   CRLF_AUTO_INPUT,
+   CRLF_AUTO_CRLF
 };
 
 struct text_stat {
@@ -167,18 +169,19 @@ static enum eol output_eol(enum crlf_action crlf_action)
switch (crlf_action) {
case CRLF_BINARY:
return EOL_UNSET;
-   case CRLF_CRLF:
+   case CRLF_TEXT_CRLF:
return EOL_CRLF;
-   case CRLF_INPUT:
+   case CRLF_TEXT_INPUT:
return EOL_LF;
-   case CRLF_GUESS:
-   if (!auto_crlf)
-   return EOL_UNSET;
-   /* fall through */
+   case CRLF_UNDEFINED:
+   case CRLF_AUTO_CRLF:
+   case CRLF_AUTO_INPUT:
case CRLF_TEXT:
case CRLF_AUTO:
+   /* fall through */
return text_eol_is_crlf() ? EOL_CRLF : EOL_LF;
}
+   warning("Illegal crlf_action %d\n", (int)crlf_action);
return core_eol;
 }
 
@@ -247,11 +250,11 @@ static int crlf_to_git(const char *path, const char *src, 
size_t len,
 
gather_stats(src, len, &stats);
 
-   if (crlf_action == CRLF_AUTO || crlf_action == CRLF_GUESS) {
+   if (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_INPUT || 
crlf_action == CRLF_AUTO_CRLF) {
if (convert_is_binary(len, &stats))
return 0;
 
-   if (crlf_action == CRLF_GUESS) {
+   if (crlf_action == CRLF_AUTO_INPUT || crlf_action == 
CRLF_AUTO_CRLF) {
/*
 * If the file in the index has any CR in it, do not 
convert.
 * This is the new safer autocrlf handling.
@@ -278,7 +281,7 @@ static int crlf_to_git(const char *path, const char *src, 
size_t len,
if (strbuf_avail(buf) + buf->len < len)
strbuf_grow(buf, len - buf->len);
dst = buf->buf;
-   if (crlf_action == CRLF_AUTO || crlf_action == CRLF_GUESS) {
+   if (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_INPUT || 
crlf_action == CRLF_AUTO_CRLF) {
/*
 * If we guessed, we already know we rejected a file with
 * lone CR, and we can strip a CR without looking at what
@@ -319,8 +322,8 @@ static int crlf_to_worktree(const char *path, const char 
*src, size_t len,
if (stats.lf == stats.crlf)
return 0;
 
-   if (crlf_action == CRLF_AUTO || crlf_action == CRLF_GUESS) {
-   if (crlf_action == CRLF_GUESS) {
+   if (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_INPUT || 
crlf_action == CRLF_AUTO_CRLF) {
+   if (crlf_action == CRLF_AUTO_INPUT || crlf_action == 
CRLF_AUTO_CRLF) {
/* If we have any CR or CRLF line endings, we do not 
touch it */
/* This is the new safer autocrlf-handling */
if (stats.cr > 0 || stats.crlf > 0)
@@ -708,16 +711,16 @@ static enum crlf_action git_path_check_crlf(struct 
git_attr_check *check)
const char *value = check->value;
 
if (ATTR_TRUE(value))
-   return CRLF_TEXT;
+   return text_eol_is_crlf() ? CRLF_TEXT_CRLF : CRLF_TEXT_INPUT;
else if (ATTR_FALSE(value))
return CRLF_BINARY;
else if (ATTR_UNSET(value))
;
else if (!strcmp(value, "input"))
-   return CRLF_INPUT;
+   return CRLF_TEXT_INPUT;
else if (!strcmp(value, "auto"))
retu

[PATCH v2 5/7] convert: auto_crlf=false and no attributes set: same as binary

2016-02-04 Thread tboegi

From: Torsten Bögershausen 

When core.autocrlf is set to false, and no attributes are set, the file
is treated as binary.
Simplify the logic and remove duplicated code when dealing with
(crlf_action == CRLF_GUESS && auto_crlf == AUTO_CRLF_FALSE) by setting
crlf_action=CRLF_BINARY already in convert_attrs().

Signed-off-by: Torsten Bögershausen 
---
 convert.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/convert.c b/convert.c
index 9ffd043..4e33db1 100644
--- a/convert.c
+++ b/convert.c
@@ -235,7 +235,6 @@ static int crlf_to_git(const char *path, const char *src, 
size_t len,
char *dst;
 
if (crlf_action == CRLF_BINARY ||
-   (crlf_action == CRLF_GUESS && auto_crlf == AUTO_CRLF_FALSE) ||
(src && !len))
return 0;
 
@@ -798,6 +797,8 @@ static void convert_attrs(struct conv_attrs *ca, const char 
*path)
ca->crlf_action = CRLF_GUESS;
ca->ident = 0;
}
+   if (ca->crlf_action == CRLF_GUESS && auto_crlf == AUTO_CRLF_FALSE)
+   ca->crlf_action = CRLF_BINARY;
 }
 
 int would_convert_to_git_filter_fd(const char *path)
@@ -1382,8 +1383,7 @@ struct stream_filter *get_stream_filter(const char *path, 
const unsigned char *s
 
crlf_action = ca.crlf_action;
 
-   if ((crlf_action == CRLF_BINARY) || (crlf_action == CRLF_INPUT) ||
-   (crlf_action == CRLF_GUESS && auto_crlf == AUTO_CRLF_FALSE))
+   if ((crlf_action == CRLF_BINARY) || (crlf_action == CRLF_INPUT))
filter = cascade_filter(filter, &null_filter_singleton);
 
else if ((crlf_action == CRLF_AUTO || crlf_action == CRLF_GUESS))
-- 
2.7.0.303.g2c4f448.dirty

--
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 v2 7/7] convert.c: simplify text_stat

2016-02-04 Thread tboegi

From: Torsten Bögershausen 

Simplify the statistics:
lonecr counts the CR which is not followed by a LF,
lonelf counts the LF which is not preceded by a CR,
crlf counts CRLF combinations.
This simplifies the evaluation of the statistics.

Signed-off-by: Torsten Bögershausen 
---
 convert.c | 58 +++---
 1 file changed, 27 insertions(+), 31 deletions(-)

diff --git a/convert.c b/convert.c
index dd9646a..ebf4d0e 100644
--- a/convert.c
+++ b/convert.c
@@ -31,7 +31,7 @@ enum crlf_action {
 
 struct text_stat {
/* NUL, CR, LF and CRLF counts */
-   unsigned nul, cr, lf, crlf;
+   unsigned nul, lonecr, lonelf, crlf;
 
/* These are just approximations! */
unsigned printable, nonprintable;
@@ -46,13 +46,15 @@ static void gather_stats(const char *buf, unsigned long 
size, struct text_stat *
for (i = 0; i < size; i++) {
unsigned char c = buf[i];
if (c == '\r') {
-   stats->cr++;
-   if (i+1 < size && buf[i+1] == '\n')
+   if (i+1 < size && buf[i+1] == '\n') {
stats->crlf++;
+   i++;
+   } else
+   stats->lonecr++;
continue;
}
if (c == '\n') {
-   stats->lf++;
+   stats->lonelf++;
continue;
}
if (c == 127)
@@ -86,7 +88,7 @@ static void gather_stats(const char *buf, unsigned long size, 
struct text_stat *
  */
 static int convert_is_binary(unsigned long size, const struct text_stat *stats)
 {
-   if (stats->cr != stats->crlf)
+   if (stats->lonecr)
return 1;
if (stats->nul)
return 1;
@@ -98,19 +100,18 @@ static int convert_is_binary(unsigned long size, const 
struct text_stat *stats)
 static unsigned int gather_convert_stats(const char *data, unsigned long size)
 {
struct text_stat stats;
+   int ret = 0;
if (!data || !size)
-   return 0;
+   return ret;
gather_stats(data, size, &stats);
if (convert_is_binary(size, &stats))
-   return CONVERT_STAT_BITS_BIN;
-   else if (stats.crlf && stats.crlf == stats.lf)
-   return CONVERT_STAT_BITS_TXT_CRLF;
-   else if (stats.crlf && stats.lf)
-   return CONVERT_STAT_BITS_TXT_CRLF | CONVERT_STAT_BITS_TXT_LF;
-   else if (stats.lf)
-   return CONVERT_STAT_BITS_TXT_LF;
-   else
-   return 0;
+   ret |= CONVERT_STAT_BITS_BIN;
+   if (stats.crlf)
+   ret |= CONVERT_STAT_BITS_TXT_CRLF;
+   if (stats.lonelf)
+   ret |=  CONVERT_STAT_BITS_TXT_LF;
+
+   return ret;
 }
 
 static const char *gather_convert_stats_ascii(const char *data, unsigned long 
size)
@@ -207,7 +208,7 @@ static void check_safe_crlf(const char *path, enum 
crlf_action crlf_action,
 * CRLFs would be added by checkout:
 * check if we have "naked" LFs
 */
-   if (stats->lf != stats->crlf) {
+   if (stats->lonelf) {
if (checksafe == SAFE_CRLF_WARN)
warning("LF will be replaced by CRLF in 
%s.\nThe file will have its original line endings in your working directory.", 
path);
else /* i.e. SAFE_CRLF_FAIL */
@@ -266,8 +267,8 @@ static int crlf_to_git(const char *path, const char *src, 
size_t len,
 
check_safe_crlf(path, crlf_action, &stats, checksafe);
 
-   /* Optimization: No CR? Nothing to convert, regardless. */
-   if (!stats.cr)
+   /* Optimization: No CRLF? Nothing to convert, regardless. */
+   if (!stats.crlf)
return 0;
 
/*
@@ -315,21 +316,16 @@ static int crlf_to_worktree(const char *path, const char 
*src, size_t len,
gather_stats(src, len, &stats);
 
/* No LF? Nothing to convert, regardless. */
-   if (!stats.lf)
-   return 0;
-
-   /* Was it already in CRLF format? */
-   if (stats.lf == stats.crlf)
+   if (!stats.lonelf)
return 0;
 
+   if (crlf_action == CRLF_AUTO_INPUT || crlf_action == CRLF_AUTO_CRLF) {
+   /* If we have any CRLF line endings, we do not touch it */
+   /* This is the new safer autocrlf-handling */
+   if (stats.crlf)
+   return 0;
+   }
if (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_INPUT || 
crlf_action == CRLF_AUTO_CRLF) {
-   if (crlf_action == CRLF_AUTO_INPUT || crlf_action == 
CRLF_AUTO_CRLF) {
-   /* If we have any CR or CRLF line endings, we do not 
touch it */
-   /* This is the new safer autocrlf-handling */
-   i

[PATCH v2 2/7] convert.c: remove unused parameter 'path'

2016-02-04 Thread tboegi

From: Torsten Bögershausen 

Some functions get a parameter path, but don't use it.
Remove the unused parameter.

Signed-off-by: Torsten Bögershausen 
---
 convert.c | 19 +--
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/convert.c b/convert.c
index 4bb4ec1..a24c2a2 100644
--- a/convert.c
+++ b/convert.c
@@ -696,7 +696,7 @@ static int ident_to_worktree(const char *path, const char 
*src, size_t len,
return 1;
 }
 
-static enum crlf_action git_path_check_crlf(const char *path, struct 
git_attr_check *check)
+static enum crlf_action git_path_check_crlf(struct git_attr_check *check)
 {
const char *value = check->value;
 
@@ -713,7 +713,7 @@ static enum crlf_action git_path_check_crlf(const char 
*path, struct git_attr_ch
return CRLF_GUESS;
 }
 
-static enum eol git_path_check_eol(const char *path, struct git_attr_check 
*check)
+static enum eol git_path_check_eol(struct git_attr_check *check)
 {
const char *value = check->value;
 
@@ -726,8 +726,7 @@ static enum eol git_path_check_eol(const char *path, struct 
git_attr_check *chec
return EOL_UNSET;
 }
 
-static struct convert_driver *git_path_check_convert(const char *path,
-struct git_attr_check *check)
+static struct convert_driver *git_path_check_convert(struct git_attr_check 
*check)
 {
const char *value = check->value;
struct convert_driver *drv;
@@ -740,7 +739,7 @@ static struct convert_driver *git_path_check_convert(const 
char *path,
return NULL;
 }
 
-static int git_path_check_ident(const char *path, struct git_attr_check *check)
+static int git_path_check_ident(struct git_attr_check *check)
 {
const char *value = check->value;
 
@@ -783,12 +782,12 @@ static void convert_attrs(struct conv_attrs *ca, const 
char *path)
}
 
if (!git_check_attr(path, NUM_CONV_ATTRS, ccheck)) {
-   ca->crlf_action = git_path_check_crlf(path, ccheck + 4);
+   ca->crlf_action = git_path_check_crlf( ccheck + 4);
if (ca->crlf_action == CRLF_GUESS)
-   ca->crlf_action = git_path_check_crlf(path, ccheck + 0);
-   ca->ident = git_path_check_ident(path, ccheck + 1);
-   ca->drv = git_path_check_convert(path, ccheck + 2);
-   ca->eol_attr = git_path_check_eol(path, ccheck + 3);
+   ca->crlf_action = git_path_check_crlf(ccheck + 0);
+   ca->ident = git_path_check_ident(ccheck + 1);
+   ca->drv = git_path_check_convert(ccheck + 2);
+   ca->eol_attr = git_path_check_eol(ccheck + 3);
} else {
ca->drv = NULL;
ca->crlf_action = CRLF_GUESS;
-- 
2.7.0.303.g2c4f448.dirty

--
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 v2 4/7] convert.c: Use text_eol_is_crlf()

2016-02-04 Thread tboegi

From: Torsten Bögershausen 

Add a helper function to find out, which line endings
text files should get at checkout, depending on
core.autocrlf and core.eol

Signed-off-by: Torsten Bögershausen 
---
 convert.c | 25 +
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/convert.c b/convert.c
index d0c8c66..9ffd043 100644
--- a/convert.c
+++ b/convert.c
@@ -149,6 +149,19 @@ const char *get_wt_convert_stats_ascii(const char *path)
return ret;
 }
 
+static int text_eol_is_crlf(void)
+{
+   if (auto_crlf == AUTO_CRLF_TRUE)
+   return 1;
+   else if (auto_crlf == AUTO_CRLF_INPUT)
+   return 0;
+   if (core_eol == EOL_CRLF)
+   return 1;
+   if (core_eol == EOL_UNSET && EOL_NATIVE == EOL_CRLF)
+   return 1;
+   return 0;
+}
+
 static enum eol output_eol(enum crlf_action crlf_action)
 {
switch (crlf_action) {
@@ -164,12 +177,7 @@ static enum eol output_eol(enum crlf_action crlf_action)
/* fall through */
case CRLF_TEXT:
case CRLF_AUTO:
-   if (auto_crlf == AUTO_CRLF_TRUE)
-   return EOL_CRLF;
-   else if (auto_crlf == AUTO_CRLF_INPUT)
-   return EOL_LF;
-   else if (core_eol == EOL_UNSET)
-   return EOL_NATIVE;
+   return text_eol_is_crlf() ? EOL_CRLF : EOL_LF;
}
return core_eol;
 }
@@ -1378,8 +1386,9 @@ struct stream_filter *get_stream_filter(const char *path, 
const unsigned char *s
(crlf_action == CRLF_GUESS && auto_crlf == AUTO_CRLF_FALSE))
filter = cascade_filter(filter, &null_filter_singleton);
 
-   else if (output_eol(crlf_action) == EOL_CRLF &&
-!(crlf_action == CRLF_AUTO || crlf_action == CRLF_GUESS))
+   else if ((crlf_action == CRLF_AUTO || crlf_action == CRLF_GUESS))
+   ;
+   else if (output_eol(crlf_action) == EOL_CRLF)
filter = cascade_filter(filter, lf_to_crlf_filter());
 
return filter;
-- 
2.7.0.303.g2c4f448.dirty

--
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 v2 1/7] t0027: Add tests for get_stream_filter()

2016-02-04 Thread tboegi

From: Torsten Bögershausen 

When a filter is configured, a different code-path is used in convert.c
and entry.c via get_stream_filter(), but there are no test cases yet.

Add tests for the filter API by configuring the ident filter.
The result of the SHA1 conversion is not checked, this is already
done in other TC.

Add a parameter to checkout_files() in t0027.
While changing the signature, add another parameter for the eol= attribute.
This is currently unused, tests for e.g.
"* text=auto eol=lf" will be added in a separate commit.

Signed-off-by: Torsten Bögershausen 
---
 Changes since v1:
 - t0027: Comments from Junio, create_gitattributes() and others
 - convert.c: Minor whitespace fixes, changed the commit message
 - Added 7/7: convert.c: simplify text_stat from the TODO stack
t/t0027-auto-crlf.sh | 281 ++-
 1 file changed, 146 insertions(+), 135 deletions(-)

diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
index 504e5a0..1d84ed3 100755
--- a/t/t0027-auto-crlf.sh
+++ b/t/t0027-auto-crlf.sh
@@ -21,38 +21,32 @@ compare_ws_file () {
pfx=$1
exp=$2.expect
act=$pfx.actual.$3
-   tr '\015\000' QN <"$2" >"$exp" &&
-   tr '\015\000' QN <"$3" >"$act" &&
-   test_cmp $exp $act &&
-   rm $exp $act
+   tr '\015\000abcdef0123456789' QN0 <"$2" >"$exp" &&
+   tr '\015\000abcdef0123456789' QN0 <"$3" >"$act" &&
+   test_cmp "$exp" "$act" &&
+   rm "$exp" "$act"
 }
 
 create_gitattributes () {
-   attr=$1
-   case "$attr" in
-   auto)
-   echo "*.txt text=auto" >.gitattributes
-   ;;
-   text)
-   echo "*.txt text" >.gitattributes
-   ;;
-   -text)
-   echo "*.txt -text" >.gitattributes
-   ;;
-   crlf)
-   echo "*.txt eol=crlf" >.gitattributes
-   ;;
-   lf)
-   echo "*.txt eol=lf" >.gitattributes
-   ;;
-   "")
-   echo >.gitattributes
-   ;;
-   *)
-   echo >&2 invalid attribute: $attr
-   exit 1
-   ;;
-   esac
+   {
+   while test "$#" != 0
+   do
+   case "$1" in
+   auto)echo '*.txt text=auto' ;;
+   i) echo '* ident' ;;
+   text)echo '*.txt text' ;;
+   -text) echo '*.txt -text' ;;
+   crlf)  echo '*.txt eol=crlf' ;;
+   lf)echo '*.txt eol=lf' ;;
+   "") echo '' ;;
+   *)
+   echo >&2 invalid attribute: $attr
+   exit 1
+   ;;
+   esac &&
+   shift
+   done
+   } >.gitattributes
 }
 
 create_NNO_files () {
@@ -208,28 +202,30 @@ check_in_repo_NNO () {
 }
 
 checkout_files () {
-   eol=$1
-   crlf=$2
-   attr=$3
-   lfname=$4
-   crlfname=$5
-   lfmixcrlf=$6
-   lfmixcr=$7
-   crlfnul=$8
-   create_gitattributes $attr &&
+   attr=$1 ; shift
+   ident=$1; shift
+   aeol=$1 ; shift
+   crlf=$1 ; shift
+   ceol=$1 ; shift
+   lfname=$1 ; shift
+   crlfname=$1 ; shift
+   lfmixcrlf=$1 ; shift
+   lfmixcr=$1 ; shift
+   crlfnul=$1 ; shift
+   create_gitattributes "$attr" "$ident" &&
git config core.autocrlf $crlf &&
-   pfx=eol_${eol}_crlf_${crlf}_attr_${attr}_ &&
+   pfx=eol_${ceol}_crlf_${crlf}_attr_${attr}_ &&
for f in LF CRLF LF_mix_CR CRLF_mix_LF LF_nul
do
rm crlf_false_attr__$f.txt &&
-   if test -z "$eol"; then
+   if test -z "$ceol"; then
git checkout crlf_false_attr__$f.txt
else
-   git -c core.eol=$eol checkout crlf_false_attr__$f.txt
+   git -c core.eol=$ceol checkout crlf_false_attr__$f.txt
fi
done
 
-   test_expect_success "ls-files --eol $lfname ${pfx}LF.txt" '
+   test_expect_success "ls-files --eol attr=$attr i=$ident $aeol 
core.autocrlf=$crlf core.eol=$ceol" '
test_when_finished "rm expect actual" &&
sort <<-EOF >expect &&
i/crlf w/$(stats_ascii $crlfname) crlf_false_attr__CRLF.txt
@@ -244,19 +240,19 @@ checkout_files () {
sort >actual &&
test_cmp expect actual
'
-   test_expect_success "checkout core.eol=$eol core.autocrlf=$crlf 
gitattributes=$attr file=LF" "
+   test_expect_success "checkout i=$ident attributes=$attr $aeol 
core.autocrlf=$crlf core.eol=$ceol file=LF" "
compare_ws_file $pfx $lfnam

Re: parse_object does check_sha1_signature but not parse_object_buffer?

2016-02-04 Thread Junio C Hamano

Jeff King  writes:

> For local repository operations, most of them are about reading data.
> And there I generally favor performance over extra validation, with the
> caveat that we should always be aware of the tradeoff. An extra
> comparison to make sure we are not going out-of-bounds on a pack .idx
> pointer is cheap. Loading a blob just to make sure its sha1 is valid
> before we mention it in `diff --raw` output is stupid. Checking the sha1
> on objects we are otherwise accessing is somewhere in between. :)
>
> For local write operations, like repacking, we should err on the careful
> side. And I think we do a good job of balancing performance and
> validation there (e.g., we reuse deltas without reconstructing the
> object, but _with_ a crc check on the delta data itself).

OK, that is a reasonable set of rules.  We can say "checkout" is
writing out the contents to the filesystem, and make the "somewhere
in between" be closer to the "error on the careful side".

>> In fact, we do this, which is quite suboptimal:
>> 
>> static int fsck_sha1(const unsigned char *sha1)
>> {
>> struct object *obj = parse_object(sha1);
>> if (!obj) {
>> errors_found |= ERROR_OBJECT;
>> return error("%s: object corrupt or missing",
>>  sha1_to_hex(sha1));
>> }
>> obj->flags |= HAS_OBJ;
>> return fsck_obj(obj);
>> }
>> 
>> This function is called for each loose object file we find in
>> fsck_object_dir(), and there are a few problems:
>> 
>>  * The function parse_object() called from here would issue an error
>>message and returns NULL; then you get another "corrupt or
>>missing" error message, because this code cannot tell from the
>>NULL which is the case.
>> 
>>  * The intent of the callchain to fsck_sha1() is to iterate over
>>loose object files xx/x{38} and validate what is contained in
>>them, but that behaviour is not guaranteed because it calls
>>parse_object(), which may get the object data from a packfile
>>if the loose object is also in the packfile.
>> 
>> This function should instead take "path" (the only caller of this
>> function fsck_loose() has it), read the data in the file, hash the
>> data to validate that it matches "sha1" and then create the object
>> out of that data it read by calling parse_object_buffer().
>
> Yeah, I agree. I think as it is written, we also end up loading the
> loose objects twice (once in parse_object, and then later again in
> fsck_object to do the real work). Your solution would fix that, too.
> ...
> I guess nobody has really noticed either issue, because repositories
> large enough for it to make a difference will usually be packed.

I'll throw it in the leftover-bits list, then ;-)
--
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: git 2.7.0 crashes when top-down memory allocation preference is set

2016-02-04 Thread Klinger, Xia

Thanks for confirming it. I hope a fix is available soon. I am using a very old 
version of Git at the moment to work around this issue, which doesn't comply to 
the requirement of our Stash Git Server from Atlassian.

Best regards,

Xia

-Original Message-
From: Johannes Schindelin [mailto:johannes.schinde...@gmx.de] 
Sent: Thursday, February 04, 2016 10:29 AM
To: Klinger, Xia
Cc: git@vger.kernel.org
Subject: RE: git 2.7.0 crashes when top-down memory allocation preference is set

Hi,

On Thu, 4 Feb 2016, Klinger, Xia wrote:

> I am not sure the Fast Track release means. Do you refer to Windows 10 
> builds (updates)? I am running Windows 7 x64 and haven't gone to 
> Windows 10.

The Fast Track release to which I referred is indeed Windows 10. In the 
meantime, I verified that the problem you described still exists on Windows 10, 
but has nothing to do with the issue 627 in Git for Windows'
bug tracker.

Ciao,
Johannes
--
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 0/3] Fix $((...)) coding style

2016-02-04 Thread John Keeping

On Thu, Feb 04, 2016 at 04:27:49PM +0100, Johannes Schindelin wrote:
> On Thu, 4 Feb 2016, John Keeping wrote:
> 
> > On Thu, Feb 04, 2016 at 02:13:47PM +0100, Johannes Schindelin wrote:
> > > Whatever the outcome, the inconsistency must be fixed.
> > 
> > I disagree.  Unless there are other changes in the same area, the noise
> > isn't worth it.
> > 
> > However, I do think we need to agree on a policy so that new code can be
> > consistent.  This should then be documented in CodingGuidelines.
> 
> If you want to enforce it in the future, how can you possibly be against
> fixing the inconsistency in the existing code? Sorry, I am really unable
> to understand this.

I avoided quoting CodingGuidelines in my previous message, but it says:

 - Fixing style violations while working on a real change as a
   preparatory clean-up step is good, but otherwise avoid useless code
   churn for the sake of conforming to the style.

   "Once it _is_ in the tree, it's not really worth the patch noise to
   go and fix it up."
   Cf. http://article.gmane.org/gmane.linux.kernel/943020

> Also, we *did* document the policy in the CodingGuidelines:
> 
>   As for more concrete guidelines, just imitate the existing code
> 
> So. There you are. By that token, my patch series was the correct thing to
> do because the first arithmetic expression introduced into a shell script
> in Git's source code had no spaces.

This is the first point I made.  To take one example, in
git-filter-branch.sh there are five occurrences of the sequence "$((";
your patch changes four of them to remove spaces.  If we standardise on
having spaces only one needs to change:

$ git grep -F '$((' origin/master -- git-filter-branch.sh
origin/master:git-filter-branch.sh: elapsed=$(($now - 
$start_timestamp))
origin/master:git-filter-branch.sh: remaining=$(( ($commits - 
$count) * $elapsed / $count ))
origin/master:git-filter-branch.sh: next_sample_at=$(( 
($elapsed + 1) * $count / $elapsed ))
origin/master:git-filter-branch.sh: 
next_sample_at=$(($next_sample_at + 1))
origin/master:git-filter-branch.sh: 
git_filter_branch__commit_count=$(($git_filter_branch__commit_count+1))

I chose git-filter-branch.sh simply because it was touched by this patch
set but it is not an outlier in this regard.  Some crude statistics
across all of git.git:

# No space after plus
$ git grep -F '$((' | grep '\+[\$0-9]' | wc -l
34

# With space after plus
$ git grep -F '$((' | grep '\+ [\$0-9]' | wc -l
96
--
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: git 2.7.0 crashes when top-down memory allocation preference is set

2016-02-04 Thread Johannes Schindelin

Hi,

On Thu, 4 Feb 2016, Klinger, Xia wrote:

> I am not sure the Fast Track release means. Do you refer to Windows 10
> builds (updates)? I am running Windows 7 x64 and haven't gone to Windows
> 10.

The Fast Track release to which I referred is indeed Windows 10. In the
meantime, I verified that the problem you described still exists on
Windows 10, but has nothing to do with the issue 627 in Git for Windows'
bug tracker.

Ciao,
Johannes
--
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 0/3] Fix $((...)) coding style

2016-02-04 Thread Johannes Schindelin

Hi John,

On Thu, 4 Feb 2016, John Keeping wrote:

> On Thu, Feb 04, 2016 at 02:13:47PM +0100, Johannes Schindelin wrote:
> > Whatever the outcome, the inconsistency must be fixed.
> 
> I disagree.  Unless there are other changes in the same area, the noise
> isn't worth it.
> 
> However, I do think we need to agree on a policy so that new code can be
> consistent.  This should then be documented in CodingGuidelines.

If you want to enforce it in the future, how can you possibly be against
fixing the inconsistency in the existing code? Sorry, I am really unable
to understand this.

Also, we *did* document the policy in the CodingGuidelines:

As for more concrete guidelines, just imitate the existing code

So. There you are. By that token, my patch series was the correct thing to
do because the first arithmetic expression introduced into a shell script
in Git's source code had no spaces.

At this point I am a bit fed up with the discussion because by now it
feels to me more like a Slashdot thread than a technical discussion with a
direction.

As far as technical direction goes, here is the summary of what I have to
say: I still think that the changes I submitted are good because they seem
to me to reinstate the coding style with which we started out.

And that's really all I have to say about this matter.

Ciao,
Johannes
--
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 0/3] Fix $((...)) coding style

2016-02-04 Thread John Keeping

On Thu, Feb 04, 2016 at 02:13:47PM +0100, Johannes Schindelin wrote:
> On Thu, 4 Feb 2016, John Keeping wrote:
> 
> > Although I don't think the historic context is useful in deciding which
> > direction to go in the future.
> 
> Being a maintainer, I find that argument particularly hard to defend.

I worded that badly, what I wanted to say is that how we got here is
less interesting than where we are.  From a quick bit of grep'ing it
looks to me like where we are is in favour of adding spaces around
binary operators inside $(( )) constructs based on the majority of the
uses in the code as it currently stands.

> But sure, you go ahead and prepare a patch series that turns everything
> around, adding spaces around those operators.
> 
> Whatever the outcome, the inconsistency must be fixed.

I disagree.  Unless there are other changes in the same area, the noise
isn't worth it.

However, I do think we need to agree on a policy so that new code can be
consistent.  This should then be documented in CodingGuidelines.
--
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: git 2.7.0 crashes when top-down memory allocation preference is set

2016-02-04 Thread Klinger, Xia

Hi Johannes,

I am not sure the Fast Track release means. Do you refer to Windows 10 builds 
(updates)? I am running Windows 7 x64 and haven't gone to Windows 10.

Default memory allocation in Windows is bottom-up, which means the lowest 
addresses are consumed the first. This is all fine until the address is over 
4GB on x64 OS with more than 4GB physical memories. Many applications don't 
handle it properly and will crash. Windows provides a registry key to force OS 
allocating memory from the highest address available first and going down the 
chain as a way to test application's memory management. That's what I did on my 
host to test our own software. And it affected Git 2.7.0.2 (latest build for 
Windows from 2/2/2016). 

I had to rollback to Git 1.8.4 to work around this problem. 1.8.4 is a 32-bit 
build not 64-bit build. That's why it worked so far.

Thanks

Xia

-Original Message-
From: Johannes Schindelin [mailto:johannes.schinde...@gmx.de] 
Sent: Thursday, February 04, 2016 2:46 AM
To: Klinger, Xia
Cc: git@vger.kernel.org
Subject: Re: git 2.7.0 crashes when top-down memory allocation preference is set

Hi,

On Wed, 3 Feb 2016, Klinger, Xia wrote:

> When the memory allocation preference is set to be Top-down on Windows 
> and there are more than 4GB memory available, git will crash. The 
> symptom is "Segmentation Fault".

This might be related to 
https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_git-2Dfor-2Dwindows_git_issues_627&d=CwIBAg&c=VCWpAYkS3z1bOCIxc-BPGZarCq9MRCAVxZJE051VqH8&r=67DEVp3lXDRlQoHC5xXHqK4qsTItx9yvwX98R_Hn9tg&m=jBFYZYDLdCLg-WMJPpbvl9rXn4syBv9RAGsbi106m3Y&s=MaQ04pu8kp8pdAOjVSd1PCqEwsYMpxpsFiqZ100gkrk&e=

Do you have access to the Fast Track releases? If so, Windows 10 build
14257 was released to it and purportedly fixes these issues.

Ciao,
Johannes
--
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 0/3] Fix $((...)) coding style

2016-02-04 Thread Johannes Schindelin

Hi John,

On Thu, 4 Feb 2016, John Keeping wrote:

> Although I don't think the historic context is useful in deciding which
> direction to go in the future.

Being a maintainer, I find that argument particularly hard to defend.

But sure, you go ahead and prepare a patch series that turns everything
around, adding spaces around those operators.

Whatever the outcome, the inconsistency must be fixed. I just submitted my
proposed solution.

Ciao,
Johannes
--
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 0/3] Fix $((...)) coding style

2016-02-04 Thread John Keeping

On Thu, Feb 04, 2016 at 01:38:51PM +0100, Johannes Schindelin wrote:
> On Thu, 4 Feb 2016, John Keeping wrote:
> 
> > Using spaces around operators also matches our C coding style.
> 
> Well, by that reasoning you should go the whole nine yards and write
> 
>   lineno = $(( $lineno + 1 ))
> 
> Except you can't. Because shell code is inherently not like C code.

That isn't my main argument, although I do think the (presumed)
rationale for using spaces in C to improve usability applies here as
well, even if the confines of the language don't let us go as far as in
C.

I'm not actually sure whether spaces inside the enclosing $(( and )) are
helpful, we're much less consistent about that than about spaces around
binary operators.  Having looked at t/ as well now, we really do seem to
favour using spaces around the binary operators so I'm further convinced
this series is switching the wrong cases.

> What I found particularly interesting about 180bad3 (rebase -i: respect
> core.commentchar, 2013-02-11) was that it *snuck in* that coding style: it
> *changed* the existing one (without rationale in the commit message, too).

The last version of that patch I can find in the archive [1] doesn't add
the spaces, so I think they must have been part of Junio's fixup
discusses in the following messages.  Although I don't think the
historic context is useful in deciding which direction to go in the
future.

[1] http://article.gmane.org/gmane.comp.version-control.git/216103
--
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 0/3] Fix $((...)) coding style

2016-02-04 Thread Johannes Schindelin

Hi John,

On Thu, 4 Feb 2016, John Keeping wrote:

> Using spaces around operators also matches our C coding style.

Well, by that reasoning you should go the whole nine yards and write

lineno = $(( $lineno + 1 ))

Except you can't. Because shell code is inherently not like C code.

What I found particularly interesting about 180bad3 (rebase -i: respect
core.commentchar, 2013-02-11) was that it *snuck in* that coding style: it
*changed* the existing one (without rationale in the commit message, too).

Ciao,
Johannes
--
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 0/3] Fix $((...)) coding style

2016-02-04 Thread John Keeping

On Thu, Feb 04, 2016 at 01:01:39PM +0100, Johannes Schindelin wrote:
> I noticed through a nearby patch series that was submitted by Elia that
> some of the $((...)) expressions introduced in scripts that I introduced
> to Git's source code did not match the existing code's convention:
> previously these expressions did not contain any spaces, now *some* do.
> 
> This patch series tries to clean that up quickly before even more code
> has to decide which one of the disagreeing coding conventions to use.
> 
> Note: For the sake of getting this patch series out, I skipped t/ and
> contrib/. I do not care much about the latter, but t/ should probably be
> fixed, too.

Should this be going this other way (i.e. standardising on having the
spaces)?

The current state (excluding contrib/ and t/) seems to favour spaces:

$ git grep '\$((' -- ':/' ':!t/' ':!contrib/'
Documentation/CodingGuidelines: - We use Arithmetic Expansion $(( ... )).
Documentation/CodingGuidelines:   of them, as some shells do not grok $((x)) 
while accepting $(($x))
generate-cmdlist.sh:n=$(($n+1))
git-filter-branch.sh:   elapsed=$(($now - $start_timestamp))
git-filter-branch.sh:   remaining=$(( ($commits - $count) * $elapsed / 
$count ))
git-filter-branch.sh:   next_sample_at=$(( ($elapsed + 1) * 
$count / $elapsed ))
git-filter-branch.sh:   next_sample_at=$(($next_sample_at + 1))
git-filter-branch.sh:   
git_filter_branch__commit_count=$(($git_filter_branch__commit_count+1))
git-rebase--interactive.sh: total=$(($new_count + $(git stripspace 
--strip-comments <"$todo" | wc -l)))
git-rebase--interactive.sh: count=$(($(sed -n \
git-rebase--interactive.sh: lineno=$(( $lineno + 1 ))
git-rebase--merge.sh:   msgnum=$(($msgnum + 1))
git-rebase--merge.sh:   eval GITHEAD_$cmt='"${cmt_name##refs/heads/}~$(($end - 
$msgnum))"'
git-rebase--merge.sh:   msgnum=$(($msgnum + 1))
git-rebase--merge.sh:   msgnum=$(($msgnum + 1))
git-submodule.sh:   n=$(($1 + 0)) 2>/dev/null && test "$n" = "$1"
git-submodule.sh:   total_commits=" ($(($total_commits + 
0)))"

I make that 3 without spaces (including the git-rebase--interactive.sh
case that wraps) and 12 that do have spaces around operators.  Using
spaces around operators also matches our C coding style.
--
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 3/3] rebase --merge: adjust $((...)) coding style

2016-02-04 Thread Johannes Schindelin

In 58634db (rebase: Allow merge strategies to be used when rebasing,
2006-06-21) we introduced $((...)) expressions containing spaces,
disagreeing with Git's existing source code's convention.

Naturally, 0bb733c (Use branch names in 'git-rebase -m' conflict hunks.,
2006-12-28) repeated the convention set forth in git-rebase--merge.sh.

Let's make the coding style consistent again.

Cc: Eric Wong 
Cc: Shawn O. Pearce 
Signed-off-by: Johannes Schindelin 
---
 git-rebase--merge.sh | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
index 2cc2a6d..a9bd39a 100644
--- a/git-rebase--merge.sh
+++ b/git-rebase--merge.sh
@@ -48,7 +48,7 @@ continue_merge () {
GIT_PAGER='' git log --format=%s -1 "$cmt"
 
# onto the next patch:
-   msgnum=$(($msgnum + 1))
+   msgnum=$(($msgnum+1))
echo "$msgnum" >"$state_dir/msgnum"
 }
 
@@ -59,7 +59,7 @@ call_merge () {
echo "$cmt" > "$state_dir/current"
hd=$(git rev-parse --verify HEAD)
cmt_name=$(git symbolic-ref HEAD 2> /dev/null || echo HEAD)
-   eval GITHEAD_$cmt='"${cmt_name##refs/heads/}~$(($end - $msgnum))"'
+   eval GITHEAD_$cmt='"${cmt_name##refs/heads/}~$(($end-$msgnum))"'
eval GITHEAD_$hd='$onto_name'
export GITHEAD_$cmt GITHEAD_$hd
if test -n "$GIT_QUIET"
@@ -126,7 +126,7 @@ continue)
 skip)
read_state
git rerere clear
-   msgnum=$(($msgnum + 1))
+   msgnum=$(($msgnum+1))
while test "$msgnum" -le "$end"
do
call_merge "$msgnum"
@@ -144,7 +144,7 @@ write_basic_state
 msgnum=0
 for cmt in $(git rev-list --reverse --no-merges "$revisions")
 do
-   msgnum=$(($msgnum + 1))
+   msgnum=$(($msgnum+1))
echo "$cmt" > "$state_dir/cmt.$msgnum"
 done
 
-- 
2.7.0.windows.1.7.g55a05c8
--
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 1/3] filter-branch: fix style of $((...) construct

2016-02-04 Thread Johannes Schindelin

In 6a9d16a (filter-branch: add passed/remaining seconds on progress,
2015-09-07) we introduced a new style to write $((...) expressions: with
spaces in the arithmetic expression. We actually introduced *two* new
styles, even: some of the $((...)) expressions had spaces after the two
opening and before the two closing parentheses, but others did not.

There had been an arithmetic expression in that script, though, and it
matched the coding style of the rest of Git's source code: $((...))
expressions simply do not contain spaces.

Let's adjust the style to the previously existing code.

Cc: Gabor Bernat 
Signed-off-by: Johannes Schindelin 
---
 git-filter-branch.sh | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 86b2ff1..1067785 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -283,13 +283,13 @@ report_progress ()
count=$git_filter_branch__commit_count
 
now=$(date +%s)
-   elapsed=$(($now - $start_timestamp))
-   remaining=$(( ($commits - $count) * $elapsed / $count ))
+   elapsed=$(($now-$start_timestamp))
+   remaining=$((($commits-$count)*$elapsed/$count))
if test $elapsed -gt 0
then
-   next_sample_at=$(( ($elapsed + 1) * $count / $elapsed ))
+   next_sample_at=$((($elapsed+1)*$count/$elapsed))
else
-   next_sample_at=$(($next_sample_at + 1))
+   next_sample_at=$(($next_sample_at+1))
fi
progress=" ($elapsed seconds passed, remaining $remaining 
predicted)"
fi
-- 
2.7.0.windows.1.7.g55a05c8


--
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 2/3] rebase--interactive: adjust the coding style of $((...))

2016-02-04 Thread Johannes Schindelin

In 180bad3 (rebase -i: respect core.commentchar, 2013-02-11) we changed
the coding style of an arithmetic expression to introduce spaces. The
existing $((...)) expression did not use that convention, though, and
neither was it the habit of the other shell scripts in Git's source code.

Later, 1db168e (rebase-i: loosen over-eager check_bad_cmd check,
2015-10-01) repeated the same coding style, again not matching the rest of
the source code's style.

Let's make the coding style consistent again.

Cc: John Keeping 
Cc: Matthieu Moy 
Signed-off-by: Johannes Schindelin 
---
 git-rebase--interactive.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index c0cfe88..3e2bd1b 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -122,7 +122,7 @@ mark_action_done () {
mv -f "$todo".new "$todo"
new_count=$(git stripspace --strip-comments <"$done" | wc -l)
echo $new_count >"$msgnum"
-   total=$(($new_count + $(git stripspace --strip-comments <"$todo" | wc 
-l)))
+   total=$(($new_count+$(git stripspace --strip-comments <"$todo" | wc 
-l)))
echo $total >"$end"
if test "$last_count" != "$new_count"
then
@@ -895,7 +895,7 @@ check_bad_cmd_and_sha () {
lineno=0
while read -r command rest
do
-   lineno=$(( $lineno + 1 ))
+   lineno=$(($lineno+1))
case $command in
"$comment_char"*|''|noop|x|exec)
# Doesn't expect a SHA-1
-- 
2.7.0.windows.1.7.g55a05c8


--
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 0/3] Fix $((...)) coding style

2016-02-04 Thread Johannes Schindelin

I noticed through a nearby patch series that was submitted by Elia that
some of the $((...)) expressions introduced in scripts that I introduced
to Git's source code did not match the existing code's convention:
previously these expressions did not contain any spaces, now *some* do.

This patch series tries to clean that up quickly before even more code
has to decide which one of the disagreeing coding conventions to use.

Note: For the sake of getting this patch series out, I skipped t/ and
contrib/. I do not care much about the latter, but t/ should probably be
fixed, too.


Johannes Schindelin (3):
  filter-branch: fix style of $((...) construct
  rebase--interactive: adjust the coding style of $((...))
  rebase --merge: adjust $((...)) coding style

 git-filter-branch.sh   | 8 
 git-rebase--interactive.sh | 4 ++--
 git-rebase--merge.sh   | 8 
 3 files changed, 10 insertions(+), 10 deletions(-)

-- 
2.7.0.windows.1.7.g55a05c8

--
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 14/15] git-am.sh: replace using expr for arithmetic operations with the equivalent shell builtin

2016-02-04 Thread Johannes Schindelin

Hi Elia,

On Thu, 4 Feb 2016, Elia Pinto wrote:

> 2016-02-04 12:14 GMT+01:00 Johannes Schindelin :
> > Hi Elia,
> >
> > On Thu, 4 Feb 2016, Elia Pinto wrote:
> >
> >> - this=$(expr "$this" + 1)
> >> + this=$(( "$this" + 1 ))
> >
> > Why the funny spaces? We do not do that anywhere in the existing code
> > except in three places (2x filter-branch, 1x rebase--interactive, all
> > three *not* my fault) and in some tests.

I actually noticed more places now, and will send a quick fixer-upper
patch series in a few seconds, that we can hopefully apply before your
series?

Thanks,
Dscho
--
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 14/15] git-am.sh: replace using expr for arithmetic operations with the equivalent shell builtin

2016-02-04 Thread Elia Pinto

2016-02-04 12:14 GMT+01:00 Johannes Schindelin :
> Hi Elia,
>
> On Thu, 4 Feb 2016, Elia Pinto wrote:
>
>> - this=$(expr "$this" + 1)
>> + this=$(( "$this" + 1 ))
>
> Why the funny spaces? We do not do that anywhere in the existing code
> except in three places (2x filter-branch, 1x rebase--interactive, all
> three *not* my fault) and in some tests.
>
> Also, I am *pretty* certain that the quotes break this code:
>
> me@work MINGW64 /usr/src/git (master)
> $ this=1
>
> me@work MINGW64 /usr/src/git (master)
> $ this=$(( "$this" + 1 ))
> bash: "1" + 1 : syntax error: operand expected (error token is ""1" + 
> 1 ")
>
> Whereas if you do *not* add the superfluous spaces and quotes, it works:
>
> me@work MINGW64 /usr/src/git (master)
> $ this=1
>
> me@work MINGW64 /usr/src/git (master)
Thanks for noticing.

You are right. I ran the test but did not notice mistakes, my fault.

I will resend. Thanks again.

Best

> $ this=$(($this+1))
>
> me@work MINGW64 /usr/src/git (master)
> $ echo $this
> 2
>
> Maybe this is only a problem with Bash 4.3.42 in MSYS2, but I do not think
> so.
>
> *Clicketyclick*
>
> Nope. It also happens in Ubuntu's Bash 4.3.42:
>
> me@ubuntu-vm  ~/git (master)
> $ this=1
>
> me@ubuntu-vm  ~/git (master)
> $ this=$(( "$this" + 1 ))
> bash: "1" + 1 : syntax error: operand expected (error token is ""1" + 
> 1 ")
>
> me@ubuntu-vm  ~/git (master)
> $ bash --version
> GNU bash, version 4.3.42(1)-release (x86_64-pc-linux-gnu)
> Copyright (C) 2013 Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later
> 
>
> This is free software; you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law.
>
> ... which makes me wonder in which environment you tested this?
>
> Ciao,
> Dscho
--
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

1 2 >

1 - 100 of 149 matches

Mail list logo