[PATCH v2 02/12] test-regex: isolate the bug test code

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

This is in preparation to turn test-regex into some generic regex
testing command.

Helped-by: Eric Sunshine 
Helped-by: Ramsay Jones 
Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: Junio C Hamano 
---
 t/t0070-fundamental.sh |  2 +-
 test-regex.c   | 12 ++--
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 5ed69a6..991ed2a 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -31,7 +31,7 @@ test_expect_success 'git_mkstemps_mode does not fail if fd 0 
is not open' '
 
 test_expect_success 'check for a bug in the regex routines' '
# if this test fails, re-build git with NO_REGEX=1
-   test-regex
+   test-regex --bug
 '
 
 test_done
diff --git a/test-regex.c b/test-regex.c
index 0dc598e..67a1a65 100644
--- a/test-regex.c
+++ b/test-regex.c
@@ -1,6 +1,6 @@
 #include "git-compat-util.h"
 
-int main(int argc, char **argv)
+static int test_regex_bug(void)
 {
char *pat = "[^={} \t]+";
char *str = "={}\nfred";
@@ -16,5 +16,13 @@ int main(int argc, char **argv)
if (m[0].rm_so == 3) /* matches '\n' when it should not */
die("regex bug confirmed: re-build git with NO_REGEX=1");
 
-   exit(0);
+   return 0;
+}
+
+int main(int argc, char **argv)
+{
+   if (argc == 2 && !strcmp(argv[1], "--bug"))
+   return test_regex_bug();
+   else
+   usage("test-regex --bug");
 }
-- 
2.8.2.526.g02eed6d

--
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 05/12] grep/icase: avoid kwsset when -F is specified

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

Similar to the previous commit, we can't use kws on icase search
outside ascii range. But we can't simply pass the pattern to
regcomp/pcre like the previous commit because it may contain regex
special characters, so we need to quote the regex first.

To avoid misquote traps that could lead to undefined behavior, we
always stick to basic regex engine in this case. We don't need fancy
features for grepping a literal string anyway.

basic_regex_quote_buf() assumes that if the pattern is in a multibyte
encoding, ascii chars must be unambiguously encoded as single
bytes. This is true at least for UTF-8. For others, let's wait until
people yell up. Chances are nobody uses multibyte, non utf-8 charsets
anymore.

Noticed-by: Plamen Totev 
Helped-by: René Scharfe 
Helped-by: Eric Sunshine 
Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: Junio C Hamano 
---
 grep.c  | 24 +++-
 quote.c | 37 +
 quote.h |  1 +
 t/t7812-grep-icase-non-ascii.sh | 26 ++
 4 files changed, 87 insertions(+), 1 deletion(-)

diff --git a/grep.c b/grep.c
index 451275d..5a8c52a 100644
--- a/grep.c
+++ b/grep.c
@@ -5,6 +5,7 @@
 #include "diff.h"
 #include "diffcore.h"
 #include "commit.h"
+#include "quote.h"
 
 static int grep_source_load(struct grep_source *gs);
 static int grep_source_is_binary(struct grep_source *gs);
@@ -397,6 +398,24 @@ static int is_fixed(const char *s, size_t len)
return 1;
 }
 
+static void compile_fixed_regexp(struct grep_pat *p, struct grep_opt *opt)
+{
+   struct strbuf sb = STRBUF_INIT;
+   int err;
+
+   basic_regex_quote_buf(, p->pattern);
+   err = regcomp(>regexp, sb.buf, opt->regflags & ~REG_EXTENDED);
+   if (opt->debug)
+   fprintf(stderr, "fixed %s\n", sb.buf);
+   strbuf_release();
+   if (err) {
+   char errbuf[1024];
+   regerror(err, >regexp, errbuf, sizeof(errbuf));
+   regfree(>regexp);
+   compile_regexp_failed(p, errbuf);
+   }
+}
+
 static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
 {
int icase, ascii_only;
@@ -408,7 +427,7 @@ static void compile_regexp(struct grep_pat *p, struct 
grep_opt *opt)
ascii_only = !has_non_ascii(p->pattern);
 
if (opt->fixed)
-   p->fixed = 1;
+   p->fixed = !icase || ascii_only;
else if ((!icase || ascii_only) &&
 is_fixed(p->pattern, p->patternlen))
p->fixed = 1;
@@ -423,6 +442,9 @@ static void compile_regexp(struct grep_pat *p, struct 
grep_opt *opt)
kwsincr(p->kws, p->pattern, p->patternlen);
kwsprep(p->kws);
return;
+   } else if (opt->fixed) {
+   compile_fixed_regexp(p, opt);
+   return;
}
 
if (opt->pcre) {
diff --git a/quote.c b/quote.c
index fe884d2..c67adb7 100644
--- a/quote.c
+++ b/quote.c
@@ -440,3 +440,40 @@ void tcl_quote_buf(struct strbuf *sb, const char *src)
}
strbuf_addch(sb, '"');
 }
+
+void basic_regex_quote_buf(struct strbuf *sb, const char *src)
+{
+   char c;
+
+   if (*src == '^') {
+   /* only beginning '^' is special and needs quoting */
+   strbuf_addch(sb, '\\');
+   strbuf_addch(sb, *src++);
+   }
+   if (*src == '*')
+   /* beginning '*' is not special, no quoting */
+   strbuf_addch(sb, *src++);
+
+   while ((c = *src++)) {
+   switch (c) {
+   case '[':
+   case '.':
+   case '\\':
+   case '*':
+   strbuf_addch(sb, '\\');
+   strbuf_addch(sb, c);
+   break;
+
+   case '$':
+   /* only the end '$' is special and needs quoting */
+   if (*src == '\0')
+   strbuf_addch(sb, '\\');
+   strbuf_addch(sb, c);
+   break;
+
+   default:
+   strbuf_addch(sb, c);
+   break;
+   }
+   }
+}
diff --git a/quote.h b/quote.h
index 99e04d3..362d315 100644
--- a/quote.h
+++ b/quote.h
@@ -67,5 +67,6 @@ extern char *quote_path_relative(const char *in, const char 
*prefix,
 extern void perl_quote_buf(struct strbuf *sb, const char *src);
 extern void python_quote_buf(struct strbuf *sb, const char *src);
 extern void tcl_quote_buf(struct strbuf *sb, const char *src);
+extern void basic_regex_quote_buf(struct strbuf *sb, const char *src);
 
 #endif
diff --git a/t/t7812-grep-icase-non-ascii.sh b/t/t7812-grep-icase-non-ascii.sh
index b78a774..1929809 100755
--- a/t/t7812-grep-icase-non-ascii.sh
+++ 

[PATCH v2 12/12] grep.c: reuse "icase" variable

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: Junio C Hamano 
---
 grep.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/grep.c b/grep.c
index 3d63612..92587a8 100644
--- a/grep.c
+++ b/grep.c
@@ -438,10 +438,7 @@ static void compile_regexp(struct grep_pat *p, struct 
grep_opt *opt)
p->fixed = 0;
 
if (p->fixed) {
-   if (opt->regflags & REG_ICASE || p->ignore_case)
-   p->kws = kwsalloc(tolower_trans_tbl);
-   else
-   p->kws = kwsalloc(NULL);
+   p->kws = kwsalloc(icase ? tolower_trans_tbl : NULL);
kwsincr(p->kws, p->pattern, p->patternlen);
kwsprep(p->kws);
return;
-- 
2.8.2.526.g02eed6d

--
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 07/12] grep/pcre: prepare locale-dependent tables for icase matching

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

The default tables are usually built with C locale and only suitable
for LANG=C or similar.  This should make case insensitive search work
correctly for all single-byte charsets.

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: Junio C Hamano 
---
 grep.c |  8 ++--
 grep.h |  1 +
 t/t7813-grep-icase-iso.sh (new +x) | 19 +++
 3 files changed, 26 insertions(+), 2 deletions(-)
 create mode 100755 t/t7813-grep-icase-iso.sh

diff --git a/grep.c b/grep.c
index 4b6b7bc..8cf4247 100644
--- a/grep.c
+++ b/grep.c
@@ -324,11 +324,14 @@ static void compile_pcre_regexp(struct grep_pat *p, const 
struct grep_opt *opt)
int erroffset;
int options = PCRE_MULTILINE;
 
-   if (opt->ignore_case)
+   if (opt->ignore_case) {
+   if (has_non_ascii(p->pattern))
+   p->pcre_tables = pcre_maketables();
options |= PCRE_CASELESS;
+   }
 
p->pcre_regexp = pcre_compile(p->pattern, options, , ,
-   NULL);
+ p->pcre_tables);
if (!p->pcre_regexp)
compile_regexp_failed(p, error);
 
@@ -362,6 +365,7 @@ static void free_pcre_regexp(struct grep_pat *p)
 {
pcre_free(p->pcre_regexp);
pcre_free(p->pcre_extra_info);
+   pcre_free((void *)p->pcre_tables);
 }
 #else /* !USE_LIBPCRE */
 static void compile_pcre_regexp(struct grep_pat *p, const struct grep_opt *opt)
diff --git a/grep.h b/grep.h
index 95f197a..cee4357 100644
--- a/grep.h
+++ b/grep.h
@@ -48,6 +48,7 @@ struct grep_pat {
regex_t regexp;
pcre *pcre_regexp;
pcre_extra *pcre_extra_info;
+   const unsigned char *pcre_tables;
kwset_t kws;
unsigned fixed:1;
unsigned ignore_case:1;
diff --git a/t/t7813-grep-icase-iso.sh b/t/t7813-grep-icase-iso.sh
new file mode 100755
index 000..efef7fb
--- /dev/null
+++ b/t/t7813-grep-icase-iso.sh
@@ -0,0 +1,19 @@
+#!/bin/sh
+
+test_description='grep icase on non-English locales'
+
+. ./lib-gettext.sh
+
+test_expect_success GETTEXT_ISO_LOCALE 'setup' '
+   printf "TILRAUN: Hall� Heimur!" >file &&
+   git add file &&
+   LC_ALL="$is_IS_iso_locale" &&
+   export LC_ALL
+'
+
+test_expect_success GETTEXT_ISO_LOCALE,LIBPCRE 'grep pcre string' '
+   git grep --perl-regexp -i "TILRAUN: H.ll� Heimur!" &&
+   git grep --perl-regexp -i "TILRAUN: H.LL� HEIMUR!"
+'
+
+test_done
-- 
2.8.2.526.g02eed6d

--
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 06/12] grep: rewrite an if/else condition to avoid duplicate expression

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

"!icase || ascii_only" is repeated twice in this if/else chain as this
series evolves. Rewrite it (and basically revert the first if
condition back to before the "grep: break down an "if" stmt..." commit).

Helped-by: Junio C Hamano 
Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 grep.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/grep.c b/grep.c
index 5a8c52a..4b6b7bc 100644
--- a/grep.c
+++ b/grep.c
@@ -426,11 +426,8 @@ static void compile_regexp(struct grep_pat *p, struct 
grep_opt *opt)
icase  = opt->regflags & REG_ICASE || p->ignore_case;
ascii_only = !has_non_ascii(p->pattern);
 
-   if (opt->fixed)
+   if (opt->fixed || is_fixed(p->pattern, p->patternlen))
p->fixed = !icase || ascii_only;
-   else if ((!icase || ascii_only) &&
-is_fixed(p->pattern, p->patternlen))
-   p->fixed = 1;
else
p->fixed = 0;
 
-- 
2.8.2.526.g02eed6d

--
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 04/12] grep/icase: avoid kwsset on literal non-ascii strings

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

When we detect the pattern is just a literal string, we avoid heavy
regex engine and use fast substring search implemented in kwsset.c.
But kws uses git-ctype which is locale-independent so it does not know
how to fold case properly outside ascii range. Let regcomp or pcre
take care of this case instead. Slower, but accurate.

Noticed-by: Plamen Totev 
Helped-by: René Scharfe 
Helped-by: Eric Sunshine 
Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: Junio C Hamano 
---
 grep.c   |  7 ++-
 t/t7812-grep-icase-non-ascii.sh (new +x) | 23 +++
 2 files changed, 29 insertions(+), 1 deletion(-)
 create mode 100755 t/t7812-grep-icase-non-ascii.sh

diff --git a/grep.c b/grep.c
index f430d7e..451275d 100644
--- a/grep.c
+++ b/grep.c
@@ -4,6 +4,7 @@
 #include "xdiff-interface.h"
 #include "diff.h"
 #include "diffcore.h"
+#include "commit.h"
 
 static int grep_source_load(struct grep_source *gs);
 static int grep_source_is_binary(struct grep_source *gs);
@@ -398,14 +399,18 @@ static int is_fixed(const char *s, size_t len)
 
 static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
 {
+   int icase, ascii_only;
int err;
 
p->word_regexp = opt->word_regexp;
p->ignore_case = opt->ignore_case;
+   icase  = opt->regflags & REG_ICASE || p->ignore_case;
+   ascii_only = !has_non_ascii(p->pattern);
 
if (opt->fixed)
p->fixed = 1;
-   else if (is_fixed(p->pattern, p->patternlen))
+   else if ((!icase || ascii_only) &&
+is_fixed(p->pattern, p->patternlen))
p->fixed = 1;
else
p->fixed = 0;
diff --git a/t/t7812-grep-icase-non-ascii.sh b/t/t7812-grep-icase-non-ascii.sh
new file mode 100755
index 000..b78a774
--- /dev/null
+++ b/t/t7812-grep-icase-non-ascii.sh
@@ -0,0 +1,23 @@
+#!/bin/sh
+
+test_description='grep icase on non-English locales'
+
+. ./lib-gettext.sh
+
+test_expect_success GETTEXT_LOCALE 'setup' '
+   test_write_lines "TILRAUN: Halló Heimur!" >file &&
+   git add file &&
+   LC_ALL="$is_IS_locale" &&
+   export LC_ALL
+'
+
+test_have_prereq GETTEXT_LOCALE &&
+test-regex "HALLÓ" "Halló" ICASE &&
+test_set_prereq REGEX_LOCALE
+
+test_expect_success REGEX_LOCALE 'grep literal string, no -F' '
+   git grep -i "TILRAUN: Halló Heimur!" &&
+   git grep -i "TILRAUN: HALLÓ HEIMUR!"
+'
+
+test_done
-- 
2.8.2.526.g02eed6d

--
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 08/12] gettext: add is_utf8_locale()

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

This function returns true if git is running under an UTF-8
locale. pcre in the next patch will need this.

is_encoding_utf8() is used instead of strcmp() to catch both "utf-8"
and "utf8" suffixes.

When built with no gettext support, we peek in several env variables
to detect UTF-8. pcre library might support utf-8 even if libc is
built without locale support.. The peeking code is a copy from
compat/regex/regcomp.c

Helped-by: Ævar Arnfjörð Bjarmason 
Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: Junio C Hamano 
---
 gettext.c | 24 ++--
 gettext.h |  1 +
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/gettext.c b/gettext.c
index a268a2c..db727ea 100644
--- a/gettext.c
+++ b/gettext.c
@@ -18,6 +18,8 @@
 #  endif
 #endif
 
+static const char *charset;
+
 /*
  * Guess the user's preferred languages from the value in LANGUAGE environment
  * variable and LC_MESSAGES locale category if NO_GETTEXT is not defined.
@@ -65,7 +67,6 @@ static int test_vsnprintf(const char *fmt, ...)
return ret;
 }
 
-static const char *charset;
 static void init_gettext_charset(const char *domain)
 {
/*
@@ -172,8 +173,27 @@ int gettext_width(const char *s)
 {
static int is_utf8 = -1;
if (is_utf8 == -1)
-   is_utf8 = !strcmp(charset, "UTF-8");
+   is_utf8 = is_utf8_locale();
 
return is_utf8 ? utf8_strwidth(s) : strlen(s);
 }
 #endif
+
+int is_utf8_locale(void)
+{
+#ifdef NO_GETTEXT
+   if (!charset) {
+   const char *env = getenv("LC_ALL");
+   if (!env || !*env)
+   env = getenv("LC_CTYPE");
+   if (!env || !*env)
+   env = getenv("LANG");
+   if (!env)
+   env = "";
+   if (strchr(env, '.'))
+   env = strchr(env, '.') + 1;
+   charset = xstrdup(env);
+   }
+#endif
+   return is_encoding_utf8(charset);
+}
diff --git a/gettext.h b/gettext.h
index 33696a4..7eee64a 100644
--- a/gettext.h
+++ b/gettext.h
@@ -90,5 +90,6 @@ const char *Q_(const char *msgid, const char *plu, unsigned 
long n)
 #endif
 
 const char *get_preferred_languages(void);
+extern int is_utf8_locale(void);
 
 #endif
-- 
2.8.2.526.g02eed6d

--
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 03/12] test-regex: expose full regcomp() to the command line

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: Junio C Hamano 
---
 test-regex.c | 51 +--
 1 file changed, 49 insertions(+), 2 deletions(-)

diff --git a/test-regex.c b/test-regex.c
index 67a1a65..eff26f5 100644
--- a/test-regex.c
+++ b/test-regex.c
@@ -1,4 +1,21 @@
 #include "git-compat-util.h"
+#include "gettext.h"
+
+struct reg_flag {
+   const char *name;
+   int flag;
+};
+
+static struct reg_flag reg_flags[] = {
+   { "EXTENDED",REG_EXTENDED   },
+   { "NEWLINE", REG_NEWLINE},
+   { "ICASE",   REG_ICASE  },
+   { "NOTBOL",  REG_NOTBOL },
+#ifdef REG_STARTEND
+   { "STARTEND",REG_STARTEND   },
+#endif
+   { NULL, 0 }
+};
 
 static int test_regex_bug(void)
 {
@@ -21,8 +38,38 @@ static int test_regex_bug(void)
 
 int main(int argc, char **argv)
 {
+   const char *pat;
+   const char *str;
+   int flags = 0;
+   regex_t r;
+   regmatch_t m[1];
+
if (argc == 2 && !strcmp(argv[1], "--bug"))
return test_regex_bug();
-   else
-   usage("test-regex --bug");
+   else if (argc < 3)
+   usage("test-regex --bug\n"
+ "test-regex   []");
+
+   argv++;
+   pat = *argv++;
+   str = *argv++;
+   while (*argv) {
+   struct reg_flag *rf;
+   for (rf = reg_flags; rf->name; rf++)
+   if (!strcmp(*argv, rf->name)) {
+   flags |= rf->flag;
+   break;
+   }
+   if (!rf->name)
+   die("do not recognize %s", *argv);
+   argv++;
+   }
+   git_setup_gettext();
+
+   if (regcomp(, pat, flags))
+   die("failed regcomp() for pattern '%s'", pat);
+   if (regexec(, str, 1, m, 0))
+   return 1;
+
+   return 0;
 }
-- 
2.8.2.526.g02eed6d

--
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 09/12] grep/pcre: support utf-8

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

In the previous change in this function, we add locale support for
single-byte encodings only. It looks like pcre only supports utf-* as
multibyte encodings, the others are left in the cold (which is
fine).

We need to enable PCRE_UTF8 so pcre can find character boundary
correctly. It's needed for case folding (when --ignore-case is used)
or '*', '+' or similar syntax is used.

The "has_non_ascii()" check is to be on the conservative side. If
there's non-ascii in the pattern, the searched content could still be
in utf-8, but we can treat it just like a byte stream and everything
should work. If we force utf-8 based on locale only and pcre validates
utf-8 and the file content is in non-utf8 encoding, things break.

Noticed-by: Plamen Totev 
Helped-by: Plamen Totev 
Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: Junio C Hamano 
---
 grep.c  |  2 ++
 t/t7812-grep-icase-non-ascii.sh | 15 +++
 2 files changed, 17 insertions(+)

diff --git a/grep.c b/grep.c
index 8cf4247..3d63612 100644
--- a/grep.c
+++ b/grep.c
@@ -329,6 +329,8 @@ static void compile_pcre_regexp(struct grep_pat *p, const 
struct grep_opt *opt)
p->pcre_tables = pcre_maketables();
options |= PCRE_CASELESS;
}
+   if (is_utf8_locale() && has_non_ascii(p->pattern))
+   options |= PCRE_UTF8;
 
p->pcre_regexp = pcre_compile(p->pattern, options, , ,
  p->pcre_tables);
diff --git a/t/t7812-grep-icase-non-ascii.sh b/t/t7812-grep-icase-non-ascii.sh
index 1929809..08ae4c9 100755
--- a/t/t7812-grep-icase-non-ascii.sh
+++ b/t/t7812-grep-icase-non-ascii.sh
@@ -20,6 +20,21 @@ test_expect_success REGEX_LOCALE 'grep literal string, no 
-F' '
git grep -i "TILRAUN: HALLÓ HEIMUR!"
 '
 
+test_expect_success GETTEXT_LOCALE,LIBPCRE 'grep pcre utf-8 icase' '
+   git grep --perl-regexp"TILRAUN: H.lló Heimur!" &&
+   git grep --perl-regexp -i "TILRAUN: H.lló Heimur!" &&
+   git grep --perl-regexp -i "TILRAUN: H.LLÓ HEIMUR!"
+'
+
+test_expect_success GETTEXT_LOCALE,LIBPCRE 'grep pcre utf-8 string with "+"' '
+   test_write_lines "TILRAUN: Hallóó Heimur!" >file2 &&
+   git add file2 &&
+   git grep -l --perl-regexp "TILRAUN: H.lló+ Heimur!" >actual &&
+   echo file >expected &&
+   echo file2 >>expected &&
+   test_cmp expected actual
+'
+
 test_expect_success REGEX_LOCALE 'grep literal string, with -F' '
git grep --debug -i -F "TILRAUN: Halló Heimur!"  2>&1 >/dev/null |
 grep fixed >debug1 &&
-- 
2.8.2.526.g02eed6d

--
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 11/12] diffcore-pickaxe: support case insensitive match on non-ascii

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

Similar to the "grep -F -i" case, we can't use kws on icase search
outside ascii range, so we quote the string and pass it to regcomp as
a basic regexp and let regex engine deal with case sensitivity.

The new test is put in t7812 instead of t4209-log-pickaxe because
lib-gettext.sh might cause problems elsewhere, probably.

Noticed-by: Plamen Totev 
Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 diffcore-pickaxe.c  | 11 +++
 t/t7812-grep-icase-non-ascii.sh |  7 +++
 2 files changed, 18 insertions(+)

diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 2093b6a..55067ca 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -7,6 +7,8 @@
 #include "diffcore.h"
 #include "xdiff-interface.h"
 #include "kwset.h"
+#include "commit.h"
+#include "quote.h"
 
 typedef int (*pickaxe_fn)(mmfile_t *one, mmfile_t *two,
  struct diff_options *o,
@@ -223,6 +225,15 @@ void diffcore_pickaxe(struct diff_options *o)
cflags |= REG_ICASE;
regcomp_or_die(, needle, cflags);
regexp = 
+   } else if (DIFF_OPT_TST(o, PICKAXE_IGNORE_CASE) &&
+  has_non_ascii(needle)) {
+   struct strbuf sb = STRBUF_INIT;
+   int cflags = REG_NEWLINE | REG_ICASE;
+
+   basic_regex_quote_buf(, needle);
+   regcomp_or_die(, sb.buf, cflags);
+   strbuf_release();
+   regexp = 
} else {
kws = kwsalloc(DIFF_OPT_TST(o, PICKAXE_IGNORE_CASE)
   ? tolower_trans_tbl : NULL);
diff --git a/t/t7812-grep-icase-non-ascii.sh b/t/t7812-grep-icase-non-ascii.sh
index 08ae4c9..169fd8d 100755
--- a/t/t7812-grep-icase-non-ascii.sh
+++ b/t/t7812-grep-icase-non-ascii.sh
@@ -61,4 +61,11 @@ test_expect_success REGEX_LOCALE 'grep string with regex, 
with -F' '
test_cmp expect2 debug2
 '
 
+test_expect_success REGEX_LOCALE 'pickaxe -i on non-ascii' '
+   git commit -m first &&
+   git log --format=%f -i -S"TILRAUN: HALLÓ HEIMUR!" >actual &&
+   echo first >expected &&
+   test_cmp expected actual
+'
+
 test_done
-- 
2.8.2.526.g02eed6d

--
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 10/12] diffcore-pickaxe: Add regcomp_or_die()

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

There's another regcomp code block coming in this function that needs
the same error handling. This function can help avoid duplicating
error handling code.

Helped-by: Jeff King 
Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 diffcore-pickaxe.c | 22 +-
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 7715c13..2093b6a 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -198,6 +198,18 @@ static void pickaxe(struct diff_queue_struct *q, struct 
diff_options *o,
*q = outq;
 }
 
+static void regcomp_or_die(regex_t *regex, const char *needle, int cflags)
+{
+   int err = regcomp(regex, needle, cflags);
+   if (err) {
+   /* The POSIX.2 people are surely sick */
+   char errbuf[1024];
+   regerror(err, regex, errbuf, 1024);
+   regfree(regex);
+   die("invalid regex: %s", errbuf);
+   }
+}
+
 void diffcore_pickaxe(struct diff_options *o)
 {
const char *needle = o->pickaxe;
@@ -206,18 +218,10 @@ void diffcore_pickaxe(struct diff_options *o)
kwset_t kws = NULL;
 
if (opts & (DIFF_PICKAXE_REGEX | DIFF_PICKAXE_KIND_G)) {
-   int err;
int cflags = REG_EXTENDED | REG_NEWLINE;
if (DIFF_OPT_TST(o, PICKAXE_IGNORE_CASE))
cflags |= REG_ICASE;
-   err = regcomp(, needle, cflags);
-   if (err) {
-   /* The POSIX.2 people are surely sick */
-   char errbuf[1024];
-   regerror(err, , errbuf, 1024);
-   regfree();
-   die("invalid regex: %s", errbuf);
-   }
+   regcomp_or_die(, needle, cflags);
regexp = 
} else {
kws = kwsalloc(DIFF_OPT_TST(o, PICKAXE_IGNORE_CASE)
-- 
2.8.2.526.g02eed6d

--
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 00/12] nd/icase updates

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

v2 fixes Junio's and Jeff's comments (both good). The sharing "!icase
|| ascii_only" is made a separate commit (6/12) because I think it
takes some seconds to realize that the conversion is correct and it's
technically not needed in 5/12 (and it's sort of the opposite of 1/12)

Interdiff

diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 0a5f877..55067ca 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -200,19 +200,30 @@ static void pickaxe(struct diff_queue_struct *q, struct 
diff_options *o,
*q = outq;
 }
 
+static void regcomp_or_die(regex_t *regex, const char *needle, int cflags)
+{
+   int err = regcomp(regex, needle, cflags);
+   if (err) {
+   /* The POSIX.2 people are surely sick */
+   char errbuf[1024];
+   regerror(err, regex, errbuf, 1024);
+   regfree(regex);
+   die("invalid regex: %s", errbuf);
+   }
+}
+
 void diffcore_pickaxe(struct diff_options *o)
 {
const char *needle = o->pickaxe;
int opts = o->pickaxe_opts;
regex_t regex, *regexp = NULL;
kwset_t kws = NULL;
-   int err = 0;
 
if (opts & (DIFF_PICKAXE_REGEX | DIFF_PICKAXE_KIND_G)) {
int cflags = REG_EXTENDED | REG_NEWLINE;
if (DIFF_OPT_TST(o, PICKAXE_IGNORE_CASE))
cflags |= REG_ICASE;
-   err = regcomp(, needle, cflags);
+   regcomp_or_die(, needle, cflags);
regexp = 
} else if (DIFF_OPT_TST(o, PICKAXE_IGNORE_CASE) &&
   has_non_ascii(needle)) {
@@ -220,7 +231,7 @@ void diffcore_pickaxe(struct diff_options *o)
int cflags = REG_NEWLINE | REG_ICASE;
 
basic_regex_quote_buf(, needle);
-   err = regcomp(, sb.buf, cflags);
+   regcomp_or_die(, sb.buf, cflags);
strbuf_release();
regexp = 
} else {
@@ -229,13 +240,6 @@ void diffcore_pickaxe(struct diff_options *o)
kwsincr(kws, needle, strlen(needle));
kwsprep(kws);
}
-   if (err) {
-   /* The POSIX.2 people are surely sick */
-   char errbuf[1024];
-   regerror(err, , errbuf, 1024);
-   regfree();
-   die("invalid regex: %s", errbuf);
-   }
 
/* Might want to warn when both S and G are on; I don't care... */
pickaxe(_queued_diff, o, regexp, kws,
diff --git a/grep.c b/grep.c
index cb058a5..92587a8 100644
--- a/grep.c
+++ b/grep.c
@@ -432,15 +432,8 @@ static void compile_regexp(struct grep_pat *p, struct 
grep_opt *opt)
icase  = opt->regflags & REG_ICASE || p->ignore_case;
ascii_only = !has_non_ascii(p->pattern);
 
-   if (opt->fixed) {
+   if (opt->fixed || is_fixed(p->pattern, p->patternlen))
p->fixed = !icase || ascii_only;
-   if (!p->fixed) {
-   compile_fixed_regexp(p, opt);
-   return;
-   }
-   } else if ((!icase || ascii_only) &&
-  is_fixed(p->pattern, p->patternlen))
-   p->fixed = 1;
else
p->fixed = 0;
 
@@ -449,6 +442,9 @@ static void compile_regexp(struct grep_pat *p, struct 
grep_opt *opt)
kwsincr(p->kws, p->pattern, p->patternlen);
kwsprep(p->kws);
return;
+   } else if (opt->fixed) {
+   compile_fixed_regexp(p, opt);
+   return;
}
 
if (opt->pcre) {

Nguyễn Thái Ngọc Duy (12):
  grep: break down an "if" stmt in preparation for next changes
  test-regex: isolate the bug test code
  test-regex: expose full regcomp() to the command line
  grep/icase: avoid kwsset on literal non-ascii strings
  grep/icase: avoid kwsset when -F is specified
  grep: rewrite an if/else condition to avoid duplicate expression
  grep/pcre: prepare locale-dependent tables for icase matching
  gettext: add is_utf8_locale()
  grep/pcre: support utf-8
  diffcore-pickaxe: Add regcomp_or_die()
  diffcore-pickaxe: support case insensitive match on non-ascii
  grep.c: reuse "icase" variable

 diffcore-pickaxe.c   | 33 +++
 gettext.c| 24 ++-
 gettext.h|  1 +
 grep.c   | 43 +++
 grep.h   |  1 +
 quote.c  | 37 +
 quote.h  |  1 +
 t/t0070-fundamental.sh   |  2 +-
 t/t7812-grep-icase-non-ascii.sh (new +x) | 71 
 t/t7813-grep-icase-iso.sh (new +x)   | 19 +
 test-regex.c | 59 +-
 11 files changed, 270 insertions(+), 21 deletions(-)
 create mode 100755 t/t7812-grep-icase-non-ascii.sh
 create mode 100755 

[PATCH v2 01/12] grep: break down an "if" stmt in preparation for next changes

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: Junio C Hamano 
---
 grep.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/grep.c b/grep.c
index 7b2b96a..f430d7e 100644
--- a/grep.c
+++ b/grep.c
@@ -403,7 +403,9 @@ static void compile_regexp(struct grep_pat *p, struct 
grep_opt *opt)
p->word_regexp = opt->word_regexp;
p->ignore_case = opt->ignore_case;
 
-   if (opt->fixed || is_fixed(p->pattern, p->patternlen))
+   if (opt->fixed)
+   p->fixed = 1;
+   else if (is_fixed(p->pattern, p->patternlen))
p->fixed = 1;
else
p->fixed = 0;
-- 
2.8.2.526.g02eed6d

--
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] builtin/worktree.c: add option for setting worktree name

[Barret Rennie]

Add the --name parameter to git worktree add that allows the user to set
the name of the created worktree directory. A worktree must not already
exist with the current name or creation will fail.

Signed-off-by: Barret Rennie 
---
 Documentation/git-worktree.txt |  6 +-
 builtin/worktree.c | 24 ++--
 t/t2025-worktree-add.sh| 16 
 3 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 23d8d2a..2af0ee4 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -9,7 +9,7 @@ git-worktree - Manage multiple working trees
 SYNOPSIS
 
 [verse]
-'git worktree add' [-f] [--detach] [--checkout] [-b ]  
[]
+'git worktree add' [-f] [--detach] [--checkout] [-b ] [--name 
]  []
 'git worktree prune' [-n] [-v] [--expire ]
 'git worktree list' [--porcelain]
 
@@ -88,6 +88,10 @@ OPTIONS
With `add`, detach HEAD in the new working tree. See "DETACHED HEAD"
in linkgit:git-checkout[1].
 
+--name::
+   Set the name for the worktree. If there is already a worktree with this
+   name, the command will fail.
+
 --[no-]checkout::
By default, `add` checks out ``, however, `--no-checkout` can
be used to suppress checkout in order to make customizations,
diff --git a/builtin/worktree.c b/builtin/worktree.c
index e3199a2..ed071b2 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -24,6 +24,7 @@ struct add_opts {
int checkout;
const char *new_branch;
int force_new_branch;
+   const char *name;
 };
 
 static int show_only;
@@ -212,19 +213,29 @@ static int add_worktree(const char *path, const char 
*refname,
die(_("invalid reference: %s"), refname);
}
 
-   name = worktree_basename(path, );
+   if (opts->name) {
+   name = opts->name;
+   len = strlen(name);
+   } else {
+   name = worktree_basename(path, );
+   }
+
strbuf_addstr(_repo,
  git_path("worktrees/%.*s", (int)(path + len - name), 
name));
+
len = sb_repo.len;
if (safe_create_leading_directories_const(sb_repo.buf))
die_errno(_("could not create leading directories of '%s'"),
  sb_repo.buf);
-   while (!stat(sb_repo.buf, )) {
-   counter++;
-   strbuf_setlen(_repo, len);
-   strbuf_addf(_repo, "%d", counter);
+
+   if (!opts->name) {
+   while (!stat(sb_repo.buf, )) {
+   counter++;
+   strbuf_setlen(_repo, len);
+   strbuf_addf(_repo, "%d", counter);
+   }
+   name = strrchr(sb_repo.buf, '/') + 1;
}
-   name = strrchr(sb_repo.buf, '/') + 1;
 
junk_pid = getpid();
atexit(remove_junk);
@@ -326,6 +337,7 @@ static int add(int ac, const char **av, const char *prefix)
   N_("create or reset a branch")),
OPT_BOOL(0, "detach", , N_("detach HEAD at named 
commit")),
OPT_BOOL(0, "checkout", , N_("populate the new 
working tree")),
+   OPT_STRING(0, "name", , N_("name"), N_("set name for 
working tree")),
OPT_END()
};
 
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 4bcc335..9abcf8e 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -63,6 +63,22 @@ test_expect_success '"add" worktree' '
)
 '
 
+test_expect_success '"add" worktree with name' '
+   git worktree add --detach --name custom-name another-worktree master &&
+   (
+   cd here &&
+   test_cmp ../init.t init.t
+   ) &&
+   (
+   cd .git/worktrees &&
+   test -d custom-name
+   )
+'
+
+test_expect_success '"add" worktree with name that already exists' '
+   test_must_fail git worktree add --name custom-name --detach 
yet-another-worktree master
+'
+
 test_expect_success '"add" worktree from a subdir' '
(
mkdir sub &&
-- 
2.9.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: [RFC] gc: correct gc.autoPackLimit documentation

[Eric Wong]

Jeff King  wrote:
> On Sat, Jun 25, 2016 at 01:14:50AM +, Eric Wong wrote:
> 
> > I'm not sure if this is the best approach, or if changing
> > too_many_packs can be done without causing problems for
> > hosts of big repos.
> > 
> > ---8<-
> > Subject: [PATCH] gc: correct gc.autoPackLimit documentation
> > 
> > I want to ensure there is only one pack in my repo to take
> > advantage of pack bitmaps.  Based on my reading of the
> > documentation, I configured gc.autoPackLimit=1 which led to
> > "gc --auto" constantly trying to repack on every invocation.
> 
> I'm not sure if you might be misinterpreting earlier advice on bitmaps
> here. At the time of packing, bitmaps need for all of the objects to go
> to a single pack (they cannot handle a case where one object in the pack
> can reach another object that is not in the pack). But that is easily
> done with "git repack -adb".
> 
> After that packing, you can add new packs that do not have bitmaps, and
> the bitmaps will gracefully degrade. E.g., imagine master was at tip X
> when you repacked with bitmaps, and now somebody has pushed to make it
> tip Y.  Somebody then clones, asking for Y. The bitmap code will start
> at Y and walk backwards. When it hits X, it stops walking as it can fill
> in the rest of the reachability from there.

Ah, thanks, makes sense.  I was misinterpreting earlier advice
on bitmaps.

> That's neither here nor there for the off-by-one in gc or its
> documentation, of course, but just FYI.

I'm now inclined to fix the problem in gc and leave the
documentation as-is (unless it cause other problems...)
--
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: [RFC] gc: correct gc.autoPackLimit documentation

[Jeff King]

On Sat, Jun 25, 2016 at 01:14:50AM +, Eric Wong wrote:

> I'm not sure if this is the best approach, or if changing
> too_many_packs can be done without causing problems for
> hosts of big repos.
> 
> ---8<-
> Subject: [PATCH] gc: correct gc.autoPackLimit documentation
> 
> I want to ensure there is only one pack in my repo to take
> advantage of pack bitmaps.  Based on my reading of the
> documentation, I configured gc.autoPackLimit=1 which led to
> "gc --auto" constantly trying to repack on every invocation.

I'm not sure if you might be misinterpreting earlier advice on bitmaps
here. At the time of packing, bitmaps need for all of the objects to go
to a single pack (they cannot handle a case where one object in the pack
can reach another object that is not in the pack). But that is easily
done with "git repack -adb".

After that packing, you can add new packs that do not have bitmaps, and
the bitmaps will gracefully degrade. E.g., imagine master was at tip X
when you repacked with bitmaps, and now somebody has pushed to make it
tip Y.  Somebody then clones, asking for Y. The bitmap code will start
at Y and walk backwards. When it hits X, it stops walking as it can fill
in the rest of the reachability from there.

So you do have to walk X..Y the old-fashioned way, but that's generally
not a big problem for a few pushes.

IOW, I think trying to repack on every single push is probably overkill.
Yes, it will buy you a little savings on fetch requests, but whether it
is worthwhile to pack depends on:

 - how big the push was (e.g., 2 commits versus thousands; the bigger
   it is, the more you save per fetch

 - how big the repo is (the bigger it is, the more it costs to do the
   repack; packing is linear-ish effort in the number of objects in the
   repo)

 - how often you get fetches versus pushes (your cost is amortized
   across all the fetches)

There are numbers where it can be worth it to pack really aggressively,
but I doubt it's common. At GitHub we use a combination of number of
packs (and we try to keep it under 50) and size of objects not in the
"main" pack (I did a bunch of fancy logging and analysis of object
counts, bytes in packs, etc, at one point, and we basically realized
that for the common cases, all of the interesting metrics are roughly
proportional to the number of bytes that could be moved into the main
pack).

That's neither here nor there for the off-by-one in gc or its
documentation, of course, but just FYI.

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

[RFC] gc: correct gc.autoPackLimit documentation

[Eric Wong]

I'm not sure if this is the best approach, or if changing
too_many_packs can be done without causing problems for
hosts of big repos.

---8<-
Subject: [PATCH] gc: correct gc.autoPackLimit documentation

I want to ensure there is only one pack in my repo to take
advantage of pack bitmaps.  Based on my reading of the
documentation, I configured gc.autoPackLimit=1 which led to
"gc --auto" constantly trying to repack on every invocation.

Update the documentation to reflect what is probably a
long-standing off-by-one bug in builtin/gc.c::too_many_packs:

-   return gc_auto_pack_limit <= cnt;
+   return gc_auto_pack_limit < cnt;

However, changing gc itself at this time may cause problems
for people who are already using gc.autoPackLimit=2 and
expect bitmaps to work for them.

Signed-off-by: Eric Wong 
---
 Documentation/config.txt | 2 +-
 Documentation/git-gc.txt | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2e1b2e4..b0de3f1 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1345,7 +1345,7 @@ gc.auto::
default value is 6700.  Setting this to 0 disables it.
 
 gc.autoPackLimit::
-   When there are more than this many packs that are not
+   When there at least this many packs that are not
marked with `*.keep` file in the repository, `git gc
--auto` consolidates them into one larger pack.  The
default value is 50.  Setting this to 0 disables it.
diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt
index fa15104..658612d 100644
--- a/Documentation/git-gc.txt
+++ b/Documentation/git-gc.txt
@@ -54,7 +54,7 @@ all loose objects are combined into a single pack using
 `git repack -d -l`.  Setting the value of `gc.auto` to 0
 disables automatic packing of loose objects.
 +
-If the number of packs exceeds the value of `gc.autoPackLimit`,
+If the number of packs matches or exceeds the value of `gc.autoPackLimit`,
 then existing packs (except those marked with a `.keep` file)
 are consolidated into a single pack by using the `-A` option of
 'git repack'. Setting `gc.autoPackLimit` to 0 disables
-- 
EW
--
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 06/11] diff: rename struct diff_filespec's sha1_valid member

[brian m. carlson]

On Fri, Jun 24, 2016 at 05:29:18PM -0700, Junio C Hamano wrote:
> "brian m. carlson"  writes:
> 
> 
> > @@
> > struct diff_filespec o;
> > @@
> > - o.sha1_valid
> > + o.oid_valid
> >
> > @@
> > struct diff_filespec *p;
> > @@
> > - p->sha1_valid
> > + p->oid_valid
> 
> Totally offtopic (from Git's point of view), but why does Coccinelle
> need both of these?  I recall that between v1 and v2 there were some
> confusing discussions about the order of these equivalent conversions
> mattering and the tool producing an incorrect conversion in v1.

As I understand it, Coccinelle doesn't transform . into ->.  Granted,
the latter is a special case of the former, but it might break workflows
where you're trying to convert an inline struct to a pointer to struct
or such.  It errs on the side of you being more explicit to allow more
flexibility in usage.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH v3 06/11] diff: rename struct diff_filespec's sha1_valid member

[Junio C Hamano]

"brian m. carlson"  writes:


> @@
> struct diff_filespec o;
> @@
> - o.sha1_valid
> + o.oid_valid
>
> @@
> struct diff_filespec *p;
> @@
> - p->sha1_valid
> + p->oid_valid

Totally offtopic (from Git's point of view), but why does Coccinelle
need both of these?  I recall that between v1 and v2 there were some
confusing discussions about the order of these equivalent conversions
mattering and the tool producing an incorrect conversion in v1.

--
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 1/4] t5000: test tar files that overflow ustar headers

[Jeff King]

On Fri, Jun 24, 2016 at 03:41:47PM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > The difference in time between the two is measurable on my system, but
> > it's only a few milliseconds (for 4096 bytes). So maybe it's not worth
> > worrying about (though as a general technique, it does make me worry
> > that it's easy to get wrong in a way that will fail racily).
> 
> Yeah, GNU dd has iflag=fullblock, but if we assume GNU, we can
> safely assume "head -c", so I do not think of a way to do this
> portably enough.

Thinking on it more, "head -c" is _not_ what one would want in all
cases. It would work here, but not in t9300, for instance, where the
code is trying to read an exact number of bytes from a fifo. I don't
think "head" makes any promises about buffering and may read extra
bytes.

So I dunno. "dd" generally does make such promises, or perhaps the perl
sysread() solution in t9300 is not so bad.

-Peff
--
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 v3 07/11] merge-recursive: convert struct stage_data to use object_id

[brian m. carlson]

Convert the anonymous struct within struct stage_data to use struct
object_id.  The following Coccinelle semantic patch was used to
implement this, followed by the transformations in object_id.cocci:

@@
struct stage_data o;
expression E1;
@@
- o.stages[E1].sha
+ o.stages[E1].oid.hash

@@
struct stage_data *p;
expression E1;
@@
- p->stages[E1].sha
+ p->stages[E1].oid.hash

Signed-off-by: brian m. carlson 
Signed-off-by: Junio C Hamano 
---
 merge-recursive.c | 38 ++
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 1e802097..a07050cd 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -90,7 +90,7 @@ struct rename_conflict_info {
 struct stage_data {
struct {
unsigned mode;
-   unsigned char sha[20];
+   struct object_id oid;
} stages[4];
struct rename_conflict_info *rename_conflict_info;
unsigned processed:1;
@@ -134,13 +134,11 @@ static inline void setup_rename_conflict_info(enum 
rename_type rename_type,
int ostage2 = ostage1 ^ 1;
 
ci->ren1_other.path = pair1->one->path;
-   hashcpy(ci->ren1_other.oid.hash,
-   src_entry1->stages[ostage1].sha);
+   oidcpy(>ren1_other.oid, _entry1->stages[ostage1].oid);
ci->ren1_other.mode = src_entry1->stages[ostage1].mode;
 
ci->ren2_other.path = pair2->one->path;
-   hashcpy(ci->ren2_other.oid.hash,
-   src_entry2->stages[ostage2].sha);
+   oidcpy(>ren2_other.oid, _entry2->stages[ostage2].oid);
ci->ren2_other.mode = src_entry2->stages[ostage2].mode;
}
 }
@@ -316,11 +314,11 @@ static struct stage_data *insert_stage_data(const char 
*path,
struct string_list_item *item;
struct stage_data *e = xcalloc(1, sizeof(struct stage_data));
get_tree_entry(o->object.oid.hash, path,
-   e->stages[1].sha, >stages[1].mode);
+   e->stages[1].oid.hash, >stages[1].mode);
get_tree_entry(a->object.oid.hash, path,
-   e->stages[2].sha, >stages[2].mode);
+   e->stages[2].oid.hash, >stages[2].mode);
get_tree_entry(b->object.oid.hash, path,
-   e->stages[3].sha, >stages[3].mode);
+   e->stages[3].oid.hash, >stages[3].mode);
item = string_list_insert(entries, path);
item->util = e;
return e;
@@ -351,7 +349,7 @@ static struct string_list *get_unmerged(void)
}
e = item->util;
e->stages[ce_stage(ce)].mode = ce->ce_mode;
-   hashcpy(e->stages[ce_stage(ce)].sha, ce->sha1);
+   hashcpy(e->stages[ce_stage(ce)].oid.hash, ce->sha1);
}
 
return unmerged;
@@ -574,9 +572,9 @@ static void update_entry(struct stage_data *entry,
entry->stages[1].mode = o->mode;
entry->stages[2].mode = a->mode;
entry->stages[3].mode = b->mode;
-   hashcpy(entry->stages[1].sha, o->oid.hash);
-   hashcpy(entry->stages[2].sha, a->oid.hash);
-   hashcpy(entry->stages[3].sha, b->oid.hash);
+   oidcpy(>stages[1].oid, >oid);
+   oidcpy(>stages[2].oid, >oid);
+   oidcpy(>stages[3].oid, >oid);
 }
 
 static int remove_file(struct merge_options *o, int clean,
@@ -,7 +1109,7 @@ static struct diff_filespec *filespec_from_entry(struct 
diff_filespec *target,
 struct stage_data *entry,
 int stage)
 {
-   unsigned char *sha = entry->stages[stage].sha;
+   unsigned char *sha = entry->stages[stage].oid.hash;
unsigned mode = entry->stages[stage].mode;
if (mode == 0 || is_null_sha1(sha))
return NULL;
@@ -1425,11 +1423,11 @@ static int process_renames(struct merge_options *o,
remove_file(o, 1, ren1_src,
renamed_stage == 2 || 
!was_tracked(ren1_src));
 
-   hashcpy(src_other.oid.hash,
-   ren1->src_entry->stages[other_stage].sha);
+   oidcpy(_other.oid,
+  >src_entry->stages[other_stage].oid);
src_other.mode = 
ren1->src_entry->stages[other_stage].mode;
-   hashcpy(dst_other.oid.hash,
-   ren1->dst_entry->stages[other_stage].sha);
+   oidcpy(_other.oid,
+  >dst_entry->stages[other_stage].oid);
dst_other.mode = 
ren1->dst_entry->stages[other_stage].mode;
try_merge = 0;
 
@@ -1703,9 +1701,9 @@ static int process_entry(struct merge_options *o,
unsigned o_mode = 

[PATCH v3 05/11] diff: convert struct diff_filespec to struct object_id

[brian m. carlson]

Convert struct diff_filespec's sha1 member to use a struct object_id
called "oid" instead.  The following Coccinelle semantic patch was used
to implement this, followed by the transformations in object_id.cocci:

@@
struct diff_filespec o;
@@
- o.sha1
+ o.oid.hash

@@
struct diff_filespec *p;
@@
- p->sha1
+ p->oid.hash

Signed-off-by: brian m. carlson 
Signed-off-by: Junio C Hamano 
---
 builtin/blame.c   |   6 +--
 builtin/fast-export.c |  10 ++---
 builtin/reset.c   |   4 +-
 combine-diff.c|  10 ++---
 diff.c|  69 +---
 diffcore-break.c  |   2 +-
 diffcore-rename.c |  14 ---
 diffcore.h|   2 +-
 line-log.c|   2 +-
 merge-recursive.c | 107 +++---
 notes-merge.c |  42 ++--
 submodule.c   |   4 +-
 wt-status.c   |   3 +-
 13 files changed, 147 insertions(+), 128 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 759d84af..c3459f54 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -599,7 +599,7 @@ static struct origin *find_origin(struct scoreboard *sb,
p->status);
case 'M':
porigin = get_origin(sb, parent, origin->path);
-   hashcpy(porigin->blob_sha1, p->one->sha1);
+   hashcpy(porigin->blob_sha1, p->one->oid.hash);
porigin->mode = p->one->mode;
break;
case 'A':
@@ -645,7 +645,7 @@ static struct origin *find_rename(struct scoreboard *sb,
if ((p->status == 'R' || p->status == 'C') &&
!strcmp(p->two->path, origin->path)) {
porigin = get_origin(sb, parent, p->one->path);
-   hashcpy(porigin->blob_sha1, p->one->sha1);
+   hashcpy(porigin->blob_sha1, p->one->oid.hash);
porigin->mode = p->one->mode;
break;
}
@@ -1309,7 +1309,7 @@ static void find_copy_in_parent(struct scoreboard *sb,
continue;
 
norigin = get_origin(sb, parent, p->one->path);
-   hashcpy(norigin->blob_sha1, p->one->sha1);
+   hashcpy(norigin->blob_sha1, p->one->oid.hash);
norigin->mode = p->one->mode;
fill_origin_blob(>revs->diffopt, norigin, _p);
if (!file_p.ptr)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 8164b581..c0652a7e 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -368,7 +368,7 @@ static void show_filemodify(struct diff_queue_struct *q,
print_path(spec->path);
putchar('\n');
 
-   if (!hashcmp(ospec->sha1, spec->sha1) &&
+   if (!oidcmp(>oid, >oid) &&
ospec->mode == spec->mode)
break;
/* fallthrough */
@@ -383,10 +383,10 @@ static void show_filemodify(struct diff_queue_struct *q,
if (no_data || S_ISGITLINK(spec->mode))
printf("M %06o %s ", spec->mode,
   sha1_to_hex(anonymize ?
-  anonymize_sha1(spec->sha1) :
-  spec->sha1));
+  
anonymize_sha1(spec->oid.hash) :
+  spec->oid.hash));
else {
-   struct object *object = 
lookup_object(spec->sha1);
+   struct object *object = 
lookup_object(spec->oid.hash);
printf("M %06o :%d ", spec->mode,
   get_object_mark(object));
}
@@ -572,7 +572,7 @@ static void handle_commit(struct commit *commit, struct 
rev_info *rev)
/* Export the referenced blobs, and remember the marks. */
for (i = 0; i < diff_queued_diff.nr; i++)
if (!S_ISGITLINK(diff_queued_diff.queue[i]->two->mode))
-   export_blob(diff_queued_diff.queue[i]->two->sha1);
+   export_blob(diff_queued_diff.queue[i]->two->oid.hash);
 
refname = commit->util;
if (anonymize) {
diff --git a/builtin/reset.c b/builtin/reset.c
index acd62788..ab1fe575 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -121,7 +121,7 @@ static void update_index_from_diff(struct diff_queue_struct 
*q,
 
for (i = 0; i < q->nr; i++) {
struct diff_filespec *one = q->queue[i]->one;
-   int is_missing = !(one->mode && !is_null_sha1(one->sha1));
+  

[PATCH v3 10/11] merge-recursive: convert merge_recursive_generic to object_id

[brian m. carlson]

Convert this function and the git merge-recursive subcommand to use
struct object_id.

Signed-off-by: brian m. carlson 
Signed-off-by: Junio C Hamano 
---
 builtin/merge-recursive.c | 20 ++--
 merge-recursive.c | 14 +++---
 merge-recursive.h |  6 +++---
 3 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
index 491efd55..fd2c4556 100644
--- a/builtin/merge-recursive.c
+++ b/builtin/merge-recursive.c
@@ -9,10 +9,10 @@ static const char builtin_merge_recursive_usage[] =
 
 static const char *better_branch_name(const char *branch)
 {
-   static char githead_env[8 + 40 + 1];
+   static char githead_env[8 + GIT_SHA1_HEXSZ + 1];
char *name;
 
-   if (strlen(branch) != 40)
+   if (strlen(branch) != GIT_SHA1_HEXSZ)
return branch;
xsnprintf(githead_env, sizeof(githead_env), "GITHEAD_%s", branch);
name = getenv(githead_env);
@@ -21,10 +21,10 @@ static const char *better_branch_name(const char *branch)
 
 int cmd_merge_recursive(int argc, const char **argv, const char *prefix)
 {
-   const unsigned char *bases[21];
+   const struct object_id *bases[21];
unsigned bases_count = 0;
int i, failed;
-   unsigned char h1[20], h2[20];
+   struct object_id h1, h2;
struct merge_options o;
struct commit *result;
 
@@ -46,10 +46,10 @@ int cmd_merge_recursive(int argc, const char **argv, const 
char *prefix)
continue;
}
if (bases_count < ARRAY_SIZE(bases)-1) {
-   unsigned char *sha = xmalloc(20);
-   if (get_sha1(argv[i], sha))
+   struct object_id *oid = xmalloc(sizeof(struct 
object_id));
+   if (get_oid(argv[i], oid))
die("Could not parse object '%s'", argv[i]);
-   bases[bases_count++] = sha;
+   bases[bases_count++] = oid;
}
else
warning("Cannot handle more than %d bases. "
@@ -62,9 +62,9 @@ int cmd_merge_recursive(int argc, const char **argv, const 
char *prefix)
o.branch1 = argv[++i];
o.branch2 = argv[++i];
 
-   if (get_sha1(o.branch1, h1))
+   if (get_oid(o.branch1, ))
die("Could not resolve ref '%s'", o.branch1);
-   if (get_sha1(o.branch2, h2))
+   if (get_oid(o.branch2, ))
die("Could not resolve ref '%s'", o.branch2);
 
o.branch1 = better_branch_name(o.branch1);
@@ -73,7 +73,7 @@ int cmd_merge_recursive(int argc, const char **argv, const 
char *prefix)
if (o.verbosity >= 3)
printf("Merging %s with %s\n", o.branch1, o.branch2);
 
-   failed = merge_recursive_generic(, h1, h2, bases_count, bases, 
);
+   failed = merge_recursive_generic(, , , bases_count, bases, 
);
if (failed < 0)
return 128; /* die() error code */
return failed;
diff --git a/merge-recursive.c b/merge-recursive.c
index 7bbd4aea..48fe7e73 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1982,11 +1982,11 @@ int merge_recursive(struct merge_options *o,
return clean;
 }
 
-static struct commit *get_ref(const unsigned char *sha1, const char *name)
+static struct commit *get_ref(const struct object_id *oid, const char *name)
 {
struct object *object;
 
-   object = deref_tag(parse_object(sha1), name, strlen(name));
+   object = deref_tag(parse_object(oid->hash), name, strlen(name));
if (!object)
return NULL;
if (object->type == OBJ_TREE)
@@ -1999,10 +1999,10 @@ static struct commit *get_ref(const unsigned char 
*sha1, const char *name)
 }
 
 int merge_recursive_generic(struct merge_options *o,
-   const unsigned char *head,
-   const unsigned char *merge,
+   const struct object_id *head,
+   const struct object_id *merge,
int num_base_list,
-   const unsigned char **base_list,
+   const struct object_id **base_list,
struct commit **result)
 {
int clean;
@@ -2015,9 +2015,9 @@ int merge_recursive_generic(struct merge_options *o,
int i;
for (i = 0; i < num_base_list; ++i) {
struct commit *base;
-   if (!(base = get_ref(base_list[i], 
sha1_to_hex(base_list[i]
+   if (!(base = get_ref(base_list[i], 
oid_to_hex(base_list[i]
return error(_("Could not parse object '%s'"),
-   sha1_to_hex(base_list[i]));
+   oid_to_hex(base_list[i]));
  

[PATCH v3 08/11] merge-recursive: convert struct merge_file_info to object_id

[brian m. carlson]

Convert struct merge_file_info to use struct object_id.  The following
Coccinelle semantic patch was used to implement this, followed by the
transformations in object_id.cocci:

@@
struct merge_file_info o;
@@
- o.sha
+ o.oid.hash

@@
struct merge_file_info *p;
@@
- p->sha
+ p->oid.hash

Signed-off-by: brian m. carlson 
Signed-off-by: Junio C Hamano 
---
 merge-recursive.c | 39 ---
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index a07050cd..bc455491 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -819,7 +819,7 @@ static void update_file(struct merge_options *o,
 /* Low level file merging, update and removal */
 
 struct merge_file_info {
-   unsigned char sha[20];
+   struct object_id oid;
unsigned mode;
unsigned clean:1,
 merge:1;
@@ -902,10 +902,10 @@ static struct merge_file_info merge_file_1(struct 
merge_options *o,
result.clean = 0;
if (S_ISREG(a->mode)) {
result.mode = a->mode;
-   hashcpy(result.sha, a->oid.hash);
+   oidcpy(, >oid);
} else {
result.mode = b->mode;
-   hashcpy(result.sha, b->oid.hash);
+   oidcpy(, >oid);
}
} else {
if (!sha_eq(a->oid.hash, one->oid.hash) && !sha_eq(b->oid.hash, 
one->oid.hash))
@@ -925,9 +925,9 @@ static struct merge_file_info merge_file_1(struct 
merge_options *o,
}
 
if (sha_eq(a->oid.hash, b->oid.hash) || sha_eq(a->oid.hash, 
one->oid.hash))
-   hashcpy(result.sha, b->oid.hash);
+   oidcpy(, >oid);
else if (sha_eq(b->oid.hash, one->oid.hash))
-   hashcpy(result.sha, a->oid.hash);
+   oidcpy(, >oid);
else if (S_ISREG(a->mode)) {
mmbuffer_t result_buf;
int merge_status;
@@ -939,21 +939,21 @@ static struct merge_file_info merge_file_1(struct 
merge_options *o,
die(_("Failed to execute internal merge"));
 
if (write_sha1_file(result_buf.ptr, result_buf.size,
-   blob_type, result.sha))
+   blob_type, result.oid.hash))
die(_("Unable to add %s to database"),
a->path);
 
free(result_buf.ptr);
result.clean = (merge_status == 0);
} else if (S_ISGITLINK(a->mode)) {
-   result.clean = merge_submodule(result.sha,
+   result.clean = merge_submodule(result.oid.hash,
   one->path,
   one->oid.hash,
   a->oid.hash,
   b->oid.hash,
   !o->call_depth);
} else if (S_ISLNK(a->mode)) {
-   hashcpy(result.sha, a->oid.hash);
+   oidcpy(, >oid);
 
if (!sha_eq(a->oid.hash, b->oid.hash))
result.clean = 0;
@@ -1192,7 +1192,7 @@ static void conflict_rename_rename_1to2(struct 
merge_options *o,
 * pathname and then either rename the add-source file to that
 * unique path, or use that unique path instead of src here.
 */
-   update_file(o, 0, mfi.sha, mfi.mode, one->path);
+   update_file(o, 0, mfi.oid.hash, mfi.mode, one->path);
 
/*
 * Above, we put the merged content at the merge-base's
@@ -1255,16 +1255,16 @@ static void conflict_rename_rename_2to1(struct 
merge_options *o,
 * again later for the non-recursive merge.
 */
remove_file(o, 0, path, 0);
-   update_file(o, 0, mfi_c1.sha, mfi_c1.mode, a->path);
-   update_file(o, 0, mfi_c2.sha, mfi_c2.mode, b->path);
+   update_file(o, 0, mfi_c1.oid.hash, mfi_c1.mode, a->path);
+   update_file(o, 0, mfi_c2.oid.hash, mfi_c2.mode, b->path);
} else {
char *new_path1 = unique_path(o, path, ci->branch1);
char *new_path2 = unique_path(o, path, ci->branch2);
output(o, 1, _("Renaming %s to %s and %s to %s instead"),
   a->path, new_path1, b->path, new_path2);
remove_file(o, 0, path, 0);
-   update_file(o, 0, mfi_c1.sha, mfi_c1.mode, new_path1);
-   update_file(o, 0, mfi_c2.sha, 

[PATCH v3 11/11] diff: convert prep_temp_blob to struct object_id.

[brian m. carlson]

All of the callers of this function use struct object_id, so convert it
to use struct object_id in its arguments and internally.

Signed-off-by: brian m. carlson 
---
 diff.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/diff.c b/diff.c
index 9abb54ad..8cdfdf32 100644
--- a/diff.c
+++ b/diff.c
@@ -2866,7 +2866,7 @@ void diff_free_filespec_data(struct diff_filespec *s)
 static void prep_temp_blob(const char *path, struct diff_tempfile *temp,
   void *blob,
   unsigned long size,
-  const unsigned char *sha1,
+  const struct object_id *oid,
   int mode)
 {
int fd;
@@ -2891,7 +2891,7 @@ static void prep_temp_blob(const char *path, struct 
diff_tempfile *temp,
die_errno("unable to write temp-file");
close_tempfile(>tempfile);
temp->name = get_tempfile_path(>tempfile);
-   sha1_to_hex_r(temp->hex, sha1);
+   oid_to_hex_r(temp->hex, oid);
xsnprintf(temp->mode, sizeof(temp->mode), "%06o", mode);
strbuf_release();
strbuf_release();
@@ -2929,7 +2929,7 @@ static struct diff_tempfile *prepare_temp_file(const char 
*name,
die_errno("readlink(%s)", name);
prep_temp_blob(name, temp, sb.buf, sb.len,
   (one->oid_valid ?
-   one->oid.hash : null_sha1),
+   >oid : _oid),
   (one->oid_valid ?
one->mode : S_IFLNK));
strbuf_release();
@@ -2955,7 +2955,7 @@ static struct diff_tempfile *prepare_temp_file(const char 
*name,
if (diff_populate_filespec(one, 0))
die("cannot read data blob for %s", one->path);
prep_temp_blob(name, temp, one->data, one->size,
-  one->oid.hash, one->mode);
+  >oid, one->mode);
}
return temp;
 }
--
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 v3 09/11] merge-recursive: convert leaf functions to use struct object_id

[brian m. carlson]

Convert all but two of the static functions in this file to use struct
object_id.

Signed-off-by: brian m. carlson 
Signed-off-by: Junio C Hamano 
---
 merge-recursive.c | 236 +++---
 1 file changed, 118 insertions(+), 118 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index bc455491..7bbd4aea 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -56,11 +56,11 @@ static struct commit *make_virtual_commit(struct tree 
*tree, const char *comment
  * Since we use get_tree_entry(), which does not put the read object into
  * the object pool, we cannot rely on a == b.
  */
-static int sha_eq(const unsigned char *a, const unsigned char *b)
+static int oid_eq(const struct object_id *a, const struct object_id *b)
 {
if (!a && !b)
return 2;
-   return a && b && hashcmp(a, b) == 0;
+   return a && b && oidcmp(a, b) == 0;
 }
 
 enum rename_type {
@@ -198,11 +198,11 @@ static void output_commit_title(struct merge_options *o, 
struct commit *commit)
}
 }
 
-static int add_cacheinfo(unsigned int mode, const unsigned char *sha1,
+static int add_cacheinfo(unsigned int mode, const struct object_id *oid,
const char *path, int stage, int refresh, int options)
 {
struct cache_entry *ce;
-   ce = make_cache_entry(mode, sha1 ? sha1 : null_sha1, path, stage,
+   ce = make_cache_entry(mode, oid ? oid->hash : null_sha1, path, stage,
  (refresh ? (CE_MATCH_REFRESH |
  CE_MATCH_IGNORE_MISSING) : 0 ));
if (!ce)
@@ -552,13 +552,13 @@ static int update_stages(const char *path, const struct 
diff_filespec *o,
if (remove_file_from_cache(path))
return -1;
if (o)
-   if (add_cacheinfo(o->mode, o->oid.hash, path, 1, 0, options))
+   if (add_cacheinfo(o->mode, >oid, path, 1, 0, options))
return -1;
if (a)
-   if (add_cacheinfo(a->mode, a->oid.hash, path, 2, 0, options))
+   if (add_cacheinfo(a->mode, >oid, path, 2, 0, options))
return -1;
if (b)
-   if (add_cacheinfo(b->mode, b->oid.hash, path, 3, 0, options))
+   if (add_cacheinfo(b->mode, >oid, path, 3, 0, options))
return -1;
return 0;
 }
@@ -736,7 +736,7 @@ static int make_room_for_path(struct merge_options *o, 
const char *path)
 }
 
 static void update_file_flags(struct merge_options *o,
- const unsigned char *sha,
+ const struct object_id *oid,
  unsigned mode,
  const char *path,
  int update_cache,
@@ -760,11 +760,11 @@ static void update_file_flags(struct merge_options *o,
goto update_index;
}
 
-   buf = read_sha1_file(sha, , );
+   buf = read_sha1_file(oid->hash, , );
if (!buf)
-   die(_("cannot read object %s '%s'"), sha1_to_hex(sha), 
path);
+   die(_("cannot read object %s '%s'"), oid_to_hex(oid), 
path);
if (type != OBJ_BLOB)
-   die(_("blob expected for %s '%s'"), sha1_to_hex(sha), 
path);
+   die(_("blob expected for %s '%s'"), oid_to_hex(oid), 
path);
if (S_ISREG(mode)) {
struct strbuf strbuf = STRBUF_INIT;
if (convert_to_working_tree(path, buf, size, )) {
@@ -799,21 +799,21 @@ static void update_file_flags(struct merge_options *o,
free(lnk);
} else
die(_("do not know what to do with %06o %s '%s'"),
-   mode, sha1_to_hex(sha), path);
+   mode, oid_to_hex(oid), path);
free(buf);
}
  update_index:
if (update_cache)
-   add_cacheinfo(mode, sha, path, 0, update_wd, 
ADD_CACHE_OK_TO_ADD);
+   add_cacheinfo(mode, oid, path, 0, update_wd, 
ADD_CACHE_OK_TO_ADD);
 }
 
 static void update_file(struct merge_options *o,
int clean,
-   const unsigned char *sha,
+   const struct object_id *oid,
unsigned mode,
const char *path)
 {
-   update_file_flags(o, sha, mode, path, o->call_depth || clean, 
!o->call_depth);
+   update_file_flags(o, oid, mode, path, o->call_depth || clean, 
!o->call_depth);
 }
 
 /* Low level file merging, update and removal */
@@ -908,7 +908,7 @@ static struct merge_file_info merge_file_1(struct 
merge_options *o,
oidcpy(, >oid);
}
} else {
-   if (!sha_eq(a->oid.hash, 

[PATCH v3 04/11] coccinelle: apply object_id Coccinelle transformations

[brian m. carlson]

Apply the set of semantic patches from contrib/coccinelle to convert
some leftover places using struct object_id's hash member to instead
use the wrapper functions that take struct object_id natively.

Signed-off-by: brian m. carlson 
Signed-off-by: Junio C Hamano 
---
 bisect.c |  2 +-
 builtin/merge.c  | 13 ++---
 refs/files-backend.c |  4 ++--
 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/bisect.c b/bisect.c
index 6d93edbc..ff147589 100644
--- a/bisect.c
+++ b/bisect.c
@@ -754,7 +754,7 @@ static void handle_bad_merge_base(void)
 static void handle_skipped_merge_base(const unsigned char *mb)
 {
char *mb_hex = sha1_to_hex(mb);
-   char *bad_hex = sha1_to_hex(current_bad_oid->hash);
+   char *bad_hex = oid_to_hex(current_bad_oid);
char *good_hex = join_sha1_array_hex(_revs, ' ');
 
warning("the merge base between %s and [%s] "
diff --git a/builtin/merge.c b/builtin/merge.c
index a9b99c9f..f66d06ce 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -501,7 +501,7 @@ static void merge_name(const char *remote, struct strbuf 
*msg)
if (ref_exists(truname.buf)) {
strbuf_addf(msg,
"%s\t\tbranch '%s'%s of .\n",
-   sha1_to_hex(remote_head->object.oid.hash),
+   oid_to_hex(_head->object.oid),
truname.buf + 11,
(early ? " (early part)" : ""));
strbuf_release();
@@ -515,7 +515,7 @@ static void merge_name(const char *remote, struct strbuf 
*msg)
desc = merge_remote_util(remote_head);
if (desc && desc->obj && desc->obj->type == OBJ_TAG) {
strbuf_addf(msg, "%s\t\t%s '%s'\n",
-   sha1_to_hex(desc->obj->oid.hash),
+   oid_to_hex(>obj->oid),
typename(desc->obj->type),
remote);
goto cleanup;
@@ -523,7 +523,7 @@ static void merge_name(const char *remote, struct strbuf 
*msg)
}
 
strbuf_addf(msg, "%s\t\tcommit '%s'\n",
-   sha1_to_hex(remote_head->object.oid.hash), remote);
+   oid_to_hex(_head->object.oid), remote);
 cleanup:
strbuf_release();
strbuf_release();
@@ -1366,7 +1366,7 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
for (p = remoteheads; p; p = p->next) {
struct commit *commit = p->item;
strbuf_addf(, "GITHEAD_%s",
-   sha1_to_hex(commit->object.oid.hash));
+   oid_to_hex(>object.oid));
setenv(buf.buf, merge_remote_util(commit)->name, 1);
strbuf_reset();
if (fast_forward != FF_ONLY &&
@@ -1425,7 +1425,7 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
goto done;
} else if (fast_forward != FF_NO && !remoteheads->next &&
!common->next &&
-   !hashcmp(common->item->object.oid.hash, 
head_commit->object.oid.hash)) {
+   !oidcmp(>item->object.oid, 
_commit->object.oid)) {
/* Again the most common case of merging one remote. */
struct strbuf msg = STRBUF_INIT;
struct commit *commit;
@@ -1499,8 +1499,7 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
 * HEAD^^" would be missed.
 */
common_one = get_merge_bases(head_commit, j->item);
-   if (hashcmp(common_one->item->object.oid.hash,
-   j->item->object.oid.hash)) {
+   if (oidcmp(_one->item->object.oid, 
>item->object.oid)) {
up_to_date = 0;
break;
}
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 1f380764..dac3a222 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1725,14 +1725,14 @@ static int verify_lock(struct ref_lock *lock,
errno = save_errno;
return -1;
} else {
-   hashclr(lock->old_oid.hash);
+   oidclr(>old_oid);
return 0;
}
}
if (old_sha1 && hashcmp(lock->old_oid.hash, old_sha1)) {
strbuf_addf(err, "ref %s is at %s but expected %s",
lock->ref_name,
-   sha1_to_hex(lock->old_oid.hash),
+   oid_to_hex(>old_oid),
sha1_to_hex(old_sha1));
errno = EBUSY;
return 

[PATCH v3 06/11] diff: rename struct diff_filespec's sha1_valid member

[brian m. carlson]

Now that this struct's sha1 member is called "oid", update the comment
and the sha1_valid member to be called "oid_valid" instead.  The
following Coccinelle semantic patch was used to implement this, followed
by the transformations in object_id.cocci:

@@
struct diff_filespec o;
@@
- o.sha1_valid
+ o.oid_valid

@@
struct diff_filespec *p;
@@
- p->sha1_valid
+ p->oid_valid

Signed-off-by: brian m. carlson 
Signed-off-by: Junio C Hamano 
---
 combine-diff.c|  4 ++--
 diff.c| 28 ++--
 diffcore-break.c  |  2 +-
 diffcore-rename.c |  4 ++--
 diffcore.h|  2 +-
 line-log.c| 10 +-
 6 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/combine-diff.c b/combine-diff.c
index 1e1ad1c0..8e2a577b 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1269,7 +1269,7 @@ static struct diff_filepair *combined_pair(struct 
combine_diff_path *p,
pair->one[i].path = p->path;
pair->one[i].mode = p->parent[i].mode;
oidcpy(>one[i].oid, >parent[i].oid);
-   pair->one[i].sha1_valid = !is_null_oid(>parent[i].oid);
+   pair->one[i].oid_valid = !is_null_oid(>parent[i].oid);
pair->one[i].has_more_entries = 1;
}
pair->one[num_parent - 1].has_more_entries = 0;
@@ -1277,7 +1277,7 @@ static struct diff_filepair *combined_pair(struct 
combine_diff_path *p,
pair->two->path = p->path;
pair->two->mode = p->mode;
oidcpy(>two->oid, >oid);
-   pair->two->sha1_valid = !is_null_oid(>oid);
+   pair->two->oid_valid = !is_null_oid(>oid);
return pair;
 }
 
diff --git a/diff.c b/diff.c
index ae9edc30..9abb54ad 100644
--- a/diff.c
+++ b/diff.c
@@ -1933,7 +1933,7 @@ static void show_dirstat(struct diff_options *options)
 
name = p->two->path ? p->two->path : p->one->path;
 
-   if (p->one->sha1_valid && p->two->sha1_valid)
+   if (p->one->oid_valid && p->two->oid_valid)
content_changed = oidcmp(>one->oid, >two->oid);
else
content_changed = 1;
@@ -2640,7 +2640,7 @@ void fill_filespec(struct diff_filespec *spec, const 
unsigned char *sha1,
if (mode) {
spec->mode = canon_mode(mode);
hashcpy(spec->oid.hash, sha1);
-   spec->sha1_valid = sha1_valid;
+   spec->oid_valid = sha1_valid;
}
 }
 
@@ -2766,7 +2766,7 @@ int diff_populate_filespec(struct diff_filespec *s, 
unsigned int flags)
if (S_ISGITLINK(s->mode))
return diff_populate_gitlink(s, size_only);
 
-   if (!s->sha1_valid ||
+   if (!s->oid_valid ||
reuse_worktree_file(s->path, s->oid.hash, 0)) {
struct strbuf buf = STRBUF_INIT;
struct stat st;
@@ -2915,7 +2915,7 @@ static struct diff_tempfile *prepare_temp_file(const char 
*name,
}
 
if (!S_ISGITLINK(one->mode) &&
-   (!one->sha1_valid ||
+   (!one->oid_valid ||
 reuse_worktree_file(name, one->oid.hash, 1))) {
struct stat st;
if (lstat(name, ) < 0) {
@@ -2928,16 +2928,16 @@ static struct diff_tempfile *prepare_temp_file(const 
char *name,
if (strbuf_readlink(, name, st.st_size) < 0)
die_errno("readlink(%s)", name);
prep_temp_blob(name, temp, sb.buf, sb.len,
-  (one->sha1_valid ?
+  (one->oid_valid ?
one->oid.hash : null_sha1),
-  (one->sha1_valid ?
+  (one->oid_valid ?
one->mode : S_IFLNK));
strbuf_release();
}
else {
/* we can borrow from the file in the work tree */
temp->name = name;
-   if (!one->sha1_valid)
+   if (!one->oid_valid)
sha1_to_hex_r(temp->hex, null_sha1);
else
sha1_to_hex_r(temp->hex, one->oid.hash);
@@ -3134,7 +3134,7 @@ static void run_diff_cmd(const char *pgm,
 static void diff_fill_sha1_info(struct diff_filespec *one)
 {
if (DIFF_FILE_VALID(one)) {
-   if (!one->sha1_valid) {
+   if (!one->oid_valid) {
struct stat st;
if (one->is_stdin) {
oidclr(>oid);
@@ -4172,11 +4172,11 @@ int diff_unmodified_pair(struct diff_filepair *p)
/* both are valid and point at the same path.  that is, we are
 * dealing with a change.
 */
-   if (one->sha1_valid && two->sha1_valid &&
+   if (one->oid_valid 

[PATCH v3 02/11] contrib/coccinelle: add basic Coccinelle transforms

[brian m. carlson]

Coccinelle (http://coccinelle.lip6.fr/) is a program which performs
mechanical transformations on C programs using semantic patches.  These
semantic patches can be used to implement automatic refactoring and
maintenance tasks.

Add a set of basic semantic patches to convert common patterns related
to the struct object_id transformation, as well as a README.

Signed-off-by: brian m. carlson 
Signed-off-by: Junio C Hamano 
---
 contrib/coccinelle/README  |  2 +
 contrib/coccinelle/object_id.cocci | 95 ++
 2 files changed, 97 insertions(+)
 create mode 100644 contrib/coccinelle/README
 create mode 100644 contrib/coccinelle/object_id.cocci

diff --git a/contrib/coccinelle/README b/contrib/coccinelle/README
new file mode 100644
index ..9c2f8879
--- /dev/null
+++ b/contrib/coccinelle/README
@@ -0,0 +1,2 @@
+This directory provides examples of Coccinelle (http://coccinelle.lip6.fr/)
+semantic patches that might be useful to developers.
diff --git a/contrib/coccinelle/object_id.cocci 
b/contrib/coccinelle/object_id.cocci
new file mode 100644
index ..8ccdbb56
--- /dev/null
+++ b/contrib/coccinelle/object_id.cocci
@@ -0,0 +1,95 @@
+@@
+expression E1;
+@@
+- is_null_sha1(E1.hash)
++ is_null_oid()
+
+@@
+expression E1;
+@@
+- is_null_sha1(E1->hash)
++ is_null_oid(E1)
+
+@@
+expression E1;
+@@
+- sha1_to_hex(E1.hash)
++ oid_to_hex()
+
+@@
+expression E1;
+@@
+- sha1_to_hex(E1->hash)
++ oid_to_hex(E1)
+
+@@
+expression E1;
+@@
+- sha1_to_hex_r(E1.hash)
++ oid_to_hex_r()
+
+@@
+expression E1;
+@@
+- sha1_to_hex_r(E1->hash)
++ oid_to_hex_r(E1)
+
+@@
+expression E1;
+@@
+- hashclr(E1.hash)
++ oidclr()
+
+@@
+expression E1;
+@@
+- hashclr(E1->hash)
++ oidclr(E1)
+
+@@
+expression E1, E2;
+@@
+- hashcmp(E1.hash, E2.hash)
++ oidcmp(, )
+
+@@
+expression E1, E2;
+@@
+- hashcmp(E1->hash, E2->hash)
++ oidcmp(E1, E2)
+
+@@
+expression E1, E2;
+@@
+- hashcmp(E1->hash, E2.hash)
++ oidcmp(E1, )
+
+@@
+expression E1, E2;
+@@
+- hashcmp(E1.hash, E2->hash)
++ oidcmp(, E2)
+
+@@
+expression E1, E2;
+@@
+- hashcpy(E1.hash, E2.hash)
++ oidcpy(, )
+
+@@
+expression E1, E2;
+@@
+- hashcpy(E1->hash, E2->hash)
++ oidcpy(E1, E2)
+
+@@
+expression E1, E2;
+@@
+- hashcpy(E1->hash, E2.hash)
++ oidcpy(E1, )
+
+@@
+expression E1, E2;
+@@
+- hashcpy(E1.hash, E2->hash)
++ oidcpy(, E2)
--
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 v3 00/11] struct object_id, Part 4

[brian m. carlson]

This series is part 4 in a series of conversions to replace instances of
unsigned char [20] with struct object_id.  Most of this series touches
the merge-recursive code.

New in this series is the use of Coccinelle (http://coccinelle.lip6.fr/)
semantic patches.  These semantic patches can make automatic
transformations to C source code for cleanup or refactoring reasons.

This series introduces a set of transforms for the struct object_id
transition, cleans up some existing code with them, and applies a small
number of semantic patches to transform parts of the merge-recursive
code.  Some manual refactoring work follows.

Note that in the patches created with the semantic patches, the only
manual change was the definition of the struct member.  Opinions on
whether this is a viable technique for further work to ease both the
creation and review of patches are of course welcomed.

The testsuite continues to pass at each step, and this series rebases
cleanly on next.

I picked up Junio's change from sha_eq to oid_eq, but didn't convert the
calls to oidcmp.  I think the current version (with oid_eq) is actually
more readable than using oidcmp, if slightly less efficient.

Changes from v2:
* Pick up improvements from Junio's version on pu.
* Add oid_to_hex_r.
* Add oid_to_hex_r to object_id.cocci.
* Convert prep_temp_blob as suggested by Junio.
* Converted hashcpy(.*, null_sha1) to hashclr.

Changes from v1:
* Move the object ID transformations to contrib/examples/coccinelle.
* Add a README to that folder explaining what they are.
* Adjust the Coccinelle patches to transform plain structs before
  pointers to structs to avoid misconversions.  This addresses the issue
  that Johannes Sixt caught originally.

brian m. carlson (11):
  hex: add oid_to_hex_r.
  contrib/coccinelle: add basic Coccinelle transforms
  Convert hashcpy with null_sha1 to hashclr.
  coccinelle: apply object_id Coccinelle transformations
  diff: convert struct diff_filespec to struct object_id
  diff: rename struct diff_filespec's sha1_valid member
  merge-recursive: convert struct stage_data to use object_id
  merge-recursive: convert struct merge_file_info to object_id
  merge-recursive: convert leaf functions to use struct object_id
  merge-recursive: convert merge_recursive_generic to object_id
  diff: convert prep_temp_blob to struct object_id.

 bisect.c   |   2 +-
 builtin/blame.c|   6 +-
 builtin/fast-export.c  |  10 +-
 builtin/merge-recursive.c  |  20 +--
 builtin/merge.c|  15 +-
 builtin/reset.c|   4 +-
 builtin/unpack-objects.c   |   4 +-
 cache.h|   1 +
 combine-diff.c |  14 +-
 contrib/coccinelle/README  |   2 +
 contrib/coccinelle/object_id.cocci |  95 
 diff.c |  99 ++--
 diffcore-break.c   |   4 +-
 diffcore-rename.c  |  16 +-
 diffcore.h |   4 +-
 hex.c  |   5 +
 line-log.c |  12 +-
 merge-recursive.c  | 310 +++--
 merge-recursive.h  |   6 +-
 notes-merge.c  |  42 ++---
 refs/files-backend.c   |   4 +-
 submodule-config.c |   2 +-
 submodule.c|   4 +-
 t/helper/test-submodule-config.c   |   2 +-
 wt-status.c|   3 +-
 25 files changed, 403 insertions(+), 283 deletions(-)
 create mode 100644 contrib/coccinelle/README
 create mode 100644 contrib/coccinelle/object_id.cocci

--
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 v3 03/11] Convert hashcpy with null_sha1 to hashclr.

[brian m. carlson]

hashcpy with null_sha1 as the source is equivalent to hashclr.  In
addition to being simpler, using hashclr may give the compiler a chance
to optimize better.  Convert instances of hashcpy with the source
argument of null_sha1 to hashclr.

This transformation was implemented using the following semantic patch:

@@
expression E1;
@@
-hashcpy(E1, null_sha1);
+hashclr(E1);

Signed-off-by: brian m. carlson 
---
 builtin/merge.c  | 2 +-
 builtin/unpack-objects.c | 4 ++--
 diff.c   | 2 +-
 submodule-config.c   | 2 +-
 t/helper/test-submodule-config.c | 2 +-
 5 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index b555a1bf..a9b99c9f 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1530,7 +1530,7 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
 * Stash away the local changes so that we can try more than one.
 */
save_state(stash))
-   hashcpy(stash, null_sha1);
+   hashclr(stash);
 
for (i = 0; i < use_strategies_nr; i++) {
int ret;
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 875e7ed9..172470bf 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -355,7 +355,7 @@ static void unpack_delta_entry(enum object_type type, 
unsigned long delta_size,
return; /* we are done */
else {
/* cannot resolve yet --- queue it */
-   hashcpy(obj_list[nr].sha1, null_sha1);
+   hashclr(obj_list[nr].sha1);
add_delta_to_list(nr, base_sha1, 0, delta_data, 
delta_size);
return;
}
@@ -406,7 +406,7 @@ static void unpack_delta_entry(enum object_type type, 
unsigned long delta_size,
 * The delta base object is itself a delta that
 * has not been resolved yet.
 */
-   hashcpy(obj_list[nr].sha1, null_sha1);
+   hashclr(obj_list[nr].sha1);
add_delta_to_list(nr, null_sha1, base_offset, 
delta_data, delta_size);
return;
}
diff --git a/diff.c b/diff.c
index fa78fc18..f2234012 100644
--- a/diff.c
+++ b/diff.c
@@ -3134,7 +3134,7 @@ static void diff_fill_sha1_info(struct diff_filespec *one)
if (!one->sha1_valid) {
struct stat st;
if (one->is_stdin) {
-   hashcpy(one->sha1, null_sha1);
+   hashclr(one->sha1);
return;
}
if (lstat(one->path, ) < 0)
diff --git a/submodule-config.c b/submodule-config.c
index db1847ff..077db405 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -377,7 +377,7 @@ static int gitmodule_sha1_from_commit(const unsigned char 
*commit_sha1,
int ret = 0;
 
if (is_null_sha1(commit_sha1)) {
-   hashcpy(gitmodules_sha1, null_sha1);
+   hashclr(gitmodules_sha1);
return 1;
}
 
diff --git a/t/helper/test-submodule-config.c b/t/helper/test-submodule-config.c
index dab8c277..7922ffba 100644
--- a/t/helper/test-submodule-config.c
+++ b/t/helper/test-submodule-config.c
@@ -49,7 +49,7 @@ int main(int argc, char **argv)
path_or_name = arg[1];
 
if (commit[0] == '\0')
-   hashcpy(commit_sha1, null_sha1);
+   hashclr(commit_sha1);
else if (get_sha1(commit, commit_sha1) < 0)
die_usage(argc, argv, "Commit not found.");
 
--
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 v3 01/11] hex: add oid_to_hex_r.

[brian m. carlson]

This function works just like sha1_to_hex_r, except that it takes a
pointer to struct object_id instead of a pointer to unsigned char.

Signed-off-by: brian m. carlson 
---
 cache.h | 1 +
 hex.c   | 5 +
 2 files changed, 6 insertions(+)

diff --git a/cache.h b/cache.h
index 6049f867..7bd7c472 100644
--- a/cache.h
+++ b/cache.h
@@ -1193,6 +1193,7 @@ extern int get_oid_hex(const char *hex, struct object_id 
*sha1);
  *   printf("%s -> %s", sha1_to_hex(one), sha1_to_hex(two));
  */
 extern char *sha1_to_hex_r(char *out, const unsigned char *sha1);
+extern char *oid_to_hex_r(char *out, const struct object_id *oid);
 extern char *sha1_to_hex(const unsigned char *sha1);   /* static buffer 
result! */
 extern char *oid_to_hex(const struct object_id *oid);  /* same static buffer 
as sha1_to_hex */
 
diff --git a/hex.c b/hex.c
index 0519f853..9619b67a 100644
--- a/hex.c
+++ b/hex.c
@@ -77,6 +77,11 @@ char *sha1_to_hex_r(char *buffer, const unsigned char *sha1)
return buffer;
 }
 
+char *oid_to_hex_r(char *buffer, const struct object_id *oid)
+{
+   return sha1_to_hex_r(buffer, oid->hash);
+}
+
 char *sha1_to_hex(const unsigned char *sha1)
 {
static int bufno;
--
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] unpack-trees: fix English grammar in do-this-before-that messages

[Vasco Almeida]

Citando Junio C Hamano :


Vasco Almeida  writes:


The only downside I can tell about this is translator are going to have
to update those strings on their translations, but that is a normal
thing to do on an active project like Git.


A larger issue is this fails to update tests that check these
messages.


Yes, I forget about tests :-). Alex Henrie, or someone else, should  
update the tests to reflect this change if it is to go forward.


--
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 1/4] t5000: test tar files that overflow ustar headers

[Junio C Hamano]

Jeff King  writes:

> The difference in time between the two is measurable on my system, but
> it's only a few milliseconds (for 4096 bytes). So maybe it's not worth
> worrying about (though as a general technique, it does make me worry
> that it's easy to get wrong in a way that will fail racily).

Yeah, GNU dd has iflag=fullblock, but if we assume GNU, we can
safely assume "head -c", so I do not think of a way to do this
portably enough.

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

[bug] Reliably Reproducible Bad Packing of Objects

[Christoph Michelbach]

Hi,

when run on a 32 bit system (Linux system, don't know about other
systems),

mkdir test && cd test && git init && touch someFile && git add someFile
&& git commit -m "Initial commit." && dd if=/dev/urandom
of=bigBinaryFile bs=1MB count=4294 && git add bigBinaryFile && git
commit -m "Introduced big biary file."

reliably produces this error message: "error: bad packed object CRC
for"

Since git counts sizes in byte and uses ints for it, it can't handle
files bigger than (2^32 - 1) byte. That's 4'294.967296 MB. If you give
git a file bigger than it can handle, it usually just says "fatal:
Cannot handle files this big" without corrupting the repository. Btw.:
It'd be nice if the error message stated that this only occurs on 32
bit system and only with files 4 GiB in size or bigger.

To provoke the bug, the commands above creates a file which cannot be
compressed slightly less than (2^32 - 1) byte big, probably resulting
in a commit more than (2^32 - 1) byte big.

I was able to reproduce the bug on a Raspberry Pi Model 3 (ARMv7 CPU)
and a virtual machine running Ubuntu 16.04 32 bit (which was
specifically set up to test this, so it was a clean installation) on a
host running Ubuntu 16.04 64 bit on an ARM 64 bit x86 CPU (i7-4720HQ).

Output on raspi: https://gist.github.com/m1cm1c/d874f03be5b12cbd8b86ced
79fa456d1
Output on virt. machine: https://gist.github.com/m1cm1c/d0dd47828386bb0
f1e001b9f750416e0

Note that on the raspi I concatenated `git gc` to the end to show that
the object exists but is corrupt but you can already see that something
went wrong before `git gc` is executed.

If you look at the output on the virtual machine, however, you will not
see that something wrong until you read the part where I typed in `git
gc` which I find particularly worrying.

When checking whether you get the same result, please make sure to use
git 1.7.6 or newer. If you use an older version of git (older versions
are still being distributed, for example for Ubuntu 14.04), git will
try to memory-map a too big file resulting in a different error.

-- 
With kind regards
Christoph Michelbach

--
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] unpack-trees: fix English grammar in do-this-before-that messages

[Junio C Hamano]

Vasco Almeida  writes:

> The only downside I can tell about this is translator are going to have
> to update those strings on their translations, but that is a normal
> thing to do on an active project like Git.

A larger issue is this fails to update tests that check these
messages.

--
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] diff: let --output= default to --no-color

[Junio C Hamano]

Johannes Schindelin  writes:

> It is highly unlikely that any user would want to see ANSI color
> sequences in a file. So let's stop doing that by default.
>
> This is a backwards-incompatible change.
>
> The reason this was not caught earlier is most likely that either
> --output= is not used, or only when stdout is redirected
> anyway.
>
> Technically, we do not default to --no-color here. Instead, we try to
> override the GIT_COLOR_AUTO default because it would let want_color()
> test whether stdout (instead of the specified file) is connected to a
> terminal. Practically, we require the user to require color "always"
> to force writing ANSI color sequences to the output file.
>
> Signed-off-by: Johannes Schindelin 
> ---
> Published-As: https://github.com/dscho/git/releases/tag/diff-o-v1
>
>   Just something I noted while working on a bit more consistency
>   with the diffopt.file patches.
>
>   This is a backwards-incompatible change, though. So I extracted it
>   from the patch series.

I think this is a bugfix.  Even though we are writing to a file that
is separate from the standard output (because we explicitly opened
that file outselves), --color=auto would still let want_color() make
the decision based on isatty(1) with hardcoded 1.  That does not
simply make sense.

If we imagine that your format-patch series with 06/10 and 07/10
swapped, then the "do not freopen(), instead point opened file with
diffopt.file" step would be seen as introducing the same bug as the
bug this patch fixes, so in that sense it is an equivalent to 06/10.
We do not need to view this patch as a compatibility-breaking
change, I would think.

Perhaps I should tweak 06/10 to assign GIT_COLOR_NEVER not 0 while
queuing it.

Thanks.

>  diff.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/diff.c b/diff.c
> index fa78fc1..b66b9be 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3977,6 +3977,8 @@ int diff_opt_parse(struct diff_options *options,
>   if (!options->file)
>   die_errno("Could not open '%s'", path);
>   options->close_file = 1;
> + if (options->use_color != GIT_COLOR_ALWAYS)
> + options->use_color = GIT_COLOR_NEVER;
>   return argcount;
>   } else
>   return 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 08/10] format-patch: use stdout directly

[Junio C Hamano]

Johannes Schindelin  writes:

> Earlier, we freopen()ed stdout in order to write patches to files.
> That forced us to duplicate stdout (naming it "realstdout") because we
> *still* wanted to be able to report the file names.
>
> As we do not abuse stdout that way anymore, we no longer need to
> duplicate stdout, either.
>
> Signed-off-by: Johannes Schindelin 
> ---
>  builtin/log.c | 6 +-
>  1 file changed, 1 insertion(+), 5 deletions(-)

The logical consequence of 07/10; very nice.
--
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/10] format-patch: explicitly switch off color when writing to files

[Junio C Hamano]

Johannes Schindelin  writes:

> We rely on the auto-detection ("is stdout a terminal?") to determine
> whether to use color in the output of format-patch or not. That happens
> to work because we freopen() stdout when redirecting the output to files.
>
> However, we are about to fix that work-around, in which case the
> auto-detection has no chance to guess whether to use color or not.
>
> But then, we do not need to guess to begin with. As argued in the commit
> message of 7787570c (format-patch: ignore ui.color, 2011-09-13), we do not
> allow the ui.color setting to affect format-patch's output. The only time,
> therefore, that we allow color sequences to be written to the output files
> is when the user specified the --color command-line option explicitly.
>
> Signed-off-by: Johannes Schindelin 
> ---

The right fix in the longer term (long after this series lands, that
is) is probably to update the world view that the codepath from
want_color_auto() to check_auto_color() has always held.  In their
world view, when they are asked to make --color=auto decision, the
output always goes the standard output, and that is why they
hardcode isatty(1) to decide.  The existing freopen() was a part of
that world view.

We'd need a workaround like this patch if we want to leave the
want_color_auto() as-is, and as a workaround I think this is the
least invasive one, so let's queue it as-is.

If the codepaths that use diffopt.file (not just this one that is
about output directory hence known to be writing to a file, but all
the log/diff family of commands after this series up to 5/10 has
been applied) have a way to tell want_color_auto() that the output
is going to fileno(diffopt.file), and have check_auto_color() use
that fd instead of the hardcoded 1, the problem this step is trying
to address goes away, and I think that would be the longer-term fix.

Thanks.

>  builtin/log.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 27bc88d..5683a42 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1578,6 +1578,8 @@ int cmd_format_patch(int argc, const char **argv, const 
> char *prefix)
>   setup_pager();
>  
>   if (output_directory) {
> + if (rev.diffopt.use_color != GIT_COLOR_ALWAYS)
> + rev.diffopt.use_color = 0;
>   if (use_stdout)
>   die(_("standard output, or directory, which one?"));
>   if (mkdir(output_directory, 0777) < 0 && errno != EEXIST)
--
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 1/4] tests: factor portable signal check out of t0005

[Johannes Sixt]

Am 24.06.2016 um 23:05 schrieb Jeff King:

On Fri, Jun 24, 2016 at 10:52:32PM +0200, Johannes Sixt wrote:

The Windows behavior is most closely described as having signal(SIGPIPE,
SIG_IGN) at the very beginning of the program.


Right, but then we would get EPIPE. So what does git do in such cases?
I'd expect it generally to either hit the check_pipe() part of
write_or_die(), or to end up complaining via die() that the write didn't
go as expected.


Ah, I have forgotten about this code path. Looks like it will trigger 
exactly the same raise() as test_sigchain. Then the check for exit code 
3 makes a bit more sense. But I'm sure we have code paths that do not go 
through checK_pipe(). Those would proceed through whatever error 
handling we have and most likely die().



IMO there is too much danger to trigger a false positive if exit code 3 is
treated special in this generality.


Yeah, I agree. But what _should_ it do? E.g., what happens to git-daemon
when it is killed via TERM?


Frankly, I don't know how bash's 'kill -TERM' and a Windows program 
interact. I've avoided this topic like the plague so far.


-- Hannes

--
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 1/4] t5000: test tar files that overflow ustar headers

[Jeff King]

On Fri, Jun 24, 2016 at 04:58:58PM -0400, Eric Sunshine wrote:

> On Fri, Jun 24, 2016 at 3:07 PM, Jeff King  wrote:
> > On Fri, Jun 24, 2016 at 11:56:19AM -0700, Junio C Hamano wrote:
> >> Jeff King  writes:
> >> > +tar_info () {
> >> > +   "$TAR" tvf "$1" | awk '{print $3 " " $4}' | cut -d- -f1
> >> > +}
> >
> >> Seeing an awk piped into cut always makes me want to suggest a
> >> single sed/awk/perl invocation.
> >
> > I want the auto-splitting of awk, but then to auto-split the result
> > using a different delimiter. Is there a not-painful way to do that in
> > awk?
> 
> The awk split() function is POSIX and accepts an optional separator argument.

Thanks. I'm not that familiar with awk functions, simply because I came
of age after perl existed, and using perl tends to be more portable and
powerful (if you can assume it's available). But this is simple enough
that it should be OK.

Replacing it with:

"$TAR" tvf "$1" |
awk '{
split($4, date, "-")
print $3 " " date[1]
}'

seems to work.

-Peff
--
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 1/4] tests: factor portable signal check out of t0005

[Jeff King]

On Fri, Jun 24, 2016 at 10:52:32PM +0200, Johannes Sixt wrote:

> Am 24.06.2016 um 21:43 schrieb Jeff King:
> > In POSIX shells, a program which exits due to a signal
> > generally has an exit code of 128 plus the signal number.
> > However, some platforms do other things. ksh uses 256 plus
> > the signal number, and on Windows, all signals are just "3".
> 
> That's not true, see below.

I was worried about that. Git for Windows seems like a labyrinth of
bizarre special cases.

> > I didn't get into the weirdness of SIGPIPE on Windows here, but I think
> > this is probably a first step toward handling it better. E.g., it may be
> > that test_match_signal should respect 128 (or even any code) when we are
> > checking for SIGPIPE.
> 
> The Windows behavior is most closely described as having signal(SIGPIPE,
> SIG_IGN) at the very beginning of the program.

Right, but then we would get EPIPE. So what does git do in such cases?
I'd expect it generally to either hit the check_pipe() part of
write_or_die(), or to end up complaining via die() that the write didn't
go as expected.

> > +# Returns true if the numeric exit code in "$2" represents the expected 
> > signal
> > +# in "$1". Signals should be given numerically.
> > +test_match_signal () {
> > +   if test "$2" = "$((128 + $1))"
> > +   then
> > +   # POSIX
> > +   return 0
> > +   elif test "$2" = "$((256 + $1))"
> > +   then
> > +   # ksh
> > +   return 0
> > +   elif test "$2" = "3"; then
> > +   # Windows
> 
> You meant well here, but this is close to pointless as a general check. We
> check for this exit code in t0005 because there program termination happens
> via raise(), which on Window just calls exit(3). This exit code is not an
> indication that something related to SIGPIPE (or any signal) happened.
> 
> IMO there is too much danger to trigger a false positive if exit code 3 is
> treated special in this generality.

Yeah, I agree. But what _should_ it do? E.g., what happens to git-daemon
when it is killed via TERM?

-Peff
--
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 01/10] Prepare log/log-tree to reuse the diffopt.close_file attribute

[Junio C Hamano]

Johannes Schindelin  writes:

> We are about to teach the log-tree machinery to reuse the diffopt.file
> field to output to a file stream other than stdout, in line with the
> diff machinery already writing to diffopt.file.
>
> However, we might want to write something after the diff in
> log_tree_commit() (e.g. with the --show-linear-break option), therefore
> we must not let the diff machinery close the file (as per
> diffopt.close_file.
>
> This means that log_tree_commit() itself must override the
> diffopt.close_file flag and close the file, and if log_tree_commit() is
> called in a loop, the caller is responsible to do the same.

Makes sense.

> Note: format-patch has an `--output-directory` option. Due to the fact
> that format-patch's options are parsed first, and that the parse-options
> machinery accepts uniquely abbreviated options, the diff options
> `--output` (and `-o`) are shadowed. Therefore close_file is not set to 1
> so that cmd_format_patch() does *not* need to handle the close_file flag
> differently, even if it calls log_tree_commit() in a loop.
>
> Signed-off-by: Johannes Schindelin 
> ---
>  builtin/log.c | 15 ---
>  log-tree.c|  5 -
>  2 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 099f4f7..27bc88d 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -243,9 +243,10 @@ static struct itimerval early_output_timer;
>  
>  static void log_show_early(struct rev_info *revs, struct commit_list *list)
>  {
> - int i = revs->early_output;
> + int i = revs->early_output, close_file = revs->diffopt.close_file;

Probably not worth a reroll, but I find this kind of thing easier to
read as two separate definitions.

>   int show_header = 1;

And this was separate from "int i = ...;" for the same reason.  It
is OK to write "int i, j, k;" but "show_header" is something that
keeps track of the more important state during the control flow and
it is easier to read if we make it stand out.  close_file falls into
the same category, I would think.

>   case commit_error:
> + if (close_file)
> + fclose(revs->diffopt.file);

I wondered if we want to also clear, i.e. revs->diffopt.file = NULL, 
but I do not think .close_file does that either, so this is good.

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 1/4] tests: factor portable signal check out of t0005

[Johannes Sixt]

Am 24.06.2016 um 21:43 schrieb Jeff King:

In POSIX shells, a program which exits due to a signal
generally has an exit code of 128 plus the signal number.
However, some platforms do other things. ksh uses 256 plus
the signal number, and on Windows, all signals are just "3".


That's not true, see below.


I didn't get into the weirdness of SIGPIPE on Windows here, but I think
this is probably a first step toward handling it better. E.g., it may be
that test_match_signal should respect 128 (or even any code) when we are
checking for SIGPIPE.


The Windows behavior is most closely described as having signal(SIGPIPE, 
SIG_IGN) at the very beginning of the program.



+# Returns true if the numeric exit code in "$2" represents the expected signal
+# in "$1". Signals should be given numerically.
+test_match_signal () {
+   if test "$2" = "$((128 + $1))"
+   then
+   # POSIX
+   return 0
+   elif test "$2" = "$((256 + $1))"
+   then
+   # ksh
+   return 0
+   elif test "$2" = "3"; then
+   # Windows


You meant well here, but this is close to pointless as a general check. 
We check for this exit code in t0005 because there program termination 
happens via raise(), which on Window just calls exit(3). This exit code 
is not an indication that something related to SIGPIPE (or any signal) 
happened.


IMO there is too much danger to trigger a false positive if exit code 3 
is treated special in this generality.



+   return 0
+   fi
+   return 1
+}


-- Hannes

--
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 1/4] t5000: test tar files that overflow ustar headers

[Jeff King]

On Fri, Jun 24, 2016 at 03:07:44PM -0400, Jeff King wrote:

> > "dd bs=1 count=4096" is hopefully more portable.
> 
> Hmm. I always wonder whether dd is actually very portable, but we do use
> it already, at least.
> 
> Perhaps the perl monstrosity in t9300 could be replaced with that, too.

Hrm. So I wrote a patch for t9300 for this. But I wanted to flip the
order to:

  dd bs=4096 count=1

because otherwise, dd will call read() 4096 times, for 1 byte each.

But it's not safe to do that on a pipe. For example:

  {
echo 1
sleep 1
echo 2
  } | dd bs=4 count=1

will copy only 2 bytes. So it's racily wrong, depending on how the
writer feeds the data to write().

The 1-byte reads do work (assuming blocking descriptors and that dd
restarts a read after a signal, which mine seems to). But yuck.

The difference in time between the two is measurable on my system, but
it's only a few milliseconds (for 4096 bytes). So maybe it's not worth
worrying about (though as a general technique, it does make me worry
that it's easy to get wrong in a way that will fail racily).

-Peff
--
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 1/4] t5000: test tar files that overflow ustar headers

[Eric Sunshine]

On Fri, Jun 24, 2016 at 3:07 PM, Jeff King  wrote:
> On Fri, Jun 24, 2016 at 11:56:19AM -0700, Junio C Hamano wrote:
>> Jeff King  writes:
>> > +tar_info () {
>> > +   "$TAR" tvf "$1" | awk '{print $3 " " $4}' | cut -d- -f1
>> > +}
>
>> Seeing an awk piped into cut always makes me want to suggest a
>> single sed/awk/perl invocation.
>
> I want the auto-splitting of awk, but then to auto-split the result
> using a different delimiter. Is there a not-painful way to do that in
> awk?

The awk split() function is POSIX and accepts an optional separator argument.
--
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] refs/files-backend: mark a file-local symbol static

[Ramsay Jones]

Commit 6d41edc6 ("refs: add methods for reflog", 24-02-2016), moved the
reflog handling into the ref-storage backend. In particular, the
files_reflog_iterator_begin() API was removed from internal refs API
header, resulting in sparse warnings.

Signed-off-by: Ramsay Jones 
---

Hi Michael,

If you need to re-roll your 'mh/ref-store' branch, could you please
squash this into the relevant patch. (or something like it - if you
think this function should be public, then re-introduce the declaration
to the header).

Thanks!

ATB,
Ramsay Jones

 refs/files-backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index b6ce19f..426e439 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3331,7 +3331,7 @@ static struct ref_iterator_vtable 
files_reflog_iterator_vtable = {
files_reflog_iterator_abort
 };
 
-struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_store)
+static struct ref_iterator *files_reflog_iterator_begin(struct ref_store 
*ref_store)
 {
struct files_reflog_iterator *iter = xcalloc(1, sizeof(*iter));
struct ref_iterator *ref_iterator = >base;
-- 
2.9.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

Loan offer for Projects Finance (1.5 -1).

[Peter K. Durst - .]

Sir,

I am a retired banker, currently working as a consultant.

By virtue of my past working experience, I have contacts to high net worth 
Individuals and Investment firms in need to place funds into different projects 
to generate profits. The funding would be provided as a soft loan at 3% Return 
on Investment (ROI) with minimal documentations required to complete the 
funding modalities.

A commission of 1% is paid to the Intermediary/Agent/Broker who introduce 
project owners for finance.

I anticipate your reply.

Yours faithfully,

Peter Karl Durst




Club Services
Participation & Club Support Department

England Golf

Tel: 01526 354500

Web: www.englandgolf.org

Follow us on Twitter: 
www.twitter.com/EnglandGolf

Find us on Facebook: www.facebook.com/EnglandGolf

The English Golf Union Ltd is 
a company limited by Guarantee registered in England, trading as England Golf.  
Registered number: 5564108. Registered Office: The National Golf Centre, The 
Broadway, 
Woodhall Spa, Lincolnshire, LN10 6PU.

If you are not the intended 
recipient, and disclosure, copying, distribution or any action taken or omitted 
in reliance on this is prohibited and may be unlawful. If you have received 
this 
message in error, please notify us and remove it from your system.  No 
liability or responsibility is accepted if information or data is, for whatever 
reason corrupted or does not reach its intended recipient.  No Warranty is 
given that this email is free of viruses.  The views expressed in this 
email are, unless otherwise stated, those of the author and not those of The 
English Golf Union Limited or its management. The English Golf Union Limited 
reserves the right to monitor, intercept and block emails addressed to its 
users 
or take any other action in accordance with its email use policy.

--
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] Refactor recv_sideband()

[Dennis Kaarsemaker]

On vr, 2016-06-24 at 14:14 -0400, Jeff King wrote:

> On Fri, Jun 24, 2016 at 07:45:04PM +0200, Johannes Schindelin wrote:
>> Do we *actually* send color via the sideband, like, ever?

> We don't, but remember that we forward arbitrary output from hooks.
> If the consensus is "nah, it is probably crazy and nobody is doing it
> today" then I am fine with that.

It's crazy, so obviously we're doing it :)

Though losing this would not be a big deal for us.

D.
--
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-svn aborts with "Use of uninitialized value $u" when a non-svn-backed branch is present in remote

[Eric Wong]

Please don't drop Cc:, re-adding git@vger and Christian

Jacob Godserv  wrote:
> > Christian (Cc-ed) also noticed the problem a few weeks ago
> > and took a more drastic approach by having git-svn die
> > instead of warning:
> > http://mid.gmane.org/1462604323-18545-1-git-send-email-chrisc...@tuxfamily.org
> > which landed as commit 523a33ca17c76bee007d7394fb3930266c577c02
> > in git.git: https://bogomips.org/mirrors/git.git/patch?id=523a33ca17c7
> >
> > Is dying here too drastic and maybe warn is preferable?
> 
> In my opinion this is too drastic. It keeps me from storing
> git-specific data on a git-svn mirror.

I tend to agree, but will wait to see what Christian thinks.

> Here's my setup:
>  - My git-svn mirror uses git-svn to create a git repo that mirrors
> svn history. This repository is then pushed to a clean bare
> repository. So far so good. Only svn-sourced branches exist.
>  - The git-svn mirror script also saves a copy of the git-svn
> configuration used to generate the git mirror repository in an
> "orphaned" branch called something like 'git-svn-conf'. This is
> completely separate from the svn history, and exists only for my
> git-svn purposes.
>  - On the "client" side, another script I wrote knows how to parse the
> git-svn configuration in that 'git-svn-conf' branch to properly
> reconfigure git-svn on the local machine, so I can use 'git svn'
> themselves to commit, etc., and still generate the same hashes so
> there's no forked history during the next mirror fetch.
> 
> Long story short: I have branches which aren't in SVN history for
> automated git-svn purposes.
> 
> It appears that simply skipping the branch in that loop fixes the
> issue. However, I don't know how the metadata is stored and what
> exactly that loop does, so I may be creating hidden side effects I
> have been lucky enough to not trigger yet.
--
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/4] portable signal-checking in tests

[Jeff King]

On Fri, Jun 24, 2016 at 03:39:24PM -0400, Jeff King wrote:

> Anyway. Here's a series that I think makes things better, and it is not
> too painful to do.
> 
>   [1/4]: tests: factor portable signal check out of t0005
>   [2/4]: t0005: use test_match_signal as appropriate
>   [3/4]: test_must_fail: use test_match_signal
>   [4/4]: t/lib-git-daemon: use test_match_signal

Oh, and I meant to mention: this covers everything I found grepping for
"141" and "143" in the test suite (though you have to filter the results
a bit for false positives).

It doesn't fix the case newly added in the tar series that started this
discussion. I don't want to hold either topic hostage to the other, so
I'll prepare a patch to go on top.

-Peff
--
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] doc: git-htmldocs.googlecode.com is no more

[Junio C Hamano]

Matthieu Moy  writes:

> Junio C Hamano  writes:
>
>> On Wed, Jun 22, 2016 at 12:00 PM, Eric Wong  wrote:
>>>
>>> Just wondering, who updates
>>> https://kernel.org/pub/software/scm/git/docs/
>>> and why hasn't it been updated in a while?
>>> (currently it says Last updated 2015-06-06 at the bottom)
>>
>> Nobody. It is too cumbersome to use their upload tool to update many
>> files (it is geared towards updating a handful of tarballs at a time).
>
> Then, I guess it would make sense to remove it to avoid pointing users
> to outdated docs?

I'll see what I can do at k.org these days.

Don't hold your breath, don't stay tuned, but don't be surprised if
you see a positive change in the future ;-)

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 2/4] archive-tar: write extended headers for file sizes >= 8GB

[Jeff King]

On Fri, Jun 24, 2016 at 12:45:05PM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > So if anything, I would put a comment here, explaining that ustar cannot
> > handle anything larger than this, and POSIX mandates it (but I didn't
> > because the commit message already goes into much more detail).
> 
> That sounds like a good thing to do.  Not everybody runs "blame" to
> find the commit whose log message she must read before touching a
> line.

Well, they should. My commit messages are works of literature. ;)

I'll add a comment.

-Peff
--
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 2/4] archive-tar: write extended headers for file sizes >= 8GB

[Junio C Hamano]

Jeff King  writes:

> So if anything, I would put a comment here, explaining that ustar cannot
> handle anything larger than this, and POSIX mandates it (but I didn't
> because the commit message already goes into much more detail).

That sounds like a good thing to do.  Not everybody runs "blame" to
find the commit whose log message she must read before touching a
line.


--
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 4/4] t/lib-git-daemon: use test_match_signal

[Jeff King]

When git-daemon exits, we expect it to be with the SIGTERM
we just sent it. If we see anything else, we'll complain.
But our check against exit code "143" is not portable. For
example:

  $ ksh93 t5570-git-daemon.sh
  [...]
  error: git daemon exited with status: 271

We can fix this by using test_match_signal.

Signed-off-by: Jeff King 
---
 t/lib-git-daemon.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index 340534c..f9cbd47 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -82,8 +82,7 @@ stop_git_daemon() {
kill "$GIT_DAEMON_PID"
wait "$GIT_DAEMON_PID" >&3 2>&4
ret=$?
-   # expect exit with status 143 = 128+15 for signal TERM=15
-   if test $ret -ne 143
+   if test_match_signal 15 $?
then
error "git daemon exited with status: $ret"
fi
-- 
2.9.0.215.gc5c4261
--
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/4] test_must_fail: use test_match_signal

[Jeff King]

In 8bf4bec (add "ok=sigpipe" to test_must_fail and use it to
fix flaky tests, 2015-11-27), test_must_fail learned to
recognize "141" as a sigpipe failure. However, testing for
a signal is more complicated than that; we should use
test_match_signal to implement more portable checking.

Signed-off-by: Jeff King 
---
 t/test-lib-functions.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 51d3775..91d3b58 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -612,7 +612,7 @@ test_must_fail () {
then
echo >&2 "test_must_fail: command succeeded: $*"
return 1
-   elif test $exit_code -eq 141 && list_contains "$_test_ok" sigpipe
+   elif test_match_signal 13 $exit_code && list_contains "$_test_ok" 
sigpipe
then
return 0
elif test $exit_code -gt 129 && test $exit_code -le 192
-- 
2.9.0.215.gc5c4261

--
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/4] t0005: use test_match_signal as appropriate

[Jeff King]

The first test already uses this more portable construct
(that was where it was factored from initially), but the
later tests do a raw comparison against 141 to look for
SIGPIPE, which can fail on some shells and platforms.

Signed-off-by: Jeff King 
---
Again, I couldn't test these. They're skipped on MINGW, and ksh93 barfs
before even executing the tests.

 t/t0005-signals.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t0005-signals.sh b/t/t0005-signals.sh
index 2d96265..c80c995 100755
--- a/t/t0005-signals.sh
+++ b/t/t0005-signals.sh
@@ -36,12 +36,12 @@ test_expect_success 'create blob' '
 
 test_expect_success !MINGW 'a constipated git dies with SIGPIPE' '
OUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 ) &&
-   test "$OUT" -eq 141
+   test_match_signal 13 "$OUT"
 '
 
 test_expect_success !MINGW 'a constipated git dies with SIGPIPE even if parent 
ignores it' '
OUT=$( ((trap "" PIPE; large_git; echo $? 1>&3) | :) 3>&1 ) &&
-   test "$OUT" -eq 141
+   test_match_signal 13 "$OUT"
 '
 
 test_done
-- 
2.9.0.215.gc5c4261

--
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 1/4] t5000: test tar files that overflow ustar headers

[Junio C Hamano]

Jeff King  writes:

>> > +# When parsing, we'll pull out only the year from the date; that
>> > +# avoids any question of timezones impacting the result. 
>> 
>> ... as long as the month-day part is not close to the year boundary.
>> So this explanation is insuffucient to convince the reader that
>> "that avoids any question" is correct, without saying that it is in
>> August of year 4147.
>
> I thought that part didn't need to be said, but I can say it
> (technically we can include the month, too, but I don't think that level
> of accuracy is really important for these tests).

Oh, I wasn't suggesting to include the month in the comparison.  But
to understand why it is safe from TZ jitters to test only year, the
reader needs to know (or do the math herself) that the timestamp is
away from the year boundary, so mentioning August in the justifying
comment is needed.

>> Seeing an awk piped into cut always makes me want to suggest a
>> single sed/awk/perl invocation.
>
> I want the auto-splitting of awk, but then to auto-split the result
> using a different delimiter. Is there a not-painful way to do that in
> awk?
>
> I could certainly come up with a regex to do it in sed, but I wanted to
> keep the parsing as liberal and generic as possible.
>
> Certainly I could do it in perl, but I had the general impression that
> we prefer to keep the dependency on perl to a minimum. Maybe it doesn't
> matter.

Heh.  It was merely "makes me want to suggest", not "I suggest".  If
I were doing this myself, I would have done a single sed but it does
not matter.

> I think we would want something more like:
>
>   test_signal_match 13 $(cat exit-code)

I like that.
--
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/4] tests: factor portable signal check out of t0005

[Jeff King]

In POSIX shells, a program which exits due to a signal
generally has an exit code of 128 plus the signal number.
However, some platforms do other things. ksh uses 256 plus
the signal number, and on Windows, all signals are just "3".

We've accounted for that in t0005, but not in other tests.
Let's pull out the logic so we can use it elsewhere.

It would be nice for debugging if this additionally printed
errors to stderr, like our other test_* helpers. But we're
going to need to use it in other places besides the innards
of a test_expect block. So let's leave it as generic as
possible.

Signed-off-by: Jeff King 
---
I didn't get into the weirdness of SIGPIPE on Windows here, but I think
this is probably a first step toward handling it better. E.g., it may be
that test_match_signal should respect 128 (or even any code) when we are
checking for SIGPIPE.

I also didn't bother with symbolic names. We could make:

  test_match_signal sigterm $?

work, but I didn't think it was worth the effort. While numbers for some
obscure signals do vary on platforms, sigpipe and sigterm are standard
enough to rely on.

 t/t0005-signals.sh  |  7 +--
 t/test-lib-functions.sh | 18 ++
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/t/t0005-signals.sh b/t/t0005-signals.sh
index e7f27eb..2d96265 100755
--- a/t/t0005-signals.sh
+++ b/t/t0005-signals.sh
@@ -11,12 +11,7 @@ EOF
 
 test_expect_success 'sigchain works' '
{ test-sigchain >actual; ret=$?; } &&
-   case "$ret" in
-   143) true ;; # POSIX w/ SIGTERM=15
-   271) true ;; # ksh w/ SIGTERM=15
- 3) true ;; # Windows
- *) false ;;
-   esac &&
+   test_match_signal 15 "$ret" &&
test_cmp expect actual
 '
 
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 48884d5..51d3775 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -961,3 +961,21 @@ test_env () {
done
)
 }
+
+# Returns true if the numeric exit code in "$2" represents the expected signal
+# in "$1". Signals should be given numerically.
+test_match_signal () {
+   if test "$2" = "$((128 + $1))"
+   then
+   # POSIX
+   return 0
+   elif test "$2" = "$((256 + $1))"
+   then
+   # ksh
+   return 0
+   elif test "$2" = "3"; then
+   # Windows
+   return 0
+   fi
+   return 1
+}
-- 
2.9.0.215.gc5c4261

--
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/4] portable signal-checking in tests

[Jeff King]

On Fri, Jun 24, 2016 at 07:05:14PM +0200, Johannes Sixt wrote:

> Am 24.06.2016 um 18:46 schrieb Jeff King:
> > On Fri, Jun 24, 2016 at 06:38:55PM +0200, Johannes Sixt wrote:
> > > It's going to be 269 with ksh, and who-knows-what on Windows (due to lack 
> > > of
> > > SIGPIPE - I haven't tested this, yet).
> > 
> > Thanks, I meant to ask about that. We do a workaround in t0005, but we
> > _don't_ do it in the new sigpipe handling for test_must_fail. Is the
> > latter just broken, too?
> 
> That's well possible. It is not prepared to see ksh's exit codes for
> signals.

I'm actually not convinced that old versions of ksh are viable for
running the test suite. mksh seems to use POSIX semantics, and I cannot
get through t0005 with ksh93, as it parses nested parentheses wrong. But
maybe there are other ksh variants that use the funny exit codes, but
are otherwise not too buggy.

I'd be more concerned with Windows. The SIGPIPE tests in t0005 are
already marked !MINGW, but other checks elsewhere are not. I know there
is no SIGPIPE on Windows, so it may be that some cases happen to work
because we end up in write_or_die(), which converts EPIPE into a 141
exit.

Anyway. Here's a series that I think makes things better, and it is not
too painful to do.

  [1/4]: tests: factor portable signal check out of t0005
  [2/4]: t0005: use test_match_signal as appropriate
  [3/4]: test_must_fail: use test_match_signal
  [4/4]: t/lib-git-daemon: use test_match_signal

-Peff
--
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-svn aborts with "Use of uninitialized value $u" when a non-svn-backed branch is present in remote

[Eric Wong]

Jacob Godserv  wrote:
> On Tue, Sep 22, 2015 at 2:48 PM, Jacob Godserv  wrote:
> > I found a specific case in which git-svn improperly aborts:
> >
> > 1. I created a git-svn repository, named "git-svn repo", by cloned an
> > svn repository via the git-svn tool.
> > 2. I created a normal git repository, named "configuration repo". I
> > renamed the master branch to "configuration". The initial commit adds
> > a README and some utility scripts.
> > 3. I created a bare repository, named "master repo".
> > 4. I pushed from the git-svn repo to the master repo.
> > 5. I pushed from the configuration repo to the master repo.
> >
> > The idea is the configuration branch, which is detached from any
> > git-svn history, can contain some useful tools, defaults, etc., that I
> > can share with teammates who want to use git on this svn project. It's
> > an odd use of git, but it has been working well.
> >
> > However, a vanilla distribution of Git for Windows 2.5.2 produces the
> > following error when running any git-svn command, such as "git svn
> > info", on the cloned master repo:
> >
> > Use of uninitialized value $u in substitution (s///) at
> > /mingw64/share/perl5/site_perl/Git/SVN.pm line 105.
> > Use of uninitialized value $u in concatenation (.) or string at
> > /mingw64/share/perl5/site_perl/Git/SVN.pm line 105.
> > refs/remotes/origin/configuration: 'svn+ssh://10.0.1.1/repos/projectA'
> > not found in ''
> >
> > In the mentioned SVN.pm file, after the line:
> >
> > my $u = (::cmt_metadata("$refname"))[0];
> >
> > I added the following four lines:
> >
> > if (not defined $u) {
> > warn "W: $refname does not exist in
> > SVN; skipping";
> > next;
> > }

Christian (Cc-ed) also noticed the problem a few weeks ago
and took a more drastic approach by having git-svn die
instead of warning:
http://mid.gmane.org/1462604323-18545-1-git-send-email-chrisc...@tuxfamily.org
which landed as commit 523a33ca17c76bee007d7394fb3930266c577c02
in git.git: https://bogomips.org/mirrors/git.git/patch?id=523a33ca17c7

Is dying here too drastic and maybe warn is preferable?

> > git-svn appears to operate correctly with this patch. This is my first
> > time ever editing a perl script, so I apologize if I murdered an
> > adorable animal just now.
> >
> > I'm sending this in so more knowledgeable git-svn developers can
> > comment on this and fix this in the official distribution of git,
> > assuming there is a bug here to fix.
> >
> > --
> > Jacob
> 
> This e-mail has gone ignored several months. Is the maintainer of
> git-svn on this mailing list? Should I submit this issue elsewhere?

Sorry, I wasn't paying attention to the list at that time.
It is customary to Cc: authors of the code in question
(that also decentralizes our workflow in case vger is down),
and also acceptable to send reminders after a week or two
in case we're overloaded with other work.
--
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 3/4] archive-tar: write extended headers for far-future mtime

[Jeff King]

On Fri, Jun 24, 2016 at 12:06:59PM -0700, Junio C Hamano wrote:

> > +   if (args->time > 0777UL) {
> > +   strbuf_append_ext_header_uint(_header, "mtime",
> > + args->time);
> > +   args->time = 0777UL;
> > +   }
> > +
> > +   if (!ext_header.len)
> > +   return 0;
> 
> Another symbolic constant to explain this, e.g. TAR_TIME_LIMIT, may
> want to exist.

This one at least appears twice. I think one of the reasons I am
slightly resistant to a symbolic constant is that it tempts people to
think that it's OK to change it. It's not. These values are mandated by
POSIX, and must match the size of the ustar header field.

So the least-repetitive thing would be to define it as:

  (1UL << (1 + (3 * (sizeof(ustar_header.mtime) - 1 - 1

That's pretty horrible to read, but if wrapped in a symbolic constant,
at least people would think twice before touching it. ;)

-Peff
--
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 2/4] archive-tar: write extended headers for file sizes >= 8GB

[Jeff King]

On Fri, Jun 24, 2016 at 12:01:16PM -0700, Junio C Hamano wrote:

> > @@ -267,7 +281,13 @@ static int write_tar_entry(struct archiver_args *args,
> > memcpy(header.linkname, buffer, size);
> > }
> >  
> > -   prepare_header(args, , mode, size);
> > +   size_in_header = size;
> > +   if (S_ISREG(mode) && size > 0777UL) {
> 
> Want a symbolic constant with a comment that says why you have
> eleven 7's?

I tried instead to make sure we only mention it once to avoid a symbolic
constant (even though the same constant appears in the next patch, too,
it would be a mistake to give them the same name; they just happen to be
the same size).

So if anything, I would put a comment here, explaining that ustar cannot
handle anything larger than this, and POSIX mandates it (but I didn't
because the commit message already goes into much more detail).

-Peff
--
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 4/4] archive-tar: drop return value

[Junio C Hamano]

Jeff King  writes:

> On Fri, Jun 24, 2016 at 01:49:24PM +0200, Remi Galan Alfonso wrote:
>
>> Hi Peff,
>> 
>> Jeff King  writes:
>> > @@ -413,7 +411,7 @@ static int write_tar_archive(const struct archiver *ar,
>> >  {
>> >  int err = 0;
>> >  
>> > -err = write_global_extended_header(args);
>> > +write_global_extended_header(args);
>> >  if (!err)
>> >  err = write_archive_entries(args, write_tar_entry);
>> 
>> If we drop the error code from 'write_global_extended_header' then the
>> above 'if (!err)' becomes useless (always evaluates to 'true' since
>> 'err' is set to '0').
>
> Thanks, you're right.
>
> I wondered if we could drop "err" entirely, but write_archive_entries()
> does indeed have some error code paths (everybody uses write_or_die, but
> we return an error for things like unknown file types).

You could if you did this ;-)

Which I do not necessarily think is a good idea.

This may be easier to read

write_global_extended_header(args)
err = write_archive_entries(args, write_tar_entry);
if (!err)
write_trailer();
return err;

though.

 archive-tar.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/archive-tar.c b/archive-tar.c
index 903b74d..eed2c96 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -412,11 +412,9 @@ static int write_tar_archive(const struct archiver *ar,
int err = 0;
 
write_global_extended_header(args);
-   if (!err)
-   err = write_archive_entries(args, write_tar_entry);
-   if (!err)
-   write_trailer();
-   return err;
+
+   return (write_archive_entries(args, write_tar_entry) ||
+   write_trailer());
 }
 
 static int write_tar_filter_archive(const struct archiver *ar,
--
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 1/4] t5000: test tar files that overflow ustar headers

[Jeff King]

On Fri, Jun 24, 2016 at 11:56:19AM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > The ustar format only has room for 11 (or 12, depending on
> > some implementations) octal digits for the size and mtime of
> > each file. After this, we have to add pax extended headers
> > to specify the real data, and git does not yet know how to
> > do so.
> 
> I am not a native speaker but "After" above made me hiccup.  I think
> I am correct to understand that it means "after passing this limit",
> aka "to represent files bigger or newer than these", but still it
> felt somewhat strange.

Yeah, I agree that it reads badly. I'm not sure what I was thinking.
I'll tweak it in the re-roll.

> > +# See if our system tar can handle a tar file with huge sizes and dates 
> > far in
> > +# the future, and that we can actually parse its output.
> > +#
> > +# The reference file was generated by GNU tar, and the magic time and size 
> > are
> > +# both octal 010001, which overflows normal ustar fields.
> > +#
> > +# When parsing, we'll pull out only the year from the date; that
> > +# avoids any question of timezones impacting the result. 
> 
> ... as long as the month-day part is not close to the year boundary.
> So this explanation is insuffucient to convince the reader that
> "that avoids any question" is correct, without saying that it is in
> August of year 4147.

I thought that part didn't need to be said, but I can say it
(technically we can include the month, too, but I don't think that level
of accuracy is really important for these tests).

> > +tar_info () {
> > +   "$TAR" tvf "$1" | awk '{print $3 " " $4}' | cut -d- -f1
> > +}
> 
> A blank after the shell function to make it easier to see the
> boundary.

I was intentionally trying to couple it with prereq below, as the
comment describes both of them.

> Seeing an awk piped into cut always makes me want to suggest a
> single sed/awk/perl invocation.

I want the auto-splitting of awk, but then to auto-split the result
using a different delimiter. Is there a not-painful way to do that in
awk?

I could certainly come up with a regex to do it in sed, but I wanted to
keep the parsing as liberal and generic as possible.

Certainly I could do it in perl, but I had the general impression that
we prefer to keep the dependency on perl to a minimum. Maybe it doesn't
matter.

> > +# We expect git to die with SIGPIPE here (otherwise we
> > +# would generate the whole 64GB).
> > +test_expect_failure BUNZIP 'generate tar with huge size' '
> > +   {
> > +   git archive HEAD
> > +   echo $? >exit-code
> > +   } | head -c 4096 >huge.tar &&
> > +   echo 141 >expect &&
> > +   test_cmp expect exit-code
> > +'
> 
> "head -c" is GNU-ism, isn't it?

You're right; for some reason I thought it was in POSIX.

We do have a couple instances of it, but they are all in the valgrind
setup code (which I guess most people don't ever run).

> "dd bs=1 count=4096" is hopefully more portable.

Hmm. I always wonder whether dd is actually very portable, but we do use
it already, at least.

Perhaps the perl monstrosity in t9300 could be replaced with that, too.

> ksh signal death you already know about.  I wonder if we want to
> expose something like list_contains as a friend of test_cmp.
> 
>   list_contains 141,269 $(cat exit-code)

I think we would want something more like:

  test_signal_match 13 $(cat exit-code)

Each call site should not have to know about every signal convention
(and in your example, the magic "3" of Windows is left out).

-Peff
--
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 3/4] archive-tar: write extended headers for far-future mtime

[Junio C Hamano]

Jeff King  writes:

> There's a slight bit of trickiness there. We may already be
> ...
> After writing the extended header, we munge the timestamp in
> the ustar headers to the maximum-allowable size. This is
> wrong, but it's the least-wrong thing we can provide to a
> tar implementation that doesn't understand pax headers (it's
> also what GNU tar does).

The above looks very sensible design, and its implementation is
surprisingly compact.  Very nicely done.

> Helped-by: René Scharfe 
> Signed-off-by: Jeff King 
> ---
>  archive-tar.c   | 16 +---
>  t/t5000-tar-tree.sh |  4 ++--
>  2 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/archive-tar.c b/archive-tar.c
> index 274bdfa..0bb164c 100644
> --- a/archive-tar.c
> +++ b/archive-tar.c
> @@ -317,7 +317,18 @@ static int write_global_extended_header(struct 
> archiver_args *args)
>   unsigned int mode;
>   int err = 0;
>  
> - strbuf_append_ext_header(_header, "comment", sha1_to_hex(sha1), 40);
> + if (sha1)
> + strbuf_append_ext_header(_header, "comment",
> +  sha1_to_hex(sha1), 40);
> + if (args->time > 0777UL) {
> + strbuf_append_ext_header_uint(_header, "mtime",
> +   args->time);
> + args->time = 0777UL;
> + }
> +
> + if (!ext_header.len)
> + return 0;

Another symbolic constant to explain this, e.g. TAR_TIME_LIMIT, may
want to exist.

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 2/4] archive-tar: write extended headers for file sizes >= 8GB

[Junio C Hamano]

Jeff King  writes:

> If the size was 64GB or greater, then we actually overflowed
> digits into the mtime field, meaning our value was was

was was

> effectively right-shifted by those lost octal digits. And
> this patch is again a strict improvement over that.

> diff --git a/archive-tar.c b/archive-tar.c
> index cb99df2..274bdfa 100644
> --- a/archive-tar.c
> +++ b/archive-tar.c
> @@ -137,6 +137,20 @@ static void strbuf_append_ext_header(struct strbuf *sb, 
> const char *keyword,
>   strbuf_addch(sb, '\n');
>  }
>  
> +/*
> + * Like strbuf_append_ext_header, but for numeric values.
> + */
> +static void strbuf_append_ext_header_uint(struct strbuf *sb,
> +   const char *keyword,
> +   uintmax_t value)
> +{
> + char buf[40]; /* big enough for 2^128 in decimal, plus NUL */
> + int len;
> +
> + len = xsnprintf(buf, sizeof(buf), "%"PRIuMAX, value);
> + strbuf_append_ext_header(sb, keyword, buf, len);
> +}
> +
>  static unsigned int ustar_header_chksum(const struct ustar_header *header)
>  {
>   const unsigned char *p = (const unsigned char *)header;
> @@ -208,7 +222,7 @@ static int write_tar_entry(struct archiver_args *args,
>   struct ustar_header header;
>   struct strbuf ext_header = STRBUF_INIT;
>   unsigned int old_mode = mode;
> - unsigned long size;
> + unsigned long size, size_in_header;
>   void *buffer;
>   int err = 0;
>  
> @@ -267,7 +281,13 @@ static int write_tar_entry(struct archiver_args *args,
>   memcpy(header.linkname, buffer, size);
>   }
>  
> - prepare_header(args, , mode, size);
> + size_in_header = size;
> + if (S_ISREG(mode) && size > 0777UL) {

Want a symbolic constant with a comment that says why you have
eleven 7's?

> + size_in_header = 0;
> + strbuf_append_ext_header_uint(_header, "size", size);
> + }
> +
> + prepare_header(args, , mode, size_in_header);

The change itself makes sense.

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 1/4] t5000: test tar files that overflow ustar headers

[Junio C Hamano]

Jeff King  writes:

> The ustar format only has room for 11 (or 12, depending on
> some implementations) octal digits for the size and mtime of
> each file. After this, we have to add pax extended headers
> to specify the real data, and git does not yet know how to
> do so.

I am not a native speaker but "After" above made me hiccup.  I think
I am correct to understand that it means "after passing this limit",
aka "to represent files bigger or newer than these", but still it
felt somewhat strange.

> So as a prerequisite, we can feed the system tar a reference
> tarball to make sure it can handle these features. The
> reference tar here was created with:
>
>   dd if=/dev/zero seek=64G bs=1 count=1 of=huge
>   touch -d @68719476737 huge
>   tar cf - --format=pax |
>   head -c 2048
>
> using GNU tar. Note that this is not a complete tarfile, but
> it's enough to contain the headers we want to examine.

Cute.  I didn't remember they had @ format,
even though I must have seen what they do while working on 2c733fb2
(parse_date(): '@' prefix forces git-timestamp, 2012-02-02).

> +# See if our system tar can handle a tar file with huge sizes and dates far 
> in
> +# the future, and that we can actually parse its output.
> +#
> +# The reference file was generated by GNU tar, and the magic time and size 
> are
> +# both octal 010001, which overflows normal ustar fields.
> +#
> +# When parsing, we'll pull out only the year from the date; that
> +# avoids any question of timezones impacting the result. 

... as long as the month-day part is not close to the year boundary.
So this explanation is insuffucient to convince the reader that
"that avoids any question" is correct, without saying that it is in
August of year 4147.

> +tar_info () {
> + "$TAR" tvf "$1" | awk '{print $3 " " $4}' | cut -d- -f1
> +}

A blank after the shell function to make it easier to see the
boundary.

Seeing an awk piped into cut always makes me want to suggest a
single sed/awk/perl invocation.

> +# We expect git to die with SIGPIPE here (otherwise we
> +# would generate the whole 64GB).
> +test_expect_failure BUNZIP 'generate tar with huge size' '
> + {
> + git archive HEAD
> + echo $? >exit-code
> + } | head -c 4096 >huge.tar &&
> + echo 141 >expect &&
> + test_cmp expect exit-code
> +'

"head -c" is GNU-ism, isn't it?

"dd bs=1 count=4096" is hopefully more portable.

ksh signal death you already know about.  I wonder if we want to
expose something like list_contains as a friend of test_cmp.

list_contains 141,269 $(cat exit-code)

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] Refactor recv_sideband()

[Junio C Hamano]

On Fri, Jun 24, 2016 at 11:14 AM, Jeff King  wrote:
>
> I do wonder about the ANSI_SUFFIX thing, though.

compat/winansi.c seems to have a provision for 'K' (and obviously 'm'
for colors).
--
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] pathspec: warn on empty strings as pathspec

[Emily Xie]

I suppose I got ahead of myself there. :-) Thanks for the
clarification on the process.

On Thu, Jun 23, 2016 at 2:17 AM, Junio C Hamano  wrote:
> Emily Xie  writes:
>
>> Awesome. Thanks, Junio---this is exciting!
>
> No, thank _you_.
>
>> As for step 2: do you have a good sense on the timing? Around how long
>> should I wait to submit the patch for it?
>
> Not so fast.  We'll wait for others to comment first.
>
> I am queuing this change but that does not mean anything more than
> that I am not outright rejecting the idea.
>
> It is possible that others may assess the cost of having to do the
> two-step migration differently, and the list concensus may end up
> being "if it hurts, don't pass an empty string", i.e. we'd better
> without this patch or subsequent second step.  If that happens, the
> first step dies without hitting 'next'.  I'd say we'd wait at least
> for a week.
>
> Otherwise, if the change is received favourably, we'll merge it to
> 'next', do the same waiting for comments.  Repeat the same and then
> merge to 'master'.  After it hits the next feature release (probably
> at around the end of August), the change will be seen by wider
> general public, and at that point we may see strong opposition from
> them with a good argument neither of us thought of why we shouldn't
> do this change, in which case we might have to revise the plan and
> scrap the whole thing.
>
> So, we'll wait and see.
>



-- 
Emily Xie
xie-emily.com
207.399.6370
--
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: Migrating away from SHA-1?

[brian m. carlson]

On Sat, Jun 18, 2016 at 03:10:27AM +0100, Leo Gaspard wrote:
> First, sorry for not having this message threaded: I'm not subscribed to
> the list and haven't found a way to get a Message-Id from gmane.

Sorry it's taken so long to get back to this.  I've been at a
conference.

> So, my questions to the git team:
>  * Is there a consensus, that git should migrate away from SHA-1 before
> it gets a collision attack, because it would mean chosen-prefix
> collision isn't far away and people wouldn't have the time to upgrade?

I plan on adding support for a new hash as soon as that's possible, but
I don't have a firm timeline.  This is a volunteer effort in my own
limited free time.

>  * Is there a consensus, that Peter Anvin's amended transition plan is
> the way to go?

I'm not planning on changing algorithms in the middle of a repository.
This will only be available on new or imported repositories.

My current thinking on proposed algorithms is SHA3-256 or BLAKE2b-256.
The cryptanalysis on SHA-256 indicates that it may not be a great
long-term choice, and I expect people won't want to change algorithms
frequently.

If time becomes extremely urgent, we can always add support for a
160-bit hash first (e.g. BLAKE2b-160) and then finish the object_id
transition later as it becomes convenient.  I'd like to avoid that,
though.

>  * If the two conditions above are fulfilled, has work started on it
> yet? (I guess as Brian Carlson had started his work 9 weeks ago and he
> was speaking about working on it on the week-end he should have finished
> it now, so excluding this)

It takes a long time to get a patch series through.  I'm rather busy and
don't always have time to rebase and address issues during the week.

>  * If the two first conditions are fulfilled, is there anything I could
> do to help this transition? (including helping Brian if his work hasn't
> actually ended yet)

You're welcome to send patches if you like.  I try to avoid areas I know
are under heavy development, like the refs code.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH v2] Refactor recv_sideband()

[Jeff King]

On Fri, Jun 24, 2016 at 07:45:04PM +0200, Johannes Schindelin wrote:

> >   $ git grep -A2 IMPORTANT color.h
> >   color.h: * IMPORTANT: Due to the way these color codes are emulated on 
> > Windows,
> >   color.h- * write them only using printf(), fprintf(), and fputs(). In 
> > particular,
> >   color.h- * do not use puts() or write().
> > 
> > Your patch converts some fprintf calls to write. What does this mean
> > on Windows for:
> > 
> >   1. Remote servers which send ANSI codes and expect them to look
> >  reasonable (this might be a losing proposition already, as the
> >  server won't know anything about the user's terminal, or whether
> >  output is going to a file).
> > 
> >   2. The use of ANSI_SUFFIX in this function, which uses a similar
> >  escape code.
> 
> Wow, I did not even think that a remote server would send color codes,
> what with the server being unable to query the local terminal via
> isatty().
> 
> Do we *actually* send color via the sideband, like, ever?

We don't, but remember that we forward arbitrary output from hooks.

If the consensus is "nah, it is probably crazy and nobody is doing it
today" then I am fine with that.

I do wonder about the ANSI_SUFFIX thing, though.

-Peff
--
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] l10n: de.po: fix translation of autostash

[Ralf Thielow]

Signed-off-by: Ralf Thielow 
---
 po/de.po | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/po/de.po b/po/de.po
index d50cb1b..fdf4d92 100644
--- a/po/de.po
+++ b/po/de.po
@@ -12408,7 +12408,7 @@ msgstr ""
 
 #: git-rebase.sh:168
 msgid "Applied autostash."
-msgstr "\"autostash\" angewendet."
+msgstr "Automatischen Stash angewendet."
 
 #: git-rebase.sh:171
 #, sh-format
@@ -12421,7 +12421,7 @@ msgid ""
 "Your changes are safe in the stash.\n"
 "You can run \"git stash pop\" or \"git stash drop\" at any time.\n"
 msgstr ""
-"Anwendung von \"autostash\" resultierte in Konflikten.\n"
+"Anwendung des automatischen Stash resultierte in Konflikten.\n"
 "Ihre Änderungen sind im Stash sicher.\n"
 "Sie können jederzeit \"git stash pop\" oder \"git stash drop\" ausführen.\n"
 
@@ -12508,12 +12508,12 @@ msgstr "fatal: Branch $branch_name nicht gefunden"
 
 #: git-rebase.sh:558
 msgid "Cannot autostash"
-msgstr "Kann \"autostash\" nicht ausführen."
+msgstr "Kann automatischen Stash nicht anwenden."
 
 #: git-rebase.sh:563
 #, sh-format
 msgid "Created autostash: $stash_abbrev"
-msgstr "\"autostash\" erzeugt: $stash_abbrev"
+msgstr "Automatischen Stash erzeugt: $stash_abbrev"
 
 #: git-rebase.sh:567
 msgid "Please commit or stash them."
-- 
2.9.0.129.g44ae68f

--
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] Refactor recv_sideband()

[Johannes Schindelin]

Hi Peff,

On Fri, 24 Jun 2016, Jeff King wrote:

> On Tue, Jun 14, 2016 at 11:00:38PM +0200, Lukas Fleischer wrote:
> 
> > Improve the readability of recv_sideband() significantly by replacing
> > fragile buffer manipulations with string buffers and more sophisticated
> > format strings. Note that each line is printed using a single write()
> > syscall to avoid garbled output when multiple processes write to stderr
> > in parallel, see 9ac13ec (atomic write for sideband remote messages,
> > 2006-10-11) for details.
> > 
> > Also, reorganize the overall control flow, remove some superfluous
> > variables and replace a custom implementation of strpbrk() with a call
> > to the standard C library function.
> 
> I happened to be looking at the color-printing code yesterday, and was
> reminded that on Windows, fprintf is responsible for converting ANSI
> codes into colors the terminal can show:

Thanks for remembering!

>   $ git grep -A2 IMPORTANT color.h
>   color.h: * IMPORTANT: Due to the way these color codes are emulated on 
> Windows,
>   color.h- * write them only using printf(), fprintf(), and fputs(). In 
> particular,
>   color.h- * do not use puts() or write().
> 
> Your patch converts some fprintf calls to write. What does this mean
> on Windows for:
> 
>   1. Remote servers which send ANSI codes and expect them to look
>  reasonable (this might be a losing proposition already, as the
>  server won't know anything about the user's terminal, or whether
>  output is going to a file).
> 
>   2. The use of ANSI_SUFFIX in this function, which uses a similar
>  escape code.

Wow, I did not even think that a remote server would send color codes,
what with the server being unable to query the local terminal via
isatty().

Do we *actually* send color via the sideband, like, ever?

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

Re: Short-term plans for the post 2.9 cycle

[Jeff King]

On Fri, Jun 24, 2016 at 09:54:21AM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > ...  It's
> > not a flag day for either, of course; we'll build in all of the usual
> > backwards-compatibility flags. But it's convenient for users to remember
> > that "3.0" is the minimum to support a new slate of
> > backwards-incompatible features.
> >
> > So my inclination is that the next version is simply v2.10. And maybe
> > you thought of all of this already, and that's why you didn't even
> > bother mentioning it. :) I'm just curious to hear any thoughts on the
> > matter.
> 
> You traced my thought very precisely.  If you take the "It is for
> compatibility breaking release" and "We plan such a release well in
> advance with transition period" together, a natural consequence is
> that by the time we tag one release (e.g. v2.9), it is expected that
> the release notes for it and a few previous releases would all have
> said "in v3.0, this and that things need to be adjusted, but the
> past few releases should have prepared all of you for that change".
> 
> So, no. I do not think the next one can sensibly be v3.0.
> 
> During this cycle what can happen at most is that harbingers of
> compatibility breakers conceived, transition plans for them laid
> out, and the first step for these compatibility breakers included.
> That will still not qualify for a version bump.  The follow-up steps
> for these compatibility breakers may start cooking in 'next', and
> during the next cycle the list may agree they are ready for the
> upcoming release.  At that point, before tagging the last release in
> v2.x series, we already know that the cycle after that will be v3.0
> to include these compatibility breakers.

Thanks, that spells out much better my "we are not ready" thoughts, and
I am in complete agreement.

-Peff
--
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/2] strbuf: improve API

[Jeff King]

On Thu, Jun 02, 2016 at 01:11:56PM +0200, Michael Haggerty wrote:

> On 06/01/2016 11:07 PM, Jeff King wrote:
> > On Wed, Jun 01, 2016 at 03:42:18AM -0400, Jeff King wrote:
> > 
> >> I have no idea if those ideas would work. But I wouldn't want to start
> >> looking into either of them without some idea of how much time we're
> >> actually spending on strbuf mallocs (or how much time we would spend if
> >> strbufs were used in some proposed sites).
> > 
> > So I tried to come up with some numbers.
> > 
> > Here's an utterly silly use of strbufs, but one that I think should
> > over-emphasize the effect of any improvements we make:
> [...]
> 
> Thanks for generating actual data.

Thanks for following up on this, and sorry for my slow response. It got
put in my "to think about and respond to later" pile (never a good place
to be :) ).

> Your benchmark could do two things to stress malloc()/free()
> even more. I'm not claiming that this makes the benchmark more typical
> of real-world use, but it maybe gets us closer to the theoretical upper
> limit on improvement.

Yeah, you're right. I added more writes to try to actually stimulate the
strbuf to grow, but if the point is to call malloc in a tight loop, it
works against that (I agree that malloc-in-a-tight-loop does not
necessarily represent a real workload, but I think the point of this
exercise is to make sure we can get a useful speedup even under
theoretical conditions).

> I ran this for a short string ("short") and a long string ("this is a
> string that we will repeatedly insert"), and also concatenating the
> string 5, 20, or 500 times. The number of loops around the whole program
> is normalized to make the total number of concatenations approximately
> constant. Here are the full results:

I suspect "5 short" and "5 long" are the only really interesting cases.
For the cases we're talking about, the author clearly expects most input
to fit into a single small-ish allocation (or else they would not bother
with the fixed strbuf in the first place). So "500 long" is really just
exercising memcpy.

> Test   0  1  2  3  4
> 
> 5 short strings 1.64   3.37   8.72   6.08   3.65
> 20 short strings1.72   2.12   5.43   4.01   3.39
> 500 short strings   1.62   1.61   3.36   3.26   3.10
> 5 long strings  2.08   6.64  13.09   8.50   3.79
> 20 long strings 2.16   3.33   7.03   4.72   3.55
> 500 long strings2.04   2.10   3.61   3.33   3.26
> 
> 
> Column 0 is approximately the "bare metal" approach, with a
> pre-allocated buffer and no strbuf overhead.
> 
> Column 1 is like column 0, except allocating a correctly-sized buffer
> each time through the loop. This increases the runtime by as much as 219%.

Makes sense. malloc() isn't free (no pun intended).

And I think this shows why the 20/500 cases aren't all that interesting,
as we really just end up measuring memcpy.

> Column 2 is a naive use of strbuf, where each time through the loop the
> strbuf is reinitialized to STRBUF_INIT, and managing the space is
> entirely left to strbuf.

I'd think this would be about the same as column 1 for our interesting
cases, modulo some strbuf overhead. But it's way higher. That implies
that either:

  1. Strbuf overhead is high (and I think in a really tight loop like
 this that things like our overflow checks might start to matter; we
 could be doing those with intrinsics, for example).

  2. We're doing multiple mallocs per strbuf, which implies our initial
 sizes could be a bit more aggressive (it's not like the 30-byte
 "short" case is that huge).

> Column 3 is like column 2, except that it initializes the strbuf to the
> correct size right away using strbuf_init(). This reduces the runtime
> relative to column 2 by as much as 35%.

That should show us the effects of multiple-allocations (my (2) above).
And indeed, it counts for some of it but not all.

> Column 4 uses a simulated "fixed strbuf", where the fixed-size buffer is
> big enough for the full string (thus there are no calls to
> malloc()/realloc()/free()).
> 
> The comparison between columns 0 and 4 shows that using a strbuf costs
> as much as 123% more than using a simple char array, even if the strbuf
> doesn't have to do any memory allocations.

Yeah, I can believe there is overhead in all of the "are we big enough"
checks (though I'd hope that for the no-allocation cases we simply rely
on snprintf to tell us "yes, you fit").

> The comparison between columns 3 and 4 shows savings a reduction in
> runtime of up to 55% from using a "fixed strbuf" rather than a pre-sized
> conventional strbuf. I think this is the comparison that is most
> relevant to the current discussion.

I'm including a patch at the end of this mail that implements a "variant
5", which essentially keeps a single-buffer cache of the last-released
strbuf, and reuses it. Other than that, it is identical to 2 (no tricks,
not 

Re: [PATCH v3 1/4] t5000: test tar files that overflow ustar headers

[Johannes Sixt]

Am 24.06.2016 um 18:46 schrieb Jeff King:

On Fri, Jun 24, 2016 at 06:38:55PM +0200, Johannes Sixt wrote:

It's going to be 269 with ksh, and who-knows-what on Windows (due to lack of
SIGPIPE - I haven't tested this, yet).


Thanks, I meant to ask about that. We do a workaround in t0005, but we
_don't_ do it in the new sigpipe handling for test_must_fail. Is the
latter just broken, too?


That's well possible. It is not prepared to see ksh's exit codes for 
signals.


-- Hannes

--
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: Short-term plans for the post 2.9 cycle

[Junio C Hamano]

Jeff King  writes:

> ...  It's
> not a flag day for either, of course; we'll build in all of the usual
> backwards-compatibility flags. But it's convenient for users to remember
> that "3.0" is the minimum to support a new slate of
> backwards-incompatible features.
>
> So my inclination is that the next version is simply v2.10. And maybe
> you thought of all of this already, and that's why you didn't even
> bother mentioning it. :) I'm just curious to hear any thoughts on the
> matter.

You traced my thought very precisely.  If you take the "It is for
compatibility breaking release" and "We plan such a release well in
advance with transition period" together, a natural consequence is
that by the time we tag one release (e.g. v2.9), it is expected that
the release notes for it and a few previous releases would all have
said "in v3.0, this and that things need to be adjusted, but the
past few releases should have prepared all of you for that change".

So, no. I do not think the next one can sensibly be v3.0.

During this cycle what can happen at most is that harbingers of
compatibility breakers conceived, transition plans for them laid
out, and the first step for these compatibility breakers included.
That will still not qualify for a version bump.  The follow-up steps
for these compatibility breakers may start cooking in 'next', and
during the next cycle the list may agree they are ready for the
upcoming release.  At that point, before tagging the last release in
v2.x series, we already know that the cycle after that will be v3.0
to include these compatibility breakers.


--
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 1/4] t5000: test tar files that overflow ustar headers

[Jeff King]

On Fri, Jun 24, 2016 at 06:38:55PM +0200, Johannes Sixt wrote:

> Am 24.06.2016 um 01:20 schrieb Jeff King:
> > +# We expect git to die with SIGPIPE here (otherwise we
> > +# would generate the whole 64GB).
> > +test_expect_failure BUNZIP 'generate tar with huge size' '
> > +   {
> > +   git archive HEAD
> > +   echo $? >exit-code
> > +   } | head -c 4096 >huge.tar &&
> > +   echo 141 >expect &&
> > +   test_cmp expect exit-code
> 
> It's going to be 269 with ksh, and who-knows-what on Windows (due to lack of
> SIGPIPE - I haven't tested this, yet).

Thanks, I meant to ask about that. We do a workaround in t0005, but we
_don't_ do it in the new sigpipe handling for test_must_fail. Is the
latter just broken, too?

-Peff
--
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: name for A..B ranges?

[Junio C Hamano]

Jeff King  writes:

> On Wed, Jun 22, 2016 at 08:25:59AM +0100, Philip Oakley wrote:
>
>> Is there a common name for the A..B range format (two dots) that would
>> complement the A...B (three dots) symmetric range format's name?
>> 
>> I was looking at the --left-right distinctions and noticed that the trail
>> back to the symmetric range description was rather thin (it's buried within
>> gitrevisions:Specifying Ranges, and even then its called a symmetric
>> difference.
>
> I would just call it a range, or possibly a set difference. But I don't
> think we have any established naming beyond that.

Yup, I think "range" is the commonly used word in discussions here.
When inventing A...B as a new thing in addition to A..B, we called
the former "symmetric difference", and what is implied by that is
the latter is "asymmetric difference"; we do not say that unless we
are contrasting between the two, though.
--
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 1/4] t5000: test tar files that overflow ustar headers

[Johannes Sixt]

Am 24.06.2016 um 01:20 schrieb Jeff King:

+# We expect git to die with SIGPIPE here (otherwise we
+# would generate the whole 64GB).
+test_expect_failure BUNZIP 'generate tar with huge size' '
+   {
+   git archive HEAD
+   echo $? >exit-code
+   } | head -c 4096 >huge.tar &&
+   echo 141 >expect &&
+   test_cmp expect exit-code


It's going to be 269 with ksh, and who-knows-what on Windows (due to 
lack of SIGPIPE - I haven't tested this, yet).


-- Hannes

--
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: name for A..B ranges?

[Jeff King]

On Wed, Jun 22, 2016 at 08:25:59AM +0100, Philip Oakley wrote:

> Is there a common name for the A..B range format (two dots) that would
> complement the A...B (three dots) symmetric range format's name?
> 
> I was looking at the --left-right distinctions and noticed that the trail
> back to the symmetric range description was rather thin (it's buried within
> gitrevisions:Specifying Ranges, and even then its called a symmetric
> difference.

I would just call it a range, or possibly a set difference. But I don't
think we have any established naming beyond that.

-Peff
--
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: Short-term plans for the post 2.9 cycle

[Jeff King]

On Sun, Jun 19, 2016 at 03:52:04PM -0700, Junio C Hamano wrote:

> Here are the list of topics that are in the "private edition" I use
> for every day work, grouped by where they sit in the the near-term
> plan of merging them up to 'next' and then to 'master'.

By the way, I wondered if you had thoughts on the number of the upcoming
version. We are looking at v2.10 in the current scheme. It was at the
v1.9/v1.10 boundary that we jumped to v2.0 last time.

Certainly it is nice to avoid bumping into double digits (if only to
prevent bugs created by lexical sorting). But it feels rather quick to
be jumping to v3.0. And indeed it is much quicker, as the v1.x series
had an extra level of versioning which meant that the second-biggest
number advanced ten times more slowly.

I know some people's opinion is that versions do not matter, are just
numbers, etc, but I am not sure I agree. If you have dots in your
version number, then bumping the one before the first dot seems like a
bigger change than usual, and I think we should reserve it for a moment
where we have bigger changes in the code.

And I am not at all sure that we have given much thought to what such
changes would be, or that such things would be ready in this cycle. Off
the top of my head, the repository-format bump for pluggable ref
backends, and protocol v2 support seem like possible candidates.  It's
not a flag day for either, of course; we'll build in all of the usual
backwards-compatibility flags. But it's convenient for users to remember
that "3.0" is the minimum to support a new slate of
backwards-incompatible features.

So my inclination is that the next version is simply v2.10. And maybe
you thought of all of this already, and that's why you didn't even
bother mentioning it. :) I'm just curious to hear any thoughts on the
matter.

-Peff
--
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: what is a snapshot?

[Jeff King]

On Sun, Jun 19, 2016 at 04:20:14PM +0100, Philip Oakley wrote:

> From: "Ovatta Bianca" 
> > I read in every comparison of git vs other version control systems,
> > that git does not record differences but takes "snapshots"
> > I was wondering what a "snapshot" is ? Does git store at every commit
> > the entire files which have been modified even if only a few bytes
> > were changed out of a large file?
> > 
> A snaphot is like a tar or zip of all your tracked files. This means it is
> easier to determine (compared to lots of diffs) the current content.
> 
> Keeping all the snapshots as separate loose items, when the majority of
> their content is unchanged would be very inefficient, so git then uses, at
> the right time, an efficient (and obviously lossless) compression technique
> to 'zip' all the snapshots together so that the final repository is
> 'packed'. The overall effect is a very efficient storage scheme.
> 
> There are some good explanations on the web, such as the
> https://git-scm.com/book/en/v2/Git-Internals-Plumbing-and-Porcelain
> though you may want to scan from the beginning ;-)

I think the delta compression is only half the story.

Each commit is a snapshot in that it points to the sha1 of the root
tree, which points to the sha1 of other trees and blobs. And following
that chain gives you the whole state of the tree, without having to care
about other commits.

And if the next commit changes only a few files, the sha1 for all the
other files will remain unchanged, and git does not need to store them
again. So already, before any explicit compression has happened, we get
de-duplication of identical content from commit to commit, at the file
and tree level.

And then when a file does change, we store the whole new version, then
delta compress it during "git gc", etc, as you describe.

-Peff
--
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] Refactor recv_sideband()

[Jeff King]

On Tue, Jun 14, 2016 at 11:00:38PM +0200, Lukas Fleischer wrote:

> Improve the readability of recv_sideband() significantly by replacing
> fragile buffer manipulations with string buffers and more sophisticated
> format strings. Note that each line is printed using a single write()
> syscall to avoid garbled output when multiple processes write to stderr
> in parallel, see 9ac13ec (atomic write for sideband remote messages,
> 2006-10-11) for details.
> 
> Also, reorganize the overall control flow, remove some superfluous
> variables and replace a custom implementation of strpbrk() with a call
> to the standard C library function.

I happened to be looking at the color-printing code yesterday, and was
reminded that on Windows, fprintf is responsible for converting ANSI
codes into colors the terminal can show:

  $ git grep -A2 IMPORTANT color.h
  color.h: * IMPORTANT: Due to the way these color codes are emulated on 
Windows,
  color.h- * write them only using printf(), fprintf(), and fputs(). In 
particular,
  color.h- * do not use puts() or write().

Your patch converts some fprintf calls to write. What does this mean
on Windows for:

  1. Remote servers which send ANSI codes and expect them to look
 reasonable (this might be a losing proposition already, as the
 server won't know anything about the user's terminal, or whether
 output is going to a file).

  2. The use of ANSI_SUFFIX in this function, which uses a similar
 escape code.

-Peff
--
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 3/8] Convert struct diff_filespec to struct object_id

[brian m. carlson]

On Tue, Jun 21, 2016 at 03:22:04PM -0700, Junio C Hamano wrote:
> "brian m. carlson"  writes:
> 
> I was trying to make sure there is no misconversion, but some lines
> that got wrapped were distracting.  For example:
> 
> > @@ -2721,7 +2722,8 @@ static int diff_populate_gitlink(struct diff_filespec 
> > *s, int size_only)
> > if (s->dirty_submodule)
> > dirty = "-dirty";
> >  
> > -   strbuf_addf(, "Subproject commit %s%s\n", sha1_to_hex(s->sha1), 
> > dirty);
> > +   strbuf_addf(, "Subproject commit %s%s\n",
> > +   oid_to_hex(>oid), dirty);
> 
> This would have been
> 
> > -   strbuf_addf(, "Subproject commit %s%s\n", sha1_to_hex(s->sha1), 
> > dirty);
> > +   strbuf_addf(, "Subproject commit %s%s\n", oid_to_hex(>oid), 
> > dirty);
> 
> which the conversion made the line _shorter_.  If the original's
> line length was acceptable, there is no reason to wrap the result.

I do tend to agree.  Coccinelle wrapped the line automatically, but I'm
not sure why it did that.  I can see if there's an option that disables
that if you'd like.  I'm a bit reticent to manually fix up the line
wrapping as I want others to be able to run Coccinelle themselves and
get identical output.

> > @@ -2937,7 +2940,7 @@ static struct diff_tempfile *prepare_temp_file(const 
> > char *name,
> > if (!one->sha1_valid)
> > sha1_to_hex_r(temp->hex, null_sha1);
> > else
> > -   sha1_to_hex_r(temp->hex, one->sha1);
> > +   sha1_to_hex_r(temp->hex, one->oid.hash);
> 
> This suggests that oid_to_hex_r() is needed, perhaps?

Probably so.  I can add that in a reroll.

> > @@ -2952,7 +2955,7 @@ static struct diff_tempfile *prepare_temp_file(const 
> > char *name,
> > if (diff_populate_filespec(one, 0))
> > die("cannot read data blob for %s", one->path);
> > prep_temp_blob(name, temp, one->data, one->size,
> > -  one->sha1, one->mode);
> > +  one->oid.hash, one->mode);
> 
> prep_temp_blob() is a file-scope static with only two callers.  It
> probably would not take too much effort to make it take >oid
> instead?

I can certainly do that in a reroll.

> > @@ -3075,8 +3078,8 @@ static void fill_metainfo(struct strbuf *msg,
> > abbrev = 40;
> > }
> > strbuf_addf(msg, "%s%sindex %s..", line_prefix, set,
> > -   find_unique_abbrev(one->sha1, abbrev));
> > -   strbuf_addstr(msg, find_unique_abbrev(two->sha1, abbrev));
> > +   find_unique_abbrev(one->oid.hash, abbrev));
> > +   strbuf_addstr(msg, find_unique_abbrev(two->oid.hash, abbrev));
> 
> Good.  As more and more callers of find_unique_abbrev() is converted
> to pass oid.hash to it, eventually we will have only a handful of
> callers that have a raw "const unsigned char sha1[20]" to pass it,
> and we can eventually make the function take   It seems we are
> not quite there yet, by the looks of 'git grep find_unique_abbrev'
> output, but we are getting closer.

Yes, I'd noticed that as well.  One thing I observed from these
conversions is that it sometimes takes a huge amount of work to get all
the callers to change.  Often, it's only one or two call sites that end
up being a bunch of work.

> > @@ -3134,17 +3137,17 @@ static void diff_fill_sha1_info(struct 
> > diff_filespec *one)
> > if (!one->sha1_valid) {
> > struct stat st;
> > if (one->is_stdin) {
> > -   hashcpy(one->sha1, null_sha1);
> > +   hashcpy(one->oid.hash, null_sha1);
> > return;
> > }
> 
> oidclr()?
> 
> Perhaps a preparatory step of
> 
>   unsigned char *E1;
> 
>   -hashcpy(E1, null_sha1);
> +hashclr(E1)
> 
> would help?

Sure.  I can do that.

> > @@ -902,13 +904,13 @@ static struct merge_file_info merge_file_1(struct 
> > merge_options *o,
> > result.clean = 0;
> > if (S_ISREG(a->mode)) {
> > result.mode = a->mode;
> > -   hashcpy(result.sha, a->sha1);
> > +   hashcpy(result.sha, a->oid.hash);
> > } else {
> > result.mode = b->mode;
> > -   hashcpy(result.sha, b->sha1);
> > +   hashcpy(result.sha, b->oid.hash);
> 
> merge_file_info is a file-scope-static type, and it shouldn't take
> too much effort to replace its sha1 with an oid, I would think.

That's one of the following patches.

> sha_eq() knows that either of its two parameters could be NULL, but
> a->sha1 is diff_filespec.sha1 which cannot be NULL.
> 
> So !sha_eq() here can become oidcmp().  There are some calls to
> sha_eq() that could pass NULL (e.g. a_sha could be NULL in
> blob_unchanged()), but 

Re: git-svn aborts with "Use of uninitialized value $u" when a non-svn-backed branch is present in remote

[Jacob Godserv]

On Tue, Sep 22, 2015 at 2:48 PM, Jacob Godserv  wrote:
> I found a specific case in which git-svn improperly aborts:
>
> 1. I created a git-svn repository, named "git-svn repo", by cloned an
> svn repository via the git-svn tool.
> 2. I created a normal git repository, named "configuration repo". I
> renamed the master branch to "configuration". The initial commit adds
> a README and some utility scripts.
> 3. I created a bare repository, named "master repo".
> 4. I pushed from the git-svn repo to the master repo.
> 5. I pushed from the configuration repo to the master repo.
>
> The idea is the configuration branch, which is detached from any
> git-svn history, can contain some useful tools, defaults, etc., that I
> can share with teammates who want to use git on this svn project. It's
> an odd use of git, but it has been working well.
>
> However, a vanilla distribution of Git for Windows 2.5.2 produces the
> following error when running any git-svn command, such as "git svn
> info", on the cloned master repo:
>
> Use of uninitialized value $u in substitution (s///) at
> /mingw64/share/perl5/site_perl/Git/SVN.pm line 105.
> Use of uninitialized value $u in concatenation (.) or string at
> /mingw64/share/perl5/site_perl/Git/SVN.pm line 105.
> refs/remotes/origin/configuration: 'svn+ssh://10.0.1.1/repos/projectA'
> not found in ''
>
> In the mentioned SVN.pm file, after the line:
>
> my $u = (::cmt_metadata("$refname"))[0];
>
> I added the following four lines:
>
> if (not defined $u) {
> warn "W: $refname does not exist in
> SVN; skipping";
> next;
> }
>
> git-svn appears to operate correctly with this patch. This is my first
> time ever editing a perl script, so I apologize if I murdered an
> adorable animal just now.
>
> I'm sending this in so more knowledgeable git-svn developers can
> comment on this and fix this in the official distribution of git,
> assuming there is a bug here to fix.
>
> --
> Jacob

This e-mail has gone ignored several months. Is the maintainer of
git-svn on this mailing list? Should I submit this issue elsewhere?

-- 
Jacob
--
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 v3 1/3] t4202: refactor test

[Mehul Jain]

Subsequent patches will want to reuse the 'signed' branch that the
'log --graph --show-signature' test creates and uses.

Split the set-up part into a test of its own, and make the existing
test into a separate one that only inspects the history on the 'signed'
branch. This way, it becomes clearer that tests added by subsequent
patches reuse the 'signed' branch in the same way.

Signed-off-by: Mehul Jain 
---
 t/t4202-log.sh | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 128ba93..ab66ee0 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -860,12 +860,15 @@ test_expect_success 'dotdot is a parent directory' '
test_cmp expect actual
 '
 
-test_expect_success GPG 'log --graph --show-signature' '
+test_expect_success GPG 'setup signed branch' '
test_when_finished "git reset --hard && git checkout master" &&
git checkout -b signed master &&
echo foo >foo &&
git add foo &&
-   git commit -S -m signed_commit &&
+   git commit -S -m signed_commit
+'
+
+test_expect_success GPG 'log --graph --show-signature' '
git log --graph --show-signature -n1 signed >actual &&
grep "^| gpg: Signature made" actual &&
grep "^| gpg: Good signature" actual
-- 
2.9.0.rc0.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

Re: Git Bash Slow in Windows (possible fix)

[Johannes Schindelin]

Hi,

On Thu, 23 Jun 2016, UberBooster wrote:

> [...] and I also installed MS Office 360.  AFTER installing this
> software, Git-Bash was AMAZINGLY fast.

I suspect you mean Office 365.

In any case, it is next to impossible to diagnose slowness after it
dissipated.

For the record, we do have a wiki page that describes how to find out more
about why Git Bash's startup might be slow (it should not be slow, of
course, yet we had enough cases where we needed to find out more):

https://github.com/git-for-windows/git/wiki/Diagnosing-performance-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 v3 4/4] archive-tar: drop return value

[Jeff King]

On Fri, Jun 24, 2016 at 01:49:24PM +0200, Remi Galan Alfonso wrote:

> Hi Peff,
> 
> Jeff King  writes:
> > @@ -413,7 +411,7 @@ static int write_tar_archive(const struct archiver *ar,
> >  {
> >  int err = 0;
> >  
> > -err = write_global_extended_header(args);
> > +write_global_extended_header(args);
> >  if (!err)
> >  err = write_archive_entries(args, write_tar_entry);
> 
> If we drop the error code from 'write_global_extended_header' then the
> above 'if (!err)' becomes useless (always evaluates to 'true' since
> 'err' is set to '0').

Thanks, you're right.

I wondered if we could drop "err" entirely, but write_archive_entries()
does indeed have some error code paths (everybody uses write_or_die, but
we return an error for things like unknown file types).

-Peff
--
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:The global market so much, You'll only other foreign customers to your door, do not open up new ways to do?

[omc10]

您好，我们是能够帮您：
1、一天搜索上万家行业内匹配客人邮箱，帮您开发到B2B、展会上遇不到的优质客户。
2、高效邮件营销，帮您获得更多一对一高质量优质客户询盘。
3、让您的潜在客户主动找您洽谈订单！
请加+QQ 3246075707 联系 在线演示主动开发全球客户，欢迎验证是否真实有效
也可加微信号：sunsesoftsam

您是选择主动出击，从源头开发优质客户，还是苦苦等待客户来联系你呢 
如有不需要此信息，请回复“不需要”，我们将会将您的邮箱进行屏蔽，不再给您发信的。


祝：生意兴隆！N�Р骒r��yb�X�肚�v�^�)藓{.n�+丕��≤�}��财�z�:+v�����赙zZ+��+zf＂�h���~i���z��wア�?�ㄨ��&�)撷f

Re: [PATCH v3 0/3] Introduce log.showSignature config variable

[Eric Sunshine]

On Fri, Jun 24, 2016 at 5:21 AM, Mehul Jain  wrote:
> On Thu, Jun 23, 2016 at 12:02 PM, Junio C Hamano  wrote:
>> Mehul Jain  writes:
>>> In patch 2/3 and 3/3, there are many tests which requires a branch
>>> similar to that of "signed" branch, i.e. a branch with a commit having
>>> GPG signature. So previously in v2, I created two new branches,
>>> "test_sign" and "no_sign", which are identical to that of "signed"
>>> branch. And with these branches, I wrote the tests in patch 2/3
>>> and 3/3.
>>>
>>> As suggested by Eric [1], rather than creating new branches, I
>>> can take advantage of "signed" branch which already exists.
>>
>> Yeah, I understand that part.  But you do not _need_ to do the split
>> you do in 1/3 in order to reuse "signed".
>
> If it's fine, then I think it would be OK to drop this 1/3. Without splitting
> the 'log --graph --show-signature' in two test will also serve the
> purpose for the new test to use the signed branch.

My understanding of Junio's response is that the missing PGP
prerequisite along with a weak commit message make for poor
justification of patch 1/3, however, if you add the prerequisite and
use the commit message he proposed (reproduced below) then it becomes
sensible to retain 1/3.

--->8---
In 2/3 and 3/3, we will use the same 'signed' branch that the
first test for 'log --graph --show-signature' uses.  This branch
is currently created in that 'log --graph --show-signature' test
itself.

Split the set-up part into a test of its own, and make the
existing first test into a separate one that only inspects the
history on the 'signed' branch.  That way, it would become
clearer that later tests added by 2/3 and 3/3 reuse the 'signed'
branch in the same way this 'log --graph --show-signature' uses
that same branch.
--->8---
--
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] unpack-trees: fix English grammar in do-this-before-that messages

[Vasco Almeida]

Às 05:31 de 24-06-2016, Alex Henrie escreveu:
> Signed-off-by: Alex Henrie 
> ---
>  unpack-trees.c | 18 +-
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 6bc9512..11c37fb 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -62,17 +62,17 @@ void setup_unpack_trees_porcelain(struct 
> unpack_trees_options *opts,
>   if (!strcmp(cmd, "checkout"))
>   msg = advice_commit_before_merge
> ? _("Your local changes to the following files would be 
> overwritten by checkout:\n%%s"
> -   "Please commit your changes or stash them before you 
> can switch branches.")
> +   "Please commit your changes or stash them before you 
> switch branches.")
> : _("Your local changes to the following files would be 
> overwritten by checkout:\n%%s");

The only downside I can tell about this is translator are going to have
to update those strings on their translations, but that is a normal
thing to do on an active project like Git.

I'm not a native speak either, but I think I have translated that as if
the sentences were like this patch introduces. I agree with this change.
--
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 4/4] archive-tar: drop return value

[Remi Galan Alfonso]

Hi Peff,

Jeff King  writes:
> @@ -413,7 +411,7 @@ static int write_tar_archive(const struct archiver *ar,
>  {
>  int err = 0;
>  
> -err = write_global_extended_header(args);
> +write_global_extended_header(args);
>  if (!err)
>  err = write_archive_entries(args, write_tar_entry);

If we drop the error code from 'write_global_extended_header' then the
above 'if (!err)' becomes useless (always evaluates to 'true' since
'err' is set to '0').

>  if (!err)

Thanks,
Rémi
--
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: may be bug in svn fetch no-follow-parent

[Александр Овчинников]

https://s3-eu-west-1.amazonaws.com/profff/mergeinfo.zip

even ordinary merge may take up to 20-30 minutes. I'll try to trace in future

23.06.2016, 01:58, "Eric Wong" :
> Александр Овчинников  wrote:
>>  Unfortunately this is not open source repository. I agree that it is 
>> pointless try to handle mergeinfo (because it always fails).
>>  Cases when it is expensive:
>>  1. delete and restore mergeinfo property
>>  2. merge trunk to very old branch
>>  3. create, delete, create branch with --no-follow-parent. All records in 
>> mergeinfo will be hadled like new.
>>
>>  I already patched like this and this is helpfull, works fine and fast.
>
> Thanks for the info. Patch + pull request below for Junio.
>
>>  I can share only mergeinfo property
>
> Oops, looks like your zip attachment got flagged as spam for
> my mailbox and swallowed by vger.kernel.org :x
>
> -8<
> Subject: [PATCH] git-svn: skip mergeinfo handling with --no-follow-parent
>
> For repositories without parent following enabled, finding
> git parents through svn:mergeinfo or svk::parents can be
> expensive and pointless.
>
> Reported-by: Александр Овчинников 
> http://mid.gmane.org/4094761466408...@web24o.yandex.ru
>
> Signed-off-by: Eric Wong 
> ---
>   The following changes since commit ab7797dbe95fff38d9265869ea367020046db118:
>
> Start the post-2.9 cycle (2016-06-20 11:06:49 -0700)
>
>   are available in the git repository at:
>
> git://bogomips.org/git-svn.git svn-nfp-mergeinfo
>
>   for you to fetch changes up to 6d523a3ab76cfa4ed9ae0ed9da7af43efcff3f07:
>
> git-svn: skip mergeinfo handling with --no-follow-parent (2016-06-22 
> 22:48:54 +)
>
>   
>   Eric Wong (1):
> git-svn: skip mergeinfo handling with --no-follow-parent
>
>  perl/Git/SVN.pm | 25 -
>  1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
> index d94d01c..bee1e7d 100644
> --- a/perl/Git/SVN.pm
> +++ b/perl/Git/SVN.pm
> @@ -1905,15 +1905,22 @@ sub make_log_entry {
>
>  my @parents = @$parents;
>  my $props = $ed->{dir_prop}{$self->path};
> - if ( $props->{"svk:merge"} ) {
> - $self->find_extra_svk_parents($props->{"svk:merge"}, \@parents);
> - }
> - if ( $props->{"svn:mergeinfo"} ) {
> - my $mi_changes = $self->mergeinfo_changes
> - ($parent_path, $parent_rev,
> - $self->path, $rev,
> - $props->{"svn:mergeinfo"});
> - $self->find_extra_svn_parents($mi_changes, \@parents);
> + if ($self->follow_parent) {
> + my $tickets = $props->{"svk:merge"};
> + if ($tickets) {
> + $self->find_extra_svk_parents($tickets, \@parents);
> + }
> +
> + my $mergeinfo_prop = $props->{"svn:mergeinfo"};
> + if ($mergeinfo_prop) {
> + my $mi_changes = $self->mergeinfo_changes(
> + $parent_path,
> + $parent_rev,
> + $self->path,
> + $rev,
> + $mergeinfo_prop);
> + $self->find_extra_svn_parents($mi_changes, \@parents);
> + }
>  }
>
>  open my $un, '>>', "$self->{dir}/unhandled.log" or croak $!;
> --
> EW
--
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] unpack-trees: fix English grammar in do-this-before-that messages

[Matthieu Moy]

Alex Henrie  writes:

> Signed-off-by: Alex Henrie 
> ---
>  unpack-trees.c | 18 +-
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 6bc9512..11c37fb 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -62,17 +62,17 @@ void setup_unpack_trees_porcelain(struct 
> unpack_trees_options *opts,
>   if (!strcmp(cmd, "checkout"))
>   msg = advice_commit_before_merge
> ? _("Your local changes to the following files would be 
> overwritten by checkout:\n%%s"
> -   "Please commit your changes or stash them before you 
> can switch branches.")
> +   "Please commit your changes or stash them before you 
> switch branches.")

I'm not a native speaker, but I think both versions are correct.

I'm OK with the change, though. Dropping something useless is usually a
good thing :-).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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 0/3] Introduce log.showSignature config variable

[Mehul Jain]

On Thu, Jun 23, 2016 at 12:02 PM, Junio C Hamano  wrote:
> Mehul Jain  writes:
>
>> In patch 2/3 and 3/3, there are many tests which requires a branch
>> similar to that of "signed" branch, i.e. a branch with a commit having
>> GPG signature. So previously in v2, I created two new branches,
>> "test_sign" and "no_sign", which are identical to that of "signed"
>> branch. And with these branches, I wrote the tests in patch 2/3
>> and 3/3.
>>
>> As suggested by Eric [1], rather than creating new branches, I
>> can take advantage of "signed" branch which already exists.
>
> Yeah, I understand that part.  But you do not _need_ to do the split
> you do in 1/3 in order to reuse "signed".

If it's fine, then I think it would be OK to drop this 1/3. Without splitting
the 'log --graph --show-signature' in two test will also serve the
purpose for the new test to use the signed branch.

Should I send a new patch series with 1/3 dropped or you can do
it manually at your end?

Thanks,
Mehul
--
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