[PATCH v3] send-email: recognize absolute path on Windows

[Erik Faye-Lund]

From: Erik Faye-Lund kusmab...@googlemail.com

On Windows, absolute paths might start with a DOS drive prefix,
which these two checks failed to recognize.

Unfortunately, we cannot simply use the file_name_is_absolute
helper in File::Spec::Functions, because Git for Windows has an
MSYS-based Perl, where this helper doesn't grok DOS
drive-prefixes.

So let's manually check for these in that case, and fall back to
the File::Spec-helper on other platforms (e.g Win32 with native
Perl)

Signed-off-by: Erik Faye-Lund kusmab...@gmail.com
---

So here's a version that does the old and long-time tested
approach without requiring breaking changes to msysGit's perl.

 git-send-email.perl | 16 ++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index fdb0029..8f5f986 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1113,6 +1113,18 @@ sub ssl_verify_params {
}
 }
 
+sub file_name_is_absolute {
+   my ($path) = @_;
+
+   # msys does not grok DOS drive-prefixes
+   if ($^O eq 'msys') {
+   return ($path =~ m#^/# || $path =~ m#[a-zA-Z]\:#)
+   }
+
+   require File::Spec::Functions;
+   return File::Spec::Functions::file_name_is_absolute($path);
+}
+
 # Returns 1 if the message was sent, and 0 otherwise.
 # In actuality, the whole program dies when there
 # is an error sending a message.
@@ -1197,7 +1209,7 @@ X-Mailer: git-send-email $gitversion
 
if ($dry_run) {
# We don't want to send the email.
-   } elsif ($smtp_server =~ m#^/#) {
+   } elsif (file_name_is_absolute($smtp_server)) {
my $pid = open my $sm, '|-';
defined $pid or die $!;
if (!$pid) {
@@ -1271,7 +1283,7 @@ X-Mailer: git-send-email $gitversion
printf (($dry_run ? Dry- : ).Sent %s\n, $subject);
} else {
print (($dry_run ? Dry- : ).OK. Log says:\n);
-   if ($smtp_server !~ m#^/#) {
+   if (!file_name_is_absolute($smtp_server)) {
print Server: $smtp_server\n;
print MAIL FROM:$raw_from\n;
foreach my $entry (@recipients) {
-- 
1.9.0.msysgit.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] git-rebase: Print name of rev when using shorthand

[Brian Gesiak]

Thank you for the feedback!

 Imagine the case where there are more than one branches
 whose tip points at the commit you came from.
 name-rev will not be able to pick correctly which one to report.

I see. Yes, you're exactly right; the following demonstrates
the problem:

$ git checkout -b xylophone master
$ git checkout -b aardvark master
$ git name-rev --name-only @{-1} # I'd want xylophone, but this
outputs aardvark

So it appears name-rev is not up to the task here.

 I think you would want to use something like:

 upstream_name=$(git rev-parse --symbolic-full-name @{-1})
 if test -n $upstream
 then
 upstream_name=${upstream_name#refs/heads/}
 else
 upstream_name=@{-1}
 fi

 if the change is to be made at that point in the code.

I agree, I will re-roll the patch to use this approach.

 I also wonder if git rebase @{-1} deserve a similar translation
 like you are giving git rebase -.

Personally, I've been using the - shorthand with git checkout
for a year or so, but only learned about @{-1} a few months ago.
I think those who use @{-1} are familiar enough with the concept
that they don't need to have the reference translated to a symbolic
full name. Users familiar with - might not be aware of @{-1},
however, so I'd prefer not to output it as we are currently.

Furthermore, were we to translate @{-1}, does that mean we
should also translate @{-2} or prior? I don't think that's the case,
but then only translating @{-1} would seem inconsistent.
From that point of view I'd prefer to simply translate -,
not @{-1}.

- Brian Gesiak
--
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] git-rebase: Print name of rev when using shorthand

[Brian Gesiak]

The output from a successful invocation of the shorthand command
git rebase - is something like Fast-forwarded HEAD to @{-1},
which includes a relative reference to a revision. Other commands
that use the shorthand -, such as git checkout -, typically
display the symbolic name of the revision.

Change rebase to output the symbolic name of the revision when using
the shorthand. For the example above, the new output is
Fast-forwarded HEAD to master, assuming @{-1} is a reference to
master.

- Use git rev-parse to retreive the name of the rev.
- Update the tests in light of this new behavior.

Requested-by: John Keeping j...@keeping.me.uk
Signed-off-by: Brian Gesiak modoca...@gmail.com
---
 git-rebase.sh | 8 +++-
 t/t3400-rebase.sh | 4 +---
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 2c75e9f..42d34a6 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -455,7 +455,13 @@ then
*)  upstream_name=$1
if test $upstream_name = -
then
-   upstream_name=@{-1}
+   upstream_name=`git rev-parse --symbolic-full-name @{-1}`
+   if test -n $upstream_name
+   then
+   upstream_name=${upstream_name#refs/heads/}
+   else
+   upstream_name=@{-1}
+   fi
fi
shift
;;
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 80e0a95..2b99940 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -91,7 +91,7 @@ test_expect_success 'rebase from ambiguous branch name' '
 test_expect_success 'rebase off of the previous branch using -' '
git checkout master 
git checkout HEAD^ 
-   git rebase @{-1} expect.messages 
+   git rebase master expect.messages 
git merge-base master HEAD expect.forkpoint 
 
git checkout master 
@@ -100,8 +100,6 @@ test_expect_success 'rebase off of the previous branch 
using -' '
git merge-base master HEAD actual.forkpoint 
 
test_cmp expect.forkpoint actual.forkpoint 
-   # the next one is dubious---we may want to say -,
-   # instead of @{-1}, in the message
test_i18ncmp expect.messages actual.messages
 '
 
-- 
1.9.0.259.gc5d75e8.dirty

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/6] contrib/credential/osxkeychain/git-credential-osxkeychain.c: reduce scope of variables

[Elia Pinto]

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
 contrib/credential/osxkeychain/git-credential-osxkeychain.c |8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c 
b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
index bcd3f57..5ae09f6 100644
--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
@@ -163,12 +163,12 @@ static void read_credential(void)
 
 int main(int argc, const char **argv)
 {
-   const char *usage =
-   usage: git credential-osxkeychain get|store|erase;
-
if (!argv[1])
+  {
+  const char *usage =
+   usage: git credential-osxkeychain get|store|erase;
die(usage);
-
+  }
read_credential();
 
if (!strcmp(argv[1], get))
-- 
1.7.10.4

--
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/6] compat/regex/regex_internal.c: reduce scope of variables

[Elia Pinto]

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
 compat/regex/regex_internal.c |6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/compat/regex/regex_internal.c b/compat/regex/regex_internal.c
index d4121f2..a7a71ec 100644
--- a/compat/regex/regex_internal.c
+++ b/compat/regex/regex_internal.c
@@ -707,7 +707,6 @@ re_string_reconstruct (re_string_t *pstr, int idx, int 
eflags)
 #ifdef RE_ENABLE_I18N
  if (pstr-mb_cur_max  1)
{
- int wcs_idx;
  wint_t wc = WEOF;
 
  if (pstr-is_utf8)
@@ -738,11 +737,11 @@ re_string_reconstruct (re_string_t *pstr, int idx, int 
eflags)
  mbstate_t cur_state;
  wchar_t wc2;
  int mlen = raw + pstr-len - p;
- unsigned char buf[6];
  size_t mbclen;
 
  if (BE (pstr-trans != NULL, 0))
{
+  unsigned char buf[6];
  int i = mlen  6 ? mlen : 6;
  while (--i = 0)
buf[i] = pstr-trans[p[i]];
@@ -778,6 +777,7 @@ re_string_reconstruct (re_string_t *pstr, int idx, int 
eflags)
? CONTEXT_NEWLINE : 0));
  if (BE (pstr-valid_len, 0))
{
+  int wcs_idx;
  for (wcs_idx = 0; wcs_idx  pstr-valid_len; ++wcs_idx)
pstr-wcs[wcs_idx] = WEOF;
  if (pstr-mbs_allocated)
@@ -925,7 +925,6 @@ static unsigned int
 internal_function
 re_string_context_at (const re_string_t *input, int idx, int eflags)
 {
-  int c;
   if (BE (idx  0, 0))
 /* In this case, we use the value stored in input-tip_context,
since we can't know the character in input-mbs[-1] here.  */
@@ -957,6 +956,7 @@ re_string_context_at (const re_string_t *input, int idx, 
int eflags)
   else
 #endif
 {
+  int c;
   c = re_string_byte_at (input, idx);
   if (bitset_contain (input-word_char, c))
return CONTEXT_WORD;
-- 
1.7.10.4

--
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/6] compat/regex/regcomp.c: reduce scope of variables

[Elia Pinto]

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
 compat/regex/regcomp.c |   17 -
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/compat/regex/regcomp.c b/compat/regex/regcomp.c
index 06f3088..c7da378 100644
--- a/compat/regex/regcomp.c
+++ b/compat/regex/regcomp.c
@@ -368,7 +368,6 @@ re_compile_fastmap_iter (regex_t *bufp, const re_dfastate_t 
*init_state,
   else if (type == COMPLEX_BRACKET)
{
  re_charset_t *cset = dfa-nodes[node].opr.mbcset;
- int i;
 
 # ifdef _LIBC
  /* See if we have to try all bytes which start multiple collation
@@ -380,6 +379,7 @@ re_compile_fastmap_iter (regex_t *bufp, const re_dfastate_t 
*init_state,
  if (_NL_CURRENT_WORD (LC_COLLATE, _NL_COLLATE_NRULES) != 0
   (cset-ncoll_syms || cset-nranges))
{
+ int i;
  const int32_t *table = (const int32_t *)
_NL_CURRENT (LC_COLLATE, _NL_COLLATE_TABLEMB);
  for (i = 0; i  SBC_MAX; ++i)
@@ -598,7 +598,7 @@ static bitset_t utf8_sb_map;
 static void
 free_dfa_content (re_dfa_t *dfa)
 {
-  int i, j;
+  int i;
 
   if (dfa-nodes)
 for (i = 0; i  dfa-nodes_len; ++i)
@@ -621,6 +621,7 @@ free_dfa_content (re_dfa_t *dfa)
   if (dfa-state_table)
 for (i = 0; i = dfa-state_hash_mask; ++i)
   {
+   int j;
struct re_state_table_entry *entry = dfa-state_table + i;
for (j = 0; j  entry-num; ++j)
  {
@@ -994,7 +995,7 @@ free_workarea_compile (regex_t *preg)
 static reg_errcode_t
 create_initial_state (re_dfa_t *dfa)
 {
-  int first, i;
+  int first;
   reg_errcode_t err;
   re_node_set init_nodes;
 
@@ -1011,6 +1012,8 @@ create_initial_state (re_dfa_t *dfa)
  Then we add epsilon closures of the nodes which are the next nodes of
  the back-references.  */
   if (dfa-nbackref  0)
+{
+int i;
 for (i = 0; i  init_nodes.nelem; ++i)
   {
int node_idx = init_nodes.elems[i];
@@ -1044,6 +1047,7 @@ create_initial_state (re_dfa_t *dfa)
  }
  }
   }
+}
 
   /* It must be the first time to invoke acquire_state.  */
   dfa-init_state = re_acquire_state_context (err, dfa, init_nodes, 0);
@@ -1682,7 +1686,6 @@ static reg_errcode_t
 calc_eclosure_iter (re_node_set *new_set, re_dfa_t *dfa, int node, int root)
 {
   reg_errcode_t err;
-  int i;
   re_node_set eclosure;
   int ret;
   int incomplete = 0;
@@ -1708,6 +1711,8 @@ calc_eclosure_iter (re_node_set *new_set, re_dfa_t *dfa, 
int node, int root)
 
   /* Expand each epsilon destination nodes.  */
   if (IS_EPSILON_NODE(dfa-nodes[node].type))
+{
+int i;
 for (i = 0; i  dfa-edests[node].nelem; ++i)
   {
re_node_set eclosure_elem;
@@ -1741,6 +1746,7 @@ calc_eclosure_iter (re_node_set *new_set, re_dfa_t *dfa, 
int node, int root)
re_node_set_free (eclosure_elem);
  }
   }
+}
 
   /* An epsilon closure includes itself.  */
   ret = re_node_set_insert (eclosure, node);
@@ -2630,7 +2636,6 @@ build_range_exp (bitset_t sbcset, bracket_elem_t 
*start_elem,
 bracket_elem_t *end_elem)
 # endif /* not RE_ENABLE_I18N */
 {
-  unsigned int start_ch, end_ch;
   /* Equivalence Classes and Character Classes can't be a range start/end.  */
   if (BE (start_elem-type == EQUIV_CLASS || start_elem-type == CHAR_CLASS
  || end_elem-type == EQUIV_CLASS || end_elem-type == CHAR_CLASS,
@@ -2647,6 +2652,7 @@ build_range_exp (bitset_t sbcset, bracket_elem_t 
*start_elem,
 
 # ifdef RE_ENABLE_I18N
   {
+unsigned int start_ch, end_ch;
 wchar_t wc;
 wint_t start_wc;
 wint_t end_wc;
@@ -2728,6 +2734,7 @@ build_range_exp (bitset_t sbcset, bracket_elem_t 
*start_elem,
 # else /* not RE_ENABLE_I18N */
   {
 unsigned int ch;
+unsigned int start_ch, end_ch;
 start_ch = ((start_elem-type == SB_CHAR ) ? start_elem-opr.ch
: ((start_elem-type == COLL_SYM) ? start_elem-opr.name[0]
   : 0));
-- 
1.7.10.4

--
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 5/6] contrib/credential/wincred/git-credential-wincred.c: reduce scope of variables

[Elia Pinto]

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
 contrib/credential/wincred/git-credential-wincred.c |6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/contrib/credential/wincred/git-credential-wincred.c 
b/contrib/credential/wincred/git-credential-wincred.c
index a1d38f0..edff71c 100644
--- a/contrib/credential/wincred/git-credential-wincred.c
+++ b/contrib/credential/wincred/git-credential-wincred.c
@@ -258,11 +258,13 @@ static void read_credential(void)
 
 int main(int argc, char *argv[])
 {
-   const char *usage =
-   usage: git credential-wincred get|store|erase\n;
 
if (!argv[1])
+   {
+   const char *usage =
+ usage: git credential-wincred get|store|erase\n;
die(usage);
+   }
 
/* git use binary pipes to avoid CRLF-issues */
_setmode(_fileno(stdin), _O_BINARY);
-- 
1.7.10.4

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

Hello

[norababy2014_2014]

Helló
a nevem Nora kedvem, hogy Önnel a kapcsolatot, miután megy keresztül a profil 
ezen az oldalon, miközben keresi a kapcsolatot. Én nagyon örülök, hogy a 
barátod, kérjük, ha nem bánja, azt fogom szeretni, hogy írjon nekem vissza az e-
mail címemet. (
johnson.nora31 @ yahoo.com)
  úgy, hogy én is adok neked a képeket, és mondd meg többet az én oldalamra. 
Alig várom, hogy olvassa el tőled.


Hello
my name is Nora i am well pleased to contact you after going through your 
profile on this site while searching for a relationship. i will be very glad to 
be your friend, please if you wouldn't mind, i will like you to write me back 
through my email. (
johnson.nor...@yahoo.com)
 so that i can give you my pictures and tell you more about my self. I am 
looking forward to read from you.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/6] contrib/credential/wincred/git-credential-wincred.c: reduce scope of variables

[Erik Faye-Lund]

On Wed, Apr 16, 2014 at 11:33 AM, Elia Pinto gitter.spi...@gmail.com wrote:
 Signed-off-by: Elia Pinto gitter.spi...@gmail.com
 ---
  contrib/credential/wincred/git-credential-wincred.c |6 --
  1 file changed, 4 insertions(+), 2 deletions(-)

 diff --git a/contrib/credential/wincred/git-credential-wincred.c 
 b/contrib/credential/wincred/git-credential-wincred.c
 index a1d38f0..edff71c 100644
 --- a/contrib/credential/wincred/git-credential-wincred.c
 +++ b/contrib/credential/wincred/git-credential-wincred.c
 @@ -258,11 +258,13 @@ static void read_credential(void)

  int main(int argc, char *argv[])
  {
 -   const char *usage =
 -   usage: git credential-wincred get|store|erase\n;

 if (!argv[1])
 +   {
 +   const char *usage =
 + usage: git credential-wincred get|store|erase\n;
 die(usage);
 +   }


This is not the indentation/bracket-style we use in this source-file.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] xdiff/xprepare.c: reduce scope of variables

[Erik Faye-Lund]

On Wed, Apr 16, 2014 at 11:33 AM, Elia Pinto gitter.spi...@gmail.com wrote:
 Signed-off-by: Elia Pinto gitter.spi...@gmail.com
 ---
  xdiff/xprepare.c |5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

 diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c
 index 63a22c6..e0b6987 100644
 --- a/xdiff/xprepare.c
 +++ b/xdiff/xprepare.c
 @@ -161,8 +161,7 @@ static int xdl_prepare_ctx(unsigned int pass, mmfile_t 
 *mf, long narec, xpparam_
xdlclassifier_t *cf, xdfile_t *xdf) {
 unsigned int hbits;
 long nrec, hsize, bsize;
 -   unsigned long hav;
 -   char const *blk, *cur, *top, *prev;
 +   char const *blk, *cur, *prev;
 xrecord_t *crec;
 xrecord_t **recs, **rrecs;
 xrecord_t **rhash;
 @@ -193,7 +192,9 @@ static int xdl_prepare_ctx(unsigned int pass, mmfile_t 
 *mf, long narec, xpparam_

 nrec = 0;
 if ((cur = blk = xdl_mmfile_first(mf, bsize)) != NULL) {
 +char const *top;
 for (top = blk + bsize; cur  top; ) {
 +unsigned long hav;
 prev = cur;

We do not indent with spaces.
--
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] Unicode: update of combining code points

[Kevin Bracey]

On 16/04/2014 07:48, Torsten Bögershausen wrote:

On 15.04.14 21:10, Peter Krefting wrote:

Torsten Bögershausen:


diff --git a/utf8.c b/utf8.c
index a831d50..77c28d4 100644
--- a/utf8.c
+++ b/utf8.c

Is there a script that generates this code from the Unicode database files, or 
did you hand-update it?


Some of the code points which have 0 length on the display are called
combining, others are called vowels or accents.
E.g. 5BF is not marked any of them, but if you look at the glyph, it should
be combining (please correct me if that is wrong).


Indeed it is combining (more specifically it has General Category 
Nonspacing_Mark = Mn).




If I could have found a file which indicates for each code point, what it
is, I could write a script.



The most complete and machine-readable data are in these files:

http://www.unicode.org/Public/UCD/latest/ucd/UnicodeData.txt
http://www.unicode.org/Public/UCD/latest/ucd/EastAsianWidth.txt

The general categories can also be seen more legibly in:

http://www.unicode.org/Public/UCD/latest/ucd/extracted/DerivedGeneralCategory.txt

For docs, see:

http://www.unicode.org/reports/tr44/
http://www.unicode.org/reports/tr11/
http://www.unicode.org/ucd/

The existing utf8.c comments describe the attributes being selected from 
the tables (general categories Cf,Mn,Me, East Asian Width W, 
F). And they suggest that the combining character table was originally 
auto-generated from UnicodeData.txt with a uniset tool. Presumably this?


https://github.com/depp/uniset

The fullwidth-checking code looks like it was done by hand, although 
apparently uniset can process EastAsianWidth.txt.


Kevin

--
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's cooking in git.git (Apr 2014, #03; Fri, 11)

[Ramsay Jones]

On 16/04/14 00:18, Duy Nguyen wrote:
 On Tue, Apr 15, 2014 at 11:45 PM, Junio C Hamano gits...@pobox.com wrote:
 Ramsay Jones ram...@ramsay1.demon.co.uk writes:

 On 11/04/14 23:22, Junio C Hamano wrote:
 [...]
 [New Topics]

 * nd/index-pack-one-fd-per-thread (2014-04-09) 1 commit
  - index-pack: work around thread-unsafe pread()

  Enable threaded index-pack on platforms without thread-unsafe
  pread() emulation.

  Will merge to 'next' and keep it there for the remainder of the cycle.

 The commit message for commit 512ebe5d (index-pack: work around
 thread-unsafe pread(), 25-03-2014) is a little misleading.

 OK.  Can we have a concrete alternative?

 Multi-threaing of index-pack was disabled with c0f8654
 (index-pack: Disable threading on cygwin - 2012-06-26), because
 pread() implementations for Cygwin and MSYS were not thread
 safe.  Recent Cygwin does offer usable pread() and we enabled
 multi-threading with 103d530f (Cygwin 1.7 has thread-safe pread,
 2013-07-19).

 Work around this problem on platforms with a thread-unsafe
 pread() emulation by opening one file handle per thread; it
 would prevent parallel pread() on different file handles from
 stepping on each other.

 Also remove NO_THREAD_SAFE_PREAD that was introduced in c0f8654
 because it's no longer used anywhere.

 This workaround is unconditional, even for platforms with
 thread-safe pread() because the overhead is small (a couple file
 handles more) and not worth fragmenting the code.

 
 OK to me.


Yep, this looks good to me too.

Thanks!

ATB,
Ramsay Jones



--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: wrong handling of text git attribute leading to files incorrectly reported as modified

[Frank Ammeter]

Am 15.04.2014 um 23:23 schrieb Junio C Hamano gits...@pobox.com:

 Brandon McCaig bamcc...@gmail.com writes:
 
 That is for your benefit, and for easily sharing that configuration
 with collaborators. Git only cares that the file exists in your
 working tree at run-time.
 
 It is a lot more than for sharing.  If you made .gitignore only
 effective after it gets committed, you cannot test your updated
 version of .gitignore is correct before committing the change.

Ok, I can follow that logic for .gitignore, but I was talking about 
.gitattributes and I always thought that .gitattributes as belonging to the 
repository, since it affects a) how files are checked out and b) how they are 
stored inside the repository.
If committing .gitattributes were only for sharing convenience, git couldn’t 
decide whether to convert line endings when checking out a file. The same 
behavior doesn’t apply to .gitignore, because git will checkout a file that was 
added even though it matches an ignore pattern in .gitignore.

--
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: On interpret-trailers standalone tool

[Christian Couder]

On Mon, Apr 14, 2014 at 11:41 PM, Junio C Hamano gits...@pobox.com wrote:
 Christian Couder chrisc...@tuxfamily.org writes:

 Yeah, except that we could add for example a '-o' option that would
 take a directory as argument and that would mean that the command
 should operate on all the files in this directory. It would be like
 the -o option of the format-patch command.

 For output for which users do not know offhand what files are to be
 produced, giving a single directory with -o makes tons of sense, but
 for input, naming each individual file (and with help with shell
 globs *) is a lot more natural UNIX tool way, I would think.

Yeah, but the git interpret-trailers command is a special, because,
if it takes files as arguments, then it is logical that its output
would be also files and not stdout. (See also at the end of this
message.)

 Take
 everything from this directory cannot be substitute for that, even
 though the reverse (i.e. by naming the input files with dir/*) is
 true.  It is not a viable replacement.

 First, if you think that the command might often be used along with
 format-patch,

 ... I am not singling out format-patch output.  Any text file/stream
 that has the commit log message may benefit from the trailers filter,
 and format-patch output is merely one very obvious example.  As to
 the detection of the end of commit log message, the current EOF is
 where the log message ends (but we would remote trailing blank line)
 can easily be updated to EOF or the first three-dash line.

Ok, I think that it's an interesting feature anyway, so I can add it
now instead of later.

 Third, if trailers arguments are passed to the command using an
 option like -z token=value or -z token:value, it would be nice
 to the user for consistency if the same option could be used when
 passing the same arguments to git commit and perhaps other
 commands like git rebase, git cherry-pick and so on. This
 means that we now have to choose carefully the name of this
 option. Perhaps we can just give it a long name now like --trailer
 and care later about a short name,...

 Absolutely.  That is a very sensible way to go.

Ok, I will use --trailer then. As I said in my previous message,
this unfortunately means that the command will not be very user
friendly until we integrate it with other commands like git commit
and find a short option name that hopefully work for all the commands.

 Fourth, some users might want the command to be passed some files as
 input, but they might not want the command to modify these input
 files. They might prefer the command to write its ouput into another
 set of output files. Maybe a syntax like cat or sed is not very well
 suited for this kind of use, while having a -o option for the output
 directory and a -i option for the input directory (if different from
 the output dir) would be nicer.

 Sure.  I would expect we would require something like Perl's '-i'
 (in-place rewrite) option for this sequence to really work:

 git format-patch -o there -5
 git that-command --options -i there/*

 and without, I would expect the output to come to its standard
 output.

If the input comes from stdin, then I agree that the command should be
a filter, so its output should be on stdout. But if the input comes
from files given as arguments, then I would say that the command
should behave as an editor and by default it should edit its input
file inplace. Its input and output files should be different only if
it is given one or more special option,

Otherwise the example you gave:

$ git format-patch -5 --cover-letter -o +my-series/ my-topic
$ git interpret-trailers some args ./+my-series/0*.patch

would result in having on stdout all the patches edited by git
interpret-trailers.
How would people could then easily send these edited patches?

Thanks,
Christian.
--
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] config.c: fix a compiler warning

[Stepan Kasal]

Date: Thu, 10 Apr 2014 16:37:15 +0200

This change fixes a gcc warning when building msysGit.
---
 config.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index 314d8ee..0b7e4f8 100644
--- a/config.c
+++ b/config.c
@@ -572,7 +572,7 @@ static void die_bad_number(const char *name, const char 
*value)
 
 int git_config_int(const char *name, const char *value)
 {
-   int ret;
+   int ret = 0;
if (!git_parse_int(value, ret))
die_bad_number(name, value);
return ret;
@@ -580,7 +580,7 @@ int git_config_int(const char *name, const char *value)
 
 int64_t git_config_int64(const char *name, const char *value)
 {
-   int64_t ret;
+   int64_t ret = 0;
if (!git_parse_int64(value, ret))
die_bad_number(name, value);
return ret;
-- 
1.9.2.msysgit.0.154.g978f18d

--
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] git tag --contains : avoid stack overflow

[Stepan Kasal]

From: Jean-Jacques Lafay jeanjacques.la...@gmail.com
Date: Sat, 10 Nov 2012 18:36:10 +0100

In large repos, the recursion implementation of contains(commit,
commit_list) may result in a stack overflow. Replace the recursion with
a loop to fix it.

This problem is more apparent on Windows than on Linux, where the stack
is more limited by default.

See also this thread on the msysGit list:

https://groups.google.com/d/topic/msysgit/FqT6boJrb2g/discussion

[jes: re-written to imitate the original recursion more closely]

Thomas Braun pointed out several documentation shortcomings.

Signed-off-by: Jean-Jacques Lafay jeanjacques.la...@gmail.com
Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
Tested-by: Stepan Kasal ka...@ucw.cz
Thanks-to: Thomas Braun thomas.br...@byte-physics.de
---
 builtin/tag.c  | 81 --
 t/t7004-tag.sh | 21 +++
 2 files changed, 88 insertions(+), 14 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 74d3780..79c8c28 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -73,11 +73,13 @@ static int in_commit_list(const struct commit_list *want, 
struct commit *c)
return 0;
 }
 
-static int contains_recurse(struct commit *candidate,
+/*
+ * Test whether the candidate or one of its parents is contained in the list.
+ * Do not recurse to find out, though, but return -1 if inconclusive.
+ */
+static int contains_test(struct commit *candidate,
const struct commit_list *want)
 {
-   struct commit_list *p;
-
/* was it previously marked as containing a want commit? */
if (candidate-object.flags  TMP_MARK)
return 1;
@@ -85,26 +87,77 @@ static int contains_recurse(struct commit *candidate,
if (candidate-object.flags  UNINTERESTING)
return 0;
/* or are we it? */
-   if (in_commit_list(want, candidate))
+   if (in_commit_list(want, candidate)) {
+   candidate-object.flags |= TMP_MARK;
return 1;
+   }
 
if (parse_commit(candidate)  0)
return 0;
 
-   /* Otherwise recurse and mark ourselves for future traversals. */
-   for (p = candidate-parents; p; p = p-next) {
-   if (contains_recurse(p-item, want)) {
-   candidate-object.flags |= TMP_MARK;
-   return 1;
-   }
-   }
-   candidate-object.flags |= UNINTERESTING;
-   return 0;
+   return -1;
+}
+
+/*
+ * Mimicking the real stack, this stack lives on the heap, avoiding stack
+ * overflows.
+ *
+ * At each recursion step, the stack items points to the commits whose
+ * ancestors are to be inspected.
+ */
+struct stack {
+   int nr, alloc;
+   struct stack_entry {
+   struct commit *commit;
+   struct commit_list *parents;
+   } *stack;
+};
+
+static void push_to_stack(struct commit *candidate, struct stack *stack)
+{
+   int index = stack-nr++;
+   ALLOC_GROW(stack-stack, stack-nr, stack-alloc);
+   stack-stack[index].commit = candidate;
+   stack-stack[index].parents = candidate-parents;
 }
 
 static int contains(struct commit *candidate, const struct commit_list *want)
 {
-   return contains_recurse(candidate, want);
+   struct stack stack = { 0, 0, NULL };
+   int result = contains_test(candidate, want);
+
+   if (result = 0)
+   return result;
+
+   push_to_stack(candidate, stack);
+   while (stack.nr) {
+   struct stack_entry *entry = stack.stack[stack.nr - 1];
+   struct commit *commit = entry-commit;
+   struct commit_list *parents = entry-parents;
+
+   if (!parents) {
+   commit-object.flags = UNINTERESTING;
+   stack.nr--;
+   }
+   /*
+* If we just popped the stack, parents-item has been marked,
+* therefore contains_test will return a meaningful 0 or 1.
+*/
+   else switch (contains_test(parents-item, want)) {
+   case 1:
+   commit-object.flags |= TMP_MARK;
+   stack.nr--;
+   break;
+   case 0:
+   entry-parents = parents-next;
+   break;
+   default:
+   push_to_stack(parents-item, stack);
+   break;
+   }
+   }
+   free(stack.stack);
+   return contains_test(candidate, want);
 }
 
 static void show_tag_lines(const unsigned char *sha1, int lines)
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index c8d6e9f..edaff13 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1380,4 +1380,25 @@ test_expect_success 'multiple --points-at are OR-ed 
together' '
test_cmp expect actual
 '
 
+expect
+# ulimit is a bash builtin; we can rely on that in 

[PATCH] Update SVN.pm

[Stepan Kasal]

From: RomanBelinsky belinsky.ro...@gmail.com
Date: Tue, 11 Feb 2014 18:23:02 +0200

fix parsing error for dates like:
2014-01-07T5:58:36.048176Z
previous regex can parse only:
2014-01-07T05:58:36.048176Z
reproduced in my svn repository during conversion.

Signed-off-by: Stepan Kasal ka...@ucw.cz
---
 perl/Git/SVN.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index a59564f..09cff13 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -1321,7 +1321,7 @@ sub get_untracked {
 sub parse_svn_date {
my $date = shift || return '+ 1970-01-01 00:00:00';
my ($Y,$m,$d,$H,$M,$S) = ($date =~ /^(\d{4})\-(\d\d)\-(\d\d)T
-   (\d\d)\:(\d\d)\:(\d\d)\.\d*Z$/x) or
+   (\d\d?)\:(\d\d)\:(\d\d)\.\d*Z$/x) or
 croak Unable to parse date: $date\n;
my $parsed_date;# Set next.
 
-- 
1.9.2.msysgit.0.154.g978f18d

--
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 16/25] commit_lock_file(): die() if called for unlocked lockfile object

[Michael Haggerty]

On 04/15/2014 08:49 AM, Johannes Sixt wrote:
 Am 4/14/2014 15:54, schrieb Michael Haggerty:
 diff --git a/lockfile.c b/lockfile.c
 index 664b0c3..1453a7a 100644
 --- a/lockfile.c
 +++ b/lockfile.c
 @@ -292,6 +292,9 @@ int commit_lock_file(struct lock_file *lk)
  if (lk-fd = 0  close_lock_file(lk))
  return -1;
  
 +if (!lk-filename[0])
 +die(BUG: attempt to commit unlocked object);
 +
 
 Shouldn't this be the first thing to check after entering the function?

You're right; I will change it.

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] config.c: fix a compiler warning

[Jeff King]

On Wed, Apr 16, 2014 at 04:13:53PM +0200, Stepan Kasal wrote:

 Date: Thu, 10 Apr 2014 16:37:15 +0200
 
 This change fixes a gcc warning when building msysGit.

What warning? I'm assuming -Wuninitialized?

 diff --git a/config.c b/config.c
 index 314d8ee..0b7e4f8 100644
 --- a/config.c
 +++ b/config.c
 @@ -572,7 +572,7 @@ static void die_bad_number(const char *name, const char 
 *value)
  
  int git_config_int(const char *name, const char *value)
  {
 - int ret;
 + int ret = 0;
   if (!git_parse_int(value, ret))
   die_bad_number(name, value);
   return ret;

Hmph. Generally gcc should assume that a variable is initialized after a
pointer to it is passed into a function. Unless it inlines that function
and can see that it isn't. But if we do inline git_parse_int, we see
that it ret is always initialized if it returns non-zero.

If it also inlines die_bad_number, it would see that we end in die(),
which is marked NORETURN. But if it does not, it will not realize that
we do not get to return ret in that case.

So perhaps a better solution is:

diff --git a/config.c b/config.c
index 6821cef..a30cb5c 100644
--- a/config.c
+++ b/config.c
@@ -557,6 +557,7 @@ int git_parse_ulong(const char *value, unsigned long *ret)
return 1;
 }
 
+NORETURN
 static void die_bad_number(const char *name, const char *value)
 {
const char *reason = errno == ERANGE ?

Does that also silence the warning?

-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 18/25] struct lock_file: declare some fields volatile

[Michael Haggerty]

On 04/15/2014 08:55 AM, Johannes Sixt wrote:
 Am 4/14/2014 15:54, schrieb Michael Haggerty:
 The function remove_lock_file_on_signal() is used as a signal handler.
 It is not realistic to make the signal handler conform strictly to the
 C standard, which is very restrictive about what a signal handler is
 allowed to do.  But let's increase the likelihood that it will work:

 The lock_file_list global variable and several fields from struct
 lock_file are used by the signal handler.  Declare those values
 volatile to increase the chance that the signal handler will see a
 valid object state.
 
 Yes, it's important that the signal handler sees a valid object state, and
 volatile can help here. But I think the reason why it helps is not
 obvious, and it should be mentioned in the commit message:
 
 It is not so much that volatile forces the compiler to lay down each
 access of the variable coded in C in the assembly code, but more
 importantly, that volatile disallows any re-ordering of these accesses.
 Then:
 
 - 'lock-active = 1' must be the last assignment during setup
 
 - 'lock-active = 0' must be the first assignment during tear-down.
 
 - Ideally, all members of struct lock_file should be volatile.
 
 The last point is important because the compiler is allowed to re-order
 accesses to non-volatile variables across volatile accesses. I say
 ideally because if filename were defined volatile filename[PATH_MAX],
 strcpy() could not be used anymore. OTOH, it is unlikely that a compiler
 re-orders a strcpy() call across other expressions, and we can get away
 without volatile in the filename case in practice.

Thanks for the clarification.  I will edit the commit message to better
explain the rationale.

 Suggested-by: Johannes Sixt j.s...@viscovery.net
 
 Not a big deal, but just in case you re-roll again and you do not forget:
 
   Johannes Sixt j...@kdbg.org
 
 is preferred.

ACK.

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git tag --contains : avoid stack overflow

[Jeff King]

On Wed, Apr 16, 2014 at 04:15:19PM +0200, Stepan Kasal wrote:

 From: Jean-Jacques Lafay jeanjacques.la...@gmail.com
 Date: Sat, 10 Nov 2012 18:36:10 +0100
 
 In large repos, the recursion implementation of contains(commit,
 commit_list) may result in a stack overflow. Replace the recursion with
 a loop to fix it.
 
 This problem is more apparent on Windows than on Linux, where the stack
 is more limited by default.

I think this is a good thing to be doing, and it looks mostly good to
me. A few comments:

 -static int contains_recurse(struct commit *candidate,
 +/*
 + * Test whether the candidate or one of its parents is contained in the list.
 + * Do not recurse to find out, though, but return -1 if inconclusive.
 + */
 +static int contains_test(struct commit *candidate,
   const struct commit_list *want)

Can we turn this return value into

  enum {
CONTAINS_UNKNOWN = -1,
CONTAINS_NO = 0,
CONTAINS_YES = 1,
  } contains_result;

to make the code a little more self-documenting?

  static int contains(struct commit *candidate, const struct commit_list *want)
  {
 - return contains_recurse(candidate, want);
 + struct stack stack = { 0, 0, NULL };
 + int result = contains_test(candidate, want);
 +
 + if (result = 0)
 + return result;

Then this can become:

  if (result != CONTAINS_UNKNOWN)
return result;

 + if (!parents) {
 + commit-object.flags = UNINTERESTING;
 + stack.nr--;
 + }

Shouldn't this be |= when setting the flag?

 + /*
 +  * If we just popped the stack, parents-item has been marked,
 +  * therefore contains_test will return a meaningful 0 or 1.
 +  */
 + else switch (contains_test(parents-item, want)) {
 + case 1:
 + commit-object.flags |= TMP_MARK;
 + stack.nr--;
 + break;
 + case 0:
 + entry-parents = parents-next;
 + break;
 + default:
 + push_to_stack(parents-item, stack);
 + break;
 + }

And if we have an enum, this switch() becomes more readable (the
default here threw me off initially, because it is actually just
looking for -1).

 +expect
 +# ulimit is a bash builtin; we can rely on that in MinGW, but nowhere else
 +test_expect_success MINGW '--contains works in a deep repo' '
 + ulimit -s 64

It would be nice to test this on Linux.

Can we do something like:

  test_lazy_prereq BASH 'bash --version'

  test_expect_success BASH '--contains works in a deep repo' '
... setup repo ...
bash -c ulimit -s 64  git tag --contains HEAD actual 
test_cmp expect actual
  '

As a bonus, then our ulimit call does not pollute the environment of
subsequent tests.

-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] config.c: fix a compiler warning

[Torsten Bögershausen]

On 2014-04-16 17.29, Jeff King wrote:
 On Wed, Apr 16, 2014 at 04:13:53PM +0200, Stepan Kasal wrote:
 
 Date: Thu, 10 Apr 2014 16:37:15 +0200

 This change fixes a gcc warning when building msysGit.

[]
 +NORETURN
  static void die_bad_number(const char *name, const char *value)
  {
   const char *reason = errno == ERANGE ?
 
 Does that also silence the warning?
 
 -Peff
This works under gcc 4.2.1 Mac OS: the warning is away.


--
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/3] rebase: avoid non-function use of return on FreeBSD

[Junio C Hamano]

Kyle J. McKay mack...@gmail.com writes:

 On Apr 14, 2014, at 15:51, Junio C Hamano wrote:
 I think we would want to see the actual change formatted this way
 (without needing to pass -w to git show), as it will make it
 clear that this artificial extra level of define the whole thing
 inside a function and then make a single call to it is a workaround
 of specific shell's glitch that we would not have to have in an
 ideal world ;-)

 Besides that would make it less likely to cause conflicts with the
 real changes in flight.

 Sounds good to me.

 Please double check what I queued on 'pu' when I push out today's
 integration result.

 diff --git a/git-rebase--am.sh b/git-rebase--am.sh
 index a4f683a5..2ab514ea 100644
 --- a/git-rebase--am.sh
 +++ b/git-rebase--am.sh
 @@ -4,6 +4,13 @@
  # Copyright (c) 2010 Junio C Hamano.
  #

 +# The whole contents of the file is run by dot-sourcing this file
 from
 +# inside a shell function, and returns we see below are expected to
 +# return from that function that dot-sources us.  However, FreeBSD
 +# /bin/sh misbehaves on such a construct, so we will work it around
 by
 +# enclosing the whole thing inside an extra layer of a function.
 +git_rebase__am () {

 I think the wording may be just a bit off:

 and returns we see below are expected to return from that function
 that dot-sources us.

 I thought that was one of the buggy behaviors we are working around?

Yeah, it is written as if every reader has the knowledge that the
extra

extra__func () {
...
}
extra__func

around did not originally exist.  The description does not read well
with the work-around already there.

 Maybe I'm just reading it wrong...

 Does this wording seem any clearer?

   and returns we see below are expected not to return
   from the function that dot-sources us, but rather to
   the next command after the one in the function that
   dot-sources us.

Not really.  The comment was not about explaining returns.  When a
reader reads the text with the work-around, it is clear that these
returns are inside this extra function and there is no possible
interpretation other than that they are to return from the extra
function.

The comment was meant to explain why a seemingly strange define a
function and then immediately call it just once pattern is used,
and Earlier, these returns were not inside any function when this
file is viewed standalone.  Because this file is to be dot-sourced
within a function, they were to return from that dot-sourcing
function --- but some shells do not behave that way, hence this
strange construct. was meant to be that explanation, but apparently
it failed to convey what I meant to say.

 If I'm the only one getting a wrong meaning from the comments, then no
 reason to change them.

I agree that the description does not read well with the work-around
already there.  I am not sure what would be a better way to phrase
it, 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 1/6] compat/regex/regcomp.c: reduce scope of variables

[Jonathan Nieder]

Hi,

Elia Pinto wrote:

 [Subject: compat/regex/regcomp.c: reduce scope of variables]

gnulib regex is still maintained upstream and available under the
LGPL 2.1+.  Shouldn't we make the change there and reimport to
make it easier to pull in later changes?

Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: wrong handling of text git attribute leading to files incorrectly reported as modified

[Junio C Hamano]

Frank Ammeter g...@ammeter.ch writes:

 Am 15.04.2014 um 23:23 schrieb Junio C Hamano gits...@pobox.com:

 Brandon McCaig bamcc...@gmail.com writes:
 
 That is for your benefit, and for easily sharing that configuration
 with collaborators. Git only cares that the file exists in your
 working tree at run-time.
 
 It is a lot more than for sharing.  If you made .gitignore only
 effective after it gets committed, you cannot test your updated
 version of .gitignore is correct before committing the change.

 Ok, I can follow that logic for .gitignore, but I was talking about 
 .gitattributes...

They are conceptually the same thing, so if you can follow the logic
for .gitignore, you already can follow the logic for .gitattributes.

The only two readons we have a separate .gitignore are because other
SCMs had a similar mechanism, and because it came before attributes.
If we didn't have these two constraints, it would have made a lot
more sense to express this path is to be ignored by setting
ignored attribute.
--
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 0/3] Make update refs more atomic

[Ronnie Sahlberg]

On Tue, Apr 15, 2014 at 1:32 PM, Michael Haggerty mhag...@alum.mit.edu wrote:
 On 04/15/2014 06:33 PM, Ronnie Sahlberg wrote:
 On Mon, Apr 14, 2014 at 11:36 PM, Michael Haggerty mhag...@alum.mit.edu 
 wrote:
 [...]
 I wonder, however, whether your approach of changing callers from

 lock = lock_ref_sha1_basic() (or varient of)
 write_ref_sha1(lock)

 to

 lock = lock_ref_sha1_basic() (or varient of)
 write_ref_sha1(lock)
 unlock_ref(lock) | commit_ref_lock(lock)

 is not doing work that we will soon need to rework.  Would it be jumping
 the gun to change the callers to

 transaction = ref_transaction_begin();
 ref_transaction_{update,delete,etc}(transaction, ...);
 ref_transaction_{commit,rollback}(transaction, ...);

 instead?  Then we could bury the details of calling write_ref_sha1() and
 commit_lock_ref() inside ref_transaction_commit() rather than having to
 expose them in the public API.

 I think you are right.

 Lets put this patch series on the backburner for now and start by
 making all callers use transactions
 and remove write_ref_sha1() from the public API thar refs.c exports.

 Once everything is switched over to transactions I can rework this
 patchseries for ref_transaction_commit()
 and resubmit to the mailing list.

 Sounds good.  Rewriting callers to use transactions would be a great
 next step.  Please especially keep track of what new features the
 transactions API still needs.  More flexible error handling?  The
 ability to have steps in the transaction that are best-effort (i.e.,
 don't abort the transaction if they fail)?  Different reflog messages
 for different updates within the same transaction rather than one reflog
 message for all updates?  Etc.

 And some callers who currently change multiple references one at a time
 might be able to be rewritten to update the references in a single
 transaction.

As an experiment I rewrite most of the callers to use transactions yesterday.
Most callers would translate just fine, but some callers, such as walker_fetch()
does not yet fit well with the current transaction code.

For example that code does want to first take out locks on all refs
before it does a
lot of processing, with the locks held, before it writes and updates the refs.


Some of my thoughts after going over the callers :

Currently any locking of refs in a transaction only happens during the commit
phase. I think it would be useful to have a mechanism where you could
optionally take out locks for the involved refs early during the transaction.
So that simple callers could continue using
ref_transaction_begin()
ref_transaction_create|update|delete()*
ref_transaction_commit()

but, if a caller such as walker_fetch() could opt to do
ref_transaction_begin()
ref_transaction_lock_ref()*
...do stuff...
ref_transaction_create|update|delete()*
ref_transaction_commit()

In this second case ref_transaction_commit() would only take out any locks that
are missing during the 'lock the refs loop.

Suggestion 1: Add a ref_transaction_lock_ref() to allow locking a ref
early during
a transaction.


A second idea is to change the signatures for
ref_transaction_create|update|delete()
slightly and allow them to return errors early.
We can check for things like add_update() failing, check that the
ref-name looks sane,
check some of the flags, like if has_old==true then old sha1 should
not be NULL or 0{40}, etc.

Additionally for robustness, if any of these functions detect an error
we can flag this in the
transaction structure and take action during ref_transaction_commit().
I.e. if a ref_transaction_update had a hard failure, do not allow
ref_transaction_commit()
to succeed.

Suggestion 2: Change ref_transaction_create|delete|update() to return an int.
All callers that use these functions should check the function for error.


Suggestion 3: remove the qsort and check for duplicates in
ref_transaction_commit()
Since we are already taking out a lock for each ref we are updating
during the transaction
any duplicate refs will fail the second attempt to lock the same ref which will
implicitly make sure that a transaction will not change the same ref twice.

There are only two caveats I see with this third suggestion.
1, The second lock attempt will always cause a die() since we
eventually would end up
in lock_ref_sha1_basic() and this function will always unconditionally
die() if the lock failed.
But your locking changes are addressing this, right?
(all callers to lock_ref_sha1() or lock_any_ref_for_update() do check
for and handle if the lock
 failed, so that change to not die() should be safe)

2, We would need to take care when a lock fails here to print the
proper error message
so that we still show Multiple updates for ref '%s' not allowed. if
the lock failed because
the transaction had already locked this file.





 Lets start preparing patches to change all external callers to use
 transactions instead.
 I am happy to help preparing patches for this. How do 

[PATCH 001/14] howto-index.sh: use the $( ... ) construct for command substitution

[Elia Pinto]

The Git CodingGuidelines prefer the $(...) construct for command
substitution instead of using the backquotes `...`.

The backquoted form is the traditional method for command
substitution, and is supported by POSIX.  However, all but the
simplest uses become complicated quickly.  In particular, embedded
command substitutions and/or the use of double quotes require
careful escaping with the backslash character.

The patch was generated by:

for _f in $(find . -name *.sh)
do
   sed -i 's@`\(.*\)`@$(\1)@g' ${_f}
done

and then carefully proof-read.

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
 Documentation/howto-index.sh |   12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/howto-index.sh b/Documentation/howto-index.sh
index a234086..167b363 100755
--- a/Documentation/howto-index.sh
+++ b/Documentation/howto-index.sh
@@ -11,8 +11,8 @@ EOF
 
 for txt
 do
-   title=`expr $txt : '.*/\(.*\)\.txt$'`
-   from=`sed -ne '
+   title=$(expr $txt : '.*/\(.*\)\.txt$')
+   from=$(sed -ne '
/^$/q
/^From:[]/{
s///
@@ -21,9 +21,9 @@ do
s/^/by /
p
}
-   ' $txt`
+   ' $txt)
 
-   abstract=`sed -ne '
+   abstract=$(sed -ne '
/^Abstract:[]/{
s/^[^   ]*//
x
@@ -39,11 +39,11 @@ do
x
p
q
-   }' $txt`
+   }' $txt)
 
if grep 'Content-type: text/asciidoc' /dev/null $txt
then
-   file=`expr $txt : '\(.*\)\.txt$'`.html
+   file=$(expr $txt : '\(.*\)\.txt$').html
else
file=$txt
fi
-- 
1.7.10.4

--
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 010/14] git-resolve.sh: use the $( ... ) construct for command substitution

[Elia Pinto]

The Git CodingGuidelines prefer the $(...) construct for command
substitution instead of using the backquotes `...`.

The backquoted form is the traditional method for command
substitution, and is supported by POSIX.  However, all but the
simplest uses become complicated quickly.  In particular, embedded
command substitutions and/or the use of double quotes require
careful escaping with the backslash character.

The patch was generated by:

for _f in $(find . -name *.sh)
do
   sed -i 's@`\(.*\)`@$(\1)@g' ${_f}
done

and then carefully proof-read.

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
 contrib/examples/git-resolve.sh |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/examples/git-resolve.sh b/contrib/examples/git-resolve.sh
index 8f98142..48d0fc9 100755
--- a/contrib/examples/git-resolve.sh
+++ b/contrib/examples/git-resolve.sh
@@ -75,7 +75,7 @@ case $common in
GIT_INDEX_FILE=$G git read-tree -m $c $head $merge \
2/dev/null || continue
# Count the paths that are unmerged.
-   cnt=`GIT_INDEX_FILE=$G git ls-files --unmerged | wc -l`
+   cnt=$(GIT_INDEX_FILE=$G git ls-files --unmerged | wc -l)
if test $best_cnt -le 0 -o $cnt -le $best_cnt
then
best=$c
-- 
1.7.10.4

--
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 002/14] install-webdoc.sh: use the $( ... ) construct for command substitution

[Elia Pinto]

The Git CodingGuidelines prefer the $(...) construct for command
substitution instead of using the backquotes `...`.

The backquoted form is the traditional method for command
substitution, and is supported by POSIX.  However, all but the
simplest uses become complicated quickly.  In particular, embedded
command substitutions and/or the use of double quotes require
careful escaping with the backslash character.

The patch was generated by:

for _f in $(find . -name *.sh)
do
   sed -i 's@`\(.*\)`@$(\1)@g' ${_f}
done

and then carefully proof-read.

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
 Documentation/install-webdoc.sh |6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/install-webdoc.sh b/Documentation/install-webdoc.sh
index 76d69a9..ed8b4ff 100755
--- a/Documentation/install-webdoc.sh
+++ b/Documentation/install-webdoc.sh
@@ -18,17 +18,17 @@ do
else
echo 2 # install $h $T/$h
rm -f $T/$h
-   mkdir -p `dirname $T/$h`
+   mkdir -p $(dirname $T/$h)
cp $h $T/$h
fi
 done
-strip_leading=`echo $T/ | sed -e 's|.|.|g'`
+strip_leading=$(echo $T/ | sed -e 's|.|.|g')
 for th in \
$T/*.html $T/*.txt \
$T/howto/*.txt $T/howto/*.html \
$T/technical/*.txt $T/technical/*.html
 do
-   h=`expr $th : $strip_leading'\(.*\)'`
+   h=$(expr $th : $strip_leading'\(.*\)')
case $h in
RelNotes-*.txt | index.html) continue ;;
esac
-- 
1.7.10.4

--
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 006/14] git-fetch.sh: use the $( ... ) construct for command substitution

[Elia Pinto]

The Git CodingGuidelines prefer the $(...) construct for command
substitution instead of using the backquotes `...`.

The backquoted form is the traditional method for command
substitution, and is supported by POSIX.  However, all but the
simplest uses become complicated quickly.  In particular, embedded
command substitutions and/or the use of double quotes require
careful escaping with the backslash character.

The patch was generated by:

for _f in $(find . -name *.sh)
do
   sed -i 's@`\(.*\)`@$(\1)@g' ${_f}
done

and then carefully proof-read.

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
 contrib/examples/git-fetch.sh |6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/contrib/examples/git-fetch.sh b/contrib/examples/git-fetch.sh
index a314273..5540709 100755
--- a/contrib/examples/git-fetch.sh
+++ b/contrib/examples/git-fetch.sh
@@ -67,7 +67,7 @@ do
keep='-k -k'
;;
--depth=*)
-   shallow_depth=--depth=`expr z$1 : 'z-[^=]*=\(.*\)'`
+   shallow_depth=--depth=$(expr z$1 : 'z-[^=]*=\(.*\)')
;;
--depth)
shift
@@ -262,12 +262,12 @@ fetch_per_ref () {
   http://* | https://* | ftp://*)
  test -n $shallow_depth 
die shallow clone with http not supported
- proto=`expr $remote : '\([^:]*\):'`
+ proto=$(expr $remote : '\([^:]*\):')
  if [ -n $GIT_SSL_NO_VERIFY ]; then
  curl_extra_args=-k
  fi
  if [ -n $GIT_CURL_FTP_NO_EPSV -o \
-   `git config --bool http.noEPSV` = true ]; then
+   $(git config --bool http.noEPSV) = true ]; then
  noepsv_opt=--disable-epsv
  fi
 
-- 
1.7.10.4

--
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 009/14] git-repack.sh: use the $( ... ) construct for command substitution

[Elia Pinto]

The Git CodingGuidelines prefer the $(...) construct for command
substitution instead of using the backquotes `...`.

The backquoted form is the traditional method for command
substitution, and is supported by POSIX.  However, all but the
simplest uses become complicated quickly.  In particular, embedded
command substitutions and/or the use of double quotes require
careful escaping with the backslash character.

The patch was generated by:

for _f in $(find . -name *.sh)
do
   sed -i 's@`\(.*\)`@$(\1)@g' ${_f}
done

and then carefully proof-read.

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
 contrib/examples/git-repack.sh |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/examples/git-repack.sh b/contrib/examples/git-repack.sh
index 7579331..f312405 100755
--- a/contrib/examples/git-repack.sh
+++ b/contrib/examples/git-repack.sh
@@ -49,7 +49,7 @@ do
shift
 done
 
-case `git config --bool repack.usedeltabaseoffset || echo true` in
+case $(git config --bool repack.usedeltabaseoffset || echo true) in
 true)
extra=$extra --delta-base-offset ;;
 esac
-- 
1.7.10.4

--
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 011/14] git-revert.sh: use the $( ... ) construct for command substitution

[Elia Pinto]

The Git CodingGuidelines prefer the $(...) construct for command
substitution instead of using the backquotes `...`.

The backquoted form is the traditional method for command
substitution, and is supported by POSIX.  However, all but the
simplest uses become complicated quickly.  In particular, embedded
command substitutions and/or the use of double quotes require
careful escaping with the backslash character.

The patch was generated by:

for _f in $(find . -name *.sh)
do
   sed -i 's@`\(.*\)`@$(\1)@g' ${_f}
done

and then carefully proof-read.

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
 contrib/examples/git-revert.sh |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/examples/git-revert.sh b/contrib/examples/git-revert.sh
index 6bf155c..7e2aad5 100755
--- a/contrib/examples/git-revert.sh
+++ b/contrib/examples/git-revert.sh
@@ -137,7 +137,7 @@ cherry-pick)
q
}'
 
-   logmsg=`git show -s --pretty=raw --encoding=$encoding $commit`
+   logmsg=$(git show -s --pretty=raw --encoding=$encoding $commit)
set_author_env=`echo $logmsg |
LANG=C LC_ALL=C sed -ne $pick_author_script`
eval $set_author_env
-- 
1.7.10.4

--
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 004/14] git-clone.sh: use the $( ... ) construct for command substitution

[Elia Pinto]

The Git CodingGuidelines prefer the $(...) construct for command
substitution instead of using the backquotes `...`.

The backquoted form is the traditional method for command
substitution, and is supported by POSIX.  However, all but the
simplest uses become complicated quickly.  In particular, embedded
command substitutions and/or the use of double quotes require
careful escaping with the backslash character.

The patch was generated by:

for _f in $(find . -name *.sh)
do
   sed -i 's@`\(.*\)`@$(\1)@g' ${_f}
done

and then carefully proof-read.

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
 contrib/examples/git-clone.sh |   20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/contrib/examples/git-clone.sh b/contrib/examples/git-clone.sh
index 547228e..b4c9376 100755
--- a/contrib/examples/git-clone.sh
+++ b/contrib/examples/git-clone.sh
@@ -40,7 +40,7 @@ eval $(echo $OPTIONS_SPEC | git rev-parse --parseopt -- 
$@ || echo exit $?)
 
 get_repo_base() {
(
-   cd `/bin/pwd` 
+   cd $(/bin/pwd) 
cd $1 || cd $1.git 
{
cd .git
@@ -50,7 +50,7 @@ get_repo_base() {
 }
 
 if [ -n $GIT_SSL_NO_VERIFY -o \
-   `git config --bool http.sslVerify` = false ]; then
+   $(git config --bool http.sslVerify) = false ]; then
 curl_extra_args=-k
 fi
 
@@ -70,7 +70,7 @@ clone_dumb_http () {
clone_tmp=$GIT_DIR/clone-tmp 
mkdir -p $clone_tmp || exit 1
if [ -n $GIT_CURL_FTP_NO_EPSV -o \
-   `git config --bool http.noEPSV` = true ]; then
+   $(git config --bool http.noEPSV) = true ]; then
curl_extra_args=${curl_extra_args} --disable-epsv
fi
http_fetch $1/info/refs $clone_tmp/refs ||
@@ -79,7 +79,7 @@ Perhaps git-update-server-info needs to be run there?
test z$quiet = z  v=-v || v=
while read sha1 refname
do
-   name=`expr z$refname : 'zrefs/\(.*\)'` 
+   name=$(expr z$refname : 'zrefs/\(.*\)') 
case $name in
*^*)continue;;
esac
@@ -88,7 +88,7 @@ Perhaps git-update-server-info needs to be run there?
*)  continue ;;
esac
if test -n $use_separate_remote 
-  branch_name=`expr z$name : 'zheads/\(.*\)'`
+  branch_name=$(expr z$name : 'zheads/\(.*\)')
then
tname=remotes/$origin/$branch_name
else
@@ -100,7 +100,7 @@ Perhaps git-update-server-info needs to be run there?
http_fetch $1/HEAD $GIT_DIR/REMOTE_HEAD ||
rm -f $GIT_DIR/REMOTE_HEAD
if test -f $GIT_DIR/REMOTE_HEAD; then
-   head_sha1=`cat $GIT_DIR/REMOTE_HEAD`
+   head_sha1=$(cat $GIT_DIR/REMOTE_HEAD)
case $head_sha1 in
'ref: refs/'*)
;;
@@ -444,15 +444,15 @@ then
# a non-bare repository is always in separate-remote layout
remote_top=refs/remotes/$origin
head_sha1=
-   test ! -r $GIT_DIR/REMOTE_HEAD || head_sha1=`cat 
$GIT_DIR/REMOTE_HEAD`
+   test ! -r $GIT_DIR/REMOTE_HEAD || head_sha1=$(cat 
$GIT_DIR/REMOTE_HEAD)
case $head_sha1 in
'ref: refs/'*)
# Uh-oh, the remote told us (http transport done against
# new style repository with a symref HEAD).
# Ideally we should skip the guesswork but for now
# opt for minimum change.
-   head_sha1=`expr z$head_sha1 : 'zref: refs/heads/\(.*\)'`
-   head_sha1=`cat $GIT_DIR/$remote_top/$head_sha1`
+   head_sha1=$(expr z$head_sha1 : 'zref: refs/heads/\(.*\)')
+   head_sha1=$(cat $GIT_DIR/$remote_top/$head_sha1)
;;
esac
 
@@ -467,7 +467,7 @@ then
while read name
do
test t = $done  continue
-   branch_tip=`cat $GIT_DIR/$remote_top/$name`
+   branch_tip=$(cat $GIT_DIR/$remote_top/$name)
if test $head_sha1 = $branch_tip
then
echo $name
-- 
1.7.10.4

--
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 005/14] git-commit.sh: use the $( ... ) construct for command substitution

[Elia Pinto]

The Git CodingGuidelines prefer the $(...) construct for command
substitution instead of using the backquotes `...`.

The backquoted form is the traditional method for command
substitution, and is supported by POSIX.  However, all but the
simplest uses become complicated quickly.  In particular, embedded
command substitutions and/or the use of double quotes require
careful escaping with the backslash character.

The patch was generated by:

for _f in $(find . -name *.sh)
do
   sed -i 's@`\(.*\)`@$(\1)@g' ${_f}
done

and then carefully proof-read.

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
 contrib/examples/git-commit.sh |   10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/contrib/examples/git-commit.sh b/contrib/examples/git-commit.sh
index 4aab1a6..5cafe2e 100755
--- a/contrib/examples/git-commit.sh
+++ b/contrib/examples/git-commit.sh
@@ -91,7 +91,7 @@ signoff=
 force_author=
 only_include_assumed=
 untracked_files=
-templatefile=`git config commit.template`
+templatefile=$(git config commit.template)
 while test $# != 0
 do
case $1 in
@@ -350,7 +350,7 @@ t,)
TMP_INDEX=$GIT_DIR/tmp-index$$
W=
test -z $initial_commit  W=--with-tree=HEAD
-   commit_only=`git ls-files --error-unmatch $W -- $@` || exit
+   commit_only=$(git ls-files --error-unmatch $W -- $@) || exit
 
# Build a temporary index and update the real index
# the same way.
@@ -475,8 +475,8 @@ then
 fi
 if test '' != $force_author
 then
-   GIT_AUTHOR_NAME=`expr z$force_author : 'z\(.*[^ ]\) *.*'` 
-   GIT_AUTHOR_EMAIL=`expr z$force_author : '.*\(.*\)'` 
+   GIT_AUTHOR_NAME=$(expr z$force_author : 'z\(.*[^ ]\) *.*') 
+   GIT_AUTHOR_EMAIL=$(expr z$force_author : '.*\(.*\)') 
test '' != $GIT_AUTHOR_NAME 
test '' != $GIT_AUTHOR_EMAIL ||
die malformed --author parameter
@@ -489,7 +489,7 @@ then
rloga='commit'
if [ -f $GIT_DIR/MERGE_HEAD ]; then
rloga='commit (merge)'
-   PARENTS=-p HEAD `sed -e 's/^/-p /' $GIT_DIR/MERGE_HEAD`
+   PARENTS=-p HEAD $(sed -e 's/^/-p /' $GIT_DIR/MERGE_HEAD)
elif test -n $amend; then
rloga='commit (amend)'
PARENTS=$(git cat-file commit HEAD |
-- 
1.7.10.4

--
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 003/14] git-checkout.sh: use the $( ... ) construct for command substitution

[Elia Pinto]

The Git CodingGuidelines prefer the $(...) construct for command
substitution instead of using the backquotes `...`.

The backquoted form is the traditional method for command
substitution, and is supported by POSIX.  However, all but the
simplest uses become complicated quickly.  In particular, embedded
command substitutions and/or the use of double quotes require
careful escaping with the backslash character.

The patch was generated by:

for _f in $(find . -name *.sh)
do
   sed -i 's@`\(.*\)`@$(\1)@g' ${_f}
done

and then carefully proof-read.

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
 contrib/examples/git-checkout.sh |8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/contrib/examples/git-checkout.sh b/contrib/examples/git-checkout.sh
index d2c1f98..683cae7 100755
--- a/contrib/examples/git-checkout.sh
+++ b/contrib/examples/git-checkout.sh
@@ -222,7 +222,7 @@ else
 
# Match the index to the working tree, and do a three-way.
git diff-files --name-only | git update-index --remove --stdin 
-   work=`git write-tree` 
+   work=$(git write-tree) 
git read-tree $v --reset -u $new || exit
 
eval GITHEAD_$new='${new_name:-${branch:-$new}}' 
@@ -233,7 +233,7 @@ else
# Do not register the cleanly merged paths in the index yet.
# this is not a real merge before committing, but just carrying
# the working tree changes along.
-   unmerged=`git ls-files -u`
+   unmerged=$(git ls-files -u)
git read-tree $v --reset $new
case $unmerged in
'') ;;
@@ -269,7 +269,7 @@ if [ $? -eq 0 ]; then
fi
if test -n $branch
then
-   old_branch_name=`expr z$oldbranch : 'zrefs/heads/\(.*\)'`
+   old_branch_name=$(expr z$oldbranch : 'zrefs/heads/\(.*\)')
GIT_DIR=$GIT_DIR git symbolic-ref -m checkout: moving from 
${old_branch_name:-$old} to $branch HEAD refs/heads/$branch
if test -n $quiet
then
@@ -282,7 +282,7 @@ if [ $? -eq 0 ]; then
fi
elif test -n $detached
then
-   old_branch_name=`expr z$oldbranch : 'zrefs/heads/\(.*\)'`
+   old_branch_name=$(expr z$oldbranch : 'zrefs/heads/\(.*\)')
git update-ref --no-deref -m checkout: moving from 
${old_branch_name:-$old} to $arg HEAD $detached ||
die Cannot detach HEAD
if test -n $detach_warn
-- 
1.7.10.4

--
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 013/14] t9360-mw-to-git-clone.sh: use the $( ... ) construct for command substitution

[Elia Pinto]

The Git CodingGuidelines prefer the $(...) construct for command
substitution instead of using the backquotes `...`.

The backquoted form is the traditional method for command
substitution, and is supported by POSIX.  However, all but the
simplest uses become complicated quickly.  In particular, embedded
command substitutions and/or the use of double quotes require
careful escaping with the backslash character.

The patch was generated by:

for _f in $(find . -name *.sh)
do
   sed -i 's@`\(.*\)`@$(\1)@g' ${_f}
done

and then carefully proof-read.

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
 contrib/mw-to-git/t/t9360-mw-to-git-clone.sh |   14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/contrib/mw-to-git/t/t9360-mw-to-git-clone.sh 
b/contrib/mw-to-git/t/t9360-mw-to-git-clone.sh
index 811a90c..22f069d 100755
--- a/contrib/mw-to-git/t/t9360-mw-to-git-clone.sh
+++ b/contrib/mw-to-git/t/t9360-mw-to-git-clone.sh
@@ -191,10 +191,10 @@ test_expect_success 'Git clone works with the shallow 
option' '
test_path_is_file mw_dir_11/Main_Page.mw 
(
cd mw_dir_11 
-   test `git log --oneline Nyan.mw | wc -l` -eq 1 
-   test `git log --oneline Foo.mw | wc -l` -eq 1 
-   test `git log --oneline Bar.mw | wc -l` -eq 1 
-   test `git log --oneline Main_Page.mw | wc -l ` -eq 1
+   test $(git log --oneline Nyan.mw | wc -l) -eq 1 
+   test $(git log --oneline Foo.mw | wc -l) -eq 1 
+   test $(git log --oneline Bar.mw | wc -l) -eq 1 
+   test $(git log --oneline Main_Page.mw | wc -l ) -eq 1
) 
wiki_check_content mw_dir_11/Nyan.mw Nyan 
wiki_check_content mw_dir_11/Foo.mw Foo 
@@ -218,9 +218,9 @@ test_expect_success 'Git clone works with the shallow 
option with a delete page'
test_path_is_file mw_dir_12/Main_Page.mw 
(
cd mw_dir_12 
-   test `git log --oneline Nyan.mw | wc -l` -eq 1 
-   test `git log --oneline Bar.mw | wc -l` -eq 1 
-   test `git log --oneline Main_Page.mw | wc -l ` -eq 1
+   test $(git log --oneline Nyan.mw | wc -l) -eq 1 
+   test $(git log --oneline Bar.mw | wc -l) -eq 1 
+   test $(git log --oneline Main_Page.mw | wc -l ) -eq 1
) 
wiki_check_content mw_dir_12/Nyan.mw Nyan 
wiki_check_content mw_dir_12/Bar.mw Bar 
-- 
1.7.10.4

--
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 007/14] git-ls-remote.sh: use the $( ... ) construct for command substitution

[Elia Pinto]

The Git CodingGuidelines prefer the $(...) construct for command
substitution instead of using the backquotes `...`.

The backquoted form is the traditional method for command
substitution, and is supported by POSIX.  However, all but the
simplest uses become complicated quickly.  In particular, embedded
command substitutions and/or the use of double quotes require
careful escaping with the backslash character.

The patch was generated by:

for _f in $(find . -name *.sh)
do
   sed -i 's@`\(.*\)`@$(\1)@g' ${_f}
done

and then carefully proof-read.

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
 contrib/examples/git-ls-remote.sh |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/examples/git-ls-remote.sh 
b/contrib/examples/git-ls-remote.sh
index fec70bb..2aa89a7 100755
--- a/contrib/examples/git-ls-remote.sh
+++ b/contrib/examples/git-ls-remote.sh
@@ -55,11 +55,11 @@ tmpdir=$tmp-d
 case $peek_repo in
 http://* | https://* | ftp://* )
if [ -n $GIT_SSL_NO_VERIFY -o \
-   `git config --bool http.sslVerify` = false ]; then
+   $(git config --bool http.sslVerify) = false ]; then
curl_extra_args=-k
fi
if [ -n $GIT_CURL_FTP_NO_EPSV -o \
-   `git config --bool http.noEPSV` = true ]; then
+   $(git config --bool http.noEPSV) = true ]; then
curl_extra_args=${curl_extra_args} --disable-epsv
fi
curl -nsf $curl_extra_args --header Pragma: no-cache 
$peek_repo/info/refs ||
-- 
1.7.10.4

--
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] config.c: mark die_bad_number as NORETURN

[Junio C Hamano]

Jeff King p...@peff.net writes:

 On Wed, Apr 16, 2014 at 06:38:19PM +0200, Torsten Bögershausen wrote:

  Does that also silence the warning?
  
 This works under gcc 4.2.1 Mac OS: the warning is away.

 Thanks. I couldn't test myself, as I could not get gcc to generate the
 warning in the first place, but I do not have anything as old as 4.2 on
 hand.

 Here it is with a commit message.

 -- 8 --
 Subject: config.c: mark die_bad_number as NORETURN

 This can help avoid -Wuninitialized false positives in
 git_config_int and git_config_ulong, as the compiler now
 knows that we do not return ret if we hit the error
 codepath.

Thanks for clearly solving the issue and describing the solution.
Will queue.

 Signed-off-by: Jeff King p...@peff.net
 ---
  config.c | 1 +
  1 file changed, 1 insertion(+)

 diff --git a/config.c b/config.c
 index 6821cef..a30cb5c 100644
 --- a/config.c
 +++ b/config.c
 @@ -557,6 +557,7 @@ int git_parse_ulong(const char *value, unsigned long *ret)
   return 1;
  }
  
 +NORETURN
  static void die_bad_number(const char *name, const char *value)
  {
   const char *reason = errno == ERANGE ?
--
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: gitignore vs. exclude vs assume-unchanged?

[Junio C Hamano]

a...@bellandwhistle.net writes:

 Any clarification on the differences much appreciated:

 http://stackoverflow.com/questions/23097368/git-ignore-vs-exclude-vs-assume-unchanged/23097509

Please don't force people to refer to external site.

The .gitignore and .git/info/exclude are the two UIs to invoke the
same mechanism.  In-tree .gitignore are to be shared among project
members (i.e. everybody working on the project should consider the
paths that match the ignore pattern in there as cruft).  On the
other hand, .git/info/exclude is meant for personal ignore patterns
(i.e. you, while working on the project, consider them as cruft).

Assume-unchanged should not be abused for an ignore mechanism.  It
is I know my filesystem operations are slow.  I'll promise Git that
I won't change these paths by making them with that bit---that way,
Git does not have to check if I changed things in there every time I
ask for 'git status' output.  It does not mean anything other than
that.  Especially, it is *not* a promise by Git that Git will always
consider these paths are unmodified---if Git can determine a path
that is marked as assume-unchanged has changed without incurring
extra lstat(2) cost, it reserves the right to report that the path
*has been* modified (as a result, git commit -a is free to commit
that 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

gitignore vs. exclude vs assume-unchanged?

[alex]

Any clarification on the differences much appreciated:

http://stackoverflow.com/questions/23097368/git-ignore-vs-exclude-vs-assume-unchanged/23097509
--
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/3] rebase: avoid non-function use of return on FreeBSD

[Junio C Hamano]

Junio C Hamano gits...@pobox.com writes:

 Kyle J. McKay mack...@gmail.com writes:

 If I'm the only one getting a wrong meaning from the comments, then no
 reason to change them.

 I agree that the description does not read well with the work-around
 already there.  I am not sure what would be a better way to phrase
 it, though.

Here is a tentative rewrite at the beginning and the end of rebase-am:

# The whole contents of the file is run by dot-sourcing this
# file from inside a shell function.  It used to be that
# returns we see below were not inside any function, and
# expected to return from the function that dot-sourced us.
#
# However, FreeBSD /bin/sh misbehaves on such a construct and
# continues to run the statements that follow such a return.
# As a work-around, we introduce an extra layer of a function
# here, and immediately call it after defining it.
git_rebase__am () {

...

}
# ... and then we call the whole thing.
git_rebase__am


By the way, you have this in your log message:

... the git-rebase--*.sh scripts have used a return to return
from the dot command that runs them.  While this is allowed by
POSIX,...


Is it this is allowed, or is it this should be the way and shells
that do not do so are buggy?
--
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/3] rebase: avoid non-function use of return on FreeBSD

[Junio C Hamano]

Junio C Hamano gits...@pobox.com writes:

 By the way, you have this in your log message:

 ... the git-rebase--*.sh scripts have used a return to return
 from the dot command that runs them.  While this is allowed by
 POSIX,...


 Is it this is allowed, or is it this should be the way and shells
 that do not do so are buggy?

Answering myself...

The only unspecified I see is this:

If the shell is not currently executing a function or dot
script, the results are unspecified.

which clearly does not apply to the version before this patch (we
are executing a dot script).  And

The return utility shall cause the shell to stop executing the
current function or dot script.

would mean that we are correct to expect that should not get here
is not reached, as the return 5 would cause the shell to stop
executing the dot script there.

So while this is allowed by POSIX may be a bit misleading and
needs to be reworded, I guess?
--
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: On interpret-trailers standalone tool

[Junio C Hamano]

Junio C Hamano gits...@pobox.com writes:

 ...  I am looking at this
 more from the angle of obtaining a useful building block, while you
 seem to be thinking of this as a specialized tool for a narrow set
 of specifkc tasks.

By the way, I am speaking with a bitter experience of designing the
original format-patch command line parameters, thinking that This
is a specialized tool to let me send what Linus hasn't picked up
yet, so the command should take where Linus is and where I am.

Not using the A..B syntax turned out to be a poor choice but that
realization came long after the ship sailed---back then, A..B syntax
was relatively new and it was unclear that it would become the
universal syntax we use throughout the system to denote a range of
commits in the DAG.

The design mistake for starting at a specialized tool for a narrow
set of specific tasks took considerable effort to retrofit the
general syntax while not breaking the traditional usage.  I am being
cautious here because I do not see us making the same mistake.

--
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: On interpret-trailers standalone tool

[Junio C Hamano]

Junio C Hamano gits...@pobox.com writes:

 ...  I am being
 cautious here because I do not see us making the same mistake.

s/do not/ want to/

Sorry for the noise.
--
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 2/2] commit.c: check for lock error and return early

[Ronnie Sahlberg]

Move the check for the lock failure to happen immediately after
lock_any_ref_for_update().
Previously the lock and the check-if-lock-failed was separated by a handful
of string manipulation statements.

Moving the check to occur immediately after the failed lock makes the
code slightly easier to read and makes it follow the pattern of
 try-to-take-a-lock()
 if (check-if-lock-failed){
error
 }
---
 builtin/commit.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d9550c5..c6320f1 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1672,6 +1672,10 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
   ? NULL
   : current_head-object.sha1,
   0, NULL);
+   if (!ref_lock) {
+   rollback_index_files();
+   die(_(cannot lock HEAD ref));
+   }
 
nl = strchr(sb.buf, '\n');
if (nl)
@@ -1681,10 +1685,6 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
strbuf_insert(sb, 0, reflog_msg, strlen(reflog_msg));
strbuf_insert(sb, strlen(reflog_msg), : , 2);
 
-   if (!ref_lock) {
-   rollback_index_files();
-   die(_(cannot lock HEAD ref));
-   }
if (write_ref_sha1(ref_lock, sha1, sb.buf)  0) {
rollback_index_files();
die(_(cannot update HEAD ref));
-- 
1.9.1.504.g5a62d94

--
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 0/2] Check for lock failures early

[Ronnie Sahlberg]

Callers outside of refs.c use either lock_ref_sha1() or 
lock_any_ref_for_update() to lock a ref during an update.

Two of these places we do not immediately check the lock for failure
making reading the code harder.

One place we do some unrelated string manipulation fucntions before we
check for failure and the other place we rely on that write_ref_sha1()
will check the lock for failure and return an error.


These two patches updates these two places so that we immediately check the
lock for failure and act on it.
It does not change any functionality or logic but makes the code easier to
read by being more consistent.

Version 2:
* Simplify the return on error case in sequencer.c.


Ronnie Sahlberg (2):
  sequencer.c: check for lock failure and bail early in fast_forward_to
  commit.c: check for lock error and return early

 builtin/commit.c | 8 
 sequencer.c  | 4 
 2 files changed, 8 insertions(+), 4 deletions(-)

-- 
1.9.1.504.g5a62d94

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/2] sequencer.c: check for lock failure and bail early in fast_forward_to

[Ronnie Sahlberg]

Change fast_forward_to() to check if locking the ref failed, print a nice
error message and bail out early.
The old code did not check if ref_lock was NULL and relied on the fact
that the write_ref_sha1() would safely detect this condition and set the
return variable ret to indicate an error.
While that is safe, it makes the code harder to read for two reasons:
* Inconsistency.  Almost all other places we do check the lock for NULL
  explicitely, so the naive reader is confused why don't we check here.
* And relying on write_ref_sha1() to detect and return an error for when
  a previous lock_any_ref_for_update() feels obfuscated.

This change should not change any functionality or logic
aside from adding an extra error message when this condition is triggered.
(write_ref_sha1() returns an error silently for this condition)

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 sequencer.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index bde5f04..0a80c58 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -281,8 +281,12 @@ static int fast_forward_to(const unsigned char *to, const 
unsigned char *from,
exit(1); /* the callee should have complained already */
ref_lock = lock_any_ref_for_update(HEAD, unborn ? null_sha1 : from,
   0, NULL);
+   if (!ref_lock)
+   return error(_(Failed to lock HEAD during fast_forward_to));
+
strbuf_addf(sb, %s: fast-forward, action_name(opts));
ret = write_ref_sha1(ref_lock, to, sb.buf);
+
strbuf_release(sb);
return ret;
 }
-- 
1.9.1.504.g5a62d94

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git-rebase: Print name of rev when using shorthand

[Junio C Hamano]

Junio C Hamano gits...@pobox.com writes:

 Furthermore, were we to translate @{-1}, does that mean we
 should also translate @{-2} or prior?

 Surely, why not.  If a user is so forgetful to need help remembering
 where s/he was immediately before, wouldn't it be more helpful to
 give here is where you were reminder for older ones to allow them
 to double check they specified the right thing and spot possible
 mistakes?

After re-reading the proposed log message of your v2, I notice one
thing:

The output from a successful invocation of the shorthand command
git rebase - is something like Fast-forwarded HEAD to @{-1},
which includes a relative reference to a revision. Other
commands that use the shorthand -, such as git checkout -,
typically display the symbolic name of the revision.
  
While the above is not incorrect per-se, have you considered _why_
it is a good thing to show the symbolic name in the first place?

Giving the symbolic name 'master' is good because it is possible
that the user thought the previous branch was 'frotz', forgetting
that another branch was checked out tentatively in between, and the
user ended up rebasing on top of a wrong branch.  Telling what that
previous branch is is a way to help user spot such a potential
mistake.  So I am all for making rebase - report what concrete
branch the branch was replayed on top of, and consider it an incomplete
improvement if rebase @{-1} (or rebase @{-2}) did not get the
same help---especially when I know that the underlying mechanism you
would use to translate @{-1} back to the concrete branch name is the
same for both cases anyway.

By the way, here is a happy tangent.  I was pleasantly surprised to
see what this procedure produced:

$ git checkout -b ef/send-email-absolute-path maint
$ git am -s3c a-patch-by-erik-on-different-topic
$ git checkout bg/rebase-off-of-previous-branch
$ git am -s3c your-v2-patch
$ git checkout jch
$ git merge --no-edit -
$ git merge --no-edit @{-2}
$ git log --first-parent -2 | grep Merge branch

Both short-hands are turned into concrete branch names, as they
should ;-)
--
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] Update SVN.pm

[Junio C Hamano]

Stepan Kasal ka...@ucw.cz writes:

 From: RomanBelinsky belinsky.ro...@gmail.com
 Date: Tue, 11 Feb 2014 18:23:02 +0200

 fix parsing error for dates like:
 2014-01-07T5:58:36.048176Z
 previous regex can parse only:
 2014-01-07T05:58:36.048176Z
 reproduced in my svn repository during conversion.

Interesting.  What other strange forms can they record in their
repositories, I have to wonder.  Can they do

2014-01-07T5:8:6.048176Z

for example?  I am wondering if it is simpler and less error prone
to turn all these we only accept two digits into \d+ not only
for the hour part but also minute and second parts.

 Signed-off-by: Stepan Kasal ka...@ucw.cz
 ---
  perl/Git/SVN.pm | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
 index a59564f..09cff13 100644
 --- a/perl/Git/SVN.pm
 +++ b/perl/Git/SVN.pm
 @@ -1321,7 +1321,7 @@ sub get_untracked {
  sub parse_svn_date {
   my $date = shift || return '+ 1970-01-01 00:00:00';
   my ($Y,$m,$d,$H,$M,$S) = ($date =~ /^(\d{4})\-(\d\d)\-(\d\d)T
 - (\d\d)\:(\d\d)\:(\d\d)\.\d*Z$/x) or
 + (\d\d?)\:(\d\d)\:(\d\d)\.\d*Z$/x) or
croak Unable to parse date: $date\n;
   my $parsed_date;# Set next.
--
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 0/3] Make update refs more atomic

[Junio C Hamano]

Ronnie Sahlberg sahlb...@google.com writes:

 Currently any locking of refs in a transaction only happens during the commit
 phase. I think it would be useful to have a mechanism where you could
 optionally take out locks for the involved refs early during the transaction.
 So that simple callers could continue using
 ref_transaction_begin()
 ref_transaction_create|update|delete()*
 ref_transaction_commit()

 but, if a caller such as walker_fetch() could opt to do
 ref_transaction_begin()
 ref_transaction_lock_ref()*
 ...do stuff...
 ref_transaction_create|update|delete()*
 ref_transaction_commit()

 In this second case ref_transaction_commit() would only take out any locks 
 that
 are missing during the 'lock the refs loop.

 Suggestion 1: Add a ref_transaction_lock_ref() to allow locking a ref
 early during
 a transaction.

Hmph.

I am not sure if that is the right way to go, or instead change all
create/update/delete to take locks without adding a new primitive.

 A second idea is to change the signatures for
 ref_transaction_create|update|delete()
 slightly and allow them to return errors early.
 We can check for things like add_update() failing, check that the
 ref-name looks sane,
 check some of the flags, like if has_old==true then old sha1 should
 not be NULL or 0{40}, etc.

 Additionally for robustness, if any of these functions detect an error
 we can flag this in the
 transaction structure and take action during ref_transaction_commit().
 I.e. if a ref_transaction_update had a hard failure, do not allow
 ref_transaction_commit()
 to succeed.

 Suggestion 2: Change ref_transaction_create|delete|update() to return an int.
 All callers that use these functions should check the function for error.

I think that is a very sensible thing to do.

The details of determining this cannot possibly succeed may change
(for example, if we have them take locks at the point of
create/delete/update, a failure to lock may count as an early
error).

Is there any reason why this should be conditional (i.e. you said
allow them to, implying that the early failure is optional)?

 Suggestion 3: remove the qsort and check for duplicates in
 ref_transaction_commit()
 Since we are already taking out a lock for each ref we are updating
 during the transaction
 any duplicate refs will fail the second attempt to lock the same ref which 
 will
 implicitly make sure that a transaction will not change the same ref twice.

I do not know if I care about the implementation detail of do we
have a unique set of update requests?.  While I do not see a strong
need for one transaction to touch the same ref twice (e.g. create to
point at commit A and update it to point at commit B), I do not see
why we should forbid such a use in the future.

Just some of my knee-jerk reactions.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 00/25] Lockfile correctness and refactoring

[Michael Haggerty]

On 04/15/2014 08:40 PM, Torsten Bögershausen wrote:
 refs.c:
 int close_ref(struct ref_lock *lock)
 {
   if (close_lock_file(lock-lk))
   return -1;
   lock-lock_fd = -1;
   return 0;
 }
 
 When the close() fails, fd is still = 0, even if the file is closed.
 
 Could it be written like this ?
 
 int close_ref(struct ref_lock *lock)
 {
   return close_lock_file(lock-lk);
 }

It seem to me it would be better to set lock-lock_fd to -1 regardless
of whether close_lock_file() succeeds.  Or maybe this field is not even
needed, and instead lock-lk-fd should be used.

This is a bit beyond the scope of this patch series, but I will put it
on my todo list.

 =
 lockfile.c, line 49
  *   - filename holds the filename of the lockfile
 
 I think we have a strbuf here? (which is a good thing),
 should we write:
  *   - strbuf filename holds the filename of the lockfile
 --
 (and at some places filename[0] is mentioned,
 should that be filename.buf[0] ?)

I think it is OK to speak of a strbuf as holding a filename (what else
would that construct mean?

But you are correct that the comments shouldn't speak of filename[0]
anymore.  I will fix it.

 =
 int commit_lock_file(struct lock_file *lk)
 {
   static struct strbuf result_file = STRBUF_INIT;
   int err;
 
   if (lk-fd = 0  close_lock_file(lk))
   return -1;
 ##What happens if close() fails and close_lock_file() returns != 0?
 ##Is the lk now in a good shaped state?
 I think the file which had been open by lk-fd is in an unkown state,
 and should not be used for anything.
 When I remember it right, an error on close() can mean the file could not
 be written and closed as expected, it can be truncated or corrupted.
 This can happen on a network file system like NFS, or probably even other FS.
 For me the failing of close() means I smell a need for a rollback.

Yes, this is a good catch.  I think a rollback should definitely be done
in this case.  I will fix it.

In fact, I'm wondering whether it would be appropriate for
close_lock_file() itself to do a rollback if close() fails.  I guess I
will first have to audit callers to make sure that they don't try to use
lock_file::filename after a failed close_lock_file() (e.g., for
generating an error message).

 Please treat my comments more than questions rather than answers,
 thanks for an interesting reading

Thanks for your helpful comments!

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Unicode: update of combining code points

[Torsten Bögershausen]

On 2014-04-16 12.51, Kevin Bracey wrote:
 On 16/04/2014 07:48, Torsten Bögershausen wrote:
 On 15.04.14 21:10, Peter Krefting wrote:
 Torsten Bögershausen:

 diff --git a/utf8.c b/utf8.c
 index a831d50..77c28d4 100644
 --- a/utf8.c
 +++ b/utf8.c
 Is there a script that generates this code from the Unicode database files, 
 or did you hand-update it?

 Some of the code points which have 0 length on the display are called
 combining, others are called vowels or accents.
 E.g. 5BF is not marked any of them, but if you look at the glyph, it should
 be combining (please correct me if that is wrong).
 
 Indeed it is combining (more specifically it has General Category 
 Nonspacing_Mark = Mn).
 

 If I could have found a file which indicates for each code point, what it
 is, I could write a script.

 
 The most complete and machine-readable data are in these files:
 
 http://www.unicode.org/Public/UCD/latest/ucd/UnicodeData.txt
 http://www.unicode.org/Public/UCD/latest/ucd/EastAsianWidth.txt
 
 The general categories can also be seen more legibly in:
 
 http://www.unicode.org/Public/UCD/latest/ucd/extracted/DerivedGeneralCategory.txt
 
 For docs, see:
 
 http://www.unicode.org/reports/tr44/
 http://www.unicode.org/reports/tr11/
 http://www.unicode.org/ucd/
 
 The existing utf8.c comments describe the attributes being selected from the 
 tables (general categories Cf,Mn,Me, East Asian Width W, F). And 
 they suggest that the combining character table was originally auto-generated 
 from UnicodeData.txt with a uniset tool. Presumably this?
 
 https://github.com/depp/uniset
 
 The fullwidth-checking code looks like it was done by hand, although 
 apparently uniset can process EastAsianWidth.txt.
 
 Kevin
Excellent, thanks for the pointers.
Running the script below shows that 
0X00AD SOFT HYPHEN should have zero length (and some others too).
I wonder if that is really the case, and which one of the last 2 lines 
in the script is the right one.

What does this mean for us:
Cf Format  a format control character


#!/bin/sh

if ! test -f UnicodeData.txt; then
  wget http://www.unicode.org/Public/UCD/latest/ucd/UnicodeData.txt
fi 
if ! test -f EastAsianWidth.txt; then
  wget http://www.unicode.org/Public/UCD/latest/ucd/EastAsianWidth.txt
fi
if ! test -f DerivedGeneralCategory.txt; then
  wget 
http://www.unicode.org/Public/UCD/latest/ucd/extracted/DerivedGeneralCategory.txt
fi 
if ! test -d uniset; then
  git clone https://github.com/tboegi/uniset.git
fi 
(
  cd uniset 
  if ! test -x uniset; then 
autoreconf -i 
./configure --enable-warnings=-Werror CFLAGS='-O0 -ggdb'
  fi 
  make
) 
UNICODE_DIR=. ./uniset/uniset --32 cat:Me,Mn,Cf
#UNICODE_DIR=. ./uniset/uniset --32 cat:Me,Mn










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

--
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] tag: add -i and --introduced modifier for --contains

[Luis R. Rodriguez]

From: Luis R. Rodriguez mcg...@suse.com

Upstream Linux kernel commit c5905afb was introduced on v3.4 but
git describe --contains yields v3.5 while if we use git to look
for the first parent with git describe --first-parent yields
v3.3. The reason for this seems to be that the merge commit that
introduced c5905afb was based on v3.3. At least for --contains
its unclear to me why we get v3.5, the result is not intuitive,
as for --first-parent the issue is that the first parent actually
*is* v3.3. The easiest way to address this it to rely on on the
git tag --contains implmenetation and add a modifier that specifies
you want the tag that first introduced the specified commit.

mcgrof@ergon ~/linux (git::master)$ git tag -i --contains c5905afb
v3.4

mcgrof@ergon ~/linux (git::master)$ git tag --introduced --contains c5905afb
v3.4

Cc: Jiri Slaby jsl...@suse.cz
Cc: Andreas Schwab sch...@suse.de
Cc: Jan Kara j...@suse.cz
Signed-off-by: Luis R. Rodriguez mcg...@suse.com
---
 builtin/tag.c | 21 -
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 6c7c6bd..65a939b 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -21,7 +21,7 @@
 static const char * const git_tag_usage[] = {
N_(git tag [-a|-s|-u key-id] [-f] [-m msg|-F file] tagname 
[head]),
N_(git tag -d tagname...),
-   N_(git tag -l [-n[num]] [--contains commit] [--points-at object] 

+   N_(git tag -l [-n[num]] [--contains commit] [ -i | --introduced 
--contains commit ] [--points-at object] 
\n\t\t[pattern...]),
N_(git tag -v tagname...),
NULL
@@ -195,13 +195,18 @@ static int sort_by_version(const void *a_, const void *b_)
 }
 
 static int list_tags(const char **patterns, int lines,
-struct commit_list *with_commit, int sort)
+struct commit_list *with_commit, int sort,
+int introduced)
 {
struct tag_filter filter;
 
filter.patterns = patterns;
filter.lines = lines;
-   filter.sort = sort;
+   if (introduced) {
+   sort = VERCMP_SORT;
+   filter.sort = sort;
+   } else
+   filter.sort = sort;
filter.with_commit = with_commit;
memset(filter.tags, 0, sizeof(filter.tags));
filter.tags.strdup_strings = 1;
@@ -216,8 +221,11 @@ static int list_tags(const char **patterns, int lines,
for (i = filter.tags.nr - 1; i = 0; i--)
printf(%s\n, filter.tags.items[i].string);
else
-   for (i = 0; i  filter.tags.nr; i++)
+   for (i = 0; i  filter.tags.nr; i++) {
printf(%s\n, filter.tags.items[i].string);
+   if (introduced)
+   break;
+   }
string_list_clear(filter.tags, 0);
}
return 0;
@@ -493,6 +501,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
char *cleanup_arg = NULL;
int annotate = 0, force = 0, lines = -1;
int cmdmode = 0, sort = 0;
+   int introduced = 0;
const char *msgfile = NULL, *keyid = NULL;
struct msg_arg msg = { 0, STRBUF_INIT };
struct commit_list *with_commit = NULL;
@@ -511,6 +520,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 N_(tag message), parse_msg_arg),
OPT_FILENAME('F', file, msgfile, N_(read message from 
file)),
OPT_BOOL('s', sign, opt.sign, N_(annotated and GPG-signed 
tag)),
+   OPT_BOOL('i', introduced, introduced, N_(print the first 
tag that introduced the commit)),
OPT_STRING(0, cleanup, cleanup_arg, N_(mode),
N_(how to strip spaces and #comments from message)),
OPT_STRING('u', local-user, keyid, N_(key-id),
@@ -576,7 +586,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
}
if (lines != -1  sort)
die(_(--sort and -n are incompatible));
-   ret = list_tags(argv, lines == -1 ? 0 : lines, with_commit, 
sort);
+   ret = list_tags(argv, lines == -1 ? 0 : lines, with_commit,
+   sort, introduced);
if (column_active(colopts))
stop_column_filter();
return ret;
-- 
1.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: [PATCH v4 0/3] Make update refs more atomic

[Ronnie Sahlberg]

On Wed, Apr 16, 2014 at 12:31 PM, Junio C Hamano gits...@pobox.com wrote:
 Ronnie Sahlberg sahlb...@google.com writes:

 Currently any locking of refs in a transaction only happens during the commit
 phase. I think it would be useful to have a mechanism where you could
 optionally take out locks for the involved refs early during the transaction.
 So that simple callers could continue using
 ref_transaction_begin()
 ref_transaction_create|update|delete()*
 ref_transaction_commit()

 but, if a caller such as walker_fetch() could opt to do
 ref_transaction_begin()
 ref_transaction_lock_ref()*
 ...do stuff...
 ref_transaction_create|update|delete()*
 ref_transaction_commit()

 In this second case ref_transaction_commit() would only take out any locks 
 that
 are missing during the 'lock the refs loop.

 Suggestion 1: Add a ref_transaction_lock_ref() to allow locking a ref
 early during
 a transaction.

 Hmph.

 I am not sure if that is the right way to go, or instead change all
 create/update/delete to take locks without adding a new primitive.

ack.


 A second idea is to change the signatures for
 ref_transaction_create|update|delete()
 slightly and allow them to return errors early.
 We can check for things like add_update() failing, check that the
 ref-name looks sane,
 check some of the flags, like if has_old==true then old sha1 should
 not be NULL or 0{40}, etc.

 Additionally for robustness, if any of these functions detect an error
 we can flag this in the
 transaction structure and take action during ref_transaction_commit().
 I.e. if a ref_transaction_update had a hard failure, do not allow
 ref_transaction_commit()
 to succeed.

 Suggestion 2: Change ref_transaction_create|delete|update() to return an int.
 All callers that use these functions should check the function for error.

 I think that is a very sensible thing to do.

 The details of determining this cannot possibly succeed may change
 (for example, if we have them take locks at the point of
 create/delete/update, a failure to lock may count as an early
 error).

 Is there any reason why this should be conditional (i.e. you said
 allow them to, implying that the early failure is optional)?

It was poor wording on my side. Checking for the ref_transaction_*()
return for error should be mandatory (modulo bugs).

But a caller could be buggy and fail to check properly.
It would be very cheap to detect this condition in
ref_transaction_commit() which could then do

  die(transaction commit called for errored transaction);

which would make it easy to spot this kind of bugs.



 Suggestion 3: remove the qsort and check for duplicates in
 ref_transaction_commit()
 Since we are already taking out a lock for each ref we are updating
 during the transaction
 any duplicate refs will fail the second attempt to lock the same ref which 
 will
 implicitly make sure that a transaction will not change the same ref twice.

 I do not know if I care about the implementation detail of do we
 have a unique set of update requests?.  While I do not see a strong
 need for one transaction to touch the same ref twice (e.g. create to
 point at commit A and update it to point at commit B), I do not see
 why we should forbid such a use in the future.


ack.
--
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 0/3] Make update refs more atomic

[Junio C Hamano]

Ronnie Sahlberg sahlb...@google.com writes:

 I am not sure if that is the right way to go, or instead change all
 create/update/delete to take locks without adding a new primitive.

 ack.

Hmph.  When I say I am not sure, I dunno, etc., I do mean it.

Did you mean by Ack I do not know, either, or I think it is
better to take locks early everywhere?
--
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 0/3] Make update refs more atomic

[Michael Haggerty]

On 04/16/2014 09:31 PM, Junio C Hamano wrote:
 Ronnie Sahlberg sahlb...@google.com writes:
 
 Currently any locking of refs in a transaction only happens during the commit
 phase. I think it would be useful to have a mechanism where you could
 optionally take out locks for the involved refs early during the transaction.
 So that simple callers could continue using
 ref_transaction_begin()
 ref_transaction_create|update|delete()*
 ref_transaction_commit()

 but, if a caller such as walker_fetch() could opt to do
 ref_transaction_begin()
 ref_transaction_lock_ref()*
 ...do stuff...
 ref_transaction_create|update|delete()*
 ref_transaction_commit()

 In this second case ref_transaction_commit() would only take out any locks 
 that
 are missing during the 'lock the refs loop.

 Suggestion 1: Add a ref_transaction_lock_ref() to allow locking a ref
 early during
 a transaction.
 
 Hmph.
 
 I am not sure if that is the right way to go, or instead change all
 create/update/delete to take locks without adding a new primitive.

Junio's suggestion seems like a good idea to me.  Obviously, as soon as
we take out a lock we could also do any applicable old_sha1 check and
possibly fail fast.

Does a verify operation require holding a lock?  If not, when is the
verification done--early, or during the commit, or both?  (I realize
that we don't yet *have* a verify operation at the API level, but we
should!)

We also need to think about for what period of time we have to hold the
packed-refs lock.

Finally, we shouldn't forget that currently the reflog files are locked
by holding the lock on the corresponding loose reference file.  Do we
need to integrate these files into our system any more than they
currently are?

[By the way, I noticed the other day that the command

git reflog expire --stale-fix --expire-unreachable=now --all

can hold loose reference locks for a *long* time (like 10s of minutes),
especially in weird cases like the repository having 9000 packfiles for
some reason or another :-)  The command execution time grows strongly
with the length of the reference's log, or maybe even the square of the
length assuming the history is roughly linear and reachability is
computed separately for each SHA-1.  This is just an empirical
observation so far; I haven't looked into the code yet.]

 A second idea is to change the signatures for
 ref_transaction_create|update|delete()
 slightly and allow them to return errors early.
 We can check for things like add_update() failing, check that the
 ref-name looks sane,
 check some of the flags, like if has_old==true then old sha1 should
 not be NULL or 0{40}, etc.

 Additionally for robustness, if any of these functions detect an error
 we can flag this in the
 transaction structure and take action during ref_transaction_commit().
 I.e. if a ref_transaction_update had a hard failure, do not allow
 ref_transaction_commit()
 to succeed.

 Suggestion 2: Change ref_transaction_create|delete|update() to return an int.
 All callers that use these functions should check the function for error.
 
 I think that is a very sensible thing to do.
 
 The details of determining this cannot possibly succeed may change
 (for example, if we have them take locks at the point of
 create/delete/update, a failure to lock may count as an early
 error).
 
 Is there any reason why this should be conditional (i.e. you said
 allow them to, implying that the early failure is optional)?

Also a good suggestion.  We should make it clear in the documentation
that the create/delete/update functions are not *obliged* to return an
error (for example that the current value of the reference does not
agree with old_sha1) because future alternate ref backends might
possibly not be able to make such checks until the commit phase.  For
example, checking old_sha1 might involve a round-trip to a database or
remote repository, in which case it might be preferable to check all
values in a single round-trip upon commit.

So, callers might be informed early of problems, or they might only
learn about problems when they try to commit the transaction.  They have
to be able to handle either type of error reporting.

So then the question arises (and maybe this is what Ronnie was getting
at by suggesting that the checks might be conditional): are callers
*obliged* to check the return values from create/delete/update, or are
they allowed to just keep throwing everything into the transaction,
ignoring errors, and only check the result of ref_transaction_commit()?

I don't feel strongly one way or the other about this question.  It
might be nice to be able to write callers sloppily, but it would cost a
bit more code in the reference backends.  Though maybe it wouldn't even
be much extra code, given that we would probably want to put consistency
checks in there anyway.

 Suggestion 3: remove the qsort and check for duplicates in
 ref_transaction_commit()
 Since we are already taking out a lock for each ref we are updating
 

hhh

[王宁]

发自我的 iPad

Re: [PATCH] tag: add -i and --introduced modifier for --contains

[Junio C Hamano]

Luis R. Rodriguez mcg...@do-not-panic.com writes:

 From: Luis R. Rodriguez mcg...@suse.com

 Upstream Linux kernel commit c5905afb was introduced on v3.4 but
 git describe --contains yields v3.5

Actually, describe --contains should yield v3.5-rc1~120^3~76^2,
not v3.5.

And you are right that the commit is contained in v3.4, so we also
should be able to describe it as v3.4~479^2~9^2 as well.

And between v3.4 and v3.5-rc1, the latter is a closer anchor point
for that commit (v3.5-rc1 only needs about 200 hops to reach the
commit, while from v3.4 you would need close to 500 hops), hence we
end up picking the latter as a better answer.

Now, with the explanation of how/why this happens behind us, I see
two possible issues with this patch:

 - The reason a human-user rejects v3.5-rc1~120^3~76^2 as the
   solution and favor v3.4~479^2~9^2 could be because of the -rc1
   part in the answer.  Perhaps we would want an option that affects
   which tags are to be used (and which tags are to be excluded) as
   anchoring points?

 - If we are truly interested in finding out the earliest tag that
   contains the given commit, shouldn't we be ignoring the tagname
   and go with the tag with the oldest timestamp?  After all, there
   may be a fix merged to v7.0 first on April 1st, and then on a
   later date the same fix may be merged to the maintenance track to
   be tagged as v6.9.1 on May 5th, and in such a case, wouldn't you
   want to say that the fix first appeared on v7.0 on April 1st,
   instead of on May 5th?

Thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] sequencer.c: check for lock failure and bail early in fast_forward_to

[Michael Haggerty]

On 04/16/2014 08:56 PM, Ronnie Sahlberg wrote:
 Change fast_forward_to() to check if locking the ref failed, print a nice
 error message and bail out early.
 The old code did not check if ref_lock was NULL and relied on the fact
 that the write_ref_sha1() would safely detect this condition and set the

s/the write_ref_sha1()/write_ref_sha1()/

 return variable ret to indicate an error.
 While that is safe, it makes the code harder to read for two reasons:
 * Inconsistency.  Almost all other places we do check the lock for NULL
   explicitely, so the naive reader is confused why don't we check here.

s/explicitely/explicitly/
s/here/here?/

 * And relying on write_ref_sha1() to detect and return an error for when
   a previous lock_any_ref_for_update() feels obfuscated.

s/feels/failed feels/ maybe?

 
 This change should not change any functionality or logic
 aside from adding an extra error message when this condition is triggered.
 (write_ref_sha1() returns an error silently for this condition)

You need a period inside the parentheses.

 
 Signed-off-by: Ronnie Sahlberg sahlb...@google.com
 ---
  sequencer.c | 4 
  1 file changed, 4 insertions(+)
 
 diff --git a/sequencer.c b/sequencer.c
 index bde5f04..0a80c58 100644
 --- a/sequencer.c
 +++ b/sequencer.c
 @@ -281,8 +281,12 @@ static int fast_forward_to(const unsigned char *to, 
 const unsigned char *from,
   exit(1); /* the callee should have complained already */
   ref_lock = lock_any_ref_for_update(HEAD, unborn ? null_sha1 : from,
  0, NULL);
 + if (!ref_lock)
 + return error(_(Failed to lock HEAD during fast_forward_to));

This error message can be emitted to the user in the normal course of
things (i.e., it is not a bug).  So the message should make sense to the
user.  Is fast_forward_to a user-facing term that the user will
understand?  I suspect that you took it from the name of the function,
which is *not* meaningful to a user.

But unfortunately I'm not familiar enough with the sequencer to be able
to suggest a better error message.

 +
   strbuf_addf(sb, %s: fast-forward, action_name(opts));
   ret = write_ref_sha1(ref_lock, to, sb.buf);
 +
   strbuf_release(sb);
   return ret;
  }
 

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] commit.c: check for lock error and return early

[Michael Haggerty]

On 04/16/2014 08:56 PM, Ronnie Sahlberg wrote:
 Move the check for the lock failure to happen immediately after
 lock_any_ref_for_update().
 Previously the lock and the check-if-lock-failed was separated by a handful
 of string manipulation statements.

Please flow sentences together into paragraphs for easier reading,
rather than having an extremely ragged right-hand margin.

The rest looks good.

Michael


 Moving the check to occur immediately after the failed lock makes the
 code slightly easier to read and makes it follow the pattern of
  try-to-take-a-lock()
  if (check-if-lock-failed){
 error
  }
 ---
  builtin/commit.c | 8 
  1 file changed, 4 insertions(+), 4 deletions(-)
 
 diff --git a/builtin/commit.c b/builtin/commit.c
 index d9550c5..c6320f1 100644
 --- a/builtin/commit.c
 +++ b/builtin/commit.c
 @@ -1672,6 +1672,10 @@ int cmd_commit(int argc, const char **argv, const char 
 *prefix)
  ? NULL
  : current_head-object.sha1,
  0, NULL);
 + if (!ref_lock) {
 + rollback_index_files();
 + die(_(cannot lock HEAD ref));
 + }
  
   nl = strchr(sb.buf, '\n');
   if (nl)
 @@ -1681,10 +1685,6 @@ int cmd_commit(int argc, const char **argv, const char 
 *prefix)
   strbuf_insert(sb, 0, reflog_msg, strlen(reflog_msg));
   strbuf_insert(sb, strlen(reflog_msg), : , 2);
  
 - if (!ref_lock) {
 - rollback_index_files();
 - die(_(cannot lock HEAD ref));
 - }
   if (write_ref_sha1(ref_lock, sha1, sb.buf)  0) {
   rollback_index_files();
   die(_(cannot update HEAD ref));
 


-- 
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tag: add -i and --introduced modifier for --contains

[Luis R. Rodriguez]

On Wed, Apr 16, 2014 at 3:02 PM, Junio C Hamano gits...@pobox.com wrote:
 Luis R. Rodriguez mcg...@do-not-panic.com writes:

 From: Luis R. Rodriguez mcg...@suse.com

 Upstream Linux kernel commit c5905afb was introduced on v3.4 but
 git describe --contains yields v3.5

 Actually, describe --contains should yield v3.5-rc1~120^3~76^2,
 not v3.5.

Yes, indeed thanks, sorry I should have been explicit.

 And you are right that the commit is contained in v3.4, so we also
 should be able to describe it as v3.4~479^2~9^2 as well.

That'd be swell :)

 And between v3.4 and v3.5-rc1, the latter is a closer anchor point
 for that commit (v3.5-rc1 only needs about 200 hops to reach the
 commit, while from v3.4 you would need close to 500 hops),

Ah! Thanks for explaining this mysterious puzzle to me. I'm a bit
perplexed why still. Can I trouble you for a little elaboration here?
How could one view from a commit merged on v3.4 possibly yield more
commits to v3.4 than to v3.5 ? Is it because it starts counting on the
merge's parent (v3.3) ?

 hence we
 end up picking the latter as a better answer.

 Now, with the explanation of how/why this happens behind us, I see
 two possible issues with this patch:

  - The reason a human-user rejects v3.5-rc1~120^3~76^2 as the
solution and favor v3.4~479^2~9^2 could be because of the -rc1
part in the answer.  Perhaps we would want an option that affects
which tags are to be used (and which tags are to be excluded) as
anchoring points?

I'd take an rc release as a blessed point too so not sure, and come to
think of it I'm not a bit perplexed why the results for my change did
not yield an rc1 as well.

  - If we are truly interested in finding out the earliest tag that
contains the given commit, shouldn't we be ignoring the tagname
and go with the tag with the oldest timestamp?  After all, there
may be a fix merged to v7.0 first on April 1st, and then on a
later date the same fix may be merged to the maintenance track to
be tagged as v6.9.1 on May 5th,

At least for Linux linux-3.X.y branches (one example linux-3.4.y) on
linux-stable has different commit IDs from patches cherry picked from
Linus' tree, and that patch just referneces the upstream commit from
Linus' tree on the commit log, but nothing more.

 and in such a case, wouldn't you  want to say that the fix first appeared on 
 v7.0 on April 1st,
 instead of on May 5th?

Sure, but I'd expect the folks maintaining v6.9.x would just refer to
the upstream commit ID from v7.0.

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

[l10n] date: Note for translators not included in .po files

[Brian Gesiak]

A note for translators in date.c is not included in git.pot.
Namely, the following note from date.c:147 is not included
(https://github.com/git/git/blob/v1.9.2/date.c#L147):

/* TRANSLATORS: %s is n years */

This is a very useful note for translators (in fact, I think
the zh_CN translation for date.c:149 might be a little off
because this note was not included. My Mandarin is rusty,
but I believe n years, m months ago should be expressed
without a comma).

According to po/README, the l10n coordinator is responsible
for updating the git.pot file. Would it be possible to update it based
on v1.9.2 and include the above comment?

By the way, I am trying to organize contributors to produce a Japanese
localization for Core Git. Currently we have plenty of interest but
only two contributors. If you or anyone you know would like to contribute
please visit the repository here: https://github.com/modocache/git-po-ja

Thanks!

- Brian Gesiak
--
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: gitignore vs. exclude vs assume-unchanged?

[alex]

On 2014-04-16 10:51, Junio C Hamano wrote:

a...@bellandwhistle.net writes:


Any clarification on the differences much appreciated:

http://stackoverflow.com/questions/23097368/git-ignore-vs-exclude-vs-assume-unchanged/23097509


Please don't force people to refer to external site.

The .gitignore and .git/info/exclude are the two UIs to invoke the
same mechanism.  In-tree .gitignore are to be shared among project
members (i.e. everybody working on the project should consider the
paths that match the ignore pattern in there as cruft).  On the
other hand, .git/info/exclude is meant for personal ignore patterns
(i.e. you, while working on the project, consider them as cruft).

Assume-unchanged should not be abused for an ignore mechanism.  It
is I know my filesystem operations are slow.  I'll promise Git that
I won't change these paths by making them with that bit---that way,
Git does not have to check if I changed things in there every time I
ask for 'git status' output.  It does not mean anything other than
that.  Especially, it is *not* a promise by Git that Git will always
consider these paths are unmodified---if Git can determine a path
that is marked as assume-unchanged has changed without incurring
extra lstat(2) cost, it reserves the right to report that the path
*has been* modified (as a result, git commit -a is free to commit
that change).


Thanks June. Great to hear this authoritatively.

IMHO your very helpful explanation about typical use cases, the purpose 
of 'exclude, and assume-unchanged not being a promise is missing from 
the docs, or at least not obviously present:


http://git-scm.com/docs

In particular, 'exclude' is spottily documented. I realize the docs are 
structured strictly as an API reference, but it would be great to see a 
comparison of ignore techniques spelled out. FWIW I asked several people 
I think of as experts and none of them felt sure of their answer. :)


thanks again.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git-rebase: Print name of rev when using shorthand

[Brian Gesiak]

 The concept of n-th prior checkout (aka @{-n}) and immediately
 previous checkout (aka -) are equivalent, even though the former
 may be more generic.

 You seem to be saying that those who understand the former are with
 superiour mental capacity in general than those who only know the
 latter, and they can always remember where they came from.

 ...have you considered _why_ it is a good thing to show the symbolic
 name in the first place?

I think I failed to express my point here; I don't think people that
use @{-1} have superior mental capacity, but rather simply that
they are aware of the @{-n} method of specifying a previous reference.
So in response to the command git rebase @{-4}, displaying the
result Fast-forwarded HEAD to @{-4} does not contain any unknown
syntax that may confuse them. They may not remember what @{-4}
refers to, but they are aware of the syntax at least.

On the other hand, people who use the - shorthand may or may
not be aware of the @{-n} syntax. In that respect, I think it would
be confusing to display Fast-forwarded HEAD to @{-1} in response
to the command git rebase -; the user may not know what @{-1}
means!

Thus my original point was that I felt displaying a symbolic name in
response to git rebase - was more important than doing so in
response to git rebase @{-1}. The issue isn't about forgetting what
@{-n} refers to, it's whether the user even knows what @{-n} is
supposed to mean.

But in light of your other comments:

 Furthermore, were we to translate @{-1}, does that mean we
 should also translate @{-2} or prior?

 Surely, why not.  If a user is so forgetful to need help remembering
 where s/he was immediately before, wouldn't it be more helpful to
 give here is where you were reminder for older ones to allow them
 to double check they specified the right thing and spot possible
 mistakes?

 ...

 Giving the symbolic name 'master' is good because it is possible
 that the user thought the previous branch was 'frotz', forgetting
 that another branch was checked out tentatively in between, and the
 user ended up rebasing on top of a wrong branch.  Telling what that
 previous branch is is a way to help user spot such a potential
 mistake.  So I am all for making rebase - report what concrete
 branch the branch was replayed on top of, and consider it an incomplete
 improvement if rebase @{-1} (or rebase @{-2}) did not get the
 same help---especially when I know that the underlying mechanism you
 would use to translate @{-1} back to the concrete branch name is the
 same for both cases anyway.

I had not originally thought of this, perhaps because I was preoccupied
with preventing users from seeing syntax they might not be aware of.
But I definitely agree that displaying symbolic names for all @{-n}
is a good way to prevent user error.

 I can buy that would be a lot more work, and I do not want to do it
 (or I do not think I can solve it in a more general way), though.

Perish the thought! :)

I will try to re-roll this patch to include symbolic names for @{-n}.

As usual, thanks for the feedback!

- Brian Gesiak
--
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: gitignore vs. exclude vs assume-unchanged?

[Jonathan Nieder]

Hi,

a...@bellandwhistle.net wrote:

 In particular, 'exclude' is spottily documented.

Where did you expect to read about it?  I see some mention of
.git/info/exclude in the gitignore(5) page, but I wouldn't be
surprised if there's room for improvement there (improvements
welcome).

  I realize the docs
 are structured strictly as an API reference,

No, the docs are meant to be helpful, not just to confirm what people
already know. ;-)

Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] rebase: avoid non-function use of return on FreeBSD

[Kyle J. McKay]

On Apr 16, 2014, at 11:11, Junio C Hamano wrote:

Junio C Hamano gits...@pobox.com writes:


Kyle J. McKay mack...@gmail.com writes:

If I'm the only one getting a wrong meaning from the comments,  
then no

reason to change them.


I agree that the description does not read well with the work-around
already there.  I am not sure what would be a better way to phrase
it, though.


Here is a tentative rewrite at the beginning and the end of rebase-am:

   # The whole contents of the file is run by dot-sourcing this
   # file from inside a shell function.  It used to be that
   # returns we see below were not inside any function, and
   # expected to return from the function that dot-sourced us.


I think it's the return from the function that dot-sourced us that  
gives me the wrong meaning.  The return from part says to me that  
function will be returning which it will not unless the dot command  
was the last command in the function.


Either return to the function that dot-sourced us or return from  
the dot command that dot-sourced us, but using the original wording  
implies to me that the function that dot-sourced us will return as  
soon as the dot-sourced script executes the return and that is exactly  
one of the bugs we're working around.


I think just the s/from/to/ would fix it so it does not give me the  
wrong impression, but that doesn't mean that would not confuse  
everyone else.  ;)



   #
   # However, FreeBSD /bin/sh misbehaves on such a construct and
   # continues to run the statements that follow such a return.
   # As a work-around, we introduce an extra layer of a function
   # here, and immediately call it after defining it.
   git_rebase__am () {

   ...

   }
   # ... and then we call the whole thing.
   git_rebase__am


On Apr 16, 2014, at 11:23, Junio C Hamano wrote:

Junio C Hamano gits...@pobox.com writes:
By the way, you have this in your log message:


   ... the git-rebase--*.sh scripts have used a return to return
   from the dot command that runs them.  While this is allowed by
   POSIX,...


Is it this is allowed, or is it this should be the way and shells
that do not do so are buggy?


Answering myself...

The only unspecified I see is this:

   If the shell is not currently executing a function or dot
   script, the results are unspecified.

which clearly does not apply to the version before this patch (we
are executing a dot script).  And

   The return utility shall cause the shell to stop executing the
   current function or dot script.

would mean that we are correct to expect that should not get here
is not reached, as the return 5 would cause the shell to stop
executing the dot script there.

So while this is allowed by POSIX may be a bit misleading and
needs to be reworded, I guess?


allowed by POSIX is not incorrect. ;)

But perhaps the log message should say 'While POSIX specifies that a  
return may be used to exit a dot sourced script,' there instead to  
be clearer.  Which would make that whole first paragraph become:


Since a1549e10, 15d4bf2e and 01a1e646 (first appearing in v1.8.4)  
the
git-rebase--*.sh scripts have used a return to return from the  
dot
command that runs them.  While POSIX specifies that a return  
may be

used to exit a dot sourced script, the FreeBSD /bin/sh utility
behaves poorly under some circumstances when such a return is  
executed.


--
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: [tig] [PATCHv2 3/3] log: Colour the diff stat

[Jonas Fonseca]

On Sun, Apr 13, 2014 at 5:54 PM, Kumar Appaiah
a.ku...@alumni.iitm.ac.in wrote:

 This commit adds custom log_read and log_draw functions that utilize
 the diff stat drawing functions from the diff module. The absence of
 the triple hyphen separator prevents direct usage of the diff drawing
 functions directly.

See my comments below.

 ---
  src/log.c | 55 +--
  1 file changed, 53 insertions(+), 2 deletions(-)

 diff --git a/src/log.c b/src/log.c
 index 40c9a21..468f7c3 100644
 --- a/src/log.c
 +++ b/src/log.c
 @@ -23,6 +23,9 @@ struct log_state {
  * up/down in the log view. */
 int last_lineno;
 enum line_type last_type;
 +   bool commit_title_read;
 +   bool after_commit_header;
 +   bool reading_diff_stat;
  };

  static void
 @@ -78,14 +81,62 @@ log_request(struct view *view, enum request request, 
 struct line *line)
 }
  }

 +static bool
 +log_read(struct view *view, char *data)
 +{
 +   enum line_type type;
 +   struct log_state *state = view-private;
 +   size_t len;
 +
 +   if (!data)
 +   return TRUE;
 +
 +   type = get_line_type(data);
 +   len = strlen(data);
 +
 +   if (type == LINE_COMMIT)
 +   state-commit_title_read = TRUE;
 +   else if (state-commit_title_read  len  1) {
 +   state-commit_title_read = FALSE;
 +   state-after_commit_header = TRUE;
 +   } else if (state-after_commit_header  len  1) {
 +   state-after_commit_header = FALSE;
 +   state-reading_diff_stat = TRUE;
 +   } else if (state-reading_diff_stat) {
 +   bool ret = diff_common_add_diff_stat(view, data);
 +   if (ret) {
 +   return TRUE;
 +   } else {
 +   state-reading_diff_stat = FALSE;
 +   }
 +   }
 +
 +   return pager_common_read(view, data, type);
 +}
 +
 +static bool
 +log_draw(struct view *view, struct line *line, unsigned int lineno)
 +{
 +   char *text = line-data;
 +   enum line_type type = line-type;
 +

This is missing a call to draw_lineno(...)

 +   if (type == LINE_DIFF_STAT) {
 +   diff_common_draw_diff_stat(view, type, text);
 +   draw_text(view, type, text);

I had to #include tig/draw.h for this to compile.

 +   return TRUE;
 +   }
 +
 +   return pager_draw(view, line, lineno);
 +}
 +
  static struct view_ops log_ops = {
 line,
 argv_env.head,
 VIEW_ADD_PAGER_REFS | VIEW_OPEN_DIFF | VIEW_SEND_CHILD_ENTER | 
 VIEW_LOG_LIKE | VIEW_REFRESH,
 sizeof(struct log_state),
 log_open,
 -   pager_read,
 -   pager_draw,
 +   log_read,
 +   log_draw,
 log_request,
 pager_grep,
 log_select,
 --
 1.9.1




-- 
Jonas Fonseca
--
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: [tig] [PATCHv2 0/3] log: colour the diffstat

[Jonas Fonseca]

Hi Kumar,

On Sun, Apr 13, 2014 at 5:54 PM, Kumar Appaiah
a.ku...@alumni.iitm.ac.in wrote:

 These patches add colourization to the log view. They reuse the diff
 stat drawing functions from the diff module directly.

This is a great idea. I wonder though if it would make sense to put
this into the pager backend instead so all pager based views can
benefit unless of course the state machine would end up becoming too
complicated.

 This version just includes some code reformatting and minor
 fixes. Please comment on what other fixes could help.

See my other email regarding fixes. Since I am currently refactoring
how views are drawn I'd prefer to postpone merging this patchset until
the dust has settled. I'll try to rebase the patches once I get there
before reaching out to you.

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

[Bakker, K.]

Dear valued customer,

Do you need an urgent loan to pay of your bills, invest more on your business, 
if yes PREMIUM CAPITAL LOAN offer loan at 3% interest rate. We are fast and 
reliable when it comes to loan lending contact email: 
premiumcapitall...@hotmail.co.uk for more information.

Contact email: premiumcapitall...@hotmail.co.uk


Best Regard
PAMELA MARTHA
Loan officer
Mansfield Road Rotherham South Yorkshire S70 2EB
email: premiumcapitall...@hotmail.co.uk
Tell: +447035958575
Fax: +448447742385

--
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: [tig] [PATCHv2 3/3] log: Colour the diff stat

[Kumar Appaiah]

On Wed, Apr 16, 2014 at 08:44:41PM -0400, Jonas Fonseca wrote:
 On Sun, Apr 13, 2014 at 5:54 PM, Kumar Appaiah
 a.ku...@alumni.iitm.ac.in wrote:
 
  This commit adds custom log_read and log_draw functions that utilize
  the diff stat drawing functions from the diff module. The absence of
  the triple hyphen separator prevents direct usage of the diff drawing
  functions directly.
 
 See my comments below.

Hi Jonas.

  +static bool
  +log_draw(struct view *view, struct line *line, unsigned int lineno)
  +{
  +   char *text = line-data;
  +   enum line_type type = line-type;
  +
 
 This is missing a call to draw_lineno(...)

Noted.

  +   if (type == LINE_DIFF_STAT) {
  +   diff_common_draw_diff_stat(view, type, text);
  +   draw_text(view, type, text);
 
 I had to #include tig/draw.h for this to compile.

I'll take care of this.

I'll send you a pull request eventually. You can handle it after your
refactor is complete.

Thanks!

Kumar
-- 
Kumar Appaiah
--
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: [l10n] date: Note for translators not included in .po files

[Jiang Xin]

2014-04-17 6:51 GMT+08:00 Brian Gesiak modoca...@gmail.com:
 A note for translators in date.c is not included in git.pot.
 Namely, the following note from date.c:147 is not included
 (https://github.com/git/git/blob/v1.9.2/date.c#L147):

 /* TRANSLATORS: %s is n years */


Comments for translators will be extracted to pot file automatically,
when run xgettext with --add-comments option.

   -c, --add-comments
  place all comment blocks preceding keyword lines in output file

For example, the comments in the following code blocks will
be extracted.

   /* TRANSLATORS: will be extracted. */
   strbuf_addf(sb, Q_(%lu year, %lu years, years), years);

strbuf_addf(sb,
   /* TRANSLATORS: will be extracted. */
   Q_(%lu year, %lu years, years), years);

But if the comment is not right before the l10n markers, such
comments will not be extracted. E.g.

/* TRANSLATORS: WARNING: will NOT be extracted. */
strbuf_addf(sb,
Q_(%lu year, %lu years, years), years);

/* TRANSLATORS: WARNING: will NOT be extracted. */
strbuf_addf(sb, Q_(
%lu year, %lu years, years), years);


I will scan all the codes and make a fix.

 This is a very useful note for translators (in fact, I think
 the zh_CN translation for date.c:149 might be a little off
 because this note was not included. My Mandarin is rusty,
 but I believe n years, m months ago should be expressed
 without a comma).

 According to po/README, the l10n coordinator is responsible
 for updating the git.pot file. Would it be possible to update it based
 on v1.9.2 and include the above comment?


I could generate a new git.pot for maint branch, but fixes for codes
may only contribute to master branch.


-- 
Jiang Xin
--
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] send-email: recognize absolute path on Windows

[brian m. carlson]

On Wed, Apr 16, 2014 at 10:19:54AM -0700, Junio C Hamano wrote:
 Ahh, OK, if you did so, you won't have any place to hook the only
 on msys do this trick into.
 
 It somehow feels somewhat confusing that we define a sub with the
 same name as the system one, while not overriding it entirely but
 delegate back to the system one.  I am debating myself if it is more
 obvious if it is done this way:
 
 use File::Spec::Functions qw(file_name_is_absolute);
 if ($^O eq 'msys') {

You would probably want a no warnings 'redefine' here as well.

 sub file_name_is_absolute {
   return $_[0] =~ /^\// || $_[0] =~ /^[A-Z]:/i;
 }
 }

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: Digital signature

Re: [PATCH] Update SVN.pm

[Stepan Kasal]

Hello,

On Wed, Apr 16, 2014 at 12:13:21PM -0700, Junio C Hamano wrote:
 Interesting.  What other strange forms can they record in their
 repositories, I have to wonder.  Can they do
 2014-01-07T5:8:6.048176Z
 for example?

Roman Belinsky, the author of this fix, witnessed after large scale
conversion that the problem happens with the hour part only.
(SVN commits from the same origin did this with hours but not with
minutes.)  Recorded here:
https://github.com/msysgit/git/pull/126#discussion_r9661916

 I am wondering if it is simpler and less error prone
 to turn all these we only accept two digits into \d+ not only
 for the hour part but also minute and second parts.

But Roman's proposed regexp nicely shows 1) what the standard is and
2) what is the deviation.

Have a nice day,
  Stepan Kasal

  Signed-off-by: Stepan Kasal ka...@ucw.cz
  ---
   perl/Git/SVN.pm | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)
 
  diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
  index a59564f..09cff13 100644
  --- a/perl/Git/SVN.pm
  +++ b/perl/Git/SVN.pm
  @@ -1321,7 +1321,7 @@ sub get_untracked {
   sub parse_svn_date {
  my $date = shift || return '+ 1970-01-01 00:00:00';
  my ($Y,$m,$d,$H,$M,$S) = ($date =~ /^(\d{4})\-(\d\d)\-(\d\d)T
  -   (\d\d)\:(\d\d)\:(\d\d)\.\d*Z$/x) or
  +   (\d\d?)\:(\d\d)\:(\d\d)\.\d*Z$/x) or
   croak Unable to parse date: $date\n;
  my $parsed_date;# Set next.
 --
 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/3] extract proper comments for l10n translators

[Jiang Xin]

When generate git.pot, many irrelevant comments are extracted as references
for translators, but one useful comment is lost.  This series patches will
fix this issue.

Jiang Xin (3):
  i18n: Fixes uncatchable comments for translators
  i18n: Only extract comments marked by special tag
  i18n: Remove obsolete comments for translators

 Makefile  | 2 +-
 builtin/init-db.c | 8 +++-
 date.c| 2 +-
 diff.c| 8 
 4 files changed, 5 insertions(+), 15 deletions(-)

-- 
1.9.2.461.g942803f

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] i18n: Fixes uncatchable comments for translators

[Jiang Xin]

Comment for l10n translators can not be extracted by xgettext if it is
not right above the l10n tag.  Moving the comment right before the l10n
tag will fix this issue.

Reported-by: Brian Gesiak modoca...@gmail.com
Signed-off-by: Jiang Xin worldhello@gmail.com
---
 date.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/date.c b/date.c
index e1a2cee..782de95 100644
--- a/date.c
+++ b/date.c
@@ -144,8 +144,8 @@ void show_date_relative(unsigned long time, int tz,
if (months) {
struct strbuf sb = STRBUF_INIT;
strbuf_addf(sb, Q_(%lu year, %lu years, years), 
years);
-   /* TRANSLATORS: %s is n years */
strbuf_addf(timebuf,
+/* TRANSLATORS: %s is n years */
 Q_(%s, %lu month ago, %s, %lu months ago, 
months),
 sb.buf, months);
strbuf_release(sb);
-- 
1.9.2.461.g942803f

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] i18n: Remove obsolete comments for translators

[Jiang Xin]

Since we do not translate diffstat any more, remove the obsolete comments.

Signed-off-by: Jiang Xin worldhello@gmail.com
---
 diff.c | 8 
 1 file changed, 8 deletions(-)

diff --git a/diff.c b/diff.c
index 539997f..54d5308 100644
--- a/diff.c
+++ b/diff.c
@@ -1461,20 +1461,12 @@ int print_stat_summary(FILE *fp, int files, int 
insertions, int deletions)
 * but nothing about added/removed lines? Is this a bug in Git?).
 */
if (insertions || deletions == 0) {
-   /*
-* TRANSLATORS: + in (+) is a line addition marker;
-* do not translate it.
-*/
strbuf_addf(sb,
(insertions == 1) ? , %d insertion(+) : , %d 
insertions(+),
insertions);
}
 
if (deletions || insertions == 0) {
-   /*
-* TRANSLATORS: - in (-) is a line removal marker;
-* do not translate it.
-*/
strbuf_addf(sb,
(deletions == 1) ? , %d deletion(-) : , %d 
deletions(-),
deletions);
-- 
1.9.2.461.g942803f

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] i18n: Only extract comments marked by special tag

[Jiang Xin]

When extract l10n messages, we use --add-comments option to keep
comments right above the l10n messages for references.  But sometimes
irrelevant comments are also extracted.  For example in the following
code block, the comment in line 2 will be extracted as comment for the
l10n message in line 3, but obviously it's wrong.

{ OPTION_CALLBACK, 0, ignore-removal, addremove_explicit,
  NULL /* takes no arguments */,
  N_(ignore paths removed in the working tree (same as
  --no-all)),
  PARSE_OPT_NOARG, ignore_removal_cb },

Since almost all comments for l10n translators are marked with the same
prefix (tag): TRANSLATORS:, it's safe to only extract comments with
this special tag.  I.E. it's better to call xgettext as:

xgettext --add-comments=TRANSLATORS: ...

Also tweaks the multi-line comment in init-db.c, to make it start with
the proper tag, not * TRANSLATORS: (which has a star before the tag).

Signed-off-by: Jiang Xin worldhello@gmail.com
---
 Makefile  | 2 +-
 builtin/init-db.c | 8 +++-
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/Makefile b/Makefile
index 2128ce3..a53f3a8 100644
--- a/Makefile
+++ b/Makefile
@@ -2102,7 +2102,7 @@ pdf:
 
 XGETTEXT_FLAGS = \
--force-po \
-   --add-comments \
+   --add-comments=TRANSLATORS: \
--msgid-bugs-address=Git Mailing List git@vger.kernel.org \
--from-code=UTF-8
 XGETTEXT_FLAGS_C = $(XGETTEXT_FLAGS) --language=C \
diff --git a/builtin/init-db.c b/builtin/init-db.c
index c7c76bb..56f85e2 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -412,11 +412,9 @@ int init_db(const char *template_dir, unsigned int flags)
if (!(flags  INIT_DB_QUIET)) {
int len = strlen(git_dir);
 
-   /*
-* TRANSLATORS: The first '%s' is either Reinitialized
-* existing or Initialized empty, the second  shared or
-* , and the last '%s%s' is the verbatim directory name.
-*/
+   /* TRANSLATORS: The first '%s' is either Reinitialized
+  existing or Initialized empty, the second  shared or
+  , and the last '%s%s' is the verbatim directory name. */
printf(_(%s%s Git repository in %s%s\n),
   reinit ? _(Reinitialized existing) : _(Initialized 
empty),
   shared_repository ? _( shared) : ,
-- 
1.9.2.461.g942803f

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] extract proper comments for l10n translators

[Jiang Xin]

2014-04-17 13:37 GMT+08:00 Jiang Xin worldhello@gmail.com:
 When generate git.pot, many irrelevant comments are extracted as references
 for translators, but one useful comment is lost.  This series patches will
 fix this issue.

Brief changes of po/git.pot after applied these patches:

diff --git a/po/git.pot b/po/git.pot
index f70b46b..9e474f3 100644
--- a/po/git.pot
+++ b/po/git.pot
@@ -23,10 +23,6 @@ msgstr 
 msgid hint: %.*s\n
 msgstr 

-#.
-#. * Message used both when 'git commit' fails and when
-#. * other commands doing a merge do.
-#.
 #: advice.c:85
 msgid 
 Fix them up in the work tree,\n
@@ -399,6 +395,7 @@ msgid_plural %lu years
 msgstr[0] 
 msgstr[1] 

+#. TRANSLATORS: %s is n years
 #: date.c:149
 #, c-format
 msgid %s, %lu month ago
@@ -582,8 +579,6 @@ msgstr 
 msgid Removing %s to make room for subdirectory\n
 msgstr 

-#. something else exists
-#. .. but not some other error (who really cares what?)
 #: merge-recursive.c:700 merge-recursive.c:721
 msgid : perhaps a D/F conflict?
 msgstr 
@@ -899,11 +894,6 @@ msgstr 
 msgid Pathspec '%s' is in submodule '%.*s'
 msgstr 

-#.
-#. * We may want to substitute this command with a command
-#. * name. E.g. when add--interactive dies when running
-#. * checkout -p
-#.
 #: pathspec.c:353
 #, c-format
 msgid %s: pathspec magic not supported by this command: %s
@@ -953,11 +943,6 @@ msgstr 
 msgid %s tracks both %s and %s
 msgstr 

-#.
-#. * This last possibility doesn't occur because
-#. * FETCH_HEAD_IGNORE entries always appear at
-#. * the end of the list.
-#.
 #: remote.c:774
 msgid Internal error
 msgstr 
@@ -1306,13 +1291,11 @@ msgstr 
 msgid Could not find section in .gitmodules where path=%s
 msgstr 

-#. Maybe the user already did that, don't error out here
 #: submodule.c:76
 #, c-format
 msgid Could not update .gitmodules entry %s
 msgstr 

-#. Maybe the user already did that, don't error out here
 #: submodule.c:109
 #, c-format
 msgid Could not remove .gitmodules entry for %s
@@ -1884,7 +1867,6 @@ msgstr 
 msgid add changes from all tracked and untracked files
 msgstr 

-#. takes no arguments
 #: builtin/add.c:260
 msgid ignore paths removed in the working tree (same as --no-all)
 msgstr 
@@ -2044,7 +2026,6 @@ msgstr 
 msgid corrupt binary patch at line %d: %.*s
 msgstr 

-#. there has to be one hunk (forward hunk)
 #: builtin/apply.c:1900
 #, c-format
 msgid unrecognized binary patch at line %d
@@ -2232,7 +2213,6 @@ msgstr 
 msgid internal error
 msgstr 

-#. Say this even without --verbose
 #: builtin/apply.c:4043
 #, c-format
 msgid Applying patch %%s with %d reject...
@@ -3232,7 +3212,6 @@ msgstr 
 msgid  ... and %d more.\n
 msgstr 

-#. The singular version
 #: builtin/checkout.c:711
 #, c-format
 msgid 
@@ -3280,7 +3259,6 @@ msgstr 
 msgid invalid reference: %s
 msgstr 

-#. case (1): want a tree
 #: builtin/checkout.c:1002
 #, c-format
 msgid reference is not a tree: %s
@@ -4276,7 +4254,6 @@ msgstr 
 msgid GPG sign commit
 msgstr 

-#. end commit message options
 #: builtin/commit.c:1508
 msgid Commit contents options
 msgstr 
@@ -5140,7 +5117,6 @@ msgstr 
 msgid See \git help gc\ for manual housekeeping.\n
 msgstr 

-#. be quiet on --auto
 #: builtin/gc.c:336
 #, c-format
 msgid 
@@ -5894,12 +5870,10 @@ msgstr 
 msgid unable to move %s to %s
 msgstr 

-#.
-#. * TRANSLATORS: The first '%s' is either Reinitialized
-#. * existing or Initialized empty, the second  shared or
-#. * , and the last '%s%s' is the verbatim directory name.
-#.
-#: builtin/init-db.c:420
+#. TRANSLATORS: The first '%s' is either Reinitialized
+#. existing or Initialized empty, the second  shared or
+#. , and the last '%s%s' is the verbatim directory name.
+#: builtin/init-db.c:418
 #, c-format
 msgid %s%s Git repository in %s%s\n
 msgstr 
@@ -6627,7 +6601,6 @@ msgstr 
 msgid Commit %s has a bad GPG signature allegedly by %s.
 msgstr 

-#. 'N'
 #: builtin/merge.c:1279
 #, c-format
 msgid Commit %s does not have a GPG signature.
@@ -9593,8 +9566,6 @@ msgstr 
 msgid 'git bisect bad' can take only one argument.
 msgstr 

-#. have bad but not good.  we could bisect although
-#. this is less optimum.
 #: git-bisect.sh:273
 msgid Warning: bisecting only with a bad commit.
 msgstr 
@@ -9690,10 +9661,6 @@ msgstr 
 msgid updating an unborn branch with changes added to the index
 msgstr 

-#. The fetch involved updating the current branch.
-#. The working tree and the index file is still based on the
-#. $orig_head commit, but we are merging into $curr_head.
-#. First update the working tree to match $curr_head.
 #: git-pull.sh:271
 #, sh-format
 msgid 
@@ -9835,7 +9802,6 @@ msgstr 
 msgid Changes from $mb to $onto:
 msgstr 

-#. Detach HEAD and reset the tree
 #: git-rebase.sh:609
 msgid First, rewinding head to replay your work on top of it...
 msgstr 
@@ -10218,7 +10184,6 @@ msgstr 
 msgid The --cached option cannot be used with the --files option
 msgstr 

-#. unexpected type
 #: git-submodule.sh:1097
 #, sh-format
 msgid unexpected mode $mod_dst


-- 
Jiang Xin

[RFC] Speed up git status by caching untracked file info

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

This patch serves as a heads up about a feature I'm working on. I hope
that by posting it early, people could double check if I have made
some fundamental mistakes that completely ruin the idea. It's about
speeding up git status by caching untracked file info in the index
_if_ your file system supports it (more below).

The whole WIP series is at

https://github.com/pclouds/git/commits/untracked-cache

I only post the real meat here. I'm aware of a few incomplete details
in this patch, but nothing fundamentally wrong. So far the numbers are
promising.  ls-files is updated to run fill_directory() twice in a
row and ls-files -o --directory --no-empty-directory --exclude-standard
(with gcc -O0) gives me:

   first run  second (cached) run
gentoo-x86500 ms 71.6  ms
wine  140 ms  9.72 ms
webkit125 ms  6.88 ms
linux-2.6 106 ms 16.2  ms

Basically untracked time is cut to one tenth in the best case
scenario. The final numbers would be a bit higher because I haven't
stored or read the cache from index yet. Real commit message follows..


read_directory() plays a bit part in the slowness of git status
because it has to read every directory and check for excluded entries,
which is really expensive. This patch adds an option to cache the
results so that after the first slow read_directory(), the following
calls should be cheap and fast.

The following inputs are sufficient to determine what files in a
directory are excluded:

 - The list of files and directories of the direction in question
 - The $GIT_DIR/index
 - The content of $GIT_DIR/info/exclude
 - The content of core.excludesfile
 - The content (or the lack) of .gitignore of all parent directories
   from $GIT_WORK_TREE

If we can cheaply validate all those inputs for a certain directory,
we are sure that the current code will always produce the same
results, so we can cache and reuse those results.

This is not a silver bullet approach. When you compile a C file, for
example, the old .o file is removed and a new one with the same name
created, effectively invalidating the containing directory's
cache. But at least with a large enough work tree, there could be many
directories you never touch. The cache could help there.

The first input can be checked using directory mtime. In many
filesystems, directory mtime is updated when direct files/dirs are
added or removed (*). If you do not use such a file system, this
feature is not for you.

The second one can be hooked from read-cache.c. Whenever a file (or a
submodule) is added or removed from a directory, we invalidate that
directory. This will be done in a later patch.

The remaining inputs are easy, their SHA-1 could be used to verify
their contents. We do need to read .gitignore files and digest
them. But they are usually few and small, so the overhead should not
be much.

At the implementation level, the whole directory structure is saved,
each directory corresponds to one struct untracked_dir. Each directory
holds SHA-1 of the .gitignore underneath (or null if it does not
exist) and the list of untracked files and subdirs that need to
recurse into if all is well. Untracked subdirectories are saved in the
file queue and are the reason of quoting files in the previous
sentence.

On the first run, no untracked_dir is valid, the default code path is
run. prep_exclude() is updated to record SHA-1 of .gitignore along the
way. read_directory_recursive() is updated to record untracked files.

On subsequent runs, read_directory_recursive() reads stat info of the
directory in question and verifies if files/dirs have been added or
removed. With the help of prep_exclude() to verify .gitignore chain,
it may decide all is well and enable the fast path in
treat_path(). read_directory_recursive() is still called for
subdirectories even in fast path, because a directory mtime does not
cover all subdirs recursively.

So if all is really well, read_directory() becomes a series of
open(.gitignore), read(.gitignore), close(), hash_sha1_file() and
stat(dir) _without_ heavyweight exclude filtering. There should be
no overhead if this feature is disabled.

(*) Looking at the code in linux-2.6, ext* family seems to expose this
behavior. vfat also does (but not so sure about fat). btrfs probably
does. statfs() could be used to detect file system.

Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
 dir.c | 336 +-
 dir.h |  31 ++
 2 files changed, 343 insertions(+), 24 deletions(-)

diff --git a/dir.c b/dir.c
index bd58c14..f5d6315 100644
--- a/dir.c
+++ b/dir.c
@@ -31,8 +31,23 @@ enum path_treatment {
path_untracked
 };
 
+struct cached_dir {
+   DIR *fdir;
+   struct untracked_dir *untracked;
+   int nr_files;
+   int nr_dirs;
+
+   /*
+* return data from read_cached_dir(). name and state are only
+* valid if de is NULL
+*/
+